[PATCH v2 0/3] Include protection information in transport header

[PATCH v2 0/3] Include protection information in transport 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 helper scsi_transfer_length which computes
wire transfer 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 re-calculate the pure data length
in case of PI presence over the wire (and modifies loopback transport
to align with other transports).

Changes from v1:
- scsi_cmnd: Rewrite scsi_transfer_length as MKP suggested
- Target/sbc: re-calculate the data_length in case PI exists
  on the wire (instead od deacrease data -= prot)

Changes from v0:
- Introduce scsi helpers to compute correct transfer length in the
  presence of protection information.
- 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                     |   17 +++++++++++++
 5 files changed, 61 insertions(+), 38 deletions(-)

[PATCH v2 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>
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 */
-- 
1.7.1

[PATCH v2 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@mellanox.com>
Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>
---
 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..8d44a40 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 = data_buf->data_len >>
+				     ilog2(sc->device->sector_size) * 8;
 	}
 
 	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

[PATCH v2 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 re-calculates
the data length from the CDB and the backed device
block size (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..4b5716f 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 = sectors * dev->dev_attrib.block_size;
+
+	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 v2 0/3] Include protection information in transport header

[Nicholas A. Bellinger]

On Wed, 2014-06-11 at 12:09 +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 helper scsi_transfer_length which computes
> wire transfer 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 re-calculate the pure data length
> in case of PI presence over the wire (and modifies loopback transport
> to align with other transports).
> 
> Changes from v1:
> - scsi_cmnd: Rewrite scsi_transfer_length as MKP suggested
> - Target/sbc: re-calculate the data_length in case PI exists
>   on the wire (instead od deacrease data -= prot)
> 
> Changes from v0:
> - Introduce scsi helpers to compute correct transfer length in the
>   presence of protection information.
> - 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                     |   17 +++++++++++++
>  5 files changed, 61 insertions(+), 38 deletions(-)
> 

Ok, I've applied these to for-next, but let's see what MKP recommends
for patch #3 wrt to recalculating cmd->data_length.

Thanks Sagi!

--nab

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

[Martin K. Petersen]

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

Sagi> In case protection information exists on the wire scsi transports
Sagi> should include it in the transfer byte count (even if protection
Sagi> information does not exist in the host memory space). This helper
Sagi> will compute the total transfer length from the scsi command data
Sagi> length and protection attributes.

Looks good!

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Christoph Hellwig]

This patch causes a regression when using the iscsi initiator over
TCP for me. When mounting a newly created ext4 filesystem I get the
following BUG: 

[   31.611803] BUG: unable to handle kernel NULL pointer dereference at 000000000000000c
[   31.613563] IP: [<ffffffff8197b38d>] iscsi_tcp_segment_done+0x2bd/0x380
[   31.613563] PGD 7a3e4067 PUD 7a45f067 PMD 0 
[   31.613563] Oops: 0000 [#1] SMP 
[   31.613563] Modules linked in:
[   31.613563] CPU: 3 PID: 3739 Comm: kworker/u8:5 Not tainted 3.16.0-rc2 #187
[   31.613563] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[   31.613563] Workqueue: iscsi_q_2 iscsi_xmitworker
[   31.613563] task: ffff88007b33cf10 ti: ffff88007ad94000 task.ti: ffff88007ad94000
[   31.613563] RIP: 0010:[<ffffffff8197b38d>]  [<ffffffff8197b38d>] iscsi_tcp_segment_done+0x2bd/0x380
[   31.613563] RSP: 0018:ffff88007ad97b38  EFLAGS: 00010246
[   31.613563] RAX: 0000000000000000 RBX: ffff88007cd67910 RCX: 0000000000000200
[   31.613563] RDX: 0000000000002000 RSI: 0000000000000000 RDI: ffff88007cd67910
[   31.613563] RBP: ffff88007ad97b98 R08: 0000000000000200 R09: 0000000000000000
[   31.613563] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
[   31.613563] R13: ffff88007cd67780 R14: 0000000000000000 R15: 0000000000000000
[   31.613563] FS:  0000000000000000(0000) GS:ffff88007fd80000(0000) knlGS:0000000000000000
[   31.613563] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[   31.613563] CR2: 000000000000000c CR3: 000000007afd9000 CR4: 00000000000006e0
[   31.613563] Stack:
[   31.613563]  ffff88007ad97b98 ffffffff81c68fd6 ffffffff81c68f20 ffff88007c8e37c8
[   31.613563]  000000007b33d728 ffff88007dc805b0 ffff88007ad97c58 0000000000000200
[   31.613563]  ffff88007cd67780 ffff880000c00040 ffff88007ad97c00 ffff88007cd67910
[   31.613563] Call Trace:
[   31.613563]  [<ffffffff81c68fd6>] ? inet_sendpage+0xb6/0x130
[   31.613563]  [<ffffffff81c68f20>] ? inet_dgram_connect+0x80/0x80
[   31.613563]  [<ffffffff8197bd95>] iscsi_sw_tcp_pdu_xmit+0xe5/0x2e0
[   31.613563]  [<ffffffff8197badf>] ? iscsi_sw_tcp_pdu_init+0x1bf/0x390
[   31.613563]  [<ffffffff81979b82>] iscsi_tcp_task_xmit+0xa2/0x2b0
[   31.613563]  [<ffffffff81974815>] ? iscsi_xmit_task+0x45/0xd0
[   31.613563]  [<ffffffff810fbb8d>] ? trace_hardirqs_on+0xd/0x10
[   31.613563]  [<ffffffff810b54a0>] ? __local_bh_enable_ip+0x70/0xd0
[   31.613563]  [<ffffffff81974829>] iscsi_xmit_task+0x59/0xd0
[   31.613563]  [<ffffffff81978468>] iscsi_xmitworker+0x288/0x330
[   31.613563]  [<ffffffff810cc847>] process_one_work+0x1c7/0x490
[   31.613563]  [<ffffffff810cc7dd>] ? process_one_work+0x15d/0x490
[   31.613563]  [<ffffffff810cd539>] worker_thread+0x119/0x4f0
[   31.613563]  [<ffffffff810fbb8d>] ? trace_hardirqs_on+0xd/0x10
[   31.613563]  [<ffffffff810cd420>] ? init_pwq+0x190/0x190
[   31.613563]  [<ffffffff810d3c3f>] kthread+0xdf/0x100
[   31.613563]  [<ffffffff810d3b60>] ? __init_kthread_worker+0x70/0x70
[   31.613563]  [<ffffffff81d904bc>] ret_from_fork+0x7c/0xb0
[   31.613563]  [<ffffffff810d3b60>] ? __init_kthread_worker+0x70/0x70
[   31.613563] Code: 89 03 31 c0 e9 cc fe ff ff 0f 1f 44 00 00 48 8b 7b
30 e8 17 74 de ff 8b 53 10 c7 43 40 00 00 00 00 48 89 43 30 44 89 f6 48
89 df <8b> 40 0c 48 c7 03 00 00 00 00 2b 53 14 39 c2 0f 47 d0 89 53 08 


(gdb) l *(iscsi_tcp_segment_done+0x2bd)
0xffffffff8197b38d is in iscsi_tcp_segment_done
(../drivers/scsi/libiscsi_tcp.c:102).
97	iscsi_tcp_segment_init_sg(struct iscsi_segment *segment,
98				  struct scatterlist *sg, unsigned int offset)
99	{
100		segment->sg = sg;
101		segment->sg_offset = offset;
102		segment->size = min(sg->length - offset,
103				    segment->total_size - segment->total_copied);
104		segment->data = NULL;
105	}
106	

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

[Mike Christie]

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

Can you do bidi and dif/dix? If so, then instead of using blk_rq_bytes
directly should it use the scsi_out/scsi_in macros and access the length
through the scsi_data_buffer?

This does not fix Christoph's bug in the other mail. Just noticed it
while looking at the code.


> +	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 */
> 

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

[Martin K. Petersen]

>>>>> "Mike" == Mike Christie <michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org> writes:
>> + unsigned int xfer_len = blk_rq_bytes(scmd->request);

Mike> Can you do bidi and dif/dix? 

Nope.

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

[Mike Christie]

On 06/23/2014 03:59 PM, Christoph Hellwig wrote:
> This patch causes a regression when using the iscsi initiator over
> TCP for me. When mounting a newly created ext4 filesystem I get the
> following BUG: 
> 
> [   31.611803] BUG: unable to handle kernel NULL pointer dereference at 000000000000000c
> [   31.613563] IP: [<ffffffff8197b38d>] iscsi_tcp_segment_done+0x2bd/0x380
> [   31.613563] PGD 7a3e4067 PUD 7a45f067 PMD 0 
> [   31.613563] Oops: 0000 [#1] SMP 
> [   31.613563] Modules linked in:
> [   31.613563] CPU: 3 PID: 3739 Comm: kworker/u8:5 Not tainted 3.16.0-rc2 #187
> [   31.613563] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
> [   31.613563] Workqueue: iscsi_q_2 iscsi_xmitworker
> [   31.613563] task: ffff88007b33cf10 ti: ffff88007ad94000 task.ti: ffff88007ad94000
> [   31.613563] RIP: 0010:[<ffffffff8197b38d>]  [<ffffffff8197b38d>] iscsi_tcp_segment_done+0x2bd/0x380
> [   31.613563] RSP: 0018:ffff88007ad97b38  EFLAGS: 00010246
> [   31.613563] RAX: 0000000000000000 RBX: ffff88007cd67910 RCX: 0000000000000200
> [   31.613563] RDX: 0000000000002000 RSI: 0000000000000000 RDI: ffff88007cd67910
> [   31.613563] RBP: ffff88007ad97b98 R08: 0000000000000200 R09: 0000000000000000
> [   31.613563] R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
> [   31.613563] R13: ffff88007cd67780 R14: 0000000000000000 R15: 0000000000000000
> [   31.613563] FS:  0000000000000000(0000) GS:ffff88007fd80000(0000) knlGS:0000000000000000
> [   31.613563] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [   31.613563] CR2: 000000000000000c CR3: 000000007afd9000 CR4: 00000000000006e0
> [   31.613563] Stack:
> [   31.613563]  ffff88007ad97b98 ffffffff81c68fd6 ffffffff81c68f20 ffff88007c8e37c8
> [   31.613563]  000000007b33d728 ffff88007dc805b0 ffff88007ad97c58 0000000000000200
> [   31.613563]  ffff88007cd67780 ffff880000c00040 ffff88007ad97c00 ffff88007cd67910
> [   31.613563] Call Trace:
> [   31.613563]  [<ffffffff81c68fd6>] ? inet_sendpage+0xb6/0x130
> [   31.613563]  [<ffffffff81c68f20>] ? inet_dgram_connect+0x80/0x80
> [   31.613563]  [<ffffffff8197bd95>] iscsi_sw_tcp_pdu_xmit+0xe5/0x2e0
> [   31.613563]  [<ffffffff8197badf>] ? iscsi_sw_tcp_pdu_init+0x1bf/0x390
> [   31.613563]  [<ffffffff81979b82>] iscsi_tcp_task_xmit+0xa2/0x2b0
> [   31.613563]  [<ffffffff81974815>] ? iscsi_xmit_task+0x45/0xd0
> [   31.613563]  [<ffffffff810fbb8d>] ? trace_hardirqs_on+0xd/0x10
> [   31.613563]  [<ffffffff810b54a0>] ? __local_bh_enable_ip+0x70/0xd0
> [   31.613563]  [<ffffffff81974829>] iscsi_xmit_task+0x59/0xd0
> [   31.613563]  [<ffffffff81978468>] iscsi_xmitworker+0x288/0x330
> [   31.613563]  [<ffffffff810cc847>] process_one_work+0x1c7/0x490
> [   31.613563]  [<ffffffff810cc7dd>] ? process_one_work+0x15d/0x490
> [   31.613563]  [<ffffffff810cd539>] worker_thread+0x119/0x4f0
> [   31.613563]  [<ffffffff810fbb8d>] ? trace_hardirqs_on+0xd/0x10
> [   31.613563]  [<ffffffff810cd420>] ? init_pwq+0x190/0x190
> [   31.613563]  [<ffffffff810d3c3f>] kthread+0xdf/0x100
> [   31.613563]  [<ffffffff810d3b60>] ? __init_kthread_worker+0x70/0x70
> [   31.613563]  [<ffffffff81d904bc>] ret_from_fork+0x7c/0xb0
> [   31.613563]  [<ffffffff810d3b60>] ? __init_kthread_worker+0x70/0x70
> [   31.613563] Code: 89 03 31 c0 e9 cc fe ff ff 0f 1f 44 00 00 48 8b 7b
> 30 e8 17 74 de ff 8b 53 10 c7 43 40 00 00 00 00 48 89 43 30 44 89 f6 48
> 89 df <8b> 40 0c 48 c7 03 00 00 00 00 2b 53 14 39 c2 0f 47 d0 89 53 08 
> 
> 
> (gdb) l *(iscsi_tcp_segment_done+0x2bd)
> 0xffffffff8197b38d is in iscsi_tcp_segment_done
> (../drivers/scsi/libiscsi_tcp.c:102).
> 97	iscsi_tcp_segment_init_sg(struct iscsi_segment *segment,
> 98				  struct scatterlist *sg, unsigned int offset)
> 99	{
> 100		segment->sg = sg;
> 101		segment->sg_offset = offset;
> 102		segment->size = min(sg->length - offset,
> 103				    segment->total_size - segment->total_copied);
> 104		segment->data = NULL;
> 105	}
> 106	
> 


Ok, it looks like scsi_out(scsi_cmnd)->length (iscsi_tcp/libiscsi_tcp
still uses that for lower level operations since it was not converted to
support t10 pi) returns a different value than scsi_transfer_length()
(libiscsi uses this for higher level operations when it was converted to
t10 support since iser uses that module and also has t10 support) for
some commands. We then end up incorrectly thinking some requests are the
wrong size and then hit this. Looking into why exactly this happens.

--
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 v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

[Mike Christie]

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-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Martin K. Petersen <martin.petersen-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> ---
>  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.
--
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 v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

[Martin K. Petersen]

>>>>> "Mike" == Mike Christie <michaelc@cs.wisc.edu> 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 <hch@infradead.org>
Debugged-by: Mike Christie <michaelc@cs.wisc.edu>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

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;
 

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

[sagi grimberg]

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.

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

[Sagi Grimberg]

On 6/24/2014 3:53 PM, Martin K. Petersen wrote:
>>>>>> "Mike" == Mike Christie <michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org> 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.

Ohhh, didn't notice this one and sent out a duplicate...

The patch looks good to me obviously.

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 v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

[Christoph Hellwig]

On Tue, Jun 24, 2014 at 08:53:25AM -0400, Martin K. Petersen wrote:
> Oh, I see. So things break because iSCSI uses scsi_transfer_length()
> where the scatterlist length was used in the past.
> 
> How about this?

This fixes the regression for me.

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

[Christoph Hellwig]

On Tue, Jun 24, 2014 at 04:08:25PM +0300, Sagi Grimberg wrote:
> >Oh, I see. So things break because iSCSI uses scsi_transfer_length()
> >where the scatterlist length was used in the past.
> 
> Ohhh, didn't notice this one and sent out a duplicate...
> 
> The patch looks good to me obviously.

Can you give me a real reviewed-by?

> 
> Sagi.
---end quoted text---

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

[sagi grimberg]

On 6/24/2014 3:53 PM, Martin K. Petersen wrote:
>
>
> 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 <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
> Debugged-by: Mike Christie <michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org>
> Signed-off-by: Martin K. Petersen <martin.petersen-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>
> 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;
>   

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

--
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 v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

[Michael Christie]

On Jun 24, 2014, at 7:53 AM, Martin K. Petersen <martin.petersen@oracle.com> wrote:

>>>>>> "Mike" == Mike Christie <michaelc@cs.wisc.edu> 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 <hch@infradead.org>
> Debugged-by: Mike Christie <michaelc@cs.wisc.edu>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> 
> 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;

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

[Christoph Hellwig]

On Tue, Jun 24, 2014 at 11:08:54AM -0500, Michael Christie wrote:
> > 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;

For non-bidi commands those are the same, but I suspect we'd need
something like that for bidi commands.

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

[Sagi Grimberg]

On 6/24/2014 7:08 PM, Michael Christie wrote:
> On Jun 24, 2014, at 7:53 AM, Martin K. Petersen <martin.petersen@oracle.com> wrote:
>
>>>>>>> "Mike" == Mike Christie <michaelc@cs.wisc.edu> 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 <hch@infradead.org>
>> Debugged-by: Mike Christie <michaelc@cs.wisc.edu>
>> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
>>
>> 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.


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;

Sagi.

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

[Christoph Hellwig]

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.

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

[Martin K. Petersen]

>>>>> "Mike" == Michael Christie <michaelc@cs.wisc.edu> writes:

Mike> Do we need to check for the data direction. Something like

Mike> if (scmd->sc_data_direction == DMA_TO_DEVICE)
Mike> 	xfer_len = scsi_out(scmnd)->length;
Mike> else
Mike> 	xfer_len = scsi_in(scmnd)->length;

I guess that depends on the context the wrapper is called in. I think
iscsi is the only place where there's a distinction thanks to bidi.

Looks like there are several places where that's done. In that case I
wonder if we should have explicit scsi_in_transfer_length() and
scsi_out_transfer_length() wrappers?

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Mike Christie]

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 <michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org>
--
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 v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

[Martin K. Petersen]

>>>>> "Mike" == Mike Christie <michaelc@cs.wisc.edu> 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?

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Mike Christie]

On 06/24/2014 11:31 AM, Martin K. Petersen wrote:
>>>>>> "Mike" == Michael Christie <michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org> writes:
> 
> Mike> Do we need to check for the data direction. Something like
> 
> Mike> if (scmd->sc_data_direction == DMA_TO_DEVICE)
> Mike> 	xfer_len = scsi_out(scmnd)->length;
> Mike> else
> Mike> 	xfer_len = scsi_in(scmnd)->length;
> 
> I guess that depends on the context the wrapper is called in. I think
> iscsi is the only place where there's a distinction thanks to bidi.
> 

We were using it generically, so we did not check if bidi or t10 pi. We
were calling it just expecting it to do the right thing for us.

> Looks like there are several places where that's done. In that case I
> wonder if we should have explicit scsi_in_transfer_length() and
> scsi_out_transfer_length() wrappers?

Yeah, maybe.
--
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 v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

[Mike Christie]

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 <michaelc@cs.wisc.edu>

Actually, let me take this back for a second. I am not sure if that is
right.

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

[Vladislav Bolkhovitin]

Martin K. Petersen, on 06/23/2014 06:58 PM wrote:
>>>>>> "Mike" == Mike Christie <michaelc@cs.wisc.edu> writes:
>>> + unsigned int xfer_len = blk_rq_bytes(scmd->request);
>
> Mike> Can you do bidi and dif/dix?
>
> Nope.

Correction: at the moment.

There is a proposal of READ GATHERED command, which is bidirectional and potentially 
DIF/DIX.

Vlad

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

[Mike Christie]

On 06/24/2014 12:08 PM, Mike Christie wrote:
> 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 <michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org>
> 
> Actually, let me take this back for a second. I am not sure if that 
> is right.

Sagi's patch was not correct because scsi_in was hardcoded to the transfer
len when bidi was used. I made the patch below which should fix both bidi
support in iscsi and also WRITE_SAME (and similar commands) support.

There is one issue/question though. Is this working ok with LIO? I left
scsi_transfer_length() with the same behavior as before assuming LIO was
ok and only the iscsi initiator had suffered a regression.
------------------


[PATCH] iscsi/scsi: Fix transfer len calculation

This patch fixes the following regressions added in commit
d77e65350f2d82dfa0557707d505711f5a43c8fd

1. The iscsi header data length is supposed to be the amount of
data being transferred and not the total length of the operation
like is given with blk_rq_bytes.

2. scsi_transfer_length is used in generic iscsi code paths, but
did not take into account bidi commands.

To fix both issues, this patch adds 2 new helpers that use the
scsi_out/in(cmnd)->length values instead of blk_rq_bytes.

Signed-off-by: Mike Christie <michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org>

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 3d1bc67..ee79cdf 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, transfer_length;
+	unsigned hdrlength, cmd_len;
 	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_transfer_length(sc);
 		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,19 +414,18 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
 		memset(r2t, 0, sizeof(*r2t));
 
 		if (session->imm_data_en) {
-			if (transfer_length >= session->first_burst)
+			if (out_len >= session->first_burst)
 				task->imm_count = min(session->first_burst,
 							conn->max_xmit_dlength);
 			else
-				task->imm_count = min(transfer_length,
+				task->imm_count = min(out_len,
 						      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,
-					       transfer_length) -
+			r2t->data_length = min(session->first_burst, out_len) -
 					       task->imm_count;
 			r2t->data_offset = task->imm_count;
 			r2t->ttt = cpu_to_be32(ISCSI_RESERVED_TAG);
@@ -439,6 +438,7 @@ 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_transfer_length(sc));
 
 		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, transfer_length,
+			  task->itt, scsi_bufflen(sc),
 			  scsi_bidi_cmnd(sc) ? scsi_in(sc)->length : 0,
 			  session->cmdsn,
 			  session->max_cmdsn - session->exp_cmdsn + 1);
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 42ed789..b04812d 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -316,9 +316,9 @@ 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)
+static inline unsigned __scsi_calculate_transfer_length(struct scsi_cmnd *scmd,
+							unsigned int xfer_len)
 {
-	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;
 
@@ -332,4 +332,20 @@ static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
 	return xfer_len + (xfer_len >> ilog2(sector_size)) * 8;
 }
 
+static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
+{
+	return __scsi_calculate_transfer_length(scmd,
+						blk_rq_bytes(scmd->request));
+}
+
+static inline unsigned scsi_in_transfer_length(struct scsi_cmnd *scmd)
+{
+	return __scsi_calculate_transfer_length(scmd, scsi_in(scmd)->length);
+}
+
+static inline unsigned scsi_out_transfer_length(struct scsi_cmnd *scmd)
+{
+	return __scsi_calculate_transfer_length(scmd, scsi_out(scmd)->length);
+}
+
 #endif /* _SCSI_SCSI_CMND_H */


--
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 v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

[Sagi Grimberg]

On 6/25/2014 6:32 AM, Mike Christie wrote:
> On 06/24/2014 12:08 PM, Mike Christie wrote:
>> 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 <michaelc@cs.wisc.edu>
>> Actually, let me take this back for a second. I am not sure if that
>> is right.

Hey Mike,

> Sagi's patch was not correct because scsi_in was hardcoded to the transfer
> len when bidi was used.

Right, should have condition that in the direction. something like:

transfer_length = sc->sc_data_direction == DMA_TO_DEVICE ? 
scsi_out(sc)->length : scsi_in(sc)->length;

would probably hit that in testing before sending out a patch.

> I made the patch below which should fix both bidi
> support in iscsi and also WRITE_SAME (and similar commands) support.

I'm a bit confused, for all commands (bidi or not) the iscsi header data_length
should describe the scsi_data_buffer length, bidi information will lie in AHS header.
(in case bidi will ever co-exist with PI, we might need another helper that will look
in req->special + PI, something like scsi_bidi_transfer_length)

So why not keep scsi_transfer_length to work on sdb length (take scsi_bufflen(scmnd) or
scsi_out(scmnd)->length as MKP suggested) and that's it - don't touch libiscsi.

Let me test that.

> There is one issue/question though. Is this working ok with LIO? I left
> scsi_transfer_length() with the same behavior as before assuming LIO was
> ok and only the iscsi initiator had suffered a regression.

I think that if we go with scsi_in/out_transfer_length then 
scsi_transfer_length should be removed
and LIO can be modified to use scsi_in/out_transfer_length.

> ------------------
>
>
> [PATCH] iscsi/scsi: Fix transfer len calculation

I'll comment on the patch as well if we decide to go this way.

> This patch fixes the following regressions added in commit
> d77e65350f2d82dfa0557707d505711f5a43c8fd
>
> 1. The iscsi header data length is supposed to be the amount of
> data being transferred and not the total length of the operation
> like is given with blk_rq_bytes.
>
> 2. scsi_transfer_length is used in generic iscsi code paths, but
> did not take into account bidi commands.
>
> To fix both issues, this patch adds 2 new helpers that use the
> scsi_out/in(cmnd)->length values instead of blk_rq_bytes.
>
> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
>
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 3d1bc67..ee79cdf 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, transfer_length;
> +	unsigned hdrlength, cmd_len;
>   	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_transfer_length(sc);
>   		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,19 +414,18 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
>   		memset(r2t, 0, sizeof(*r2t));
>   
>   		if (session->imm_data_en) {
> -			if (transfer_length >= session->first_burst)
> +			if (out_len >= session->first_burst)
>   				task->imm_count = min(session->first_burst,
>   							conn->max_xmit_dlength);
>   			else
> -				task->imm_count = min(transfer_length,
> +				task->imm_count = min(out_len,
>   						      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,
> -					       transfer_length) -
> +			r2t->data_length = min(session->first_burst, out_len) -
>   					       task->imm_count;
>   			r2t->data_offset = task->imm_count;
>   			r2t->ttt = cpu_to_be32(ISCSI_RESERVED_TAG);
> @@ -439,6 +438,7 @@ 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_transfer_length(sc));

In light of last comment from Vlad where bidi and PI may co-exist, 
shouldn't
scsi_in_transfer_length(sc) be used also in iscsi_prep_bidi_ahs()? and 
also in the print below?

>   
>   		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, transfer_length,
> +			  task->itt, scsi_bufflen(sc),

In the DIF case length print would be wrong (doesn't include PI).

>   			  scsi_bidi_cmnd(sc) ? scsi_in(sc)->length : 0,
>   			  session->cmdsn,
>   			  session->max_cmdsn - session->exp_cmdsn + 1);
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 42ed789..b04812d 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -316,9 +316,9 @@ 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)
> +static inline unsigned __scsi_calculate_transfer_length(struct scsi_cmnd *scmd,
> +							unsigned int xfer_len)
>   {
> -	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;
>   
> @@ -332,4 +332,20 @@ static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
>   	return xfer_len + (xfer_len >> ilog2(sector_size)) * 8;
>   }
>   
> +static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
> +{
> +	return __scsi_calculate_transfer_length(scmd,
> +						blk_rq_bytes(scmd->request));
> +}
> +
> +static inline unsigned scsi_in_transfer_length(struct scsi_cmnd *scmd)
> +{
> +	return __scsi_calculate_transfer_length(scmd, scsi_in(scmd)->length);
> +}
> +
> +static inline unsigned scsi_out_transfer_length(struct scsi_cmnd *scmd)
> +{
> +	return __scsi_calculate_transfer_length(scmd, scsi_out(scmd)->length);
> +}
> +
>   #endif /* _SCSI_SCSI_CMND_H */
>
>

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

[Christoph Hellwig]

Mike, I'd prefer a fix on top of the core-for-3.16 branch in my
scsi-queue tree, which already has the fix from Martin.

I also really don't like these three confusing helpers:

> +static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
> +{
> +	return __scsi_calculate_transfer_length(scmd,
> +						blk_rq_bytes(scmd->request));
> +}

So here we use blk_rq_bytes still, which is incorrect for WRITE SAME.

> +static inline unsigned scsi_in_transfer_length(struct scsi_cmnd *scmd)
> +{
> +	return __scsi_calculate_transfer_length(scmd, scsi_in(scmd)->length);
> +}
> +
> +static inline unsigned scsi_out_transfer_length(struct scsi_cmnd *scmd)
> +{
> +	return __scsi_calculate_transfer_length(scmd, scsi_out(scmd)->length);

And here we use the in/out length.  And no documentation whatsover which
one you'd want to choose.

I think the easiest fix is to just pass a scsi_data_buffer to
scsi_transfer_length(), and let the caller use scsi_in/scsi_out to find
the right one.

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

[Christoph Hellwig]

> >Sagi's patch was not correct because scsi_in was hardcoded to the transfer
> >len when bidi was used.
> 
> Right, should have condition that in the direction. something like:
> 
> transfer_length = sc->sc_data_direction == DMA_TO_DEVICE ?
> scsi_out(sc)->length : scsi_in(sc)->length;
> 
> would probably hit that in testing before sending out a patch.

We could also pass the dma direction indeed.  Compared to my suggestion
of passing thr scsi_data_buffer this makes life a lot easier for drivers
that don't try to support the bidi madness.

> >There is one issue/question though. Is this working ok with LIO? I left
> >scsi_transfer_length() with the same behavior as before assuming LIO was
> >ok and only the iscsi initiator had suffered a regression.
> 
> I think that if we go with scsi_in/out_transfer_length then
> scsi_transfer_length should be removed
> and LIO can be modified to use scsi_in/out_transfer_length.

drivers/target/loopback/tcm_loop.c doesn't (and absolutely shouldn't!)
use scsi_transfer_length in target context, it uses it in it's initiator
side in the same way as iscsi, so the same semantics apply.

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

[Sagi Grimberg]

On 6/25/2014 11:48 AM, Sagi Grimberg wrote:

<SNIP>
>
>> I made the patch below which should fix both bidi
>> support in iscsi and also WRITE_SAME (and similar commands) support.
>
> I'm a bit confused, for all commands (bidi or not) the iscsi header 
> data_length
> should describe the scsi_data_buffer length, bidi information will lie 
> in AHS header.
> (in case bidi will ever co-exist with PI, we might need another helper 
> that will look
> in req->special + PI, something like scsi_bidi_transfer_length)
>
> So why not keep scsi_transfer_length to work on sdb length (take 
> scsi_bufflen(scmnd) or
> scsi_out(scmnd)->length as MKP suggested) and that's it - don't touch 
> libiscsi.
>
> Let me test that.
>

So I tested a bidirectional command using:
$ sg_raw --infile=/root/filex --send=1024 --request=1024 
--outfile=/root/filex "/dev/bsg/7:0:0:0" 53 00 00 00 00 00 00 00 02 00

And I see:
kernel: session1: iscsi_prep_scsi_cmd_pdu iscsi prep [bidirectional cid 
0 sc ffff880468ca1e00 cdb 0x53 itt 0x16 len 1024 bidi_len 1024 cmdsn 223 
win 64]

This confirms what I wrote above, so AFAICT no additional fix is 
required for libiscsi wrt bidi commands support.

Mike?

Sagi.

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

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:

Christoph> So here we use blk_rq_bytes still, which is incorrect for
Christoph> WRITE SAME.

Yeah, scsi_transfer_length() needs to go away completely if we go with
the in and out variants.

Christoph> I think the easiest fix is to just pass a scsi_data_buffer to
Christoph> scsi_transfer_length(), and let the caller use
Christoph> scsi_in/scsi_out to find the right one.

I'm perfectly OK with that approach.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Christoph Hellwig]

On Wed, Jun 25, 2014 at 01:32:39PM +0300, Sagi Grimberg wrote:
> So I tested a bidirectional command using:
> $ sg_raw --infile=/root/filex --send=1024 --request=1024
> --outfile=/root/filex "/dev/bsg/7:0:0:0" 53 00 00 00 00 00 00 00 02 00
> 
> And I see:
> kernel: session1: iscsi_prep_scsi_cmd_pdu iscsi prep [bidirectional cid 0 sc
> ffff880468ca1e00 cdb 0x53 itt 0x16 len 1024 bidi_len 1024 cmdsn 223 win 64]
> 
> This confirms what I wrote above, so AFAICT no additional fix is required
> for libiscsi wrt bidi commands support.

Good to know.  I'd really prefer just going with the fix from Martin
that I have already queued up for 3.16, and then we can have the fully
fledged out "new" scsi_transfer_length() for 3.17.

--
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 v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

[Michael Christie]

On Jun 25, 2014, at 6:35 AM, Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:

> On Wed, Jun 25, 2014 at 01:32:39PM +0300, Sagi Grimberg wrote:
>> So I tested a bidirectional command using:
>> $ sg_raw --infile=/root/filex --send=1024 --request=1024
>> --outfile=/root/filex "/dev/bsg/7:0:0:0" 53 00 00 00 00 00 00 00 02 00
>> 
>> And I see:
>> kernel: session1: iscsi_prep_scsi_cmd_pdu iscsi prep [bidirectional cid 0 sc
>> ffff880468ca1e00 cdb 0x53 itt 0x16 len 1024 bidi_len 1024 cmdsn 223 win 64]
>> 
>> This confirms what I wrote above, so AFAICT no additional fix is required
>> for libiscsi wrt bidi commands support.
> 
> Good to know.  I'd really prefer just going with the fix from Martin
> that I have already queued up for 3.16, and then we can have the fully
> fledged out "new" scsi_transfer_length() for 3.17.
> 

I am fine with this too.

I was way off track. I was more concerned with not wanting to use multiple functions/macros to get the transfer len. That should definitely be done when there is more time.
--
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 v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

[Bart Van Assche]

On 06/11/14 11:09, Sagi Grimberg wrote:
> +	return xfer_len + (xfer_len >> ilog2(sector_size)) * 8;

Sorry that I just noticed this now, but why is a shift-right and ilog2()
used in the above expression instead of just dividing the transfer
length by the sector size ?

Thanks,

Bart.

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

[James Bottomley]

On Thu, 2014-06-26 at 16:53 +0200, Bart Van Assche wrote:
> On 06/11/14 11:09, Sagi Grimberg wrote:
> > +	return xfer_len + (xfer_len >> ilog2(sector_size)) * 8;
> 
> Sorry that I just noticed this now, but why is a shift-right and ilog2()
> used in the above expression instead of just dividing the transfer
> length by the sector size ?

It's a performance thing.  Division is really slow on most CPUs.
However, we know the divisor is a power of two so we re-express the
division as a shift, which the processor can do really fast.

James


--
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 v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

[Atchley, Scott]

On Jun 26, 2014, at 10:55 AM, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Thu, 2014-06-26 at 16:53 +0200, Bart Van Assche wrote:
>> On 06/11/14 11:09, Sagi Grimberg wrote:
>>> +	return xfer_len + (xfer_len >> ilog2(sector_size)) * 8;
>> 
>> Sorry that I just noticed this now, but why is a shift-right and ilog2()
>> used in the above expression instead of just dividing the transfer
>> length by the sector size ?
> 
> It's a performance thing.  Division is really slow on most CPUs.
> However, we know the divisor is a power of two so we re-express the
> division as a shift, which the processor can do really fast.
> 
> James

I have done this in the past as well, but have you benchmarked it? Compilers typically do the right thing in this case (i.e replace division with shift).

Scott

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

[James Bottomley]

On June 26, 2014 11:41:48 AM EDT, "Atchley, Scott" <atchleyes@ornl.gov> wrote:
>On Jun 26, 2014, at 10:55 AM, James Bottomley
><James.Bottomley@HansenPartnership.com> wrote:
>
>> On Thu, 2014-06-26 at 16:53 +0200, Bart Van Assche wrote:
>>> On 06/11/14 11:09, Sagi Grimberg wrote:
>>>> +	return xfer_len + (xfer_len >> ilog2(sector_size)) * 8;
>>> 
>>> Sorry that I just noticed this now, but why is a shift-right and
>ilog2()
>>> used in the above expression instead of just dividing the transfer
>>> length by the sector size ?
>> 
>> It's a performance thing.  Division is really slow on most CPUs.
>> However, we know the divisor is a power of two so we re-express the
>> division as a shift, which the processor can do really fast.
>> 
>> James
>
>I have done this in the past as well, but have you benchmarked it?
>Compilers typically do the right thing in this case (i.e replace
>division with shift).

The compiler can only do that for values which are reducible to constants at compile time. This is a runtime value, the compiler has no way of deducing that it will be a power of 2

James
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

[Atchley, Scott]

On Jun 26, 2014, at 12:38 PM, James Bottomley <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:

> On June 26, 2014 11:41:48 AM EDT, "Atchley, Scott" <atchleyes-1Heg1YXhbW8@public.gmane.org> wrote:
>> On Jun 26, 2014, at 10:55 AM, James Bottomley
>> <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
>> 
>>> On Thu, 2014-06-26 at 16:53 +0200, Bart Van Assche wrote:
>>>> On 06/11/14 11:09, Sagi Grimberg wrote:
>>>>> +	return xfer_len + (xfer_len >> ilog2(sector_size)) * 8;
>>>> 
>>>> Sorry that I just noticed this now, but why is a shift-right and
>> ilog2()
>>>> used in the above expression instead of just dividing the transfer
>>>> length by the sector size ?
>>> 
>>> It's a performance thing.  Division is really slow on most CPUs.
>>> However, we know the divisor is a power of two so we re-express the
>>> division as a shift, which the processor can do really fast.
>>> 
>>> James
>> 
>> I have done this in the past as well, but have you benchmarked it?
>> Compilers typically do the right thing in this case (i.e replace
>> division with shift).
> 
> The compiler can only do that for values which are reducible to constants at compile time. This is a runtime value, the compiler has no way of deducing that it will be a power of 2
> 
> James

You're right, I should have said runtime.

However, gcc on Intel seems to choose the right algorithm at runtime. On a trivial app with -O0, I see the same performance for shift and division if the divisor is a power of two. Is see ~38% penalty if the divisor is not a power of 2. With -O3, shift is faster than division by about ~17% when the divisor is a power of two.

Scott--
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 v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

[Christoph Hellwig]

Sagi,

can you send an update for the core-for-3.17 tree to pass a
dma_direction or the scsi data buffer to scsi_transfer_length so we can
make safe for bidi usage in the future?

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

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:

Christoph> Sagi, can you send an update for the core-for-3.17 tree to
Christoph> pass a dma_direction or the scsi data buffer to
Christoph> scsi_transfer_length so we can make safe for bidi usage in
Christoph> the future?

I have cleaned this up in my integrity patch set since we already have
the flag to determine whether to transfer PI or not.

I'll get those patches out today.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH] [SCSI] Make scsi_transfer_length take a scsi_data_buffer argument

[Martin K. Petersen]

For bidirectional commands we need to be able to distinguish between the
in and out scsi_data_buffers when calculating the wire transfer length.
Make scsi_transfer_length() take a scsi_data_buffer argument so the
caller can choose which I/O direction the calculation should apply to.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Christoph Hellwig <hch@infradead.org>
---
 drivers/scsi/libiscsi.c            | 2 +-
 drivers/target/loopback/tcm_loop.c | 2 +-
 include/scsi/scsi_cmnd.h           | 5 +++--
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index f2db82beb646..fdea8c1527d4 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -391,7 +391,7 @@ 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);
+	transfer_length = scsi_transfer_length(sc, scsi_out(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;
diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index 340de9d92b15..c50453df555a 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -213,7 +213,7 @@ static void tcm_loop_submission_work(struct work_struct *work)
 
 	}
 
-	transfer_length = scsi_transfer_length(sc);
+	transfer_length = scsi_transfer_length(sc, scsi_out(sc));
 	if (!scsi_prot_sg_count(sc) &&
 	    scsi_get_prot_op(sc) != SCSI_PROT_NORMAL) {
 		se_cmd->prot_pto = true;
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 73f349044941..42e62cfff515 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -313,9 +313,10 @@ 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)
+static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd,
+					    struct scsi_data_buffer *sdb)
 {
-	unsigned int xfer_len = scsi_out(scmd)->length;
+	unsigned int xfer_len = sdb->length;
 	unsigned int prot_op = scsi_get_prot_op(scmd);
 	unsigned int sector_size = scmd->device->sector_size;
 
-- 
1.9.3

Re: [PATCH] [SCSI] Make scsi_transfer_length take a scsi_data_buffer argument

[Christoph Hellwig]

On Fri, Jul 25, 2014 at 04:00:19PM -0400, Martin K. Petersen wrote:
> For bidirectional commands we need to be able to distinguish between the
> in and out scsi_data_buffers when calculating the wire transfer length.
> Make scsi_transfer_length() take a scsi_data_buffer argument so the
> caller can choose which I/O direction the calculation should apply to.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

Thanks, this looks good to me,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Boaz Harrosh]

On 06/25/2014 04:17 AM, Vladislav Bolkhovitin wrote:
> Martin K. Petersen, on 06/23/2014 06:58 PM wrote:
>>>>>>> "Mike" == Mike Christie <michaelc@cs.wisc.edu> writes:
>>>> + unsigned int xfer_len = blk_rq_bytes(scmd->request);
>>
>> Mike> Can you do bidi and dif/dix?
>>
>> Nope.
> 
> Correction: at the moment.
> 
> There is a proposal of READ GATHERED command, which is bidirectional and potentially 
> DIF/DIX.
> 

That's all very good. But current infrastructure can not support BIDI+DIFF.
Because you we'll need *two* diff vectors one for each side.

Now in fact at the block layer this is actually supported. Since BIDI is
two requests, and each can have its own DIFF bio. But on the scsi layer
this is not implemented.

So I'd say: *For now DIFF and BIDI are mutually exclusive.*

(You'll need someone to love these new commands a lot in order to
 enable it)

> Vlad
> 

Cheers
Boaz

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

[Boaz Harrosh]

On 06/25/2014 01:32 PM, Sagi Grimberg wrote:
> On 6/25/2014 11:48 AM, Sagi Grimberg wrote:
> 
> <SNIP>
>>
>>> I made the patch below which should fix both bidi
>>> support in iscsi and also WRITE_SAME (and similar commands) support.
>>
>> I'm a bit confused, for all commands (bidi or not) the iscsi header 
>> data_length
>> should describe the scsi_data_buffer length, bidi information will lie 
>> in AHS header.
>> (in case bidi will ever co-exist with PI, we might need another helper 
>> that will look
>> in req->special + PI, something like scsi_bidi_transfer_length)
>>
>> So why not keep scsi_transfer_length to work on sdb length (take 
>> scsi_bufflen(scmnd) or
>> scsi_out(scmnd)->length as MKP suggested) and that's it - don't touch 
>> libiscsi.
>>
>> Let me test that.
>>
> 
> So I tested a bidirectional command using:
> $ sg_raw --infile=/root/filex --send=1024 --request=1024 
> --outfile=/root/filex "/dev/bsg/7:0:0:0" 53 00 00 00 00 00 00 00 02 00
> 
> And I see:
> kernel: session1: iscsi_prep_scsi_cmd_pdu iscsi prep [bidirectional cid 
> 0 sc ffff880468ca1e00 cdb 0x53 itt 0x16 len 1024 bidi_len 1024 cmdsn 223 
> win 64]
> 

This is a very bad example and tested nothing, since len && bidi_len
are the same. So even if you had a bug and took length from the
wrong side it would come out the same.

You must test with a bidi command that has two different lengths for
each side

If you have a tree that you want me to test I will be glad too.
>From this thread I'm confused as to what patches you want me to
test? please point me to a tree you need testing. You can bug me
any time, any tree. I will be happy to test.

Cheers
Boaz

> This confirms what I wrote above, so AFAICT no additional fix is 
> required for libiscsi wrt bidi commands support.
> 
> Mike?
> 
> Sagi.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Boaz Harrosh]

On 06/11/2014 12:09 PM, 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-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Mike Christie <michaelc-hcNo3dDEHLuVc3sceRu5cw@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..8d44a40 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 = data_buf->data_len >>
> +				     ilog2(sc->device->sector_size) * 8;
>  	}
>  
>  	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;

I hate that you introduced this new transfer_length variable. It does
not exist. In BIDI supporting driver there is out_len and in_len just
as original code.

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

In light of all the comments and the obvious bugs, please just
re do this patch. Do not just later fix this one.

All you need is:
+		unsigned out_len = scsi_transfer_length(sc ,scsi_out(sc));

Also I would love if you left the addition to the user .I.E

		out_len += scsi_proto_length(sc ,scsi_out(sc));

This way we can see that this addition is because of scsi_proto and that
this particular driver puts them together in the same payload. There can be
other DIFF supporting drivers that put the DIFF payload on another stream / another
SG list, like the sata one, right?

Then  scsi_transfer_length() becomes:
static inline unsigned scsi_proto_length(struct scsi_cmnd *scmd, scsi_data_buffer *sdb)
{
	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 0;
	}

	return (sdb->length >> ilog2(sector_size)) * 8;
}


>  		struct iscsi_r2t_info *r2t = &task->unsol_r2t;
>  
> -		hdr->data_length = cpu_to_be32(out_len);

And this stays as is

>  		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)

And this stays as is

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

And this stays as is

>  			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) -

And this stays as is

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

And this stays as is

BTW: When reading DIFF devices, don't you have extra bits to read as well?
     How does the DIFF read works? Please get me up to speed. I'm not familiar
     with this protocol? 
     (I'd imagine that if say an app reads X bytes with DIFF info, it wants to
      receive its DIFF checksome for every sector in X, where is this info
      on the iscsi wire?)

>  
>  		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,

And this

This print is correct as it covers all cases. If you want to print the adjusted
length then OK, you'd need to store this I guess, but store it as a different
variable like proto_length and print it as an additional variable.
The current print is one-to-one with what the caller requested, FS wrote X bytes.
If any was added for proto I'd like to see both prints.

>  			  scsi_bidi_cmnd(sc) ? scsi_in(sc)->length : 0,
>  			  session->cmdsn,
>  			  session->max_cmdsn - session->exp_cmdsn + 1);
> 


Rrrr

I see that this patch is already in mainline. I'd like to see the fixing
patch reverting all these wrong changes to the code and only leaving
the single needed change above.

I'll send a PATCH over linus/master later today.

Thanks
Boaz

--
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 v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

[Christoph Hellwig]

On Sun, Jul 27, 2014 at 12:11:11PM +0300, Boaz Harrosh wrote:
> If you have a tree that you want me to test I will be glad too.
> >From this thread I'm confused as to what patches you want me to
> test? please point me to a tree you need testing. You can bug me
> any time, any tree. I will be happy to test.

You're about a month late to the game :)

I think everything is sorted out properly, but if you want to verify
it yourself just test the latest 3.16 release candidate from Linus.

--
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] [SCSI] Make scsi_transfer_length take a scsi_data_buffer argument

[Christoph Hellwig]

Can I get another review for this one?  Sami?

On Fri, Jul 25, 2014 at 02:19:27PM -0700, Christoph Hellwig wrote:
> On Fri, Jul 25, 2014 at 04:00:19PM -0400, Martin K. Petersen wrote:
> > For bidirectional commands we need to be able to distinguish between the
> > in and out scsi_data_buffers when calculating the wire transfer length.
> > Make scsi_transfer_length() take a scsi_data_buffer argument so the
> > caller can choose which I/O direction the calculation should apply to.
> > 
> > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> 
> Thanks, this looks good to me,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
---end quoted text---

Re: [PATCH] [SCSI] Make scsi_transfer_length take a scsi_data_buffer argument

[Sagi Grimberg]

On 7/25/2014 11:00 PM, Martin K. Petersen wrote:
> For bidirectional commands we need to be able to distinguish between the
> in and out scsi_data_buffers when calculating the wire transfer length.
> Make scsi_transfer_length() take a scsi_data_buffer argument so the
> caller can choose which I/O direction the calculation should apply to.
>

Hey Martin, Christoph,

Sorry for the late response, I was on vacation.
Some comments below.

> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> ---
>   drivers/scsi/libiscsi.c            | 2 +-
>   drivers/target/loopback/tcm_loop.c | 2 +-
>   include/scsi/scsi_cmnd.h           | 5 +++--
>   3 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index f2db82beb646..fdea8c1527d4 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -391,7 +391,7 @@ 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);
> +	transfer_length = scsi_transfer_length(sc, scsi_out(sc));

If we go down this road we should return to the in_len/out_len
approach that was used before since taking scsi_out(sc) doesn't make
sense for READs (although it would work).

>   	hdr->data_length = cpu_to_be32(transfer_length);
>   	if (sc->sc_data_direction == DMA_TO_DEVICE) {
>   		struct iscsi_r2t_info *r2t = &task->unsol_r2t;
> diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
> index 340de9d92b15..c50453df555a 100644
> --- a/drivers/target/loopback/tcm_loop.c
> +++ b/drivers/target/loopback/tcm_loop.c
> @@ -213,7 +213,7 @@ static void tcm_loop_submission_work(struct work_struct *work)
>
>   	}
>
> -	transfer_length = scsi_transfer_length(sc);
> +	transfer_length = scsi_transfer_length(sc, scsi_out(sc));

Again, this will work but it implies out direction.

Sagi.

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

[Sagi Grimberg]

On 7/27/2014 12:11 PM, Boaz Harrosh wrote:
> On 06/25/2014 01:32 PM, Sagi Grimberg wrote:
>> On 6/25/2014 11:48 AM, Sagi Grimberg wrote:
>>
>> <SNIP>
>>>
>>>> I made the patch below which should fix both bidi
>>>> support in iscsi and also WRITE_SAME (and similar commands) support.
>>>
>>> I'm a bit confused, for all commands (bidi or not) the iscsi header
>>> data_length
>>> should describe the scsi_data_buffer length, bidi information will lie
>>> in AHS header.
>>> (in case bidi will ever co-exist with PI, we might need another helper
>>> that will look
>>> in req->special + PI, something like scsi_bidi_transfer_length)
>>>
>>> So why not keep scsi_transfer_length to work on sdb length (take
>>> scsi_bufflen(scmnd) or
>>> scsi_out(scmnd)->length as MKP suggested) and that's it - don't touch
>>> libiscsi.
>>>
>>> Let me test that.
>>>
>>
>> So I tested a bidirectional command using:
>> $ sg_raw --infile=/root/filex --send=1024 --request=1024
>> --outfile=/root/filex "/dev/bsg/7:0:0:0" 53 00 00 00 00 00 00 00 02 00
>>
>> And I see:
>> kernel: session1: iscsi_prep_scsi_cmd_pdu iscsi prep [bidirectional cid
>> 0 sc ffff880468ca1e00 cdb 0x53 itt 0x16 len 1024 bidi_len 1024 cmdsn 223
>> win 64]
>>
>
> This is a very bad example and tested nothing, since len && bidi_len
> are the same. So even if you had a bug and took length from the
> wrong side it would come out the same.
>
> You must test with a bidi command that has two different lengths for
> each side
>

Yes, I thought the same thing right after I sent this, so I tested it
with different lengths and it does work. I guess I was lazy in replying
it on top...

Sagi.

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

[Sagi Grimberg]

Hi Boaz,

On 7/27/2014 1:08 PM, Boaz Harrosh wrote:
<SNIP>
>> 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;
>
> I hate that you introduced this new transfer_length variable. It does
> not exist. In BIDI supporting driver there is out_len and in_len just
> as original code.

Effectively, out_len and in_len are the same except for the bidi case.
Moreover, the hdr->data_length is exactly the command scsi data buffer
length, the bidi length is taken from scsi_in when we build the AHS for
bidi (rlen_ahdr->read_length).

So to me it is correct and makes sense. But I'm don't feel so strong
about it...

Mike what's your take on this?

>
>>   	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;
>
> In light of all the comments and the obvious bugs, please just
> re do this patch. Do not just later fix this one.
>
> All you need is:
> +		unsigned out_len = scsi_transfer_length(sc ,scsi_out(sc));
>
> Also I would love if you left the addition to the user .I.E
>
> 		out_len += scsi_proto_length(sc ,scsi_out(sc));
>
> This way we can see that this addition is because of scsi_proto and that
> this particular driver puts them together in the same payload. There can be
> other DIFF supporting drivers that put the DIFF payload on another stream / another
> SG list, like the sata one, right?

I think that DIF specification says that on the wire the data and
protection are interleaved i.e. Block1, DIF1, Block2, DIF2...

So I do think that the transfer length should always include
data_length + protection_length.

>
> BTW: When reading DIFF devices, don't you have extra bits to read as well?
>       How does the DIFF read works? Please get me up to speed. I'm not familiar
>       with this protocol?
>       (I'd imagine that if say an app reads X bytes with DIFF info, it wants to
>        receive its DIFF checksome for every sector in X, where is this info
>        on the iscsi wire?)
>

The application wants to read X bytes from a DIF formatted device, the
initiator will prepare buffers for data and for DIF data (in some
implementations it can be the same buffer but not in Linux), and request
reading X+DIF_size bytes (where each block is followed by it's
corresponding integrity field) and place the data blocks in the data
buffer and the integrity fields in the DIF data buffer (usually this
is offloaded).

>>
>>   		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,
>
> And this
>
> This print is correct as it covers all cases. If you want to print the adjusted
> length then OK, you'd need to store this I guess, but store it as a different
> variable like proto_length and print it as an additional variable.

But it is the transfer length that is sent in iSCSI header. Why do you
want to print it as additional info? for transactions that include DIF
the length is the data + protection.

> The current print is one-to-one with what the caller requested, FS wrote X bytes.

It is still one-to-one isn't it?

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] [SCSI] Make scsi_transfer_length take a scsi_data_buffer argument

[Sagi Grimberg]

On 8/6/2014 3:12 PM, Sagi Grimberg wrote:

<SNIP>
>> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
>> index f2db82beb646..fdea8c1527d4 100644
>> --- a/drivers/scsi/libiscsi.c
>> +++ b/drivers/scsi/libiscsi.c
>> @@ -391,7 +391,7 @@ 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);
>> +    transfer_length = scsi_transfer_length(sc, scsi_out(sc));
>
> If we go down this road we should return to the in_len/out_len
> approach that was used before since taking scsi_out(sc) doesn't make
> sense for READs (although it would work).
>

On second thought, since the transfer length is always the
command scsi data buffer length, why not keep it as is and
if at any point in the future DIF will co-exist with bidi,
we can add scsi_bidi_transfer_length which will calculate
the bidi in length.

In this case, an iSCSI bidi transaction will do:
hdr->data_length = scsi_transfer_legnth(sg);
rlen_ahdr->read_length = scsi_bidi_transfer_length(sc);



Sagi.

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

[Boaz Harrosh]

On 08/06/2014 03:43 PM, Sagi Grimberg wrote:
> Hi Boaz,
> 
<>
>>
>> I hate that you introduced this new transfer_length variable. It does
>> not exist. In BIDI supporting driver there is out_len and in_len just
>> as original code.
> 
> Effectively, out_len and in_len are the same except for the bidi case.
> Moreover, the hdr->data_length is exactly the command scsi data buffer
> length, the bidi length is taken from scsi_in when we build the AHS for
> bidi (rlen_ahdr->read_length).
> 
> So to me it is correct and makes sense. But I'm don't feel so strong
> about it...
> 
> Mike what's your take on this?
> 

I have a patch to clean all that, will send tomorrow. 

What I mean is that this is on the out-path only the in path is different.
See the code this variable is only used in the if (== DMA_TO_DEVICE) case and
should be local to that scope. This is my clean up

<>
>> this particular driver puts them together in the same payload. There can be
>> other DIFF supporting drivers that put the DIFF payload on another stream / another
>> SG list, like the sata one, right?
> 
> I think that DIF specification says that on the wire the data and
> protection are interleaved i.e. Block1, DIF1, Block2, DIF2...
> 

No it does not. This is a per transport, and actually per device host
driver. Yes in iSCSI_tcp they are inline in HW cards they might come as
two different SGs (Like the Linux model). Even with iscsi-offload they might
want to be two SG-lists.

> So I do think that the transfer length should always include
> data_length + protection_length.
> 

This is at the iscsi level. But the scsi_transfer_length() is on the scsi
level which keeps them separate. So I think the proper API should be
	scsi_proto_length()

And for LLDs that want them together they can do scsi_bufflen() + scsi_proto_length()
and for other drivers they can do it separately. Don't infect iscsi level assumptions
on the generic layer API.

Again my patch fixes this.

>> And this
>>
>> This print is correct as it covers all cases. If you want to print the adjusted
>> length then OK, you'd need to store this I guess, but store it as a different
>> variable like proto_length and print it as an additional variable.
> 
> But it is the transfer length that is sent in iSCSI header. Why do you
> want to print it as additional info? 

I want to see what was the length the app/FS sent, then as added
info how much was added for DIFF, your way there is lost information.

> for transactions that include DIF
> the length is the data + protection.
> 
> It is still one-to-one isn't it?
> 

No! the original submitted length is lost from the print

> Sagi.
> 

Shalom
Boaz

Re: [PATCH] [SCSI] Make scsi_transfer_length take a scsi_data_buffer argument

[Martin K. Petersen]

>>>>> "Sagi" == Sagi Grimberg <sagig@dev.mellanox.co.il> writes:

Sagi> On second thought, since the transfer length is always the command
Sagi> scsi data buffer length, why not keep it as is and if at any point
Sagi> in the future DIF will co-exist with bidi, we can add
Sagi> scsi_bidi_transfer_length which will calculate the bidi in length.

Sagi> In this case, an iSCSI bidi transaction will do:

Sagi>	hdr-> data_length = scsi_transfer_legnth(sg);
Sagi>	rlen_ahdr-> read_length = scsi_bidi_transfer_length(sc);

I agree that the distinction between in and out buffers is unnecessary
and a bit of a burden. Unless it's a bidi command there is only one
buffer. I don't think there's an inherent win in having multiple names
for it depending on I/O direction.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Martin K. Petersen]

>>>>> "Sagi" == Sagi Grimberg <sagig-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> writes:

>> BTW: When reading DIFF devices, don't you have extra bits to read as
>> well?  How does the DIFF read works? Please get me up to speed. I'm
>> not familiar with this protocol?  (I'd imagine that if say an app
>> reads X bytes with DIFF info, it wants to receive its DIFF checksome
>> for every sector in X, where is this info on the iscsi wire?)
>> 

Sagi> The application wants to read X bytes from a DIF formatted device,
Sagi> the initiator will prepare buffers for data and for DIF data (in
Sagi> some implementations it can be the same buffer but not in Linux),
Sagi> and request reading X+DIF_size bytes (where each block is followed
Sagi> by it's corresponding integrity field) and place the data blocks
Sagi> in the data buffer and the integrity fields in the DIF data buffer
Sagi> (usually this is offloaded).

READ CAPACITY returns a block size of 512 even though the blocks on disk
are 520. That's the beauty of it.

At the command level all transfers are expressed in number of blocks,
not bytes. The application will see an N x 512 byte buffer but on the
wire between initiator and target we'll transfer N x 520 bytes.

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

[Sagi Grimberg]

On 8/6/2014 4:25 PM, Boaz Harrosh wrote:
> On 08/06/2014 03:43 PM, Sagi Grimberg wrote:
>> Hi Boaz,
>>
> <>
>>>
>>> I hate that you introduced this new transfer_length variable. It does
>>> not exist. In BIDI supporting driver there is out_len and in_len just
>>> as original code.
>>
>> Effectively, out_len and in_len are the same except for the bidi case.
>> Moreover, the hdr->data_length is exactly the command scsi data buffer
>> length, the bidi length is taken from scsi_in when we build the AHS for
>> bidi (rlen_ahdr->read_length).
>>
>> So to me it is correct and makes sense. But I'm don't feel so strong
>> about it...
>>
>> Mike what's your take on this?
>>
>
> I have a patch to clean all that, will send tomorrow.
>
> What I mean is that this is on the out-path only the in path is different.
> See the code this variable is only used in the if (== DMA_TO_DEVICE) case and
> should be local to that scope. This is my clean up
>

Hey Boaz,

So in the current flow, I still don't think it is wrong/buggy, the
transfer byte length related to scsi buffer length (In iscsi for sure 
but I think that for all transports it is the same).

But,
Since you have such a strong objection on this I'm also OK with changing
stuff... We can pass a buffer to scsi_transfer_length (although it has 
no meaning effectively) and we can keep in/out_len local variables and
print them along with total transfer.

Do you want me to send out a patch here (since I have some hardware to 
test it...) or are you still planning to send out something?

Cheers,
Sagi.

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

[Boaz Harrosh]

On 08/13/2014 04:09 PM, Sagi Grimberg wrote:
> On 8/6/2014 4:25 PM, Boaz Harrosh wrote:
> 
> Hey Boaz,
> 
> So in the current flow, I still don't think it is wrong/buggy, the
> transfer byte length related to scsi buffer length 
(In iscsi for sure 
> but I think that for all transports it is the same).
> 
> But,
> Since you have such a strong objection on this I'm also OK with changing
> stuff... We can pass a buffer to scsi_transfer_length (although it has 
> no meaning effectively) and we can keep in/out_len local variables and
> print them along with total transfer.
> 
> Do you want me to send out a patch here (since I have some hardware to 
> test it...) or are you still planning to send out something?
> 

I'll do it. As you said there is no bugs, just ugly code. I will send a
cleanup

Thanks
Boaz

> Cheers,
> 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