On Wednesday 16 April 2008, FUJITA Tomonori wrote:
> 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.
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
--
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
