[PATCH 2/3] tcm/pscsi: Add proper BIDI-COMMAND passthrough to SCSI MidLayer

[PATCH 2/3] tcm/pscsi: Add proper BIDI-COMMAND passthrough to SCSI MidLayer

[Nicholas A. Bellinger]

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

This patch updates the TCM/pSCSI plugin to support BIDI-COMMAND passthrough to the
Linux/SCSI MidLayer using struct se_task->task_sg_bidi[] from commit 2f2e67d85860,
and the setup of the extra struct request for the BIDI SCSI READ payload.

This patchturns the original pscsi_map_task_SG() into a wrapper call for the
new __pscsi_map_task_SG() which now accepts struct scatterlist *task_sg and u32 task_sg_num
for struct se_task -> struct bio allocation, map and blk_make_request() calls.

It also updates pscsi_blk_init_request() which now gets called twice for BIDI-COMMAND
case.  This includes the pscsi_blk_init_request() change to perform the assignment of
'req->cmd = &pt->pscsi_cdb[0]' and drop the original CDB memcpy() of pt->pscsi_cdb[]
into struct request->cmd[] -> struct request->__cmd[MAX_BLK_CDB].

Tested with scsi_debug w/ XDWRITE_READ32 and TCM_Loop w/ TCM/pSCSI backstores
using 'sgv4_xdwriteread -e'.

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

diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index beca807..60f825d 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -727,29 +727,37 @@ static void *pscsi_allocate_request(
 
 static inline void pscsi_blk_init_request(
 	struct se_task *task,
-	struct pscsi_plugin_task *pt)
+	struct pscsi_plugin_task *pt,
+	struct request *req,
+	int bidi_read)
 {
 	/*
 	 * Defined as "scsi command" in include/linux/blkdev.h.
 	 */
-	pt->pscsi_req->cmd_type = REQ_TYPE_BLOCK_PC;
+	req->cmd_type = REQ_TYPE_BLOCK_PC;
+	/*
+	 * For the extra BIDI-COMMAND READ struct request we do not
+	 * need to setup the remaining structure members
+	 */
+	if (bidi_read)
+		return;
 	/*
 	 * Setup the done function pointer for struct request,
 	 * also set the end_io_data pointer.to struct se_task.
 	 */
-	pt->pscsi_req->end_io = pscsi_req_done;
-	pt->pscsi_req->end_io_data = (void *)task;
+	req->end_io = pscsi_req_done;
+	req->end_io_data = (void *)task;
 	/*
 	 * Load the referenced struct se_task's SCSI CDB into
 	 * include/linux/blkdev.h:struct request->cmd
 	 */
-	pt->pscsi_req->cmd_len = scsi_command_size(pt->pscsi_cdb);
-	memcpy(pt->pscsi_req->cmd, pt->pscsi_cdb, pt->pscsi_req->cmd_len);
+	req->cmd_len = scsi_command_size(pt->pscsi_cdb);
+	req->cmd = &pt->pscsi_cdb[0];
 	/*
 	 * Setup pointer for outgoing sense data.
 	 */
-	pt->pscsi_req->sense = (void *)&pt->pscsi_sense[0];
-	pt->pscsi_req->sense_len = 0;
+	req->sense = (void *)&pt->pscsi_sense[0];
+	req->sense_len = 0;
 }
 
 /*
@@ -771,7 +779,7 @@ static int pscsi_blk_get_request(struct se_task *task)
 	 * Setup the newly allocated struct request for REQ_TYPE_BLOCK_PC,
 	 * and setup rq callback, CDB and sense.
 	 */
-	pscsi_blk_init_request(task, pt);
+	pscsi_blk_init_request(task, pt, pt->pscsi_req, 0);
 	return 0;
 }
 
@@ -1051,11 +1059,11 @@ static inline struct bio *pscsi_get_bio(struct pscsi_dev_virt *pdv, int sg_num)
 #define DEBUG_PSCSI(x...)
 #endif
 
-/*      pscsi_map_task_SG():
- *
- *
- */
-static int pscsi_map_task_SG(struct se_task *task)
+static int __pscsi_map_task_SG(
+	struct se_task *task,
+	struct scatterlist *task_sg,
+	u32 task_sg_num,
+	int bidi_read)
 {
 	struct pscsi_plugin_task *pt = (struct pscsi_plugin_task *) task->transport_req;
 	struct pscsi_dev_virt *pdv = (struct pscsi_dev_virt *) task->se_dev->dev_ptr;
@@ -1063,7 +1071,7 @@ static int pscsi_map_task_SG(struct se_task *task)
 	struct page *page;
 	struct scatterlist *sg;
 	u32 data_len = task->task_size, i, len, bytes, off;
-	int nr_pages = (task->task_size + task->task_sg[0].offset +
+	int nr_pages = (task->task_size + task_sg[0].offset +
 			PAGE_SIZE - 1) >> PAGE_SHIFT;
 	int nr_vecs = 0, ret = 0;
 	int rw = (TASK_CMD(task)->data_direction == DMA_TO_DEVICE);
@@ -1083,7 +1091,7 @@ static int pscsi_map_task_SG(struct se_task *task)
 	 */
 	DEBUG_PSCSI("PSCSI: nr_pages: %d\n", nr_pages);
 
-	for_each_sg(task->task_sg, sg, task->task_sg_num, i) {
+	for_each_sg(task_sg, sg, task_sg_num, i) {
 		page = sg_page(sg);
 		off = sg->offset;
 		len = sg->length;
@@ -1155,20 +1163,44 @@ static int pscsi_map_task_SG(struct se_task *task)
 		}
 	}
 	/*
-	 * Starting with v2.6.31, call blk_make_request() passing in *hbio to
-	 * allocate the pSCSI task a struct request.
+	 * Setup the primary pt->pscsi_req used for non BIDI and BIDI-COMMAND
+	 * primary SCSI WRITE poayload mapped for struct se_task->task_sg[]
 	 */
-	pt->pscsi_req = blk_make_request(pdv->pdv_sd->request_queue,
-				hbio, GFP_KERNEL);
-	if (!(pt->pscsi_req)) {
-		printk(KERN_ERR "pSCSI: blk_make_request() failed\n");
-		goto fail;
+	if (!(bidi_read)) {
+		/*
+		 * Starting with v2.6.31, call blk_make_request() passing in *hbio to
+		 * allocate the pSCSI task a struct request.
+		 */
+		pt->pscsi_req = blk_make_request(pdv->pdv_sd->request_queue,
+					hbio, GFP_KERNEL);
+		if (!(pt->pscsi_req)) {
+			printk(KERN_ERR "pSCSI: blk_make_request() failed\n");
+			goto fail;
+		}
+		/*
+		 * Setup the newly allocated struct request for REQ_TYPE_BLOCK_PC,
+		 * and setup rq callback, CDB and sense.
+		 */
+		pscsi_blk_init_request(task, pt, pt->pscsi_req, 0);
+
+		return task->task_sg_num;
 	}
 	/*
-	 * Setup the newly allocated struct request for REQ_TYPE_BLOCK_PC,
-	 * and setup rq callback, CDB and sense.
+	 * Setup the secondary pt->pscsi_req_bidi used for the extra BIDI-COMMAND
+	 * SCSI READ paylaod mapped for struct se_task->task_sg_bidi[]
 	 */
-	pscsi_blk_init_request(task, pt);
+	pt->pscsi_req_bidi = blk_make_request(pdv->pdv_sd->request_queue,
+					hbio, GFP_KERNEL);
+	if (!(pt->pscsi_req_bidi)) {
+		printk(KERN_ERR "pSCSI: blk_make_request() failed for BIDI\n");
+		goto fail;
+	}
+	pscsi_blk_init_request(task, pt, pt->pscsi_req_bidi, 1);
+	/*
+	 * Setup the magic BIDI-COMMAND ->next_req pointer to the original
+	 * pt->pscsi_req.
+	 */	
+	pt->pscsi_req->next_rq = pt->pscsi_req_bidi;
 
 	return task->task_sg_num;
 fail:
@@ -1181,6 +1213,27 @@ fail:
 	return ret;
 }
 
+static int pscsi_map_task_SG(struct se_task *task)
+{
+	int ret;
+	/*
+	 * Setup the main struct request for the task->task_sg[] payload
+	 */
+
+	ret = __pscsi_map_task_SG(task, task->task_sg, task->task_sg_num, 0);
+	if (ret < 0)
+		return ret;
+
+	if (!(task->task_sg_bidi))
+		return ret;
+	/*
+	 * If present, set up the extra BIDI-COMMAND SCSI READ
+	 * struct request and payload.
+	 */
+	return __pscsi_map_task_SG(task, task->task_sg_bidi,
+				task->task_sg_num, 1);
+}
+
 /*	pscsi_map_task_non_SG():
  *
  *
@@ -1438,6 +1491,12 @@ static void pscsi_req_done(struct request *req, int uptodate)
 	pt->pscsi_resid = req->resid_len;
 
 	pscsi_process_SAM_status(task, pt);
+
+	if (req->next_rq != NULL) {
+		__blk_put_request(req->q, req->next_rq);
+		pt->pscsi_req_bidi = NULL;
+	}
+
 	__blk_put_request(req->q, req);
 	pt->pscsi_req = NULL;
 }
diff --git a/drivers/target/target_core_pscsi.h b/drivers/target/target_core_pscsi.h
index 086e6f1..d3c8972 100644
--- a/drivers/target/target_core_pscsi.h
+++ b/drivers/target/target_core_pscsi.h
@@ -34,6 +34,7 @@ struct pscsi_plugin_task {
 	int	pscsi_result;
 	u32	pscsi_resid;
 	struct request *pscsi_req;
+	struct request *pscsi_req_bidi;
 } ____cacheline_aligned;
 
 #define PDF_HAS_CHANNEL_ID	0x01
-- 
1.5.6.5

Re: [PATCH 2/3] tcm/pscsi: Add proper BIDI-COMMAND passthrough to SCSI MidLayer

[Boaz Harrosh]

On 09/22/2010 08:08 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch updates the TCM/pSCSI plugin to support BIDI-COMMAND passthrough to the
> Linux/SCSI MidLayer using struct se_task->task_sg_bidi[] from commit 2f2e67d85860,
> and the setup of the extra struct request for the BIDI SCSI READ payload.
> 
> This patchturns the original pscsi_map_task_SG() into a wrapper call for the
> new __pscsi_map_task_SG() which now accepts struct scatterlist *task_sg and u32 task_sg_num
> for struct se_task -> struct bio allocation, map and blk_make_request() calls.
> 
> It also updates pscsi_blk_init_request() which now gets called twice for BIDI-COMMAND
> case.  This includes the pscsi_blk_init_request() change to perform the assignment of
> 'req->cmd = &pt->pscsi_cdb[0]' and drop the original CDB memcpy() of pt->pscsi_cdb[]
> into struct request->cmd[] -> struct request->__cmd[MAX_BLK_CDB].
> 
> Tested with scsi_debug w/ XDWRITE_READ32 and TCM_Loop w/ TCM/pSCSI backstores
> using 'sgv4_xdwriteread -e'.
> 
> Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> ---
>  drivers/target/target_core_pscsi.c |  111 +++++++++++++++++++++++++++--------
>  drivers/target/target_core_pscsi.h |    1 +
>  2 files changed, 86 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
> index beca807..60f825d 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -727,29 +727,37 @@ static void *pscsi_allocate_request(
>  
>  static inline void pscsi_blk_init_request(
>  	struct se_task *task,
> -	struct pscsi_plugin_task *pt)
> +	struct pscsi_plugin_task *pt,
> +	struct request *req,
> +	int bidi_read)
>  {
>  	/*
>  	 * Defined as "scsi command" in include/linux/blkdev.h.
>  	 */
> -	pt->pscsi_req->cmd_type = REQ_TYPE_BLOCK_PC;
> +	req->cmd_type = REQ_TYPE_BLOCK_PC;
> +	/*
> +	 * For the extra BIDI-COMMAND READ struct request we do not
> +	 * need to setup the remaining structure members
> +	 */
> +	if (bidi_read)
> +		return;

I think that if you embed this pscsi_blk_init_request() inside it's
caller it would be more clear.

But if the caller becomes too big, and this is a logical separation
then do the bidi_req->cmd_type = REQ_TYPE_BLOCK_PC in the caller
and call this one only for the main request, as before.

(BTW: Bidi does not even need cmd_type = REQ_TYPE_BLOCK_PC but
 just to make sure it's safer to do it.)

>  	/*
>  	 * Setup the done function pointer for struct request,
>  	 * also set the end_io_data pointer.to struct se_task.
>  	 */
> -	pt->pscsi_req->end_io = pscsi_req_done;
> -	pt->pscsi_req->end_io_data = (void *)task;
> +	req->end_io = pscsi_req_done;
> +	req->end_io_data = (void *)task;
>  	/*
>  	 * Load the referenced struct se_task's SCSI CDB into
>  	 * include/linux/blkdev.h:struct request->cmd
>  	 */
> -	pt->pscsi_req->cmd_len = scsi_command_size(pt->pscsi_cdb);
> -	memcpy(pt->pscsi_req->cmd, pt->pscsi_cdb, pt->pscsi_req->cmd_len);
> +	req->cmd_len = scsi_command_size(pt->pscsi_cdb);
> +	req->cmd = &pt->pscsi_cdb[0];
>  	/*
>  	 * Setup pointer for outgoing sense data.
>  	 */
> -	pt->pscsi_req->sense = (void *)&pt->pscsi_sense[0];
> -	pt->pscsi_req->sense_len = 0;
> +	req->sense = (void *)&pt->pscsi_sense[0];
> +	req->sense_len = 0;
>  }
>  
>  /*
> @@ -771,7 +779,7 @@ static int pscsi_blk_get_request(struct se_task *task)
>  	 * Setup the newly allocated struct request for REQ_TYPE_BLOCK_PC,
>  	 * and setup rq callback, CDB and sense.
>  	 */
> -	pscsi_blk_init_request(task, pt);
> +	pscsi_blk_init_request(task, pt, pt->pscsi_req, 0);
>  	return 0;
>  }
>  
> @@ -1051,11 +1059,11 @@ static inline struct bio *pscsi_get_bio(struct pscsi_dev_virt *pdv, int sg_num)
>  #define DEBUG_PSCSI(x...)
>  #endif
>  
> -/*      pscsi_map_task_SG():
> - *
> - *
> - */
> -static int pscsi_map_task_SG(struct se_task *task)
> +static int __pscsi_map_task_SG(
> +	struct se_task *task,
> +	struct scatterlist *task_sg,
> +	u32 task_sg_num,
> +	int bidi_read)
>  {
>  	struct pscsi_plugin_task *pt = (struct pscsi_plugin_task *) task->transport_req;
>  	struct pscsi_dev_virt *pdv = (struct pscsi_dev_virt *) task->se_dev->dev_ptr;
> @@ -1063,7 +1071,7 @@ static int pscsi_map_task_SG(struct se_task *task)
>  	struct page *page;
>  	struct scatterlist *sg;
>  	u32 data_len = task->task_size, i, len, bytes, off;
> -	int nr_pages = (task->task_size + task->task_sg[0].offset +
> +	int nr_pages = (task->task_size + task_sg[0].offset +
>  			PAGE_SIZE - 1) >> PAGE_SHIFT;

OK, I'm begining to suspect task->task_size is the size of what?
See below (below)

>  	int nr_vecs = 0, ret = 0;
>  	int rw = (TASK_CMD(task)->data_direction == DMA_TO_DEVICE);
> @@ -1083,7 +1091,7 @@ static int pscsi_map_task_SG(struct se_task *task)
>  	 */
>  	DEBUG_PSCSI("PSCSI: nr_pages: %d\n", nr_pages);
>  
> -	for_each_sg(task->task_sg, sg, task->task_sg_num, i) {
> +	for_each_sg(task_sg, sg, task_sg_num, i) {
>  		page = sg_page(sg);
>  		off = sg->offset;
>  		len = sg->length;

OK this code goes off and I don't see the rest of it. But from below with the
local use of hbio I'm assuming you are building the hbio here right?

So I had a wild thought! instead of having a page_list ==> sg_list ==> bio
could you keep bios at the task level? that is instead of sg_list.

So you do page_list ==> bio directly. Then in some places where you later
need an actual sg_list for DMA. The block layer supplies an easy bio ==> sg_list
conversion.

(Just a wild idea)

> @@ -1155,20 +1163,44 @@ static int pscsi_map_task_SG(struct se_task *task)
>  		}
>  	}
>  	/*
> -	 * Starting with v2.6.31, call blk_make_request() passing in *hbio to
> -	 * allocate the pSCSI task a struct request.
> +	 * Setup the primary pt->pscsi_req used for non BIDI and BIDI-COMMAND
> +	 * primary SCSI WRITE poayload mapped for struct se_task->task_sg[]
>  	 */
> -	pt->pscsi_req = blk_make_request(pdv->pdv_sd->request_queue,
> -				hbio, GFP_KERNEL);
> -	if (!(pt->pscsi_req)) {
> -		printk(KERN_ERR "pSCSI: blk_make_request() failed\n");
> -		goto fail;
> +	if (!(bidi_read)) {
> +		/*
> +		 * Starting with v2.6.31, call blk_make_request() passing in *hbio to
> +		 * allocate the pSCSI task a struct request.
> +		 */
> +		pt->pscsi_req = blk_make_request(pdv->pdv_sd->request_queue,
> +					hbio, GFP_KERNEL);
> +		if (!(pt->pscsi_req)) {
> +			printk(KERN_ERR "pSCSI: blk_make_request() failed\n");
> +			goto fail;
> +		}
> +		/*
> +		 * Setup the newly allocated struct request for REQ_TYPE_BLOCK_PC,
> +		 * and setup rq callback, CDB and sense.
> +		 */
> +		pscsi_blk_init_request(task, pt, pt->pscsi_req, 0);
> +
> +		return task->task_sg_num;
>  	}
>  	/*
> -	 * Setup the newly allocated struct request for REQ_TYPE_BLOCK_PC,
> -	 * and setup rq callback, CDB and sense.
> +	 * Setup the secondary pt->pscsi_req_bidi used for the extra BIDI-COMMAND
> +	 * SCSI READ paylaod mapped for struct se_task->task_sg_bidi[]
>  	 */
> -	pscsi_blk_init_request(task, pt);
> +	pt->pscsi_req_bidi = blk_make_request(pdv->pdv_sd->request_queue,
> +					hbio, GFP_KERNEL);
> +	if (!(pt->pscsi_req_bidi)) {
> +		printk(KERN_ERR "pSCSI: blk_make_request() failed for BIDI\n");
> +		goto fail;
> +	}
> +	pscsi_blk_init_request(task, pt, pt->pscsi_req_bidi, 1);
> +	/*
> +	 * Setup the magic BIDI-COMMAND ->next_req pointer to the original
> +	 * pt->pscsi_req.
> +	 */	
> +	pt->pscsi_req->next_rq = pt->pscsi_req_bidi;
>  
>  	return task->task_sg_num;

OK Now I'm sure!
You have completely missed the fact that bidi entails two sg_list(s)
two sg_num(s) and two io_byte_count(s).

The use of sg_table will clear that confusion a bit, though I wanted it
to carry an  io_byte_count as well, but never came to do that.

>  fail:
> @@ -1181,6 +1213,27 @@ fail:
>  	return ret;
>  }
>  
> +static int pscsi_map_task_SG(struct se_task *task)
> +{
> +	int ret;
> +	/*
> +	 * Setup the main struct request for the task->task_sg[] payload
> +	 */
> +
> +	ret = __pscsi_map_task_SG(task, task->task_sg, task->task_sg_num, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!(task->task_sg_bidi))
> +		return ret;
> +	/*
> +	 * If present, set up the extra BIDI-COMMAND SCSI READ
> +	 * struct request and payload.
> +	 */
> +	return __pscsi_map_task_SG(task, task->task_sg_bidi,
> +				task->task_sg_num, 1);

What? the task->task_sg_bidi as the same sg_num as the above
task->task_sg. (See you should use sg_table to remove these
bugs.

(I wrote this comment before I wrote the above one ;-))

> +}
> +
>  /*	pscsi_map_task_non_SG():
>   *
>   *
> @@ -1438,6 +1491,12 @@ static void pscsi_req_done(struct request *req, int uptodate)
>  	pt->pscsi_resid = req->resid_len;
>  
>  	pscsi_process_SAM_status(task, pt);
> +
> +	if (req->next_rq != NULL) {
> +		__blk_put_request(req->q, req->next_rq);

		req->next_rq = NULL;

> +		pt->pscsi_req_bidi = NULL;
> +	}
> +
>  	__blk_put_request(req->q, req);
>  	pt->pscsi_req = NULL;
>  }
> diff --git a/drivers/target/target_core_pscsi.h b/drivers/target/target_core_pscsi.h
> index 086e6f1..d3c8972 100644
> --- a/drivers/target/target_core_pscsi.h
> +++ b/drivers/target/target_core_pscsi.h
> @@ -34,6 +34,7 @@ struct pscsi_plugin_task {
>  	int	pscsi_result;
>  	u32	pscsi_resid;
>  	struct request *pscsi_req;
> +	struct request *pscsi_req_bidi;

You don't really need to, you know. You have it right here at
pscsi_req->next_rq. But it's your call

>  } ____cacheline_aligned;
>  
>  #define PDF_HAS_CHANNEL_ID	0x01

Cheers (life is hard)
Boaz

Re: [PATCH 2/3] tcm/pscsi: Add proper BIDI-COMMAND passthrough to SCSI MidLayer

[Boaz Harrosh]

On 09/22/2010 01:13 PM, Boaz Harrosh wrote:
> On 09/22/2010 08:08 AM, Nicholas A. Bellinger wrote:

<snip>

>>  	return task->task_sg_num;
> 
> OK Now I'm sure!
> You have completely missed the fact that bidi entails two sg_list(s)
> two sg_num(s) and two io_byte_count(s).
> 
> The use of sg_table will clear that confusion a bit, though I wanted it
> to carry an  io_byte_count as well, but never came to do that.
> 

OK actually you should use scsi_data_buffer better then sg_table. Because
it also has the length. And it even has a resid. Because with bidi-commands
there are two residual counters reported in command-response. You can see
libiscsi for how it handles the bidi residual counters.

Boaz

Re: [PATCH 2/3] tcm/pscsi: Add proper BIDI-COMMAND passthrough to SCSI MidLayer

[Nicholas A. Bellinger]

On Wed, 2010-09-22 at 13:13 +0200, Boaz Harrosh wrote:
> On 09/22/2010 08:08 AM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > This patch updates the TCM/pSCSI plugin to support BIDI-COMMAND passthrough to the
> > Linux/SCSI MidLayer using struct se_task->task_sg_bidi[] from commit 2f2e67d85860,
> > and the setup of the extra struct request for the BIDI SCSI READ payload.
> > 
> > This patchturns the original pscsi_map_task_SG() into a wrapper call for the
> > new __pscsi_map_task_SG() which now accepts struct scatterlist *task_sg and u32 task_sg_num
> > for struct se_task -> struct bio allocation, map and blk_make_request() calls.
> > 
> > It also updates pscsi_blk_init_request() which now gets called twice for BIDI-COMMAND
> > case.  This includes the pscsi_blk_init_request() change to perform the assignment of
> > 'req->cmd = &pt->pscsi_cdb[0]' and drop the original CDB memcpy() of pt->pscsi_cdb[]
> > into struct request->cmd[] -> struct request->__cmd[MAX_BLK_CDB].
> > 
> > Tested with scsi_debug w/ XDWRITE_READ32 and TCM_Loop w/ TCM/pSCSI backstores
> > using 'sgv4_xdwriteread -e'.
> > 
> > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> > ---
> >  drivers/target/target_core_pscsi.c |  111 +++++++++++++++++++++++++++--------
> >  drivers/target/target_core_pscsi.h |    1 +
> >  2 files changed, 86 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
> > index beca807..60f825d 100644
> > --- a/drivers/target/target_core_pscsi.c
> > +++ b/drivers/target/target_core_pscsi.c
> > @@ -727,29 +727,37 @@ static void *pscsi_allocate_request(
> >  
> >  static inline void pscsi_blk_init_request(
> >  	struct se_task *task,
> > -	struct pscsi_plugin_task *pt)
> > +	struct pscsi_plugin_task *pt,
> > +	struct request *req,
> > +	int bidi_read)
> >  {
> >  	/*
> >  	 * Defined as "scsi command" in include/linux/blkdev.h.
> >  	 */
> > -	pt->pscsi_req->cmd_type = REQ_TYPE_BLOCK_PC;
> > +	req->cmd_type = REQ_TYPE_BLOCK_PC;
> > +	/*
> > +	 * For the extra BIDI-COMMAND READ struct request we do not
> > +	 * need to setup the remaining structure members
> > +	 */
> > +	if (bidi_read)
> > +		return;
> 
> I think that if you embed this pscsi_blk_init_request() inside it's
> caller it would be more clear.
> 
> But if the caller becomes too big, and this is a logical separation
> then do the bidi_req->cmd_type = REQ_TYPE_BLOCK_PC in the caller
> and call this one only for the main request, as before.
> 
> (BTW: Bidi does not even need cmd_type = REQ_TYPE_BLOCK_PC but
>  just to make sure it's safer to do it.)

Ok, I was just following what drivers/scsi/osd/osd_initiator.c:
_init_blk_request() current does.

> 
> >  	/*
> >  	 * Setup the done function pointer for struct request,
> >  	 * also set the end_io_data pointer.to struct se_task.
> >  	 */
> > -	pt->pscsi_req->end_io = pscsi_req_done;
> > -	pt->pscsi_req->end_io_data = (void *)task;
> > +	req->end_io = pscsi_req_done;
> > +	req->end_io_data = (void *)task;
> >  	/*
> >  	 * Load the referenced struct se_task's SCSI CDB into
> >  	 * include/linux/blkdev.h:struct request->cmd
> >  	 */
> > -	pt->pscsi_req->cmd_len = scsi_command_size(pt->pscsi_cdb);
> > -	memcpy(pt->pscsi_req->cmd, pt->pscsi_cdb, pt->pscsi_req->cmd_len);
> > +	req->cmd_len = scsi_command_size(pt->pscsi_cdb);
> > +	req->cmd = &pt->pscsi_cdb[0];
> >  	/*
> >  	 * Setup pointer for outgoing sense data.
> >  	 */
> > -	pt->pscsi_req->sense = (void *)&pt->pscsi_sense[0];
> > -	pt->pscsi_req->sense_len = 0;
> > +	req->sense = (void *)&pt->pscsi_sense[0];
> > +	req->sense_len = 0;
> >  }
> >  
> >  /*
> > @@ -771,7 +779,7 @@ static int pscsi_blk_get_request(struct se_task *task)
> >  	 * Setup the newly allocated struct request for REQ_TYPE_BLOCK_PC,
> >  	 * and setup rq callback, CDB and sense.
> >  	 */
> > -	pscsi_blk_init_request(task, pt);
> > +	pscsi_blk_init_request(task, pt, pt->pscsi_req, 0);
> >  	return 0;
> >  }
> >  
> > @@ -1051,11 +1059,11 @@ static inline struct bio *pscsi_get_bio(struct pscsi_dev_virt *pdv, int sg_num)
> >  #define DEBUG_PSCSI(x...)
> >  #endif
> >  
> > -/*      pscsi_map_task_SG():
> > - *
> > - *
> > - */
> > -static int pscsi_map_task_SG(struct se_task *task)
> > +static int __pscsi_map_task_SG(
> > +	struct se_task *task,
> > +	struct scatterlist *task_sg,
> > +	u32 task_sg_num,
> > +	int bidi_read)
> >  {
> >  	struct pscsi_plugin_task *pt = (struct pscsi_plugin_task *) task->transport_req;
> >  	struct pscsi_dev_virt *pdv = (struct pscsi_dev_virt *) task->se_dev->dev_ptr;
> > @@ -1063,7 +1071,7 @@ static int pscsi_map_task_SG(struct se_task *task)
> >  	struct page *page;
> >  	struct scatterlist *sg;
> >  	u32 data_len = task->task_size, i, len, bytes, off;
> > -	int nr_pages = (task->task_size + task->task_sg[0].offset +
> > +	int nr_pages = (task->task_size + task_sg[0].offset +
> >  			PAGE_SIZE - 1) >> PAGE_SHIFT;
> 
> OK, I'm begining to suspect task->task_size is the size of what?
> See below (below)

Btw task->task_size is to set to (task->task_sectors * block_size) in
target_core_transport.c:transport_generic_get_cdb_count()

> 
> >  	int nr_vecs = 0, ret = 0;
> >  	int rw = (TASK_CMD(task)->data_direction == DMA_TO_DEVICE);
> > @@ -1083,7 +1091,7 @@ static int pscsi_map_task_SG(struct se_task *task)
> >  	 */
> >  	DEBUG_PSCSI("PSCSI: nr_pages: %d\n", nr_pages);
> >  
> > -	for_each_sg(task->task_sg, sg, task->task_sg_num, i) {
> > +	for_each_sg(task_sg, sg, task_sg_num, i) {
> >  		page = sg_page(sg);
> >  		off = sg->offset;
> >  		len = sg->length;
> 
> OK this code goes off and I don't see the rest of it. But from below with the
> local use of hbio I'm assuming you are building the hbio here right?
> 
> So I had a wild thought! instead of having a page_list ==> sg_list ==> bio
> could you keep bios at the task level? that is instead of sg_list.
> 
> So you do page_list ==> bio directly. Then in some places where you later
> need an actual sg_list for DMA. The block layer supplies an easy bio ==> sg_list
> conversion.
>
> (Just a wild idea)
> 

Hmmm, interesting point here..

> > @@ -1155,20 +1163,44 @@ static int pscsi_map_task_SG(struct se_task *task)
> >  		}
> >  	}
> >  	/*
> > -	 * Starting with v2.6.31, call blk_make_request() passing in *hbio to
> > -	 * allocate the pSCSI task a struct request.
> > +	 * Setup the primary pt->pscsi_req used for non BIDI and BIDI-COMMAND
> > +	 * primary SCSI WRITE poayload mapped for struct se_task->task_sg[]
> >  	 */
> > -	pt->pscsi_req = blk_make_request(pdv->pdv_sd->request_queue,
> > -				hbio, GFP_KERNEL);
> > -	if (!(pt->pscsi_req)) {
> > -		printk(KERN_ERR "pSCSI: blk_make_request() failed\n");
> > -		goto fail;
> > +	if (!(bidi_read)) {
> > +		/*
> > +		 * Starting with v2.6.31, call blk_make_request() passing in *hbio to
> > +		 * allocate the pSCSI task a struct request.
> > +		 */
> > +		pt->pscsi_req = blk_make_request(pdv->pdv_sd->request_queue,
> > +					hbio, GFP_KERNEL);
> > +		if (!(pt->pscsi_req)) {
> > +			printk(KERN_ERR "pSCSI: blk_make_request() failed\n");
> > +			goto fail;
> > +		}
> > +		/*
> > +		 * Setup the newly allocated struct request for REQ_TYPE_BLOCK_PC,
> > +		 * and setup rq callback, CDB and sense.
> > +		 */
> > +		pscsi_blk_init_request(task, pt, pt->pscsi_req, 0);
> > +
> > +		return task->task_sg_num;
> >  	}
> >  	/*
> > -	 * Setup the newly allocated struct request for REQ_TYPE_BLOCK_PC,
> > -	 * and setup rq callback, CDB and sense.
> > +	 * Setup the secondary pt->pscsi_req_bidi used for the extra BIDI-COMMAND
> > +	 * SCSI READ paylaod mapped for struct se_task->task_sg_bidi[]
> >  	 */
> > -	pscsi_blk_init_request(task, pt);
> > +	pt->pscsi_req_bidi = blk_make_request(pdv->pdv_sd->request_queue,
> > +					hbio, GFP_KERNEL);
> > +	if (!(pt->pscsi_req_bidi)) {
> > +		printk(KERN_ERR "pSCSI: blk_make_request() failed for BIDI\n");
> > +		goto fail;
> > +	}
> > +	pscsi_blk_init_request(task, pt, pt->pscsi_req_bidi, 1);
> > +	/*
> > +	 * Setup the magic BIDI-COMMAND ->next_req pointer to the original
> > +	 * pt->pscsi_req.
> > +	 */	
> > +	pt->pscsi_req->next_rq = pt->pscsi_req_bidi;
> >  
> >  	return task->task_sg_num;
> 
> OK Now I'm sure!
> You have completely missed the fact that bidi entails two sg_list(s)
> two sg_num(s) and two io_byte_count(s).
> 
> The use of sg_table will clear that confusion a bit, though I wanted it
> to carry an  io_byte_count as well, but never came to do that.

Hmmm, so this patch currently assumes that BIDI-COMMAND WRITE and extra
READ payloads contain the same struct se_task->task_sg_num for both
struct se_task->task_sg[] and struct se_task->task_sg_bidi[].

If we need to assume that the internally allocated (LIO-Target iSCSI)
*or* pre-exiting SGL mapped T_TASK(cmd)->t_mem_list (TCM_Loop Virtual
SCSI LLD) and T_TASK(cmd)->t_mem_bidi_list memory can be mapped
differently, then an seperate second call to transport_calc_sg_num() in
order to determine a struct se_task->task_sg_bidi_num is required.

> 
> >  fail:
> > @@ -1181,6 +1213,27 @@ fail:
> >  	return ret;
> >  }
> >  
> > +static int pscsi_map_task_SG(struct se_task *task)
> > +{
> > +	int ret;
> > +	/*
> > +	 * Setup the main struct request for the task->task_sg[] payload
> > +	 */
> > +
> > +	ret = __pscsi_map_task_SG(task, task->task_sg, task->task_sg_num, 0);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (!(task->task_sg_bidi))
> > +		return ret;
> > +	/*
> > +	 * If present, set up the extra BIDI-COMMAND SCSI READ
> > +	 * struct request and payload.
> > +	 */
> > +	return __pscsi_map_task_SG(task, task->task_sg_bidi,
> > +				task->task_sg_num, 1);
> 
> What? the task->task_sg_bidi as the same sg_num as the above
> task->task_sg. (See you should use sg_table to remove these
> bugs.
> 

<nod>

> (I wrote this comment before I wrote the above one ;-))
> 
> > +}
> > +
> >  /*	pscsi_map_task_non_SG():
> >   *
> >   *
> > @@ -1438,6 +1491,12 @@ static void pscsi_req_done(struct request *req, int uptodate)
> >  	pt->pscsi_resid = req->resid_len;
> >  
> >  	pscsi_process_SAM_status(task, pt);
> > +
> > +	if (req->next_rq != NULL) {
> > +		__blk_put_request(req->q, req->next_rq);
> 
> 		req->next_rq = NULL;
> 
> > +		pt->pscsi_req_bidi = NULL;
> > +	}
> > +
> >  	__blk_put_request(req->q, req);
> >  	pt->pscsi_req = NULL;
> >  }
> > diff --git a/drivers/target/target_core_pscsi.h b/drivers/target/target_core_pscsi.h
> > index 086e6f1..d3c8972 100644
> > --- a/drivers/target/target_core_pscsi.h
> > +++ b/drivers/target/target_core_pscsi.h
> > @@ -34,6 +34,7 @@ struct pscsi_plugin_task {
> >  	int	pscsi_result;
> >  	u32	pscsi_resid;
> >  	struct request *pscsi_req;
> > +	struct request *pscsi_req_bidi;
> 
> You don't really need to, you know. You have it right here at
> pscsi_req->next_rq. But it's your call

OK, I will go ahead and drop this item.

> 
> >  } ____cacheline_aligned;
> >  
> >  #define PDF_HAS_CHANNEL_ID	0x01
> 
> Cheers (life is hard)
> Boaz

Many thanks Boaz!

--nab

Re: [PATCH 2/3] tcm/pscsi: Add proper BIDI-COMMAND passthrough to SCSI MidLayer

[Boaz Harrosh]

On 09/22/2010 01:38 PM, Nicholas A. Bellinger wrote:
> On Wed, 2010-09-22 at 13:13 +0200, Boaz Harrosh wrote:
>>
>> OK, I'm begining to suspect task->task_size is the size of what?
>> See below (below)
> 
> Btw task->task_size is to set to (task->task_sectors * block_size) in
> target_core_transport.c:transport_generic_get_cdb_count()
> 

Maybe you are confused by these XOR functions. But there is no
task->task_sectors, only for these TYPE_DISK pesky writes/reads

For general SCSI from tapes, scanners, printers to OSD There is
in_transfer_length and out_transfer_length. For READ_XXX/WRITE_XXX
these happen to be task->task_sectors.

<snip>

>>>  	return task->task_sg_num;
>>
>> OK Now I'm sure!
>> You have completely missed the fact that bidi entails two sg_list(s)
>> two sg_num(s) and two io_byte_count(s).
>>
>> The use of sg_table will clear that confusion a bit, though I wanted it
>> to carry an  io_byte_count as well, but never came to do that.
> 
> Hmmm, so this patch currently assumes that BIDI-COMMAND WRITE and extra
> READ payloads contain the same struct se_task->task_sg_num for both
> struct se_task->task_sg[] and struct se_task->task_sg_bidi[].
> 
> If we need to assume that the internally allocated (LIO-Target iSCSI)

There is no assumption they are completely separate (Even if they happen
to be the equal).

The fabric(s) know that. For pSCSI this comes in the scsi_in()->length
and the scsi_out()->length (and ->sg_count). For iscsi it has an rlenght
AHS header that tells it the transfer length of the bidi_read side.
In iSER and I think FB it is completely symmetrical like with the BSG
API. (Member for "in" and member for "out")

And so on. Each side is completely orthogonal to the other, in every
respect. (They might even execute in parallel)

> *or* pre-exiting SGL mapped T_TASK(cmd)->t_mem_list (TCM_Loop Virtual
> SCSI LLD) and T_TASK(cmd)->t_mem_bidi_list memory can be mapped
> differently, then an seperate second call to transport_calc_sg_num() in
> order to determine a struct se_task->task_sg_bidi_num is required.
> 

Perhaps it would be easier to do, if you convert to use of scsi_data_buffer
in current code. Then apply the double instance. You might also need a similar
mechanics for your ->t_mem_list and associated members. Group them together
then double the handling.

Down the road you'll need a third one for supporting DIFF

Boaz
>>
>>>  fail:
>>> @@ -1181,6 +1213,27 @@ fail:
>>>  	return ret;
>>>  }
>>>  
>>> +static int pscsi_map_task_SG(struct se_task *task)
>>> +{
>>> +	int ret;
>>> +	/*
>>> +	 * Setup the main struct request for the task->task_sg[] payload
>>> +	 */
>>> +
>>> +	ret = __pscsi_map_task_SG(task, task->task_sg, task->task_sg_num, 0);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	if (!(task->task_sg_bidi))
>>> +		return ret;
>>> +	/*
>>> +	 * If present, set up the extra BIDI-COMMAND SCSI READ
>>> +	 * struct request and payload.
>>> +	 */
>>> +	return __pscsi_map_task_SG(task, task->task_sg_bidi,
>>> +				task->task_sg_num, 1);
>>
>> What? the task->task_sg_bidi as the same sg_num as the above
>> task->task_sg. (See you should use sg_table to remove these
>> bugs.
>>
> 
> <nod>
> 
>> (I wrote this comment before I wrote the above one ;-))
>>
>>> +}
>>> +
>>>  /*	pscsi_map_task_non_SG():
>>>   *
>>>   *
>>> @@ -1438,6 +1491,12 @@ static void pscsi_req_done(struct request *req, int uptodate)
>>>  	pt->pscsi_resid = req->resid_len;
>>>  
>>>  	pscsi_process_SAM_status(task, pt);
>>> +
>>> +	if (req->next_rq != NULL) {
>>> +		__blk_put_request(req->q, req->next_rq);
>>
>> 		req->next_rq = NULL;
>>
>>> +		pt->pscsi_req_bidi = NULL;
>>> +	}
>>> +
>>>  	__blk_put_request(req->q, req);
>>>  	pt->pscsi_req = NULL;
>>>  }
>>> diff --git a/drivers/target/target_core_pscsi.h b/drivers/target/target_core_pscsi.h
>>> index 086e6f1..d3c8972 100644
>>> --- a/drivers/target/target_core_pscsi.h
>>> +++ b/drivers/target/target_core_pscsi.h
>>> @@ -34,6 +34,7 @@ struct pscsi_plugin_task {
>>>  	int	pscsi_result;
>>>  	u32	pscsi_resid;
>>>  	struct request *pscsi_req;
>>> +	struct request *pscsi_req_bidi;
>>
>> You don't really need to, you know. You have it right here at
>> pscsi_req->next_rq. But it's your call
> 
> OK, I will go ahead and drop this item.
> 
>>
>>>  } ____cacheline_aligned;
>>>  
>>>  #define PDF_HAS_CHANNEL_ID	0x01
>>
>> Cheers (life is hard)
>> Boaz
> 
> Many thanks Boaz!
> 
> --nab
> 
> 

Re: [PATCH 2/3] tcm/pscsi: Add proper BIDI-COMMAND passthrough to SCSI MidLayer

[Nicholas A. Bellinger]

On Wed, 2010-09-22 at 14:08 +0200, Boaz Harrosh wrote:
> On 09/22/2010 01:38 PM, Nicholas A. Bellinger wrote:
> > On Wed, 2010-09-22 at 13:13 +0200, Boaz Harrosh wrote:
> >>
> >> OK, I'm begining to suspect task->task_size is the size of what?
> >> See below (below)
> > 
> > Btw task->task_size is to set to (task->task_sectors * block_size) in
> > target_core_transport.c:transport_generic_get_cdb_count()
> > 
> 
> Maybe you are confused by these XOR functions. But there is no
> task->task_sectors, only for these TYPE_DISK pesky writes/reads
> 
> For general SCSI from tapes, scanners, printers to OSD There is
> in_transfer_length and out_transfer_length. For READ_XXX/WRITE_XXX
> these happen to be task->task_sectors.
> 

OK, in the current code we do the conversion for TYPE_DISK and non
TYPE_DISK backstores w/ TCM/pSCSI in transport_set_tasks_sectors_disk()
and transport_set_tasks_sectors_non_disk().

I am pretty sure we do handle this correctly now, but I have not been
able to test with non TYPE_DISK backstores just yet..

> <snip>
> 
> >>>  	return task->task_sg_num;
> >>
> >> OK Now I'm sure!
> >> You have completely missed the fact that bidi entails two sg_list(s)
> >> two sg_num(s) and two io_byte_count(s).
> >>
> >> The use of sg_table will clear that confusion a bit, though I wanted it
> >> to carry an  io_byte_count as well, but never came to do that.
> > 
> > Hmmm, so this patch currently assumes that BIDI-COMMAND WRITE and extra
> > READ payloads contain the same struct se_task->task_sg_num for both
> > struct se_task->task_sg[] and struct se_task->task_sg_bidi[].
> > 
> > If we need to assume that the internally allocated (LIO-Target iSCSI)
> 
> There is no assumption they are completely separate (Even if they happen
> to be the equal).
> 
> The fabric(s) know that. For pSCSI this comes in the scsi_in()->length
> and the scsi_out()->length (and ->sg_count). For iscsi it has an rlenght
> AHS header that tells it the transfer length of the bidi_read side.
> In iSER and I think FB it is completely symmetrical like with the BSG
> API. (Member for "in" and member for "out")
> 
> And so on. Each side is completely orthogonal to the other, in every
> respect. (They might even execute in parallel)

Hmmm, point taken.  I will address this item later today.

> 
> > *or* pre-exiting SGL mapped T_TASK(cmd)->t_mem_list (TCM_Loop Virtual
> > SCSI LLD) and T_TASK(cmd)->t_mem_bidi_list memory can be mapped
> > differently, then an seperate second call to transport_calc_sg_num() in
> > order to determine a struct se_task->task_sg_bidi_num is required.
> > 
> 
> Perhaps it would be easier to do, if you convert to use of scsi_data_buffer
> in current code. Then apply the double instance. You might also need a similar
> mechanics for your ->t_mem_list and associated members. Group them together
> then double the handling.
> 
> Down the road you'll need a third one for supporting DIFF
> 

Hmmm, another good point for scsi_dat_buffer conversion here.

Best,

--nab