[PATCH 0/2] Include protection information in iscsi header

[PATCH 0/2] 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.

This set modifies both the iscsi initiator (patch #1), and
target (patch #2) to expect data length which includes
protection information.

Although these patches involve 3 subsystems with different
maintainers (scsi, iser, target) I would prefer seeing these
patches included together.

Sagi Grimberg (2):
  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                      |   35 +++++++++++++++++++++++++-
 drivers/target/loopback/tcm_loop.c           |   35 +++++++++++++++++++++++---
 drivers/target/target_core_sbc.c             |   15 +++++++++-
 include/scsi/libiscsi.h                      |   19 ++++++++++++++
 5 files changed, 107 insertions(+), 31 deletions(-)

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

[Sagi Grimberg]

In case protection information exists over the wire
iscsi header data_length field is required to include it.

Also 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                      |   35 +++++++++++++++++++++++++-
 include/scsi/libiscsi.h                      |   19 ++++++++++++++
 3 files changed, 63 insertions(+), 25 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iser_initiator.c b/drivers/infiniband/ulp/iser/iser_initiator.c
index 2e2d903..1600e35 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 = iscsi_prot_len(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..b54d1cc 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -326,6 +326,31 @@ static int iscsi_check_tmf_restrictions(struct iscsi_task *task, int opcode)
 }
 
 /**
+ * iscsi_adjust_dl - Adjust SCSI data length to include PI
+ * @sc: scsi command.
+ * @data_length: command data length.
+ *
+ * Adjust the data length to account for how much data
+ * is actually on the wire.
+ *
+ * returns the adjusted data length
+ **/
+static unsigned
+iscsi_adjust_dl(struct scsi_cmnd *sc, unsigned data_len)
+{
+	if (sc->sc_data_direction == DMA_FROM_DEVICE) {
+		if (scsi_get_prot_op(sc) ==  SCSI_PROT_READ_INSERT)
+			return data_len;
+	} else {
+		if (scsi_get_prot_op(sc) ==  SCSI_PROT_WRITE_STRIP)
+			return data_len;
+	}
+
+	/* Protection information exists on the wire */
+	return data_len + iscsi_prot_len(data_len, sc->device->sector_size);
+}
+
+/**
  * iscsi_prep_scsi_cmd_pdu - prep iscsi scsi cmd pdu
  * @task: iscsi task
  *
@@ -395,6 +420,9 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
 		unsigned out_len = scsi_out(sc)->length;
 		struct iscsi_r2t_info *r2t = &task->unsol_r2t;
 
+		if (task->protected)
+			out_len = iscsi_adjust_dl(sc, out_len);
+
 		hdr->data_length = cpu_to_be32(out_len);
 		hdr->flags |= ISCSI_FLAG_CMD_WRITE;
 		/*
@@ -436,9 +464,14 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
 			/* No unsolicit Data-Out's */
 			hdr->flags |= ISCSI_FLAG_CMD_FINAL;
 	} else {
+		unsigned in_len = scsi_in(sc)->length;
+
 		hdr->flags |= ISCSI_FLAG_CMD_FINAL;
 		zero_data(hdr->dlength);
-		hdr->data_length = cpu_to_be32(scsi_in(sc)->length);
+		if (task->protected)
+			in_len = iscsi_adjust_dl(sc, in_len);
+
+		hdr->data_length = cpu_to_be32(in_len);
 
 		if (sc->sc_data_direction == DMA_FROM_DEVICE)
 			hdr->flags |= ISCSI_FLAG_CMD_READ;
diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
index 728c9ad..534a1dc 100644
--- a/include/scsi/libiscsi.h
+++ b/include/scsi/libiscsi.h
@@ -494,4 +494,23 @@ iscsi_padding(unsigned int len)
 	return len;
 }
 
+/*
+ * data integrity helpers
+ */
+static inline unsigned
+iscsi_prot_len(unsigned data_len, unsigned sector_size)
+{
+	switch (sector_size) {
+	case 512:
+		return (data_len >> 9) * 8;
+	case 1024:
+		return (data_len >> 10) * 8;
+	case 2048:
+		return (data_len >> 11) * 8;
+	case 4096:
+		return (data_len >> 12) * 8;
+	default:
+		return (data_len >> ilog2(sector_size)) * 8;
+	}
+}
 #endif
-- 
1.7.1

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

[Mike Christie]

On 06/01/2014 11:19 AM, Sagi Grimberg wrote:
>  
> +/*
> + * data integrity helpers
> + */
> +static inline unsigned
> +iscsi_prot_len(unsigned data_len, unsigned sector_size)
> +{
> +	switch (sector_size) {
> +	case 512:
> +		return (data_len >> 9) * 8;
> +	case 1024:
> +		return (data_len >> 10) * 8;
> +	case 2048:
> +		return (data_len >> 11) * 8;
> +	case 4096:
> +		return (data_len >> 12) * 8;
> +	default:
> +		return (data_len >> ilog2(sector_size)) * 8;
> +	}
> +}
>  #endif

I do not think this should not be in the iscsi code.

--
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 1/2] libiscsi, iser: Adjust data_length to include protection information

[Martin K. Petersen]

>>>>> "Mike" == Mike Christie <michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org> writes:

Mike> On 06/01/2014 11:19 AM, Sagi Grimberg wrote:
>> 
>> +/*
>> + * data integrity helpers
>> + */
>> +static inline unsigned +iscsi_prot_len(unsigned data_len, unsigned
>> sector_size) +{
>> + switch (sector_size) {
>> + case 512:
>> + return (data_len >> 9) * 8;
>> + case 1024:
>> + return (data_len >> 10) * 8;
>> + case 2048:
>> + return (data_len >> 11) * 8;
>> + case 4096:
>> + return (data_len >> 12) * 8;
>> + default:
>> + return (data_len >> ilog2(sector_size)) * 8;
>> + }
>> +}
>> #endif

Mike> I do not think this should not be in the iscsi code.

In the data integrity update I posted there's a flag saying "transfer PI
on the wire". That was meant to be the thing driver's should key off of
to adjust transfer length. But I'm also happy to provide a unsigned int
scsi_transfer_length(struct scsi_cmnd *) thingy that returns the right
byte count. Just bear in mind that the host-facing DIX transfer length
may be different.

-- 
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 1/2] libiscsi, iser: Adjust data_length to include protection information

[Sagi Grimberg]

On 6/4/2014 1:18 AM, Martin K. Petersen wrote:
>>>>>> "Mike" == Mike Christie <michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org> writes:
> Mike> On 06/01/2014 11:19 AM, Sagi Grimberg wrote:
>>> +/*
>>> + * data integrity helpers
>>> + */
>>> +static inline unsigned +iscsi_prot_len(unsigned data_len, unsigned
>>> sector_size) +{
>>> + switch (sector_size) {
>>> + case 512:
>>> + return (data_len >> 9) * 8;
>>> + case 1024:
>>> + return (data_len >> 10) * 8;
>>> + case 2048:
>>> + return (data_len >> 11) * 8;
>>> + case 4096:
>>> + return (data_len >> 12) * 8;
>>> + default:
>>> + return (data_len >> ilog2(sector_size)) * 8;
>>> + }
>>> +}
>>> #endif
> Mike> I do not think this should not be in the iscsi code.
>
> In the data integrity update I posted there's a flag saying "transfer PI
> on the wire". That was meant to be the thing driver's should key off of
> to adjust transfer length. But I'm also happy to provide a unsigned int
> scsi_transfer_length(struct scsi_cmnd *) thingy that returns the right
> byte count. Just bear in mind that the host-facing DIX transfer length
> may be different.
>

OK, let me prepare v1 moving this logic to a scsi helper and we'll have 
another round of comments.

Thanks,
Sagi.
--
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 1/2] libiscsi, iser: Adjust data_length to include protection information

[Mike Christie]

On 06/01/2014 11:19 AM, Sagi Grimberg wrote:
>  /**
> + * iscsi_adjust_dl - Adjust SCSI data length to include PI
> + * @sc: scsi command.
> + * @data_length: command data length.
> + *
> + * Adjust the data length to account for how much data
> + * is actually on the wire.
> + *
> + * returns the adjusted data length
> + **/
> +static unsigned
> +iscsi_adjust_dl(struct scsi_cmnd *sc, unsigned data_len)

Hey, one other comment. Could you rename this to iscsi_adjust_data_len
or iscsi_adjust_dlength? It is more common in the iscsi code to use
those names for data length.
--
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 1/2] libiscsi, iser: Adjust data_length to include protection information

[Sagi Grimberg]

On 6/3/2014 7:11 PM, Mike Christie wrote:
> On 06/01/2014 11:19 AM, Sagi Grimberg wrote:
>>   /**
>> + * iscsi_adjust_dl - Adjust SCSI data length to include PI
>> + * @sc: scsi command.
>> + * @data_length: command data length.
>> + *
>> + * Adjust the data length to account for how much data
>> + * is actually on the wire.
>> + *
>> + * returns the adjusted data length
>> + **/
>> +static unsigned
>> +iscsi_adjust_dl(struct scsi_cmnd *sc, unsigned data_len)
> Hey, one other comment. Could you rename this to iscsi_adjust_data_len
> or iscsi_adjust_dlength? It is more common in the iscsi code to use
> those names for data length.

No need - I moved this logic to a scsi helper anyway (as MKP suggested)...

Thanks!
Sagi.

[PATCH 2/2] 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 transport 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 |   35 +++++++++++++++++++++++++++++++----
 drivers/target/target_core_sbc.c   |   15 +++++++++++++--
 2 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index c886ad1..9adde8d 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, data_len;
 	int rc;
 
 	tl_hba = *(struct tcm_loop_hba **)shost_priv(sc->device->host);
@@ -213,12 +213,39 @@ static void tcm_loop_submission_work(struct work_struct *work)
 
 	}
 
-	if (!scsi_prot_sg_count(sc) && scsi_get_prot_op(sc) != SCSI_PROT_NORMAL)
-		se_cmd->prot_pto = true;
+	data_len = scsi_bufflen(sc);
+	if (scsi_get_prot_op(sc) != SCSI_PROT_NORMAL) {
+		if (!scsi_prot_sg_count(sc))
+			se_cmd->prot_pto = true;
+		else {
+			/**
+			 * Adjust data_length to include
+			 * protection information
+			 **/
+			switch (sc->device->sector_size) {
+			case 512:
+				data_len += (data_len >> 9) * 8;
+				break;
+			case 1024:
+				data_len += (data_len >> 10) * 8;
+				break;
+			case 2048:
+				data_len += (data_len >> 11) * 8;
+				break;
+			case 4096:
+				data_len += (data_len >> 12) * 8;
+				break;
+			default:
+				data_len +=
+				(data_len >> ilog2(sc->device->sector_size)) * 8;
+			}
+		}
+	}
+
 
 	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),
+			data_len, 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 2/2] TARGET/sbc,loopback: Adjust command data length in case pi exists on the wire

[Martin K. Petersen]

>>>>> "Sagi" == Sagi Grimberg <sagig@mellanox.com> writes:

Sagi,

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

I completely agree with the notion of including PI in the transport byte
count.

Why do you open code the transfer length adjustment below?

+			/**
+			 * Adjust data_length to include
+			 * protection information
+			 **/
+			switch (sc->device->sector_size) {
+			case 512:
+				data_len += (data_len >> 9) * 8;
+				break;
+			case 1024:
+				data_len += (data_len >> 10) * 8;
+				break;
+			case 2048:
+				data_len += (data_len >> 11) * 8;
+				break;
+			case 4096:
+				data_len += (data_len >> 12) * 8;
+				break;
+			default:
+				data_len +=
+				(data_len >> ilog2(sc->device->sector_size)) * 8;
+			}

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Sagi Grimberg]

On 6/2/2014 7:54 PM, Martin K. Petersen wrote:
>>>>>> "Sagi" == Sagi Grimberg <sagig@mellanox.com> writes:
> Sagi,
>
> Sagi> In various areas of the code, it is assumed that
> Sagi> se_cmd-> data_length describes pure data. In case
> Sagi> that protection information exists over the wire (protect bits is
> Sagi> are on) the target core decrease the protection length from the
> Sagi> data length (instead of each transport peeking in the cdb).
>
> I completely agree with the notion of including PI in the transport byte
> count.
>
> Why do you open code the transfer length adjustment below?

Can't say I have a good reason for that.
I'll move this logic to scsi_cmnd.h.

>
> +			/**
> +			 * Adjust data_length to include
> +			 * protection information
> +			 **/
> +			switch (sc->device->sector_size) {
> +			case 512:
> +				data_len += (data_len >> 9) * 8;
> +				break;
> +			case 1024:
> +				data_len += (data_len >> 10) * 8;
> +				break;
> +			case 2048:
> +				data_len += (data_len >> 11) * 8;
> +				break;
> +			case 4096:
> +				data_len += (data_len >> 12) * 8;
> +				break;
> +			default:
> +				data_len +=
> +				(data_len >> ilog2(sc->device->sector_size)) * 8;
> +			}
>

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

[Roland Dreier]

On Sun, Jun 1, 2014 at 9:19 AM, Sagi Grimberg <sagig@mellanox.com> wrote:
> Although these patches involve 3 subsystems with different
> maintainers (scsi, iser, target) I would prefer seeing these
> patches included together.

Why?  Because they break wire compatibility?

I hate to say it but even if they're merged at the same time, you
can't guarantee that targets and initiators will be updated together.

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

[Or Gerlitz]

On Tue, Jun 3, 2014 at 9:16 AM, Roland Dreier <roland@purestorage.com> wrote:
> On Sun, Jun 1, 2014 at 9:19 AM, Sagi Grimberg <sagig@mellanox.com> wrote:
>> Although these patches involve 3 subsystems with different
>> maintainers (scsi, iser, target) I would prefer seeing these
>> patches included together.
>
> Why?  Because they break wire compatibility?
>
> I hate to say it but even if they're merged at the same time, you
> can't guarantee that targets and initiators will be updated together.

Guys, this all deals with code merged in 3.15-rc1, and (thanks god and
linus that -rc8 took place this cycle) 3.15 isn't out yet!! -- so we
just need to act quickly and this (having the fix in 3.15 or if too
late in 3.15.1) would be OK

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

[Sagi Grimberg]

On 6/3/2014 9:16 AM, Roland Dreier wrote:
> On Sun, Jun 1, 2014 at 9:19 AM, Sagi Grimberg <sagig@mellanox.com> wrote:
>> Although these patches involve 3 subsystems with different
>> maintainers (scsi, iser, target) I would prefer seeing these
>> patches included together.
> Why?  Because they break wire compatibility?
>
> I hate to say it but even if they're merged at the same time, you
> can't guarantee that targets and initiators will be updated together.


Yes that's true, but still I would like to avoid a kernel release that 
the target and initiator
can't talk to one another...

Sagi.