From: James Bottomley <[email protected]>

Reviewed-by FUJITA Tomonori <[email protected]>

---
 block/blk-core.c        |   25 +++++++++++++++++++++++++
 block/blk-settings.c    |   17 +++++++++++++++++
 drivers/scsi/scsi_lib.c |    2 +-
 include/linux/blkdev.h  |    4 ++++
 4 files changed, 47 insertions(+), 1 deletions(-)

block/blk-core.c +25 -0

index 3c37894..5ab3ac2 100644
@@ -608,6 +608,7 @@ blk_init_allocated_queue_node(struct request_queue *q, request_fn_proc *rfn,
 
 	q->request_fn		= rfn;
 	q->prep_rq_fn		= NULL;
+	q->unprep_rq_fn		= NULL;
 	q->unplug_fn		= generic_unplug_device;
 	q->queue_flags		= QUEUE_FLAG_DEFAULT;
 	q->queue_lock		= lock;

@@ -2133,6 +2134,26 @@ static bool blk_update_bidi_request(struct request *rq, int error,
 	return false;
 }
 
+/**
+ * blk_unprep_request - unprepare a request
+ * @req:	the request
+ *
+ * This function makes a request ready for complete resubmission (or
+ * completion).  It happens only after all error handling is complete,
+ * so represents the appropriate moment to deallocate any resources
+ * that were allocated to the request in the prep_rq_fn.  The queue
+ * lock is held when calling this.
+ */
+void blk_unprep_request(struct request *req)
+{
+	struct request_queue *q = req->q;
+
+	req->cmd_flags &= ~REQ_DONTPREP;
+	if (q->unprep_rq_fn)
+		q->unprep_rq_fn(q, req);
+}
+EXPORT_SYMBOL_GPL(blk_unprep_request);
+
 /*
  * queue lock must be held
  */

@@ -2148,6 +2169,10 @@ static void blk_finish_request(struct request *req, int error)
 
 	blk_delete_timer(req);
 
+	if (req->cmd_flags & REQ_DONTPREP)
+		blk_unprep_request(req);
+
+
 	blk_account_io_done(req);
 
 	if (req->end_io)

block/blk-settings.c +17 -0

index f5ed5a1..a234f4b 100644
@@ -37,6 +37,23 @@ void blk_queue_prep_rq(struct request_queue *q, prep_rq_fn *pfn)
 EXPORT_SYMBOL(blk_queue_prep_rq);
 
 /**
+ * blk_queue_unprep_rq - set an unprepare_request function for queue
+ * @q:		queue
+ * @ufn:	unprepare_request function
+ *
+ * It's possible for a queue to register an unprepare_request callback
+ * which is invoked before the request is finally completed. The goal
+ * of the function is to deallocate any data that was allocated in the
+ * prepare_request callback.
+ *
+ */
+void blk_queue_unprep_rq(struct request_queue *q, unprep_rq_fn *ufn)
+{
+	q->unprep_rq_fn = ufn;
+}
+EXPORT_SYMBOL(blk_queue_unprep_rq);
+
+/**
  * blk_queue_merge_bvec - set a merge_bvec function for queue
  * @q:		queue
  * @mbfn:	merge_bvec_fn

drivers/scsi/scsi_lib.c +1 -1

index 5f11608..ee83619 100644
@@ -85,7 +85,7 @@ static void scsi_unprep_request(struct request *req)
 {
 	struct scsi_cmnd *cmd = req->special;
 
-	req->cmd_flags &= ~REQ_DONTPREP;
+	blk_unprep_request(req);
 	req->special = NULL;
 
 	scsi_put_command(cmd);

include/linux/blkdev.h +4 -0

index 204fbe2..6bba04c 100644
@@ -200,6 +200,7 @@ struct request_pm_state
 typedef void (request_fn_proc) (struct request_queue *q);
 typedef int (make_request_fn) (struct request_queue *q, struct bio *bio);
 typedef int (prep_rq_fn) (struct request_queue *, struct request *);
+typedef void (unprep_rq_fn) (struct request_queue *, struct request *);
 typedef void (unplug_fn) (struct request_queue *);
 
 struct bio_vec;

@@ -282,6 +283,7 @@ struct request_queue
 	request_fn_proc		*request_fn;
 	make_request_fn		*make_request_fn;
 	prep_rq_fn		*prep_rq_fn;
+	unprep_rq_fn		*unprep_rq_fn;
 	unplug_fn		*unplug_fn;
 	merge_bvec_fn		*merge_bvec_fn;
 	prepare_flush_fn	*prepare_flush_fn;

@@ -841,6 +843,7 @@ extern void blk_complete_request(struct request *);
 extern void __blk_complete_request(struct request *);
 extern void blk_abort_request(struct request *);
 extern void blk_abort_queue(struct request_queue *);
+extern void blk_unprep_request(struct request *);
 
 /*
  * Access functions for manipulating queue properties

@@ -885,6 +888,7 @@ extern int blk_queue_dma_drain(struct request_queue *q,
 extern void blk_queue_lld_busy(struct request_queue *q, lld_busy_fn *fn);
 extern void blk_queue_segment_boundary(struct request_queue *, unsigned long);
 extern void blk_queue_prep_rq(struct request_queue *, prep_rq_fn *pfn);
+extern void blk_queue_unprep_rq(struct request_queue *, unprep_rq_fn *ufn);
 extern void blk_queue_merge_bvec(struct request_queue *, merge_bvec_fn *);
 extern void blk_queue_dma_alignment(struct request_queue *, int);
 extern void blk_queue_update_dma_alignment(struct request_queue *, int);

This fixes discard page leak by using q->unprep_rq_fn facility.

q->unprep_rq_fn is called when all the data buffer (req->bio and scsi_data_buffer) in the request is freed.

sd_unprep() uses rq->buffer to free discard page allocated in sd_prepare_discard().

Signed-off-by FUJITA Tomonori <[email protected]>

Quoted 8 lines · FUJITA Tomonori

This fixes discard page leak by using q->unprep_rq_fn facility.

q->unprep_rq_fn is called when all the data buffer (req->bio and scsi_data_buffer) in the request is freed.

sd_unprep() uses rq->buffer to free discard page allocated in sd_prepare_discard().

Quoted 1 line

Signed-off-by FUJITA Tomonori <[email protected]>

Thanks for sorting this out Tomo, all 3 patches work great!

BTW, there is one remaining (rare) leak in the allocation path.

The following patch serves to fix it but I'm not sure if there is a more elegant way to address this.

An alternative would be to check if the page is already allocated (before allocating the page in scsi_setup_discard_cmnd)?

Please advise, thanks.

From: Mike Snitzer <[email protected]>
Subject: scsi: address leak in the error path of discard page allocation

Be sure to free the discard page if scsi_setup_blk_pc_cmnd fails. E.g. Returning BLKPREP_DEFER from scsi_setup_blk_pc_cmnd will not cause the request to be processed by sd_unprep_fn before the request is retried (preparation included).

Signed-off-by Mike Snitzer <[email protected]>

---
 block/blk-core.c       |   23 +++++++++++++++++++++++
 drivers/scsi/sd.c      |    6 +++++-
 include/linux/blkdev.h |    1 +
 3 files changed, 29 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/scsi/sd.c
===================================================================

drivers/scsi/sd.c +5 -1

@@ -466,7 +466,11 @@ static int scsi_setup_discard_cmnd(struc
 
 	blk_add_request_payload(rq, page, len);
 	ret = scsi_setup_blk_pc_cmnd(sdp, rq);
-	rq->buffer = page_address(page);
+	if (ret != BLKPREP_OK) {
+		blk_clear_request_payload(rq);
+		__free_page(page);
+	} else
+		rq->buffer = page_address(page);
 	return ret;
 }
 

Index: linux-2.6/block/blk-core.c
===================================================================

block/blk-core.c +23 -0

@@ -1164,6 +1164,29 @@ void blk_add_request_payload(struct requ
 }
 EXPORT_SYMBOL_GPL(blk_add_request_payload);
 
+/**
+ * blk_clear_request_payload - clear a request's payload
+ * @rq: request to update
+ *
+ * The driver needs to take care of freeing the payload itself.
+ */
+void blk_clear_request_payload(struct request *rq)
+{
+	struct bio *bio = rq->bio;
+
+	rq->__data_len = rq->resid_len = 0;
+	rq->nr_phys_segments = 0;
+	rq->buffer = NULL;
+
+	bio->bi_size = 0;
+	bio->bi_vcnt = 0;
+	bio->bi_phys_segments = 0;
+
+	bio->bi_io_vec->bv_page = NULL;
+	bio->bi_io_vec->bv_len = 0;
+}
+EXPORT_SYMBOL_GPL(blk_clear_request_payload);
+
 void init_request_from_bio(struct request *req, struct bio *bio)
 {
 	req->cpu = bio->bi_comp_cpu;

Index: linux-2.6/include/linux/blkdev.h
===================================================================

include/linux/blkdev.h +1 -0

@@ -781,6 +781,7 @@ extern void blk_insert_request(struct re
 extern void blk_requeue_request(struct request_queue *, struct request *);
 extern void blk_add_request_payload(struct request *rq, struct page *page,
 		unsigned int len);
+extern void blk_clear_request_payload(struct request *rq);
 extern int blk_rq_check_limits(struct request_queue *q, struct request *rq);
 extern int blk_lld_busy(struct request_queue *q);
 extern int blk_rq_prep_clone(struct request *rq, struct request *rq_src,

Quoted 11 lines · Mike Snitzer

Quoted 8 lines · FUJITA Tomonori

This fixes discard page leak by using q->unprep_rq_fn facility.

q->unprep_rq_fn is called when all the data buffer (req->bio and scsi_data_buffer) in the request is freed.

sd_unprep() uses rq->buffer to free discard page allocated in sd_prepare_discard().

Quoted 8 lines

Quoted 1 line

Signed-off-by FUJITA Tomonori <[email protected]>

Thanks for sorting this out Tomo, all 3 patches work great!

BTW, there is one remaining (rare) leak in the allocation path.

The following patch serves to fix it but I'm not sure if there is a more elegant way to address this.

I've continued to look at this to arrive at alternative implementation. Here is a summary of the problem:

A 'scsi_setup_discard_cmnd' return other than BLKPREP_OK will not cause a discard request to get completely stripped down ('blk_finish_request' isn't calling 'blk_unprep_request' because REQ_DONTPREP is not set by 'scsi_prep_return' for none BLKPREP_OK return). Therefore the discard request's page will _not_ get cleaned up.

Aside from code inspection, I confirmed this by adding some test code to force a one-time initial BLKPREP_DEFER return from 'scsi_setup_discard_cmnd'.

Quoted 2 lines

An alternative would be to check if the page is already allocated (before allocating the page in scsi_setup_discard_cmnd)?

Unfortunatey this "alternative" won't work because it completely ignores the case where BLKPREP_KILL is returned from scsi_setup_discard_cmnd'.

Quoted 1 line

Please advise, thanks.

In short, I'm not too happy that the following patch doesn't allow for centralized cleanup of the discard request's page (via sd_unprep_fn). But in order to do that we'd likely have to:
1) relax blk_finish_request's REQ_DONTPREP constraint
2) add other weird conditionals within blk_unprep_request because the discard request wasn't _really_ prepared?

So given this I'm inclined to stick with the following patch.

Jens and/or James, what do you think?

Quoted 51 lines · Mike Snitzer

Quoted 11 lines · Mike Snitzer

Quoted 8 lines · FUJITA Tomonori

This fixes discard page leak by using q->unprep_rq_fn facility.

q->unprep_rq_fn is called when all the data buffer (req->bio and scsi_data_buffer) in the request is freed.

sd_unprep() uses rq->buffer to free discard page allocated in sd_prepare_discard().

Quoted 8 lines

Quoted 1 line

Signed-off-by FUJITA Tomonori <[email protected]>

Thanks for sorting this out Tomo, all 3 patches work great!

BTW, there is one remaining (rare) leak in the allocation path.

The following patch serves to fix it but I'm not sure if there is a more elegant way to address this.

I've continued to look at this to arrive at alternative implementation. Here is a summary of the problem:

A 'scsi_setup_discard_cmnd' return other than BLKPREP_OK will not cause a discard request to get completely stripped down ('blk_finish_request' isn't calling 'blk_unprep_request' because REQ_DONTPREP is not set by 'scsi_prep_return' for none BLKPREP_OK return). Therefore the discard request's page will _not_ get cleaned up.

Aside from code inspection, I confirmed this by adding some test code to force a one-time initial BLKPREP_DEFER return from 'scsi_setup_discard_cmnd'.

Quoted 2 lines

An alternative would be to check if the page is already allocated (before allocating the page in scsi_setup_discard_cmnd)?

Unfortunatey this "alternative" won't work because it completely ignores the case where BLKPREP_KILL is returned from scsi_setup_discard_cmnd'.

Quoted 1 line

Please advise, thanks.

In short, I'm not too happy that the following patch doesn't allow for centralized cleanup of the discard request's page (via sd_unprep_fn). But in order to do that we'd likely have to:
1) relax blk_finish_request's REQ_DONTPREP constraint
2) add other weird conditionals within blk_unprep_request because the discard request wasn't _really_ prepared?

Quoted 3 lines

So given this I'm inclined to stick with the following patch.

Jens and/or James, what do you think?

The rules are pretty clear: Unprep is only called if the request gets prepped ... that means you have to return BLKPREP_OK. Defer or kill assume there's no teardown to do, so the allocation (if it took place) must be reversed before returning them

OK, thanks for clarifying. This confirms that the general approach I took in this patch is correct. It remains to be seen if Jens is agreeable with blk_clear_request_payload.

I know Christoph thought my introduction and use of blk_clear_request_payload was reasonable. Christoph, please feel free to add your Ack to this patch if you approve.

Quoted 51 lines · Mike Snitzer

Quoted 11 lines · Mike Snitzer

Quoted 8 lines · FUJITA Tomonori

This fixes discard page leak by using q->unprep_rq_fn facility.

q->unprep_rq_fn is called when all the data buffer (req->bio and scsi_data_buffer) in the request is freed.

sd_unprep() uses rq->buffer to free discard page allocated in sd_prepare_discard().

Quoted 8 lines

Quoted 1 line

Signed-off-by FUJITA Tomonori <[email protected]>

Thanks for sorting this out Tomo, all 3 patches work great!

BTW, there is one remaining (rare) leak in the allocation path.

The following patch serves to fix it but I'm not sure if there is a more elegant way to address this.

I've continued to look at this to arrive at alternative implementation. Here is a summary of the problem:

A 'scsi_setup_discard_cmnd' return other than BLKPREP_OK will not cause a discard request to get completely stripped down ('blk_finish_request' isn't calling 'blk_unprep_request' because REQ_DONTPREP is not set by 'scsi_prep_return' for none BLKPREP_OK return). Therefore the discard request's page will _not_ get cleaned up.

Aside from code inspection, I confirmed this by adding some test code to force a one-time initial BLKPREP_DEFER return from 'scsi_setup_discard_cmnd'.

Quoted 2 lines

An alternative would be to check if the page is already allocated (before allocating the page in scsi_setup_discard_cmnd)?

Unfortunatey this "alternative" won't work because it completely ignores the case where BLKPREP_KILL is returned from scsi_setup_discard_cmnd'.

Quoted 1 line

Please advise, thanks.

In short, I'm not too happy that the following patch doesn't allow for centralized cleanup of the discard request's page (via sd_unprep_fn). But in order to do that we'd likely have to:
1) relax blk_finish_request's REQ_DONTPREP constraint
2) add other weird conditionals within blk_unprep_request because the discard request wasn't _really_ prepared?

Quoted 3 lines

So given this I'm inclined to stick with the following patch.

Jens and/or James, what do you think?

The rules are pretty clear: Unprep is only called if the request gets prepped ... that means you have to return BLKPREP_OK. Defer or kill assume there's no teardown to do, so the allocation (if it took place) must be reversed before returning them

Seems that scsi-ml calls scsi_unprep_request() for not-prepped requests in scsi_init_io error path. So we could move that scsi_unprep_request() to the error path in scsi_prep_return(). Then we can free discard page in the single place.

Applying the rule strictly is fine by me too; we remove scsi_unprep_request() in scsi_init_io error path and clean up things in each prep function's error path.

Btw, blk_clear_request_payload() is necessary?

Making sure that a request is clean is not a bad idea but if we hit BLKPREP_KILL or BLKPREP_DEFER, we call
blk_end_request(). blk_end_request() can free a request properly even if we don't do something like blk_clear_request_payload?

Quoted 8 lines · FUJITA Tomonori

Seems that scsi-ml calls scsi_unprep_request() for not-prepped requests in scsi_init_io error path. So we could move that scsi_unprep_request() to the error path in scsi_prep_return(). Then we can free discard page in the single place.

Applying the rule strictly is fine by me too; we remove scsi_unprep_request() in scsi_init_io error path and clean up things in each prep function's error path.

That would be my preference. Making sure a function cleans up all allocations / state changes on errors means code is a lot fragile and easier to understand.

Quoted 6 lines

Btw, blk_clear_request_payload() is necessary?

Making sure that a request is clean is not a bad idea but if we hit BLKPREP_KILL or BLKPREP_DEFER, we call
blk_end_request(). blk_end_request() can free a request properly even if we don't do something like blk_clear_request_payload?

For BLKPREP_KILL we do call __blk_end_request_all, but for BLKPREP_DEFER we don't. In that case we just leave it on the queue for a later retry. So we either have to clean it up, or leave the detect the case of a partially constructed command in ->prep_fn. I think cleaning up properly and having defined state when entering ->prep_fn is the better variant.

Quoted 21 lines · Christoph Hellwig

Quoted 8 lines · FUJITA Tomonori

Seems that scsi-ml calls scsi_unprep_request() for not-prepped requests in scsi_init_io error path. So we could move that scsi_unprep_request() to the error path in scsi_prep_return(). Then we can free discard page in the single place.

Applying the rule strictly is fine by me too; we remove scsi_unprep_request() in scsi_init_io error path and clean up things in each prep function's error path.

That would be my preference. Making sure a function cleans up all allocations / state changes on errors means code is a lot fragile and easier to understand.

Quoted 6 lines

Btw, blk_clear_request_payload() is necessary?

Making sure that a request is clean is not a bad idea but if we hit BLKPREP_KILL or BLKPREP_DEFER, we call
blk_end_request(). blk_end_request() can free a request properly even if we don't do something like blk_clear_request_payload?

Quoted 6 lines

For BLKPREP_KILL we do call __blk_end_request_all, but for BLKPREP_DEFER we don't. In that case we just leave it on the queue for a later retry. So we either have to clean it up, or leave the detect the case of a partially constructed command in ->prep_fn. I think cleaning up properly and having defined state when entering ->prep_fn is the better variant.

Right, I shared the same opinion earlier in this thread, so please let your ACKs fly now that we seem to all be in agreement.

Quoted 9 lines · FUJITA Tomonori

This fixes discard page leak by using q->unprep_rq_fn facility.

q->unprep_rq_fn is called when all the data buffer (req->bio and scsi_data_buffer) in the request is freed.

sd_unprep() uses rq->buffer to free discard page allocated in sd_prepare_discard().

Signed-off-by FUJITA Tomonori <[email protected]>

Thanks for sorting this out Tomo, all 3 patches work great!

BTW, there is one remaining (rare) leak in the allocation path.

The following patch serves to fix it but I'm not sure if there is a more elegant way to address this.

An alternative would be to check if the page is already allocated (before allocating the page in scsi_setup_discard_cmnd)?

sd_unprep() uses rq->buffer to free discard page allocated in sd_prepare_discard().

This fixes discard page leak by using q->unprep_rq_fn facility.

q->unprep_rq_fn is called when all the data buffer (req->bio and scsi_data_buffer) in the request is freed.

sd_unprep() uses rq->buffer to free discard page allocated in sd_prepare_discard().

Signed-off-by FUJITA Tomonori <[email protected]>

---
 drivers/scsi/sd.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

drivers/scsi/sd.c +11 -1

index 86da819..2d4e3a8 100644
@@ -425,6 +425,7 @@ static int scsi_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
 	sector_t sector = bio->bi_sector;
 	unsigned int nr_sectors = bio_sectors(bio);
 	unsigned int len;
+	int ret;
 	struct page *page;
 
 	if (sdkp->device->sector_size == 4096) {

@@ -465,7 +466,15 @@ static int scsi_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
 	}
 
 	blk_add_request_payload(rq, page, len);
-	return scsi_setup_blk_pc_cmnd(sdp, rq);
+	ret = scsi_setup_blk_pc_cmnd(sdp, rq);
+	rq->buffer = page_address(page);
+	return ret;
+}
+
+static void sd_unprep_fn(struct request_queue *q, struct request *rq)
+{
+	if (rq->cmd_flags & REQ_DISCARD)
+		__free_page(virt_to_page(rq->buffer));
 }
 
 /**

@@ -2242,6 +2251,7 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
 	sd_revalidate_disk(gd);
 
 	blk_queue_prep_rq(sdp->request_queue, sd_prep_fn);
+	blk_queue_unprep_rq(sdp->request_queue, sd_unprep_fn);
 
 	gd->driverfs_dev = &sdp->sdev_gendev;
 	gd->flags = GENHD_FL_EXT_DEVT;

- sd_done isn't called for pc request so we never call the code.
- we use sd_unprep to free discard page now.

Signed-off-by FUJITA Tomonori <[email protected]>

This patchset fixes page leak issue in discard commands with unprep facility that James posted:

http://marc.info/?l=linux-scsi&m=127791727508214&w=2

The 1/3 patch adds unprep facility to the block layer (identical to what James posted).

The 2/3 patch frees a page for discard commands by using the unprep facility. James' original patch doesn't work since it accesses to rq->bio in q->unprep_rq_fn. We hit oops since q->unprep_rq_fn is called when all the data buffer (req->bio and scsi_data_buffer) in the request is freed.

I use rq->buffer to keep track of an allocated page as the block layer sets rq->buffer to the address of bio's page. scsi-ml (and llds) don't use rq->buffer (rq->buffer is set to NULL). So I can't say that I like it lots. Any other way to do that?

The 3/3 path just removes the dead code.

This patchset fixes page leak issue in discard commands with unprep facility that James posted:

http://marc.info/?l=linux-scsi&m=127791727508214&w=2

The 1/3 patch adds unprep facility to the block layer (identical to what James posted).

The 2/3 patch frees a page for discard commands by using the unprep facility. James' original patch doesn't work since it accesses to rq->bio in q->unprep_rq_fn. We hit oops since q->unprep_rq_fn is called when all the data buffer (req->bio and scsi_data_buffer) in the request is freed.

I use rq->buffer to keep track of an allocated page as the block layer sets rq->buffer to the address of bio's page. scsi-ml (and llds) don't use rq->buffer (rq->buffer is set to NULL). So I can't say that I like it lots. Any other way to do that?

The 3/3 path just removes the dead code.

This is against Jens' for-2.6.36.

The git tree is also available:

git://git.kernel.org/pub/scm/linux/kernel/git/tomo/linux-2.6-misc.git unprep

I'll update the discard FS request conversion on the top of this soon. But this can be applied independently (and fixes the memory leak).

=

 block/blk-core.c        |   25 +++++++++++++++++++++++++
 block/blk-settings.c    |   17 +++++++++++++++++
 drivers/scsi/scsi_lib.c |    2 +-
 drivers/scsi/sd.c       |   25 +++++++++++++++----------
 include/linux/blkdev.h  |    4 ++++
 5 files changed, 62 insertions(+), 11 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · FUJITA Tomonori

This patchset fixes page leak issue in discard commands with unprep facility that James posted:

http://marc.info/?l=linux-scsi&m=127791727508214&w=2

Quoted 3 lines

The 1/3 patch adds unprep facility to the block layer (identical to what James posted).

Alternatively to this patch you could also call scsi_driver->done() on all commands. There are only two users (sd sr) it's not that bad to add an if (blk_pc_req()) inside these ->done() function.

Quoted 11 lines

The 2/3 patch frees a page for discard commands by using the unprep facility. James' original patch doesn't work since it accesses to rq->bio in q->unprep_rq_fn. We hit oops since q->unprep_rq_fn is called when all the data buffer (req->bio and scsi_data_buffer) in the request is freed.

I use rq->buffer to keep track of an allocated page as the block layer sets rq->buffer to the address of bio's page. scsi-ml (and llds) don't use rq->buffer (rq->buffer is set to NULL). So I can't say that I like it lots. Any other way to do that?

rq->buffer is intended for block-driver use as well as req->special. sd+scsi-ml is the block-driver here. req->special is used by scsi-ml and rq->buffer is set to NULL inside the call to scsi_setup_blk_pc_cmnd/scsi_setup_fs_cmnd. Since you set the ->buffer after the call to scsi_setup_blk_pc_cmnd you should be in the clear.

I think scsi-ml should stop setting rq->buffer to NULL and leave it be for ULD use. It is left from the time that LLDs where converted to use BIOs, just to make sure out-of-tree drivers crash.