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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 16 Apr 2008 00:50:54 +0200
Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > Cc: Jens Axboe <jens.axboe@xxxxxxxxxx>
> > > ---
> > >  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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux