Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

[sagi grimberg <sagig@mellanox.com>]

On 6/24/2014 9:54 AM, Mike Christie wrote:
> On 06/11/2014 04:09 AM, Sagi Grimberg wrote:
>> In case protection information exists on the wire
>> scsi transports should include it in the transfer
>> byte count (even if protection information does not
>> exist in the host memory space). This helper will
>> compute the total transfer length from the scsi
>> command data length and protection attributes.
>>
>> Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
>> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
>> ---
>>   include/scsi/scsi_cmnd.h |   17 +++++++++++++++++
>>   1 files changed, 17 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
>> index dd7c998..a100c6e 100644
>> --- a/include/scsi/scsi_cmnd.h
>> +++ b/include/scsi/scsi_cmnd.h
>> @@ -7,6 +7,7 @@
>>   #include <linux/types.h>
>>   #include <linux/timer.h>
>>   #include <linux/scatterlist.h>
>> +#include <scsi/scsi_device.h>
>>   
>>   struct Scsi_Host;
>>   struct scsi_device;
>> @@ -306,4 +307,20 @@ static inline void set_driver_byte(struct scsi_cmnd *cmd, char status)
>>   	cmd->result = (cmd->result & 0x00ffffff) | (status << 24);
>>   }
>>   
>> +static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
>> +{
>> +	unsigned int xfer_len = blk_rq_bytes(scmd->request);
>> +	unsigned int prot_op = scsi_get_prot_op(scmd);
>> +	unsigned int sector_size = scmd->device->sector_size;
>> +
>> +	switch (prot_op) {
>> +	case SCSI_PROT_NORMAL:
>> +	case SCSI_PROT_WRITE_STRIP:
>> +	case SCSI_PROT_READ_INSERT:
>> +		return xfer_len;
>> +	}
>> +
>> +	return xfer_len + (xfer_len >> ilog2(sector_size)) * 8;
>> +}
>> +
>>   #endif /* _SCSI_SCSI_CMND_H */
>>
> I found the issue Christoph is hitting in the other thread.
>
> The problem is WRITE_SAME requests are setup so that req->__data_len is
> the value of the entire request when the setup is completed but during
> the setup process it's value changes
>
> So __data_len could be thousands of bytes but
> scsi_out(scsi_cmnd)->length for this case was only returning 512 which
> is the sector size. This is because sd_setup_-write_same_cmnd does:
>
>
> rq->__data_len = sdp->sector_size;
> ....
> scsi_setup_blk_pc_cmnd()
> ....
> rq->__data_len = nr_bytes;
>
> and scsi_setup_blk_pc_cmnd does scsi_init_io() -> scsi_init_sgtable()
> and that does
>
> sdb->length = blk_rq_bytes(req);
>
> and at this time because before we called scsi_setup_blk_pc_cmnd we set
> the __data_len to sector size, the sdb length is going to be only 512
> but the final request->__data_len is the total size of the operation.


Hey Christoph, Mike &MKP,

So just got first look in this thread, didn't have time to reproduce it yet.
Mike's analysis makes sense to me, I think the below change should 
resolve this one.

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index a100c6e..2afd9c2 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -309,7 +309,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_bufflen(scmd);
         unsigned int prot_op = scsi_get_prot_op(scmd);
         unsigned int sector_size = scmd->device->sector_size;


Moreover, since bidi and dif are adjacent, this will also needed:

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) {
                 struct iscsi_r2t_info *r2t = &task->unsol_r2t;

Let me test and queue it up.

Sagi.