[0/3] Last 3 patches for bidi support

[0/3] Last 3 patches for bidi support

[Boaz Harrosh]

[1]
I propose a small change to scsi_tgt_lib.c that will make
tgt completely neutral to the scsi_data_buffer patch. And will
make it all that more ready for bidi, too. TOMO is this OK?

(Can you do without the GFP_KERNEL allocation flag? It could
make the code a bit more simple)

[2]
scsi_data_buffer patch. 
As requested by TOMO the all patch is squashed into one, 600 
and some lines. TOMO if it makes you happy, OK, here it is.

There is one hunk from libsrp.c that I really hate. and from what 
I understand of the code, only the "request_bufflen =" is really used, 
All users of "request_buffer = addr" pass NULL, as far as I can see.
If you would like to make me happy, and it is at all possible, please 
clean it.

[3]
Once scsi_data_buffer is in, then scsi bidi support can be applied. 
(Block bidi was already merged 3 kernels ago). It makes no changes
to scsi_cmnd and only adds some, not-at-all-dangerous, code to scsi_lib.
So I don't see any reason to wait. please all review.

If ACKed by all parties and applied for inclusion for 2.6.25, 
then they will have a long time to sit in MM and collect
compilation breakage reports from ARCHs we never compiled.

I wish these make everybody happy this time

Boaz Harrosh

[PATCH 1/3] scsi_tgt_lib: Use scsi_init_io instead of scsi_alloc_sgtable

[Boaz Harrosh]

  - If we export scsi_init_io()/scsi_release_buffers() instead of
    scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is
    much more insulated from scsi_lib changes. As a bonus it will
    also gain bidi capability when it comes.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_lib.c     |   21 ++++++++++-----------
 drivers/scsi/scsi_tgt_lib.c |   34 +++++-----------------------------
 include/scsi/scsi_cmnd.h    |    4 ++--
 3 files changed, 17 insertions(+), 42 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 0e81e4c..a8bf7cb 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -737,7 +737,8 @@ static inline unsigned int scsi_sgtable_index(unsigned short nents)
 	return index;
 }
 
-struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
+static struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd,
+                                              gfp_t gfp_mask)
 {
 	struct scsi_host_sg_pool *sgp;
 	struct scatterlist *sgl, *prev, *ret;
@@ -823,9 +824,7 @@ enomem:
 	return NULL;
 }
 
-EXPORT_SYMBOL(scsi_alloc_sgtable);
-
-void scsi_free_sgtable(struct scsi_cmnd *cmd)
+static void scsi_free_sgtable(struct scsi_cmnd *cmd)
 {
 	struct scatterlist *sgl = cmd->request_buffer;
 	struct scsi_host_sg_pool *sgp;
@@ -871,8 +870,6 @@ void scsi_free_sgtable(struct scsi_cmnd *cmd)
 	mempool_free(sgl, sgp->pool);
 }
 
-EXPORT_SYMBOL(scsi_free_sgtable);
-
 /*
  * Function:    scsi_release_buffers()
  *
@@ -890,7 +887,7 @@ EXPORT_SYMBOL(scsi_free_sgtable);
  *		the scatter-gather table, and potentially any bounce
  *		buffers.
  */
-static void scsi_release_buffers(struct scsi_cmnd *cmd)
+void scsi_release_buffers(struct scsi_cmnd *cmd)
 {
 	if (cmd->use_sg)
 		scsi_free_sgtable(cmd);
@@ -902,6 +899,7 @@ static void scsi_release_buffers(struct scsi_cmnd *cmd)
 	cmd->request_buffer = NULL;
 	cmd->request_bufflen = 0;
 }
+EXPORT_SYMBOL(scsi_release_buffers);
 
 /*
  * Function:    scsi_io_completion()
@@ -1104,7 +1102,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
  *		BLKPREP_DEFER if the failure is retryable
  *		BLKPREP_KILL if the failure is fatal
  */
-static int scsi_init_io(struct scsi_cmnd *cmd)
+int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 {
 	struct request     *req = cmd->request;
 	int		   count;
@@ -1119,7 +1117,7 @@ static int scsi_init_io(struct scsi_cmnd *cmd)
 	/*
 	 * If sg table allocation fails, requeue request later.
 	 */
-	cmd->request_buffer = scsi_alloc_sgtable(cmd, GFP_ATOMIC);
+	cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
 	if (unlikely(!cmd->request_buffer)) {
 		scsi_unprep_request(req);
 		return BLKPREP_DEFER;
@@ -1148,6 +1146,7 @@ static int scsi_init_io(struct scsi_cmnd *cmd)
 
 	return BLKPREP_KILL;
 }
+EXPORT_SYMBOL(scsi_init_io);
 
 static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
 		struct request *req)
@@ -1193,7 +1192,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
 
 		BUG_ON(!req->nr_phys_segments);
 
-		ret = scsi_init_io(cmd);
+		ret = scsi_init_io(cmd, GFP_ATOMIC);
 		if (unlikely(ret))
 			return ret;
 	} else {
@@ -1244,7 +1243,7 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
 	if (unlikely(!cmd))
 		return BLKPREP_DEFER;
 
-	return scsi_init_io(cmd);
+	return scsi_init_io(cmd, GFP_ATOMIC);
 }
 EXPORT_SYMBOL(scsi_setup_fs_cmnd);
 
diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
index deea3cd..5fd5fca 100644
--- a/drivers/scsi/scsi_tgt_lib.c
+++ b/drivers/scsi/scsi_tgt_lib.c
@@ -331,8 +331,7 @@ static void scsi_tgt_cmd_done(struct scsi_cmnd *cmd)
 
 	scsi_tgt_uspace_send_status(cmd, tcmd->itn_id, tcmd->tag);
 
-	if (scsi_sglist(cmd))
-		scsi_free_sgtable(cmd);
+	scsi_release_buffers(cmd);
 
 	queue_work(scsi_tgtd, &tcmd->work);
 }
@@ -353,31 +352,6 @@ static int scsi_tgt_transfer_response(struct scsi_cmnd *cmd)
 	return 0;
 }
 
-static int scsi_tgt_init_cmd(struct scsi_cmnd *cmd, gfp_t gfp_mask)
-{
-	struct request *rq = cmd->request;
-	int count;
-
-	cmd->use_sg = rq->nr_phys_segments;
-	cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
-	if (!cmd->request_buffer)
-		return -ENOMEM;
-
-	cmd->request_bufflen = rq->data_len;
-
-	dprintk("cmd %p cnt %d %lu\n", cmd, scsi_sg_count(cmd),
-		rq_data_dir(rq));
-	count = blk_rq_map_sg(rq->q, rq, scsi_sglist(cmd));
-	if (likely(count <= scsi_sg_count(cmd))) {
-		cmd->use_sg = count;
-		return 0;
-	}
-
-	eprintk("cmd %p cnt %d\n", cmd, scsi_sg_count(cmd));
-	scsi_free_sgtable(cmd);
-	return -EINVAL;
-}
-
 /* TODO: test this crap and replace bio_map_user with new interface maybe */
 static int scsi_map_user_pages(struct scsi_tgt_cmd *tcmd, struct scsi_cmnd *cmd,
 			       unsigned long uaddr, unsigned int len, int rw)
@@ -403,9 +377,11 @@ static int scsi_map_user_pages(struct scsi_tgt_cmd *tcmd, struct scsi_cmnd *cmd,
 	}
 
 	tcmd->bio = rq->bio;
-	err = scsi_tgt_init_cmd(cmd, GFP_KERNEL);
-	if (err)
+	err = scsi_init_io(cmd, GFP_KERNEL);
+	if (err) {
+		scsi_release_buffers(cmd);
 		goto unmap_rq;
+	}
 
 	return 0;
 
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 3f47e52..a7be605 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -128,8 +128,8 @@ extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
 				 size_t *offset, size_t *len);
 extern void scsi_kunmap_atomic_sg(void *virt);
 
-extern struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t);
-extern void scsi_free_sgtable(struct scsi_cmnd *);
+extern int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask);
+extern void scsi_release_buffers(struct scsi_cmnd *cmd);
 
 extern int scsi_dma_map(struct scsi_cmnd *cmd);
 extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
-- 
1.5.3.1

Re: [PATCH 1/3] scsi_tgt_lib: Use scsi_init_io instead of scsi_alloc_sgtable

[FUJITA Tomonori]

On Tue, 06 Nov 2007 20:16:19 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> 
>   - If we export scsi_init_io()/scsi_release_buffers() instead of
>     scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is
>     much more insulated from scsi_lib changes. As a bonus it will
>     also gain bidi capability when it comes.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>

Looks good for me except for this:

./scripts/checkpatch.pl ~/Mail/kernel/scsi/28814
ERROR: use tabs not spaces
#101: FILE: drivers/scsi/scsi_lib.c:741:
+                                              gfp_t gfp_mask)$

total: 1 errors, 0 warnings, 144 lines checked

Re: [PATCH 1/3] scsi_tgt_lib: Use scsi_init_io instead of scsi_alloc_sgtable

[Benny Halevy]

On Nov. 08, 2007, 5:13 +0200, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> On Tue, 06 Nov 2007 20:16:19 +0200
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>>   - If we export scsi_init_io()/scsi_release_buffers() instead of
>>     scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is
>>     much more insulated from scsi_lib changes. As a bonus it will
>>     also gain bidi capability when it comes.
>>
>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> 
> Looks good for me except for this:
> 
> ./scripts/checkpatch.pl ~/Mail/kernel/scsi/28814
> ERROR: use tabs not spaces
> #101: FILE: drivers/scsi/scsi_lib.c:741:
> +                                              gfp_t gfp_mask)$

Come on Tomo, tabs should be used for nesting, not for decoration.
This way no matter what's your tab expansion setup is the
code will look correct and will make sense.  The number of space
characters in this case is equal to the number of characters in the
line above and with fixed-width fonts the line will be indented
just as you wanted.

The bottom line is that you should indent with as many tabs as your
nesting level, where all statements will begin, and from there on
use space characters.

For example:

{
	if (very_long_expression &&
	    it_needs_to_be_broken_into_several_lines)
		return a_very_long_result +
		       the_remainder_of_it_that_spilled_off +
		       to_the_next_lines;

	return printk("just my %d cents\n",
	              2);
}

Benny

> 
> total: 1 errors, 0 warnings, 144 lines checked

Re: [PATCH 1/3] scsi_tgt_lib: Use scsi_init_io instead of scsi_alloc_sgtable

[FUJITA Tomonori]

On Thu, 08 Nov 2007 10:32:56 +0200
Benny Halevy <bhalevy@panasas.com> wrote:

> On Nov. 08, 2007, 5:13 +0200, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > On Tue, 06 Nov 2007 20:16:19 +0200
> > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > 
> >>   - If we export scsi_init_io()/scsi_release_buffers() instead of
> >>     scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is
> >>     much more insulated from scsi_lib changes. As a bonus it will
> >>     also gain bidi capability when it comes.
> >>
> >> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> > 
> > Looks good for me except for this:
> > 
> > ./scripts/checkpatch.pl ~/Mail/kernel/scsi/28814
> > ERROR: use tabs not spaces
> > #101: FILE: drivers/scsi/scsi_lib.c:741:
> > +                                              gfp_t gfp_mask)$
> 
> Come on Tomo, tabs should be used for nesting, not for decoration.
> This way no matter what's your tab expansion setup is the
> code will look correct and will make sense.  The number of space

I've never heard about that rule. I use tabs and minimum spaces for
decoration.

But it's just about the style. The patch is fine by me if you like to
use only spaces there.

Thanks,

Re: [PATCH 1/3] scsi_tgt_lib: Use scsi_init_io instead of scsi_alloc_sgtable

[Boaz Harrosh]

On Thu, Nov 08 2007 at 15:04 +0200, FUJITA Tomonori <tomof@acm.org> wrote:
> On Thu, 08 Nov 2007 10:32:56 +0200
> Benny Halevy <bhalevy@panasas.com> wrote:
> 
>> On Nov. 08, 2007, 5:13 +0200, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
>>> On Tue, 06 Nov 2007 20:16:19 +0200
>>> Boaz Harrosh <bharrosh@panasas.com> wrote:
>>>
>>>>   - If we export scsi_init_io()/scsi_release_buffers() instead of
>>>>     scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is
>>>>     much more insulated from scsi_lib changes. As a bonus it will
>>>>     also gain bidi capability when it comes.
>>>>
>>>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>>> Looks good for me except for this:
>>>
>>> ./scripts/checkpatch.pl ~/Mail/kernel/scsi/28814
>>> ERROR: use tabs not spaces
>>> #101: FILE: drivers/scsi/scsi_lib.c:741:
>>> +                                              gfp_t gfp_mask)$
>> Come on Tomo, tabs should be used for nesting, not for decoration.
>> This way no matter what's your tab expansion setup is the
>> code will look correct and will make sense.  The number of space
> 
> I've never heard about that rule. I use tabs and minimum spaces for
> decoration.
> 
> But it's just about the style. The patch is fine by me if you like to
> use only spaces there.
> 
> Thanks,
Thanks Tomo, I'm resending with the way you like it. You are
the maintainer and you should be comfortable with the code.

I do need your Signed-off-by on this. Since you are the maintainer.
Do you want that we push this through James, or through your tree?

Thanks again, will send revision soon.
Boaz

Re: [PATCH 1/3] scsi_tgt_lib: Use scsi_init_io instead of scsi_alloc_sgtable

[FUJITA Tomonori]

On Thu, 08 Nov 2007 16:01:53 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> On Thu, Nov 08 2007 at 15:04 +0200, FUJITA Tomonori <tomof@acm.org> wrote:
> > On Thu, 08 Nov 2007 10:32:56 +0200
> > Benny Halevy <bhalevy@panasas.com> wrote:
> > 
> >> On Nov. 08, 2007, 5:13 +0200, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> >>> On Tue, 06 Nov 2007 20:16:19 +0200
> >>> Boaz Harrosh <bharrosh@panasas.com> wrote:
> >>>
> >>>>   - If we export scsi_init_io()/scsi_release_buffers() instead of
> >>>>     scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is
> >>>>     much more insulated from scsi_lib changes. As a bonus it will
> >>>>     also gain bidi capability when it comes.
> >>>>
> >>>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> >>> Looks good for me except for this:
> >>>
> >>> ./scripts/checkpatch.pl ~/Mail/kernel/scsi/28814
> >>> ERROR: use tabs not spaces
> >>> #101: FILE: drivers/scsi/scsi_lib.c:741:
> >>> +                                              gfp_t gfp_mask)$
> >> Come on Tomo, tabs should be used for nesting, not for decoration.
> >> This way no matter what's your tab expansion setup is the
> >> code will look correct and will make sense.  The number of space
> > 
> > I've never heard about that rule. I use tabs and minimum spaces for
> > decoration.
> > 
> > But it's just about the style. The patch is fine by me if you like to
> > use only spaces there.
> > 
> > Thanks,
> Thanks Tomo, I'm resending with the way you like it. You are
> the maintainer and you should be comfortable with the code.

Actually, checkpatch complains about scsi_lib part. You don't need my
ACK on that part.


> I do need your Signed-off-by on this. Since you are the maintainer.
> Do you want that we push this through James, or through your tree?

Acked-by instead of Signed-off-by, I guess.

ACK on target mode portion. You can directly send it to James.

Oh, one more trivial stuff. Can you use 'tgt: hoge' subject? We
usually use that style for target mode though it doesn't matter much.

Thanks,

[PATCH 2/3] scsi_data_buffer

[Boaz Harrosh]

  In preparation for bidi we abstract all IO members of scsi_cmnd,
  that will need to duplicate, into a substructure.

  - Group all IO members of scsi_cmnd into a scsi_data_buffer
    structure.
  - Adjust accessors to new members.
  - scsi_{alloc,free}_sgtable receive a scsi_data_buffer instead of
    scsi_cmnd. And work on it.
  - Adjust scsi_init_io() and  scsi_release_buffers() for above
    change.
  - Fix other parts of scsi_lib/scsi.c to members migration. Use
    accessors where appropriate.

  - fix Documentation about scsi_cmnd in scsi_host.h

  - scsi_error.c
    * Changed needed members of struct scsi_eh_save.
    * Careful considerations in scsi_eh_prep/restore_cmnd.

  - sd.c and sr.c
    * sd and sr would adjust IO size to align on device's block
      size so code needs to change once we move to scsi_data_buff
      implementation.
    * Convert code to use scsi_for_each_sg
    * Use data accessors where appropriate.
    * Remove dead code (req_data_dir() != READ && != WRITE)

  - tgt: convert libsrp to use scsi_data_buffer
  - isd200: This driver still bangs on scsi_cmnd IO members,
    so need changing

 Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
 Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/libsrp.c        |    4 +-
 drivers/scsi/scsi.c          |    2 +-
 drivers/scsi/scsi_error.c    |   28 +++++---------
 drivers/scsi/scsi_lib.c      |   79 ++++++++++++++++--------------------------
 drivers/scsi/sd.c            |    7 +---
 drivers/scsi/sr.c            |   28 +++++++--------
 drivers/usb/storage/isd200.c |    8 ++--
 include/scsi/scsi_cmnd.h     |   39 +++++++++++++-------
 include/scsi/scsi_eh.h       |    8 ++---
 include/scsi/scsi_host.h     |    4 +-
 10 files changed, 92 insertions(+), 115 deletions(-)

diff --git a/drivers/scsi/libsrp.c b/drivers/scsi/libsrp.c
index 5cff020..8a8562a 100644
--- a/drivers/scsi/libsrp.c
+++ b/drivers/scsi/libsrp.c
@@ -426,8 +426,8 @@ int srp_cmd_queue(struct Scsi_Host *shost, struct srp_cmd *cmd, void *info,
 
 	sc->SCp.ptr = info;
 	memcpy(sc->cmnd, cmd->cdb, MAX_COMMAND_SIZE);
-	sc->request_bufflen = len;
-	sc->request_buffer = (void *) (unsigned long) addr;
+	sc->sdb.length = len;
+	sc->sdb.sglist = (void *) (unsigned long) addr;
 	sc->tag = tag;
 	err = scsi_tgt_queue_command(sc, itn_id, (struct scsi_lun *)&cmd->lun,
 				     cmd->tag);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 1929488..73d2216 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -698,7 +698,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
 				"Notifying upper driver of completion "
 				"(result %x)\n", cmd->result));
 
-	good_bytes = cmd->request_bufflen;
+	good_bytes = scsi_bufflen(cmd);
         if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
 		drv = scsi_cmd_to_driver(cmd);
 		if (drv->done)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index ebaca4c..e7b87ea 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -617,29 +617,25 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,
 	ses->cmd_len = scmd->cmd_len;
 	memcpy(ses->cmnd, scmd->cmnd, sizeof(scmd->cmnd));
 	ses->data_direction = scmd->sc_data_direction;
-	ses->bufflen = scmd->request_bufflen;
-	ses->buffer = scmd->request_buffer;
-	ses->use_sg = scmd->use_sg;
-	ses->resid = scmd->resid;
+	ses->sdb = scmd->sdb;
 	ses->result = scmd->result;
 
+	memset(&scmd->sdb, 0, sizeof(scmd->sdb));
+
 	if (sense_bytes) {
-		scmd->request_bufflen = min_t(unsigned,
+		scmd->sdb.length = min_t(unsigned,
 		                       sizeof(scmd->sense_buffer), sense_bytes);
 		sg_init_one(&ses->sense_sgl, scmd->sense_buffer,
-		                                       scmd->request_bufflen);
-		scmd->request_buffer = &ses->sense_sgl;
+		                                       scmd->sdb.length);
+		scmd->sdb.sglist = &ses->sense_sgl;
 		scmd->sc_data_direction = DMA_FROM_DEVICE;
-		scmd->use_sg = 1;
+		scmd->sdb.sg_count = 1;
 		memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
 		scmd->cmnd[0] = REQUEST_SENSE;
-		scmd->cmnd[4] = scmd->request_bufflen;
+		scmd->cmnd[4] = scmd->sdb.length;
 		scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
 	} else {
-		scmd->request_buffer = NULL;
-		scmd->request_bufflen = 0;
 		scmd->sc_data_direction = DMA_NONE;
-		scmd->use_sg = 0;
 		if (cmnd) {
 			memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
 			memcpy(scmd->cmnd, cmnd, cmnd_size);
@@ -676,10 +672,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, struct scsi_eh_save *ses)
 	scmd->cmd_len = ses->cmd_len;
 	memcpy(scmd->cmnd, ses->cmnd, sizeof(scmd->cmnd));
 	scmd->sc_data_direction = ses->data_direction;
-	scmd->request_bufflen = ses->bufflen;
-	scmd->request_buffer = ses->buffer;
-	scmd->use_sg = ses->use_sg;
-	scmd->resid = ses->resid;
+	scmd->sdb = ses->sdb;
 	scmd->result = ses->result;
 }
 EXPORT_SYMBOL(scsi_eh_restore_cmnd);
@@ -1699,8 +1692,7 @@ scsi_reset_provider(struct scsi_device *dev, int flag)
 	memset(&scmd->cmnd, '\0', sizeof(scmd->cmnd));
     
 	scmd->scsi_done		= scsi_reset_provider_done_command;
-	scmd->request_buffer		= NULL;
-	scmd->request_bufflen		= 0;
+	memset(&scmd->sdb, 0, sizeof(scmd->sdb));
 
 	scmd->cmd_len			= 0;
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index a8bf7cb..f904692 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -438,7 +438,7 @@ EXPORT_SYMBOL_GPL(scsi_execute_async);
 static void scsi_init_cmd_errh(struct scsi_cmnd *cmd)
 {
 	cmd->serial_number = 0;
-	cmd->resid = 0;
+	scsi_set_resid(cmd, 0);
 	memset(cmd->sense_buffer, 0, sizeof cmd->sense_buffer);
 	if (cmd->cmd_len == 0)
 		cmd->cmd_len = COMMAND_SIZE(cmd->cmnd[0]);
@@ -737,17 +737,15 @@ static inline unsigned int scsi_sgtable_index(unsigned short nents)
 	return index;
 }
 
-static struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd,
-                                              gfp_t gfp_mask)
+static int scsi_alloc_sgtable(struct scsi_data_buffer *sdb,
+                                       unsigned short sg_count, gfp_t gfp_mask)
 {
 	struct scsi_host_sg_pool *sgp;
 	struct scatterlist *sgl, *prev, *ret;
 	unsigned int index;
 	int this, left;
 
-	BUG_ON(!cmd->use_sg);
-
-	left = cmd->use_sg;
+	left = sg_count;
 	ret = prev = NULL;
 	do {
 		this = left;
@@ -801,8 +799,9 @@ static struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd,
 	 * ->use_sg may get modified after dma mapping has potentially
 	 * shrunk the number of segments, so keep a copy of it for free.
 	 */
-	cmd->__use_sg = cmd->use_sg;
-	return ret;
+	sdb->alloc_sg_count = sdb->sg_count = sg_count;
+	sdb->sglist = ret;
+	return 0;
 enomem:
 	if (ret) {
 		/*
@@ -821,24 +820,24 @@ enomem:
 
 		mempool_free(prev, sgp->pool);
 	}
-	return NULL;
+	return -1;
 }
 
-static void scsi_free_sgtable(struct scsi_cmnd *cmd)
+static void scsi_free_sgtable(struct scsi_data_buffer *sdb)
 {
-	struct scatterlist *sgl = cmd->request_buffer;
+	struct scatterlist *sgl = sdb->sglist;
 	struct scsi_host_sg_pool *sgp;
 
 	/*
 	 * if this is the biggest size sglist, check if we have
 	 * chained parts we need to free
 	 */
-	if (cmd->__use_sg > SCSI_MAX_SG_SEGMENTS) {
+	if (sdb->alloc_sg_count > SCSI_MAX_SG_SEGMENTS) {
 		unsigned short this, left;
 		struct scatterlist *next;
 		unsigned int index;
 
-		left = cmd->__use_sg - (SCSI_MAX_SG_SEGMENTS - 1);
+		left = sdb->alloc_sg_count - (SCSI_MAX_SG_SEGMENTS - 1);
 		next = sg_chain_ptr(&sgl[SCSI_MAX_SG_SEGMENTS - 1]);
 		while (left && next) {
 			sgl = next;
@@ -862,10 +861,10 @@ static void scsi_free_sgtable(struct scsi_cmnd *cmd)
 		/*
 		 * Restore original, will be freed below
 		 */
-		sgl = cmd->request_buffer;
+		sgl = sdb->sglist;
 		sgp = scsi_sg_pools + SG_MEMPOOL_NR - 1;
 	} else
-		sgp = scsi_sg_pools + scsi_sgtable_index(cmd->__use_sg);
+		sgp = scsi_sg_pools + scsi_sgtable_index(sdb->alloc_sg_count);
 
 	mempool_free(sgl, sgp->pool);
 }
@@ -889,15 +888,10 @@ static void scsi_free_sgtable(struct scsi_cmnd *cmd)
  */
 void scsi_release_buffers(struct scsi_cmnd *cmd)
 {
-	if (cmd->use_sg)
-		scsi_free_sgtable(cmd);
+	if (cmd->sdb.sglist)
+		scsi_free_sgtable(&cmd->sdb);
 
-	/*
-	 * Zero these out.  They now point to freed memory, and it is
-	 * dangerous to hang onto the pointers.
-	 */
-	cmd->request_buffer = NULL;
-	cmd->request_bufflen = 0;
+	memset(&cmd->sdb, 0, sizeof(cmd->sdb));
 }
 EXPORT_SYMBOL(scsi_release_buffers);
 
@@ -932,7 +926,7 @@ EXPORT_SYMBOL(scsi_release_buffers);
 void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 {
 	int result = cmd->result;
-	int this_count = cmd->request_bufflen;
+	int this_count = scsi_bufflen(cmd);
 	struct request_queue *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
 	int clear_errors = 1;
@@ -940,8 +934,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	int sense_valid = 0;
 	int sense_deferred = 0;
 
-	scsi_release_buffers(cmd);
-
 	if (result) {
 		sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
 		if (sense_valid)
@@ -964,9 +956,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				req->sense_len = len;
 			}
 		}
-		req->data_len = cmd->resid;
+		req->data_len = scsi_get_resid(cmd);
 	}
 
+	scsi_release_buffers(cmd);
+
 	/*
 	 * Next deal with any sectors which we were able to correctly
 	 * handle.
@@ -974,7 +968,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	SCSI_LOG_HLCOMPLETE(1, printk("%ld sectors total, "
 				      "%d bytes done.\n",
 				      req->nr_sectors, good_bytes));
-	SCSI_LOG_HLCOMPLETE(1, printk("use_sg is %d\n", cmd->use_sg));
 
 	if (clear_errors)
 		req->errors = 0;
@@ -1106,41 +1099,31 @@ int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 {
 	struct request     *req = cmd->request;
 	int		   count;
+	struct scsi_data_buffer *sdb = &cmd->sdb;
 
-	/*
-	 * We used to not use scatter-gather for single segment request,
-	 * but now we do (it makes highmem I/O easier to support without
-	 * kmapping pages)
-	 */
-	cmd->use_sg = req->nr_phys_segments;
-
-	/*
-	 * If sg table allocation fails, requeue request later.
-	 */
-	cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
-	if (unlikely(!cmd->request_buffer)) {
+	if (scsi_alloc_sgtable(sdb, req->nr_phys_segments, gfp_mask)) {
 		scsi_unprep_request(req);
 		return BLKPREP_DEFER;
 	}
 
 	req->buffer = NULL;
 	if (blk_pc_request(req))
-		cmd->request_bufflen = req->data_len;
+		sdb->length = req->data_len;
 	else
-		cmd->request_bufflen = req->nr_sectors << 9;
+		sdb->length = req->nr_sectors << 9;
 
 	/* 
 	 * Next, walk the list, and fill in the addresses and sizes of
 	 * each segment.
 	 */
-	count = blk_rq_map_sg(req->q, req, cmd->request_buffer);
-	if (likely(count <= cmd->use_sg)) {
-		cmd->use_sg = count;
+	count = blk_rq_map_sg(req->q, req, sdb->sglist);
+	if (likely(count <= sdb->sg_count)) {
+		sdb->sg_count = count;
 		return BLKPREP_OK;
 	}
 
 	printk(KERN_ERR "Incorrect number of segments after building list\n");
-	printk(KERN_ERR "counted %d, received %d\n", count, cmd->use_sg);
+	printk(KERN_ERR "counted %d, received %d\n", count, sdb->sg_count);
 	printk(KERN_ERR "req nr_sec %lu, cur_nr_sec %u\n", req->nr_sectors,
 			req->current_nr_sectors);
 
@@ -1199,9 +1182,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
 		BUG_ON(req->data_len);
 		BUG_ON(req->data);
 
-		cmd->request_bufflen = 0;
-		cmd->request_buffer = NULL;
-		cmd->use_sg = 0;
+		memset(&cmd->sdb, 0, sizeof(cmd->sdb));
 		req->buffer = NULL;
 	}
 
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 18343a6..28cf6fe 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -448,9 +448,6 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	} else if (rq_data_dir(rq) == READ) {
 		SCpnt->cmnd[0] = READ_6;
 		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-	} else {
-		scmd_printk(KERN_ERR, SCpnt, "Unknown command %x\n", rq->cmd_flags);
-		goto out;
 	}
 
 	SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
@@ -510,7 +507,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 		SCpnt->cmnd[4] = (unsigned char) this_count;
 		SCpnt->cmnd[5] = 0;
 	}
-	SCpnt->request_bufflen = this_count * sdp->sector_size;
+	SCpnt->sdb.length = this_count * sdp->sector_size;
 
 	/*
 	 * We shouldn't disconnect in the middle of a sector, so with a dumb
@@ -910,7 +907,7 @@ static struct block_device_operations sd_fops = {
 static int sd_done(struct scsi_cmnd *SCpnt)
 {
 	int result = SCpnt->result;
- 	unsigned int xfer_size = SCpnt->request_bufflen;
+	unsigned int xfer_size = scsi_bufflen(SCpnt);
  	unsigned int good_bytes = result ? 0 : xfer_size;
  	u64 start_lba = SCpnt->request->sector;
  	u64 bad_lba;
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 7702681..6d3bf41 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -226,7 +226,7 @@ out:
 static int sr_done(struct scsi_cmnd *SCpnt)
 {
 	int result = SCpnt->result;
-	int this_count = SCpnt->request_bufflen;
+	int this_count = scsi_bufflen(SCpnt);
 	int good_bytes = (result == 0 ? this_count : 0);
 	int block_sectors = 0;
 	long error_sector;
@@ -368,23 +368,21 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 	} else if (rq_data_dir(rq) == READ) {
 		SCpnt->cmnd[0] = READ_10;
 		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-	} else {
-		blk_dump_rq_flags(rq, "Unknown sr command");
-		goto out;
 	}
 
 	{
-		struct scatterlist *sg = SCpnt->request_buffer;
-		int i, size = 0;
-		for (i = 0; i < SCpnt->use_sg; i++)
-			size += sg[i].length;
+		struct scatterlist *sg;
+		int i, size = 0, sg_count = scsi_sg_count(SCpnt);
+
+		scsi_for_each_sg (SCpnt, sg, sg_count, i)
+			size += sg->length;
 
-		if (size != SCpnt->request_bufflen && SCpnt->use_sg) {
+		if (size != scsi_bufflen(SCpnt)) {
 			scmd_printk(KERN_ERR, SCpnt,
 				"mismatch count %d, bytes %d\n",
-				size, SCpnt->request_bufflen);
-			if (SCpnt->request_bufflen > size)
-				SCpnt->request_bufflen = size;
+				size, scsi_bufflen(SCpnt));
+			if (scsi_bufflen(SCpnt) > size)
+				SCpnt->sdb.length = size;
 		}
 	}
 
@@ -392,12 +390,12 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 	 * request doesn't start on hw block boundary, add scatter pads
 	 */
 	if (((unsigned int)rq->sector % (s_size >> 9)) ||
-	    (SCpnt->request_bufflen % s_size)) {
+	    (scsi_bufflen(SCpnt) % s_size)) {
 		scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n");
 		goto out;
 	}
 
-	this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9);
+	this_count = (scsi_bufflen(SCpnt) >> 9) / (s_size >> 9);
 
 
 	SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n",
@@ -411,7 +409,7 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 
 	if (this_count > 0xffff) {
 		this_count = 0xffff;
-		SCpnt->request_bufflen = this_count * s_size;
+		SCpnt->sdb.length = this_count * s_size;
 	}
 
 	SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index 178e8c2..2d9a32b 100644
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
@@ -415,14 +415,14 @@ static void isd200_set_srb(struct isd200_info *info,
 		sg_init_one(&info->sg, buff, bufflen);
 
 	srb->sc_data_direction = dir;
-	srb->request_buffer = buff ? &info->sg : NULL;
-	srb->request_bufflen = bufflen;
-	srb->use_sg = buff ? 1 : 0;
+	srb->sdb.sglist = buff ? &info->sg : NULL;
+	srb->sdb.length = bufflen;
+	srb->sdb.sg_count = buff ? 1 : 0;
 }
 
 static void isd200_srb_set_bufflen(struct scsi_cmnd *srb, unsigned bufflen)
 {
-	srb->request_bufflen = bufflen;
+	srb->sdb.length = bufflen;
 }
 
 
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index a7be605..4e3cf43 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -12,6 +12,13 @@ struct scatterlist;
 struct Scsi_Host;
 struct scsi_device;
 
+struct scsi_data_buffer {
+	unsigned length;
+	int resid;
+	unsigned short sg_count;
+	unsigned short alloc_sg_count;
+	struct scatterlist* sglist;
+};
 
 /* embedded in scsi_cmnd */
 struct scsi_pointer {
@@ -62,15 +69,10 @@ struct scsi_cmnd {
 	/* These elements define the operation we are about to perform */
 #define MAX_COMMAND_SIZE	16
 	unsigned char cmnd[MAX_COMMAND_SIZE];
-	unsigned request_bufflen;	/* Actual request size */
 
 	struct timer_list eh_timeout;	/* Used to time out the command. */
-	void *request_buffer;		/* Actual requested buffer */
 
 	/* These elements define the operation we ultimately want to perform */
-	unsigned short use_sg;	/* Number of pieces of scatter-gather */
-	unsigned short __use_sg;
-
 	unsigned underflow;	/* Return error if less than
 				   this amount is transferred */
 
@@ -80,10 +82,6 @@ struct scsi_cmnd {
 				   reconnects.   Probably == sector
 				   size */
 
-	int resid;		/* Number of bytes requested to be
-				   transferred less actual number
-				   transferred (0 if not supported) */
-
 	struct request *request;	/* The command we are
 				   	   working on */
 
@@ -114,6 +112,8 @@ struct scsi_cmnd {
 	int result;		/* Status code from lower level driver */
 
 	unsigned char tag;	/* SCSI-II queued command tag */
+
+	struct scsi_data_buffer sdb;
 };
 
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
@@ -134,18 +134,29 @@ extern void scsi_release_buffers(struct scsi_cmnd *cmd);
 extern int scsi_dma_map(struct scsi_cmnd *cmd);
 extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
 
-#define scsi_sg_count(cmd) ((cmd)->use_sg)
-#define scsi_sglist(cmd) ((struct scatterlist *)(cmd)->request_buffer)
-#define scsi_bufflen(cmd) ((cmd)->request_bufflen)
+static inline unsigned scsi_sg_count(struct scsi_cmnd *cmd)
+{
+	return cmd->sdb.sg_count;
+}
+
+static inline struct scatterlist *scsi_sglist(struct scsi_cmnd *cmd)
+{
+	return cmd->sdb.sglist;
+}
+
+static inline unsigned scsi_bufflen(struct scsi_cmnd *cmd)
+{
+	return cmd->sdb.length;
+}
 
 static inline void scsi_set_resid(struct scsi_cmnd *cmd, int resid)
 {
-	cmd->resid = resid;
+	cmd->sdb.resid = resid;
 }
 
 static inline int scsi_get_resid(struct scsi_cmnd *cmd)
 {
-	return cmd->resid;
+	return cmd->sdb.resid;
 }
 
 #define scsi_for_each_sg(cmd, sg, nseg, __i)			\
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index d21b891..1e08be1 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -68,16 +68,14 @@ extern int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
 extern int scsi_reset_provider(struct scsi_device *, int);
 
 struct scsi_eh_save {
+	/* saved state */
 	int result;
 	enum dma_data_direction data_direction;
 	unsigned char cmd_len;
 	unsigned char cmnd[MAX_COMMAND_SIZE];
+	struct scsi_data_buffer sdb;
 
-	void *buffer;
-	unsigned bufflen;
-	unsigned short use_sg;
-	int resid;
-
+	/* new command support */
 	struct scatterlist sense_sgl;
 };
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 0fd4746..cb2bcab 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -136,9 +136,9 @@ struct scsi_host_template {
 	 * the done callback is invoked.
 	 *
 	 * This is called to inform the LLD to transfer
-	 * cmd->request_bufflen bytes. The cmd->use_sg speciefies the
+	 * scsi_bufflen(cmd) bytes. scsi_sg_count(cmd) speciefies the
 	 * number of scatterlist entried in the command and
-	 * cmd->request_buffer contains the scatterlist.
+	 * scsi_sglist(cmd) returns the scatterlist.
 	 *
 	 * return values: see queuecommand
 	 *
-- 
1.5.3.1

Re: [PATCH 2/3] scsi_data_buffer

[FUJITA Tomonori]

On Tue, 06 Nov 2007 20:19:32 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> 
>   In preparation for bidi we abstract all IO members of scsi_cmnd,
>   that will need to duplicate, into a substructure.
> 
>   - Group all IO members of scsi_cmnd into a scsi_data_buffer
>     structure.
>   - Adjust accessors to new members.
>   - scsi_{alloc,free}_sgtable receive a scsi_data_buffer instead of
>     scsi_cmnd. And work on it.
>   - Adjust scsi_init_io() and  scsi_release_buffers() for above
>     change.
>   - Fix other parts of scsi_lib/scsi.c to members migration. Use
>     accessors where appropriate.
> 
>   - fix Documentation about scsi_cmnd in scsi_host.h
> 
>   - scsi_error.c
>     * Changed needed members of struct scsi_eh_save.
>     * Careful considerations in scsi_eh_prep/restore_cmnd.
> 
>   - sd.c and sr.c
>     * sd and sr would adjust IO size to align on device's block
>       size so code needs to change once we move to scsi_data_buff
>       implementation.
>     * Convert code to use scsi_for_each_sg
>     * Use data accessors where appropriate.
>     * Remove dead code (req_data_dir() != READ && != WRITE)
> 
>   - tgt: convert libsrp to use scsi_data_buffer
>   - isd200: This driver still bangs on scsi_cmnd IO members,
>     so need changing
> 
>  Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>  Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

Hmm, checkpatch.pl complains reasonably:

./scripts/checkpatch.pl ~/Mail/kernel/scsi/28815
ERROR: use tabs not spaces
#177: FILE: drivers/scsi/scsi_error.c:629:
+^I^I                                       scmd->sdb.length);$

ERROR: use tabs not spaces
#237: FILE: drivers/scsi/scsi_lib.c:741:
+                                       unsigned short sg_count, gfp_t gfp_mask)$

WARNING: no space between function name and open parenthesis '('
#487: FILE: drivers/scsi/sr.c:377:
+               scsi_for_each_sg (SCpnt, sg, sg_count, i)

ERROR: "foo* bar" should be "foo *bar"
#563: FILE: include/scsi/scsi_cmnd.h:20:
+       struct scatterlist* sglist;

total: 3 errors, 1 warnings, 482 lines checked


> diff --git a/drivers/scsi/libsrp.c b/drivers/scsi/libsrp.c
> index 5cff020..8a8562a 100644
> --- a/drivers/scsi/libsrp.c
> +++ b/drivers/scsi/libsrp.c
> @@ -426,8 +426,8 @@ int srp_cmd_queue(struct Scsi_Host *shost, struct srp_cmd *cmd, void *info,
>  
>  	sc->SCp.ptr = info;
>  	memcpy(sc->cmnd, cmd->cdb, MAX_COMMAND_SIZE);
> -	sc->request_bufflen = len;
> -	sc->request_buffer = (void *) (unsigned long) addr;
> +	sc->sdb.length = len;
> +	sc->sdb.sglist = (void *) (unsigned long) addr;
>  	sc->tag = tag;
>  	err = scsi_tgt_queue_command(sc, itn_id, (struct scsi_lun *)&cmd->lun,
>  				     cmd->tag);
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 1929488..73d2216 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -698,7 +698,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
>  				"Notifying upper driver of completion "
>  				"(result %x)\n", cmd->result));
>  
> -	good_bytes = cmd->request_bufflen;
> +	good_bytes = scsi_bufflen(cmd);
>          if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
>  		drv = scsi_cmd_to_driver(cmd);
>  		if (drv->done)
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index ebaca4c..e7b87ea 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -617,29 +617,25 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,
>  	ses->cmd_len = scmd->cmd_len;
>  	memcpy(ses->cmnd, scmd->cmnd, sizeof(scmd->cmnd));
>  	ses->data_direction = scmd->sc_data_direction;
> -	ses->bufflen = scmd->request_bufflen;
> -	ses->buffer = scmd->request_buffer;
> -	ses->use_sg = scmd->use_sg;
> -	ses->resid = scmd->resid;
> +	ses->sdb = scmd->sdb;
>  	ses->result = scmd->result;
>  
> +	memset(&scmd->sdb, 0, sizeof(scmd->sdb));
> +
>  	if (sense_bytes) {
> -		scmd->request_bufflen = min_t(unsigned,
> +		scmd->sdb.length = min_t(unsigned,
>  		                       sizeof(scmd->sense_buffer), sense_bytes);
>  		sg_init_one(&ses->sense_sgl, scmd->sense_buffer,
> -		                                       scmd->request_bufflen);
> -		scmd->request_buffer = &ses->sense_sgl;
> +		                                       scmd->sdb.length);
> +		scmd->sdb.sglist = &ses->sense_sgl;
>  		scmd->sc_data_direction = DMA_FROM_DEVICE;
> -		scmd->use_sg = 1;
> +		scmd->sdb.sg_count = 1;
>  		memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
>  		scmd->cmnd[0] = REQUEST_SENSE;
> -		scmd->cmnd[4] = scmd->request_bufflen;
> +		scmd->cmnd[4] = scmd->sdb.length;
>  		scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
>  	} else {
> -		scmd->request_buffer = NULL;
> -		scmd->request_bufflen = 0;
>  		scmd->sc_data_direction = DMA_NONE;
> -		scmd->use_sg = 0;
>  		if (cmnd) {
>  			memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
>  			memcpy(scmd->cmnd, cmnd, cmnd_size);
> @@ -676,10 +672,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, struct scsi_eh_save *ses)
>  	scmd->cmd_len = ses->cmd_len;
>  	memcpy(scmd->cmnd, ses->cmnd, sizeof(scmd->cmnd));
>  	scmd->sc_data_direction = ses->data_direction;
> -	scmd->request_bufflen = ses->bufflen;
> -	scmd->request_buffer = ses->buffer;
> -	scmd->use_sg = ses->use_sg;
> -	scmd->resid = ses->resid;
> +	scmd->sdb = ses->sdb;
>  	scmd->result = ses->result;
>  }
>  EXPORT_SYMBOL(scsi_eh_restore_cmnd);
> @@ -1699,8 +1692,7 @@ scsi_reset_provider(struct scsi_device *dev, int flag)
>  	memset(&scmd->cmnd, '\0', sizeof(scmd->cmnd));
>      
>  	scmd->scsi_done		= scsi_reset_provider_done_command;
> -	scmd->request_buffer		= NULL;
> -	scmd->request_bufflen		= 0;
> +	memset(&scmd->sdb, 0, sizeof(scmd->sdb));
>  
>  	scmd->cmd_len			= 0;
>  
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index a8bf7cb..f904692 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -438,7 +438,7 @@ EXPORT_SYMBOL_GPL(scsi_execute_async);
>  static void scsi_init_cmd_errh(struct scsi_cmnd *cmd)
>  {
>  	cmd->serial_number = 0;
> -	cmd->resid = 0;
> +	scsi_set_resid(cmd, 0);
>  	memset(cmd->sense_buffer, 0, sizeof cmd->sense_buffer);
>  	if (cmd->cmd_len == 0)
>  		cmd->cmd_len = COMMAND_SIZE(cmd->cmnd[0]);
> @@ -737,17 +737,15 @@ static inline unsigned int scsi_sgtable_index(unsigned short nents)
>  	return index;
>  }
>  
> -static struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd,
> -                                              gfp_t gfp_mask)
> +static int scsi_alloc_sgtable(struct scsi_data_buffer *sdb,
> +                                       unsigned short sg_count, gfp_t gfp_mask)
>  {
>  	struct scsi_host_sg_pool *sgp;
>  	struct scatterlist *sgl, *prev, *ret;
>  	unsigned int index;
>  	int this, left;
>  
> -	BUG_ON(!cmd->use_sg);
> -
> -	left = cmd->use_sg;
> +	left = sg_count;
>  	ret = prev = NULL;
>  	do {
>  		this = left;
> @@ -801,8 +799,9 @@ static struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd,
>  	 * ->use_sg may get modified after dma mapping has potentially
>  	 * shrunk the number of segments, so keep a copy of it for free.
>  	 */
> -	cmd->__use_sg = cmd->use_sg;
> -	return ret;
> +	sdb->alloc_sg_count = sdb->sg_count = sg_count;
> +	sdb->sglist = ret;
> +	return 0;
>  enomem:
>  	if (ret) {
>  		/*
> @@ -821,24 +820,24 @@ enomem:
>  
>  		mempool_free(prev, sgp->pool);
>  	}
> -	return NULL;
> +	return -1;

I think that -ENOMEM is better. The other functions in scsi_lib.c
(even static functions) use proper error values.


>  }
>  
> -static void scsi_free_sgtable(struct scsi_cmnd *cmd)
> +static void scsi_free_sgtable(struct scsi_data_buffer *sdb)
>  {
> -	struct scatterlist *sgl = cmd->request_buffer;
> +	struct scatterlist *sgl = sdb->sglist;
>  	struct scsi_host_sg_pool *sgp;
>  
>  	/*
>  	 * if this is the biggest size sglist, check if we have
>  	 * chained parts we need to free
>  	 */
> -	if (cmd->__use_sg > SCSI_MAX_SG_SEGMENTS) {
> +	if (sdb->alloc_sg_count > SCSI_MAX_SG_SEGMENTS) {
>  		unsigned short this, left;
>  		struct scatterlist *next;
>  		unsigned int index;
>  
> -		left = cmd->__use_sg - (SCSI_MAX_SG_SEGMENTS - 1);
> +		left = sdb->alloc_sg_count - (SCSI_MAX_SG_SEGMENTS - 1);
>  		next = sg_chain_ptr(&sgl[SCSI_MAX_SG_SEGMENTS - 1]);
>  		while (left && next) {
>  			sgl = next;
> @@ -862,10 +861,10 @@ static void scsi_free_sgtable(struct scsi_cmnd *cmd)
>  		/*
>  		 * Restore original, will be freed below
>  		 */
> -		sgl = cmd->request_buffer;
> +		sgl = sdb->sglist;
>  		sgp = scsi_sg_pools + SG_MEMPOOL_NR - 1;
>  	} else
> -		sgp = scsi_sg_pools + scsi_sgtable_index(cmd->__use_sg);
> +		sgp = scsi_sg_pools + scsi_sgtable_index(sdb->alloc_sg_count);
>  
>  	mempool_free(sgl, sgp->pool);
>  }
> @@ -889,15 +888,10 @@ static void scsi_free_sgtable(struct scsi_cmnd *cmd)
>   */
>  void scsi_release_buffers(struct scsi_cmnd *cmd)
>  {
> -	if (cmd->use_sg)
> -		scsi_free_sgtable(cmd);
> +	if (cmd->sdb.sglist)
> +		scsi_free_sgtable(&cmd->sdb);
>  
> -	/*
> -	 * Zero these out.  They now point to freed memory, and it is
> -	 * dangerous to hang onto the pointers.
> -	 */
> -	cmd->request_buffer = NULL;
> -	cmd->request_bufflen = 0;
> +	memset(&cmd->sdb, 0, sizeof(cmd->sdb));
>  }
>  EXPORT_SYMBOL(scsi_release_buffers);
>  
> @@ -932,7 +926,7 @@ EXPORT_SYMBOL(scsi_release_buffers);
>  void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  {
>  	int result = cmd->result;
> -	int this_count = cmd->request_bufflen;
> +	int this_count = scsi_bufflen(cmd);
>  	struct request_queue *q = cmd->device->request_queue;
>  	struct request *req = cmd->request;
>  	int clear_errors = 1;
> @@ -940,8 +934,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  	int sense_valid = 0;
>  	int sense_deferred = 0;
>  
> -	scsi_release_buffers(cmd);
> -
>  	if (result) {
>  		sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
>  		if (sense_valid)
> @@ -964,9 +956,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  				req->sense_len = len;
>  			}
>  		}
> -		req->data_len = cmd->resid;
> +		req->data_len = scsi_get_resid(cmd);
>  	}
>  
> +	scsi_release_buffers(cmd);
> +
>  	/*
>  	 * Next deal with any sectors which we were able to correctly
>  	 * handle.
> @@ -974,7 +968,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  	SCSI_LOG_HLCOMPLETE(1, printk("%ld sectors total, "
>  				      "%d bytes done.\n",
>  				      req->nr_sectors, good_bytes));
> -	SCSI_LOG_HLCOMPLETE(1, printk("use_sg is %d\n", cmd->use_sg));
>  
>  	if (clear_errors)
>  		req->errors = 0;
> @@ -1106,41 +1099,31 @@ int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
>  {
>  	struct request     *req = cmd->request;
>  	int		   count;
> +	struct scsi_data_buffer *sdb = &cmd->sdb;
>  
> -	/*
> -	 * We used to not use scatter-gather for single segment request,
> -	 * but now we do (it makes highmem I/O easier to support without
> -	 * kmapping pages)
> -	 */
> -	cmd->use_sg = req->nr_phys_segments;
> -
> -	/*
> -	 * If sg table allocation fails, requeue request later.
> -	 */
> -	cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
> -	if (unlikely(!cmd->request_buffer)) {
> +	if (scsi_alloc_sgtable(sdb, req->nr_phys_segments, gfp_mask)) {

IIRC, preferable style is:

	ret = scsi_alloc_sgtable(sdb, req->nr_phys_segments, gfp_mask);
	if (ret) {


>  		scsi_unprep_request(req);
>  		return BLKPREP_DEFER;
>  	}
>  
>  	req->buffer = NULL;
>  	if (blk_pc_request(req))
> -		cmd->request_bufflen = req->data_len;
> +		sdb->length = req->data_len;
>  	else
> -		cmd->request_bufflen = req->nr_sectors << 9;
> +		sdb->length = req->nr_sectors << 9;
>  
>  	/* 
>  	 * Next, walk the list, and fill in the addresses and sizes of
>  	 * each segment.
>  	 */
> -	count = blk_rq_map_sg(req->q, req, cmd->request_buffer);
> -	if (likely(count <= cmd->use_sg)) {
> -		cmd->use_sg = count;
> +	count = blk_rq_map_sg(req->q, req, sdb->sglist);
> +	if (likely(count <= sdb->sg_count)) {
> +		sdb->sg_count = count;
>  		return BLKPREP_OK;
>  	}
>  
>  	printk(KERN_ERR "Incorrect number of segments after building list\n");
> -	printk(KERN_ERR "counted %d, received %d\n", count, cmd->use_sg);
> +	printk(KERN_ERR "counted %d, received %d\n", count, sdb->sg_count);
>  	printk(KERN_ERR "req nr_sec %lu, cur_nr_sec %u\n", req->nr_sectors,
>  			req->current_nr_sectors);
>  
> @@ -1199,9 +1182,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
>  		BUG_ON(req->data_len);
>  		BUG_ON(req->data);
>  
> -		cmd->request_bufflen = 0;
> -		cmd->request_buffer = NULL;
> -		cmd->use_sg = 0;
> +		memset(&cmd->sdb, 0, sizeof(cmd->sdb));
>  		req->buffer = NULL;
>  	}
>  
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 18343a6..28cf6fe 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -448,9 +448,6 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
>  	} else if (rq_data_dir(rq) == READ) {
>  		SCpnt->cmnd[0] = READ_6;
>  		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
> -	} else {
> -		scmd_printk(KERN_ERR, SCpnt, "Unknown command %x\n", rq->cmd_flags);
> -		goto out;

This should go to the bidi patch?

>  	}
>  
>  	SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
> @@ -510,7 +507,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
>  		SCpnt->cmnd[4] = (unsigned char) this_count;
>  		SCpnt->cmnd[5] = 0;
>  	}
> -	SCpnt->request_bufflen = this_count * sdp->sector_size;
> +	SCpnt->sdb.length = this_count * sdp->sector_size;
>  
>  	/*
>  	 * We shouldn't disconnect in the middle of a sector, so with a dumb
> @@ -910,7 +907,7 @@ static struct block_device_operations sd_fops = {
>  static int sd_done(struct scsi_cmnd *SCpnt)
>  {
>  	int result = SCpnt->result;
> - 	unsigned int xfer_size = SCpnt->request_bufflen;
> +	unsigned int xfer_size = scsi_bufflen(SCpnt);
>   	unsigned int good_bytes = result ? 0 : xfer_size;
>   	u64 start_lba = SCpnt->request->sector;
>   	u64 bad_lba;
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 7702681..6d3bf41 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -226,7 +226,7 @@ out:
>  static int sr_done(struct scsi_cmnd *SCpnt)
>  {
>  	int result = SCpnt->result;
> -	int this_count = SCpnt->request_bufflen;
> +	int this_count = scsi_bufflen(SCpnt);
>  	int good_bytes = (result == 0 ? this_count : 0);
>  	int block_sectors = 0;
>  	long error_sector;
> @@ -368,23 +368,21 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
>  	} else if (rq_data_dir(rq) == READ) {
>  		SCpnt->cmnd[0] = READ_10;
>  		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
> -	} else {
> -		blk_dump_rq_flags(rq, "Unknown sr command");
> -		goto out;

Ditto.


>  	}
>  
>  	{
> -		struct scatterlist *sg = SCpnt->request_buffer;
> -		int i, size = 0;
> -		for (i = 0; i < SCpnt->use_sg; i++)
> -			size += sg[i].length;
> +		struct scatterlist *sg;
> +		int i, size = 0, sg_count = scsi_sg_count(SCpnt);
> +
> +		scsi_for_each_sg (SCpnt, sg, sg_count, i)
> +			size += sg->length;
>  
> -		if (size != SCpnt->request_bufflen && SCpnt->use_sg) {
> +		if (size != scsi_bufflen(SCpnt)) {
>  			scmd_printk(KERN_ERR, SCpnt,
>  				"mismatch count %d, bytes %d\n",
> -				size, SCpnt->request_bufflen);
> -			if (SCpnt->request_bufflen > size)
> -				SCpnt->request_bufflen = size;
> +				size, scsi_bufflen(SCpnt));
> +			if (scsi_bufflen(SCpnt) > size)
> +				SCpnt->sdb.length = size;
>  		}
>  	}
>  
> @@ -392,12 +390,12 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
>  	 * request doesn't start on hw block boundary, add scatter pads
>  	 */
>  	if (((unsigned int)rq->sector % (s_size >> 9)) ||
> -	    (SCpnt->request_bufflen % s_size)) {
> +	    (scsi_bufflen(SCpnt) % s_size)) {
>  		scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n");
>  		goto out;
>  	}
>  
> -	this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9);
> +	this_count = (scsi_bufflen(SCpnt) >> 9) / (s_size >> 9);
>  
>  
>  	SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n",
> @@ -411,7 +409,7 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
>  
>  	if (this_count > 0xffff) {
>  		this_count = 0xffff;
> -		SCpnt->request_bufflen = this_count * s_size;
> +		SCpnt->sdb.length = this_count * s_size;
>  	}
>  
>  	SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
> diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
> index 178e8c2..2d9a32b 100644
> --- a/drivers/usb/storage/isd200.c
> +++ b/drivers/usb/storage/isd200.c
> @@ -415,14 +415,14 @@ static void isd200_set_srb(struct isd200_info *info,
>  		sg_init_one(&info->sg, buff, bufflen);
>  
>  	srb->sc_data_direction = dir;
> -	srb->request_buffer = buff ? &info->sg : NULL;
> -	srb->request_bufflen = bufflen;
> -	srb->use_sg = buff ? 1 : 0;
> +	srb->sdb.sglist = buff ? &info->sg : NULL;
> +	srb->sdb.length = bufflen;
> +	srb->sdb.sg_count = buff ? 1 : 0;
>  }
>  
>  static void isd200_srb_set_bufflen(struct scsi_cmnd *srb, unsigned bufflen)
>  {
> -	srb->request_bufflen = bufflen;
> +	srb->sdb.length = bufflen;
>  }
>  
>  
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index a7be605..4e3cf43 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -12,6 +12,13 @@ struct scatterlist;
>  struct Scsi_Host;
>  struct scsi_device;
>  
> +struct scsi_data_buffer {
> +	unsigned length;
> +	int resid;
> +	unsigned short sg_count;
> +	unsigned short alloc_sg_count;

sg_count and alloc_sg_count are a bit lengthy?

My suggestion is using nr_* like the block layer (nr_sg and
nr_alloc_sg ?).


> +	struct scatterlist* sglist;
> +};
>  
>  /* embedded in scsi_cmnd */
>  struct scsi_pointer {
> @@ -62,15 +69,10 @@ struct scsi_cmnd {
>  	/* These elements define the operation we are about to perform */
>  #define MAX_COMMAND_SIZE	16
>  	unsigned char cmnd[MAX_COMMAND_SIZE];
> -	unsigned request_bufflen;	/* Actual request size */
>  
>  	struct timer_list eh_timeout;	/* Used to time out the command. */
> -	void *request_buffer;		/* Actual requested buffer */
>  
>  	/* These elements define the operation we ultimately want to perform */
> -	unsigned short use_sg;	/* Number of pieces of scatter-gather */
> -	unsigned short __use_sg;
> -
>  	unsigned underflow;	/* Return error if less than
>  				   this amount is transferred */
>  
> @@ -80,10 +82,6 @@ struct scsi_cmnd {
>  				   reconnects.   Probably == sector
>  				   size */
>  
> -	int resid;		/* Number of bytes requested to be
> -				   transferred less actual number
> -				   transferred (0 if not supported) */
> -
>  	struct request *request;	/* The command we are
>  				   	   working on */
>  
> @@ -114,6 +112,8 @@ struct scsi_cmnd {
>  	int result;		/* Status code from lower level driver */
>  
>  	unsigned char tag;	/* SCSI-II queued command tag */
> +
> +	struct scsi_data_buffer sdb;
>  };
>  
>  extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
> @@ -134,18 +134,29 @@ extern void scsi_release_buffers(struct scsi_cmnd *cmd);
>  extern int scsi_dma_map(struct scsi_cmnd *cmd);
>  extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
>  
> -#define scsi_sg_count(cmd) ((cmd)->use_sg)
> -#define scsi_sglist(cmd) ((struct scatterlist *)(cmd)->request_buffer)
> -#define scsi_bufflen(cmd) ((cmd)->request_bufflen)
> +static inline unsigned scsi_sg_count(struct scsi_cmnd *cmd)
> +{
> +	return cmd->sdb.sg_count;
> +}
> +
> +static inline struct scatterlist *scsi_sglist(struct scsi_cmnd *cmd)
> +{
> +	return cmd->sdb.sglist;
> +}
> +
> +static inline unsigned scsi_bufflen(struct scsi_cmnd *cmd)
> +{
> +	return cmd->sdb.length;
> +}
>  
>  static inline void scsi_set_resid(struct scsi_cmnd *cmd, int resid)
>  {
> -	cmd->resid = resid;
> +	cmd->sdb.resid = resid;
>  }
>  
>  static inline int scsi_get_resid(struct scsi_cmnd *cmd)
>  {
> -	return cmd->resid;
> +	return cmd->sdb.resid;
>  }
>  
>  #define scsi_for_each_sg(cmd, sg, nseg, __i)			\
> diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
> index d21b891..1e08be1 100644
> --- a/include/scsi/scsi_eh.h
> +++ b/include/scsi/scsi_eh.h
> @@ -68,16 +68,14 @@ extern int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
>  extern int scsi_reset_provider(struct scsi_device *, int);
>  
>  struct scsi_eh_save {
> +	/* saved state */
>  	int result;
>  	enum dma_data_direction data_direction;
>  	unsigned char cmd_len;
>  	unsigned char cmnd[MAX_COMMAND_SIZE];
> +	struct scsi_data_buffer sdb;
>  
> -	void *buffer;
> -	unsigned bufflen;
> -	unsigned short use_sg;
> -	int resid;
> -
> +	/* new command support */
>  	struct scatterlist sense_sgl;
>  };
>  
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 0fd4746..cb2bcab 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -136,9 +136,9 @@ struct scsi_host_template {
>  	 * the done callback is invoked.
>  	 *
>  	 * This is called to inform the LLD to transfer
> -	 * cmd->request_bufflen bytes. The cmd->use_sg speciefies the
> +	 * scsi_bufflen(cmd) bytes. scsi_sg_count(cmd) speciefies the
>  	 * number of scatterlist entried in the command and
> -	 * cmd->request_buffer contains the scatterlist.
> +	 * scsi_sglist(cmd) returns the scatterlist.
>  	 *
>  	 * return values: see queuecommand
>  	 *
> -- 
> 1.5.3.1
> 
> 
> -
> 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 2/3] scsi_data_buffer

[Boaz Harrosh]

On Thu, Nov 08 2007 at 5:14 +0200, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> On Tue, 06 Nov 2007 20:19:32 +0200
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
<snip>
> 
> Hmm, checkpatch.pl complains reasonably:
> 
> ./scripts/checkpatch.pl ~/Mail/kernel/scsi/28815
> ERROR: use tabs not spaces
> #177: FILE: drivers/scsi/scsi_error.c:629:
> +^I^I                                       scmd->sdb.length);$
> 
> ERROR: use tabs not spaces
> #237: FILE: drivers/scsi/scsi_lib.c:741:
> +                                       unsigned short sg_count, gfp_t gfp_mask)$
> 
> WARNING: no space between function name and open parenthesis '('
> #487: FILE: drivers/scsi/sr.c:377:
> +               scsi_for_each_sg (SCpnt, sg, sg_count, i)
> 
> ERROR: "foo* bar" should be "foo *bar"
> #563: FILE: include/scsi/scsi_cmnd.h:20:
> +       struct scatterlist* sglist;
> 
> total: 3 errors, 1 warnings, 482 lines checked
I think that checkpatch is wrong in two accounts.
1. Tabs are used for "Indents" not "align-to-the-right".
   All above have 0 indent and are right aligned for
   readability.
2. check patch should be fixed that if a macro is followed
   by a "{" it means a control block and not a function-call/
   macro-expansion, and a space is recommended.
   (Like: for, if, while ...)

  But I don't mind. I'll change all in a fixing patch.
> 
> 
<snip>

>> @@ -821,24 +820,24 @@ enomem:
>>  
>>  		mempool_free(prev, sgp->pool);
>>  	}
>> -	return NULL;
>> +	return -1;
> 
> I think that -ENOMEM is better. The other functions in scsi_lib.c
> (even static functions) use proper error values.
> 
> 
will fix, thanks.

<snip>
>> -	/*
>> -	 * If sg table allocation fails, requeue request later.
>> -	 */
>> -	cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
>> -	if (unlikely(!cmd->request_buffer)) {
>> +	if (scsi_alloc_sgtable(sdb, req->nr_phys_segments, gfp_mask)) {
> 
> IIRC, preferable style is:
> 
> 	ret = scsi_alloc_sgtable(sdb, req->nr_phys_segments, gfp_mask);
> 	if (ret) {
> 
> 
It is more readable I agree, but I did not want to allocate one more
stack variable just for the if(), since I am returning a BLKPREP_DEFER
any way. I am not using ret for anything else.

>>  		scsi_unprep_request(req);
>>  		return BLKPREP_DEFER;
>>  	}
>>  
>>  	req->buffer = NULL;
>>  	if (blk_pc_request(req))
>> -		cmd->request_bufflen = req->data_len;
>> +		sdb->length = req->data_len;
>>  	else
>> -		cmd->request_bufflen = req->nr_sectors << 9;
>> +		sdb->length = req->nr_sectors << 9;
>>  
>>  	/* 
>>  	 * Next, walk the list, and fill in the addresses and sizes of
>>  	 * each segment.
>>  	 */
>> -	count = blk_rq_map_sg(req->q, req, cmd->request_buffer);
>> -	if (likely(count <= cmd->use_sg)) {
>> -		cmd->use_sg = count;
>> +	count = blk_rq_map_sg(req->q, req, sdb->sglist);
>> +	if (likely(count <= sdb->sg_count)) {
>> +		sdb->sg_count = count;
>>  		return BLKPREP_OK;
>>  	}
>>  
>>  	printk(KERN_ERR "Incorrect number of segments after building list\n");
>> -	printk(KERN_ERR "counted %d, received %d\n", count, cmd->use_sg);
>> +	printk(KERN_ERR "counted %d, received %d\n", count, sdb->sg_count);
>>  	printk(KERN_ERR "req nr_sec %lu, cur_nr_sec %u\n", req->nr_sectors,
>>  			req->current_nr_sectors);
>>  
>> @@ -1199,9 +1182,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
>>  		BUG_ON(req->data_len);
>>  		BUG_ON(req->data);
>>  
>> -		cmd->request_bufflen = 0;
>> -		cmd->request_buffer = NULL;
>> -		cmd->use_sg = 0;
>> +		memset(&cmd->sdb, 0, sizeof(cmd->sdb));
>>  		req->buffer = NULL;
>>  	}
>>  

>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 18343a6..28cf6fe 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -448,9 +448,6 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
>>  	} else if (rq_data_dir(rq) == READ) {
>>  		SCpnt->cmnd[0] = READ_6;
>>  		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
>> -	} else {
>> -		scmd_printk(KERN_ERR, SCpnt, "Unknown command %x\n", rq->cmd_flags);
>> -		goto out;
> 
> This should go to the bidi patch?
> 
>>  	}
>>  
This is just a dead code cleanup. It is got nothing to do with bidi or scsi_data_buffer
for that matter. It could be in it's own patch, but surly it will not go into the bidi
patch. I will submit a new patch just for that code. Independent of these.
(I was hoping to save the extra effort)

>>  	SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
>> @@ -510,7 +507,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
>>  		SCpnt->cmnd[4] = (unsigned char) this_count;
>>  		SCpnt->cmnd[5] = 0;
>>  	}
>> -	SCpnt->request_bufflen = this_count * sdp->sector_size;
>> +	SCpnt->sdb.length = this_count * sdp->sector_size;
>>  
>>  	/*
>>  	 * We shouldn't disconnect in the middle of a sector, so with a dumb
>> @@ -910,7 +907,7 @@ static struct block_device_operations sd_fops = {
>>  static int sd_done(struct scsi_cmnd *SCpnt)
>>  {
>>  	int result = SCpnt->result;
>> - 	unsigned int xfer_size = SCpnt->request_bufflen;
>> +	unsigned int xfer_size = scsi_bufflen(SCpnt);
>>   	unsigned int good_bytes = result ? 0 : xfer_size;
>>   	u64 start_lba = SCpnt->request->sector;
>>   	u64 bad_lba;
>> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
>> index 7702681..6d3bf41 100644
>> --- a/drivers/scsi/sr.c
>> +++ b/drivers/scsi/sr.c
>> @@ -226,7 +226,7 @@ out:
>>  static int sr_done(struct scsi_cmnd *SCpnt)
>>  {
>>  	int result = SCpnt->result;
>> -	int this_count = SCpnt->request_bufflen;
>> +	int this_count = scsi_bufflen(SCpnt);
>>  	int good_bytes = (result == 0 ? this_count : 0);
>>  	int block_sectors = 0;
>>  	long error_sector;
>> @@ -368,23 +368,21 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
>>  	} else if (rq_data_dir(rq) == READ) {
>>  		SCpnt->cmnd[0] = READ_10;
>>  		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
>> -	} else {
>> -		blk_dump_rq_flags(rq, "Unknown sr command");
>> -		goto out;
> 
> Ditto.
> 
Ditto

> 
>>  	}
>>  
>>  	{
>> -		struct scatterlist *sg = SCpnt->request_buffer;
>> -		int i, size = 0;
>> -		for (i = 0; i < SCpnt->use_sg; i++)
>> -			size += sg[i].length;
>> +		struct scatterlist *sg;
>> +		int i, size = 0, sg_count = scsi_sg_count(SCpnt);
>> +
>> +		scsi_for_each_sg (SCpnt, sg, sg_count, i)
>> +			size += sg->length;
>>  
>> -		if (size != SCpnt->request_bufflen && SCpnt->use_sg) {
>> +		if (size != scsi_bufflen(SCpnt)) {
>>  			scmd_printk(KERN_ERR, SCpnt,
>>  				"mismatch count %d, bytes %d\n",
>> -				size, SCpnt->request_bufflen);
>> -			if (SCpnt->request_bufflen > size)
>> -				SCpnt->request_bufflen = size;
>> +				size, scsi_bufflen(SCpnt));
>> +			if (scsi_bufflen(SCpnt) > size)
>> +				SCpnt->sdb.length = size;
>>  		}
>>  	}
>>  
>> @@ -392,12 +390,12 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
>>  	 * request doesn't start on hw block boundary, add scatter pads
>>  	 */
>>  	if (((unsigned int)rq->sector % (s_size >> 9)) ||
>> -	    (SCpnt->request_bufflen % s_size)) {
>> +	    (scsi_bufflen(SCpnt) % s_size)) {
>>  		scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n");
>>  		goto out;
>>  	}
>>  
>> -	this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9);
>> +	this_count = (scsi_bufflen(SCpnt) >> 9) / (s_size >> 9);
>>  
>>  
>>  	SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n",
>> @@ -411,7 +409,7 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
>>  
>>  	if (this_count > 0xffff) {
>>  		this_count = 0xffff;
>> -		SCpnt->request_bufflen = this_count * s_size;
>> +		SCpnt->sdb.length = this_count * s_size;
>>  	}
>>  
>>  	SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
>> diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
>> index 178e8c2..2d9a32b 100644
>> --- a/drivers/usb/storage/isd200.c
>> +++ b/drivers/usb/storage/isd200.c
>> @@ -415,14 +415,14 @@ static void isd200_set_srb(struct isd200_info *info,
>>  		sg_init_one(&info->sg, buff, bufflen);
>>  
>>  	srb->sc_data_direction = dir;
>> -	srb->request_buffer = buff ? &info->sg : NULL;
>> -	srb->request_bufflen = bufflen;
>> -	srb->use_sg = buff ? 1 : 0;
>> +	srb->sdb.sglist = buff ? &info->sg : NULL;
>> +	srb->sdb.length = bufflen;
>> +	srb->sdb.sg_count = buff ? 1 : 0;
>>  }
>>  
>>  static void isd200_srb_set_bufflen(struct scsi_cmnd *srb, unsigned bufflen)
>>  {
>> -	srb->request_bufflen = bufflen;
>> +	srb->sdb.length = bufflen;
>>  }
>>  
>>  
>> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
>> index a7be605..4e3cf43 100644
>> --- a/include/scsi/scsi_cmnd.h
>> +++ b/include/scsi/scsi_cmnd.h
>> @@ -12,6 +12,13 @@ struct scatterlist;
>>  struct Scsi_Host;
>>  struct scsi_device;
>>  
>> +struct scsi_data_buffer {
>> +	unsigned length;
>> +	int resid;
>> +	unsigned short sg_count;
>> +	unsigned short alloc_sg_count;
> 
> sg_count and alloc_sg_count are a bit lengthy?
> 
> My suggestion is using nr_* like the block layer (nr_sg and
> nr_alloc_sg ?).

will not! sg_count is the name you gave to the accessor and it
must be the same. alloc_sg_count is a private member that must only
be used by scsi_lib.c. If the long name is a discouragement of use
than that much better.

> 
> 
>> +	struct scatterlist* sglist;
>> +};
>>  
>>  /* embedded in scsi_cmnd */
>>  struct scsi_pointer {
>> @@ -62,15 +69,10 @@ struct scsi_cmnd {
>>  	/* These elements define the operation we are about to perform */
>>  #define MAX_COMMAND_SIZE	16
>>  	unsigned char cmnd[MAX_COMMAND_SIZE];
>> -	unsigned request_bufflen;	/* Actual request size */
>>  
>>  	struct timer_list eh_timeout;	/* Used to time out the command. */
>> -	void *request_buffer;		/* Actual requested buffer */
>>  
>>  	/* These elements define the operation we ultimately want to perform */
>> -	unsigned short use_sg;	/* Number of pieces of scatter-gather */
>> -	unsigned short __use_sg;
>> -
>>  	unsigned underflow;	/* Return error if less than
>>  				   this amount is transferred */
>>  
<snip>

Will send two new patches.

Boaz

Re: [PATCH 2/3] scsi_data_buffer

[FUJITA Tomonori]

On Thu, 08 Nov 2007 11:24:36 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> >> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> >> index 18343a6..28cf6fe 100644
> >> --- a/drivers/scsi/sd.c
> >> +++ b/drivers/scsi/sd.c
> >> @@ -448,9 +448,6 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
> >>  	} else if (rq_data_dir(rq) == READ) {
> >>  		SCpnt->cmnd[0] = READ_6;
> >>  		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
> >> -	} else {
> >> -		scmd_printk(KERN_ERR, SCpnt, "Unknown command %x\n", rq->cmd_flags);
> >> -		goto out;
> > 
> > This should go to the bidi patch?
> > 
> >>  	}
> >>  
> This is just a dead code cleanup. It is got nothing to do with bidi or scsi_data_buffer
> for that matter. It could be in it's own patch, but surly it will not go into the bidi
> patch. I will submit a new patch just for that code. Independent of these.
> (I was hoping to save the extra effort)

Hm, is it dead code? I think it's kinda BUG_ON, that is, we should not
hit that code. sd only accetps READ and WRITE requests. It prevents
funcy requests like BIDI from accidentally comming.

Re: [PATCH 2/3] scsi_data_buffer

[Boaz Harrosh]

On Thu, Nov 08 2007 at 15:03 +0200, FUJITA Tomonori <tomof@acm.org> wrote:
> On Thu, 08 Nov 2007 11:24:36 +0200
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>>>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>>>> index 18343a6..28cf6fe 100644
>>>> --- a/drivers/scsi/sd.c
>>>> +++ b/drivers/scsi/sd.c
>>>> @@ -448,9 +448,6 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
>>>>  	} else if (rq_data_dir(rq) == READ) {
>>>>  		SCpnt->cmnd[0] = READ_6;
>>>>  		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
>>>> -	} else {
>>>> -		scmd_printk(KERN_ERR, SCpnt, "Unknown command %x\n", rq->cmd_flags);
>>>> -		goto out;
>>> This should go to the bidi patch?
>>>
>>>>  	}
>>>>  
>> This is just a dead code cleanup. It is got nothing to do with bidi or scsi_data_buffer
>> for that matter. It could be in it's own patch, but surly it will not go into the bidi
>> patch. I will submit a new patch just for that code. Independent of these.
>> (I was hoping to save the extra effort)
> 
> Hm, is it dead code? I think it's kinda BUG_ON, that is, we should not
> hit that code. sd only accetps READ and WRITE requests. It prevents
> funcy requests like BIDI from accidentally comming.
It is dead code. The rq_data_dir(rq) does a (->flags & 0x1) inline
the compiler will remove the extra code.

Also with bidi rq_data_dir(rq) is decided to return WRITE, a bidi
request is blk_bidi_rq(rq).

(I have separated this to a patch of it's own.)

Boaz

Re: [PATCH 2/3] scsi_data_buffer

[Boaz Harrosh]

James, Jens please note the question below
It is something that bothers me about sr.c 

On Tue, Nov 06 2007 at 20:19 +0200, Boaz Harrosh <bharrosh@panasas.com> wrote:
>   In preparation for bidi we abstract all IO members of scsi_cmnd,
>   that will need to duplicate, into a substructure.
> 
<snip>
> 
>   - sd.c and sr.c
>     * sd and sr would adjust IO size to align on device's block
>       size so code needs to change once we move to scsi_data_buff
>       implementation.
>     * Convert code to use scsi_for_each_sg
>     * Use data accessors where appropriate.
>     * Remove dead code (req_data_dir() != READ && != WRITE)
> 
<snip>
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 7702681..6d3bf41 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -226,7 +226,7 @@ out:
>  static int sr_done(struct scsi_cmnd *SCpnt)
>  {
>  	int result = SCpnt->result;
> -	int this_count = SCpnt->request_bufflen;
> +	int this_count = scsi_bufflen(SCpnt);
>  	int good_bytes = (result == 0 ? this_count : 0);
>  	int block_sectors = 0;
>  	long error_sector;
> @@ -368,23 +368,21 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
>  	} else if (rq_data_dir(rq) == READ) {
>  		SCpnt->cmnd[0] = READ_10;
>  		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
> -	} else {
> -		blk_dump_rq_flags(rq, "Unknown sr command");
> -		goto out;
>  	}
>  
>  	{
> -		struct scatterlist *sg = SCpnt->request_buffer;
> -		int i, size = 0;
> -		for (i = 0; i < SCpnt->use_sg; i++)
> -			size += sg[i].length;
> +		struct scatterlist *sg;
> +		int i, size = 0, sg_count = scsi_sg_count(SCpnt);
> +
> +		scsi_for_each_sg (SCpnt, sg, sg_count, i)
> +			size += sg->length;
>  
> -		if (size != SCpnt->request_bufflen && SCpnt->use_sg) {
> +		if (size != scsi_bufflen(SCpnt)) {
>  			scmd_printk(KERN_ERR, SCpnt,
>  				"mismatch count %d, bytes %d\n",
> -				size, SCpnt->request_bufflen);
> -			if (SCpnt->request_bufflen > size)
> -				SCpnt->request_bufflen = size;
> +				size, scsi_bufflen(SCpnt));
> +			if (scsi_bufflen(SCpnt) > size)
> +				SCpnt->sdb.length = size;
>  		}
>  	}
>  
> @@ -392,12 +390,12 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
>  	 * request doesn't start on hw block boundary, add scatter pads
>  	 */
>  	if (((unsigned int)rq->sector % (s_size >> 9)) ||
> -	    (SCpnt->request_bufflen % s_size)) {
> +	    (scsi_bufflen(SCpnt) % s_size)) {
>  		scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n");
>  		goto out;
>  	}
Here we check I/O is "large-block" aligned. Both start and size

>  
> -	this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9);
> +	this_count = (scsi_bufflen(SCpnt) >> 9) / (s_size >> 9);
>  
Number of "large-blocks"

>  
>  	SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n",
> @@ -411,7 +409,7 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
>  
>  	if (this_count > 0xffff) {
>  		this_count = 0xffff;
> -		SCpnt->request_bufflen = this_count * s_size;
> +		SCpnt->sdb.length = this_count * s_size;
>  	}
>  
Here is my problem:
In the case that the transfer is bigger than 0xffff * s_size (512/1024/2048)
we modify ->request_bufflen. Now this has two bad effects, the way I understand it,
please fix me in my misunderstanding.

1. Later in sr_done doing return good_bytes=cmd->request_bufflen will only complete
  the cut-out bytes. Meaning possible BIO leak, since the original request_bufflen
  was lost. (not all bytes are completed)

2. What mechanics will re-send, or even knows, that not the complete request was 
  transfered? The way I understand it, a cmnd->resid must be set, in this case. 
  Now the normal cmnd->resid is not enough because it will be written by
  drivers, sr needs to stash a resid somewhere and add it to cmnd->resid in
  sr_done(). But ...

I have a better solution for this. At attachment time. sr will modify the
request_queue's max_hw_sectors to not max over 0xffff * s_size, this way
the block layer will split it's I/O, and no extra resid handling is needed.

If the later is accepted than where this blk_set_max_hw_sectors() be?
on the first request, like block-size. Or at sr_open()/sr_block_open()

Please comment and I'll scribble a patch.

>  	SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
> diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
> index 178e8c2..2d9a32b 100644
> --- a/drivers/usb/storage/isd200.c
> +++ b/drivers/usb/storage/isd200.c
<snip>

Thanks
Boaz

Re: [PATCH 2/3] scsi_data_buffer

[Jens Axboe]

On Thu, Nov 08 2007, Boaz Harrosh wrote:
> James, Jens please note the question below
> It is something that bothers me about sr.c 
> 
> On Tue, Nov 06 2007 at 20:19 +0200, Boaz Harrosh <bharrosh@panasas.com> wrote:
> >   In preparation for bidi we abstract all IO members of scsi_cmnd,
> >   that will need to duplicate, into a substructure.
> > 
> <snip>
> > 
> >   - sd.c and sr.c
> >     * sd and sr would adjust IO size to align on device's block
> >       size so code needs to change once we move to scsi_data_buff
> >       implementation.
> >     * Convert code to use scsi_for_each_sg
> >     * Use data accessors where appropriate.
> >     * Remove dead code (req_data_dir() != READ && != WRITE)
> > 
> <snip>
> > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> > index 7702681..6d3bf41 100644
> > --- a/drivers/scsi/sr.c
> > +++ b/drivers/scsi/sr.c
> > @@ -226,7 +226,7 @@ out:
> >  static int sr_done(struct scsi_cmnd *SCpnt)
> >  {
> >  	int result = SCpnt->result;
> > -	int this_count = SCpnt->request_bufflen;
> > +	int this_count = scsi_bufflen(SCpnt);
> >  	int good_bytes = (result == 0 ? this_count : 0);
> >  	int block_sectors = 0;
> >  	long error_sector;
> > @@ -368,23 +368,21 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
> >  	} else if (rq_data_dir(rq) == READ) {
> >  		SCpnt->cmnd[0] = READ_10;
> >  		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
> > -	} else {
> > -		blk_dump_rq_flags(rq, "Unknown sr command");
> > -		goto out;
> >  	}
> >  
> >  	{
> > -		struct scatterlist *sg = SCpnt->request_buffer;
> > -		int i, size = 0;
> > -		for (i = 0; i < SCpnt->use_sg; i++)
> > -			size += sg[i].length;
> > +		struct scatterlist *sg;
> > +		int i, size = 0, sg_count = scsi_sg_count(SCpnt);
> > +
> > +		scsi_for_each_sg (SCpnt, sg, sg_count, i)
> > +			size += sg->length;
> >  
> > -		if (size != SCpnt->request_bufflen && SCpnt->use_sg) {
> > +		if (size != scsi_bufflen(SCpnt)) {
> >  			scmd_printk(KERN_ERR, SCpnt,
> >  				"mismatch count %d, bytes %d\n",
> > -				size, SCpnt->request_bufflen);
> > -			if (SCpnt->request_bufflen > size)
> > -				SCpnt->request_bufflen = size;
> > +				size, scsi_bufflen(SCpnt));
> > +			if (scsi_bufflen(SCpnt) > size)
> > +				SCpnt->sdb.length = size;
> >  		}
> >  	}
> >  
> > @@ -392,12 +390,12 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
> >  	 * request doesn't start on hw block boundary, add scatter pads
> >  	 */
> >  	if (((unsigned int)rq->sector % (s_size >> 9)) ||
> > -	    (SCpnt->request_bufflen % s_size)) {
> > +	    (scsi_bufflen(SCpnt) % s_size)) {
> >  		scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n");
> >  		goto out;
> >  	}
> Here we check I/O is "large-block" aligned. Both start and size
> 
> >  
> > -	this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9);
> > +	this_count = (scsi_bufflen(SCpnt) >> 9) / (s_size >> 9);
> >  
> Number of "large-blocks"
> 
> >  
> >  	SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n",
> > @@ -411,7 +409,7 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
> >  
> >  	if (this_count > 0xffff) {
> >  		this_count = 0xffff;
> > -		SCpnt->request_bufflen = this_count * s_size;
> > +		SCpnt->sdb.length = this_count * s_size;
> >  	}
> >  
> Here is my problem:
> In the case that the transfer is bigger than 0xffff * s_size
> (512/1024/2048) we modify ->request_bufflen. Now this has two bad
> effects, the way I understand it, please fix me in my
> misunderstanding.
> 
> 1. Later in sr_done doing return good_bytes=cmd->request_bufflen will
> only complete the cut-out bytes. Meaning possible BIO leak, since the
> original request_bufflen was lost. (not all bytes are completed)
>
> 
> 2. What mechanics will re-send, or even knows, that not the complete
> request was transfered? The way I understand it, a cmnd->resid must be
> set, in this case.  Now the normal cmnd->resid is not enough because
> it will be written by drivers, sr needs to stash a resid somewhere and
> add it to cmnd->resid in sr_done(). But ...
> 

It's not lost, the request will be requeued in scsi_end_request(). The
original state is in the request structure, and end_that_request_chunk()
will return not-done when you complete this first part.

-- 
Jens Axboe

Re: [PATCH 2/3] scsi_data_buffer

[Boaz Harrosh]

On Thu, Nov 08 2007 at 15:54 +0200, Jens Axboe <jens.axboe@oracle.com> wrote:
> On Thu, Nov 08 2007, Boaz Harrosh wrote:
>> James, Jens please note the question below
>> It is something that bothers me about sr.c 
>>
>> On Tue, Nov 06 2007 at 20:19 +0200, Boaz Harrosh <bharrosh@panasas.com> wrote:
>>>   In preparation for bidi we abstract all IO members of scsi_cmnd,
>>>   that will need to duplicate, into a substructure.
>>>
>> <snip>
>>>   - sd.c and sr.c
>>>     * sd and sr would adjust IO size to align on device's block
>>>       size so code needs to change once we move to scsi_data_buff
>>>       implementation.
>>>     * Convert code to use scsi_for_each_sg
>>>     * Use data accessors where appropriate.
>>>     * Remove dead code (req_data_dir() != READ && != WRITE)
>>>
>> <snip>
>>> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
>>> index 7702681..6d3bf41 100644
>>> --- a/drivers/scsi/sr.c
>>> +++ b/drivers/scsi/sr.c
>>> @@ -226,7 +226,7 @@ out:
>>>  static int sr_done(struct scsi_cmnd *SCpnt)
>>>  {
>>>  	int result = SCpnt->result;
>>> -	int this_count = SCpnt->request_bufflen;
>>> +	int this_count = scsi_bufflen(SCpnt);
>>>  	int good_bytes = (result == 0 ? this_count : 0);
>>>  	int block_sectors = 0;
>>>  	long error_sector;
>>> @@ -368,23 +368,21 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
>>>  	} else if (rq_data_dir(rq) == READ) {
>>>  		SCpnt->cmnd[0] = READ_10;
>>>  		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
>>> -	} else {
>>> -		blk_dump_rq_flags(rq, "Unknown sr command");
>>> -		goto out;
>>>  	}
>>>  
>>>  	{
>>> -		struct scatterlist *sg = SCpnt->request_buffer;
>>> -		int i, size = 0;
>>> -		for (i = 0; i < SCpnt->use_sg; i++)
>>> -			size += sg[i].length;
>>> +		struct scatterlist *sg;
>>> +		int i, size = 0, sg_count = scsi_sg_count(SCpnt);
>>> +
>>> +		scsi_for_each_sg (SCpnt, sg, sg_count, i)
>>> +			size += sg->length;
>>>  
>>> -		if (size != SCpnt->request_bufflen && SCpnt->use_sg) {
>>> +		if (size != scsi_bufflen(SCpnt)) {
>>>  			scmd_printk(KERN_ERR, SCpnt,
>>>  				"mismatch count %d, bytes %d\n",
>>> -				size, SCpnt->request_bufflen);
>>> -			if (SCpnt->request_bufflen > size)
>>> -				SCpnt->request_bufflen = size;
>>> +				size, scsi_bufflen(SCpnt));
>>> +			if (scsi_bufflen(SCpnt) > size)
>>> +				SCpnt->sdb.length = size;
>>>  		}
>>>  	}
>>>  
>>> @@ -392,12 +390,12 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
>>>  	 * request doesn't start on hw block boundary, add scatter pads
>>>  	 */
>>>  	if (((unsigned int)rq->sector % (s_size >> 9)) ||
>>> -	    (SCpnt->request_bufflen % s_size)) {
>>> +	    (scsi_bufflen(SCpnt) % s_size)) {
>>>  		scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n");
>>>  		goto out;
>>>  	}
>> Here we check I/O is "large-block" aligned. Both start and size
>>
>>>  
>>> -	this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9);
>>> +	this_count = (scsi_bufflen(SCpnt) >> 9) / (s_size >> 9);
>>>  
>> Number of "large-blocks"
>>
>>>  
>>>  	SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n",
>>> @@ -411,7 +409,7 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
>>>  
>>>  	if (this_count > 0xffff) {
>>>  		this_count = 0xffff;
>>> -		SCpnt->request_bufflen = this_count * s_size;
>>> +		SCpnt->sdb.length = this_count * s_size;
>>>  	}
>>>  
>> Here is my problem:
>> In the case that the transfer is bigger than 0xffff * s_size
>> (512/1024/2048) we modify ->request_bufflen. Now this has two bad
>> effects, the way I understand it, please fix me in my
>> misunderstanding.
>>
>> 1. Later in sr_done doing return good_bytes=cmd->request_bufflen will
>> only complete the cut-out bytes. Meaning possible BIO leak, since the
>> original request_bufflen was lost. (not all bytes are completed)
>>
>>
>> 2. What mechanics will re-send, or even knows, that not the complete
>> request was transfered? The way I understand it, a cmnd->resid must be
>> set, in this case.  Now the normal cmnd->resid is not enough because
>> it will be written by drivers, sr needs to stash a resid somewhere and
>> add it to cmnd->resid in sr_done(). But ...
>>
> 
> It's not lost, the request will be requeued in scsi_end_request(). The
> original state is in the request structure, and end_that_request_chunk()
> will return not-done when you complete this first part.
> 
OK, I see it now, thanks.

Should we adjust the q->max_hw_sectors anyway so elevator can divide the
work load more evenly? The way it is now, it's possible that we always do 
small leftovers at the end.

Boaz

[PATCH 3/3] SCSI: bidi support

[Boaz Harrosh]

  At the block level bidi request uses req->next_rq pointer for a second
  bidi_read request.
  At Scsi-midlayer a second scsi_data_buffer structure is used for the
  bidi_read part. This bidi scsi_data_buffer is put on
  request->next_rq->special. Struct scsi_cmnd is not changed.

  - Define scsi_bidi_cmnd() to return true if it is a bidi request and a
    second sgtable was allocated.

  - Define scsi_in()/scsi_out() to return the in or out scsi_data_buffer
    from this command This API is to isolate users from the mechanics of
    bidi.

  - Define scsi_end_bidi_request() to do what scsi_end_request() does but
    for a bidi request. This is necessary because bidi commands are a bit
    tricky here. (See comments in body)

  - scsi_release_buffers() will also release the bidi_read scsi_data_buffer

  - scsi_io_completion() on bidi commands will now call
    scsi_end_bidi_request() and return.

  - The previous work done in scsi_init_io() is now done in a new
    scsi_init_sgtable() (which is 99% identical to old scsi_init_io())
    The new scsi_init_io() will call the above twice if needed also for
    the bidi_read command. Only at this point is a command bidi.

  - In scsi_error.c at scsi_eh_prep/restore_cmnd() make sure bidi-lld is not
    confused by a get-sense command that looks like bidi. This is done
    by puting NULL at request->next_rq, and restoring.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_error.c |    3 +
 drivers/scsi/scsi_lib.c   |  154 ++++++++++++++++++++++++++++++++++++---------
 include/scsi/scsi_cmnd.h  |   23 +++++++-
 include/scsi/scsi_eh.h    |    1 +
 4 files changed, 149 insertions(+), 32 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index e7b87ea..51e2a2a 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -618,9 +618,11 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,
 	memcpy(ses->cmnd, scmd->cmnd, sizeof(scmd->cmnd));
 	ses->data_direction = scmd->sc_data_direction;
 	ses->sdb = scmd->sdb;
+	ses->next_rq = scmd->request->next_rq;
 	ses->result = scmd->result;
 
 	memset(&scmd->sdb, 0, sizeof(scmd->sdb));
+	scmd->request->next_rq = NULL;
 
 	if (sense_bytes) {
 		scmd->sdb.length = min_t(unsigned,
@@ -673,6 +675,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, struct scsi_eh_save *ses)
 	memcpy(scmd->cmnd, ses->cmnd, sizeof(scmd->cmnd));
 	scmd->sc_data_direction = ses->data_direction;
 	scmd->sdb = ses->sdb;
+	scmd->request->next_rq = ses->next_rq;
 	scmd->result = ses->result;
 }
 EXPORT_SYMBOL(scsi_eh_restore_cmnd);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f904692..85eda10 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -64,6 +64,8 @@ static struct scsi_host_sg_pool scsi_sg_pools[] = {
 };
 #undef SP
 
+static struct kmem_cache *scsi_bidi_sdb_cache;
+
 static void scsi_run_queue(struct request_queue *q);
 
 /*
@@ -625,6 +627,28 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
 		scsi_run_queue(sdev->request_queue);
 }
 
+static void scsi_finalize_request(struct scsi_cmnd *cmd, int uptodate)
+{
+	struct request_queue *q = cmd->device->request_queue;
+	struct request *req = cmd->request;
+	unsigned long flags;
+
+	add_disk_randomness(req->rq_disk);
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	if (blk_rq_tagged(req))
+		blk_queue_end_tag(q, req);
+
+	end_that_request_last(req, uptodate);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+
+	/*
+	 * This will goose the queue request function at the end, so we don't
+	 * need to worry about launching another command.
+	 */
+	scsi_next_command(cmd);
+}
+
 /*
  * Function:    scsi_end_request()
  *
@@ -652,7 +676,6 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int uptodate,
 {
 	struct request_queue *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
-	unsigned long flags;
 
 	/*
 	 * If there are blocks left over at the end, set up the command
@@ -681,19 +704,7 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int uptodate,
 		}
 	}
 
-	add_disk_randomness(req->rq_disk);
-
-	spin_lock_irqsave(q->queue_lock, flags);
-	if (blk_rq_tagged(req))
-		blk_queue_end_tag(q, req);
-	end_that_request_last(req, uptodate);
-	spin_unlock_irqrestore(q->queue_lock, flags);
-
-	/*
-	 * This will goose the queue request function at the end, so we don't
-	 * need to worry about launching another command.
-	 */
-	scsi_next_command(cmd);
+	scsi_finalize_request(cmd ,uptodate);
 	return NULL;
 }
 
@@ -892,10 +903,47 @@ void scsi_release_buffers(struct scsi_cmnd *cmd)
 		scsi_free_sgtable(&cmd->sdb);
 
 	memset(&cmd->sdb, 0, sizeof(cmd->sdb));
+
+	if (scsi_bidi_cmnd(cmd)) {
+		struct scsi_data_buffer *bidi_sdb = 
+			cmd->request->next_rq->special;
+		scsi_free_sgtable(bidi_sdb);
+		kmem_cache_free(scsi_bidi_sdb_cache, bidi_sdb);
+		cmd->request->next_rq->special = NULL;
+	}
 }
 EXPORT_SYMBOL(scsi_release_buffers);
 
 /*
+ * Bidi commands Must be complete as a whole, both sides at once.
+ * If part of the bytes were written and lld returned
+ * scsi_in()->resid and/or scsi_out()->resid this information will be left
+ * in req->data_len and req->next_rq->data_len. The upper-layer driver can
+ * decide what to do with this information.
+ */
+void scsi_end_bidi_request(struct scsi_cmnd *cmd)
+{
+	struct request *req = cmd->request;
+
+	end_that_request_chunk(req, 1, req->data_len);
+	req->data_len = scsi_out(cmd)->resid;
+
+	end_that_request_chunk(req->next_rq, 1, req->next_rq->data_len);
+	req->next_rq->data_len = scsi_in(cmd)->resid;
+
+	scsi_release_buffers(cmd);
+
+	/*
+	 *FIXME: If ll_rw_blk.c is changed to also put_request(req->next_rq)
+	 *       in end_that_request_last() then this WARN_ON must be removed.
+	 *       for now, upper-driver must have registered an end_io.
+	 */
+	WARN_ON(!req->end_io);
+
+	scsi_finalize_request(cmd, 1);
+}
+
+/*
  * Function:    scsi_io_completion()
  *
  * Purpose:     Completion processing for block device I/O requests.
@@ -956,9 +1004,15 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				req->sense_len = len;
 			}
 		}
+		if (scsi_bidi_cmnd(cmd)) {
+			/* will also release_buffers */
+			scsi_end_bidi_request(cmd);
+			return;
+		}
 		req->data_len = scsi_get_resid(cmd);
 	}
 
+	BUG_ON(blk_bidi_rq(req)); /* bidi not support for !blk_pc_request yet */
 	scsi_release_buffers(cmd);
 
 	/*
@@ -1084,25 +1138,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	scsi_end_request(cmd, 0, this_count, !result);
 }
 
-/*
- * Function:    scsi_init_io()
- *
- * Purpose:     SCSI I/O initialize function.
- *
- * Arguments:   cmd   - Command descriptor we wish to initialize
- *
- * Returns:     0 on success
- *		BLKPREP_DEFER if the failure is retryable
- *		BLKPREP_KILL if the failure is fatal
- */
-int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
+static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb,
+                             gfp_t gfp_mask)
 {
-	struct request     *req = cmd->request;
-	int		   count;
-	struct scsi_data_buffer *sdb = &cmd->sdb;
+	int count;
 
 	if (scsi_alloc_sgtable(sdb, req->nr_phys_segments, gfp_mask)) {
-		scsi_unprep_request(req);
 		return BLKPREP_DEFER;
 	}
 
@@ -1126,9 +1167,52 @@ int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 	printk(KERN_ERR "counted %d, received %d\n", count, sdb->sg_count);
 	printk(KERN_ERR "req nr_sec %lu, cur_nr_sec %u\n", req->nr_sectors,
 			req->current_nr_sectors);
-
 	return BLKPREP_KILL;
 }
+
+/*
+ * Function:    scsi_init_io()
+ *
+ * Purpose:     SCSI I/O initialize function.
+ *
+ * Arguments:   cmd   - Command descriptor we wish to initialize
+ *
+ * Returns:     0 on success
+ *		BLKPREP_DEFER if the failure is retryable
+ *		BLKPREP_KILL if the failure is fatal
+ */
+int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
+{
+	int error = scsi_init_sgtable(cmd->request ,&cmd->sdb, gfp_mask);
+	if (error)
+		goto err_exit;
+
+	if (blk_bidi_rq(cmd->request)) {
+		struct scsi_data_buffer *bidi_sdb = kmem_cache_zalloc(
+			scsi_bidi_sdb_cache, GFP_ATOMIC);
+		if (!bidi_sdb) {
+			error = BLKPREP_DEFER;
+			goto err_exit;
+		}
+
+		cmd->request->next_rq->special = bidi_sdb;
+		error = scsi_init_sgtable(cmd->request->next_rq ,bidi_sdb,
+		                                                    GFP_ATOMIC);
+		if (error)
+			goto err_exit;
+	}
+
+	return BLKPREP_OK ;
+
+err_exit:
+	scsi_release_buffers(cmd);
+	if (error == BLKPREP_KILL)
+		scsi_put_command(cmd);
+	else /* BLKPREP_DEFER */
+		scsi_unprep_request(cmd->request);
+
+	return error;
+}
 EXPORT_SYMBOL(scsi_init_io);
 
 static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
@@ -1738,6 +1822,14 @@ int __init scsi_init_queue(void)
 		return -ENOMEM;
 	}
 
+	scsi_bidi_sdb_cache = kmem_cache_create("scsi_bidi_sdb",
+					sizeof(struct scsi_data_buffer),
+					0, 0, NULL);
+	if (!scsi_bidi_sdb_cache) {
+		printk(KERN_ERR "SCSI: can't init scsi bidi sdb cache\n");
+		return -ENOMEM;
+	}
+
 	for (i = 0; i < SG_MEMPOOL_NR; i++) {
 		struct scsi_host_sg_pool *sgp = scsi_sg_pools + i;
 		int size = sgp->size * sizeof(struct scatterlist);
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 4e3cf43..f84d6b6 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -2,12 +2,12 @@
 #define _SCSI_SCSI_CMND_H
 
 #include <linux/dma-mapping.h>
+#include <linux/blkdev.h>
 #include <linux/list.h>
 #include <linux/types.h>
 #include <linux/timer.h>
 #include <linux/scatterlist.h>
 
-struct request;
 struct scatterlist;
 struct Scsi_Host;
 struct scsi_device;
@@ -162,4 +162,25 @@ static inline int scsi_get_resid(struct scsi_cmnd *cmd)
 #define scsi_for_each_sg(cmd, sg, nseg, __i)			\
 	for_each_sg(scsi_sglist(cmd), sg, nseg, __i)
 
+static inline int scsi_bidi_cmnd(struct scsi_cmnd *cmd)
+{
+	return blk_bidi_rq(cmd->request) &&
+	       (cmd->request->next_rq->special != NULL);
+}
+
+static inline struct scsi_data_buffer *scsi_in(struct scsi_cmnd *cmd)
+{
+	WARN_ON((cmd->sc_data_direction != DMA_FROM_DEVICE) &&
+	                                            !scsi_bidi_cmnd(cmd));
+	return scsi_bidi_cmnd(cmd) ?
+	       cmd->request->next_rq->special : &cmd->sdb;
+}
+
+static inline struct scsi_data_buffer *scsi_out(struct scsi_cmnd *cmd)
+{
+	WARN_ON((cmd->sc_data_direction != DMA_TO_DEVICE) &&
+	                                            !scsi_bidi_cmnd(cmd));
+	return &cmd->sdb;
+}
+
 #endif /* _SCSI_SCSI_CMND_H */
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index 1e08be1..25071d5 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -74,6 +74,7 @@ struct scsi_eh_save {
 	unsigned char cmd_len;
 	unsigned char cmnd[MAX_COMMAND_SIZE];
 	struct scsi_data_buffer sdb;
+	struct request *next_rq;
 
 	/* new command support */
 	struct scatterlist sense_sgl;
-- 
1.5.3.1

Re: [0/3] Last 3 patches for bidi support

[Mike Christie]

Boaz Harrosh wrote:
> [1]
> I propose a small change to scsi_tgt_lib.c that will make
> tgt completely neutral to the scsi_data_buffer patch. And will
> make it all that more ready for bidi, too. TOMO is this OK?
> 
> (Can you do without the GFP_KERNEL allocation flag? It could
> make the code a bit more simple)
> 

GFP_KERNEL is nice for the target layer because it can sleep in that 
path you changed and it and does not have the "cannot write out pages 
because it may come back to the same device issues" like an initiator does.

If we ever changed to a softirq instead of the work queue then we would 
not need the flag since it would have to GFP_ATOMIC, but I am not sure 
if we have plans to do that anytime soon.

Re: [0/3] Last 3 patches for bidi support

[Boaz Harrosh]

On Tue, Nov 06 2007 at 20:25 +0200, Mike Christie <michaelc@cs.wisc.edu> wrote:
> Boaz Harrosh wrote:
>> [1]
>> I propose a small change to scsi_tgt_lib.c that will make
>> tgt completely neutral to the scsi_data_buffer patch. And will
>> make it all that more ready for bidi, too. TOMO is this OK?
>>
>> (Can you do without the GFP_KERNEL allocation flag? It could
>> make the code a bit more simple)
>>
> 
> GFP_KERNEL is nice for the target layer because it can sleep in that 
> path you changed and it and does not have the "cannot write out pages 
> because it may come back to the same device issues" like an initiator does.
> 
> If we ever changed to a softirq instead of the work queue then we would 
> not need the flag since it would have to GFP_ATOMIC, but I am not sure 
> if we have plans to do that anytime soon.

Yes I understand that, hence the GFP_KERNEL was kept intact in my patch.
But I was thinking perhaps it was possible to sleep outside, if
the return value was BLKPREP_DEFER, the way the block layer sleeps,
just not in allocation stage.

Boaz

Re: [0/3] Last 3 patches for bidi support

[FUJITA Tomonori]

On Tue, 06 Nov 2007 20:38:33 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> On Tue, Nov 06 2007 at 20:25 +0200, Mike Christie <michaelc@cs.wisc.edu> wrote:
> > Boaz Harrosh wrote:
> >> [1]
> >> I propose a small change to scsi_tgt_lib.c that will make
> >> tgt completely neutral to the scsi_data_buffer patch. And will
> >> make it all that more ready for bidi, too. TOMO is this OK?
> >>
> >> (Can you do without the GFP_KERNEL allocation flag? It could
> >> make the code a bit more simple)
> >>
> > 
> > GFP_KERNEL is nice for the target layer because it can sleep in that 
> > path you changed and it and does not have the "cannot write out pages 
> > because it may come back to the same device issues" like an initiator does.
> > 
> > If we ever changed to a softirq instead of the work queue then we would 
> > not need the flag since it would have to GFP_ATOMIC, but I am not sure 
> > if we have plans to do that anytime soon.
> 
> Yes I understand that, hence the GFP_KERNEL was kept intact in my patch.
> But I was thinking perhaps it was possible to sleep outside, if
> the return value was BLKPREP_DEFER, the way the block layer sleeps,
> just not in allocation stage.

The block layer is designed to handle that case but target drivers are
not.

Re: [0/4 ver2] Last 3 patches for bidi support

[Boaz Harrosh]

On Tue, Nov 06 2007 at 20:04 +0200, Boaz Harrosh <bharrosh@panasas.com> wrote:
> [1]
> I propose a small change to scsi_tgt_lib.c that will make
> tgt completely neutral to the scsi_data_buffer patch. And will
> make it all that more ready for bidi, too. TOMO is this OK?
> 
> (Can you do without the GFP_KERNEL allocation flag? It could
> make the code a bit more simple)
> 
> [2]
> scsi_data_buffer patch. 
> As requested by TOMO the all patch is squashed into one, 600 
> and some lines. TOMO if it makes you happy, OK, here it is.
> 
> There is one hunk from libsrp.c that I really hate. and from what 
> I understand of the code, only the "request_bufflen =" is really used, 
> All users of "request_buffer = addr" pass NULL, as far as I can see.
> If you would like to make me happy, and it is at all possible, please 
> clean it.
> 
> [3]
> Once scsi_data_buffer is in, then scsi bidi support can be applied. 
> (Block bidi was already merged 3 kernels ago). It makes no changes
> to scsi_cmnd and only adds some, not-at-all-dangerous, code to scsi_lib.
> So I don't see any reason to wait. please all review.
> 
> If ACKed by all parties and applied for inclusion for 2.6.25, 
> then they will have a long time to sit in MM and collect
> compilation breakage reports from ARCHs we never compiled.
> 
> I wish these make everybody happy this time
> 
> Boaz Harrosh
> 

Version 2, fixed as of Tomo's comments. Thanks Tomo.

[0]
   Remove dead code from sd.c & sr.c. It was conceived by programmers
   in the passed that rq_data_dir() could perhaps be expanded to 2 bits
   and return READ/WRITE/BIDI. But we have decided long ago that a bidi
   request is when blk_bidi_rq() == true, and in that case 
   rq_data_dir() == WRITE. So now we can be sure of what the compiler knew
   for a long time. (The future is already here)

[1] [2] [3]
See above

I must insist again they should have time to sit in -mm. 
For posible compilation breakage of other ARCHs.

Boaz

[PATCH 1/4] sr/sd: Remove dead code

[Boaz Harrosh]

  if (rq_data_dir() == WRITE) else if() else chain had an extra
  "else" since the if() is on a value of 1 bit.

  Also with a bidi request, rq_data_dir() == WRITE
  and blk_bidi_rq() == true.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/sd.c |    5 +----
 drivers/scsi/sr.c |    5 +----
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 18343a6..f1ec2bd 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -445,12 +445,9 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 		}
 		SCpnt->cmnd[0] = WRITE_6;
 		SCpnt->sc_data_direction = DMA_TO_DEVICE;
-	} else if (rq_data_dir(rq) == READ) {
+	} else {
 		SCpnt->cmnd[0] = READ_6;
 		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-	} else {
-		scmd_printk(KERN_ERR, SCpnt, "Unknown command %x\n", rq->cmd_flags);
-		goto out;
 	}
 
 	SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 7702681..a72ae85 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -365,12 +365,9 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 		SCpnt->cmnd[0] = WRITE_10;
 		SCpnt->sc_data_direction = DMA_TO_DEVICE;
  	 	cd->cdi.media_written = 1;
-	} else if (rq_data_dir(rq) == READ) {
+	} else {
 		SCpnt->cmnd[0] = READ_10;
 		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-	} else {
-		blk_dump_rq_flags(rq, "Unknown sr command");
-		goto out;
 	}
 
 	{
-- 
1.5.3.1

[PATCH 2/4] tgt: Use scsi_init_io instead of scsi_alloc_sgtable

[Boaz Harrosh]

  - If we export scsi_init_io()/scsi_release_buffers() instead of
    scsi_{alloc,free}_sgtable() from scsi_lib than tgt code is
    much more insulated from scsi_lib changes. As a bonus it will
    also gain bidi capability when it comes.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
Acked-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

---
 drivers/scsi/scsi_lib.c     |   21 ++++++++++-----------
 drivers/scsi/scsi_tgt_lib.c |   34 +++++-----------------------------
 include/scsi/scsi_cmnd.h    |    4 ++--
 3 files changed, 17 insertions(+), 42 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 0e81e4c..6d8ea69 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -737,7 +737,8 @@ static inline unsigned int scsi_sgtable_index(unsigned short nents)
 	return index;
 }
 
-struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
+static struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd,
+								gfp_t gfp_mask)
 {
 	struct scsi_host_sg_pool *sgp;
 	struct scatterlist *sgl, *prev, *ret;
@@ -823,9 +824,7 @@ enomem:
 	return NULL;
 }
 
-EXPORT_SYMBOL(scsi_alloc_sgtable);
-
-void scsi_free_sgtable(struct scsi_cmnd *cmd)
+static void scsi_free_sgtable(struct scsi_cmnd *cmd)
 {
 	struct scatterlist *sgl = cmd->request_buffer;
 	struct scsi_host_sg_pool *sgp;
@@ -871,8 +870,6 @@ void scsi_free_sgtable(struct scsi_cmnd *cmd)
 	mempool_free(sgl, sgp->pool);
 }
 
-EXPORT_SYMBOL(scsi_free_sgtable);
-
 /*
  * Function:    scsi_release_buffers()
  *
@@ -890,7 +887,7 @@ EXPORT_SYMBOL(scsi_free_sgtable);
  *		the scatter-gather table, and potentially any bounce
  *		buffers.
  */
-static void scsi_release_buffers(struct scsi_cmnd *cmd)
+void scsi_release_buffers(struct scsi_cmnd *cmd)
 {
 	if (cmd->use_sg)
 		scsi_free_sgtable(cmd);
@@ -902,6 +899,7 @@ static void scsi_release_buffers(struct scsi_cmnd *cmd)
 	cmd->request_buffer = NULL;
 	cmd->request_bufflen = 0;
 }
+EXPORT_SYMBOL(scsi_release_buffers);
 
 /*
  * Function:    scsi_io_completion()
@@ -1104,7 +1102,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
  *		BLKPREP_DEFER if the failure is retryable
  *		BLKPREP_KILL if the failure is fatal
  */
-static int scsi_init_io(struct scsi_cmnd *cmd)
+int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 {
 	struct request     *req = cmd->request;
 	int		   count;
@@ -1119,7 +1117,7 @@ static int scsi_init_io(struct scsi_cmnd *cmd)
 	/*
 	 * If sg table allocation fails, requeue request later.
 	 */
-	cmd->request_buffer = scsi_alloc_sgtable(cmd, GFP_ATOMIC);
+	cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
 	if (unlikely(!cmd->request_buffer)) {
 		scsi_unprep_request(req);
 		return BLKPREP_DEFER;
@@ -1148,6 +1146,7 @@ static int scsi_init_io(struct scsi_cmnd *cmd)
 
 	return BLKPREP_KILL;
 }
+EXPORT_SYMBOL(scsi_init_io);
 
 static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
 		struct request *req)
@@ -1193,7 +1192,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
 
 		BUG_ON(!req->nr_phys_segments);
 
-		ret = scsi_init_io(cmd);
+		ret = scsi_init_io(cmd, GFP_ATOMIC);
 		if (unlikely(ret))
 			return ret;
 	} else {
@@ -1244,7 +1243,7 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
 	if (unlikely(!cmd))
 		return BLKPREP_DEFER;
 
-	return scsi_init_io(cmd);
+	return scsi_init_io(cmd, GFP_ATOMIC);
 }
 EXPORT_SYMBOL(scsi_setup_fs_cmnd);
 
diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
index deea3cd..5fd5fca 100644
--- a/drivers/scsi/scsi_tgt_lib.c
+++ b/drivers/scsi/scsi_tgt_lib.c
@@ -331,8 +331,7 @@ static void scsi_tgt_cmd_done(struct scsi_cmnd *cmd)
 
 	scsi_tgt_uspace_send_status(cmd, tcmd->itn_id, tcmd->tag);
 
-	if (scsi_sglist(cmd))
-		scsi_free_sgtable(cmd);
+	scsi_release_buffers(cmd);
 
 	queue_work(scsi_tgtd, &tcmd->work);
 }
@@ -353,31 +352,6 @@ static int scsi_tgt_transfer_response(struct scsi_cmnd *cmd)
 	return 0;
 }
 
-static int scsi_tgt_init_cmd(struct scsi_cmnd *cmd, gfp_t gfp_mask)
-{
-	struct request *rq = cmd->request;
-	int count;
-
-	cmd->use_sg = rq->nr_phys_segments;
-	cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
-	if (!cmd->request_buffer)
-		return -ENOMEM;
-
-	cmd->request_bufflen = rq->data_len;
-
-	dprintk("cmd %p cnt %d %lu\n", cmd, scsi_sg_count(cmd),
-		rq_data_dir(rq));
-	count = blk_rq_map_sg(rq->q, rq, scsi_sglist(cmd));
-	if (likely(count <= scsi_sg_count(cmd))) {
-		cmd->use_sg = count;
-		return 0;
-	}
-
-	eprintk("cmd %p cnt %d\n", cmd, scsi_sg_count(cmd));
-	scsi_free_sgtable(cmd);
-	return -EINVAL;
-}
-
 /* TODO: test this crap and replace bio_map_user with new interface maybe */
 static int scsi_map_user_pages(struct scsi_tgt_cmd *tcmd, struct scsi_cmnd *cmd,
 			       unsigned long uaddr, unsigned int len, int rw)
@@ -403,9 +377,11 @@ static int scsi_map_user_pages(struct scsi_tgt_cmd *tcmd, struct scsi_cmnd *cmd,
 	}
 
 	tcmd->bio = rq->bio;
-	err = scsi_tgt_init_cmd(cmd, GFP_KERNEL);
-	if (err)
+	err = scsi_init_io(cmd, GFP_KERNEL);
+	if (err) {
+		scsi_release_buffers(cmd);
 		goto unmap_rq;
+	}
 
 	return 0;
 
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 3f47e52..a7be605 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -128,8 +128,8 @@ extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
 				 size_t *offset, size_t *len);
 extern void scsi_kunmap_atomic_sg(void *virt);
 
-extern struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t);
-extern void scsi_free_sgtable(struct scsi_cmnd *);
+extern int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask);
+extern void scsi_release_buffers(struct scsi_cmnd *cmd);
 
 extern int scsi_dma_map(struct scsi_cmnd *cmd);
 extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
-- 
1.5.3.1

[PATCH 3/4] scsi_data_buffer

[Boaz Harrosh]

  In preparation for bidi we abstract all IO members of scsi_cmnd,
  that will need to duplicate, into a substructure.

  - Group all IO members of scsi_cmnd into a scsi_data_buffer
    structure.
  - Adjust accessors to new members.
  - scsi_{alloc,free}_sgtable receive a scsi_data_buffer instead of
    scsi_cmnd. And work on it.
  - Adjust scsi_init_io() and  scsi_release_buffers() for above
    change.
  - Fix other parts of scsi_lib/scsi.c to members migration. Use
    accessors where appropriate.

  - fix Documentation about scsi_cmnd in scsi_host.h

  - scsi_error.c
    * Changed needed members of struct scsi_eh_save.
    * Careful considerations in scsi_eh_prep/restore_cmnd.

  - sd.c and sr.c
    * sd and sr would adjust IO size to align on device's block
      size so code needs to change once we move to scsi_data_buff
      implementation.
    * Convert code to use scsi_for_each_sg
    * Use data accessors where appropriate.

  - tgt: convert libsrp to use scsi_data_buffer
  - isd200: This driver still bangs on scsi_cmnd IO members,
    so need changing

 Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
 Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/scsi/libsrp.c        |    4 +-
 drivers/scsi/scsi.c          |    2 +-
 drivers/scsi/scsi_error.c    |   28 +++++---------
 drivers/scsi/scsi_lib.c      |   79 ++++++++++++++++--------------------------
 drivers/scsi/sd.c            |    4 +-
 drivers/scsi/sr.c            |   25 +++++++------
 drivers/usb/storage/isd200.c |    8 ++--
 include/scsi/scsi_cmnd.h     |   39 +++++++++++++-------
 include/scsi/scsi_eh.h       |    8 ++---
 include/scsi/scsi_host.h     |    4 +-
 10 files changed, 92 insertions(+), 109 deletions(-)

diff --git a/drivers/scsi/libsrp.c b/drivers/scsi/libsrp.c
index 5cff020..8a8562a 100644
--- a/drivers/scsi/libsrp.c
+++ b/drivers/scsi/libsrp.c
@@ -426,8 +426,8 @@ int srp_cmd_queue(struct Scsi_Host *shost, struct srp_cmd *cmd, void *info,
 
 	sc->SCp.ptr = info;
 	memcpy(sc->cmnd, cmd->cdb, MAX_COMMAND_SIZE);
-	sc->request_bufflen = len;
-	sc->request_buffer = (void *) (unsigned long) addr;
+	sc->sdb.length = len;
+	sc->sdb.sglist = (void *) (unsigned long) addr;
 	sc->tag = tag;
 	err = scsi_tgt_queue_command(sc, itn_id, (struct scsi_lun *)&cmd->lun,
 				     cmd->tag);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 1929488..73d2216 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -698,7 +698,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
 				"Notifying upper driver of completion "
 				"(result %x)\n", cmd->result));
 
-	good_bytes = cmd->request_bufflen;
+	good_bytes = scsi_bufflen(cmd);
         if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
 		drv = scsi_cmd_to_driver(cmd);
 		if (drv->done)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index ebaca4c..9daa983 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -617,29 +617,25 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,
 	ses->cmd_len = scmd->cmd_len;
 	memcpy(ses->cmnd, scmd->cmnd, sizeof(scmd->cmnd));
 	ses->data_direction = scmd->sc_data_direction;
-	ses->bufflen = scmd->request_bufflen;
-	ses->buffer = scmd->request_buffer;
-	ses->use_sg = scmd->use_sg;
-	ses->resid = scmd->resid;
+	ses->sdb = scmd->sdb;
 	ses->result = scmd->result;
 
+	memset(&scmd->sdb, 0, sizeof(scmd->sdb));
+
 	if (sense_bytes) {
-		scmd->request_bufflen = min_t(unsigned,
+		scmd->sdb.length = min_t(unsigned,
 		                       sizeof(scmd->sense_buffer), sense_bytes);
 		sg_init_one(&ses->sense_sgl, scmd->sense_buffer,
-		                                       scmd->request_bufflen);
-		scmd->request_buffer = &ses->sense_sgl;
+							      scmd->sdb.length);
+		scmd->sdb.sglist = &ses->sense_sgl;
 		scmd->sc_data_direction = DMA_FROM_DEVICE;
-		scmd->use_sg = 1;
+		scmd->sdb.sg_count = 1;
 		memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
 		scmd->cmnd[0] = REQUEST_SENSE;
-		scmd->cmnd[4] = scmd->request_bufflen;
+		scmd->cmnd[4] = scmd->sdb.length;
 		scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
 	} else {
-		scmd->request_buffer = NULL;
-		scmd->request_bufflen = 0;
 		scmd->sc_data_direction = DMA_NONE;
-		scmd->use_sg = 0;
 		if (cmnd) {
 			memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
 			memcpy(scmd->cmnd, cmnd, cmnd_size);
@@ -676,10 +672,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, struct scsi_eh_save *ses)
 	scmd->cmd_len = ses->cmd_len;
 	memcpy(scmd->cmnd, ses->cmnd, sizeof(scmd->cmnd));
 	scmd->sc_data_direction = ses->data_direction;
-	scmd->request_bufflen = ses->bufflen;
-	scmd->request_buffer = ses->buffer;
-	scmd->use_sg = ses->use_sg;
-	scmd->resid = ses->resid;
+	scmd->sdb = ses->sdb;
 	scmd->result = ses->result;
 }
 EXPORT_SYMBOL(scsi_eh_restore_cmnd);
@@ -1699,8 +1692,7 @@ scsi_reset_provider(struct scsi_device *dev, int flag)
 	memset(&scmd->cmnd, '\0', sizeof(scmd->cmnd));
     
 	scmd->scsi_done		= scsi_reset_provider_done_command;
-	scmd->request_buffer		= NULL;
-	scmd->request_bufflen		= 0;
+	memset(&scmd->sdb, 0, sizeof(scmd->sdb));
 
 	scmd->cmd_len			= 0;
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 6d8ea69..edfd8d8 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -438,7 +438,7 @@ EXPORT_SYMBOL_GPL(scsi_execute_async);
 static void scsi_init_cmd_errh(struct scsi_cmnd *cmd)
 {
 	cmd->serial_number = 0;
-	cmd->resid = 0;
+	scsi_set_resid(cmd, 0);
 	memset(cmd->sense_buffer, 0, sizeof cmd->sense_buffer);
 	if (cmd->cmd_len == 0)
 		cmd->cmd_len = COMMAND_SIZE(cmd->cmnd[0]);
@@ -737,17 +737,15 @@ static inline unsigned int scsi_sgtable_index(unsigned short nents)
 	return index;
 }
 
-static struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd,
-								gfp_t gfp_mask)
+static int scsi_alloc_sgtable(struct scsi_data_buffer *sdb,
+					unsigned short sg_count, gfp_t gfp_mask)
 {
 	struct scsi_host_sg_pool *sgp;
 	struct scatterlist *sgl, *prev, *ret;
 	unsigned int index;
 	int this, left;
 
-	BUG_ON(!cmd->use_sg);
-
-	left = cmd->use_sg;
+	left = sg_count;
 	ret = prev = NULL;
 	do {
 		this = left;
@@ -801,8 +799,9 @@ static struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd,
 	 * ->use_sg may get modified after dma mapping has potentially
 	 * shrunk the number of segments, so keep a copy of it for free.
 	 */
-	cmd->__use_sg = cmd->use_sg;
-	return ret;
+	sdb->alloc_sg_count = sdb->sg_count = sg_count;
+	sdb->sglist = ret;
+	return 0;
 enomem:
 	if (ret) {
 		/*
@@ -821,24 +820,24 @@ enomem:
 
 		mempool_free(prev, sgp->pool);
 	}
-	return NULL;
+	return -ENOMEM;
 }
 
-static void scsi_free_sgtable(struct scsi_cmnd *cmd)
+static void scsi_free_sgtable(struct scsi_data_buffer *sdb)
 {
-	struct scatterlist *sgl = cmd->request_buffer;
+	struct scatterlist *sgl = sdb->sglist;
 	struct scsi_host_sg_pool *sgp;
 
 	/*
 	 * if this is the biggest size sglist, check if we have
 	 * chained parts we need to free
 	 */
-	if (cmd->__use_sg > SCSI_MAX_SG_SEGMENTS) {
+	if (sdb->alloc_sg_count > SCSI_MAX_SG_SEGMENTS) {
 		unsigned short this, left;
 		struct scatterlist *next;
 		unsigned int index;
 
-		left = cmd->__use_sg - (SCSI_MAX_SG_SEGMENTS - 1);
+		left = sdb->alloc_sg_count - (SCSI_MAX_SG_SEGMENTS - 1);
 		next = sg_chain_ptr(&sgl[SCSI_MAX_SG_SEGMENTS - 1]);
 		while (left && next) {
 			sgl = next;
@@ -862,10 +861,10 @@ static void scsi_free_sgtable(struct scsi_cmnd *cmd)
 		/*
 		 * Restore original, will be freed below
 		 */
-		sgl = cmd->request_buffer;
+		sgl = sdb->sglist;
 		sgp = scsi_sg_pools + SG_MEMPOOL_NR - 1;
 	} else
-		sgp = scsi_sg_pools + scsi_sgtable_index(cmd->__use_sg);
+		sgp = scsi_sg_pools + scsi_sgtable_index(sdb->alloc_sg_count);
 
 	mempool_free(sgl, sgp->pool);
 }
@@ -889,15 +888,10 @@ static void scsi_free_sgtable(struct scsi_cmnd *cmd)
  */
 void scsi_release_buffers(struct scsi_cmnd *cmd)
 {
-	if (cmd->use_sg)
-		scsi_free_sgtable(cmd);
+	if (cmd->sdb.sglist)
+		scsi_free_sgtable(&cmd->sdb);
 
-	/*
-	 * Zero these out.  They now point to freed memory, and it is
-	 * dangerous to hang onto the pointers.
-	 */
-	cmd->request_buffer = NULL;
-	cmd->request_bufflen = 0;
+	memset(&cmd->sdb, 0, sizeof(cmd->sdb));
 }
 EXPORT_SYMBOL(scsi_release_buffers);
 
@@ -932,7 +926,7 @@ EXPORT_SYMBOL(scsi_release_buffers);
 void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 {
 	int result = cmd->result;
-	int this_count = cmd->request_bufflen;
+	int this_count = scsi_bufflen(cmd);
 	struct request_queue *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
 	int clear_errors = 1;
@@ -940,8 +934,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	int sense_valid = 0;
 	int sense_deferred = 0;
 
-	scsi_release_buffers(cmd);
-
 	if (result) {
 		sense_valid = scsi_command_normalize_sense(cmd, &sshdr);
 		if (sense_valid)
@@ -964,9 +956,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				req->sense_len = len;
 			}
 		}
-		req->data_len = cmd->resid;
+		req->data_len = scsi_get_resid(cmd);
 	}
 
+	scsi_release_buffers(cmd);
+
 	/*
 	 * Next deal with any sectors which we were able to correctly
 	 * handle.
@@ -974,7 +968,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	SCSI_LOG_HLCOMPLETE(1, printk("%ld sectors total, "
 				      "%d bytes done.\n",
 				      req->nr_sectors, good_bytes));
-	SCSI_LOG_HLCOMPLETE(1, printk("use_sg is %d\n", cmd->use_sg));
 
 	if (clear_errors)
 		req->errors = 0;
@@ -1106,41 +1099,31 @@ int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 {
 	struct request     *req = cmd->request;
 	int		   count;
+	struct scsi_data_buffer *sdb = &cmd->sdb;
 
-	/*
-	 * We used to not use scatter-gather for single segment request,
-	 * but now we do (it makes highmem I/O easier to support without
-	 * kmapping pages)
-	 */
-	cmd->use_sg = req->nr_phys_segments;
-
-	/*
-	 * If sg table allocation fails, requeue request later.
-	 */
-	cmd->request_buffer = scsi_alloc_sgtable(cmd, gfp_mask);
-	if (unlikely(!cmd->request_buffer)) {
+	if (scsi_alloc_sgtable(sdb, req->nr_phys_segments, gfp_mask)) {
 		scsi_unprep_request(req);
 		return BLKPREP_DEFER;
 	}
 
 	req->buffer = NULL;
 	if (blk_pc_request(req))
-		cmd->request_bufflen = req->data_len;
+		sdb->length = req->data_len;
 	else
-		cmd->request_bufflen = req->nr_sectors << 9;
+		sdb->length = req->nr_sectors << 9;
 
 	/* 
 	 * Next, walk the list, and fill in the addresses and sizes of
 	 * each segment.
 	 */
-	count = blk_rq_map_sg(req->q, req, cmd->request_buffer);
-	if (likely(count <= cmd->use_sg)) {
-		cmd->use_sg = count;
+	count = blk_rq_map_sg(req->q, req, sdb->sglist);
+	if (likely(count <= sdb->sg_count)) {
+		sdb->sg_count = count;
 		return BLKPREP_OK;
 	}
 
 	printk(KERN_ERR "Incorrect number of segments after building list\n");
-	printk(KERN_ERR "counted %d, received %d\n", count, cmd->use_sg);
+	printk(KERN_ERR "counted %d, received %d\n", count, sdb->sg_count);
 	printk(KERN_ERR "req nr_sec %lu, cur_nr_sec %u\n", req->nr_sectors,
 			req->current_nr_sectors);
 
@@ -1199,9 +1182,7 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
 		BUG_ON(req->data_len);
 		BUG_ON(req->data);
 
-		cmd->request_bufflen = 0;
-		cmd->request_buffer = NULL;
-		cmd->use_sg = 0;
+		memset(&cmd->sdb, 0, sizeof(cmd->sdb));
 		req->buffer = NULL;
 	}
 
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index f1ec2bd..d97621c 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -507,7 +507,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 		SCpnt->cmnd[4] = (unsigned char) this_count;
 		SCpnt->cmnd[5] = 0;
 	}
-	SCpnt->request_bufflen = this_count * sdp->sector_size;
+	SCpnt->sdb.length = this_count * sdp->sector_size;
 
 	/*
 	 * We shouldn't disconnect in the middle of a sector, so with a dumb
@@ -907,7 +907,7 @@ static struct block_device_operations sd_fops = {
 static int sd_done(struct scsi_cmnd *SCpnt)
 {
 	int result = SCpnt->result;
- 	unsigned int xfer_size = SCpnt->request_bufflen;
+	unsigned int xfer_size = scsi_bufflen(SCpnt);
  	unsigned int good_bytes = result ? 0 : xfer_size;
  	u64 start_lba = SCpnt->request->sector;
  	u64 bad_lba;
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index a72ae85..f26389a 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -226,7 +226,7 @@ out:
 static int sr_done(struct scsi_cmnd *SCpnt)
 {
 	int result = SCpnt->result;
-	int this_count = SCpnt->request_bufflen;
+	int this_count = scsi_bufflen(SCpnt);
 	int good_bytes = (result == 0 ? this_count : 0);
 	int block_sectors = 0;
 	long error_sector;
@@ -371,17 +371,18 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 	}
 
 	{
-		struct scatterlist *sg = SCpnt->request_buffer;
-		int i, size = 0;
-		for (i = 0; i < SCpnt->use_sg; i++)
-			size += sg[i].length;
+		struct scatterlist *sg;
+		int i, size = 0, sg_count = scsi_sg_count(SCpnt);
 
-		if (size != SCpnt->request_bufflen && SCpnt->use_sg) {
+		scsi_for_each_sg(SCpnt, sg, sg_count, i)
+			size += sg->length;
+
+		if (size != scsi_bufflen(SCpnt)) {
 			scmd_printk(KERN_ERR, SCpnt,
 				"mismatch count %d, bytes %d\n",
-				size, SCpnt->request_bufflen);
-			if (SCpnt->request_bufflen > size)
-				SCpnt->request_bufflen = size;
+				size, scsi_bufflen(SCpnt));
+			if (scsi_bufflen(SCpnt) > size)
+				SCpnt->sdb.length = size;
 		}
 	}
 
@@ -389,12 +390,12 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 	 * request doesn't start on hw block boundary, add scatter pads
 	 */
 	if (((unsigned int)rq->sector % (s_size >> 9)) ||
-	    (SCpnt->request_bufflen % s_size)) {
+	    (scsi_bufflen(SCpnt) % s_size)) {
 		scmd_printk(KERN_NOTICE, SCpnt, "unaligned transfer\n");
 		goto out;
 	}
 
-	this_count = (SCpnt->request_bufflen >> 9) / (s_size >> 9);
+	this_count = (scsi_bufflen(SCpnt) >> 9) / (s_size >> 9);
 
 
 	SCSI_LOG_HLQUEUE(2, printk("%s : %s %d/%ld 512 byte blocks.\n",
@@ -408,7 +409,7 @@ static int sr_prep_fn(struct request_queue *q, struct request *rq)
 
 	if (this_count > 0xffff) {
 		this_count = 0xffff;
-		SCpnt->request_bufflen = this_count * s_size;
+		SCpnt->sdb.length = this_count * s_size;
 	}
 
 	SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index 178e8c2..2d9a32b 100644
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
@@ -415,14 +415,14 @@ static void isd200_set_srb(struct isd200_info *info,
 		sg_init_one(&info->sg, buff, bufflen);
 
 	srb->sc_data_direction = dir;
-	srb->request_buffer = buff ? &info->sg : NULL;
-	srb->request_bufflen = bufflen;
-	srb->use_sg = buff ? 1 : 0;
+	srb->sdb.sglist = buff ? &info->sg : NULL;
+	srb->sdb.length = bufflen;
+	srb->sdb.sg_count = buff ? 1 : 0;
 }
 
 static void isd200_srb_set_bufflen(struct scsi_cmnd *srb, unsigned bufflen)
 {
-	srb->request_bufflen = bufflen;
+	srb->sdb.length = bufflen;
 }
 
 
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index a7be605..887a12a 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -12,6 +12,13 @@ struct scatterlist;
 struct Scsi_Host;
 struct scsi_device;
 
+struct scsi_data_buffer {
+	struct scatterlist *sglist;
+	unsigned length;
+	int resid;
+	unsigned short sg_count;
+	unsigned short alloc_sg_count; /* private to scsi_lib */
+};
 
 /* embedded in scsi_cmnd */
 struct scsi_pointer {
@@ -62,15 +69,10 @@ struct scsi_cmnd {
 	/* These elements define the operation we are about to perform */
 #define MAX_COMMAND_SIZE	16
 	unsigned char cmnd[MAX_COMMAND_SIZE];
-	unsigned request_bufflen;	/* Actual request size */
 
 	struct timer_list eh_timeout;	/* Used to time out the command. */
-	void *request_buffer;		/* Actual requested buffer */
 
 	/* These elements define the operation we ultimately want to perform */
-	unsigned short use_sg;	/* Number of pieces of scatter-gather */
-	unsigned short __use_sg;
-
 	unsigned underflow;	/* Return error if less than
 				   this amount is transferred */
 
@@ -80,10 +82,6 @@ struct scsi_cmnd {
 				   reconnects.   Probably == sector
 				   size */
 
-	int resid;		/* Number of bytes requested to be
-				   transferred less actual number
-				   transferred (0 if not supported) */
-
 	struct request *request;	/* The command we are
 				   	   working on */
 
@@ -114,6 +112,8 @@ struct scsi_cmnd {
 	int result;		/* Status code from lower level driver */
 
 	unsigned char tag;	/* SCSI-II queued command tag */
+
+	struct scsi_data_buffer sdb;
 };
 
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
@@ -134,18 +134,29 @@ extern void scsi_release_buffers(struct scsi_cmnd *cmd);
 extern int scsi_dma_map(struct scsi_cmnd *cmd);
 extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
 
-#define scsi_sg_count(cmd) ((cmd)->use_sg)
-#define scsi_sglist(cmd) ((struct scatterlist *)(cmd)->request_buffer)
-#define scsi_bufflen(cmd) ((cmd)->request_bufflen)
+static inline unsigned scsi_sg_count(struct scsi_cmnd *cmd)
+{
+	return cmd->sdb.sg_count;
+}
+
+static inline struct scatterlist *scsi_sglist(struct scsi_cmnd *cmd)
+{
+	return cmd->sdb.sglist;
+}
+
+static inline unsigned scsi_bufflen(struct scsi_cmnd *cmd)
+{
+	return cmd->sdb.length;
+}
 
 static inline void scsi_set_resid(struct scsi_cmnd *cmd, int resid)
 {
-	cmd->resid = resid;
+	cmd->sdb.resid = resid;
 }
 
 static inline int scsi_get_resid(struct scsi_cmnd *cmd)
 {
-	return cmd->resid;
+	return cmd->sdb.resid;
 }
 
 #define scsi_for_each_sg(cmd, sg, nseg, __i)			\
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index d21b891..1e08be1 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -68,16 +68,14 @@ extern int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
 extern int scsi_reset_provider(struct scsi_device *, int);
 
 struct scsi_eh_save {
+	/* saved state */
 	int result;
 	enum dma_data_direction data_direction;
 	unsigned char cmd_len;
 	unsigned char cmnd[MAX_COMMAND_SIZE];
+	struct scsi_data_buffer sdb;
 
-	void *buffer;
-	unsigned bufflen;
-	unsigned short use_sg;
-	int resid;
-
+	/* new command support */
 	struct scatterlist sense_sgl;
 };
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 0fd4746..cb2bcab 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -136,9 +136,9 @@ struct scsi_host_template {
 	 * the done callback is invoked.
 	 *
 	 * This is called to inform the LLD to transfer
-	 * cmd->request_bufflen bytes. The cmd->use_sg speciefies the
+	 * scsi_bufflen(cmd) bytes. scsi_sg_count(cmd) speciefies the
 	 * number of scatterlist entried in the command and
-	 * cmd->request_buffer contains the scatterlist.
+	 * scsi_sglist(cmd) returns the scatterlist.
 	 *
 	 * return values: see queuecommand
 	 *
-- 
1.5.3.1

Re: [PATCH 3/4] scsi_data_buffer

[Andrew Morton]

On Thu, 08 Nov 2007 18:59:30 +0200 Boaz Harrosh <bharrosh@panasas.com> wrote:

>   In preparation for bidi we abstract all IO members of scsi_cmnd,
>   that will need to duplicate, into a substructure.
> 
>   - Group all IO members of scsi_cmnd into a scsi_data_buffer
>     structure.

drivers/scsi/qla1280.c: In function 'qla1280_done':
drivers/scsi/qla1280.c:1313: error: 'struct scsi_cmnd' has no member named 'use_sg'
drivers/scsi/qla1280.c:1314: error: 'struct scsi_cmnd' has no member named 'request_buffer'
drivers/scsi/qla1280.c:1315: error: 'struct scsi_cmnd' has no member named 'use_sg'
drivers/scsi/qla1280.c:1316: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
drivers/scsi/qla1280.c:1318: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
drivers/scsi/qla1280.c: In function 'qla1280_return_status':
drivers/scsi/qla1280.c:1409: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
drivers/scsi/qla1280.c:1416: error: 'struct scsi_cmnd' has no member named 'resid'
drivers/scsi/qla1280.c: In function 'qla1280_64bit_start_scsi':
drivers/scsi/qla1280.c:2791: error: 'struct scsi_cmnd' has no member named 'use_sg'
drivers/scsi/qla1280.c:2792: error: 'struct scsi_cmnd' has no member named 'request_buffer'
drivers/scsi/qla1280.c:2793: error: 'struct scsi_cmnd' has no member named 'use_sg'
drivers/scsi/qla1280.c:2801: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
drivers/scsi/qla1280.c:2896: error: 'struct scsi_cmnd' has no member named 'use_sg'
drivers/scsi/qla1280.c:2991: error: 'struct scsi_cmnd' has no member named 'request_buffer'
drivers/scsi/qla1280.c:2992: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
drivers/scsi/qla1280.c:3004: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
make[2]: *** [drivers/scsi/qla1280.o] Error 1

It mystfies me how a patch like this can have been floating about in N
submissions across M months and nobody has done an allmodconfig build or
even a grep to find out what broke.

ho hum.  I shall mark qla1280 BROKEN and shall plod onwards.

Re: [PATCH 3/4] scsi_data_buffer

[FUJITA Tomonori]

On Mon, 12 Nov 2007 22:06:52 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Thu, 08 Nov 2007 18:59:30 +0200 Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
> >   In preparation for bidi we abstract all IO members of scsi_cmnd,
> >   that will need to duplicate, into a substructure.
> > 
> >   - Group all IO members of scsi_cmnd into a scsi_data_buffer
> >     structure.
> 
> drivers/scsi/qla1280.c: In function 'qla1280_done':
> drivers/scsi/qla1280.c:1313: error: 'struct scsi_cmnd' has no member named 'use_sg'
> drivers/scsi/qla1280.c:1314: error: 'struct scsi_cmnd' has no member named 'request_buffer'
> drivers/scsi/qla1280.c:1315: error: 'struct scsi_cmnd' has no member named 'use_sg'
> drivers/scsi/qla1280.c:1316: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
> drivers/scsi/qla1280.c:1318: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
> drivers/scsi/qla1280.c: In function 'qla1280_return_status':
> drivers/scsi/qla1280.c:1409: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
> drivers/scsi/qla1280.c:1416: error: 'struct scsi_cmnd' has no member named 'resid'
> drivers/scsi/qla1280.c: In function 'qla1280_64bit_start_scsi':
> drivers/scsi/qla1280.c:2791: error: 'struct scsi_cmnd' has no member named 'use_sg'
> drivers/scsi/qla1280.c:2792: error: 'struct scsi_cmnd' has no member named 'request_buffer'
> drivers/scsi/qla1280.c:2793: error: 'struct scsi_cmnd' has no member named 'use_sg'
> drivers/scsi/qla1280.c:2801: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
> drivers/scsi/qla1280.c:2896: error: 'struct scsi_cmnd' has no member named 'use_sg'
> drivers/scsi/qla1280.c:2991: error: 'struct scsi_cmnd' has no member named 'request_buffer'
> drivers/scsi/qla1280.c:2992: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
> drivers/scsi/qla1280.c:3004: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
> make[2]: *** [drivers/scsi/qla1280.o] Error 1
> 
> It mystfies me how a patch like this can have been floating about in N
> submissions across M months and nobody has done an allmodconfig build or
> even a grep to find out what broke.
> 
> ho hum.  I shall mark qla1280 BROKEN and shall plod onwards.

A patch to fix this is in James' scsi-pending tree. Jes tested and
fixed it (thanks !) so it will go to -mm via scsi-misc soon.

Boaz, it's better to send major scsi patches to -mm via scsi-misc to
avoid problems like this.


By the way, Andrew, can you add the following patchset to -mm?

http://lkml.org/lkml/2007/10/24/138

It fixes the IOMMUs' problem to merge scatter/gather segments without
considering LLDs' restrictions.


Thanks,

Re: [PATCH 3/4] scsi_data_buffer

[Andrew Morton]

On Tue, 13 Nov 2007 15:40:42 +0900 FUJITA Tomonori <tomof@acm.org> wrote:

> On Mon, 12 Nov 2007 22:06:52 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Thu, 08 Nov 2007 18:59:30 +0200 Boaz Harrosh <bharrosh@panasas.com> wrote:
> > 
> > >   In preparation for bidi we abstract all IO members of scsi_cmnd,
> > >   that will need to duplicate, into a substructure.
> > > 
> > >   - Group all IO members of scsi_cmnd into a scsi_data_buffer
> > >     structure.
> > 
> > drivers/scsi/qla1280.c: In function 'qla1280_done':
> > drivers/scsi/qla1280.c:1313: error: 'struct scsi_cmnd' has no member named 'use_sg'
> > drivers/scsi/qla1280.c:1314: error: 'struct scsi_cmnd' has no member named 'request_buffer'
> > drivers/scsi/qla1280.c:1315: error: 'struct scsi_cmnd' has no member named 'use_sg'
> > drivers/scsi/qla1280.c:1316: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
> > drivers/scsi/qla1280.c:1318: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
> > drivers/scsi/qla1280.c: In function 'qla1280_return_status':
> > drivers/scsi/qla1280.c:1409: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
> > drivers/scsi/qla1280.c:1416: error: 'struct scsi_cmnd' has no member named 'resid'
> > drivers/scsi/qla1280.c: In function 'qla1280_64bit_start_scsi':
> > drivers/scsi/qla1280.c:2791: error: 'struct scsi_cmnd' has no member named 'use_sg'
> > drivers/scsi/qla1280.c:2792: error: 'struct scsi_cmnd' has no member named 'request_buffer'
> > drivers/scsi/qla1280.c:2793: error: 'struct scsi_cmnd' has no member named 'use_sg'
> > drivers/scsi/qla1280.c:2801: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
> > drivers/scsi/qla1280.c:2896: error: 'struct scsi_cmnd' has no member named 'use_sg'
> > drivers/scsi/qla1280.c:2991: error: 'struct scsi_cmnd' has no member named 'request_buffer'
> > drivers/scsi/qla1280.c:2992: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
> > drivers/scsi/qla1280.c:3004: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
> > make[2]: *** [drivers/scsi/qla1280.o] Error 1
> > 
> > It mystfies me how a patch like this can have been floating about in N
> > submissions across M months and nobody has done an allmodconfig build or
> > even a grep to find out what broke.
> > 
> > ho hum.  I shall mark qla1280 BROKEN and shall plod onwards.
> 
> A patch to fix this is in James' scsi-pending tree. Jes tested and
> fixed it (thanks !) so it will go to -mm via scsi-misc soon.

oh gawd.  So we have git-scsi-misc, git-scsi-rc-fixes and now
git-scsi-pending?

I hope you fixed imm, ppa and any other broken drivers?

> Boaz, it's better to send major scsi patches to -mm via scsi-misc to
> avoid problems like this.
> 
> 
> By the way, Andrew, can you add the following patchset to -mm?
> 
> http://lkml.org/lkml/2007/10/24/138
> 
> It fixes the IOMMUs' problem to merge scatter/gather segments without
> considering LLDs' restrictions.

hmm, OK, I saved them away to look at after next -mm.

Re: [PATCH 3/4] scsi_data_buffer

[FUJITA Tomonori]

On Mon, 12 Nov 2007 23:07:03 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 13 Nov 2007 15:40:42 +0900 FUJITA Tomonori <tomof@acm.org> wrote:
> 
> > On Mon, 12 Nov 2007 22:06:52 -0800
> > Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > > On Thu, 08 Nov 2007 18:59:30 +0200 Boaz Harrosh <bharrosh@panasas.com> wrote:
> > > 
> > > >   In preparation for bidi we abstract all IO members of scsi_cmnd,
> > > >   that will need to duplicate, into a substructure.
> > > > 
> > > >   - Group all IO members of scsi_cmnd into a scsi_data_buffer
> > > >     structure.
> > > 
> > > drivers/scsi/qla1280.c: In function 'qla1280_done':
> > > drivers/scsi/qla1280.c:1313: error: 'struct scsi_cmnd' has no member named 'use_sg'
> > > drivers/scsi/qla1280.c:1314: error: 'struct scsi_cmnd' has no member named 'request_buffer'
> > > drivers/scsi/qla1280.c:1315: error: 'struct scsi_cmnd' has no member named 'use_sg'
> > > drivers/scsi/qla1280.c:1316: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
> > > drivers/scsi/qla1280.c:1318: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
> > > drivers/scsi/qla1280.c: In function 'qla1280_return_status':
> > > drivers/scsi/qla1280.c:1409: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
> > > drivers/scsi/qla1280.c:1416: error: 'struct scsi_cmnd' has no member named 'resid'
> > > drivers/scsi/qla1280.c: In function 'qla1280_64bit_start_scsi':
> > > drivers/scsi/qla1280.c:2791: error: 'struct scsi_cmnd' has no member named 'use_sg'
> > > drivers/scsi/qla1280.c:2792: error: 'struct scsi_cmnd' has no member named 'request_buffer'
> > > drivers/scsi/qla1280.c:2793: error: 'struct scsi_cmnd' has no member named 'use_sg'
> > > drivers/scsi/qla1280.c:2801: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
> > > drivers/scsi/qla1280.c:2896: error: 'struct scsi_cmnd' has no member named 'use_sg'
> > > drivers/scsi/qla1280.c:2991: error: 'struct scsi_cmnd' has no member named 'request_buffer'
> > > drivers/scsi/qla1280.c:2992: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
> > > drivers/scsi/qla1280.c:3004: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
> > > make[2]: *** [drivers/scsi/qla1280.o] Error 1
> > > 
> > > It mystfies me how a patch like this can have been floating about in N
> > > submissions across M months and nobody has done an allmodconfig build or
> > > even a grep to find out what broke.
> > > 
> > > ho hum.  I shall mark qla1280 BROKEN and shall plod onwards.
> > 
> > A patch to fix this is in James' scsi-pending tree. Jes tested and
> > fixed it (thanks !) so it will go to -mm via scsi-misc soon.
> 
> oh gawd.  So we have git-scsi-misc, git-scsi-rc-fixes and now
> git-scsi-pending?

I don't think that you will have git-scsi-pending. James just uses the
tree for patches that are not ready for scsi-misc.


> I hope you fixed imm, ppa and any other broken drivers?

There are patches to fix both.

All the scsi drivers are ready for the scsi_data_buffer patchset
though some patches have not been merged yet, I think.

Probabaly, it's the easiest way to handle the scsi_data_buffer
patchset is adding it via scsi-misc.


> > Boaz, it's better to send major scsi patches to -mm via scsi-misc to
> > avoid problems like this.
> > 
> > 
> > By the way, Andrew, can you add the following patchset to -mm?
> > 
> > http://lkml.org/lkml/2007/10/24/138
> > 
> > It fixes the IOMMUs' problem to merge scatter/gather segments without
> > considering LLDs' restrictions.
> 
> hmm, OK, I saved them away to look at after next -mm.

Thanks.

Re: [PATCH 3/4] scsi_data_buffer

[Boaz Harrosh]

On Tue, Nov 13 2007 at 8:40 +0200, FUJITA Tomonori <tomof@acm.org> wrote:
> On Mon, 12 Nov 2007 22:06:52 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
>> On Thu, 08 Nov 2007 18:59:30 +0200 Boaz Harrosh <bharrosh@panasas.com> wrote:
>>
>>>   In preparation for bidi we abstract all IO members of scsi_cmnd,
>>>   that will need to duplicate, into a substructure.
>>>
>>>   - Group all IO members of scsi_cmnd into a scsi_data_buffer
>>>     structure.
>> drivers/scsi/qla1280.c: In function 'qla1280_done':
>> drivers/scsi/qla1280.c:1313: error: 'struct scsi_cmnd' has no member named 'use_sg'
>> drivers/scsi/qla1280.c:1314: error: 'struct scsi_cmnd' has no member named 'request_buffer'
>> drivers/scsi/qla1280.c:1315: error: 'struct scsi_cmnd' has no member named 'use_sg'
>> drivers/scsi/qla1280.c:1316: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
>> drivers/scsi/qla1280.c:1318: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
>> drivers/scsi/qla1280.c: In function 'qla1280_return_status':
>> drivers/scsi/qla1280.c:1409: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
>> drivers/scsi/qla1280.c:1416: error: 'struct scsi_cmnd' has no member named 'resid'
>> drivers/scsi/qla1280.c: In function 'qla1280_64bit_start_scsi':
>> drivers/scsi/qla1280.c:2791: error: 'struct scsi_cmnd' has no member named 'use_sg'
>> drivers/scsi/qla1280.c:2792: error: 'struct scsi_cmnd' has no member named 'request_buffer'
>> drivers/scsi/qla1280.c:2793: error: 'struct scsi_cmnd' has no member named 'use_sg'
>> drivers/scsi/qla1280.c:2801: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
>> drivers/scsi/qla1280.c:2896: error: 'struct scsi_cmnd' has no member named 'use_sg'
>> drivers/scsi/qla1280.c:2991: error: 'struct scsi_cmnd' has no member named 'request_buffer'
>> drivers/scsi/qla1280.c:2992: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
>> drivers/scsi/qla1280.c:3004: error: 'struct scsi_cmnd' has no member named 'request_bufflen'
>> make[2]: *** [drivers/scsi/qla1280.o] Error 1
>>
>> It mystfies me how a patch like this can have been floating about in N
>> submissions across M months and nobody has done an allmodconfig build or
>> even a grep to find out what broke.
>>
>> ho hum.  I shall mark qla1280 BROKEN and shall plod onwards.
> 
> A patch to fix this is in James' scsi-pending tree. Jes tested and
> fixed it (thanks !) so it will go to -mm via scsi-misc soon.
> 
> Boaz, it's better to send major scsi patches to -mm via scsi-misc to
> avoid problems like this.
> 
> 
> By the way, Andrew, can you add the following patchset to -mm?
> 
> http://lkml.org/lkml/2007/10/24/138
> 
> It fixes the IOMMUs' problem to merge scatter/gather segments without
> considering LLDs' restrictions.
> 
> 
> Thanks,

I was surprised that the patches reached -mm. My bad, I CCed Morton
because I thought that they should sit in mm early for collecting
complains, but I meant them to go through scsi-misc.
scsi-pending is patches that need approval from maintainers, but
that James thought they are ready. I was sure it is included in -mm, 
I should have checked.

The scsi_data_buffer patch must wait behind the patches in scsi-pending 
and behind the patch I sent for removal of the NCR53C9x/esp family of drivers.
http://www.spinics.net/lists/linux-scsi/msg20914.html

James please Submit this patch. At least to scsi-pending I have sent it to all 
the registered maintainers and mailing lists. I have got no response ether way. 
>From what Christoph said the only way we can get their comments, if any, is by 
removing the drivers. The only 2 active people (CCed) with interest in this HW, 
are working on the new driver set, and it should be easy to convert any esp HW 
to the new driver set.

Sorry for the mess
Boaz

[PATCH 4/4] SCSI: bidi support

[Boaz Harrosh]

  At the block level bidi request uses req->next_rq pointer for a second
  bidi_read request.
  At Scsi-midlayer a second scsi_data_buffer structure is used for the
  bidi_read part. This bidi scsi_data_buffer is put on
  request->next_rq->special. Struct scsi_cmnd is not changed.

  - Define scsi_bidi_cmnd() to return true if it is a bidi request and a
    second sgtable was allocated.

  - Define scsi_in()/scsi_out() to return the in or out scsi_data_buffer
    from this command This API is to isolate users from the mechanics of
    bidi.

  - Define scsi_end_bidi_request() to do what scsi_end_request() does but
    for a bidi request. This is necessary because bidi commands are a bit
    tricky here. (See comments in body)

  - scsi_release_buffers() will also release the bidi_read scsi_data_buffer

  - scsi_io_completion() on bidi commands will now call
    scsi_end_bidi_request() and return.

  - The previous work done in scsi_init_io() is now done in a new
    scsi_init_sgtable() (which is 99% identical to old scsi_init_io())
    The new scsi_init_io() will call the above twice if needed also for
    the bidi_read command. Only at this point is a command bidi.

  - In scsi_error.c at scsi_eh_prep/restore_cmnd() make sure bidi-lld is not
    confused by a get-sense command that looks like bidi. This is done
    by puting NULL at request->next_rq, and restoring.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_error.c |    3 +
 drivers/scsi/scsi_lib.c   |  154 ++++++++++++++++++++++++++++++++++++---------
 include/scsi/scsi_cmnd.h  |   23 +++++++-
 include/scsi/scsi_eh.h    |    1 +
 4 files changed, 149 insertions(+), 32 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 9daa983..13b6802 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -618,9 +618,11 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,
 	memcpy(ses->cmnd, scmd->cmnd, sizeof(scmd->cmnd));
 	ses->data_direction = scmd->sc_data_direction;
 	ses->sdb = scmd->sdb;
+	ses->next_rq = scmd->request->next_rq;
 	ses->result = scmd->result;
 
 	memset(&scmd->sdb, 0, sizeof(scmd->sdb));
+	scmd->request->next_rq = NULL;
 
 	if (sense_bytes) {
 		scmd->sdb.length = min_t(unsigned,
@@ -673,6 +675,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, struct scsi_eh_save *ses)
 	memcpy(scmd->cmnd, ses->cmnd, sizeof(scmd->cmnd));
 	scmd->sc_data_direction = ses->data_direction;
 	scmd->sdb = ses->sdb;
+	scmd->request->next_rq = ses->next_rq;
 	scmd->result = ses->result;
 }
 EXPORT_SYMBOL(scsi_eh_restore_cmnd);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index edfd8d8..1ed6c6b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -64,6 +64,8 @@ static struct scsi_host_sg_pool scsi_sg_pools[] = {
 };
 #undef SP
 
+static struct kmem_cache *scsi_bidi_sdb_cache;
+
 static void scsi_run_queue(struct request_queue *q);
 
 /*
@@ -625,6 +627,28 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
 		scsi_run_queue(sdev->request_queue);
 }
 
+static void scsi_finalize_request(struct scsi_cmnd *cmd, int uptodate)
+{
+	struct request_queue *q = cmd->device->request_queue;
+	struct request *req = cmd->request;
+	unsigned long flags;
+
+	add_disk_randomness(req->rq_disk);
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	if (blk_rq_tagged(req))
+		blk_queue_end_tag(q, req);
+
+	end_that_request_last(req, uptodate);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+
+	/*
+	 * This will goose the queue request function at the end, so we don't
+	 * need to worry about launching another command.
+	 */
+	scsi_next_command(cmd);
+}
+
 /*
  * Function:    scsi_end_request()
  *
@@ -652,7 +676,6 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int uptodate,
 {
 	struct request_queue *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
-	unsigned long flags;
 
 	/*
 	 * If there are blocks left over at the end, set up the command
@@ -681,19 +704,7 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int uptodate,
 		}
 	}
 
-	add_disk_randomness(req->rq_disk);
-
-	spin_lock_irqsave(q->queue_lock, flags);
-	if (blk_rq_tagged(req))
-		blk_queue_end_tag(q, req);
-	end_that_request_last(req, uptodate);
-	spin_unlock_irqrestore(q->queue_lock, flags);
-
-	/*
-	 * This will goose the queue request function at the end, so we don't
-	 * need to worry about launching another command.
-	 */
-	scsi_next_command(cmd);
+	scsi_finalize_request(cmd, uptodate);
 	return NULL;
 }
 
@@ -892,10 +903,47 @@ void scsi_release_buffers(struct scsi_cmnd *cmd)
 		scsi_free_sgtable(&cmd->sdb);
 
 	memset(&cmd->sdb, 0, sizeof(cmd->sdb));
+
+	if (scsi_bidi_cmnd(cmd)) {
+		struct scsi_data_buffer *bidi_sdb =
+			cmd->request->next_rq->special;
+		scsi_free_sgtable(bidi_sdb);
+		kmem_cache_free(scsi_bidi_sdb_cache, bidi_sdb);
+		cmd->request->next_rq->special = NULL;
+	}
 }
 EXPORT_SYMBOL(scsi_release_buffers);
 
 /*
+ * Bidi commands Must be complete as a whole, both sides at once.
+ * If part of the bytes were written and lld returned
+ * scsi_in()->resid and/or scsi_out()->resid this information will be left
+ * in req->data_len and req->next_rq->data_len. The upper-layer driver can
+ * decide what to do with this information.
+ */
+void scsi_end_bidi_request(struct scsi_cmnd *cmd)
+{
+	struct request *req = cmd->request;
+
+	end_that_request_chunk(req, 1, req->data_len);
+	req->data_len = scsi_out(cmd)->resid;
+
+	end_that_request_chunk(req->next_rq, 1, req->next_rq->data_len);
+	req->next_rq->data_len = scsi_in(cmd)->resid;
+
+	scsi_release_buffers(cmd);
+
+	/*
+	 *FIXME: If ll_rw_blk.c is changed to also put_request(req->next_rq)
+	 *       in end_that_request_last() then this WARN_ON must be removed.
+	 *       for now, upper-driver must have registered an end_io.
+	 */
+	WARN_ON(!req->end_io);
+
+	scsi_finalize_request(cmd, 1);
+}
+
+/*
  * Function:    scsi_io_completion()
  *
  * Purpose:     Completion processing for block device I/O requests.
@@ -956,9 +1004,15 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				req->sense_len = len;
 			}
 		}
+		if (scsi_bidi_cmnd(cmd)) {
+			/* will also release_buffers */
+			scsi_end_bidi_request(cmd);
+			return;
+		}
 		req->data_len = scsi_get_resid(cmd);
 	}
 
+	BUG_ON(blk_bidi_rq(req)); /* bidi not support for !blk_pc_request yet */
 	scsi_release_buffers(cmd);
 
 	/*
@@ -1084,25 +1138,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	scsi_end_request(cmd, 0, this_count, !result);
 }
 
-/*
- * Function:    scsi_init_io()
- *
- * Purpose:     SCSI I/O initialize function.
- *
- * Arguments:   cmd   - Command descriptor we wish to initialize
- *
- * Returns:     0 on success
- *		BLKPREP_DEFER if the failure is retryable
- *		BLKPREP_KILL if the failure is fatal
- */
-int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
+static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb,
+								 gfp_t gfp_mask)
 {
-	struct request     *req = cmd->request;
-	int		   count;
-	struct scsi_data_buffer *sdb = &cmd->sdb;
+	int count;
 
 	if (scsi_alloc_sgtable(sdb, req->nr_phys_segments, gfp_mask)) {
-		scsi_unprep_request(req);
 		return BLKPREP_DEFER;
 	}
 
@@ -1126,9 +1167,52 @@ int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 	printk(KERN_ERR "counted %d, received %d\n", count, sdb->sg_count);
 	printk(KERN_ERR "req nr_sec %lu, cur_nr_sec %u\n", req->nr_sectors,
 			req->current_nr_sectors);
-
 	return BLKPREP_KILL;
 }
+
+/*
+ * Function:    scsi_init_io()
+ *
+ * Purpose:     SCSI I/O initialize function.
+ *
+ * Arguments:   cmd   - Command descriptor we wish to initialize
+ *
+ * Returns:     0 on success
+ *		BLKPREP_DEFER if the failure is retryable
+ *		BLKPREP_KILL if the failure is fatal
+ */
+int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
+{
+	int error = scsi_init_sgtable(cmd->request, &cmd->sdb, gfp_mask);
+	if (error)
+		goto err_exit;
+
+	if (blk_bidi_rq(cmd->request)) {
+		struct scsi_data_buffer *bidi_sdb = kmem_cache_zalloc(
+			scsi_bidi_sdb_cache, GFP_ATOMIC);
+		if (!bidi_sdb) {
+			error = BLKPREP_DEFER;
+			goto err_exit;
+		}
+
+		cmd->request->next_rq->special = bidi_sdb;
+		error = scsi_init_sgtable(cmd->request->next_rq, bidi_sdb,
+								    GFP_ATOMIC);
+		if (error)
+			goto err_exit;
+	}
+
+	return BLKPREP_OK ;
+
+err_exit:
+	scsi_release_buffers(cmd);
+	if (error == BLKPREP_KILL)
+		scsi_put_command(cmd);
+	else /* BLKPREP_DEFER */
+		scsi_unprep_request(cmd->request);
+
+	return error;
+}
 EXPORT_SYMBOL(scsi_init_io);
 
 static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
@@ -1738,6 +1822,14 @@ int __init scsi_init_queue(void)
 		return -ENOMEM;
 	}
 
+	scsi_bidi_sdb_cache = kmem_cache_create("scsi_bidi_sdb",
+					sizeof(struct scsi_data_buffer),
+					0, 0, NULL);
+	if (!scsi_bidi_sdb_cache) {
+		printk(KERN_ERR "SCSI: can't init scsi bidi sdb cache\n");
+		return -ENOMEM;
+	}
+
 	for (i = 0; i < SG_MEMPOOL_NR; i++) {
 		struct scsi_host_sg_pool *sgp = scsi_sg_pools + i;
 		int size = sgp->size * sizeof(struct scatterlist);
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 887a12a..cd2851a 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -2,12 +2,12 @@
 #define _SCSI_SCSI_CMND_H
 
 #include <linux/dma-mapping.h>
+#include <linux/blkdev.h>
 #include <linux/list.h>
 #include <linux/types.h>
 #include <linux/timer.h>
 #include <linux/scatterlist.h>
 
-struct request;
 struct scatterlist;
 struct Scsi_Host;
 struct scsi_device;
@@ -162,4 +162,25 @@ static inline int scsi_get_resid(struct scsi_cmnd *cmd)
 #define scsi_for_each_sg(cmd, sg, nseg, __i)			\
 	for_each_sg(scsi_sglist(cmd), sg, nseg, __i)
 
+static inline int scsi_bidi_cmnd(struct scsi_cmnd *cmd)
+{
+	return blk_bidi_rq(cmd->request) &&
+		(cmd->request->next_rq->special != NULL);
+}
+
+static inline struct scsi_data_buffer *scsi_in(struct scsi_cmnd *cmd)
+{
+	WARN_ON((cmd->sc_data_direction != DMA_FROM_DEVICE) &&
+							!scsi_bidi_cmnd(cmd));
+	return scsi_bidi_cmnd(cmd) ?
+		cmd->request->next_rq->special : &cmd->sdb;
+}
+
+static inline struct scsi_data_buffer *scsi_out(struct scsi_cmnd *cmd)
+{
+	WARN_ON((cmd->sc_data_direction != DMA_TO_DEVICE) &&
+							!scsi_bidi_cmnd(cmd));
+	return &cmd->sdb;
+}
+
 #endif /* _SCSI_SCSI_CMND_H */
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index 1e08be1..25071d5 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -74,6 +74,7 @@ struct scsi_eh_save {
 	unsigned char cmd_len;
 	unsigned char cmnd[MAX_COMMAND_SIZE];
 	struct scsi_data_buffer sdb;
+	struct request *next_rq;
 
 	/* new command support */
 	struct scatterlist sense_sgl;
-- 
1.5.3.1

Re: [PATCH 4/4] SCSI: bidi support

[Kiyoshi Ueda]

Hi Boaz,

On Thu, 08 Nov 2007 19:03:18 +0200, Boaz Harrosh <bharrosh@panasas.com> wrote:
>   At the block level bidi request uses req->next_rq pointer for a second
>   bidi_read request.
>   At Scsi-midlayer a second scsi_data_buffer structure is used for the
>   bidi_read part. This bidi scsi_data_buffer is put on
>   request->next_rq->special. Struct scsi_cmnd is not changed.
> 
>   - Define scsi_bidi_cmnd() to return true if it is a bidi request and a
>     second sgtable was allocated.
> 
>   - Define scsi_in()/scsi_out() to return the in or out scsi_data_buffer
>     from this command This API is to isolate users from the mechanics of
>     bidi.
> 
>   - Define scsi_end_bidi_request() to do what scsi_end_request() does but
>     for a bidi request. This is necessary because bidi commands are a bit
>     tricky here. (See comments in body)
<snip>
>  /*
> + * Bidi commands Must be complete as a whole, both sides at once.
> + * If part of the bytes were written and lld returned
> + * scsi_in()->resid and/or scsi_out()->resid this information will be left
> + * in req->data_len and req->next_rq->data_len. The upper-layer driver can
> + * decide what to do with this information.
> + */
> +void scsi_end_bidi_request(struct scsi_cmnd *cmd)
> +{
> +	struct request *req = cmd->request;
> +
> +	end_that_request_chunk(req, 1, req->data_len);
> +	req->data_len = scsi_out(cmd)->resid;
> +
> +	end_that_request_chunk(req->next_rq, 1, req->next_rq->data_len);
> +	req->next_rq->data_len = scsi_in(cmd)->resid;
> +
> +	scsi_release_buffers(cmd);
> +
> +	/*
> +	 *FIXME: If ll_rw_blk.c is changed to also put_request(req->next_rq)
> +	 *       in end_that_request_last() then this WARN_ON must be removed.
> +	 *       for now, upper-driver must have registered an end_io.
> +	 */
> +	WARN_ON(!req->end_io);
> +
> +	scsi_finalize_request(cmd, 1);
> +}

I'm trying to refine the request completion interface between
device drivers and the block-layer.
(http://marc.info/?l=linux-kernel&m=118953696504819&w=2)

For that work, I wish to keep the space between
end_that_request_chunk() and end_that_request_last() simpler
as much as possible so that they can be converted to the interface
easily.
So I'd like to ask you whether 3 reorderings below cause problem
for you.
  1. reorder end_that_request_chunk() calls for req and req->next_rq
  2. set data_len before end_that_request_chunk() calls
  3. call scsi_release_buffer() after scsi_finalize_request()

void scsi_end_bidi_request(struct scsi_cmnd *cmd)
{
	struct request *req = cmd->request;
	unsigned int dlen = req->data_len;
	unsigned int next_dlen = req->next_rq->data_len;

	req->data_len = scsi_out(cmd)->resid;
	req->next_rq->data_len = scsi_in(cmd)->resid;

	end_that_request_chunk(req->next_rq, 1, next_dlen);
	end_that_request_chunk(req, 1, dlen);

	scsi_finalize_request(cmd, 1);
	scsi_release_buffers(cmd);
}

If they are no problem, I could easily convert it to the new interface.

Thanks,
Kiyoshi Ueda

Re: [PATCH 4/4] SCSI: bidi support

[Kiyoshi Ueda]

Hi Boaz,

On Mon, 12 Nov 2007 12:51:12 +0200, Boaz Harrosh <bharrosh@panasas.com> wrote:
> > I'm trying to refine the request completion interface between
> > device drivers and the block-layer.
> > (http://marc.info/?l=linux-kernel&m=118953696504819&w=2)
> > 
> > For that work, I wish to keep the space between
> > end_that_request_chunk() and end_that_request_last() simpler
> > as much as possible so that they can be converted to the interface
> > easily.
> > So I'd like to ask you whether 3 reorderings below cause problem
> > for you.
> >   1. reorder end_that_request_chunk() calls for req and req->next_rq
> >   2. set data_len before end_that_request_chunk() calls
> >   3. call scsi_release_buffer() after scsi_finalize_request()
> > 
> > void scsi_end_bidi_request(struct scsi_cmnd *cmd)
> > {
> > 	struct request *req = cmd->request;
> > 	unsigned int dlen = req->data_len;
> > 	unsigned int next_dlen = req->next_rq->data_len;
> > 
> > 	req->data_len = scsi_out(cmd)->resid;
> > 	req->next_rq->data_len = scsi_in(cmd)->resid;
> > 
> > 	end_that_request_chunk(req->next_rq, 1, next_dlen);
> > 	end_that_request_chunk(req, 1, dlen);
> > 
> > 	scsi_finalize_request(cmd, 1);
> > 	scsi_release_buffers(cmd);
> > }
> > 
> > If they are no problem, I could easily convert it to the new interface.
>
> 1. If req->data_len is not affected by end_that_request_chunk() than sure.
> 2. Please note that "end_that_request_chunk(req->next_rq, 1, next_dlen);"
>    *must* not be paired with a call to end_that_request_last().
>    It is not a "Queued" request and will break the system. This is
>    the "bidi trick". So end_that_request_chunk() must be exported still.
> 3. scsi_release_buffers(cmd) must stay where it is, and certainly before 
>    scsi_finalize_request() which does a put_command.
> 4. If you are already touching block-layer. I would be happy if you add a
>    blk_end_request_bidi(struct request *, int write_resid, int read_resid,
>    error ...) ;
>    That will release both request and request->next_rq, completely and set
>    resid for ULDs. Then scsi_end_bidi_request() can be removed.
> 
> I would prefer to leave the code as it is for now and change it to what ever
> it needs to be in your patchset.
> But if you insist What I would suggest is below: (Minus the comments)

Thank you for detailed answer and looking the blk_end_request().
I'll consider how to convert based on your answer.

Leaving the code for now is OK to me.

Thanks,
Kiyoshi Ueda