[PATCH 3/4] block: replace sizeof(rq->cmd) with BLK_MAX_CDB

[PATCH 3/4] block: replace sizeof(rq->cmd) with BLK_MAX_CDB

[FUJITA Tomonori]

This is a preparation for changing rq->cmd from the static array to a
pointer.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
---
 block/blk-core.c             |    4 ++--
 drivers/ide/ide-cd.c         |    4 ++--
 drivers/ide/ide-cd_verbose.c |    2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2a438a9..6669238 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -132,7 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->errors = 0;
 	rq->ref_count = 1;
 	rq->cmd_len = 0;
-	memset(rq->cmd, 0, sizeof(rq->cmd));
+	memset(rq->cmd, 0, BLK_MAX_CDB);
 	rq->data_len = 0;
 	rq->extra_len = 0;
 	rq->sense_len = 0;
@@ -194,7 +194,7 @@ void blk_dump_rq_flags(struct request *rq, char *msg)
 
 	if (blk_pc_request(rq)) {
 		printk(KERN_INFO "  cdb: ");
-		for (bit = 0; bit < sizeof(rq->cmd); bit++)
+		for (bit = 0; bit < BLK_MAX_CDB; bit++)
 			printk("%02x ", rq->cmd[bit]);
 		printk("\n");
 	}
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index f8cea94..db29bde 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -879,7 +879,7 @@ static ide_startstop_t cdrom_start_seek_continuation (ide_drive_t *drive)
 
 	sector_div(frame, queue_hardsect_size(drive->queue) >> SECTOR_BITS);
 
-	memset(rq->cmd, 0, sizeof(rq->cmd));
+	memset(rq->cmd, 0, BLK_MAX_CDB);
 	rq->cmd[0] = GPCMD_SEEK;
 	put_unaligned(cpu_to_be32(frame), (unsigned int *) &rq->cmd[2]);
 
@@ -1812,7 +1812,7 @@ static int ide_cdrom_prep_fs(struct request_queue *q, struct request *rq)
 	long block = (long)rq->hard_sector / (hard_sect >> 9);
 	unsigned long blocks = rq->hard_nr_sectors / (hard_sect >> 9);
 
-	memset(rq->cmd, 0, sizeof(rq->cmd));
+	memset(rq->cmd, 0, BLK_MAX_CDB);
 
 	if (rq_data_dir(rq) == READ)
 		rq->cmd[0] = GPCMD_READ_10;
diff --git a/drivers/ide/ide-cd_verbose.c b/drivers/ide/ide-cd_verbose.c
index 6ed7ca0..6490a2d 100644
--- a/drivers/ide/ide-cd_verbose.c
+++ b/drivers/ide/ide-cd_verbose.c
@@ -326,7 +326,7 @@ void ide_cd_log_error(const char *name, struct request *failed_command,
 
 		printk(KERN_ERR "  The failed \"%s\" packet command "
 				"was: \n  \"", s);
-		for (i = 0; i < sizeof(failed_command->cmd); i++)
+		for (i = 0; i < BLK_MAX_CDB; i++)
 			printk(KERN_CONT "%02x ", failed_command->cmd[i]);
 		printk(KERN_CONT "\"\n");
 	}
-- 
1.5.4.2

[PATCH 4/4] block: add large command support

[FUJITA Tomonori]

This patch changes rq->cmd from the static array to a pointer to
support large commands.

We rarely handle large commands. So for optimization, a struct request
still has a static array for a command. rq_init sets rq->cmd pointer
to the static array.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Jens Axboe <jens.axboe@oracle.com>
---
 block/blk-core.c       |    1 +
 drivers/ide/ide-io.c   |    1 +
 include/linux/blkdev.h |   12 ++++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 6669238..6f0968f 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->errors = 0;
 	rq->ref_count = 1;
 	rq->cmd_len = 0;
+	rq->cmd = rq->__cmd;
 	memset(rq->cmd, 0, BLK_MAX_CDB);
 	rq->data_len = 0;
 	rq->extra_len = 0;
diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index 7153796..bac5ea1 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
 {
 	memset(rq, 0, sizeof(*rq));
 	rq->ref_count = 1;
+	rq->cmd = rq->__cmd;
 }
 
 EXPORT_SYMBOL(ide_init_drive_cmd);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index b3a58ad..5710ae4 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -215,8 +215,9 @@ struct request {
 	/*
 	 * when request is used as a packet command carrier
 	 */
-	unsigned int cmd_len;
-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned char __cmd[BLK_MAX_CDB];
+	unsigned char *cmd;
 
 	unsigned int data_len;
 	unsigned int extra_len;	/* length of alignment and padding */
@@ -812,6 +813,13 @@ static inline void put_dev_sector(Sector p)
 	page_cache_release(p.v);
 }
 
+static inline void rq_set_cmd(struct request *rq, unsigned char *cmd,
+			      unsigned short cmd_len)
+{
+	rq->cmd = cmd;
+	rq->cmd_len = cmd_len;
+}
+
 struct work_struct;
 int kblockd_schedule_work(struct work_struct *work);
 void kblockd_flush_work(struct work_struct *work);
-- 
1.5.4.2

Re: [PATCH 4/4] block: add large command support

[Jens Axboe]

On Mon, Apr 14 2008, FUJITA Tomonori wrote:
> This patch changes rq->cmd from the static array to a pointer to
> support large commands.
> 
> We rarely handle large commands. So for optimization, a struct request
> still has a static array for a command. rq_init sets rq->cmd pointer
> to the static array.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Cc: Jens Axboe <jens.axboe@oracle.com>
> ---
>  block/blk-core.c       |    1 +
>  drivers/ide/ide-io.c   |    1 +
>  include/linux/blkdev.h |   12 ++++++++++--
>  3 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 6669238..6f0968f 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
>  	rq->errors = 0;
>  	rq->ref_count = 1;
>  	rq->cmd_len = 0;
> +	rq->cmd = rq->__cmd;
>  	memset(rq->cmd, 0, BLK_MAX_CDB);
>  	rq->data_len = 0;
>  	rq->extra_len = 0;
> diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> index 7153796..bac5ea1 100644
> --- a/drivers/ide/ide-io.c
> +++ b/drivers/ide/ide-io.c
> @@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
>  {
>  	memset(rq, 0, sizeof(*rq));
>  	rq->ref_count = 1;
> +	rq->cmd = rq->__cmd;
>  }
>  
>  EXPORT_SYMBOL(ide_init_drive_cmd);
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index b3a58ad..5710ae4 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -215,8 +215,9 @@ struct request {
>  	/*
>  	 * when request is used as a packet command carrier
>  	 */
> -	unsigned int cmd_len;
> -	unsigned char cmd[BLK_MAX_CDB];
> +	unsigned short cmd_len;
> +	unsigned char __cmd[BLK_MAX_CDB];
> +	unsigned char *cmd;
>  
>  	unsigned int data_len;
>  	unsigned int extra_len;	/* length of alignment and padding */
> @@ -812,6 +813,13 @@ static inline void put_dev_sector(Sector p)
>  	page_cache_release(p.v);
>  }
>  
> +static inline void rq_set_cmd(struct request *rq, unsigned char *cmd,
> +			      unsigned short cmd_len)
> +{
> +	rq->cmd = cmd;
> +	rq->cmd_len = cmd_len;
> +}
> +

This is 100% identical to what I suggested be done instead, so of course
I agree with this.

-- 
Jens Axboe

Re: [PATCH 4/4] block: add large command support

[FUJITA Tomonori]

On Mon, 14 Apr 2008 13:29:20 +0200
Jens Axboe <jens.axboe@oracle.com> wrote:

> On Mon, Apr 14 2008, FUJITA Tomonori wrote:
> > This patch changes rq->cmd from the static array to a pointer to
> > support large commands.
> > 
> > We rarely handle large commands. So for optimization, a struct request
> > still has a static array for a command. rq_init sets rq->cmd pointer
> > to the static array.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Cc: Jens Axboe <jens.axboe@oracle.com>
> > ---
> >  block/blk-core.c       |    1 +
> >  drivers/ide/ide-io.c   |    1 +
> >  include/linux/blkdev.h |   12 ++++++++++--
> >  3 files changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index 6669238..6f0968f 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
> >  	rq->errors = 0;
> >  	rq->ref_count = 1;
> >  	rq->cmd_len = 0;
> > +	rq->cmd = rq->__cmd;
> >  	memset(rq->cmd, 0, BLK_MAX_CDB);
> >  	rq->data_len = 0;
> >  	rq->extra_len = 0;
> > diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> > index 7153796..bac5ea1 100644
> > --- a/drivers/ide/ide-io.c
> > +++ b/drivers/ide/ide-io.c
> > @@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
> >  {
> >  	memset(rq, 0, sizeof(*rq));
> >  	rq->ref_count = 1;
> > +	rq->cmd = rq->__cmd;
> >  }
> >  
> >  EXPORT_SYMBOL(ide_init_drive_cmd);
> > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > index b3a58ad..5710ae4 100644
> > --- a/include/linux/blkdev.h
> > +++ b/include/linux/blkdev.h
> > @@ -215,8 +215,9 @@ struct request {
> >  	/*
> >  	 * when request is used as a packet command carrier
> >  	 */
> > -	unsigned int cmd_len;
> > -	unsigned char cmd[BLK_MAX_CDB];
> > +	unsigned short cmd_len;
> > +	unsigned char __cmd[BLK_MAX_CDB];
> > +	unsigned char *cmd;
> >  
> >  	unsigned int data_len;
> >  	unsigned int extra_len;	/* length of alignment and padding */
> > @@ -812,6 +813,13 @@ static inline void put_dev_sector(Sector p)
> >  	page_cache_release(p.v);
> >  }
> >  
> > +static inline void rq_set_cmd(struct request *rq, unsigned char *cmd,
> > +			      unsigned short cmd_len)
> > +{
> > +	rq->cmd = cmd;
> > +	rq->cmd_len = cmd_len;
> > +}
> > +
> 
> This is 100% identical to what I suggested be done instead, so of course
> I agree with this.

Yeah, I think that it's the most straightforward. We have one value to
represent one item, the length of command. We can access to rq->cmd
and rq->cmd_len as before.

I don't see how it's is dangerous to have a single value to represent
the length of command though changing rq->cmd from the static array to
a pointer to support large commands is tricky. It's possible that I
overlooked something that uses the block layer in an uncommon way as
ide does.

Re: [PATCH 4/4] block: add large command support

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday 14 April 2008, Jens Axboe wrote:
> On Mon, Apr 14 2008, FUJITA Tomonori wrote:
> > This patch changes rq->cmd from the static array to a pointer to
> > support large commands.
> > 
> > We rarely handle large commands. So for optimization, a struct request
> > still has a static array for a command. rq_init sets rq->cmd pointer
> > to the static array.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Cc: Jens Axboe <jens.axboe@oracle.com>
> > ---
> >  block/blk-core.c       |    1 +
> >  drivers/ide/ide-io.c   |    1 +
> >  include/linux/blkdev.h |   12 ++++++++++--
> >  3 files changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index 6669238..6f0968f 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
> >  	rq->errors = 0;
> >  	rq->ref_count = 1;
> >  	rq->cmd_len = 0;
> > +	rq->cmd = rq->__cmd;
> >  	memset(rq->cmd, 0, BLK_MAX_CDB);
> >  	rq->data_len = 0;
> >  	rq->extra_len = 0;
> > diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> > index 7153796..bac5ea1 100644
> > --- a/drivers/ide/ide-io.c
> > +++ b/drivers/ide/ide-io.c
> > @@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
> >  {
> >  	memset(rq, 0, sizeof(*rq));
> >  	rq->ref_count = 1;
> > +	rq->cmd = rq->__cmd;
> >  }

Tomo, some more changes are needed:

Please think about all _static_/dynamic allocations of 'struct request'
used together with REQ_TYPE_SPECIAL etc., i.e.

static void idetape_init_rq(struct request *rq, u8 cmd)

[ rq can be from privately allocated driver's "stack" so no rq_init() ]

{
	memset(rq, 0, sizeof(*rq));
	rq->cmd_type = REQ_TYPE_SPECIAL;
	rq->cmd[0] = cmd;
}

in ide-tape.c or REQ_TYPE_SENSE in ide-cd.c:

static void cdrom_queue_request_sense(ide_drive_t *drive, void *sense,
                                      struct request *failed_command)
{
        struct cdrom_info *info         = drive->driver_data;
        struct request *rq              = &info->request_sense_request;

        if (sense == NULL)
                sense = &info->sense_data;

        /* stuff the sense request in front of our current request */
        ide_cd_init_rq(drive, rq);

        rq->data = sense;
        rq->cmd[0] = GPCMD_REQUEST_SENSE;
        rq->cmd[4] = rq->data_len = 18;

        rq->cmd_type = REQ_TYPE_SENSE;

        /* NOTE! Save the failed command in "rq->buffer" */
        rq->buffer = (void *) failed_command;

        (void) ide_do_drive_cmd(drive, rq, ide_preempt);
}

> >  EXPORT_SYMBOL(ide_init_drive_cmd);
> > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > index b3a58ad..5710ae4 100644
> > --- a/include/linux/blkdev.h
> > +++ b/include/linux/blkdev.h
> > @@ -215,8 +215,9 @@ struct request {
> >  	/*
> >  	 * when request is used as a packet command carrier
> >  	 */
> > -	unsigned int cmd_len;
> > -	unsigned char cmd[BLK_MAX_CDB];
> > +	unsigned short cmd_len;
> > +	unsigned char __cmd[BLK_MAX_CDB];
> > +	unsigned char *cmd;

The source issue lies here:

rq->cmd _silently_ becomes something else and unconverted code will happily
compile without even a _single_ warning (+ memory corruption / oops later).

This is _guaranteed_ to cause problems:

- overlooked code (like the IDE code above, with the current approach
  you have to _manually_ audit all code and still _can't_ be sure about
  the result)

- unmerged code from other trees (i.e., I _have_ WIP patches mapping
  REQ_TYPE_TASKFILE requests onto rq->cmd)

- out of tree code (in theory we don't care but in this specific
  case there is no reason to break things silently)

Please just add new field instead (cost should be negligable and if
we're concerned about it I see no problem with using unnamed union like
it was done by Boaz). 

[ Probably it is also worth to add new length field instead of re-using
  ->cmd_len, just to stay on the safe side (+ for better consistency). ]

> >  	unsigned int data_len;
> >  	unsigned int extra_len;	/* length of alignment and padding */
> > @@ -812,6 +813,13 @@ static inline void put_dev_sector(Sector p)
> >  	page_cache_release(p.v);
> >  }
> >  
> > +static inline void rq_set_cmd(struct request *rq, unsigned char *cmd,
> > +			      unsigned short cmd_len)
> > +{
> > +	rq->cmd = cmd;
> > +	rq->cmd_len = cmd_len;
> > +}
> > +
> 
> This is 100% identical to what I suggested be done instead, so of course
> I agree with this.

Jens, I see that you've applied this patch to block tree
- please revert it for now, it needs to be re-designed.

Thanks,
Bart

Re: [PATCH 4/4] block: add large command support

[FUJITA Tomonori]

On Wed, 16 Apr 2008 00:50:54 +0200
Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:

> 
> Hi,
> 
> On Monday 14 April 2008, Jens Axboe wrote:
> > On Mon, Apr 14 2008, FUJITA Tomonori wrote:
> > > This patch changes rq->cmd from the static array to a pointer to
> > > support large commands.
> > > 
> > > We rarely handle large commands. So for optimization, a struct request
> > > still has a static array for a command. rq_init sets rq->cmd pointer
> > > to the static array.
> > > 
> > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > Cc: Jens Axboe <jens.axboe@oracle.com>
> > > ---
> > >  block/blk-core.c       |    1 +
> > >  drivers/ide/ide-io.c   |    1 +
> > >  include/linux/blkdev.h |   12 ++++++++++--
> > >  3 files changed, 12 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/block/blk-core.c b/block/blk-core.c
> > > index 6669238..6f0968f 100644
> > > --- a/block/blk-core.c
> > > +++ b/block/blk-core.c
> > > @@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
> > >  	rq->errors = 0;
> > >  	rq->ref_count = 1;
> > >  	rq->cmd_len = 0;
> > > +	rq->cmd = rq->__cmd;
> > >  	memset(rq->cmd, 0, BLK_MAX_CDB);
> > >  	rq->data_len = 0;
> > >  	rq->extra_len = 0;
> > > diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> > > index 7153796..bac5ea1 100644
> > > --- a/drivers/ide/ide-io.c
> > > +++ b/drivers/ide/ide-io.c
> > > @@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
> > >  {
> > >  	memset(rq, 0, sizeof(*rq));
> > >  	rq->ref_count = 1;
> > > +	rq->cmd = rq->__cmd;
> > >  }
> 
> Tomo, some more changes are needed:
> 
> Please think about all _static_/dynamic allocations of 'struct request'
> used together with REQ_TYPE_SPECIAL etc., i.e.

I think that using struct request allocated statically is wrong from
the perspective of the block layer design, that is, you always need to
use blk_get_request. I think that except ide, everyone does.

I try to convert ide to use blk_get_request properly if you want.


> static void idetape_init_rq(struct request *rq, u8 cmd)
> 
> [ rq can be from privately allocated driver's "stack" so no rq_init() ]
> 
> {
> 	memset(rq, 0, sizeof(*rq));
> 	rq->cmd_type = REQ_TYPE_SPECIAL;
> 	rq->cmd[0] = cmd;
> }
> 
> in ide-tape.c or REQ_TYPE_SENSE in ide-cd.c:

Thanks, I overlooked this. As I did for ide_init_drive_cmd, we need:


+	rq->cmd = rq->__cmd;


> static void cdrom_queue_request_sense(ide_drive_t *drive, void *sense,
>                                       struct request *failed_command)
> {
>         struct cdrom_info *info         = drive->driver_data;
>         struct request *rq              = &info->request_sense_request;
> 
>         if (sense == NULL)
>                 sense = &info->sense_data;
> 
>         /* stuff the sense request in front of our current request */
>         ide_cd_init_rq(drive, rq);
> 
>         rq->data = sense;
>         rq->cmd[0] = GPCMD_REQUEST_SENSE;
>         rq->cmd[4] = rq->data_len = 18;
> 
>         rq->cmd_type = REQ_TYPE_SENSE;
> 
>         /* NOTE! Save the failed command in "rq->buffer" */
>         rq->buffer = (void *) failed_command;
> 
>         (void) ide_do_drive_cmd(drive, rq, ide_preempt);
> }

This should work since I put the above hack to ide_init_drive_cmd (I
tested the patchset with an ide cdrom drive).


> > >  EXPORT_SYMBOL(ide_init_drive_cmd);
> > > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > > index b3a58ad..5710ae4 100644
> > > --- a/include/linux/blkdev.h
> > > +++ b/include/linux/blkdev.h
> > > @@ -215,8 +215,9 @@ struct request {
> > >  	/*
> > >  	 * when request is used as a packet command carrier
> > >  	 */
> > > -	unsigned int cmd_len;
> > > -	unsigned char cmd[BLK_MAX_CDB];
> > > +	unsigned short cmd_len;
> > > +	unsigned char __cmd[BLK_MAX_CDB];
> > > +	unsigned char *cmd;
> 
> The source issue lies here:
> 
> rq->cmd _silently_ becomes something else and unconverted code will happily
> compile without even a _single_ warning (+ memory corruption / oops later).
> 
> This is _guaranteed_ to cause problems:
> 
> - overlooked code (like the IDE code above, with the current approach
>   you have to _manually_ audit all code and still _can't_ be sure about
>   the result)

As far as I know, only ide does that.


> - unmerged code from other trees (i.e., I _have_ WIP patches mapping
>   REQ_TYPE_TASKFILE requests onto rq->cmd)
> 
> - out of tree code (in theory we don't care but in this specific
>   case there is no reason to break things silently)

Again, I think that we can say that we need to use the block layer
properly, struct request always needs to be allocated via
blk_get_request.


> Please just add new field instead (cost should be negligable and if
> we're concerned about it I see no problem with using unnamed union like
> it was done by Boaz). 
> 
> [ Probably it is also worth to add new length field instead of re-using
>   ->cmd_len, just to stay on the safe side (+ for better consistency). ]

As we discussed, we don't like that hack:

http://marc.info/?t=120724777800003&r=1&w=2


> > >  	unsigned int data_len;
> > >  	unsigned int extra_len;	/* length of alignment and padding */
> > > @@ -812,6 +813,13 @@ static inline void put_dev_sector(Sector p)
> > >  	page_cache_release(p.v);
> > >  }
> > >  
> > > +static inline void rq_set_cmd(struct request *rq, unsigned char *cmd,
> > > +			      unsigned short cmd_len)
> > > +{
> > > +	rq->cmd = cmd;
> > > +	rq->cmd_len = cmd_len;
> > > +}
> > > +
> > 
> > This is 100% identical to what I suggested be done instead, so of course
> > I agree with this.
> 
> Jens, I see that you've applied this patch to block tree
> - please revert it for now, it needs to be re-designed.
> 
> Thanks,
> Bart
> --
> 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 4/4] block: add large command support

[Bartlomiej Zolnierkiewicz]

On Wednesday 16 April 2008, FUJITA Tomonori wrote:
> On Wed, 16 Apr 2008 00:50:54 +0200
> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> 
> > 
> > Hi,
> > 
> > On Monday 14 April 2008, Jens Axboe wrote:
> > > On Mon, Apr 14 2008, FUJITA Tomonori wrote:
> > > > This patch changes rq->cmd from the static array to a pointer to
> > > > support large commands.
> > > > 
> > > > We rarely handle large commands. So for optimization, a struct request
> > > > still has a static array for a command. rq_init sets rq->cmd pointer
> > > > to the static array.
> > > > 
> > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > > Cc: Jens Axboe <jens.axboe@oracle.com>
> > > > ---
> > > >  block/blk-core.c       |    1 +
> > > >  drivers/ide/ide-io.c   |    1 +
> > > >  include/linux/blkdev.h |   12 ++++++++++--
> > > >  3 files changed, 12 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/block/blk-core.c b/block/blk-core.c
> > > > index 6669238..6f0968f 100644
> > > > --- a/block/blk-core.c
> > > > +++ b/block/blk-core.c
> > > > @@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
> > > >  	rq->errors = 0;
> > > >  	rq->ref_count = 1;
> > > >  	rq->cmd_len = 0;
> > > > +	rq->cmd = rq->__cmd;
> > > >  	memset(rq->cmd, 0, BLK_MAX_CDB);
> > > >  	rq->data_len = 0;
> > > >  	rq->extra_len = 0;
> > > > diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> > > > index 7153796..bac5ea1 100644
> > > > --- a/drivers/ide/ide-io.c
> > > > +++ b/drivers/ide/ide-io.c
> > > > @@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
> > > >  {
> > > >  	memset(rq, 0, sizeof(*rq));
> > > >  	rq->ref_count = 1;
> > > > +	rq->cmd = rq->__cmd;
> > > >  }
> > 
> > Tomo, some more changes are needed:
> > 
> > Please think about all _static_/dynamic allocations of 'struct request'
> > used together with REQ_TYPE_SPECIAL etc., i.e.
> 
> I think that using struct request allocated statically is wrong from
> the perspective of the block layer design, that is, you always need to
> use blk_get_request. I think that except ide, everyone does.

There still are some others like:

- scsi/scsi_error.c::scsi_reset_provider()

- paride/pd.c::pd_special_command()

but yeah, IDE has the most users left.

> I try to convert ide to use blk_get_request properly if you want.

I would love to see the patches.

> > static void idetape_init_rq(struct request *rq, u8 cmd)
> > 
> > [ rq can be from privately allocated driver's "stack" so no rq_init() ]
> > 
> > {
> > 	memset(rq, 0, sizeof(*rq));
> > 	rq->cmd_type = REQ_TYPE_SPECIAL;
> > 	rq->cmd[0] = cmd;
> > }
> > 
> > in ide-tape.c or REQ_TYPE_SENSE in ide-cd.c:
> 
> Thanks, I overlooked this. As I did for ide_init_drive_cmd, we need:
> 
> 
> +	rq->cmd = rq->__cmd;
> 
> 
> > static void cdrom_queue_request_sense(ide_drive_t *drive, void *sense,
> >                                       struct request *failed_command)
> > {
> >         struct cdrom_info *info         = drive->driver_data;
> >         struct request *rq              = &info->request_sense_request;
> > 
> >         if (sense == NULL)
> >                 sense = &info->sense_data;
> > 
> >         /* stuff the sense request in front of our current request */
> >         ide_cd_init_rq(drive, rq);
> > 
> >         rq->data = sense;
> >         rq->cmd[0] = GPCMD_REQUEST_SENSE;
> >         rq->cmd[4] = rq->data_len = 18;
> > 
> >         rq->cmd_type = REQ_TYPE_SENSE;
> > 
> >         /* NOTE! Save the failed command in "rq->buffer" */
> >         rq->buffer = (void *) failed_command;
> > 
> >         (void) ide_do_drive_cmd(drive, rq, ide_preempt);
> > }
> 
> This should work since I put the above hack to ide_init_drive_cmd (I
> tested the patchset with an ide cdrom drive).

Indeed, it is called by ide_cd_init_rq() (I blame 2AM).

> > > >  EXPORT_SYMBOL(ide_init_drive_cmd);
> > > > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > > > index b3a58ad..5710ae4 100644
> > > > --- a/include/linux/blkdev.h
> > > > +++ b/include/linux/blkdev.h
> > > > @@ -215,8 +215,9 @@ struct request {
> > > >  	/*
> > > >  	 * when request is used as a packet command carrier
> > > >  	 */
> > > > -	unsigned int cmd_len;
> > > > -	unsigned char cmd[BLK_MAX_CDB];
> > > > +	unsigned short cmd_len;
> > > > +	unsigned char __cmd[BLK_MAX_CDB];
> > > > +	unsigned char *cmd;
> > 
> > The source issue lies here:
> > 
> > rq->cmd _silently_ becomes something else and unconverted code will happily
> > compile without even a _single_ warning (+ memory corruption / oops later).
> > 
> > This is _guaranteed_ to cause problems:
> > 
> > - overlooked code (like the IDE code above, with the current approach
> >   you have to _manually_ audit all code and still _can't_ be sure about
> >   the result)
> 
> As far as I know, only ide does that.

Well, if there are others you'll learn about them the hard-way... ;-)

[ The thing is that you can avoid answering this question completely
  with the "ugly" approach. ]

> > - unmerged code from other trees (i.e., I _have_ WIP patches mapping
> >   REQ_TYPE_TASKFILE requests onto rq->cmd)
> > 
> > - out of tree code (in theory we don't care but in this specific
> >   case there is no reason to break things silently)
> 
> Again, I think that we can say that we need to use the block layer
> properly, struct request always needs to be allocated via
> blk_get_request.

Hmm, in this case some code asserting that only blk_bet_request()
requests allowed in the block layer would be useful.

> > Please just add new field instead (cost should be negligable and if
> > we're concerned about it I see no problem with using unnamed union like
> > it was done by Boaz). 
> > 
> > [ Probably it is also worth to add new length field instead of re-using
> >   ->cmd_len, just to stay on the safe side (+ for better consistency). ]
> 
> As we discussed, we don't like that hack:
> 
> http://marc.info/?t=120724777800003&r=1&w=2

If you audit+fixup IDE I'm also fine with non-"hack" solution.

Thanks,
Bart

Re: [PATCH 4/4] block: add large command support

[Jens Axboe]

On Wed, Apr 16 2008, FUJITA Tomonori wrote:
> On Wed, 16 Apr 2008 00:50:54 +0200
> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> 
> > 
> > Hi,
> > 
> > On Monday 14 April 2008, Jens Axboe wrote:
> > > On Mon, Apr 14 2008, FUJITA Tomonori wrote:
> > > > This patch changes rq->cmd from the static array to a pointer to
> > > > support large commands.
> > > > 
> > > > We rarely handle large commands. So for optimization, a struct request
> > > > still has a static array for a command. rq_init sets rq->cmd pointer
> > > > to the static array.
> > > > 
> > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > > Cc: Jens Axboe <jens.axboe@oracle.com>
> > > > ---
> > > >  block/blk-core.c       |    1 +
> > > >  drivers/ide/ide-io.c   |    1 +
> > > >  include/linux/blkdev.h |   12 ++++++++++--
> > > >  3 files changed, 12 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/block/blk-core.c b/block/blk-core.c
> > > > index 6669238..6f0968f 100644
> > > > --- a/block/blk-core.c
> > > > +++ b/block/blk-core.c
> > > > @@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
> > > >  	rq->errors = 0;
> > > >  	rq->ref_count = 1;
> > > >  	rq->cmd_len = 0;
> > > > +	rq->cmd = rq->__cmd;
> > > >  	memset(rq->cmd, 0, BLK_MAX_CDB);
> > > >  	rq->data_len = 0;
> > > >  	rq->extra_len = 0;
> > > > diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> > > > index 7153796..bac5ea1 100644
> > > > --- a/drivers/ide/ide-io.c
> > > > +++ b/drivers/ide/ide-io.c
> > > > @@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
> > > >  {
> > > >  	memset(rq, 0, sizeof(*rq));
> > > >  	rq->ref_count = 1;
> > > > +	rq->cmd = rq->__cmd;
> > > >  }
> > 
> > Tomo, some more changes are needed:
> > 
> > Please think about all _static_/dynamic allocations of 'struct request'
> > used together with REQ_TYPE_SPECIAL etc., i.e.
> 
> I think that using struct request allocated statically is wrong from
> the perspective of the block layer design, that is, you always need to
> use blk_get_request. I think that except ide, everyone does.
> 
> I try to convert ide to use blk_get_request properly if you want.

That would be best, but the on-stack allocation has the benefit that
it'll always work. So until we can completely get rid of that, lets just
make it a hard rule that ANY rq allocation MUST call rq_init(). It's a
lot saner than doing a memset() anyway.

-- 
Jens Axboe

Re: [PATCH 4/4] block: add large command support

[Boaz Harrosh]

On Wed, Apr 16 2008 at 11:33 +0300, Jens Axboe <jens.axboe@oracle.com> wrote:
> On Wed, Apr 16 2008, FUJITA Tomonori wrote:
>> On Wed, 16 Apr 2008 00:50:54 +0200
>> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
>>
>>> Hi,
>>>
>>> On Monday 14 April 2008, Jens Axboe wrote:
>>>> On Mon, Apr 14 2008, FUJITA Tomonori wrote:
>>>>> This patch changes rq->cmd from the static array to a pointer to
>>>>> support large commands.
>>>>>
>>>>> We rarely handle large commands. So for optimization, a struct request
>>>>> still has a static array for a command. rq_init sets rq->cmd pointer
>>>>> to the static array.
>>>>>
>>>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
>>>>> Cc: Jens Axboe <jens.axboe@oracle.com>
>>>>> ---
>>>>>  block/blk-core.c       |    1 +
>>>>>  drivers/ide/ide-io.c   |    1 +
>>>>>  include/linux/blkdev.h |   12 ++++++++++--
>>>>>  3 files changed, 12 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>>> index 6669238..6f0968f 100644
>>>>> --- a/block/blk-core.c
>>>>> +++ b/block/blk-core.c
>>>>> @@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
>>>>>  	rq->errors = 0;
>>>>>  	rq->ref_count = 1;
>>>>>  	rq->cmd_len = 0;
>>>>> +	rq->cmd = rq->__cmd;
>>>>>  	memset(rq->cmd, 0, BLK_MAX_CDB);
>>>>>  	rq->data_len = 0;
>>>>>  	rq->extra_len = 0;
>>>>> diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
>>>>> index 7153796..bac5ea1 100644
>>>>> --- a/drivers/ide/ide-io.c
>>>>> +++ b/drivers/ide/ide-io.c
>>>>> @@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
>>>>>  {
>>>>>  	memset(rq, 0, sizeof(*rq));
>>>>>  	rq->ref_count = 1;
>>>>> +	rq->cmd = rq->__cmd;
>>>>>  }
>>> Tomo, some more changes are needed:
>>>
>>> Please think about all _static_/dynamic allocations of 'struct request'
>>> used together with REQ_TYPE_SPECIAL etc., i.e.
>> I think that using struct request allocated statically is wrong from
>> the perspective of the block layer design, that is, you always need to
>> use blk_get_request. I think that except ide, everyone does.
>>
>> I try to convert ide to use blk_get_request properly if you want.
> 
> That would be best, but the on-stack allocation has the benefit that
> it'll always work. So until we can completely get rid of that, lets just
> make it a hard rule that ANY rq allocation MUST call rq_init(). It's a
> lot saner than doing a memset() anyway.
> 

Just a patch that I had for ages since the bad old request bidi times,
perhaps is also good today. (rebased to for-2.6.26 branch)
---
From: Boaz Harrosh <bharrosh@panasas.com>
Date: Wed, 16 Apr 2008 12:05:33 +0300
Subject: [PATCH] Initialize all members of struct request in rq_init

Before, every member added/removed from struct request would entitle a change
to rq_init, for initialization. Now all members are default to zero and only
the none zero members are specifically initialized.

Users that need requests on the stack or pre-allocated, must call rq_init()
before use.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 block/blk-core.c |   22 ++--------------------
 1 files changed, 2 insertions(+), 20 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 6f0968f..3f4c563 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -113,36 +113,18 @@ EXPORT_SYMBOL(blk_get_backing_dev_info);
  */
 void rq_init(struct request_queue *q, struct request *rq)
 {
+	memset(rq, 0, sizeof(*rq));
 	INIT_LIST_HEAD(&rq->queuelist);
 	INIT_LIST_HEAD(&rq->donelist);
 	rq->q = q;
 	rq->sector = rq->hard_sector = (sector_t) -1;
-	rq->nr_sectors = rq->hard_nr_sectors = 0;
-	rq->current_nr_sectors = rq->hard_cur_sectors = 0;
-	rq->bio = rq->biotail = NULL;
 	INIT_HLIST_NODE(&rq->hash);
 	RB_CLEAR_NODE(&rq->rb_node);
-	rq->rq_disk = NULL;
-	rq->nr_phys_segments = 0;
-	rq->nr_hw_segments = 0;
-	rq->ioprio = 0;
-	rq->special = NULL;
-	rq->buffer = NULL;
 	rq->tag = -1;
-	rq->errors = 0;
 	rq->ref_count = 1;
-	rq->cmd_len = 0;
 	rq->cmd = rq->__cmd;
-	memset(rq->cmd, 0, BLK_MAX_CDB);
-	rq->data_len = 0;
-	rq->extra_len = 0;
-	rq->sense_len = 0;
-	rq->data = NULL;
-	rq->sense = NULL;
-	rq->end_io = NULL;
-	rq->end_io_data = NULL;
-	rq->next_rq = NULL;
 }
+EXPORT_SYMBOL(rq_init);
 
 static void req_bio_endio(struct request *rq, struct bio *bio,
 			  unsigned int nbytes, int error)
-- 
1.5.3.3

Re: [PATCH 4/4] block: add large command support

[Jens Axboe]

On Wed, Apr 16 2008, Boaz Harrosh wrote:
> On Wed, Apr 16 2008 at 11:33 +0300, Jens Axboe <jens.axboe@oracle.com> wrote:
> > On Wed, Apr 16 2008, FUJITA Tomonori wrote:
> >> On Wed, 16 Apr 2008 00:50:54 +0200
> >> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> >>
> >>> Hi,
> >>>
> >>> On Monday 14 April 2008, Jens Axboe wrote:
> >>>> On Mon, Apr 14 2008, FUJITA Tomonori wrote:
> >>>>> This patch changes rq->cmd from the static array to a pointer to
> >>>>> support large commands.
> >>>>>
> >>>>> We rarely handle large commands. So for optimization, a struct request
> >>>>> still has a static array for a command. rq_init sets rq->cmd pointer
> >>>>> to the static array.
> >>>>>
> >>>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> >>>>> Cc: Jens Axboe <jens.axboe@oracle.com>
> >>>>> ---
> >>>>>  block/blk-core.c       |    1 +
> >>>>>  drivers/ide/ide-io.c   |    1 +
> >>>>>  include/linux/blkdev.h |   12 ++++++++++--
> >>>>>  3 files changed, 12 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/block/blk-core.c b/block/blk-core.c
> >>>>> index 6669238..6f0968f 100644
> >>>>> --- a/block/blk-core.c
> >>>>> +++ b/block/blk-core.c
> >>>>> @@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
> >>>>>  	rq->errors = 0;
> >>>>>  	rq->ref_count = 1;
> >>>>>  	rq->cmd_len = 0;
> >>>>> +	rq->cmd = rq->__cmd;
> >>>>>  	memset(rq->cmd, 0, BLK_MAX_CDB);
> >>>>>  	rq->data_len = 0;
> >>>>>  	rq->extra_len = 0;
> >>>>> diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> >>>>> index 7153796..bac5ea1 100644
> >>>>> --- a/drivers/ide/ide-io.c
> >>>>> +++ b/drivers/ide/ide-io.c
> >>>>> @@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
> >>>>>  {
> >>>>>  	memset(rq, 0, sizeof(*rq));
> >>>>>  	rq->ref_count = 1;
> >>>>> +	rq->cmd = rq->__cmd;
> >>>>>  }
> >>> Tomo, some more changes are needed:
> >>>
> >>> Please think about all _static_/dynamic allocations of 'struct request'
> >>> used together with REQ_TYPE_SPECIAL etc., i.e.
> >> I think that using struct request allocated statically is wrong from
> >> the perspective of the block layer design, that is, you always need to
> >> use blk_get_request. I think that except ide, everyone does.
> >>
> >> I try to convert ide to use blk_get_request properly if you want.
> > 
> > That would be best, but the on-stack allocation has the benefit that
> > it'll always work. So until we can completely get rid of that, lets just
> > make it a hard rule that ANY rq allocation MUST call rq_init(). It's a
> > lot saner than doing a memset() anyway.
> > 
> 
> Just a patch that I had for ages since the bad old request bidi times,
> perhaps is also good today. (rebased to for-2.6.26 branch)
> ---
> From: Boaz Harrosh <bharrosh@panasas.com>
> Date: Wed, 16 Apr 2008 12:05:33 +0300
> Subject: [PATCH] Initialize all members of struct request in rq_init
> 
> Before, every member added/removed from struct request would entitle a change
> to rq_init, for initialization. Now all members are default to zero and only
> the none zero members are specifically initialized.
> 
> Users that need requests on the stack or pre-allocated, must call rq_init()
> before use.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  block/blk-core.c |   22 ++--------------------
>  1 files changed, 2 insertions(+), 20 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 6f0968f..3f4c563 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -113,36 +113,18 @@ EXPORT_SYMBOL(blk_get_backing_dev_info);
>   */
>  void rq_init(struct request_queue *q, struct request *rq)
>  {
> +	memset(rq, 0, sizeof(*rq));
>  	INIT_LIST_HEAD(&rq->queuelist);
>  	INIT_LIST_HEAD(&rq->donelist);
>  	rq->q = q;
>  	rq->sector = rq->hard_sector = (sector_t) -1;
> -	rq->nr_sectors = rq->hard_nr_sectors = 0;
> -	rq->current_nr_sectors = rq->hard_cur_sectors = 0;
> -	rq->bio = rq->biotail = NULL;
>  	INIT_HLIST_NODE(&rq->hash);
>  	RB_CLEAR_NODE(&rq->rb_node);
> -	rq->rq_disk = NULL;
> -	rq->nr_phys_segments = 0;
> -	rq->nr_hw_segments = 0;
> -	rq->ioprio = 0;
> -	rq->special = NULL;
> -	rq->buffer = NULL;
>  	rq->tag = -1;
> -	rq->errors = 0;
>  	rq->ref_count = 1;
> -	rq->cmd_len = 0;
>  	rq->cmd = rq->__cmd;
> -	memset(rq->cmd, 0, BLK_MAX_CDB);
> -	rq->data_len = 0;
> -	rq->extra_len = 0;
> -	rq->sense_len = 0;
> -	rq->data = NULL;
> -	rq->sense = NULL;
> -	rq->end_io = NULL;
> -	rq->end_io_data = NULL;
> -	rq->next_rq = NULL;
>  }
> +EXPORT_SYMBOL(rq_init);
>  
>  static void req_bio_endio(struct request *rq, struct bio *bio,
>  			  unsigned int nbytes, int error)
> -- 
> 1.5.3.3
> 

Totally agree, if we expand rq_init() to all users (including on stack),
this must be included as well.

-- 
Jens Axboe

Re: [PATCH 4/4] block: add large command support

[Bartlomiej Zolnierkiewicz]

On Wednesday 16 April 2008, Jens Axboe wrote:
> On Wed, Apr 16 2008, Boaz Harrosh wrote:
> > On Wed, Apr 16 2008 at 11:33 +0300, Jens Axboe <jens.axboe@oracle.com> wrote:
> > > On Wed, Apr 16 2008, FUJITA Tomonori wrote:
> > >> On Wed, 16 Apr 2008 00:50:54 +0200
> > >> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> > >>
> > >>> Hi,
> > >>>
> > >>> On Monday 14 April 2008, Jens Axboe wrote:
> > >>>> On Mon, Apr 14 2008, FUJITA Tomonori wrote:
> > >>>>> This patch changes rq->cmd from the static array to a pointer to
> > >>>>> support large commands.
> > >>>>>
> > >>>>> We rarely handle large commands. So for optimization, a struct request
> > >>>>> still has a static array for a command. rq_init sets rq->cmd pointer
> > >>>>> to the static array.
> > >>>>>
> > >>>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > >>>>> Cc: Jens Axboe <jens.axboe@oracle.com>
> > >>>>> ---
> > >>>>>  block/blk-core.c       |    1 +
> > >>>>>  drivers/ide/ide-io.c   |    1 +
> > >>>>>  include/linux/blkdev.h |   12 ++++++++++--
> > >>>>>  3 files changed, 12 insertions(+), 2 deletions(-)
> > >>>>>
> > >>>>> diff --git a/block/blk-core.c b/block/blk-core.c
> > >>>>> index 6669238..6f0968f 100644
> > >>>>> --- a/block/blk-core.c
> > >>>>> +++ b/block/blk-core.c
> > >>>>> @@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
> > >>>>>  	rq->errors = 0;
> > >>>>>  	rq->ref_count = 1;
> > >>>>>  	rq->cmd_len = 0;
> > >>>>> +	rq->cmd = rq->__cmd;
> > >>>>>  	memset(rq->cmd, 0, BLK_MAX_CDB);
> > >>>>>  	rq->data_len = 0;
> > >>>>>  	rq->extra_len = 0;
> > >>>>> diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> > >>>>> index 7153796..bac5ea1 100644
> > >>>>> --- a/drivers/ide/ide-io.c
> > >>>>> +++ b/drivers/ide/ide-io.c
> > >>>>> @@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
> > >>>>>  {
> > >>>>>  	memset(rq, 0, sizeof(*rq));
> > >>>>>  	rq->ref_count = 1;
> > >>>>> +	rq->cmd = rq->__cmd;
> > >>>>>  }
> > >>> Tomo, some more changes are needed:
> > >>>
> > >>> Please think about all _static_/dynamic allocations of 'struct request'
> > >>> used together with REQ_TYPE_SPECIAL etc., i.e.
> > >> I think that using struct request allocated statically is wrong from
> > >> the perspective of the block layer design, that is, you always need to
> > >> use blk_get_request. I think that except ide, everyone does.
> > >>
> > >> I try to convert ide to use blk_get_request properly if you want.
> > > 
> > > That would be best, but the on-stack allocation has the benefit that
> > > it'll always work. So until we can completely get rid of that, lets just
> > > make it a hard rule that ANY rq allocation MUST call rq_init(). It's a
> > > lot saner than doing a memset() anyway.

Sounds OK to me.

> > Just a patch that I had for ages since the bad old request bidi times,
> > perhaps is also good today. (rebased to for-2.6.26 branch)
> > ---
> > From: Boaz Harrosh <bharrosh@panasas.com>
> > Date: Wed, 16 Apr 2008 12:05:33 +0300
> > Subject: [PATCH] Initialize all members of struct request in rq_init
> > 
> > Before, every member added/removed from struct request would entitle a change
> > to rq_init, for initialization. Now all members are default to zero and only
> > the none zero members are specifically initialized.
> > 
> > Users that need requests on the stack or pre-allocated, must call rq_init()
> > before use.
> > 
> > Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> > ---
> >  block/blk-core.c |   22 ++--------------------
> >  1 files changed, 2 insertions(+), 20 deletions(-)
> > 
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index 6f0968f..3f4c563 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -113,36 +113,18 @@ EXPORT_SYMBOL(blk_get_backing_dev_info);
> >   */
> >  void rq_init(struct request_queue *q, struct request *rq)
> >  {
> > +	memset(rq, 0, sizeof(*rq));
> >  	INIT_LIST_HEAD(&rq->queuelist);
> >  	INIT_LIST_HEAD(&rq->donelist);
> >  	rq->q = q;
> >  	rq->sector = rq->hard_sector = (sector_t) -1;
> > -	rq->nr_sectors = rq->hard_nr_sectors = 0;
> > -	rq->current_nr_sectors = rq->hard_cur_sectors = 0;
> > -	rq->bio = rq->biotail = NULL;
> >  	INIT_HLIST_NODE(&rq->hash);
> >  	RB_CLEAR_NODE(&rq->rb_node);
> > -	rq->rq_disk = NULL;
> > -	rq->nr_phys_segments = 0;
> > -	rq->nr_hw_segments = 0;
> > -	rq->ioprio = 0;
> > -	rq->special = NULL;
> > -	rq->buffer = NULL;
> >  	rq->tag = -1;
> > -	rq->errors = 0;
> >  	rq->ref_count = 1;
> > -	rq->cmd_len = 0;
> >  	rq->cmd = rq->__cmd;
> > -	memset(rq->cmd, 0, BLK_MAX_CDB);
> > -	rq->data_len = 0;
> > -	rq->extra_len = 0;
> > -	rq->sense_len = 0;
> > -	rq->data = NULL;
> > -	rq->sense = NULL;
> > -	rq->end_io = NULL;
> > -	rq->end_io_data = NULL;
> > -	rq->next_rq = NULL;
> >  }
> > +EXPORT_SYMBOL(rq_init);
> >  
> >  static void req_bio_endio(struct request *rq, struct bio *bio,
> >  			  unsigned int nbytes, int error)
> > -- 
> > 1.5.3.3
> > 
> 
> Totally agree, if we expand rq_init() to all users (including on stack),
> this must be included as well.

Completely, totally and fully agreed...

Thanks,
Bart

Re: [PATCH 4/4] block: add large command support

[FUJITA Tomonori]

On Wed, 16 Apr 2008 12:08:25 +0300
Boaz Harrosh <bharrosh@panasas.com> wrote:

> On Wed, Apr 16 2008 at 11:33 +0300, Jens Axboe <jens.axboe@oracle.com> wrote:
> > On Wed, Apr 16 2008, FUJITA Tomonori wrote:
> >> On Wed, 16 Apr 2008 00:50:54 +0200
> >> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> >>
> >>> Hi,
> >>>
> >>> On Monday 14 April 2008, Jens Axboe wrote:
> >>>> On Mon, Apr 14 2008, FUJITA Tomonori wrote:
> >>>>> This patch changes rq->cmd from the static array to a pointer to
> >>>>> support large commands.
> >>>>>
> >>>>> We rarely handle large commands. So for optimization, a struct request
> >>>>> still has a static array for a command. rq_init sets rq->cmd pointer
> >>>>> to the static array.
> >>>>>
> >>>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> >>>>> Cc: Jens Axboe <jens.axboe@oracle.com>
> >>>>> ---
> >>>>>  block/blk-core.c       |    1 +
> >>>>>  drivers/ide/ide-io.c   |    1 +
> >>>>>  include/linux/blkdev.h |   12 ++++++++++--
> >>>>>  3 files changed, 12 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/block/blk-core.c b/block/blk-core.c
> >>>>> index 6669238..6f0968f 100644
> >>>>> --- a/block/blk-core.c
> >>>>> +++ b/block/blk-core.c
> >>>>> @@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
> >>>>>  	rq->errors = 0;
> >>>>>  	rq->ref_count = 1;
> >>>>>  	rq->cmd_len = 0;
> >>>>> +	rq->cmd = rq->__cmd;
> >>>>>  	memset(rq->cmd, 0, BLK_MAX_CDB);
> >>>>>  	rq->data_len = 0;
> >>>>>  	rq->extra_len = 0;
> >>>>> diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> >>>>> index 7153796..bac5ea1 100644
> >>>>> --- a/drivers/ide/ide-io.c
> >>>>> +++ b/drivers/ide/ide-io.c
> >>>>> @@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
> >>>>>  {
> >>>>>  	memset(rq, 0, sizeof(*rq));
> >>>>>  	rq->ref_count = 1;
> >>>>> +	rq->cmd = rq->__cmd;
> >>>>>  }
> >>> Tomo, some more changes are needed:
> >>>
> >>> Please think about all _static_/dynamic allocations of 'struct request'
> >>> used together with REQ_TYPE_SPECIAL etc., i.e.
> >> I think that using struct request allocated statically is wrong from
> >> the perspective of the block layer design, that is, you always need to
> >> use blk_get_request. I think that except ide, everyone does.
> >>
> >> I try to convert ide to use blk_get_request properly if you want.
> > 
> > That would be best, but the on-stack allocation has the benefit that
> > it'll always work. So until we can completely get rid of that, lets just
> > make it a hard rule that ANY rq allocation MUST call rq_init(). It's a
> > lot saner than doing a memset() anyway.
> > 
> 
> Just a patch that I had for ages since the bad old request bidi times,
> perhaps is also good today. (rebased to for-2.6.26 branch)
> ---
> From: Boaz Harrosh <bharrosh@panasas.com>
> Date: Wed, 16 Apr 2008 12:05:33 +0300
> Subject: [PATCH] Initialize all members of struct request in rq_init
> 
> Before, every member added/removed from struct request would entitle a change
> to rq_init, for initialization. Now all members are default to zero and only
> the none zero members are specifically initialized.
> 
> Users that need requests on the stack or pre-allocated, must call rq_init()
> before use.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  block/blk-core.c |   22 ++--------------------
>  1 files changed, 2 insertions(+), 20 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 6f0968f..3f4c563 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -113,36 +113,18 @@ EXPORT_SYMBOL(blk_get_backing_dev_info);
>   */
>  void rq_init(struct request_queue *q, struct request *rq)
>  {
> +	memset(rq, 0, sizeof(*rq));

Hmm, rq_init comment says:

/*
 * We can't just memset() the structure, since the allocation path
 * already stored some information in the request.
 */

I think that we can't initialize rq->cmd_flags here.



>  	INIT_LIST_HEAD(&rq->queuelist);
>  	INIT_LIST_HEAD(&rq->donelist);
>  	rq->q = q;
>  	rq->sector = rq->hard_sector = (sector_t) -1;
> -	rq->nr_sectors = rq->hard_nr_sectors = 0;
> -	rq->current_nr_sectors = rq->hard_cur_sectors = 0;
> -	rq->bio = rq->biotail = NULL;
>  	INIT_HLIST_NODE(&rq->hash);
>  	RB_CLEAR_NODE(&rq->rb_node);
> -	rq->rq_disk = NULL;
> -	rq->nr_phys_segments = 0;
> -	rq->nr_hw_segments = 0;
> -	rq->ioprio = 0;
> -	rq->special = NULL;
> -	rq->buffer = NULL;
>  	rq->tag = -1;
> -	rq->errors = 0;
>  	rq->ref_count = 1;
> -	rq->cmd_len = 0;
>  	rq->cmd = rq->__cmd;
> -	memset(rq->cmd, 0, BLK_MAX_CDB);
> -	rq->data_len = 0;
> -	rq->extra_len = 0;
> -	rq->sense_len = 0;
> -	rq->data = NULL;
> -	rq->sense = NULL;
> -	rq->end_io = NULL;
> -	rq->end_io_data = NULL;
> -	rq->next_rq = NULL;
>  }
> +EXPORT_SYMBOL(rq_init);
>  
>  static void req_bio_endio(struct request *rq, struct bio *bio,
>  			  unsigned int nbytes, int error)
> -- 
> 1.5.3.3
> 
> --
> 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 4/4] block: add large command support

[Jens Axboe]

On Thu, Apr 17 2008, FUJITA Tomonori wrote:
> On Wed, 16 Apr 2008 12:08:25 +0300
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
> > On Wed, Apr 16 2008 at 11:33 +0300, Jens Axboe <jens.axboe@oracle.com> wrote:
> > > On Wed, Apr 16 2008, FUJITA Tomonori wrote:
> > >> On Wed, 16 Apr 2008 00:50:54 +0200
> > >> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> > >>
> > >>> Hi,
> > >>>
> > >>> On Monday 14 April 2008, Jens Axboe wrote:
> > >>>> On Mon, Apr 14 2008, FUJITA Tomonori wrote:
> > >>>>> This patch changes rq->cmd from the static array to a pointer to
> > >>>>> support large commands.
> > >>>>>
> > >>>>> We rarely handle large commands. So for optimization, a struct request
> > >>>>> still has a static array for a command. rq_init sets rq->cmd pointer
> > >>>>> to the static array.
> > >>>>>
> > >>>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > >>>>> Cc: Jens Axboe <jens.axboe@oracle.com>
> > >>>>> ---
> > >>>>>  block/blk-core.c       |    1 +
> > >>>>>  drivers/ide/ide-io.c   |    1 +
> > >>>>>  include/linux/blkdev.h |   12 ++++++++++--
> > >>>>>  3 files changed, 12 insertions(+), 2 deletions(-)
> > >>>>>
> > >>>>> diff --git a/block/blk-core.c b/block/blk-core.c
> > >>>>> index 6669238..6f0968f 100644
> > >>>>> --- a/block/blk-core.c
> > >>>>> +++ b/block/blk-core.c
> > >>>>> @@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
> > >>>>>  	rq->errors = 0;
> > >>>>>  	rq->ref_count = 1;
> > >>>>>  	rq->cmd_len = 0;
> > >>>>> +	rq->cmd = rq->__cmd;
> > >>>>>  	memset(rq->cmd, 0, BLK_MAX_CDB);
> > >>>>>  	rq->data_len = 0;
> > >>>>>  	rq->extra_len = 0;
> > >>>>> diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> > >>>>> index 7153796..bac5ea1 100644
> > >>>>> --- a/drivers/ide/ide-io.c
> > >>>>> +++ b/drivers/ide/ide-io.c
> > >>>>> @@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
> > >>>>>  {
> > >>>>>  	memset(rq, 0, sizeof(*rq));
> > >>>>>  	rq->ref_count = 1;
> > >>>>> +	rq->cmd = rq->__cmd;
> > >>>>>  }
> > >>> Tomo, some more changes are needed:
> > >>>
> > >>> Please think about all _static_/dynamic allocations of 'struct request'
> > >>> used together with REQ_TYPE_SPECIAL etc., i.e.
> > >> I think that using struct request allocated statically is wrong from
> > >> the perspective of the block layer design, that is, you always need to
> > >> use blk_get_request. I think that except ide, everyone does.
> > >>
> > >> I try to convert ide to use blk_get_request properly if you want.
> > > 
> > > That would be best, but the on-stack allocation has the benefit that
> > > it'll always work. So until we can completely get rid of that, lets just
> > > make it a hard rule that ANY rq allocation MUST call rq_init(). It's a
> > > lot saner than doing a memset() anyway.
> > > 
> > 
> > Just a patch that I had for ages since the bad old request bidi times,
> > perhaps is also good today. (rebased to for-2.6.26 branch)
> > ---
> > From: Boaz Harrosh <bharrosh@panasas.com>
> > Date: Wed, 16 Apr 2008 12:05:33 +0300
> > Subject: [PATCH] Initialize all members of struct request in rq_init
> > 
> > Before, every member added/removed from struct request would entitle a change
> > to rq_init, for initialization. Now all members are default to zero and only
> > the none zero members are specifically initialized.
> > 
> > Users that need requests on the stack or pre-allocated, must call rq_init()
> > before use.
> > 
> > Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> > ---
> >  block/blk-core.c |   22 ++--------------------
> >  1 files changed, 2 insertions(+), 20 deletions(-)
> > 
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index 6f0968f..3f4c563 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -113,36 +113,18 @@ EXPORT_SYMBOL(blk_get_backing_dev_info);
> >   */
> >  void rq_init(struct request_queue *q, struct request *rq)
> >  {
> > +	memset(rq, 0, sizeof(*rq));
> 
> Hmm, rq_init comment says:
> 
> /*
>  * We can't just memset() the structure, since the allocation path
>  * already stored some information in the request.
>  */
> 
> I think that we can't initialize rq->cmd_flags here.

That is correct, the patch wont work as-is. The principle of clearing
every member except block internal is sound and should be applied,
though.

-- 
Jens Axboe

Re: [PATCH 4/4] block: add large command support

[Bartlomiej Zolnierkiewicz]

On Thu, Apr 17, 2008 at 9:07 AM, Jens Axboe <jens.axboe@oracle.com> wrote:
>
> On Thu, Apr 17 2008, FUJITA Tomonori wrote:
>  > On Wed, 16 Apr 2008 12:08:25 +0300
>  > Boaz Harrosh <bharrosh@panasas.com> wrote:
>  >
>  > > On Wed, Apr 16 2008 at 11:33 +0300, Jens Axboe <jens.axboe@oracle.com> wrote:
>  > > > On Wed, Apr 16 2008, FUJITA Tomonori wrote:
>  > > >> On Wed, 16 Apr 2008 00:50:54 +0200
>  > > >> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
>  > > >>
>  > > >>> Hi,
>  > > >>>
>  > > >>> On Monday 14 April 2008, Jens Axboe wrote:
>  > > >>>> On Mon, Apr 14 2008, FUJITA Tomonori wrote:
>  > > >>>>> This patch changes rq->cmd from the static array to a pointer to
>  > > >>>>> support large commands.
>  > > >>>>>
>  > > >>>>> We rarely handle large commands. So for optimization, a struct request
>  > > >>>>> still has a static array for a command. rq_init sets rq->cmd pointer
>  > > >>>>> to the static array.
>  > > >>>>>
>  > > >>>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
>  > > >>>>> Cc: Jens Axboe <jens.axboe@oracle.com>
>  > > >>>>> ---
>  > > >>>>>  block/blk-core.c       |    1 +
>  > > >>>>>  drivers/ide/ide-io.c   |    1 +
>  > > >>>>>  include/linux/blkdev.h |   12 ++++++++++--
>  > > >>>>>  3 files changed, 12 insertions(+), 2 deletions(-)
>  > > >>>>>
>  > > >>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>  > > >>>>> index 6669238..6f0968f 100644
>  > > >>>>> --- a/block/blk-core.c
>  > > >>>>> +++ b/block/blk-core.c
>  > > >>>>> @@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
>  > > >>>>>       rq->errors = 0;
>  > > >>>>>       rq->ref_count = 1;
>  > > >>>>>       rq->cmd_len = 0;
>  > > >>>>> +     rq->cmd = rq->__cmd;
>  > > >>>>>       memset(rq->cmd, 0, BLK_MAX_CDB);
>  > > >>>>>       rq->data_len = 0;
>  > > >>>>>       rq->extra_len = 0;
>  > > >>>>> diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
>  > > >>>>> index 7153796..bac5ea1 100644
>  > > >>>>> --- a/drivers/ide/ide-io.c
>  > > >>>>> +++ b/drivers/ide/ide-io.c
>  > > >>>>> @@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
>  > > >>>>>  {
>  > > >>>>>       memset(rq, 0, sizeof(*rq));
>  > > >>>>>       rq->ref_count = 1;
>  > > >>>>> +     rq->cmd = rq->__cmd;
>  > > >>>>>  }
>  > > >>> Tomo, some more changes are needed:
>  > > >>>
>  > > >>> Please think about all _static_/dynamic allocations of 'struct request'
>  > > >>> used together with REQ_TYPE_SPECIAL etc., i.e.
>  > > >> I think that using struct request allocated statically is wrong from
>  > > >> the perspective of the block layer design, that is, you always need to
>  > > >> use blk_get_request. I think that except ide, everyone does.
>  > > >>
>  > > >> I try to convert ide to use blk_get_request properly if you want.
>  > > >
>  > > > That would be best, but the on-stack allocation has the benefit that
>  > > > it'll always work. So until we can completely get rid of that, lets just
>  > > > make it a hard rule that ANY rq allocation MUST call rq_init(). It's a
>  > > > lot saner than doing a memset() anyway.
>  > > >
>  > >
>  > > Just a patch that I had for ages since the bad old request bidi times,
>  > > perhaps is also good today. (rebased to for-2.6.26 branch)
>  > > ---
>  > > From: Boaz Harrosh <bharrosh@panasas.com>
>  > > Date: Wed, 16 Apr 2008 12:05:33 +0300
>  > > Subject: [PATCH] Initialize all members of struct request in rq_init
>  > >
>  > > Before, every member added/removed from struct request would entitle a change
>  > > to rq_init, for initialization. Now all members are default to zero and only
>  > > the none zero members are specifically initialized.
>  > >
>  > > Users that need requests on the stack or pre-allocated, must call rq_init()
>  > > before use.
>  > >
>  > > Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>  > > ---
>  > >  block/blk-core.c |   22 ++--------------------
>  > >  1 files changed, 2 insertions(+), 20 deletions(-)
>  > >
>  > > diff --git a/block/blk-core.c b/block/blk-core.c
>  > > index 6f0968f..3f4c563 100644
>  > > --- a/block/blk-core.c
>  > > +++ b/block/blk-core.c
>  > > @@ -113,36 +113,18 @@ EXPORT_SYMBOL(blk_get_backing_dev_info);
>  > >   */
>  > >  void rq_init(struct request_queue *q, struct request *rq)
>  > >  {
>  > > +   memset(rq, 0, sizeof(*rq));
>  >
>  > Hmm, rq_init comment says:
>  >
>  > /*
>  >  * We can't just memset() the structure, since the allocation path
>  >  * already stored some information in the request.
>  >  */
>  >
>  > I think that we can't initialize rq->cmd_flags here.
>
>  That is correct, the patch wont work as-is. The principle of clearing
>  every member except block internal is sound and should be applied,
>  though.

While we're at it we may fix it as well (preferably in another pre-patch):

- move setting rq->cmd_flags from blk_alloc_request() to the callers
  (this is only get_request() ATM)

- re-order setting rq->cmd_flags vs calling rq_init() in get_request(),
  queue_lfush() and  start_ordered()

Thanks,
Bart

Re: [PATCH 4/4] block: add large command support

[Bartlomiej Zolnierkiewicz]

On Thu, Apr 17, 2008 at 1:55 PM, Bartlomiej Zolnierkiewicz
<bzolnier@gmail.com> wrote:
>
> On Thu, Apr 17, 2008 at 9:07 AM, Jens Axboe <jens.axboe@oracle.com> wrote:
>  >
>  > On Thu, Apr 17 2008, FUJITA Tomonori wrote:
>  >  > On Wed, 16 Apr 2008 12:08:25 +0300
>  >  > Boaz Harrosh <bharrosh@panasas.com> wrote:
>  >  >
>  >  > > On Wed, Apr 16 2008 at 11:33 +0300, Jens Axboe <jens.axboe@oracle.com> wrote:
>  >  > > > On Wed, Apr 16 2008, FUJITA Tomonori wrote:
>  >  > > >> On Wed, 16 Apr 2008 00:50:54 +0200
>  >  > > >> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
>  >  > > >>
>  >  > > >>> Hi,
>  >  > > >>>
>  >  > > >>> On Monday 14 April 2008, Jens Axboe wrote:
>  >  > > >>>> On Mon, Apr 14 2008, FUJITA Tomonori wrote:
>  >  > > >>>>> This patch changes rq->cmd from the static array to a pointer to
>  >  > > >>>>> support large commands.
>  >  > > >>>>>
>  >  > > >>>>> We rarely handle large commands. So for optimization, a struct request
>  >  > > >>>>> still has a static array for a command. rq_init sets rq->cmd pointer
>  >  > > >>>>> to the static array.
>  >  > > >>>>>
>  >  > > >>>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
>  >  > > >>>>> Cc: Jens Axboe <jens.axboe@oracle.com>
>  >  > > >>>>> ---
>  >  > > >>>>>  block/blk-core.c       |    1 +
>  >  > > >>>>>  drivers/ide/ide-io.c   |    1 +
>  >  > > >>>>>  include/linux/blkdev.h |   12 ++++++++++--
>  >  > > >>>>>  3 files changed, 12 insertions(+), 2 deletions(-)
>  >  > > >>>>>
>  >  > > >>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>  >  > > >>>>> index 6669238..6f0968f 100644
>  >  > > >>>>> --- a/block/blk-core.c
>  >  > > >>>>> +++ b/block/blk-core.c
>  >  > > >>>>> @@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
>  >  > > >>>>>       rq->errors = 0;
>  >  > > >>>>>       rq->ref_count = 1;
>  >  > > >>>>>       rq->cmd_len = 0;
>  >  > > >>>>> +     rq->cmd = rq->__cmd;
>  >  > > >>>>>       memset(rq->cmd, 0, BLK_MAX_CDB);
>  >  > > >>>>>       rq->data_len = 0;
>  >  > > >>>>>       rq->extra_len = 0;
>  >  > > >>>>> diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
>  >  > > >>>>> index 7153796..bac5ea1 100644
>  >  > > >>>>> --- a/drivers/ide/ide-io.c
>  >  > > >>>>> +++ b/drivers/ide/ide-io.c
>  >  > > >>>>> @@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
>  >  > > >>>>>  {
>  >  > > >>>>>       memset(rq, 0, sizeof(*rq));
>  >  > > >>>>>       rq->ref_count = 1;
>  >  > > >>>>> +     rq->cmd = rq->__cmd;
>  >  > > >>>>>  }
>  >  > > >>> Tomo, some more changes are needed:
>  >  > > >>>
>  >  > > >>> Please think about all _static_/dynamic allocations of 'struct request'
>  >  > > >>> used together with REQ_TYPE_SPECIAL etc., i.e.
>  >  > > >> I think that using struct request allocated statically is wrong from
>  >  > > >> the perspective of the block layer design, that is, you always need to
>  >  > > >> use blk_get_request. I think that except ide, everyone does.
>  >  > > >>
>  >  > > >> I try to convert ide to use blk_get_request properly if you want.
>  >  > > >
>  >  > > > That would be best, but the on-stack allocation has the benefit that
>  >  > > > it'll always work. So until we can completely get rid of that, lets just
>  >  > > > make it a hard rule that ANY rq allocation MUST call rq_init(). It's a
>  >  > > > lot saner than doing a memset() anyway.
>  >  > > >
>  >  > >
>  >  > > Just a patch that I had for ages since the bad old request bidi times,
>  >  > > perhaps is also good today. (rebased to for-2.6.26 branch)
>  >  > > ---
>  >  > > From: Boaz Harrosh <bharrosh@panasas.com>
>  >  > > Date: Wed, 16 Apr 2008 12:05:33 +0300
>  >  > > Subject: [PATCH] Initialize all members of struct request in rq_init
>  >  > >
>  >  > > Before, every member added/removed from struct request would entitle a change
>  >  > > to rq_init, for initialization. Now all members are default to zero and only
>  >  > > the none zero members are specifically initialized.
>  >  > >
>  >  > > Users that need requests on the stack or pre-allocated, must call rq_init()
>  >  > > before use.
>  >  > >
>  >  > > Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>  >  > > ---
>  >  > >  block/blk-core.c |   22 ++--------------------
>  >  > >  1 files changed, 2 insertions(+), 20 deletions(-)
>  >  > >
>  >  > > diff --git a/block/blk-core.c b/block/blk-core.c
>  >  > > index 6f0968f..3f4c563 100644
>  >  > > --- a/block/blk-core.c
>  >  > > +++ b/block/blk-core.c
>  >  > > @@ -113,36 +113,18 @@ EXPORT_SYMBOL(blk_get_backing_dev_info);
>  >  > >   */
>  >  > >  void rq_init(struct request_queue *q, struct request *rq)
>  >  > >  {
>  >  > > +   memset(rq, 0, sizeof(*rq));
>  >  >
>  >  > Hmm, rq_init comment says:
>  >  >
>  >  > /*
>  >  >  * We can't just memset() the structure, since the allocation path
>  >  >  * already stored some information in the request.
>  >  >  */
>  >  >
>  >  > I think that we can't initialize rq->cmd_flags here.
>  >
>  >  That is correct, the patch wont work as-is. The principle of clearing
>  >  every member except block internal is sound and should be applied,
>  >  though.
>
>  While we're at it we may fix it as well (preferably in another pre-patch):
>
>  - move setting rq->cmd_flags from blk_alloc_request() to the callers
>   (this is only get_request() ATM)

ditto for elv_set_request() call

>  - re-order setting rq->cmd_flags vs calling rq_init() in get_request(),
>   queue_lfush() and  start_ordered()

Re: [PATCH 4/4] block: add large command support

[FUJITA Tomonori]

On Thu, 17 Apr 2008 09:07:05 +0200
Jens Axboe <jens.axboe@oracle.com> wrote:

> On Thu, Apr 17 2008, FUJITA Tomonori wrote:
> > On Wed, 16 Apr 2008 12:08:25 +0300
> > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > 
> > > On Wed, Apr 16 2008 at 11:33 +0300, Jens Axboe <jens.axboe@oracle.com> wrote:
> > > > On Wed, Apr 16 2008, FUJITA Tomonori wrote:
> > > >> On Wed, 16 Apr 2008 00:50:54 +0200
> > > >> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> > > >>
> > > >>> Hi,
> > > >>>
> > > >>> On Monday 14 April 2008, Jens Axboe wrote:
> > > >>>> On Mon, Apr 14 2008, FUJITA Tomonori wrote:
> > > >>>>> This patch changes rq->cmd from the static array to a pointer to
> > > >>>>> support large commands.
> > > >>>>>
> > > >>>>> We rarely handle large commands. So for optimization, a struct request
> > > >>>>> still has a static array for a command. rq_init sets rq->cmd pointer
> > > >>>>> to the static array.
> > > >>>>>
> > > >>>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > >>>>> Cc: Jens Axboe <jens.axboe@oracle.com>
> > > >>>>> ---
> > > >>>>>  block/blk-core.c       |    1 +
> > > >>>>>  drivers/ide/ide-io.c   |    1 +
> > > >>>>>  include/linux/blkdev.h |   12 ++++++++++--
> > > >>>>>  3 files changed, 12 insertions(+), 2 deletions(-)
> > > >>>>>
> > > >>>>> diff --git a/block/blk-core.c b/block/blk-core.c
> > > >>>>> index 6669238..6f0968f 100644
> > > >>>>> --- a/block/blk-core.c
> > > >>>>> +++ b/block/blk-core.c
> > > >>>>> @@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
> > > >>>>>  	rq->errors = 0;
> > > >>>>>  	rq->ref_count = 1;
> > > >>>>>  	rq->cmd_len = 0;
> > > >>>>> +	rq->cmd = rq->__cmd;
> > > >>>>>  	memset(rq->cmd, 0, BLK_MAX_CDB);
> > > >>>>>  	rq->data_len = 0;
> > > >>>>>  	rq->extra_len = 0;
> > > >>>>> diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> > > >>>>> index 7153796..bac5ea1 100644
> > > >>>>> --- a/drivers/ide/ide-io.c
> > > >>>>> +++ b/drivers/ide/ide-io.c
> > > >>>>> @@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
> > > >>>>>  {
> > > >>>>>  	memset(rq, 0, sizeof(*rq));
> > > >>>>>  	rq->ref_count = 1;
> > > >>>>> +	rq->cmd = rq->__cmd;
> > > >>>>>  }
> > > >>> Tomo, some more changes are needed:
> > > >>>
> > > >>> Please think about all _static_/dynamic allocations of 'struct request'
> > > >>> used together with REQ_TYPE_SPECIAL etc., i.e.
> > > >> I think that using struct request allocated statically is wrong from
> > > >> the perspective of the block layer design, that is, you always need to
> > > >> use blk_get_request. I think that except ide, everyone does.
> > > >>
> > > >> I try to convert ide to use blk_get_request properly if you want.
> > > > 
> > > > That would be best, but the on-stack allocation has the benefit that
> > > > it'll always work. So until we can completely get rid of that, lets just
> > > > make it a hard rule that ANY rq allocation MUST call rq_init(). It's a
> > > > lot saner than doing a memset() anyway.
> > > > 
> > > 
> > > Just a patch that I had for ages since the bad old request bidi times,
> > > perhaps is also good today. (rebased to for-2.6.26 branch)
> > > ---
> > > From: Boaz Harrosh <bharrosh@panasas.com>
> > > Date: Wed, 16 Apr 2008 12:05:33 +0300
> > > Subject: [PATCH] Initialize all members of struct request in rq_init
> > > 
> > > Before, every member added/removed from struct request would entitle a change
> > > to rq_init, for initialization. Now all members are default to zero and only
> > > the none zero members are specifically initialized.
> > > 
> > > Users that need requests on the stack or pre-allocated, must call rq_init()
> > > before use.
> > > 
> > > Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> > > ---
> > >  block/blk-core.c |   22 ++--------------------
> > >  1 files changed, 2 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/block/blk-core.c b/block/blk-core.c
> > > index 6f0968f..3f4c563 100644
> > > --- a/block/blk-core.c
> > > +++ b/block/blk-core.c
> > > @@ -113,36 +113,18 @@ EXPORT_SYMBOL(blk_get_backing_dev_info);
> > >   */
> > >  void rq_init(struct request_queue *q, struct request *rq)
> > >  {
> > > +	memset(rq, 0, sizeof(*rq));
> > 
> > Hmm, rq_init comment says:
> > 
> > /*
> >  * We can't just memset() the structure, since the allocation path
> >  * already stored some information in the request.
> >  */
> > 
> > I think that we can't initialize rq->cmd_flags here.
> 
> That is correct, the patch wont work as-is. The principle of clearing
> every member except block internal is sound and should be applied,
> though.

1) calls rq_init() and initializes rq->cmd_flags manually.
2) adds a new helper function to just memset() against rq.
3) modifies the block layer so that rq_init() can memset() against rq.
4) uses blk_get_request

I think that the second option would be the best for now but I don't
have a good name for such function.

The third or fourth option looks preferable for me in the long term
(probably, for 2.6.27).

Here's a patch to initialize rq->cmd by hand. I think that it's ok for
now.

drivers/scsi_errro.c doesn't need this hack now (since it doesn't use
struct request) but pending Boaz's extended cdbs patch needs this hack
since it removes scmd->cmnd array and uses rq->cmd.

===
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: [PATCH] block: initialize the cmd pointer in struct request

struct request that is not allocated via blk_get_request needs to
set up the cmd pointer.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
---
 drivers/block/nbd.c        |    3 +++
 drivers/block/paride/pd.c  |    1 +
 drivers/ide/ide-tape.c     |    1 +
 drivers/ide/ide-taskfile.c |    3 +--
 drivers/ide/ide.c          |    2 ++
 drivers/scsi/scsi_error.c  |    1 +
 6 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 60cc543..ebc653d 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -534,6 +534,9 @@ static int nbd_ioctl(struct inode *inode, struct file *file,
 	dprintk(DBG_IOCTL, "%s: nbd_ioctl cmd=%s(0x%x) arg=%lu\n",
 			lo->disk->disk_name, ioctl_cmd_to_ascii(cmd), cmd, arg);
 
+	memset(&sreq, 0, sizeof(sreq));
+	sreq.cmd = sreq.__cmd;
+
 	switch (cmd) {
 	case NBD_DISCONNECT:
 	        printk(KERN_INFO "%s: NBD_DISCONNECT\n", lo->disk->disk_name);
diff --git a/drivers/block/paride/pd.c b/drivers/block/paride/pd.c
index df819f8..e82a669 100644
--- a/drivers/block/paride/pd.c
+++ b/drivers/block/paride/pd.c
@@ -717,6 +717,7 @@ static int pd_special_command(struct pd_unit *disk,
 	int err = 0;
 
 	memset(&rq, 0, sizeof(rq));
+	rq.cmd = rq.__cmd;
 	rq.errors = 0;
 	rq.rq_disk = disk->gd;
 	rq.ref_count = 1;
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 0598ecf..d9c0267 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -946,6 +946,7 @@ static void idetape_create_request_sense_cmd(idetape_pc_t *pc)
 static void idetape_init_rq(struct request *rq, u8 cmd)
 {
 	memset(rq, 0, sizeof(*rq));
+	rq->cmd = rq->__cmd;
 	rq->cmd_type = REQ_TYPE_SPECIAL;
 	rq->cmd[0] = cmd;
 }
diff --git a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
index 4c86a8d..1c30a57 100644
--- a/drivers/ide/ide-taskfile.c
+++ b/drivers/ide/ide-taskfile.c
@@ -529,8 +529,7 @@ int ide_raw_taskfile(ide_drive_t *drive, ide_task_t *task, u8 *buf, u16 nsect)
 {
 	struct request rq;
 
-	memset(&rq, 0, sizeof(rq));
-	rq.ref_count = 1;
+	ide_init_drive_cmd(&rq);
 	rq.cmd_type = REQ_TYPE_ATA_TASKFILE;
 	rq.buffer = buf;
 
diff --git a/drivers/ide/ide.c b/drivers/ide/ide.c
index fc69fe2..96aaec2 100644
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -881,6 +881,7 @@ static int generic_ide_suspend(struct device *dev, pm_message_t mesg)
 		ide_acpi_get_timing(hwif);
 
 	memset(&rq, 0, sizeof(rq));
+	rq.cmd = rq.__cmd;
 	memset(&rqpm, 0, sizeof(rqpm));
 	memset(&args, 0, sizeof(args));
 	rq.cmd_type = REQ_TYPE_PM_SUSPEND;
@@ -919,6 +920,7 @@ static int generic_ide_resume(struct device *dev)
 	ide_acpi_exec_tfs(drive);
 
 	memset(&rq, 0, sizeof(rq));
+	rq.cmd = rq.__cmd;
 	memset(&rqpm, 0, sizeof(rqpm));
 	memset(&args, 0, sizeof(args));
 	rq.cmd_type = REQ_TYPE_PM_RESUME;
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 045a086..020b678 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1690,6 +1690,7 @@ scsi_reset_provider(struct scsi_device *dev, int flag)
 	unsigned long flags;
 	int rtn;
 
+	req.cmd = req.__cmd;
 	scmd->request = &req;
 	memset(&scmd->eh_timeout, 0, sizeof(scmd->eh_timeout));
 
-- 
1.5.4.2

Re: [PATCH 4/4] block: add large command support

[FUJITA Tomonori]

On Wed, 16 Apr 2008 10:33:06 +0200
Jens Axboe <jens.axboe@oracle.com> wrote:

> On Wed, Apr 16 2008, FUJITA Tomonori wrote:
> > On Wed, 16 Apr 2008 00:50:54 +0200
> > Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> > 
> > > 
> > > Hi,
> > > 
> > > On Monday 14 April 2008, Jens Axboe wrote:
> > > > On Mon, Apr 14 2008, FUJITA Tomonori wrote:
> > > > > This patch changes rq->cmd from the static array to a pointer to
> > > > > support large commands.
> > > > > 
> > > > > We rarely handle large commands. So for optimization, a struct request
> > > > > still has a static array for a command. rq_init sets rq->cmd pointer
> > > > > to the static array.
> > > > > 
> > > > > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > > > > Cc: Jens Axboe <jens.axboe@oracle.com>
> > > > > ---
> > > > >  block/blk-core.c       |    1 +
> > > > >  drivers/ide/ide-io.c   |    1 +
> > > > >  include/linux/blkdev.h |   12 ++++++++++--
> > > > >  3 files changed, 12 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/block/blk-core.c b/block/blk-core.c
> > > > > index 6669238..6f0968f 100644
> > > > > --- a/block/blk-core.c
> > > > > +++ b/block/blk-core.c
> > > > > @@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
> > > > >  	rq->errors = 0;
> > > > >  	rq->ref_count = 1;
> > > > >  	rq->cmd_len = 0;
> > > > > +	rq->cmd = rq->__cmd;
> > > > >  	memset(rq->cmd, 0, BLK_MAX_CDB);
> > > > >  	rq->data_len = 0;
> > > > >  	rq->extra_len = 0;
> > > > > diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
> > > > > index 7153796..bac5ea1 100644
> > > > > --- a/drivers/ide/ide-io.c
> > > > > +++ b/drivers/ide/ide-io.c
> > > > > @@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
> > > > >  {
> > > > >  	memset(rq, 0, sizeof(*rq));
> > > > >  	rq->ref_count = 1;
> > > > > +	rq->cmd = rq->__cmd;
> > > > >  }
> > > 
> > > Tomo, some more changes are needed:
> > > 
> > > Please think about all _static_/dynamic allocations of 'struct request'
> > > used together with REQ_TYPE_SPECIAL etc., i.e.
> > 
> > I think that using struct request allocated statically is wrong from
> > the perspective of the block layer design, that is, you always need to
> > use blk_get_request. I think that except ide, everyone does.
> > 
> > I try to convert ide to use blk_get_request properly if you want.
> 
> That would be best, but the on-stack allocation has the benefit that
> it'll always work. So until we can completely get rid of that, lets just
> make it a hard rule that ANY rq allocation MUST call rq_init(). It's a
> lot saner than doing a memset() anyway.

I'm fine with that.

There is one minor issue. rq_init doesn't initialize rq->cmd_flags so
the callers need to do it for themselves (If they don't, probably they
hit BUG_ON in blk_queue_end_tag).

Re: [PATCH 4/4] block: add large command support

[Pete Wyckoff]

fujita.tomonori@lab.ntt.co.jp wrote on Mon, 14 Apr 2008 19:50 +0900:
> This patch changes rq->cmd from the static array to a pointer to
> support large commands.
> 
> We rarely handle large commands. So for optimization, a struct request
> still has a static array for a command. rq_init sets rq->cmd pointer
> to the static array.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Cc: Jens Axboe <jens.axboe@oracle.com>
[..]
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index b3a58ad..5710ae4 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -215,8 +215,9 @@ struct request {
>  	/*
>  	 * when request is used as a packet command carrier
>  	 */
> -	unsigned int cmd_len;
> -	unsigned char cmd[BLK_MAX_CDB];
> +	unsigned short cmd_len;
> +	unsigned char __cmd[BLK_MAX_CDB];
> +	unsigned char *cmd;
>  
>  	unsigned int data_len;
>  	unsigned int extra_len;	/* length of alignment and padding */
> @@ -812,6 +813,13 @@ static inline void put_dev_sector(Sector p)
>  	page_cache_release(p.v);
>  }
>  
> +static inline void rq_set_cmd(struct request *rq, unsigned char *cmd,
> +			      unsigned short cmd_len)
> +{
> +	rq->cmd = cmd;
> +	rq->cmd_len = cmd_len;
> +}

Here's one way this will be used, in a patched bsg that understands
large commands.  Complication is the need to copy and hold onto the
big command across the duration of the request.

Submit time is fairly clean:

	/* buf, len from user request */
	rq = blk_get_request(..);
	rq->cmd_len = len;
	if (len > BLK_MAX_CDB) {
		rq->cmd = kmalloc(len);
		if (rq->cmd == NULL)
			goto out;
	}
	copy_from_user(rq->cmd, buf, len);

Completion time needs to know when to free rq->cmd:

	if (rq->cmd_len > BLK_MAX_CDB)
		kfree(rq->cmd);
	blk_put_request(rq);

Could use (rq->cmd != rq->__cmd) instead, but nothing had better
ever touch rq->cmd_len.

I don't think the helper rq_set_cmd() will be very useful, as the
caller (bsg) must think about allocation of the command buffer if it
is big.

One option would be to handle allocation/freeing of the big command
in rq_set_... functions, but I don't think you want to constrain the
interface like that.

Boaz's concern about big rq->cmd_len still worries me, although I
think this approach is better and worth solving bugs in drivers as
they arise.  It only matters in the case that someone adds, say, a
bsg interface to all block devices though.  The queuecommand of ub
shows a good example of how this will break.

In sum, this is a cleaner approach, and a bit easier for callers
with long commands to deal with.  And you could get rid of the
trivial helper.

		-- Pete

Re: [PATCH 4/4] block: add large command support

[FUJITA Tomonori]

On Mon, 14 Apr 2008 10:41:54 -0400
Pete Wyckoff <pw@osc.edu> wrote:

> fujita.tomonori@lab.ntt.co.jp wrote on Mon, 14 Apr 2008 19:50 +0900:
> > This patch changes rq->cmd from the static array to a pointer to
> > support large commands.
> > 
> > We rarely handle large commands. So for optimization, a struct request
> > still has a static array for a command. rq_init sets rq->cmd pointer
> > to the static array.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Cc: Jens Axboe <jens.axboe@oracle.com>
> [..]
> > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > index b3a58ad..5710ae4 100644
> > --- a/include/linux/blkdev.h
> > +++ b/include/linux/blkdev.h
> > @@ -215,8 +215,9 @@ struct request {
> >  	/*
> >  	 * when request is used as a packet command carrier
> >  	 */
> > -	unsigned int cmd_len;
> > -	unsigned char cmd[BLK_MAX_CDB];
> > +	unsigned short cmd_len;
> > +	unsigned char __cmd[BLK_MAX_CDB];
> > +	unsigned char *cmd;
> >  
> >  	unsigned int data_len;
> >  	unsigned int extra_len;	/* length of alignment and padding */
> > @@ -812,6 +813,13 @@ static inline void put_dev_sector(Sector p)
> >  	page_cache_release(p.v);
> >  }
> >  
> > +static inline void rq_set_cmd(struct request *rq, unsigned char *cmd,
> > +			      unsigned short cmd_len)
> > +{
> > +	rq->cmd = cmd;
> > +	rq->cmd_len = cmd_len;
> > +}
> 
> Here's one way this will be used, in a patched bsg that understands
> large commands.  Complication is the need to copy and hold onto the
> big command across the duration of the request.
> 
> Submit time is fairly clean:
> 
> 	/* buf, len from user request */
> 	rq = blk_get_request(..);
> 	rq->cmd_len = len;
> 	if (len > BLK_MAX_CDB) {
> 		rq->cmd = kmalloc(len);
> 		if (rq->cmd == NULL)
> 			goto out;
> 	}
> 	copy_from_user(rq->cmd, buf, len);
> 
> Completion time needs to know when to free rq->cmd:
> 
> 	if (rq->cmd_len > BLK_MAX_CDB)
> 		kfree(rq->cmd);
> 	blk_put_request(rq);
> 
> Could use (rq->cmd != rq->__cmd) instead, but nothing had better
> ever touch rq->cmd_len.
> 
> I don't think the helper rq_set_cmd() will be very useful, as the
> caller (bsg) must think about allocation of the command buffer if it
> is big.

Yeah, I think so. If you need large command support, you need to know
how to handle it for now. Now only bsg supports large commands. So I
guess that nobody complains about the current interface.


> One option would be to handle allocation/freeing of the big command
> in rq_set_... functions, but I don't think you want to constrain the
> interface like that.

Or, you can change blk_get_request to take the command length
argument. But I don't think that such is the right approach.


> Boaz's concern about big rq->cmd_len still worries me, although I
> think this approach is better and worth solving bugs in drivers as
> they arise.  It only matters in the case that someone adds, say, a
> bsg interface to all block devices though.  The queuecommand of ub
> shows a good example of how this will break.

Yes, a bsg hook will need to handle large commands. I don't see any
problem about it. All a hook needs to do is just looking at the legnth
and dropping or executing the command. And of course, we are unlikely
to add a bsg device to all the block devices.

As I said, if we want to govern the command length in a common place,
we can have the limit of the command length in request queues. It's
clear than an implicit checking with two lengths, cmd_len and
ext_cdb_len.

I thought about adding the code to check the command length in UB. But
I thought that we were unlikely to create bsg devices for ub. Common
people use USB_STORAGE rather than UB, I guess.


> In sum, this is a cleaner approach, and a bit easier for callers
> with long commands to deal with.  And you could get rid of the
> trivial helper.

Yeah, it's a cleaner design, that's main point, I think.


BTW, have you had a chance to try the bsg patches to fix the problems
that you reported? I think that Mike and I analyzed the problem
correctly and the patches works for me, but it would be nice if you
can confirm that they also work for you.

Thanks,

Re: [PATCH 4/4] block: add large command support

[Pete Wyckoff]

fujita.tomonori@lab.ntt.co.jp wrote on Tue, 15 Apr 2008 07:33 +0900:
> BTW, have you had a chance to try the bsg patches to fix the problems
> that you reported? I think that Mike and I analyzed the problem
> correctly and the patches works for me, but it would be nice if you
> can confirm that they also work for you.

It's on my TODO list.  Machines were down and papers are due.  But I
hope you can push the patches as bug fixes anyway.

		-- Pete

Re: [PATCH 4/4] block: add large command support

[Boaz Harrosh]

On Mon, Apr 14 2008 at 17:41 +0300, Pete Wyckoff <pw@osc.edu> wrote:
> fujita.tomonori@lab.ntt.co.jp wrote on Mon, 14 Apr 2008 19:50 +0900:
>> This patch changes rq->cmd from the static array to a pointer to
>> support large commands.
>>
>> We rarely handle large commands. So for optimization, a struct request
>> still has a static array for a command. rq_init sets rq->cmd pointer
>> to the static array.
>>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
>> Cc: Jens Axboe <jens.axboe@oracle.com>
> [..]
>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>> index b3a58ad..5710ae4 100644
>> --- a/include/linux/blkdev.h
>> +++ b/include/linux/blkdev.h
>> @@ -215,8 +215,9 @@ struct request {
>>  	/*
>>  	 * when request is used as a packet command carrier
>>  	 */
>> -	unsigned int cmd_len;
>> -	unsigned char cmd[BLK_MAX_CDB];
>> +	unsigned short cmd_len;
>> +	unsigned char __cmd[BLK_MAX_CDB];
>> +	unsigned char *cmd;
>>  
>>  	unsigned int data_len;
>>  	unsigned int extra_len;	/* length of alignment and padding */
>> @@ -812,6 +813,13 @@ static inline void put_dev_sector(Sector p)
>>  	page_cache_release(p.v);
>>  }
>>  
>> +static inline void rq_set_cmd(struct request *rq, unsigned char *cmd,
>> +			      unsigned short cmd_len)
>> +{
>> +	rq->cmd = cmd;
>> +	rq->cmd_len = cmd_len;
>> +}
> 
> Here's one way this will be used, in a patched bsg that understands
> large commands.  Complication is the need to copy and hold onto the
> big command across the duration of the request.
> 
> Submit time is fairly clean:
> 
> 	/* buf, len from user request */
> 	rq = blk_get_request(..);
> 	rq->cmd_len = len;
> 	if (len > BLK_MAX_CDB) {
> 		rq->cmd = kmalloc(len);
> 		if (rq->cmd == NULL)
> 			goto out;
> 	}
> 	copy_from_user(rq->cmd, buf, len);
> 
> Completion time needs to know when to free rq->cmd:
> 
> 	if (rq->cmd_len > BLK_MAX_CDB)
> 		kfree(rq->cmd);
> 	blk_put_request(rq);
> 
> Could use (rq->cmd != rq->__cmd) instead, but nothing had better
> ever touch rq->cmd_len.
> 

Don't do any of that please. The 16 bytes buffer at request is eventually
going away. Just always allocate and call rq_set_cmd(). This way we are
free to change in the future, and users code need not change.

And better then kmalloc the space for the command, is if you have
a structure that governs your request, just allocate a 
SCSI_MAX_VARLEN_CDB_SIZE (260) bytes array at your structure and always
use that.

> I don't think the helper rq_set_cmd() will be very useful, as the
> caller (bsg) must think about allocation of the command buffer if it
> is big.
> 
> One option would be to handle allocation/freeing of the big command
> in rq_set_... functions, but I don't think you want to constrain the
> interface like that.
> 
> Boaz's concern about big rq->cmd_len still worries me, although I
> think this approach is better and worth solving bugs in drivers as
> they arise.  It only matters in the case that someone adds, say, a
> bsg interface to all block devices though.  The queuecommand of ub
> shows a good example of how this will break.
> 
> In sum, this is a cleaner approach, and a bit easier for callers
> with long commands to deal with.  And you could get rid of the
> trivial helper.
> 

Don't. Please use the set helper it will give us future compatibility.

> 		-- Pete
> --

Boaz

Re: [PATCH 4/4] block: add large command support

[FUJITA Tomonori]

On Tue, 15 Apr 2008 10:45:52 +0300
Boaz Harrosh <bharrosh@panasas.com> wrote:

> On Mon, Apr 14 2008 at 17:41 +0300, Pete Wyckoff <pw@osc.edu> wrote:
> > fujita.tomonori@lab.ntt.co.jp wrote on Mon, 14 Apr 2008 19:50 +0900:
> >> This patch changes rq->cmd from the static array to a pointer to
> >> support large commands.
> >>
> >> We rarely handle large commands. So for optimization, a struct request
> >> still has a static array for a command. rq_init sets rq->cmd pointer
> >> to the static array.
> >>
> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> >> Cc: Jens Axboe <jens.axboe@oracle.com>
> > [..]
> >> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> >> index b3a58ad..5710ae4 100644
> >> --- a/include/linux/blkdev.h
> >> +++ b/include/linux/blkdev.h
> >> @@ -215,8 +215,9 @@ struct request {
> >>  	/*
> >>  	 * when request is used as a packet command carrier
> >>  	 */
> >> -	unsigned int cmd_len;
> >> -	unsigned char cmd[BLK_MAX_CDB];
> >> +	unsigned short cmd_len;
> >> +	unsigned char __cmd[BLK_MAX_CDB];
> >> +	unsigned char *cmd;
> >>  
> >>  	unsigned int data_len;
> >>  	unsigned int extra_len;	/* length of alignment and padding */
> >> @@ -812,6 +813,13 @@ static inline void put_dev_sector(Sector p)
> >>  	page_cache_release(p.v);
> >>  }
> >>  
> >> +static inline void rq_set_cmd(struct request *rq, unsigned char *cmd,
> >> +			      unsigned short cmd_len)
> >> +{
> >> +	rq->cmd = cmd;
> >> +	rq->cmd_len = cmd_len;
> >> +}
> > 
> > Here's one way this will be used, in a patched bsg that understands
> > large commands.  Complication is the need to copy and hold onto the
> > big command across the duration of the request.
> > 
> > Submit time is fairly clean:
> > 
> > 	/* buf, len from user request */
> > 	rq = blk_get_request(..);
> > 	rq->cmd_len = len;
> > 	if (len > BLK_MAX_CDB) {
> > 		rq->cmd = kmalloc(len);
> > 		if (rq->cmd == NULL)
> > 			goto out;
> > 	}
> > 	copy_from_user(rq->cmd, buf, len);
> > 
> > Completion time needs to know when to free rq->cmd:
> > 
> > 	if (rq->cmd_len > BLK_MAX_CDB)
> > 		kfree(rq->cmd);
> > 	blk_put_request(rq);
> > 
> > Could use (rq->cmd != rq->__cmd) instead, but nothing had better
> > ever touch rq->cmd_len.
> > 
> 
> Don't do any of that please. The 16 bytes buffer at request is eventually
> going away. Just always allocate and call rq_set_cmd(). This way we are
> free to change in the future, and users code need not change.

Hmm, it's clean for bsg, but we nearly always allocates 16 bytes
dynamically. That's wasteful. Probably, we would loose some
performance as we did for sense buffer.


> And better then kmalloc the space for the command, is if you have
> a structure that governs your request, just allocate a 
> SCSI_MAX_VARLEN_CDB_SIZE (260) bytes array at your structure and always
> use that.

I don't put 260 bytes into bsg_command structure. I don't see why bsg
needs to be tuned up for OSD or for SCSI commands.

Re: [PATCH 4/4] block: add large command support

[Jens Axboe]

On Mon, Apr 14 2008, FUJITA Tomonori wrote:
> This patch changes rq->cmd from the static array to a pointer to
> support large commands.
> 
> We rarely handle large commands. So for optimization, a struct request
> still has a static array for a command. rq_init sets rq->cmd pointer
> to the static array.

I applied 1-4, thanks.

-- 
Jens Axboe