Re: [PATCH 14/17] scsi: replace custom rq mapping withblk_rq_map_kern_sgl()

[Borislav Petkov]

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(-)

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;
+
 		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];
+
+	/* 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/