[PATCH 0/7] iscsi-target: Bugfixes for t_mem_list -> t_mem_sg[] conversion

[PATCH 0/7] iscsi-target: Bugfixes for t_mem_list -> t_mem_sg[] conversion

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Hi Andy and Christoph,

This series addresses some issues that have appeared during recent testing on
lio-core-2.6.git/master (v3.0.0-rc1) with the iscsi-target patches from
to convert direct usage of struct iscsi_cmd->se_cmd.t_mem_list into struct
iscsi_cmd->t_mem_sg[] using iscsit_alloc_buffs() + iscsit_map_iovec(), and
sendpage() in the outgoing TX path.

Patch #1 involves changes to iscsit_alloc_buffs() required to properly handle
offset_in_page() from kmalloc() with SCF_SCSI_CONTROL_NONSG_IO_CDB payloads
required for initial INQUIRY + REPORT_LUNs, SCAN et al to function..

Patch #2 fixes a payload length padding bug in iscsit_send_data_in() that was
causing problems, and Patch #3 is a bugfux for iscsit_target_tx_thread() to disable
sendpage() with map_sg=0 for non ISTATE_SEND_DATAIN outgoing payload ops that was
causing a seperate OOPs.

Patch #5 addresses the transport_generic_new_cmd() failure in iscsit_handle_scsi_cmd(),
and Patch #6->#7 contain two bugfixes required for I/O payloads to function using
non PAGE_SIZE iSCSI MaxRecvDataSegmentLength with new scatterlist mapping logic in
iscsit_map_iovec() and iscsit_fe_sendpage_sg() respectively.

Finally, Patch #8 addresses a seperate iscsit_do_crypto_hash_sg() SGL pointer walk
bug also introduced with the recent merge.

This series is now passing initial MRDSL walking tests w/ 512 byte increments using
randrw verify fio tests for SCF_SCSI_DATA_SG_IO_CDB and SCF_SCSI_CONTROL_SG_IO_CDB
payloads, but still needs more needs more max_sectors backend testing with new
immediate data WRITEs -> transport_generic_map_mem_to_cmd() usage, and also verified
with MRDSL < 8192 cases with contiguous buffers for SCF_SCSI_CONTROL_NONSG_IO_CDB.

Please have a look and comment.

Thanks,

--nab

Nicholas Bellinger (7):
  iscsi-target: Fix iscsit_alloc_buffs() breakage with offset_in_page
  iscsi-target: Fix padding breakage in iscsit_send_data_in
  iscsi-target: Clear map_sg usage for non ISTATE_SEND_DATAIN ops
  iscsi-target: Handle transport_generic_new_cmd failure
  iscsi-target: Fix iscsit_map_iovec cur_len + data_offset breakage
  iscsi-target: Fix iscsit_fe_sendpage_sg breakage
  iscsi-target: Fix iscsit_do_crypto_hash_sg() bug

 drivers/target/iscsi/iscsi_target.c      |  106 ++++++++++++++++++------------
 drivers/target/iscsi/iscsi_target_util.c |   15 ++--
 2 files changed, 72 insertions(+), 49 deletions(-)

-- 
1.7.2.5

[PATCH 1/7] iscsi-target: Fix iscsit_alloc_buffs() breakage with offset_in_page

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch changes iscsit_alloc_buffs() w/ SCF_SCSI_CONTROL_NONSG_IO_CDB
to take into account the offset_in_page(), and also handles the case
where length and PAGE_SIZE are identical, and require nents to be incremented.

It also moves iscsit_allocate_iovecs() for the SCSI payload case until
after transport_generic_map_mem_to_cmd() has been called because the
iovec allocation depends upon cmd->se_cmd.t_tasks_se_num having been
set by the return of transport_map_sg_to_mem() done in RX thread context.

Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target.c |   78 +++++++++++++++++++++++------------
 1 files changed, 52 insertions(+), 26 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 2503ef3..959f616 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -779,27 +779,17 @@ static int iscsit_allocate_iovecs(struct iscsi_cmd *cmd)
 
 static int iscsit_alloc_buffs(struct iscsi_cmd *cmd)
 {
-	u32 length = cmd->se_cmd.data_length;
 	struct scatterlist *sgl;
+	void *buf, *cur;
+	u32 length = cmd->se_cmd.data_length;
 	int nents = DIV_ROUND_UP(length, PAGE_SIZE);
-	int i;
-	void *buf;
-	void *cur;
-	int ret;
-
-	/* Even no-length cmds need some iovecs, apparently? */
-	ret = iscsit_allocate_iovecs(cmd);
-	if (ret < 0)
-		return ret;
-
+	int i = 0, ret;
+	/*
+	 * If no SCSI payload is present, allocate the default iovecs used for
+	 * iSCSI PDU Header
+	 */
 	if (!length)
-		return 0;
-
-	sgl = kzalloc(sizeof(*sgl) * nents, GFP_KERNEL);
-	if (!sgl)
-		return -ENOMEM;
-	sg_init_table(sgl, nents);
-
+		return iscsit_allocate_iovecs(cmd);
 	/*
 	 * Allocate from slab if nonsg, but sgl should point
 	 * to the malloced mem.
@@ -807,17 +797,38 @@ static int iscsit_alloc_buffs(struct iscsi_cmd *cmd)
 	 * Alloc pages if sg.
 	 */
 	if (cmd->se_cmd.se_cmd_flags & SCF_SCSI_CONTROL_NONSG_IO_CDB) {
+		int pg_off = 0, buf_size;
 
 		buf = kmalloc(length, GFP_KERNEL);
 		if (!buf)
 			return -ENOMEM;
+		/*
+		 * Allocate extra SGL for offset_in_page exceeding DIV_ROUND_UP
+		 */
+		pg_off = offset_in_page(buf);
+		if ((pg_off + length) > (PAGE_SIZE * nents))
+			nents++;
+
+		sgl = kzalloc(sizeof(*sgl) * nents, GFP_KERNEL);
+		if (!sgl)
+			return -ENOMEM;
+		sg_init_table(sgl, nents);
 
 		cur = buf;
 		while (length) {
-			int buf_size = min_t(int, length, PAGE_SIZE);
-
+			if (pg_off != 0) {
+				buf_size = min_t(int, PAGE_SIZE - pg_off, length);
+				pg_off = 0;
+			} else
+				buf_size = min_t(int, length, PAGE_SIZE);
+					
 			sg_set_buf(&sgl[i], cur, buf_size);
 
+			if (sgl[i].length < 0) {
+				printk("sg_set_buf: page: %p, len: %d, offset: %d\n",
+					sg_page(&sgl[i]), sgl[i].length, sgl[i].offset);
+				BUG();
+			}
 			length -= buf_size;
 			cur += buf_size;
 			i++;
@@ -826,7 +837,12 @@ static int iscsit_alloc_buffs(struct iscsi_cmd *cmd)
 		cmd->t_mem = buf;
 
 	} else {
-		i = 0;
+		sgl = kzalloc(sizeof(*sgl) * nents, GFP_KERNEL);
+		if (!sgl)
+			return -ENOMEM;
+
+		sg_init_table(sgl, nents);
+
 		while (length) {
 			int buf_size = min_t(int, length, PAGE_SIZE);
 			struct page *page;
@@ -849,6 +865,13 @@ static int iscsit_alloc_buffs(struct iscsi_cmd *cmd)
 
 	/* make se_mem list from the memory */
 	transport_generic_map_mem_to_cmd(&cmd->se_cmd, sgl, nents, NULL, 0);
+	/*
+	 * Allocate iovecs for SCSI payload after transport_generic_map_mem_to_cmd
+	 * so that cmd->se_cmd.t_tasks_se_num has been set.
+	 */
+        ret = iscsit_allocate_iovecs(cmd);
+        if (ret < 0)
+		goto page_alloc_failed;
 
 	return 0;
 
@@ -1081,12 +1104,15 @@ attach_cmd:
 	 * Active/NonOptimized primary access state..
 	 */
 	core_alua_check_nonop_delay(SE_CMD(cmd));
-
+	/*
+	 * Allocate and setup SGL used with transport_generic_map_mem_to_cmd().
+	 * also call iscsit_allocate_iovecs()
+	 */
 	ret = iscsit_alloc_buffs(cmd);
-	if (ret < 0) {
-		return ret;
-	}
-
+	if (ret < 0)
+		return iscsit_add_reject_from_cmd(
+				ISCSI_REASON_BOOKMARK_NO_RESOURCES,
+				1, 1, buf, cmd);
 	/*
 	 * Check the CmdSN against ExpCmdSN/MaxCmdSN here if
 	 * the Immediate Bit is not set, and no Immediate
-- 
1.7.2.5

Re: [PATCH 1/7] iscsi-target: Fix iscsit_alloc_buffs() breakage with offset_in_page

[Christoph Hellwig]

On Fri, Jun 03, 2011 at 06:18:12PM -0700, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch changes iscsit_alloc_buffs() w/ SCF_SCSI_CONTROL_NONSG_IO_CDB
> to take into account the offset_in_page(), and also handles the case
> where length and PAGE_SIZE are identical, and require nents to be incremented.
> 
> It also moves iscsit_allocate_iovecs() for the SCSI payload case until
> after transport_generic_map_mem_to_cmd() has been called because the
> iovec allocation depends upon cmd->se_cmd.t_tasks_se_num having been
> set by the return of transport_map_sg_to_mem() done in RX thread context.

I'd rather just remove the SCF_SCSI_CONTROL_NONSG_IO_CDB side of the
code path, and always allocate one page per S/G list item.  That simplifies
the code, and fixes the issues with non-aligned kmallocs.

Re: [PATCH 1/7] iscsi-target: Fix iscsit_alloc_buffs() breakage with offset_in_page

[Andy Grover]

On 06/04/2011 07:21 AM, Christoph Hellwig wrote:
> On Fri, Jun 03, 2011 at 06:18:12PM -0700, Nicholas A. Bellinger wrote:
>> From: Nicholas Bellinger <nab@linux-iscsi.org>
>>
>> This patch changes iscsit_alloc_buffs() w/ SCF_SCSI_CONTROL_NONSG_IO_CDB
>> to take into account the offset_in_page(), and also handles the case
>> where length and PAGE_SIZE are identical, and require nents to be incremented.
>>
>> It also moves iscsit_allocate_iovecs() for the SCSI payload case until
>> after transport_generic_map_mem_to_cmd() has been called because the
>> iovec allocation depends upon cmd->se_cmd.t_tasks_se_num having been
>> set by the return of transport_map_sg_to_mem() done in RX thread context.
> 
> I'd rather just remove the SCF_SCSI_CONTROL_NONSG_IO_CDB side of the
> code path, and always allocate one page per S/G list item.  That simplifies
> the code, and fixes the issues with non-aligned kmallocs.

I thought about doing that but didn't quite have the guts.

It *would* simplify things a lot, though. Offsets of offsets are no fun.
+1 from me. Express everything in pages and scatterlists.

-- Andy

[PATCH 2/7] iscsi-target: Fix padding breakage in iscsit_send_data_in

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds the missing cmd->padding assignment in iscsit_send_data_in()
that is required for proper iscsit_fe_sendpage_sg() operation.

Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 959f616..5a14318 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -2508,7 +2508,6 @@ static int iscsit_send_data_in(
 	struct iscsi_datain_req *dr;
 	struct iscsi_data_rsp *hdr;
 	struct kvec *iov;
-	u32 padding;
 
 	memset(&datain, 0, sizeof(struct iscsi_datain));
 	dr = iscsit_get_datain_values(cmd, &datain);
@@ -2612,8 +2611,8 @@ static int iscsit_send_data_in(
 	iov_count += iov_ret;
 	tx_size += datain.length;
 
-	padding = ((-datain.length) & 3);
-	if (padding) {
+	cmd->padding = ((-datain.length) & 3);
+	if (cmd->padding) {
 		iov[iov_count].iov_base		= cmd->pad_bytes;
 		iov[iov_count++].iov_len	= cmd->padding;
 		tx_size += cmd->padding;
-- 
1.7.2.5

[PATCH 3/7] iscsi-target: Clear map_sg usage for non ISTATE_SEND_DATAIN ops

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch fixes a bug in iscsi_target_tx_thread() where 'map_sg' was not
being cleared after ISTATE_SEND_DATAIN and causing iscsit_fe_sendpage_sg()
to be called for non ISTATE_SEND_DATAIN + non ISCSI_OP_SCSI_CMD payloads.

Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 5a14318..8dedfbf 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -3743,6 +3743,7 @@ check_rsp_state:
 					goto transport_err;
 				}
 			}
+			map_sg = 0;
 			iscsit_unmap_iovec(cmd);
 
 			spin_lock_bh(&cmd->istate_lock);
-- 
1.7.2.5

[PATCH 4/7] iscsi-target: Handle transport_generic_new_cmd failure

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch updates iscsit_handle_scsi_cmd() to handle transport_generic_new_cmd()
failures using iscsit_add_reject_from_cmd(), and updates the comment.

Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 8dedfbf..6ebc247 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1162,13 +1162,11 @@ attach_cmd:
 		goto after_immediate_data;
 	}
 	/*
-	 * Call into transport_generic_handle_cdb() that now translates
-	 * into a direct transport_generic_new_cmd() call with a NULL
-	 * se_cmd->se_tfo->new_cmd_map() pointer.
+	 * Call directly into transport_generic_new_cmd() to perform
+	 * the backend memory allocation.
 	 */
-	transport_generic_new_cmd(SE_CMD(cmd));
-
-	if (SE_CMD(cmd)->se_cmd_flags & SCF_SE_CMD_FAILED) {
+	ret = transport_generic_new_cmd(&cmd->se_cmd);
+	if ((ret < 0) || (SE_CMD(cmd)->se_cmd_flags & SCF_SE_CMD_FAILED)) {
 		immed_ret = IMMEDIATE_DATA_NORMAL_OPERATION;
 		dump_immediate_data = 1;
 		goto after_immediate_data;
-- 
1.7.2.5

Re: [PATCH 4/7] iscsi-target: Handle transport_generic_new_cmd failure

[Christoph Hellwig]

On Fri, Jun 03, 2011 at 06:18:15PM -0700, Nicholas A. Bellinger wrote:
> +	ret = transport_generic_new_cmd(&cmd->se_cmd);
> +	if ((ret < 0) || (SE_CMD(cmd)->se_cmd_flags & SCF_SE_CMD_FAILED)) {

No need for the inner braces.  Also SCF_SE_CMD_FAILED won't ever be
set if you call transport_generic_new_cmd, so no need to check it.

[PATCH 5/7] iscsi-target: Fix iscsit_map_iovec cur_len + data_offset breakage

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch fixes two length + offset releated bugs in the new iscsit_map_iovec().
The first is where cur_len was being calculated incorrectly for the case type
SCF_SCSI_CONTROL_NONSG_IO_CDB with an sg->offset non zero offset.

The second adds the missing pg_off to kmap() when setting iov_base
from sg_page() with the current scatterlist pointer.

It also converts iscsit_map_iovec() to use sg_next() instead of
incremented the scatterlist pointer following lib/scatterlist.h

Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 6ebc247..d84d52b 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -696,7 +696,7 @@ static int iscsit_map_iovec(
 	u32 data_offset,
 	u32 data_length)
 {
-	u32 i;
+	u32 i = 0;
 	struct scatterlist *sg;
 	unsigned int page_off;
 
@@ -705,20 +705,20 @@ static int iscsit_map_iovec(
 	 * At this point, we also know each contains a page.
 	 */
 	sg = &cmd->t_mem_sg[data_offset / PAGE_SIZE];
-	page_off = (data_offset % PAGE_SIZE) + sg->offset;
+	page_off = (data_offset % PAGE_SIZE);
 
 	cmd->first_data_sg = sg;
 	cmd->first_data_sg_off = page_off;
 
-	i = 0;
 	while (data_length) {
-		u32 cur_len = min_t(u32, data_length, (sg[i].length - page_off));
+		u32 cur_len = min_t(u32, data_length, sg->length - page_off);
 
-		iov[i].iov_base = kmap(sg_page(&sg[i]));
+		iov[i].iov_base = kmap(sg_page(sg)) + sg->offset + page_off;
 		iov[i].iov_len = cur_len;
 
 		data_length -= cur_len;
 		page_off = 0;
+		sg = sg_next(sg);
 		i++;
 	}
 
-- 
1.7.2.5

[PATCH 6/7] iscsi-target: Fix iscsit_fe_sendpage_sg breakage

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

The patch fixes iscsit_fe_sendpage_sg() to follow iscsit_map_iovec() usage of
cmd->first_data_sg_off for the iSCSI data sequence offset into the first page,
along with individual sg->offset set for SCF_SCSI_CONTROL_NONSG_IO_CDB in
iscsit_alloc_buffs().

It also fixes the padding and DataDigest iovec offsets to use the updated
cmd->iov_data_count in iscsit_allocate_iovecs().

Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target_util.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index c6219e5..7eeddd2 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -1277,12 +1277,11 @@ int iscsit_fe_sendpage_sg(
 	struct iscsi_cmd *cmd,
 	struct iscsi_conn *conn)
 {
-	int tx_sent;
-	u32 tx_hdr_size;
-	u32 data_len;
-	struct kvec iov;
 	struct scatterlist *sg = cmd->first_data_sg;
+	struct kvec iov;
+	u32 tx_hdr_size, data_len;
 	u32 offset = cmd->first_data_sg_off;
+	int tx_sent;
 
 send_hdr:
 	tx_hdr_size = ISCSI_HDR_LEN;
@@ -1313,7 +1312,7 @@ send_hdr:
 		u32 sub_len = min_t(u32, data_len, space);
 send_pg:
 		tx_sent = conn->sock->ops->sendpage(conn->sock,
-						    sg_page(sg), offset, sub_len, 0);
+					sg_page(sg), sg->offset + offset, sub_len, 0);
 		if (tx_sent != sub_len) {
 			if (tx_sent == -EAGAIN) {
 				printk(KERN_ERR "tcp_sendpage() returned"
@@ -1328,13 +1327,13 @@ send_pg:
 
 		data_len -= sub_len;
 		offset = 0;
-		sg++;
+		sg = sg_next(sg);
 	}
 
 send_padding:
 	if (cmd->padding) {
 		struct kvec *iov_p =
-			&cmd->iov_data[cmd->iov_data_count-2];
+			&cmd->iov_data[cmd->iov_data_count-1];
 
 		tx_sent = tx_data(conn, iov_p, 1, cmd->padding);
 		if (cmd->padding != tx_sent) {
@@ -1349,7 +1348,7 @@ send_padding:
 send_datacrc:
 	if (conn->conn_ops->DataDigest) {
 		struct kvec *iov_d =
-			&cmd->iov_data[cmd->iov_data_count-1];
+			&cmd->iov_data[cmd->iov_data_count];
 
 		tx_sent = tx_data(conn, iov_d, 1, ISCSI_CRC_LEN);
 		if (ISCSI_CRC_LEN != tx_sent) {
-- 
1.7.2.5

[PATCH 7/7] iscsi-target: Fix iscsit_do_crypto_hash_sg() bug

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch fixes breakage in iscsit_do_crypto_hash_sg() where
crypto_hash_update() was not referencing the current sg[] offset 'i'.

Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index d84d52b..fdeea0f 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1243,7 +1243,7 @@ static u32 iscsit_do_crypto_hash_sg(
 	while (data_length) {
 		u32 cur_len = min_t(u32, data_length, (sg[i].length - page_off));
 
-		crypto_hash_update(hash, sg, cur_len);
+		crypto_hash_update(hash, &sg[i], cur_len);
 
 		data_length -= cur_len;
 		page_off = 0;
-- 
1.7.2.5