[v2,1/3] scsi_cmnd: Introduce scsi_transfer_length helper

Commit Message

Sagi Grimberg June 24, 2014, 4:27 p.m. UTC

On 6/24/2014 7:08 PM, Michael Christie wrote:
> On Jun 24, 2014, at 7:53 AM, Martin K. Petersen <[e-mail]> wrote:
>
>>>>>>> "Mike" == Mike Christie <[e-mail]> writes:
>> Mike> The problem is WRITE_SAME requests are setup so that
>> Mike> req->__data_len is the value of the entire request when the setup
>> Mike> is completed but during the setup process it's value changes
>>
>> Oh, I see. So things break because iSCSI uses scsi_transfer_length()
>> where the scatterlist length was used in the past.
>>
>> How about this?
>>
>>
>> SCSI: Use SCSI data buffer length to extract transfer size
>>
>> Commit 8846bab180fa introduced a helper that can be used to query the
>> wire transfer size for a SCSI command taking protection information into
>> account.
>>
>> However, some commands do not have a 1:1 mapping between the block range
>> they work on and the payload size (discard, write same). After the
>> scatterlist has been set up these requests use __data_len to store the
>> number of bytes to report completion on. This means that callers of
>> scsi_transfer_length() would get the wrong byte count for these types of
>> requests.
>>
>> To overcome this we make scsi_transfer_length() use the scatterlist
>> length in the scsi_data_buffer as basis for the wire transfer
>> calculation instead of __data_len.
>>
>> Reported-by: Christoph Hellwig <[e-mail]>
>> Debugged-by: Mike Christie <[e-mail]>
>> Signed-off-by: Martin K. Petersen <[e-mail]>
>>
>> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
>> index 42ed789ebafc..e0ae71098144 100644
>> --- a/include/scsi/scsi_cmnd.h
>> +++ b/include/scsi/scsi_cmnd.h
>> @@ -318,7 +318,7 @@ static inline void set_driver_byte(struct scsi_cmnd *cmd, char status)
>>
>> static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
>> {
>> -	unsigned int xfer_len = blk_rq_bytes(scmd->request);
>> +	unsigned int xfer_len = scsi_out(scmd)->length;
>> 	unsigned int prot_op = scsi_get_prot_op(scmd);
>> 	unsigned int sector_size = scmd->device->sector_size;
>
> Do we need to check for the data direction. Something like
>
> if (scmd->sc_data_direction == DMA_TO_DEVICE)
> 	xfer_len = scsi_out(scmnd)->length;
> else
> 	xfer_len = scsi_in(scmnd)->length;

This condition only matters in the bidi case, which is not relevant for 
the PI case.
I suggested to condition that in libiscsi (posted in the second thread, 
copy-paste below).
Although I do agree that scsi_transfer_length() helper is not really 
just for PI and not more.
I think Mike's way is cleaner.


                 struct iscsi_r2t_info *r2t = &task->unsol_r2t;

Sagi.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Christoph Hellwig June 24, 2014, 4:30 p.m. UTC | #1

On Tue, Jun 24, 2014 at 07:27:46PM +0300, Sagi Grimberg wrote:
> This condition only matters in the bidi case, which is not relevant for the
> PI case.
> I suggested to condition that in libiscsi (posted in the second thread,
> copy-paste below).
> Although I do agree that scsi_transfer_length() helper is not really just
> for PI and not more.
> I think Mike's way is cleaner.

But for bidi there are two transfers.  So either scsi_transfer_length()
needs to take the scsi_data_buffer, or we need to avoid using it.

For 3.16 I'd prefer something like you're patch below.  This patch which
has been rushed in last minute and not through the scsi tree has already
causes enough harm.  If you can come up with a clean version to
transparently handle the bidi case I'd be happy to pick that up for
3.17.

In the meantime please provide a version of the patch below with a
proper description and signoff.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mike Christie June 24, 2014, 5 p.m. UTC | #2

On 06/24/2014 11:30 AM, Christoph Hellwig wrote:
> On Tue, Jun 24, 2014 at 07:27:46PM +0300, Sagi Grimberg wrote:
>> This condition only matters in the bidi case, which is not relevant for the
>> PI case.
>> I suggested to condition that in libiscsi (posted in the second thread,
>> copy-paste below).
>> Although I do agree that scsi_transfer_length() helper is not really just
>> for PI and not more.
>> I think Mike's way is cleaner.
> 
> But for bidi there are two transfers.  So either scsi_transfer_length()
> needs to take the scsi_data_buffer, or we need to avoid using it.
> 
> For 3.16 I'd prefer something like you're patch below.  This patch which
> has been rushed in last minute and not through the scsi tree has already
> causes enough harm.  If you can come up with a clean version to
> transparently handle the bidi case I'd be happy to pick that up for
> 3.17.
> 
> In the meantime please provide a version of the patch below with a
> proper description and signoff.
> 

It would be nice to just have one function to call and it just do the
right thing for the drivers. I am fine with Sagi's libiscsi patch for
now though:

Acked-by: Mike Christie <[e-mail]>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Martin K. Petersen June 24, 2014, 5:04 p.m. UTC | #3

>>>>> "Mike" == Mike Christie <[e-mail]> writes:

Mike> It would be nice to just have one function to call and it just do
Mike> the right thing for the drivers.

But what is the right thing when there are buffers for both directions?

Mike Christie June 24, 2014, 5:08 p.m. UTC | #4

On 06/24/2014 12:00 PM, Mike Christie wrote:
> On 06/24/2014 11:30 AM, Christoph Hellwig wrote:
>> On Tue, Jun 24, 2014 at 07:27:46PM +0300, Sagi Grimberg wrote:
>>> This condition only matters in the bidi case, which is not relevant for the
>>> PI case.
>>> I suggested to condition that in libiscsi (posted in the second thread,
>>> copy-paste below).
>>> Although I do agree that scsi_transfer_length() helper is not really just
>>> for PI and not more.
>>> I think Mike's way is cleaner.
>>
>> But for bidi there are two transfers.  So either scsi_transfer_length()
>> needs to take the scsi_data_buffer, or we need to avoid using it.
>>
>> For 3.16 I'd prefer something like you're patch below.  This patch which
>> has been rushed in last minute and not through the scsi tree has already
>> causes enough harm.  If you can come up with a clean version to
>> transparently handle the bidi case I'd be happy to pick that up for
>> 3.17.
>>
>> In the meantime please provide a version of the patch below with a
>> proper description and signoff.
>>
> 
> It would be nice to just have one function to call and it just do the
> right thing for the drivers. I am fine with Sagi's libiscsi patch for
> now though:
> 
> Acked-by: Mike Christie <[e-mail]>

Actually, let me take this back for a second. I am not sure if that is
right.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 3f46234..abf0c3e 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -386,12 +386,14 @@  static int iscsi_prep_scsi_cmd_pdu(struct 
iscsi_task *task)
                 rc = iscsi_prep_bidi_ahs(task);
                 if (rc)
                         return rc;
+               transfer_length = scsi_in(sc)->length;
+       } else {
+               transfer_length = scsi_transfer_length(sc);
         }

         if (scsi_get_prot_op(sc) != SCSI_PROT_NORMAL)
                 task->protected = true;

-       transfer_length = scsi_transfer_length(sc);
         hdr->data_length = cpu_to_be32(transfer_length);
         if (sc->sc_data_direction == DMA_TO_DEVICE) {