[PATCH v1 0/3] Include protection information in iscsi header

[PATCH v1 0/3] Include protection information in iscsi header

[Sagi Grimberg]

At the SCSI transport level, there is no distinction between
user data and protection information. Thus, iscsi header field
"expected data transfer length" should include protection
information.

Patch #1 introduces scsi helpers scsi_transfer_length (compute
wire transfer byte count) and scsi_prot_len (compute protection
information byte count).

Patch #2 modifies iscsi initiator to set correct wire transfer length
in iscsi header data_length field (and modifies iser accordingly).

Patch #3 modifies target core to expect protection information included
in the wire transfer length (and modifies loopback transport to do so).

I have 2 patches that convert lpfc/qla2xxx drivers to use scsi helpers
but these are completely untested at the moment. Once we get this set
to land upstream, I can queue them up as RFCs.

Changes from v0:
- Introduce scsi helpers to compute correct transfer length in the
  presence of protection information (instead of having each transport
  doing the same computation).
- Modify iscsi to set correct transfer length using scsi helpers
- Modify loopback transport to set correct transfer length using
  scsi helpers

Sagi Grimberg (3):
  scsi_cmnd: Introduce scsi_transfer_length helper
  libiscsi, iser: Adjust data_length to include protection information
  TARGET/sbc,loopback: Adjust command data length in case pi exists on
    the wire

 drivers/infiniband/ulp/iser/iser_initiator.c |   34 ++++++----------------
 drivers/scsi/libiscsi.c                      |   18 ++++++------
 drivers/target/loopback/tcm_loop.c           |   15 ++++++++--
 drivers/target/target_core_sbc.c             |   15 ++++++++-
 include/scsi/scsi_cmnd.h                     |   39 ++++++++++++++++++++++++++
 5 files changed, 83 insertions(+), 38 deletions(-)

[PATCH v1 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

[Sagi Grimberg]

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>
---
 include/scsi/scsi_cmnd.h |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index dd7c998..84d9593 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,42 @@ static inline void set_driver_byte(struct scsi_cmnd *cmd, char status)
 	cmd->result = (cmd->result & 0x00ffffff) | (status << 24);
 }
 
+static inline unsigned scsi_prot_length(unsigned data_length,
+					unsigned sector_size)
+{
+	switch (sector_size) {
+	case 512:
+		return (data_length >> 9) * 8;
+	case 1024:
+		return (data_length >> 10) * 8;
+	case 2048:
+		return (data_length >> 11) * 8;
+	case 4096:
+		return (data_length >> 12) * 8;
+	default:
+		return (data_length >> ilog2(sector_size)) * 8;
+	}
+}
+
+static inline unsigned scsi_transfer_length(struct scsi_cmnd *cmd)
+{
+	unsigned data_length;
+
+	if (cmd->sc_data_direction == DMA_FROM_DEVICE) {
+		data_length = scsi_in(cmd)->length;
+		if (scsi_get_prot_op(cmd) ==  SCSI_PROT_NORMAL ||
+		    scsi_get_prot_op(cmd) ==  SCSI_PROT_READ_INSERT)
+			return data_length;
+	} else {
+		data_length = scsi_out(cmd)->length;
+		if (scsi_get_prot_op(cmd) ==  SCSI_PROT_NORMAL ||
+		    scsi_get_prot_op(cmd) ==  SCSI_PROT_WRITE_STRIP)
+			return data_length;
+	}
+
+	/* Protection information exists on the wire */
+	return data_length + scsi_prot_length(data_length,
+					      cmd->device->sector_size);
+}
+
 #endif /* _SCSI_SCSI_CMND_H */
-- 
1.7.1

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

[Martin K. Petersen]

>>>>> "Sagi" == Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> writes:

+static inline unsigned scsi_prot_length(unsigned data_length,
+					unsigned sector_size)
+{
+	switch (sector_size) {
+	case 512:
+		return (data_length >> 9) * 8;
+	case 1024:
+		return (data_length >> 10) * 8;
+	case 2048:
+		return (data_length >> 11) * 8;
+	case 4096:
+		return (data_length >> 12) * 8;
+	default:
+		return (data_length >> ilog2(sector_size)) * 8;
+	}
+}
+
+static inline unsigned scsi_transfer_length(struct scsi_cmnd *cmd)
+{
+	unsigned data_length;
+
+	if (cmd->sc_data_direction == DMA_FROM_DEVICE) {
+		data_length = scsi_in(cmd)->length;
+		if (scsi_get_prot_op(cmd) ==  SCSI_PROT_NORMAL ||
+		    scsi_get_prot_op(cmd) ==  SCSI_PROT_READ_INSERT)
+			return data_length;
+	} else {
+		data_length = scsi_out(cmd)->length;
+		if (scsi_get_prot_op(cmd) ==  SCSI_PROT_NORMAL ||
+		    scsi_get_prot_op(cmd) ==  SCSI_PROT_WRITE_STRIP)
+			return data_length;
+	}
+
+	/* Protection information exists on the wire */
+	return data_length + scsi_prot_length(data_length,
+					      cmd->device->sector_size);
+}

Let's do this for 3.16:

static inline unsigned int 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);

        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;
}

And then in 3.17 we'll have:

static inline unsigned int scsi_transfer_length(struct scsi_cmnd *scmd)
{
        unsigned int xfer_len = blk_rq_bytes(scmd->request);
 
        if (scsi_prot_flagged(SCSI_PROT_TRANSFER_PI))
           xfer_len += (xfer_len >> ilog2(scsi_prot_interval(scmd))) * 8;

        return xfer_len;
}

-- 
Martin K. Petersen	Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sagi Grimberg]

On 6/10/2014 10:02 PM, Martin K. Petersen wrote:
>>>>>> "Sagi" == Sagi Grimberg <sagig@mellanox.com> writes:
> +static inline unsigned scsi_prot_length(unsigned data_length,
> +					unsigned sector_size)
> +{
> +	switch (sector_size) {
> +	case 512:
> +		return (data_length >> 9) * 8;
> +	case 1024:
> +		return (data_length >> 10) * 8;
> +	case 2048:
> +		return (data_length >> 11) * 8;
> +	case 4096:
> +		return (data_length >> 12) * 8;
> +	default:
> +		return (data_length >> ilog2(sector_size)) * 8;
> +	}
> +}
> +
> +static inline unsigned scsi_transfer_length(struct scsi_cmnd *cmd)
> +{
> +	unsigned data_length;
> +
> +	if (cmd->sc_data_direction == DMA_FROM_DEVICE) {
> +		data_length = scsi_in(cmd)->length;
> +		if (scsi_get_prot_op(cmd) ==  SCSI_PROT_NORMAL ||
> +		    scsi_get_prot_op(cmd) ==  SCSI_PROT_READ_INSERT)
> +			return data_length;
> +	} else {
> +		data_length = scsi_out(cmd)->length;
> +		if (scsi_get_prot_op(cmd) ==  SCSI_PROT_NORMAL ||
> +		    scsi_get_prot_op(cmd) ==  SCSI_PROT_WRITE_STRIP)
> +			return data_length;
> +	}
> +
> +	/* Protection information exists on the wire */
> +	return data_length + scsi_prot_length(data_length,
> +					      cmd->device->sector_size);
> +}
>
> Let's do this for 3.16:
>
> static inline unsigned int 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);
>
>          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;
> }
>
> And then in 3.17 we'll have:
>
> static inline unsigned int scsi_transfer_length(struct scsi_cmnd *scmd)
> {
>          unsigned int xfer_len = blk_rq_bytes(scmd->request);
>   
>          if (scsi_prot_flagged(SCSI_PROT_TRANSFER_PI))
>             xfer_len += (xfer_len >> ilog2(scsi_prot_interval(scmd))) * 8;
>
>          return xfer_len;
> }
>

No problem,

I'll send out v2 tomorrow (your tonight...)

Thanks,
Sagi.

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

[Or Gerlitz]

On Tue, Jun 10, 2014 at 10:02 PM, Martin K. Petersen
<martin.petersen-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:
>>>>>> "Sagi" == Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> writes:
>
> +static inline unsigned scsi_prot_length(unsigned data_length,
> +                                       unsigned sector_size)
> +{
> +       switch (sector_size) {
> +       case 512:
> +               return (data_length >> 9) * 8;
> +       case 1024:
> +               return (data_length >> 10) * 8;
> +       case 2048:
> +               return (data_length >> 11) * 8;
> +       case 4096:
> +               return (data_length >> 12) * 8;
> +       default:
> +               return (data_length >> ilog2(sector_size)) * 8;
> +       }
> +}
> +
> +static inline unsigned scsi_transfer_length(struct scsi_cmnd *cmd)
> +{
> +       unsigned data_length;
> +
> +       if (cmd->sc_data_direction == DMA_FROM_DEVICE) {
> +               data_length = scsi_in(cmd)->length;
> +               if (scsi_get_prot_op(cmd) ==  SCSI_PROT_NORMAL ||
> +                   scsi_get_prot_op(cmd) ==  SCSI_PROT_READ_INSERT)
> +                       return data_length;
> +       } else {
> +               data_length = scsi_out(cmd)->length;
> +               if (scsi_get_prot_op(cmd) ==  SCSI_PROT_NORMAL ||
> +                   scsi_get_prot_op(cmd) ==  SCSI_PROT_WRITE_STRIP)
> +                       return data_length;
> +       }
> +
> +       /* Protection information exists on the wire */
> +       return data_length + scsi_prot_length(data_length,
> +                                             cmd->device->sector_size);
> +}
>
> Let's do this for 3.16:

Just to make sure, by 3.16 you also mean 3.15.y, right?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Martin K. Petersen]

>>>>> "Or" == Or Gerlitz <or.gerlitz@gmail.com> writes:

Or> Just to make sure, by 3.16 you also mean 3.15.y, right?

Yes.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

[Sagi Grimberg]

In various areas of the code, it is assumed that
se_cmd->data_length describes pure data. In case
that protection information exists over the wire
(protect bits is are on) the target core decrease
the protection length from the data length (instead
of each transport peeking in the cdb).

Modify loopback device to include protection information
in the transferred data length (like other scsi transports).

Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/target/loopback/tcm_loop.c |   15 ++++++++++++---
 drivers/target/target_core_sbc.c   |   15 +++++++++++++--
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index c886ad1..1f4c015 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -179,7 +179,7 @@ static void tcm_loop_submission_work(struct work_struct *work)
 	struct tcm_loop_hba *tl_hba;
 	struct tcm_loop_tpg *tl_tpg;
 	struct scatterlist *sgl_bidi = NULL;
-	u32 sgl_bidi_count = 0;
+	u32 sgl_bidi_count = 0, transfer_length;
 	int rc;
 
 	tl_hba = *(struct tcm_loop_hba **)shost_priv(sc->device->host);
@@ -213,12 +213,21 @@ static void tcm_loop_submission_work(struct work_struct *work)
 
 	}
 
-	if (!scsi_prot_sg_count(sc) && scsi_get_prot_op(sc) != SCSI_PROT_NORMAL)
+	transfer_length = scsi_transfer_length(sc);
+	if (!scsi_prot_sg_count(sc) &&
+	    scsi_get_prot_op(sc) != SCSI_PROT_NORMAL) {
 		se_cmd->prot_pto = true;
+		/*
+		 * loopback transport doesn't support
+		 * WRITE_GENERATE, READ_STRIP protection
+		 * information operations, go ahead unprotected.
+		 */
+		transfer_length = scsi_bufflen(sc);
+	}
 
 	rc = target_submit_cmd_map_sgls(se_cmd, tl_nexus->se_sess, sc->cmnd,
 			&tl_cmd->tl_sense_buf[0], tl_cmd->sc->device->lun,
-			scsi_bufflen(sc), tcm_loop_sam_attr(sc),
+			transfer_length, tcm_loop_sam_attr(sc),
 			sc->sc_data_direction, 0,
 			scsi_sglist(sc), scsi_sg_count(sc),
 			sgl_bidi, sgl_bidi_count,
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index e022959..06f8ecd 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -665,8 +665,19 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
 
 	cmd->prot_type = dev->dev_attrib.pi_prot_type;
 	cmd->prot_length = dev->prot_length * sectors;
-	pr_debug("%s: prot_type=%d, prot_length=%d prot_op=%d prot_checks=%d\n",
-		 __func__, cmd->prot_type, cmd->prot_length,
+
+	/**
+	 * In case protection information exists over the wire
+	 * we modify command data length to describe pure data.
+	 * The actual transfer length is data length + protection
+	 * length
+	 **/
+	if (protect)
+		cmd->data_length -= cmd->prot_length;
+
+	pr_debug("%s: prot_type=%d, data_length=%d, prot_length=%d "
+		 "prot_op=%d prot_checks=%d\n",
+		 __func__, cmd->prot_type, cmd->data_length, cmd->prot_length,
 		 cmd->prot_op, cmd->prot_checks);
 
 	return true;
-- 
1.7.1

Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

[Nicholas A. Bellinger]

Hi Sagi & Co,

On Sun, 2014-06-08 at 13:27 +0300, Sagi Grimberg wrote:
> In various areas of the code, it is assumed that
> se_cmd->data_length describes pure data. In case
> that protection information exists over the wire
> (protect bits is are on) the target core decrease
> the protection length from the data length (instead
> of each transport peeking in the cdb).
> 
> Modify loopback device to include protection information
> in the transferred data length (like other scsi transports).
> 
> Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
> ---
>  drivers/target/loopback/tcm_loop.c |   15 ++++++++++++---
>  drivers/target/target_core_sbc.c   |   15 +++++++++++++--
>  2 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
> index c886ad1..1f4c015 100644
> --- a/drivers/target/loopback/tcm_loop.c
> +++ b/drivers/target/loopback/tcm_loop.c
> @@ -179,7 +179,7 @@ static void tcm_loop_submission_work(struct work_struct *work)
>  	struct tcm_loop_hba *tl_hba;
>  	struct tcm_loop_tpg *tl_tpg;
>  	struct scatterlist *sgl_bidi = NULL;
> -	u32 sgl_bidi_count = 0;
> +	u32 sgl_bidi_count = 0, transfer_length;
>  	int rc;
>  
>  	tl_hba = *(struct tcm_loop_hba **)shost_priv(sc->device->host);
> @@ -213,12 +213,21 @@ static void tcm_loop_submission_work(struct work_struct *work)
>  
>  	}
>  
> -	if (!scsi_prot_sg_count(sc) && scsi_get_prot_op(sc) != SCSI_PROT_NORMAL)
> +	transfer_length = scsi_transfer_length(sc);
> +	if (!scsi_prot_sg_count(sc) &&
> +	    scsi_get_prot_op(sc) != SCSI_PROT_NORMAL) {
>  		se_cmd->prot_pto = true;
> +		/*
> +		 * loopback transport doesn't support
> +		 * WRITE_GENERATE, READ_STRIP protection
> +		 * information operations, go ahead unprotected.
> +		 */
> +		transfer_length = scsi_bufflen(sc);
> +	}
>  
>  	rc = target_submit_cmd_map_sgls(se_cmd, tl_nexus->se_sess, sc->cmnd,
>  			&tl_cmd->tl_sense_buf[0], tl_cmd->sc->device->lun,
> -			scsi_bufflen(sc), tcm_loop_sam_attr(sc),
> +			transfer_length, tcm_loop_sam_attr(sc),
>  			sc->sc_data_direction, 0,
>  			scsi_sglist(sc), scsi_sg_count(sc),
>  			sgl_bidi, sgl_bidi_count,
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index e022959..06f8ecd 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -665,8 +665,19 @@ sbc_check_prot(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb,
>  
>  	cmd->prot_type = dev->dev_attrib.pi_prot_type;
>  	cmd->prot_length = dev->prot_length * sectors;
> -	pr_debug("%s: prot_type=%d, prot_length=%d prot_op=%d prot_checks=%d\n",
> -		 __func__, cmd->prot_type, cmd->prot_length,
> +
> +	/**
> +	 * In case protection information exists over the wire
> +	 * we modify command data length to describe pure data.
> +	 * The actual transfer length is data length + protection
> +	 * length
> +	 **/
> +	if (protect)
> +		cmd->data_length -= cmd->prot_length;
> +

This fabric wide assumption breaks the vhost-scsi + tcm_qla2xxx PI code
already queued for v3.16-rc1..

A vhost-scsi side conversion to combine exp_data_len + prot_bytes is
easy enough in target-pending/for-next (see below), given that
vhost-scsi's exp_data_len is coming from virtio buffer iovecs, so
changing virtio-scsi LLD to use scsi_transfer_length() doesn't really
make any sense. (Adding CC's for MST + Paolo)

The qla2xxx target T10 PI patch in scsi/for-next (adding Quinn CC) will
also need a similar change to update qlt_do_work() to include PI bytes
for data_length being passed into tgt_ops->handle_cmd(), following what
qlt_build_ctio_crc2_pkt() is already doing for calculating
transfer_length for HW offload.

That said, there is one other small qla2xxx change to enable per-session
PI that is currently missing in Quinn's patch in scsi/for-next code. 

Sooo, I'll go ahead and include Sagi's patches with the vhost-scsi
change below if there are no objections from MKP and/or driver
maintainers, and post an -rc2 series after the merge window closes to
address the two remaining qla2xxx specific items.

--nab

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 667e72d..03e484f 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1144,7 +1144,8 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
                }
 
                cmd = vhost_scsi_get_tag(vq, tpg, cdb, tag, lun, task_attr,
-                                        exp_data_len, data_direction);
+                                        exp_data_len + prot_bytes,
+                                        data_direction);
                if (IS_ERR(cmd)) {
                        vq_err(vq, "vhost_scsi_get_tag failed %ld\n",
                                        PTR_ERR(cmd));

Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

[Paolo Bonzini]

Il 10/06/2014 10:04, Nicholas A. Bellinger ha scritto:
>
> That said, there is one other small qla2xxx change to enable per-session
> PI that is currently missing in Quinn's patch in scsi/for-next code.
>
> Sooo, I'll go ahead and include Sagi's patches with the vhost-scsi
> change below if there are no objections from MKP and/or driver
> maintainers, and post an -rc2 series after the merge window closes to
> address the two remaining qla2xxx specific items.

Indeed the patch looks like a tcm bugfix.  There is no reason to modify 
the LLD.

Paolo

Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

[Quinn Tran]

[-- Attachment #1: Type: text/plain, Size: 6878 bytes --]

All,

Comments inline.

Regards,
Quinn Tran




On 6/10/14 1:04 AM, "Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote:

>Hi Sagi & Co,
>
>On Sun, 2014-06-08 at 13:27 +0300, Sagi Grimberg wrote:
>> In various areas of the code, it is assumed that
>> se_cmd->data_length describes pure data. In case
>> that protection information exists over the wire
>> (protect bits is are on) the target core decrease
>> the protection length from the data length (instead
>> of each transport peeking in the cdb).
>>
>> Modify loopback device to include protection information
>> in the transferred data length (like other scsi transports).
>>
>> Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
>> ---
>>  drivers/target/loopback/tcm_loop.c |   15 ++++++++++++---
>>  drivers/target/target_core_sbc.c   |   15 +++++++++++++--
>>  2 files changed, 25 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/target/loopback/tcm_loop.c
>>b/drivers/target/loopback/tcm_loop.c
>> index c886ad1..1f4c015 100644
>> --- a/drivers/target/loopback/tcm_loop.c
>> +++ b/drivers/target/loopback/tcm_loop.c
>> @@ -179,7 +179,7 @@ static void tcm_loop_submission_work(struct
>>work_struct *work)
>>      struct tcm_loop_hba *tl_hba;
>>      struct tcm_loop_tpg *tl_tpg;
>>      struct scatterlist *sgl_bidi = NULL;
>> -    u32 sgl_bidi_count = 0;
>> +    u32 sgl_bidi_count = 0, transfer_length;
>>      int rc;
>>
>>      tl_hba = *(struct tcm_loop_hba **)shost_priv(sc->device->host);
>> @@ -213,12 +213,21 @@ static void tcm_loop_submission_work(struct
>>work_struct *work)
>>
>>      }
>>
>> -    if (!scsi_prot_sg_count(sc) && scsi_get_prot_op(sc) !=
>>SCSI_PROT_NORMAL)
>> +    transfer_length = scsi_transfer_length(sc);
>> +    if (!scsi_prot_sg_count(sc) &&
>> +        scsi_get_prot_op(sc) != SCSI_PROT_NORMAL) {
>>              se_cmd->prot_pto = true;
>> +            /*
>> +             * loopback transport doesn't support
>> +             * WRITE_GENERATE, READ_STRIP protection
>> +             * information operations, go ahead unprotected.
>> +             */
>> +            transfer_length = scsi_bufflen(sc);
>> +    }
>>
>>      rc = target_submit_cmd_map_sgls(se_cmd, tl_nexus->se_sess, sc->cmnd,
>>                      &tl_cmd->tl_sense_buf[0], tl_cmd->sc->device->lun,
>> -                    scsi_bufflen(sc), tcm_loop_sam_attr(sc),
>> +                    transfer_length, tcm_loop_sam_attr(sc),
>>                      sc->sc_data_direction, 0,
>>                      scsi_sglist(sc), scsi_sg_count(sc),
>>                      sgl_bidi, sgl_bidi_count,
>> diff --git a/drivers/target/target_core_sbc.c
>>b/drivers/target/target_core_sbc.c
>> index e022959..06f8ecd 100644
>> --- a/drivers/target/target_core_sbc.c
>> +++ b/drivers/target/target_core_sbc.c
>> @@ -665,8 +665,19 @@ sbc_check_prot(struct se_device *dev, struct
>>se_cmd *cmd, unsigned char *cdb,
>>
>>      cmd->prot_type = dev->dev_attrib.pi_prot_type;
>>      cmd->prot_length = dev->prot_length * sectors;
>> -    pr_debug("%s: prot_type=%d, prot_length=%d prot_op=%d
>>prot_checks=%d\n",
>> -             __func__, cmd->prot_type, cmd->prot_length,
>> +
>> +    /**
>> +     * In case protection information exists over the wire
>> +     * we modify command data length to describe pure data.
>> +     * The actual transfer length is data length + protection
>> +     * length
>> +     **/
>> +    if (protect)
>> +            cmd->data_length -= cmd->prot_length;
>> +

QT> Instead of using existing value within cmd->data_length, can we
calculated data_length based on secstors & blocksize.

cmd->data_length = sectors * dev->dev_attrib.block_size;


>From the QLogic perfective, the cmd->data_length is pull directly from the
wire (i.e. FCP header) when cmd is received.  In essence, it's whatever
the Initiator is set it to.  We does not have any indicator to spot DIF vs
none-DIF cmd when 1st received, unless the code do a peek.

With that said, the cmd->data_length does not guarantee to contain both
"data length" & "protection length" when cmd is submit to
TCM/target_submit_cmd().  In Dif-Insert mode, data_length will only
contain the actual data(no Dif).

It's best that the SBC code re-calculate the actual data length and dif
data length based on the number of sectors derived from the cmd.

Thanks.

>
>This fabric wide assumption breaks the vhost-scsi + tcm_qla2xxx PI code
>already queued for v3.16-rc1..
>
>A vhost-scsi side conversion to combine exp_data_len + prot_bytes is
>easy enough in target-pending/for-next (see below), given that
>vhost-scsi's exp_data_len is coming from virtio buffer iovecs, so
>changing virtio-scsi LLD to use scsi_transfer_length() doesn't really
>make any sense. (Adding CC's for MST + Paolo)
>
>The qla2xxx target T10 PI patch in scsi/for-next (adding Quinn CC) will
>also need a similar change to update qlt_do_work() to include PI bytes
>for data_length being passed into tgt_ops->handle_cmd(), following what
>qlt_build_ctio_crc2_pkt() is already doing for calculating
>transfer_length for HW offload.
>
>That said, there is one other small qla2xxx change to enable per-session
>PI that is currently missing in Quinn's patch in scsi/for-next code.

QT> I've been bog down with other front, I have not had a chance to come
back and re-patch this area. Will find some cycle to work on this soon.

>
>Sooo, I'll go ahead and include Sagi's patches with the vhost-scsi
>change below if there are no objections from MKP and/or driver
>maintainers, and post an -rc2 series after the merge window closes to
>address the two remaining qla2xxx specific items.
>
>--nab
>
>diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
>index 667e72d..03e484f 100644
>--- a/drivers/vhost/scsi.c
>+++ b/drivers/vhost/scsi.c
>@@ -1144,7 +1144,8 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct
>vhost_virtqueue *vq)
>                }
>
>                cmd = vhost_scsi_get_tag(vq, tpg, cdb, tag, lun,
>task_attr,
>-                                        exp_data_len, data_direction);
>+                                        exp_data_len + prot_bytes,
>+                                        data_direction);
>                if (IS_ERR(cmd)) {
>                        vq_err(vq, "vhost_scsi_get_tag failed %ld\n",
>                                        PTR_ERR(cmd));
>


________________________________

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 7485 bytes --]

Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

[Sagi Grimberg]

On 6/11/2014 12:17 AM, Quinn Tran wrote:

<SNIP>

> QT> Instead of using existing value within cmd->data_length, can we
> calculated data_length based on secstors & blocksize.
>
> cmd->data_length = sectors * dev->dev_attrib.block_size;

We can do that I suppose...
Although it seems weird that the core discards the transport byte-count...
Just wandering if we should recalc on the DIF case only or always.

>
>  From the QLogic perfective, the cmd->data_length is pull directly from the
> wire (i.e. FCP header) when cmd is received.  In essence, it's whatever
> the Initiator is set it to.  We does not have any indicator to spot DIF vs
> none-DIF cmd when 1st received, unless the code do a peek.

Same for all transports I assume...

>
> With that said, the cmd->data_length does not guarantee to contain both
> "data length" & "protection length" when cmd is submit to
> TCM/target_submit_cmd().  In Dif-Insert mode, data_length will only
> contain the actual data(no Dif).

No, in the DOUT_INSERT/DIN_STRIP case, protect bits are off and the core 
will take the data length as is.
This case is covered.

> It's best that the SBC code re-calculate the actual data length and dif
> data length based on the number of sectors derived from the cmd.

Nic, what's your take on this?

Sagi.

Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

[Nicholas A. Bellinger]

On Wed, 2014-06-11 at 10:24 +0300, Sagi Grimberg wrote:
> On 6/11/2014 12:17 AM, Quinn Tran wrote:
> 
> <SNIP>
> 
> > QT> Instead of using existing value within cmd->data_length, can we
> > calculated data_length based on secstors & blocksize.
> >
> > cmd->data_length = sectors * dev->dev_attrib.block_size;
> 
> We can do that I suppose...
> Although it seems weird that the core discards the transport byte-count...
> Just wandering if we should recalc on the DIF case only or always.
> 

Yeah, same concern here wrt to discarding the transport length.

This effectively makes the residual handling further down in
sbc_parse_cdb() -> target_cmd_size_check() always match size ==
cmd->data_length, because the latter as been recalculated by the former.

Honestly, I don't know if this is a problem for normal READ + WRITE with
prot=1 as I've never seen size != cmd->data_length for I/O related
commands, but it does seem potentially dangerous.

> >
> >  From the QLogic perfective, the cmd->data_length is pull directly from the
> > wire (i.e. FCP header) when cmd is received.  In essence, it's whatever
> > the Initiator is set it to.  We does not have any indicator to spot DIF vs
> > none-DIF cmd when 1st received, unless the code do a peek.
> 
> Same for all transports I assume...
> 

So just to confirm Quinn, the Qlogic the initiator includes the PI bytes
into the FCP header data_length for the target-side *_PASS and
DOUT_STRIP / DIN_INSERT, that is currently passed into
qla_tgt_ops->handle_cmd(), right..?

If that is the case, Sagi's v1 to cmd->data_length -= cmd->prot_length
seems like it would still do right thing for *_PASS and DOUT_STRIP /
DIN_INSERT, given that cmd->prot_length is calculated in
sbc_check_prot() based upon dev->prot_length * sectors..

> >
> > With that said, the cmd->data_length does not guarantee to contain both
> > "data length" & "protection length" when cmd is submit to
> > TCM/target_submit_cmd().  In Dif-Insert mode, data_length will only
> > contain the actual data(no Dif).
> 
> No, in the DOUT_INSERT/DIN_STRIP case, protect bits are off and the core 
> will take the data length as is.
> This case is covered.
> 

<nod>

> > It's best that the SBC code re-calculate the actual data length and dif
> > data length based on the number of sectors derived from the cmd.
> 
> Nic, what's your take on this?
> 

Hard to say.  Discarding the transport length in v2 doesn't seem like a
good idea, but subtracting from cmd->prot_length in v1 is using the
sector count from the CDB anyways, so it's essentially the same tradeoff
of recalculating the transport's cmd->data_length from values within the
CDB w/ prot=1.

MKP, WDYT..?

--nab

Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

[Quinn Tran]

[-- Attachment #1: Type: text/plain, Size: 3737 bytes --]



On 6/11/14 2:30 PM, "Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote:

>On Wed, 2014-06-11 at 10:24 +0300, Sagi Grimberg wrote:
>> On 6/11/2014 12:17 AM, Quinn Tran wrote:
>>
>> <SNIP>
>>
>> > QT> Instead of using existing value within cmd->data_length, can we
>> > calculated data_length based on secstors & blocksize.
>> >
>> > cmd->data_length = sectors * dev->dev_attrib.block_size;
>>
>> We can do that I suppose...
>> Although it seems weird that the core discards the transport
>>byte-count...
>> Just wandering if we should recalc on the DIF case only or always.
>>
>
>Yeah, same concern here wrt to discarding the transport length.
>
>This effectively makes the residual handling further down in
>sbc_parse_cdb() -> target_cmd_size_check() always match size ==
>cmd->data_length, because the latter as been recalculated by the former.
>
>Honestly, I don't know if this is a problem for normal READ + WRITE with
>prot=1 as I've never seen size != cmd->data_length for I/O related
>commands, but it does seem potentially dangerous.
>
>> >
>> >  From the QLogic perfective, the cmd->data_length is pull directly
>>from the
>> > wire (i.e. FCP header) when cmd is received.  In essence, it's
>>whatever
>> > the Initiator is set it to.  We does not have any indicator to spot
>>DIF vs
>> > none-DIF cmd when 1st received, unless the code do a peek.
>>
>> Same for all transports I assume...
>>
>
>So just to confirm Quinn, the Qlogic the initiator includes the PI bytes
>into the FCP header data_length for the target-side *_PASS and
>DOUT_STRIP / DIN_INSERT, that is currently passed into
>qla_tgt_ops->handle_cmd(), right..?

QT>
Initiator:
DOUT_STRIP/DIN_Insert:  FCP_DL = data length only (no dif length)
Dif PASS: FCP_DL = data length + Dif length.

Target:
DOUT_strip/DIN_Insert:  qla_tgt_ops->handle_cmd(data length only)

sbc_check_prot()
If (protect)
   cmd->data_length -= cmd->prot_length;  << truncation


---
DIF_PASS: qla_tgt_ops->handle_cmd(data length + Dif length)


>
>If that is the case, Sagi's v1 to cmd->data_length -= cmd->prot_length
>seems like it would still do right thing for *_PASS and DOUT_STRIP /
>DIN_INSERT, given that cmd->prot_length is calculated in
>sbc_check_prot() based upon dev->prot_length * sectors..
>
>> >
>> > With that said, the cmd->data_length does not guarantee to contain
>>both
>> > "data length" & "protection length" when cmd is submit to
>> > TCM/target_submit_cmd().  In Dif-Insert mode, data_length will only
>> > contain the actual data(no Dif).
>>
>> No, in the DOUT_INSERT/DIN_STRIP case, protect bits are off and the
>>core
>> will take the data length as is.
>> This case is covered.
>>
>
><nod>

QT> agree.

>
>> > It's best that the SBC code re-calculate the actual data length and
>>dif
>> > data length based on the number of sectors derived from the cmd.
>>
>> Nic, what's your take on this?
>>
>
>Hard to say.  Discarding the transport length in v2 doesn't seem like a
>good idea, but subtracting from cmd->prot_length in v1 is using the
>sector count from the CDB anyways, so it's essentially the same tradeoff
>of recalculating the transport's cmd->data_length from values within the
>CDB w/ prot=1.
>
>MKP, WDYT..?
>
>--nab
>
>
>
>


________________________________

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 6473 bytes --]

Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

[Nicholas A. Bellinger]

On Wed, 2014-06-11 at 22:32 +0000, Quinn Tran wrote:
> 
> On 6/11/14 2:30 PM, "Nicholas A. Bellinger" <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org> wrote:
> 
> >On Wed, 2014-06-11 at 10:24 +0300, Sagi Grimberg wrote:
> >> On 6/11/2014 12:17 AM, Quinn Tran wrote:
> >>
> >> <SNIP>
> >>
> >> > QT> Instead of using existing value within cmd->data_length, can we
> >> > calculated data_length based on secstors & blocksize.
> >> >
> >> > cmd->data_length = sectors * dev->dev_attrib.block_size;
> >>
> >> We can do that I suppose...
> >> Although it seems weird that the core discards the transport
> >>byte-count...
> >> Just wandering if we should recalc on the DIF case only or always.
> >>
> >
> >Yeah, same concern here wrt to discarding the transport length.
> >
> >This effectively makes the residual handling further down in
> >sbc_parse_cdb() -> target_cmd_size_check() always match size ==
> >cmd->data_length, because the latter as been recalculated by the former.
> >
> >Honestly, I don't know if this is a problem for normal READ + WRITE with
> >prot=1 as I've never seen size != cmd->data_length for I/O related
> >commands, but it does seem potentially dangerous.
> >
> >> >
> >> >  From the QLogic perfective, the cmd->data_length is pull directly
> >>from the
> >> > wire (i.e. FCP header) when cmd is received.  In essence, it's
> >>whatever
> >> > the Initiator is set it to.  We does not have any indicator to spot
> >>DIF vs
> >> > none-DIF cmd when 1st received, unless the code do a peek.
> >>
> >> Same for all transports I assume...
> >>
> >
> >So just to confirm Quinn, the Qlogic the initiator includes the PI bytes
> >into the FCP header data_length for the target-side *_PASS and
> >DOUT_STRIP / DIN_INSERT, that is currently passed into
> >qla_tgt_ops->handle_cmd(), right..?
> 
> QT>
> Initiator:
> DOUT_STRIP/DIN_Insert:  FCP_DL = data length only (no dif length)
> Dif PASS: FCP_DL = data length + Dif length.
> 
> Target:
> DOUT_strip/DIN_Insert:  qla_tgt_ops->handle_cmd(data length only)
> 
> sbc_check_prot()
> If (protect)
>    cmd->data_length -= cmd->prot_length;  << truncation
> 
> 

<nod>

> ---
> DIF_PASS: qla_tgt_ops->handle_cmd(data length + Dif length)
> 
> 

Ok, so Sagi's patches for legacy fabric <-> PI backend and
PI fabric <-> PI backend cases look OK, but not for the
PI fabric <-> legacy backend case as noted above..

Given DOUT_STRIP + DIN_INSERT have not been enabled in sbc_check_prot()
just yet, I'll go ahead and merge his v2 series to address the two
existing cases in -rc1 + v3.15.y stable code.

>From there, enabling PI fabric <-> legacy backend support in >= rc2 with
target-core will be easy, as it's just a matter of updating
sbc_set_prot_op_checks() to assign prot_ops, avoid sbc_check_prot()
cmd->data_length recalculation, and making sure PROTECTION control
feature bits are still exposed for legacy -> unprotected backends.

Thanks!

--nab

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/3] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

[Martin K. Petersen]

>>>>> "nab" == Nicholas A Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org> writes:

nab> Hard to say.  Discarding the transport length in v2 doesn't seem
nab> like a good idea, but subtracting from cmd->prot_length in v1 is
nab> using the sector count from the CDB anyways, so it's essentially
nab> the same tradeoff of recalculating the transport's cmd->data_length
nab> from values within the CDB w/ prot=1.

nab> MKP, WDYT..?

My general feeling is that once sbc on the target sees {rd,wr}protect >
0 we're in the territory where you should start separating data and
PI internally.

-- 
Martin K. Petersen	Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 2/3] libiscsi, iser: Adjust data_length to include protection information

[Sagi Grimberg]

In case protection information exists over the wire
iscsi header data_length is required to include it.
Use protection information aware scsi helpers to set
the correct transfer length.

In order to avoid breakage, remove iser transfer length
checks for each task as they are not always true and
somewhat redundant anyway.

Signed-off-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/ulp/iser/iser_initiator.c |   34 +++++++------------------
 drivers/scsi/libiscsi.c                      |   18 +++++++-------
 2 files changed, 19 insertions(+), 33 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iser_initiator.c b/drivers/infiniband/ulp/iser/iser_initiator.c
index 2e2d903..e3b0af5 100644
--- a/drivers/infiniband/ulp/iser/iser_initiator.c
+++ b/drivers/infiniband/ulp/iser/iser_initiator.c
@@ -41,11 +41,11 @@
 #include "iscsi_iser.h"
 
 /* Register user buffer memory and initialize passive rdma
- *  dto descriptor. Total data size is stored in
- *  iser_task->data[ISER_DIR_IN].data_len
+ *  dto descriptor. Data size is stored in
+ *  task->data[ISER_DIR_IN].data_len, Protection size
+ *  os stored in task->prot[ISER_DIR_IN].data_len
  */
-static int iser_prepare_read_cmd(struct iscsi_task *task,
-				 unsigned int edtl)
+static int iser_prepare_read_cmd(struct iscsi_task *task)
 
 {
 	struct iscsi_iser_task *iser_task = task->dd_data;
@@ -73,14 +73,6 @@ static int iser_prepare_read_cmd(struct iscsi_task *task,
 			return err;
 	}
 
-	if (edtl > iser_task->data[ISER_DIR_IN].data_len) {
-		iser_err("Total data length: %ld, less than EDTL: "
-			 "%d, in READ cmd BHS itt: %d, conn: 0x%p\n",
-			 iser_task->data[ISER_DIR_IN].data_len, edtl,
-			 task->itt, iser_task->ib_conn);
-		return -EINVAL;
-	}
-
 	err = device->iser_reg_rdma_mem(iser_task, ISER_DIR_IN);
 	if (err) {
 		iser_err("Failed to set up Data-IN RDMA\n");
@@ -100,8 +92,9 @@ static int iser_prepare_read_cmd(struct iscsi_task *task,
 }
 
 /* Register user buffer memory and initialize passive rdma
- *  dto descriptor. Total data size is stored in
- *  task->data[ISER_DIR_OUT].data_len
+ *  dto descriptor. Data size is stored in
+ *  task->data[ISER_DIR_OUT].data_len, Protection size
+ *  is stored at task->prot[ISER_DIR_OUT].data_len
  */
 static int
 iser_prepare_write_cmd(struct iscsi_task *task,
@@ -135,14 +128,6 @@ iser_prepare_write_cmd(struct iscsi_task *task,
 			return err;
 	}
 
-	if (edtl > iser_task->data[ISER_DIR_OUT].data_len) {
-		iser_err("Total data length: %ld, less than EDTL: %d, "
-			 "in WRITE cmd BHS itt: %d, conn: 0x%p\n",
-			 iser_task->data[ISER_DIR_OUT].data_len,
-			 edtl, task->itt, task->conn);
-		return -EINVAL;
-	}
-
 	err = device->iser_reg_rdma_mem(iser_task, ISER_DIR_OUT);
 	if (err != 0) {
 		iser_err("Failed to register write cmd RDMA mem\n");
@@ -417,11 +402,12 @@ int iser_send_command(struct iscsi_conn *conn,
 	if (scsi_prot_sg_count(sc)) {
 		prot_buf->buf  = scsi_prot_sglist(sc);
 		prot_buf->size = scsi_prot_sg_count(sc);
-		prot_buf->data_len = sc->prot_sdb->length;
+		prot_buf->data_len = scsi_prot_length(data_buf->data_len,
+						      sc->device->sector_size);
 	}
 
 	if (hdr->flags & ISCSI_FLAG_CMD_READ) {
-		err = iser_prepare_read_cmd(task, edtl);
+		err = iser_prepare_read_cmd(task);
 		if (err)
 			goto send_command_error;
 	}
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 26dc005..3f46234 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -338,7 +338,7 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
 	struct iscsi_session *session = conn->session;
 	struct scsi_cmnd *sc = task->sc;
 	struct iscsi_scsi_req *hdr;
-	unsigned hdrlength, cmd_len;
+	unsigned hdrlength, cmd_len, transfer_length;
 	itt_t itt;
 	int rc;
 
@@ -391,11 +391,11 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
 	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) {
-		unsigned out_len = scsi_out(sc)->length;
 		struct iscsi_r2t_info *r2t = &task->unsol_r2t;
 
-		hdr->data_length = cpu_to_be32(out_len);
 		hdr->flags |= ISCSI_FLAG_CMD_WRITE;
 		/*
 		 * Write counters:
@@ -414,18 +414,19 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
 		memset(r2t, 0, sizeof(*r2t));
 
 		if (session->imm_data_en) {
-			if (out_len >= session->first_burst)
+			if (transfer_length >= session->first_burst)
 				task->imm_count = min(session->first_burst,
 							conn->max_xmit_dlength);
 			else
-				task->imm_count = min(out_len,
-							conn->max_xmit_dlength);
+				task->imm_count = min(transfer_length,
+						      conn->max_xmit_dlength);
 			hton24(hdr->dlength, task->imm_count);
 		} else
 			zero_data(hdr->dlength);
 
 		if (!session->initial_r2t_en) {
-			r2t->data_length = min(session->first_burst, out_len) -
+			r2t->data_length = min(session->first_burst,
+					       transfer_length) -
 					       task->imm_count;
 			r2t->data_offset = task->imm_count;
 			r2t->ttt = cpu_to_be32(ISCSI_RESERVED_TAG);
@@ -438,7 +439,6 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
 	} else {
 		hdr->flags |= ISCSI_FLAG_CMD_FINAL;
 		zero_data(hdr->dlength);
-		hdr->data_length = cpu_to_be32(scsi_in(sc)->length);
 
 		if (sc->sc_data_direction == DMA_FROM_DEVICE)
 			hdr->flags |= ISCSI_FLAG_CMD_READ;
@@ -466,7 +466,7 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
 			  scsi_bidi_cmnd(sc) ? "bidirectional" :
 			  sc->sc_data_direction == DMA_TO_DEVICE ?
 			  "write" : "read", conn->id, sc, sc->cmnd[0],
-			  task->itt, scsi_bufflen(sc),
+			  task->itt, transfer_length,
 			  scsi_bidi_cmnd(sc) ? scsi_in(sc)->length : 0,
 			  session->cmdsn,
 			  session->max_cmdsn - session->exp_cmdsn + 1);
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/3] libiscsi, iser: Adjust data_length to include protection information

[Mike Christie]

On 06/08/2014 05:27 AM, Sagi Grimberg wrote:
> In case protection information exists over the wire
> iscsi header data_length is required to include it.
> Use protection information aware scsi helpers to set
> the correct transfer length.
> 
> In order to avoid breakage, remove iser transfer length
> checks for each task as they are not always true and
> somewhat redundant anyway.
> 
> Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
> ---
>  drivers/infiniband/ulp/iser/iser_initiator.c |   34 +++++++------------------
>  drivers/scsi/libiscsi.c                      |   18 +++++++-------


Looks ok to me now. Thanks.

Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>

Not sure, if I am supposed to do a acked-by for the libiscsi.c change,
but if so

Acked-by: Mike Christie <michaelc@cs.wisc.edu>

Re: [PATCH v1 0/3] Include protection information in iscsi header

[Nicholas A. Bellinger]

Hi MKP + Mike + Roland,

On Sun, 2014-06-08 at 13:27 +0300, Sagi Grimberg wrote:
> At the SCSI transport level, there is no distinction between
> user data and protection information. Thus, iscsi header field
> "expected data transfer length" should include protection
> information.
> 
> Patch #1 introduces scsi helpers scsi_transfer_length (compute
> wire transfer byte count) and scsi_prot_len (compute protection
> information byte count).
> 
> Patch #2 modifies iscsi initiator to set correct wire transfer length
> in iscsi header data_length field (and modifies iser accordingly).
> 
> Patch #3 modifies target core to expect protection information included
> in the wire transfer length (and modifies loopback transport to do so).
> 
> I have 2 patches that convert lpfc/qla2xxx drivers to use scsi helpers
> but these are completely untested at the moment. Once we get this set
> to land upstream, I can queue them up as RFCs.
> 
> Changes from v0:
> - Introduce scsi helpers to compute correct transfer length in the
>   presence of protection information (instead of having each transport
>   doing the same computation).
> - Modify iscsi to set correct transfer length using scsi helpers
> - Modify loopback transport to set correct transfer length using
>   scsi helpers
> 
> Sagi Grimberg (3):
>   scsi_cmnd: Introduce scsi_transfer_length helper
>   libiscsi, iser: Adjust data_length to include protection information
>   TARGET/sbc,loopback: Adjust command data length in case pi exists on
>     the wire
> 
>  drivers/infiniband/ulp/iser/iser_initiator.c |   34 ++++++----------------
>  drivers/scsi/libiscsi.c                      |   18 ++++++------
>  drivers/target/loopback/tcm_loop.c           |   15 ++++++++--
>  drivers/target/target_core_sbc.c             |   15 ++++++++-
>  include/scsi/scsi_cmnd.h                     |   39 ++++++++++++++++++++++++++
>  5 files changed, 83 insertions(+), 38 deletions(-)
> 

Please review + ACK this series.

Also, given the nature of the changes, they will need to be included
into v3.15.y code to avoid any potential wire protocol compatibility
issues.

--nab

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html