IB/srp: implement SRP_CRED_REQ

This patch enables allocation of request and response information units on the send queue instead of only requests, and implements processing of SRP_CRED_REQ information units. Also, declarations have been added to include/scsi/srp.h for the SRP_CRED_REQ and SRP_CRED_RSP information units.

+/*
+ * The SRP spec defines the size of the CRED_REQ structure to be 16 bytes,
+ * so it needs to be packed to avoid having it padded to 24 bytes on
+ * 64-bit architectures.
+ */

On Mon, 2010-08-02 at 17:32 +0200, Bart Van Assche wrote:
[ ... ]

There seems to be an accounting change from using tx_head/tx_tail to using tx_req and tx_rsp. I'm not sure that really makes sense -- splitting that into its own patch would allow for an easier review and may make it obvious if it is the right thing to do.

SRP_AER_REQ isn't handled, and it wouldn't be much to add it. That would improve our standards compatibility.

Quoted 7 lines · David Dillow

On Mon, 2010-08-02 at 17:32 +0200, Bart Van Assche wrote:
[ ... ]

There seems to be an accounting change from using tx_head/tx_tail to using tx_req and tx_rsp. I'm not sure that really makes sense -- splitting that into its own patch would allow for an easier review and may make it obvious if it is the right thing to do.

As I wrote in [PATCH 0/4], both initator-to-target requests and initiator-to-target responses are now allocated from the same send queue. Since tx_head - tx_tail is now the sum of the number of requests and the number of responses allocated on the send queue, new variables had to be introduced in order to count how many requests and how many responses had been allocated on the send queue.

Quoted 2 lines

SRP_AER_REQ isn't handled, and it wouldn't be much to add it. That would improve our standards compatibility.

The message "Unhandled SRP opcode" is printed for SRP_AER_REQ information units. Support for SRP_AER_REQ can still be added in a follow-up patch.

Quoted 9 lines · Bart Van Assche

Quoted 7 lines · David Dillow

On Mon, 2010-08-02 at 17:32 +0200, Bart Van Assche wrote:
[ ... ]

There seems to be an accounting change from using tx_head/tx_tail to using tx_req and tx_rsp. I'm not sure that really makes sense -- splitting that into its own patch would allow for an easier review and may make it obvious if it is the right thing to do.

Quoted 6 lines

As I wrote in [PATCH 0/4], both initator-to-target requests and initiator-to-target responses are now allocated from the same send queue. Since tx_head - tx_tail is now the sum of the number of requests and the number of responses allocated on the send queue, new variables had to be introduced in order to count how many requests and how many responses had been allocated on the send queue.

I understood that you were allocating from the same queue of buffers, I'm just not sure you need to keep track separately. We already hold SRP_RSP_SQ_SIZE buffers back from sending requests, so we will always have those (well, one) available for sending a response.

If we use too many for responses, that's not great, but it isn't completely fatal -- srp_queuecommand() will bounce the command back to the mid-layer with a BUSY status, and while the error handling (task mgmt) will go to the big guns and reset the connection sooner, we will recover.

If there isn't a buffer available to send the response, it'll get dropped on the floor, just like in your patch.

I just don't see a reason for the accounting change.

Quoted 14 lines · David Dillow

I understood that you were allocating from the same queue of buffers, I'm just not sure you need to keep track separately. We already hold SRP_RSP_SQ_SIZE buffers back from sending requests, so we will always have those (well, one) available for sending a response.

If we use too many for responses, that's not great, but it isn't completely fatal -- srp_queuecommand() will bounce the command back to the mid-layer with a BUSY status, and while the error handling (task mgmt) will go to the big guns and reset the connection sooner, we will recover.

If there isn't a buffer available to send the response, it'll get dropped on the floor, just like in your patch.

Quoted 1 line

I just don't see a reason for the accounting change.

I'm not sure it is a good idea to allow that all transmit buffers get allocated for sending CMD_RSP information units and that none remain for replying to incoming SRP_CRED_REQ / SRP_AER_REQ / SRP_T_LOGOUT requests. Wouldn't that be a protocol violation ?

[ ... ]

If SRQ_SQ_SIZE is 64, we set .can_queue to 63 which will leave a buffer available for the reply.

Now, if the target sends us multiple SRP_CRED_REQ without waiting for a reply, then we could use up buffers and not be able to send requests, but that would be a protocol violation on the target's side.

On Tue, 2010-08-03 at 17:26 +0200, Bart Van Assche wrote:
[ ... ]

Quoted 4 lines

I'm not sure it is a good idea to allow that all transmit buffers get allocated for sending CMD_RSP information units and that none remain for replying to incoming SRP_CRED_REQ / SRP_AER_REQ / SRP_T_LOGOUT requests. Wouldn't that be a protocol violation ?

You mean SRP_CMD? We don't do that, we clamp the outstanding requests we generate so we'll always have a buffer to reply to SRP_CRED_REQ et al.

If SRQ_SQ_SIZE is 64, we set .can_queue to 63 which will leave a buffer available for the reply.

Now, if the target sends us multiple SRP_CRED_REQ without waiting for a reply, then we could use up buffers and not be able to send requests, but that would be a protocol violation on the target's side.

Quoted 11 lines · David Dillow

On Tue, 2010-08-03 at 17:26 +0200, Bart Van Assche wrote:
[ ... ]

Quoted 4 lines

I'm not sure it is a good idea to allow that all transmit buffers get allocated for sending CMD_RSP information units and that none remain for replying to incoming SRP_CRED_REQ / SRP_AER_REQ / SRP_T_LOGOUT requests. Wouldn't that be a protocol violation ?

You mean SRP_CMD? We don't do that, we clamp the outstanding requests we generate so we'll always have a buffer to reply to SRP_CRED_REQ et al.

Quoted 6 lines

If SRQ_SQ_SIZE is 64, we set .can_queue to 63 which will leave a buffer available for the reply.

Now, if the target sends us multiple SRP_CRED_REQ without waiting for a reply, then we could use up buffers and not be able to send requests, but that would be a protocol violation on the target's side.

(resending as plain text)

I have been looking further into this. There is a slight asymmetry in the current transmit buffer allocation code: one buffer element is reserved on the target for task management requests but not on the initiator. So - at least in theory - it is possible that all elements of the initiator transmit ring are allocated for SRP_CMD requests at the time an SRP_TSK_MGMT request should be sent. Wouldn't it be more symmetric to change __srp_get_tx_iu() as follows (diff against the current for-next branch) ?

Quoted 21 lines · Bart Van Assche

Quoted 11 lines · David Dillow

On Tue, 2010-08-03 at 17:26 +0200, Bart Van Assche wrote:
[ ... ]

Quoted 4 lines

I'm not sure it is a good idea to allow that all transmit buffers get allocated for sending CMD_RSP information units and that none remain for replying to incoming SRP_CRED_REQ / SRP_AER_REQ / SRP_T_LOGOUT requests. Wouldn't that be a protocol violation ?

You mean SRP_CMD? We don't do that, we clamp the outstanding requests we generate so we'll always have a buffer to reply to SRP_CRED_REQ et al.

Quoted 6 lines

If SRQ_SQ_SIZE is 64, we set .can_queue to 63 which will leave a buffer available for the reply.

Now, if the target sends us multiple SRP_CRED_REQ without waiting for a reply, then we could use up buffers and not be able to send requests, but that would be a protocol violation on the target's side.

(resending as plain text)

Quoted 8 lines

I have been looking further into this. There is a slight asymmetry in the current transmit buffer allocation code: one buffer element is reserved on the target for task management requests but not on the initiator. So - at least in theory - it is possible that all elements of the initiator transmit ring are allocated for SRP_CMD requests at the time an SRP_TSK_MGMT request should be sent. Wouldn't it be more symmetric to change __srp_get_tx_iu() as follows (diff against the current for-next branch) ?

This is not needed in the code as it currently stands. We cannot have all elements of the tx ring allocated for SRP_CMD requests, because we don't send one if we don't have a credit for it, and we save a credit for the SRP_TSK_MGMT request. Hence, there will always be a buffer available for the management request.

We don't need it for when we start replying to target requests in the future, because we already reserve that buffer by limiting our view of credits to one less than the actual ring size.

Quoted 37 lines · David Dillow

Quoted 21 lines · Bart Van Assche

Quoted 11 lines · David Dillow

On Tue, 2010-08-03 at 17:26 +0200, Bart Van Assche wrote:
[ ... ]

Quoted 4 lines

I'm not sure it is a good idea to allow that all transmit buffers get allocated for sending CMD_RSP information units and that none remain for replying to incoming SRP_CRED_REQ / SRP_AER_REQ / SRP_T_LOGOUT requests. Wouldn't that be a protocol violation ?

You mean SRP_CMD? We don't do that, we clamp the outstanding requests we generate so we'll always have a buffer to reply to SRP_CRED_REQ et al.

Quoted 6 lines

If SRQ_SQ_SIZE is 64, we set .can_queue to 63 which will leave a buffer available for the reply.

Now, if the target sends us multiple SRP_CRED_REQ without waiting for a reply, then we could use up buffers and not be able to send requests, but that would be a protocol violation on the target's side.

(resending as plain text)

Quoted 8 lines

I have been looking further into this. There is a slight asymmetry in the current transmit buffer allocation code: one buffer element is reserved on the target for task management requests but not on the initiator. So - at least in theory - it is possible that all elements of the initiator transmit ring are allocated for SRP_CMD requests at the time an SRP_TSK_MGMT request should be sent. Wouldn't it be more symmetric to change __srp_get_tx_iu() as follows (diff against the current for-next branch) ?

This is not needed in the code as it currently stands. We cannot have all elements of the tx ring allocated for SRP_CMD requests, because we don't send one if we don't have a credit for it, and we save a credit for the SRP_TSK_MGMT request. Hence, there will always be a buffer available for the management request.

Quoted 3 lines

We don't need it for when we start replying to target requests in the future, because we already reserve that buffer by limiting our view of credits to one less than the actual ring size.

It is not needed as long as the target respects the limit of at most one outstanding request, as required by the SRP (draft) standard. The test in question is a single if-statement that provides some additional robustness and no measurable overhead. So I don't see why it should be removed.