On Wed, 15 Apr 2009 09:26:55 +0200 Borislav Petkov <petkovbb@xxxxxxxxxxxxxx> wrote: > Hi guys, > > On Wed, Apr 15, 2009 at 01:25:04PM +0900, Tejun Heo wrote: > > >> Basically, I opted for preallocating a sense request in the ->do_request > > >> routine and do that only on demand, i.e. I reinitialize it only if it > > >> got used in the irq handler. So in case you want to shove a rq sense in > > >> front of the queue, you simply use the already prepared one. Then in the > > >> irq handler it is being finished the usual ways (blk_end_request). Next > > >> time around you ->do_request, you reallocate it again since it got eaten > > >> in the last round. > > > > > > Sounds a workable solution. > > > > Haven't actually looked at the code but sweeeeeet. > > > > >> The good thing is that now I don't need all those static block layer > > >> structs in the driver (bio, bio_vec, etc) and do the preferred dynamic > > >> allocation instead. > > > > > > That's surely good. > > > > > > Well, if you could remove the usage of request structure that are not > > > came from blk_get_request, it will be super. But it's a different > > > topic and Tejun can go forward without such change. > > > > > >> The patch is ontop of Tejun's series at > > >> http://git.kernel.org/?p=linux/kernel/git/tj/misc.git;a=shortlog;h=ide-phase1 > > >> with some small modifications in commit 15783b1443f810ae72cb5ccb3a3a3ccc3aeb8729 > > >> wrt proper sense buffer length. > > > > > > I think that Tejun will drop some of the patchset. At least, we don't > > > need blk_rq_map_kern_prealloc stuff. I think that Tejun doesn't need > > > to play with the mapping API. Well, we need to play with the mapping > > > API for OSD but it's not directly related with the block layer > > > cleanups necessary for the libata SCSI separation. > > > > Yeah, the blk_rq_map_kern_prealloc() was basically shifting rq map > > from ide to blk/bio so that at least codes are all in one place. If > > it's not necessary, super. :-) > > > > I'll drop stuff from this and the other patchset and repost them with > > Borislav's patch in a few hours. Thanks guys. > > here's a version which gets rid of the static drive->request_sense_rq > structure and does the usual blk_get_request(), as Fujita suggested. > > @Tejun: we're gonna need the same thing for ide-atapi before you'll be > able to get rid of the _prealloc() hack. I'll try to cook something up by > tomorrow the latest. > > --- > From: Borislav Petkov <petkovbb@xxxxxxxxx> > Date: Tue, 14 Apr 2009 13:24:43 +0200 > Subject: [PATCH] ide-cd: preallocate rq sense out of the irq path > > Preallocate a sense request in the ->do_request method and reinitialize > it only on demand, in case it's been consumed in the IRQ handler path. > The reason for this is that we don't want to be mapping rq to bio in > the IRQ path and introduce all kinds of unnecessary hacks to the block > layer. > > CC: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx> > CC: FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> > CC: Tejun Heo <tj@xxxxxxxxxx> > Signed-off-by: Borislav Petkov <petkovbb@xxxxxxxxx> > --- > drivers/ide/ide-cd.c | 67 +++++++++++++++++++++++++++++--------------------- > include/linux/ide.h | 3 ++ > 2 files changed, 42 insertions(+), 28 deletions(-) Great, thanks! I have some comments. > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c > index 35d0973..82c9339 100644 > --- a/drivers/ide/ide-cd.c > +++ b/drivers/ide/ide-cd.c > @@ -206,32 +206,21 @@ static void cdrom_analyze_sense_data(ide_drive_t *drive, > ide_cd_log_error(drive->name, failed_command, sense); > } > > -static void cdrom_queue_request_sense(ide_drive_t *drive, void *sense, > - struct request *failed_command) > +static struct request *ide_cd_prep_sense(ide_drive_t *drive) > { > struct cdrom_info *info = drive->driver_data; > - struct request *rq = &drive->request_sense_rq; > - struct bio *bio = &drive->request_sense_bio; > - struct bio_vec *bvec = drive->request_sense_bvec; > - unsigned int bvec_len = ARRAY_SIZE(drive->request_sense_bvec); > - unsigned sense_len = 18; > - int error; > + void *sense = &info->sense_data; > + unsigned sense_len = sizeof(struct request_sense); > + struct request *rq; > > ide_debug_log(IDE_DBG_SENSE, "enter"); > > - if (sense == NULL) { > - sense = &info->sense_data; > - sense_len = sizeof(struct request_sense); > - } > - > memset(sense, 0, sense_len); > > - /* stuff the sense request in front of our current request */ > - blk_rq_init(NULL, rq); > + rq = blk_get_request(drive->queue, 0, __GFP_WAIT); > > - error = blk_rq_map_kern_prealloc(drive->queue, rq, bio, bvec, bvec_len, > - sense, sense_len, true); > - BUG_ON(error); > + if (blk_rq_map_kern(drive->queue, rq, sense, sense_len, __GFP_WAIT)) > + return NULL; > > rq->rq_disk = info->disk; > > @@ -241,18 +230,17 @@ static void cdrom_queue_request_sense(ide_drive_t *drive, void *sense, > rq->cmd_type = REQ_TYPE_SENSE; > rq->cmd_flags |= REQ_PREEMPT; > > - /* NOTE! Save the failed command in "rq->special" */ > - rq->special = (void *)failed_command; > - > - if (failed_command) > - ide_debug_log(IDE_DBG_SENSE, "failed_cmd: 0x%x", > - failed_command->cmd[0]); > + return rq; > +} > > - drive->hwif->rq = NULL; > +static void ide_cd_queue_rq_sense(ide_drive_t *drive) > +{ > + BUG_ON(!drive->rq_sense); > > - elv_add_request(drive->queue, rq, ELEVATOR_INSERT_FRONT, 0); > + elv_add_request(drive->queue, drive->rq_sense, ELEVATOR_INSERT_FRONT, 0); > } > > + > static void ide_cd_complete_failed_rq(ide_drive_t *drive, struct request *rq) > { > /* > @@ -440,7 +428,7 @@ static int cdrom_decode_status(ide_drive_t *drive, u8 stat) > > /* if we got a CHECK_CONDITION status, queue a request sense command */ > if (stat & ATA_ERR) > - cdrom_queue_request_sense(drive, NULL, NULL); > + ide_cd_queue_rq_sense(drive); > return 1; > > end_request: > @@ -454,7 +442,7 @@ end_request: > > hwif->rq = NULL; > > - cdrom_queue_request_sense(drive, rq->sense, rq); > + ide_cd_queue_rq_sense(drive); > return 1; > } else > return 2; > @@ -788,6 +776,10 @@ out_end: > > ide_complete_rq(drive, uptodate ? 0 : -EIO, nsectors << 9); > > + /* our sense buffer got used, reset it the next time around. */ > + if (sense) > + drive->rq_sense = NULL; Needs to call blk_put_request() here? I guess that we also need to call blk_put_request() when ide_drive_s is freed. > + > if (sense && rc == 2) > ide_error(drive, "request sense failure", stat); > } > @@ -901,6 +893,25 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq, > goto out_end; > } > > + /* > + * prepare request sense if it got used with the last rq > + */ > + if (!drive->rq_sense) { > + drive->rq_sense = ide_cd_prep_sense(drive); > + if (!drive->rq_sense) { > + printk(KERN_ERR "%s: error prepping sense request!\n", > + drive->name); > + return ide_stopped; > + } > + } > + > + /* > + * save the current request in case we'll be queueing a sense rq > + * afterwards due to its potential failure. > + */ > + if (!blk_sense_request(rq)) > + drive->rq_sense->special = (void *)rq; > + > memset(&cmd, 0, sizeof(cmd)); > > if (rq_data_dir(rq)) > diff --git a/include/linux/ide.h b/include/linux/ide.h > index c942533..4c2d310 100644 > --- a/include/linux/ide.h > +++ b/include/linux/ide.h > @@ -605,6 +605,9 @@ struct ide_drive_s { > struct request request_sense_rq; > struct bio request_sense_bio; > struct bio_vec request_sense_bvec[2]; We can remove the above, right? > + > + /* current sense rq */ > + struct request *rq_sense; > }; > > typedef struct ide_drive_s ide_drive_t; > -- > 1.6.2.2 > > > -- > Regards/Gruss, > Boris. > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- 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
