Re: [RFC PATCHSET block] block: blk-map updates and API cleanup

Re: [RFC PATCHSET block] block: blk-map updates and API cleanup

[Tejun Heo]

Tejun Heo wrote:
> Hello, all.
> 
> These patches are available in the following git tree but it's on top
> of the previous blk-map-related-fixes patchset which needs some
> updating, so this posting is just for review and comments.  This
> patchset needs to spend quite some time in RCs even if it gets acked
> eventually, so definitely no .30 material.  Please also note that I
> haven't updated the comment about bio chaining violating queue limit,
> so please ignore that part.
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git blk-map
>  http://git.kernel.org/?p=linux/kernel/git/tj/misc.git;a=shortlog;h=blk-map

Oops, forgot to cc linux-scsi.  Cc'ing linux-scsi and James.  The
thread is (gmane hasn't picked it up yet).

 http://lkml.org/lkml/2009/4/1/228

Thanks.

-- 
tejun

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

[Boaz Harrosh]

On 04/01/2009 04:44 PM, Tejun Heo wrote:
> Impact: hack removal
> 
> SCSI needs to map sgl into rq for kernel PC requests; however, block
> API didn't have such feature so it used its own rq mapping function
> which hooked into block/bio internals and is generally considered an
> ugly hack.  The private function may also produce requests which are
> bigger than queue per-rq limits.
> 
> Block blk_rq_map_kern_sgl().  Kill the private implementation and use
> it.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>

James, TOMO

what happened to Tomo's patches that removes all this after fixing up
all users (sg.c)?

I thought that was agreed and done? What is left to do for that to go
in.

Thanks Boaz

> ---
>  drivers/scsi/scsi_lib.c |  108 +----------------------------------------------
>  1 files changed, 1 insertions(+), 107 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 3196c83..3fa5589 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -296,112 +296,6 @@ static void scsi_end_async(struct request *req, int uptodate)
>  	__blk_put_request(req->q, req);
>  }
>  
> -static int scsi_merge_bio(struct request *rq, struct bio *bio)
> -{
> -	struct request_queue *q = rq->q;
> -
> -	bio->bi_flags &= ~(1 << BIO_SEG_VALID);
> -	if (rq_data_dir(rq) == WRITE)
> -		bio->bi_rw |= (1 << BIO_RW);
> -	blk_queue_bounce(q, &bio);
> -
> -	return blk_rq_append_bio(q, rq, bio);
> -}
> -
> -static void scsi_bi_endio(struct bio *bio, int error)
> -{
> -	bio_put(bio);
> -}
> -
> -/**
> - * scsi_req_map_sg - map a scatterlist into a request
> - * @rq:		request to fill
> - * @sgl:	scatterlist
> - * @nsegs:	number of elements
> - * @bufflen:	len of buffer
> - * @gfp:	memory allocation flags
> - *
> - * scsi_req_map_sg maps a scatterlist into a request so that the
> - * request can be sent to the block layer. We do not trust the scatterlist
> - * sent to use, as some ULDs use that struct to only organize the pages.
> - */
> -static int scsi_req_map_sg(struct request *rq, struct scatterlist *sgl,
> -			   int nsegs, unsigned bufflen, gfp_t gfp)
> -{
> -	struct request_queue *q = rq->q;
> -	int nr_pages = (bufflen + sgl[0].offset + PAGE_SIZE - 1) >> PAGE_SHIFT;
> -	unsigned int data_len = bufflen, len, bytes, off;
> -	struct scatterlist *sg;
> -	struct page *page;
> -	struct bio *bio = NULL;
> -	int i, err, nr_vecs = 0;
> -
> -	for_each_sg(sgl, sg, nsegs, i) {
> -		page = sg_page(sg);
> -		off = sg->offset;
> -		len = sg->length;
> -
> -		while (len > 0 && data_len > 0) {
> -			/*
> -			 * sg sends a scatterlist that is larger than
> -			 * the data_len it wants transferred for certain
> -			 * IO sizes
> -			 */
> -			bytes = min_t(unsigned int, len, PAGE_SIZE - off);
> -			bytes = min(bytes, data_len);
> -
> -			if (!bio) {
> -				nr_vecs = min_t(int, BIO_GUARANTEED_PAGES,
> -						nr_pages);
> -				nr_pages -= nr_vecs;
> -
> -				bio = bio_alloc(gfp, nr_vecs);
> -				if (!bio) {
> -					err = -ENOMEM;
> -					goto free_bios;
> -				}
> -				bio->bi_end_io = scsi_bi_endio;
> -			}
> -
> -			if (bio_add_pc_page(q, bio, page, bytes, off) !=
> -			    bytes) {
> -				bio_put(bio);
> -				err = -EINVAL;
> -				goto free_bios;
> -			}
> -
> -			if (bio->bi_vcnt >= nr_vecs) {
> -				err = scsi_merge_bio(rq, bio);
> -				if (err) {
> -					bio_endio(bio, 0);
> -					goto free_bios;
> -				}
> -				bio = NULL;
> -			}
> -
> -			page++;
> -			len -= bytes;
> -			data_len -=bytes;
> -			off = 0;
> -		}
> -	}
> -
> -	rq->buffer = rq->data = NULL;
> -	rq->data_len = bufflen;
> -	return 0;
> -
> -free_bios:
> -	while ((bio = rq->bio) != NULL) {
> -		rq->bio = bio->bi_next;
> -		/*
> -		 * call endio instead of bio_put incase it was bounced
> -		 */
> -		bio_endio(bio, 0);
> -	}
> -
> -	return err;
> -}
> -
>  /**
>   * scsi_execute_async - insert request
>   * @sdev:	scsi device
> @@ -438,7 +332,7 @@ int scsi_execute_async(struct scsi_device *sdev, const unsigned char *cmd,
>  	req->cmd_flags |= REQ_QUIET;
>  
>  	if (use_sg)
> -		err = scsi_req_map_sg(req, buffer, use_sg, bufflen, gfp);
> +		err = blk_rq_map_kern_sg(req->q, req, buffer, use_sg, gfp);
>  	else if (bufflen)
>  		err = blk_rq_map_kern(req->q, req, buffer, bufflen, gfp);
>  

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

[James Bottomley]

On Wed, 2009-04-01 at 20:00 +0300, Boaz Harrosh wrote:
> On 04/01/2009 04:44 PM, Tejun Heo wrote:
> > Impact: hack removal
> > 
> > SCSI needs to map sgl into rq for kernel PC requests; however, block
> > API didn't have such feature so it used its own rq mapping function
> > which hooked into block/bio internals and is generally considered an
> > ugly hack.  The private function may also produce requests which are
> > bigger than queue per-rq limits.
> > 
> > Block blk_rq_map_kern_sgl().  Kill the private implementation and use
> > it.
> > 
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> 
> James, TOMO
> 
> what happened to Tomo's patches that removes all this after fixing up
> all users (sg.c)?
> 
> I thought that was agreed and done? What is left to do for that to go
> in.

They couldn't go in because they would break libosd.  You were going to
send patches to fix libosd so it no longer relied on the exported
function ... did that happen and I missed it?

James

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

[Boaz Harrosh]

On 04/01/2009 08:05 PM, James Bottomley wrote:
> On Wed, 2009-04-01 at 20:00 +0300, Boaz Harrosh wrote:
>> On 04/01/2009 04:44 PM, Tejun Heo wrote:
>>> Impact: hack removal
>>>
>>> SCSI needs to map sgl into rq for kernel PC requests; however, block
>>> API didn't have such feature so it used its own rq mapping function
>>> which hooked into block/bio internals and is generally considered an
>>> ugly hack.  The private function may also produce requests which are
>>> bigger than queue per-rq limits.
>>>
>>> Block blk_rq_map_kern_sgl().  Kill the private implementation and use
>>> it.
>>>
>>> Signed-off-by: Tejun Heo <tj@kernel.org>
>> James, TOMO
>>
>> what happened to Tomo's patches that removes all this after fixing up
>> all users (sg.c)?
>>
>> I thought that was agreed and done? What is left to do for that to go
>> in.
> 
> They couldn't go in because they would break libosd.  You were going to
> send patches to fix libosd so it no longer relied on the exported
> function ... did that happen and I missed it?
> 

That's not related. I'm asking about the scsi ULD patches and finally the
patch to scsi_lib.c. libosd only conflicts with the very last patch to block
layer. I don't see how that prevents the cleanups to scsi?

And BTW, I did send RFC patches that removes usage of blk_rq_append_bio() and
did not receive any comments

> James
> 
> 

Boaz

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

[FUJITA Tomonori]

On Wed, 01 Apr 2009 20:00:44 +0300
Boaz Harrosh <bharrosh@panasas.com> wrote:

> On 04/01/2009 04:44 PM, Tejun Heo wrote:
> > Impact: hack removal
> > 
> > SCSI needs to map sgl into rq for kernel PC requests; however, block
> > API didn't have such feature so it used its own rq mapping function
> > which hooked into block/bio internals and is generally considered an
> > ugly hack.  The private function may also produce requests which are
> > bigger than queue per-rq limits.
> > 
> > Block blk_rq_map_kern_sgl().  Kill the private implementation and use
> > it.
> > 
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> 
> James, TOMO
> 
> what happened to Tomo's patches that removes all this after fixing up
> all users (sg.c)?
> 
> I thought that was agreed and done? What is left to do for that to go
> in.

I've converted all the users (sg, st, osst). Nothing is left. So we
don't need this.

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

[Tejun Heo]

FUJITA Tomonori wrote:
>> I thought that was agreed and done? What is left to do for that to go
>> in.
> 
> I've converted all the users (sg, st, osst). Nothing is left. So we
> don't need this.

Yeah, pulled it.  Okay, so we can postpone diddling with request
mapping for now.  I'll re-post fixes only from this and the previous
patchset and proceed with other patchsets.

Thanks.

-- 
tejun

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

[FUJITA Tomonori]

On Mon, 13 Apr 2009 18:38:23 +0900
Tejun Heo <tj@kernel.org> wrote:

> FUJITA Tomonori wrote:
> >> I thought that was agreed and done? What is left to do for that to go
> >> in.
> > 
> > I've converted all the users (sg, st, osst). Nothing is left. So we
> > don't need this.
> 
> Yeah, pulled it.  Okay, so we can postpone diddling with request
> mapping for now.  I'll re-post fixes only from this and the previous
> patchset and proceed with other patchsets.

To be honest, I don't think that we can clean up the block
mapping. For example, blk_rq_map_kern_prealloc() in your patchset
doesn't look cleanup to me. It's just moving a hack from ide to the
block (well, I have to admit that I did the same thing when I
converted sg/st/osst...).

We can't have good API with insane users. I don't want to ask this
publicly but don't we have any chance to merge the old ide code and
libata?

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

[Borislav Petkov]

Hi,

On Mon, Apr 13, 2009 at 07:07:58PM +0900, FUJITA Tomonori wrote:
> On Mon, 13 Apr 2009 18:38:23 +0900
> Tejun Heo <tj@kernel.org> wrote:
> 
> > FUJITA Tomonori wrote:
> > >> I thought that was agreed and done? What is left to do for that to go
> > >> in.
> > > 
> > > I've converted all the users (sg, st, osst). Nothing is left. So we
> > > don't need this.
> > 
> > Yeah, pulled it.  Okay, so we can postpone diddling with request
> > mapping for now.  I'll re-post fixes only from this and the previous
> > patchset and proceed with other patchsets.
> 
> To be honest, I don't think that we can clean up the block
> mapping. For example, blk_rq_map_kern_prealloc() in your patchset
> doesn't look cleanup to me. It's just moving a hack from ide to the
> block (well, I have to admit that I did the same thing when I
> converted sg/st/osst...).

Well, since blk_rq_map_kern_prealloc() is going to be used only in
ide-cd (driver needs it to queue a sense request from within the irq
handler) and since it is considered a hack I could try to move it out of
the irq handler and do away only with blk_rq_map_kern() if that is more
of an agreeable solution?

-- 
Regards/Gruss,
    Boris.

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

[FUJITA Tomonori]

On Mon, 13 Apr 2009 14:59:12 +0200
Borislav Petkov <petkovbb@googlemail.com> wrote:

> Hi,
> 
> On Mon, Apr 13, 2009 at 07:07:58PM +0900, FUJITA Tomonori wrote:
> > On Mon, 13 Apr 2009 18:38:23 +0900
> > Tejun Heo <tj@kernel.org> wrote:
> > 
> > > FUJITA Tomonori wrote:
> > > >> I thought that was agreed and done? What is left to do for that to go
> > > >> in.
> > > > 
> > > > I've converted all the users (sg, st, osst). Nothing is left. So we
> > > > don't need this.
> > > 
> > > Yeah, pulled it.  Okay, so we can postpone diddling with request
> > > mapping for now.  I'll re-post fixes only from this and the previous
> > > patchset and proceed with other patchsets.
> > 
> > To be honest, I don't think that we can clean up the block
> > mapping. For example, blk_rq_map_kern_prealloc() in your patchset
> > doesn't look cleanup to me. It's just moving a hack from ide to the
> > block (well, I have to admit that I did the same thing when I
> > converted sg/st/osst...).
> 
> Well, since blk_rq_map_kern_prealloc() is going to be used only in
> ide-cd (driver needs it to queue a sense request from within the irq
> handler) and since it is considered a hack I could try to move it out of
> the irq handler

It's quite nice if you could do that.

But I think that ide-atapi also needs blk_rq_map_kern_prealloc().

> and do away only with blk_rq_map_kern() if that is more
> of an agreeable solution?

Yeah, blk_rq_map_kern() is much proper.

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

[Borislav Petkov]

(adding Bart to CC)

On Tue, Apr 14, 2009 at 09:44:31AM +0900, FUJITA Tomonori wrote:
> On Mon, 13 Apr 2009 14:59:12 +0200
> Borislav Petkov <petkovbb@googlemail.com> wrote:
> 
> > Hi,
> > 
> > On Mon, Apr 13, 2009 at 07:07:58PM +0900, FUJITA Tomonori wrote:
> > > On Mon, 13 Apr 2009 18:38:23 +0900
> > > Tejun Heo <tj@kernel.org> wrote:
> > > 
> > > > FUJITA Tomonori wrote:
> > > > >> I thought that was agreed and done? What is left to do for that to go
> > > > >> in.
> > > > > 
> > > > > I've converted all the users (sg, st, osst). Nothing is left. So we
> > > > > don't need this.
> > > > 
> > > > Yeah, pulled it.  Okay, so we can postpone diddling with request
> > > > mapping for now.  I'll re-post fixes only from this and the previous
> > > > patchset and proceed with other patchsets.
> > > 
> > > To be honest, I don't think that we can clean up the block
> > > mapping. For example, blk_rq_map_kern_prealloc() in your patchset
> > > doesn't look cleanup to me. It's just moving a hack from ide to the
> > > block (well, I have to admit that I did the same thing when I
> > > converted sg/st/osst...).
> > 
> > Well, since blk_rq_map_kern_prealloc() is going to be used only in
> > ide-cd (driver needs it to queue a sense request from within the irq
> > handler) and since it is considered a hack I could try to move it out of
> > the irq handler
> 
> It's quite nice if you could do that.
> 
> But I think that ide-atapi also needs blk_rq_map_kern_prealloc().

I'll look into that too.

> > and do away only with blk_rq_map_kern() if that is more
> > of an agreeable solution?
> 
> Yeah, blk_rq_map_kern() is much proper.

How's that for starters? I know, it is rough but it seems to work
according to my initial testing.

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.

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.

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'm sure there's enough room for improvement so please let me know if
any objections/comments.

---
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 35d0973..e689494 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;
+	void *sense		= &info->sense_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;
+	unsigned sense_len	= sizeof(struct request_sense);
 
 	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);
 
-	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;
-- 
Regards/Gruss,
    Boris.

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

[FUJITA Tomonori]

On Tue, 14 Apr 2009 12:01:31 +0200
Borislav Petkov <petkovbb@googlemail.com> wrote:

> (adding Bart to CC)
> 
> On Tue, Apr 14, 2009 at 09:44:31AM +0900, FUJITA Tomonori wrote:
> > On Mon, 13 Apr 2009 14:59:12 +0200
> > Borislav Petkov <petkovbb@googlemail.com> wrote:
> > 
> > > Hi,
> > > 
> > > On Mon, Apr 13, 2009 at 07:07:58PM +0900, FUJITA Tomonori wrote:
> > > > On Mon, 13 Apr 2009 18:38:23 +0900
> > > > Tejun Heo <tj@kernel.org> wrote:
> > > > 
> > > > > FUJITA Tomonori wrote:
> > > > > >> I thought that was agreed and done? What is left to do for that to go
> > > > > >> in.
> > > > > > 
> > > > > > I've converted all the users (sg, st, osst). Nothing is left. So we
> > > > > > don't need this.
> > > > > 
> > > > > Yeah, pulled it.  Okay, so we can postpone diddling with request
> > > > > mapping for now.  I'll re-post fixes only from this and the previous
> > > > > patchset and proceed with other patchsets.
> > > > 
> > > > To be honest, I don't think that we can clean up the block
> > > > mapping. For example, blk_rq_map_kern_prealloc() in your patchset
> > > > doesn't look cleanup to me. It's just moving a hack from ide to the
> > > > block (well, I have to admit that I did the same thing when I
> > > > converted sg/st/osst...).
> > > 
> > > Well, since blk_rq_map_kern_prealloc() is going to be used only in
> > > ide-cd (driver needs it to queue a sense request from within the irq
> > > handler) and since it is considered a hack I could try to move it out of
> > > the irq handler
> > 
> > It's quite nice if you could do that.
> > 
> > But I think that ide-atapi also needs blk_rq_map_kern_prealloc().
> 
> I'll look into that too.

Great, thanks!


> > > and do away only with blk_rq_map_kern() if that is more
> > > of an agreeable solution?
> > 
> > Yeah, blk_rq_map_kern() is much proper.
> 
> How's that for starters? I know, it is rough but it seems to work
> according to my initial testing.
> 
> 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.


> 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.

Tejun?

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

[Tejun Heo]

Hello,

Sorry about the delay.

FUJITA Tomonori 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.

-- 
tejun

Re: [PATCH 14/17] scsi: replace custom rq mapping with blk_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@gmail.com>
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@gmail.com>
CC: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
CC: Tejun Heo <tj@kernel.org>
Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 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.

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

[FUJITA Tomonori]

On Wed, 15 Apr 2009 09:26:55 +0200
Borislav Petkov <petkovbb@googlemail.com> 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@gmail.com>
> 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@gmail.com>
> CC: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> CC: Tejun Heo <tj@kernel.org>
> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
> ---
>  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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Borislav Petkov]

Hi,

On Wed, Apr 15, 2009 at 04:48:35PM +0900, FUJITA Tomonori wrote:

[..]

> 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?

No, because this is done by ide_complete_rq() above. Otherwise I'd be
ending the sense request even it didn't get used. This way, I only use
it if I call ide_cd_queue_rq_sense() from which cdrom_decode_status().

> I guess that we also need to call blk_put_request() when ide_drive_s
> is freed.

That we need to do, thanks. Will update accordingly.

> > +
> >  		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?

id-atapi uses them too, see
http://git.kernel.org/?p=linux/kernel/git/tj/misc.git;a=commitdiff;h=9ac15840a6e5bf1fa6dce293484cb7aba4d078bb

They can go after I've converted ide-floppy and ide-tape to the same
sense handling as ide-cd. I'm on it...

-- 
Regards/Gruss,
    Boris.

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

[Tejun Heo]

I updated the patch a bit and folded it into the series after patch
10.  I hope I didn't butcher the patch too bad.  Can I add your
Signed-off-by: on the modified patch?  Also, I just started working on
the atapi version.  Do you already have something?

Thanks.

From: Borislav Petkov <petkovbb@gmail.com>
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.

tj: * After this patch, ide_cd_do_request() might sleep.  This should
      be okay as ide request_fn - do_ide_request() - is invoked only
      from make_request and plug work.  Make sure this is the case by
      adding might_sleep() to do_ide_request().

    * Adapted to apply to the tree without blk_rq_map_prealloc()
      changes.

    * Use of blk_rq_map() and possible failure handling from it are
      moved to later separate patch.  ide_cd_prep_sense() now handles
      everything regarding sense rq preparation.

    * Move ide_drive->rq_sense to cdrom_info->sense_rq and put the
      request when releasing cdrom_info.

    * Both user and kernel PC requests expect sense data to be stored
      in separate storage other than info->sense_data.  Copy sense
      data to rq->sense on completion if rq->sense is not NULL.  This
      fixes bogus sense data on PC requests.

CC: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
CC: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
CC: Tejun Heo <tj@kernel.org>
Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ide/ide-cd.c |   69 +++++++++++++++++++++++++++++++--------------------
 drivers/ide/ide-cd.h |    3 ++
 drivers/ide/ide-io.c |    3 ++
 3 files changed, 49 insertions(+), 26 deletions(-)

Index: block/drivers/ide/ide-cd.c
===================================================================
--- block.orig/drivers/ide/ide-cd.c
+++ block/drivers/ide/ide-cd.c
@@ -206,42 +206,44 @@ static void cdrom_analyze_sense_data(ide
 	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 void ide_cd_prep_sense(ide_drive_t *drive, struct request *rq)
 {
 	struct cdrom_info *info		= drive->driver_data;
-	struct request *rq		= &drive->request_sense_rq;
+	void *sense			= &info->sense_data;
 
 	ide_debug_log(IDE_DBG_SENSE, "enter");
 
-	if (sense == NULL)
-		sense = &info->sense_data;
+	if (blk_sense_request(rq))
+		return;
 
-	memset(sense, 0, 18);
+	if (!info->sense_rq) {
+		struct request *sense_rq;
 
-	/* stuff the sense request in front of our current request */
-	blk_rq_init(NULL, rq);
-	rq->cmd_type = REQ_TYPE_ATA_PC;
-	rq->rq_disk = info->disk;
+		memset(sense, 0, 18);
 
-	rq->data = sense;
-	rq->cmd[0] = GPCMD_REQUEST_SENSE;
-	rq->cmd[4] = 18;
-	rq->data_len = 18;
+		sense_rq = blk_get_request(drive->queue, 0, __GFP_WAIT);
+		sense_rq->rq_disk = info->disk;
 
-	rq->cmd_type = REQ_TYPE_SENSE;
-	rq->cmd_flags |= REQ_PREEMPT;
+		sense_rq->data = sense;
+		sense_rq->cmd[0] = GPCMD_REQUEST_SENSE;
+		sense_rq->cmd[4] = 18;
+		sense_rq->data_len = 18;
 
-	/* NOTE! Save the failed command in "rq->special" */
-	rq->special = (void *)failed_command;
+		sense_rq->cmd_type = REQ_TYPE_SENSE;
+		sense_rq->cmd_flags |= REQ_PREEMPT;
 
-	if (failed_command)
-		ide_debug_log(IDE_DBG_SENSE, "failed_cmd: 0x%x",
-					     failed_command->cmd[0]);
+		info->sense_rq = sense_rq;
+	}
+
+	info->sense_rq->special = rq;
+}
 
-	drive->hwif->rq = NULL;
+static void ide_cd_queue_sense_rq(ide_drive_t *drive)
+{
+	struct cdrom_info *info = drive->driver_data;
 
-	elv_add_request(drive->queue, rq, ELEVATOR_INSERT_FRONT, 0);
+	BUG_ON(!info->sense_rq);
+	elv_add_request(drive->queue, info->sense_rq, ELEVATOR_INSERT_FRONT, 0);
 }
 
 static void ide_cd_complete_failed_rq(ide_drive_t *drive, struct request *rq)
@@ -252,10 +254,16 @@ static void ide_cd_complete_failed_rq(id
 	 */
 	struct request *failed = (struct request *)rq->special;
 	struct cdrom_info *info = drive->driver_data;
-	void *sense = &info->sense_data;
+	struct request_sense *sense = &info->sense_data;
 
 	if (failed) {
 		if (failed->sense) {
+			/*
+			 * Sense is always read into info->sense_data.
+			 * Copy back if the failed request has its
+			 * sense pointer set.
+			 */
+			memcpy(failed->sense, sense, sizeof(*sense));
 			sense = failed->sense;
 			failed->sense_len = rq->sense_len;
 		}
@@ -431,7 +439,7 @@ static int cdrom_decode_status(ide_drive
 
 	/* 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_sense_rq(drive);
 	return 1;
 
 end_request:
@@ -445,7 +453,7 @@ end_request:
 
 		hwif->rq = NULL;
 
-		cdrom_queue_request_sense(drive, rq->sense, rq);
+		ide_cd_queue_sense_rq(drive);
 		return 1;
 	} else
 		return 2;
@@ -600,6 +608,7 @@ static void ide_cd_error_cmd(ide_drive_t
 
 static ide_startstop_t cdrom_newpc_intr(ide_drive_t *drive)
 {
+	struct cdrom_info *info = drive->driver_data;
 	ide_hwif_t *hwif = drive->hwif;
 	struct ide_cmd *cmd = &hwif->cmd;
 	struct request *rq = hwif->rq;
@@ -775,6 +784,10 @@ out_end:
 
 		ide_complete_rq(drive, uptodate ? 0 : -EIO, nsectors << 9);
 
+		/* our sense buffer got used, reset it for the next round */
+		if (sense)
+			info->sense_rq = NULL;
+
 		if (sense && rc == 2)
 			ide_error(drive, "request sense failure", stat);
 	}
@@ -893,6 +906,9 @@ static ide_startstop_t ide_cd_do_request
 		goto out_end;
 	}
 
+	/* prepare sense request */
+	ide_cd_prep_sense(drive, rq);
+
 	memset(&cmd, 0, sizeof(cmd));
 
 	if (rq_data_dir(rq))
@@ -1634,6 +1650,7 @@ static void ide_cd_release(struct device
 	ide_debug_log(IDE_DBG_FUNC, "enter");
 
 	kfree(info->toc);
+	blk_put_request(info->sense_rq);
 	if (devinfo->handle == drive)
 		unregister_cdrom(devinfo);
 	drive->driver_data = NULL;
Index: block/drivers/ide/ide-io.c
===================================================================
--- block.orig/drivers/ide/ide-io.c
+++ block/drivers/ide/ide-io.c
@@ -478,6 +478,9 @@ void do_ide_request(struct request_queue
 
 	spin_unlock_irq(q->queue_lock);
 
+	/* HLD do_request() callback might sleep, make sure it's okay */
+	might_sleep();
+
 	if (ide_lock_host(host, hwif))
 		goto plug_device_2;
 
Index: block/drivers/ide/ide-cd.h
===================================================================
--- block.orig/drivers/ide/ide-cd.h
+++ block/drivers/ide/ide-cd.h
@@ -98,6 +98,9 @@ struct cdrom_info {
         struct cdrom_device_info devinfo;
 
 	unsigned long write_timeout;
+
+	/* current sense rq */
+	struct request *sense_rq;
 };
 
 /* ide-cd_verbose.c */

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

[Borislav Petkov]

Hi,

On Thu, Apr 16, 2009 at 12:06:29PM +0900, Tejun Heo wrote:
> I updated the patch a bit and folded it into the series after patch
> 10.  I hope I didn't butcher the patch too bad.  Can I add your
> Signed-off-by: on the modified patch?  Also, I just started working on
> the atapi version.  Do you already have something?

Ok here's what I got: I went and converted all ide-cd and ide-atapi to
using a common routine ide_prep_sense, see following patches for that.
Preliminary testing is promising but you never know :).

[..]

> 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.
> 
> tj: * After this patch, ide_cd_do_request() might sleep.  This should
>       be okay as ide request_fn - do_ide_request() - is invoked only
>       from make_request and plug work.  Make sure this is the case by
>       adding might_sleep() to do_ide_request().
> 
>     * Adapted to apply to the tree without blk_rq_map_prealloc()
>       changes.
> 
>     * Use of blk_rq_map() and possible failure handling from it are
>       moved to later separate patch.  ide_cd_prep_sense() now handles
>       everything regarding sense rq preparation.
>
>     * Move ide_drive->rq_sense to cdrom_info->sense_rq and put the
>       request when releasing cdrom_info.

I put the request_sense buffer as defined in <linux/cdrom.h> into the
drive struct so that each device can have its own buffer. IMHO, request
sense standard data should be pretty identical across most ATAPI devices
(yeah, I'm sure there are exceptions :)). We might move the struct
request_sense to a more generic location instead of <linux/cdrom.h> if
we decide to go with that.

>     * Both user and kernel PC requests expect sense data to be stored
>       in separate storage other than info->sense_data.  Copy sense
>       data to rq->sense on completion if rq->sense is not NULL.  This
>       fixes bogus sense data on PC requests.

I took that into my version of the patch.

Anyway, please do have a closer look in case I've missed something. Of
course you can split them the way you see fit, there's more to be done
there anyways but should suffice for your block layer stuff, I hope.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

[Tejun Heo]

Hello, Borislav.

Borislav Petkov wrote:
> Hi,
> 
> On Thu, Apr 16, 2009 at 12:06:29PM +0900, Tejun Heo wrote:
>> I updated the patch a bit and folded it into the series after patch
>> 10.  I hope I didn't butcher the patch too bad.  Can I add your
>> Signed-off-by: on the modified patch?  Also, I just started working on
>> the atapi version.  Do you already have something?
> 
> Ok here's what I got: I went and converted all ide-cd and ide-atapi to
> using a common routine ide_prep_sense, see following patches for that.
> Preliminary testing is promising but you never know :).

Cool.  I'll take a look.

> I put the request_sense buffer as defined in <linux/cdrom.h> into the
> drive struct so that each device can have its own buffer. IMHO, request
> sense standard data should be pretty identical across most ATAPI devices
> (yeah, I'm sure there are exceptions :)). We might move the struct
> request_sense to a more generic location instead of <linux/cdrom.h> if
> we decide to go with that.

Okay.

>>     * Both user and kernel PC requests expect sense data to be stored
>>       in separate storage other than info->sense_data.  Copy sense
>>       data to rq->sense on completion if rq->sense is not NULL.  This
>>       fixes bogus sense data on PC requests.
> 
> I took that into my version of the patch.
> 
> Anyway, please do have a closer look in case I've missed something. Of
> course you can split them the way you see fit, there's more to be done
> there anyways but should suffice for your block layer stuff, I hope.

There was another problem.  If we use blk_rq_map_kern() the failed rq
must be finished after the sense rq is finished because that's when
the bio is copied back if it was copied.  Before sense_rq completion,
the sense buffer doesn't contain any valid data.

Anyways, I'll review your patchset and integrate it with mine.  Please
standby a bit.

Thanks.

-- 
tejun

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

[Borislav Petkov]

Hi Tejun,

On Thu, Apr 16, 2009 at 03:07:09PM +0900, Tejun Heo wrote:

[..]

> There was another problem.  If we use blk_rq_map_kern() the failed rq
> must be finished after the sense rq is finished because that's when
> the bio is copied back if it was copied.  Before sense_rq completion,
> the sense buffer doesn't contain any valid data.

Well, as an idea, we could just postpone the completion of the failed rq
in:

if (sense && uptodate)
        ide_cd_complete_failed_rq(drive, rq);

and put that just after

ide_complete_rq(drive, uptodate ? 0 : -EIO, nsectors << 9);

line in cdrom_newpc_intr(). This way, we can copy back the sense data
safely and then kill the rq.

The only problem I fear with changes like that is that later some subtle
interactions come about with some device which shouldn't normally
happen. This is old code, you know, which still needs lots of scrubbing.
It's like walking on a minefield :).

Thanks.

-- 
Regards/Gruss,
    Boris.

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

[Tejun Heo]

Borislav Petkov wrote:
> Hi Tejun,
> 
> On Thu, Apr 16, 2009 at 03:07:09PM +0900, Tejun Heo wrote:
> 
> [..]
> 
>> There was another problem.  If we use blk_rq_map_kern() the failed rq
>> must be finished after the sense rq is finished because that's when
>> the bio is copied back if it was copied.  Before sense_rq completion,
>> the sense buffer doesn't contain any valid data.
> 
> Well, as an idea, we could just postpone the completion of the failed rq
> in:
> 
> if (sense && uptodate)
>         ide_cd_complete_failed_rq(drive, rq);
> 
> and put that just after
> 
> ide_complete_rq(drive, uptodate ? 0 : -EIO, nsectors << 9);
> 
> line in cdrom_newpc_intr(). This way, we can copy back the sense data
> safely and then kill the rq.

Yeah, already have that in my patch.

> The only problem I fear with changes like that is that later some subtle
> interactions come about with some device which shouldn't normally
> happen. This is old code, you know, which still needs lots of scrubbing.
> It's like walking on a minefield :).

:-)

-- 
tejun