block: don't allocate a payload for discard request

Allocating a fixed payload for discard requests always was a horrible hack, and it's not coming to byte us when adding support for discard in DM/MD.

So change the code to leave the allocation of a payload to the lowlevel driver. Unfortunately that means we'll need another hack, which allows us to update the various block layer length fields indicating that we have a payload. Instead of hiding this in sd.c, which we already partially do for UNMAP support add a documented helper in the core block layer for it.

Signed-off-by Christoph Hellwig <[email protected]>

Quoted 10 lines · Christoph Hellwig

Allocating a fixed payload for discard requests always was a horrible hack, and it's not coming to byte us when adding support for discard in DM/MD.

So change the code to leave the allocation of a payload to the lowlevel driver. Unfortunately that means we'll need another hack, which allows us to update the various block layer length fields indicating that we have a payload. Instead of hiding this in sd.c, which we already partially do for UNMAP support add a documented helper in the core block layer for it.

Quoted 1 line

Signed-off-by Christoph Hellwig <[email protected]>

Acked-by Mike Snitzer <[email protected]>

I've built on your patch to successfully implement discard support for DM (this includes splitting discards that span targets; only the linear and striped DM targets are supported so far).

Fix leaks introduced via "block: don't allocate a payload for discard request" commit a1d949f5f44.

sd_done() is not called for REQ_TYPE_BLOCK_PC commands so cleanup discard request's payload directly in scsi_finish_command().

Also cleanup page allocated for discard payload in scsi_setup_discard_cmnd's scsi_setup_blk_pc_cmnd error path.

Quoted 5 lines · On Sat, 26 Jun 2010 15:56:50 -0400 Mike Snitzer <[email protected]> wrote:

Fix leaks introduced via "block: don't allocate a payload for discard request" commit a1d949f5f44.

sd_done() is not called for REQ_TYPE_BLOCK_PC commands so cleanup discard request's payload directly in scsi_finish_command().

Instead of adding another discard hack to scsi_finish_command(), how about converting discard to REQ_TYPE_FS request? discard is FS request from the perspective of the block layer. It also fixes a problem that discard isn't retried in the case of UNIT ATTENTION.

I think that we can get more cleaner code if we handle discard as normal (fs) request in the block layer (and scsi-ml). We need more changes but this patch is the first step.

Quoted 14 lines · FUJITA Tomonori

Quoted 5 lines · On Sat, 26 Jun 2010 15:56:50 -0400 Mike Snitzer <[email protected]> wrote:

Fix leaks introduced via "block: don't allocate a payload for discard request" commit a1d949f5f44.

sd_done() is not called for REQ_TYPE_BLOCK_PC commands so cleanup discard request's payload directly in scsi_finish_command().

Instead of adding another discard hack to scsi_finish_command(), how about converting discard to REQ_TYPE_FS request? discard is FS request from the perspective of the block layer. It also fixes a problem that discard isn't retried in the case of UNIT ATTENTION.

Quoted 3 lines

I think that we can get more cleaner code if we handle discard as normal (fs) request in the block layer (and scsi-ml). We need more changes but this patch is the first step.

Making discard a REQ_TYPE_FS inside scsi (it already is before entering sd_prep_fn) means we'll need to special case it all over the I/O submission and completion path. Having the payload length not matching

the transfer length is something we don't expect for FS requests.

Quoted 7 lines

index e16185b..9e15c46 100644

block/blk-lib.c +0 -0

@@ -20,6 +20,10 @@ static void blkdev_discard_end_io(struct bio *bio, int err)
 	if (bio->bi_private)
 		complete(bio->bi_private);
 

block/blk-lib.c static void blkdev_discard_end_io(struct bio *bio, int err) · line 26

Quoted 4 lines

block/blk-lib.c +4 -0

+	/* free the page that the lower layer allocated */
+	if (bio_page(bio))
+		__free_page(bio_page(bio));
+

This is exactly what this patchkit gets rid off. Having a payload page that the caller tracks (previously fully, with this patch only for freeing) makes DM's life a lot harder. Remember we don't actually store any payload in there before entering sd_prep_fn - it's just that the scsi commands implementing discards need some payload - either a sector sizes zero filled buffer for WRITE SAME, or an LBA/len encoding inside the payload for UNMAP.

block/blk-lib.c static void blkdev_discard_end_io(struct bio *bio, int err) · line 27

Quoted 2 lines

block/blk-lib.c +1 -1

-	rq->cmd_type = REQ_TYPE_BLOCK_PC;
+	rq->cmd_type = REQ_TYPE_FS;

No need to set REQ_TYPE_FS here, it's already set up that way.

Quoted 15 lines · On Sun, 27 Jun 2010 11:26:52 +0200 Christoph Hellwig <[email protected]> wrote:

Quoted 14 lines · FUJITA Tomonori

Quoted 5 lines · On Sat, 26 Jun 2010 15:56:50 -0400 Mike Snitzer <[email protected]> wrote:

Fix leaks introduced via "block: don't allocate a payload for discard request" commit a1d949f5f44.

sd_done() is not called for REQ_TYPE_BLOCK_PC commands so cleanup discard request's payload directly in scsi_finish_command().

Instead of adding another discard hack to scsi_finish_command(), how about converting discard to REQ_TYPE_FS request? discard is FS request from the perspective of the block layer. It also fixes a problem that discard isn't retried in the case of UNIT ATTENTION.

Quoted 7 lines

Quoted 3 lines

I think that we can get more cleaner code if we handle discard as normal (fs) request in the block layer (and scsi-ml). We need more changes but this patch is the first step.

Making discard a REQ_TYPE_FS inside scsi (it already is before entering sd_prep_fn) means we'll need to special case it all over the I/O submission and completion path. Having the payload length not matching

Hmm, my patch doesn't add any special case in scsi submission and completion. sd_prep_fn already has a hack for discard to set bi->bi_size to rq->__data_size so scsi can tell the block layer to finish discard requests.

Adding another special case for discard to scsi_io_completion() doesn't look good.

About the block layer, we already have special case for discard everywhere (rq->cmd_flags & REQ_DISCARD).

Quoted 1 line

the transfer length is something we don't expect for FS requests.

Yeah, that's tricky. I'm not sure yet which is better; change how the block layer handles the transfer length or let the lower layer to add pages (as we do now).

Quoted 7 lines

Quoted 7 lines

index e16185b..9e15c46 100644

block/blk-lib.c +0 -0

@@ -20,6 +20,10 @@ static void blkdev_discard_end_io(struct bio *bio, int err)
 	if (bio->bi_private)
 		complete(bio->bi_private);
 

Quoted 12 lines

block/blk-lib.c static void blkdev_discard_end_io(struct bio *bio, int err) · line 26

Quoted 4 lines

block/blk-lib.c +4 -0

+	/* free the page that the lower layer allocated */
+	if (bio_page(bio))
+		__free_page(bio_page(bio));
+

This is exactly what this patchkit gets rid off. Having a payload page that the caller tracks (previously fully, with this patch only for freeing) makes DM's life a lot harder. Remember we don't actually store any payload in there before entering sd_prep_fn - it's just that the scsi commands implementing discards need some payload - either a sector sizes zero filled buffer for WRITE SAME, or an LBA/len encoding inside the payload for UNMAP.

It's so bad if the block layer frees pages that the lower layer allocates? I thought it's ok if the block layer doesn't allocate.

It's better if sd_done() frees a page? As my patch does, if we handle discard as FS in scsi-ml, sd_done() is called.

How about this?

Quoted 1 line

How about this?

As I tried to explain before this utterly confuses the I/O completion path. With the patch applied even a simple mkfs.xfs that issues discard just hangs.

Quoted 5 lines · On Sun, 27 Jun 2010 13:07:12 +0200 Christoph Hellwig <[email protected]> wrote:

Quoted 1 line

How about this?

As I tried to explain before this utterly confuses the I/O completion path. With the patch applied even a simple mkfs.xfs that issues discard just hangs.

Wired. I just tried mkfs.xfs against scsi_debug with my block patches (I saw one discard command). Seemed that it worked fine.

My leak fixes have been tested extensively against all permuations of devices with discards (ATA trim, SCSI UNMAP, SCSI WRTIE SAME w/ unmap=1).

I think we need to get Christoph's discard payload transformation complete by fixing the leaks _without_ trying to rework how discard commands are tagged, etc. E.g. fix what Jens already has staged in linux-2.6-block's 'for-next' and 'for-2.6.36'.

With that sorted out we can then look at longer term changes to cleanup discard request processing.

Quoted 8 lines · Mike Snitzer

My leak fixes have been tested extensively against all permuations of devices with discards (ATA trim, SCSI UNMAP, SCSI WRTIE SAME w/ unmap=1).

I think we need to get Christoph's discard payload transformation complete by fixing the leaks _without_ trying to rework how discard commands are tagged, etc. E.g. fix what Jens already has staged in linux-2.6-block's 'for-next' and 'for-2.6.36'.

Quoted 2 lines

With that sorted out we can then look at longer term changes to cleanup discard request processing.

I tend to agree. I'll look into getting rid of treating discards as BLOCK_PC commands ontop of you patchset.

Quoted 5 lines · On Sun, 27 Jun 2010 13:07:12 +0200 Christoph Hellwig <[email protected]> wrote:

Quoted 1 line

How about this?

As I tried to explain before this utterly confuses the I/O completion path. With the patch applied even a simple mkfs.xfs that issues discard just hangs.

Wired. I just tried mkfs.xfs against scsi_debug with my block patches (I saw one discard command). Seemed that it worked fine.

Quoted 5 lines · On Sun, 27 Jun 2010 13:07:12 +0200 Christoph Hellwig <[email protected]> wrote:

Quoted 1 line

How about this?

As I tried to explain before this utterly confuses the I/O completion path. With the patch applied even a simple mkfs.xfs that issues discard just hangs.

Wired. I just tried mkfs.xfs against scsi_debug with my block patches (I saw one discard command). Seemed that it worked fine.

Quoted 9 lines · FUJITA Tomonori

Quoted 5 lines · On Sun, 27 Jun 2010 13:07:12 +0200 Christoph Hellwig <[email protected]> wrote:

Quoted 1 line

How about this?

As I tried to explain before this utterly confuses the I/O completion path. With the patch applied even a simple mkfs.xfs that issues discard just hangs.

Quoted 2 lines

Wired. I just tried mkfs.xfs against scsi_debug with my block patches (I saw one discard command). Seemed that it worked fine.

I've tracked it down to the call to scsi_requeue_command in scsi_end_request. When the command is marked BLOCK_PC we'll just get it back as such in ->prep_fn next time, but now it's reverting to the previous state.

While I see the problems with leaking ressources in that case I still can't quite explain the hang I see.

Quoted 10 lines · On Mon, 28 Jun 2010 09:57:38 +0200 Christoph Hellwig <[email protected]> wrote:

Quoted 9 lines · FUJITA Tomonori

Quoted 5 lines · On Sun, 27 Jun 2010 13:07:12 +0200 Christoph Hellwig <[email protected]> wrote:

Quoted 1 line

How about this?

As I tried to explain before this utterly confuses the I/O completion path. With the patch applied even a simple mkfs.xfs that issues discard just hangs.

Quoted 6 lines

Quoted 2 lines

Wired. I just tried mkfs.xfs against scsi_debug with my block patches (I saw one discard command). Seemed that it worked fine.

I've tracked it down to the call to scsi_requeue_command in scsi_end_request. When the command is marked BLOCK_PC we'll just get it back as such in ->prep_fn next time, but now it's reverting to the previous state.

If scsi_end_request() calls scsi_requeue_command(), the command has a left over (i.e. hasn't finished all the data), right? You hit such condition with discard commands?

BLOCK_PC requests don't hit this case since blk_end_request() always return false for PC.

Quoted 24 lines · FUJITA Tomonori

Quoted 10 lines · On Mon, 28 Jun 2010 09:57:38 +0200 Christoph Hellwig <[email protected]> wrote:

Quoted 9 lines · FUJITA Tomonori

Quoted 5 lines · On Sun, 27 Jun 2010 13:07:12 +0200 Christoph Hellwig <[email protected]> wrote:

Quoted 1 line

How about this?

As I tried to explain before this utterly confuses the I/O completion path. With the patch applied even a simple mkfs.xfs that issues discard just hangs.

Quoted 6 lines

Quoted 2 lines

Wired. I just tried mkfs.xfs against scsi_debug with my block patches (I saw one discard command). Seemed that it worked fine.

I've tracked it down to the call to scsi_requeue_command in scsi_end_request. When the command is marked BLOCK_PC we'll just get it back as such in ->prep_fn next time, but now it's reverting to the previous state.

If scsi_end_request() calls scsi_requeue_command(), the command has a left over (i.e. hasn't finished all the data), right? You hit such condition with discard commands?

Quoted 2 lines

BLOCK_PC requests don't hit this case since blk_end_request() always return false for PC.

You can get requeues on the ->queuecommand() path as well, for a variety of reasons, and that would be what Christoph is hitting.

Quoted 2 lines

While I see the problems with leaking ressources in that case I still can't quite explain the hang I see.

Any way to reproduce the hang without ssd drives?

Quoted 4 lines · FUJITA Tomonori

Quoted 2 lines

While I see the problems with leaking ressources in that case I still can't quite explain the hang I see.

Any way to reproduce the hang without ssd drives?

Actually the SSDs don't fully hang, they just causes lots of I/O errors and hit the error handler hard. The hard hang is when running under qemu. Apply the patch below, then create an if=scsi drive that resides on an XFS filesystem, and you'll have scsi TP support in the guest:

Quoted 10 lines · On Mon, 28 Jun 2010 17:25:36 +0200 Christoph Hellwig <[email protected]> wrote:

Quoted 4 lines · FUJITA Tomonori

Quoted 2 lines

While I see the problems with leaking ressources in that case I still can't quite explain the hang I see.

Any way to reproduce the hang without ssd drives?

Actually the SSDs don't fully hang, they just causes lots of I/O errors and hit the error handler hard. The hard hang is when running under qemu. Apply the patch below, then create an if=scsi drive that resides on an XFS filesystem, and you'll have scsi TP support in the guest:

Ok, I figured out what's wrong.

As I suspected, it's due to the partial completion.

qemu scsi driver tells that the WRITE_SAME command was successful but somehow the command has resid. So we retry it again and again (and leak some memory).

I don't know yet why qemu scsi driver is broken. Maybe there is a bug in it or converting discard to FS sends broken commands to the driver.

I've put a patch to complete discard command in the all-or-nothing manner:

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

Fix leaks introduced via "block: don't allocate a payload for discard request" commit a1d949f5f44.

sd_done() is not called for REQ_TYPE_BLOCK_PC commands so cleanup discard request's payload directly in scsi_finish_command().

Also cleanup page allocated for discard payload in scsi_setup_discard_cmnd's scsi_setup_blk_pc_cmnd error path.

Fix leaks introduced via "block: don't allocate a payload for discard request" commit a1d949f5f44.

sd_done() is not called for REQ_TYPE_BLOCK_PC commands so cleanup discard request's payload directly in scsi_finish_command().

Also cleanup page allocated for discard payload in scsi_setup_discard_cmnd's scsi_setup_blk_pc_cmnd error path.

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

---
 block/blk-core.c       |   23 +++++++++++++++++++++++
 drivers/scsi/scsi.c    |    8 ++++++++
 drivers/scsi/sd.c      |   18 ++++++++----------
 include/linux/blkdev.h |    1 +
 4 files changed, 40 insertions(+), 10 deletions(-)

block/blk-core.c +23 -0

index 98b4cee..07925aa 100644
@@ -1167,6 +1167,29 @@ void blk_add_request_payload(struct request *rq, struct page *page,
 }
 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;

drivers/scsi/scsi.c +0 -0

index ad0ed21..69c7ea4 100644
@@ -851,6 +851,14 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
 		 */

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"James" == James Bottomley <[email protected]> writes:

I'm still dreaming of the day where libata moves out from under SCSI so we don't have to translate square pegs into round holes...

linux-scsi cc added, since it's a SCSI patch.

Quoted 57 lines · Mike Snitzer

Fix leaks introduced via "block: don't allocate a payload for discard request" commit a1d949f5f44.

sd_done() is not called for REQ_TYPE_BLOCK_PC commands so cleanup discard request's payload directly in scsi_finish_command().

Also cleanup page allocated for discard payload in scsi_setup_discard_cmnd's scsi_setup_blk_pc_cmnd error path.

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

---
 block/blk-core.c       |   23 +++++++++++++++++++++++
 drivers/scsi/scsi.c    |    8 ++++++++
 drivers/scsi/sd.c      |   18 ++++++++----------
 include/linux/blkdev.h |    1 +
 4 files changed, 40 insertions(+), 10 deletions(-)

block/blk-core.c +23 -0

index 98b4cee..07925aa 100644
@@ -1167,6 +1167,29 @@ void blk_add_request_payload(struct request *rq, struct page *page,
 }
 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;

drivers/scsi/scsi.c +0 -0

index ad0ed21..69c7ea4 100644
@@ -851,6 +851,14 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
 		 */

drivers/scsi/scsi.c void scsi_finish_command(struct scsi_cmnd *cmd) · line 861

Quoted 10 lines

drivers/scsi/scsi.c +8 -0

 		if (good_bytes == old_good_bytes)
 			good_bytes -= scsi_get_resid(cmd);
+	} else if (cmd->request->cmd_flags & REQ_DISCARD) {
+		/*
+		 * If this is a discard request that originated from the kernel
+		 * we need to free our payload here.  Note that we need to check
+		 * the request flag as the normal payload rules apply for
+		 * pass-through UNMAP / WRITE SAME requests.
+		 */
+		__free_page(bio_page(cmd->request->bio));

This is another layering violation: the page is allocated in the Upper layer and freed in the mid-layer.

I really hate these growing contortions for discard. They're a clear signal that we haven't implemented it right.

So let's first work out how it should be done. I really like Tomo's idea of doing discard through the normal REQ_TYPE_FS route, which means we can control the setup in prep and the tear down in done, all confined to the ULD.

Where I think I'm at is partially what Christoph says: The command transformation belongs in the ULD so that's where the allocation and deallocation should be, and partly what Tomo says in that we should eliminate the special case paths.

The payload vs actual request size should be a red herring if we've got everything correct: only the ULD cares about the request parameters. Once we've got everything set up, the mid layer and LLD should only care about the parameters in the command, so we can confine the size changing part to the ULD doing the discard.

Could someone take a stab at this and see if it works without layering violations or any other problematic signals?

Thanks,

James

Quoted 93 lines · James Bottomley

linux-scsi cc added, since it's a SCSI patch.

Quoted 57 lines · Mike Snitzer

Fix leaks introduced via "block: don't allocate a payload for discard request" commit a1d949f5f44.

sd_done() is not called for REQ_TYPE_BLOCK_PC commands so cleanup discard request's payload directly in scsi_finish_command().

Also cleanup page allocated for discard payload in scsi_setup_discard_cmnd's scsi_setup_blk_pc_cmnd error path.

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

---
 block/blk-core.c       |   23 +++++++++++++++++++++++
 drivers/scsi/scsi.c    |    8 ++++++++
 drivers/scsi/sd.c      |   18 ++++++++----------
 include/linux/blkdev.h |    1 +
 4 files changed, 40 insertions(+), 10 deletions(-)

block/blk-core.c +23 -0

index 98b4cee..07925aa 100644
@@ -1167,6 +1167,29 @@ void blk_add_request_payload(struct request *rq, struct page *page,
 }
 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;

drivers/scsi/scsi.c +0 -0

index ad0ed21..69c7ea4 100644
@@ -851,6 +851,14 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
 		 */

drivers/scsi/scsi.c void scsi_finish_command(struct scsi_cmnd *cmd) · line 861

Quoted 10 lines

drivers/scsi/scsi.c +8 -0

 		if (good_bytes == old_good_bytes)
 			good_bytes -= scsi_get_resid(cmd);
+	} else if (cmd->request->cmd_flags & REQ_DISCARD) {
+		/*
+		 * If this is a discard request that originated from the kernel
+		 * we need to free our payload here.  Note that we need to check
+		 * the request flag as the normal payload rules apply for
+		 * pass-through UNMAP / WRITE SAME requests.
+		 */
+		__free_page(bio_page(cmd->request->bio));

This is another layering violation: the page is allocated in the Upper layer and freed in the mid-layer.

I really hate these growing contortions for discard. They're a clear signal that we haven't implemented it right.

So let's first work out how it should be done. I really like Tomo's idea of doing discard through the normal REQ_TYPE_FS route, which means we can control the setup in prep and the tear down in done, all confined to the ULD.

Where I think I'm at is partially what Christoph says: The command transformation belongs in the ULD so that's where the allocation and deallocation should be, and partly what Tomo says in that we should eliminate the special case paths.

The payload vs actual request size should be a red herring if we've got everything correct: only the ULD cares about the request parameters. Once we've got everything set up, the mid layer and LLD should only care about the parameters in the command, so we can confine the size changing part to the ULD doing the discard.

Quoted 6 lines

Could someone take a stab at this and see if it works without layering violations or any other problematic signals?

Thanks,

James

Well, I think that you overestimate the importance of scsi code too much.

There is a layering violation in the code. So what --- you either fix the layering violation or let it be there and grind your teeth on it. But in either case, that layering violation won't affect anyone except scsi developers.

On the other hand, if you say "because we want to avoid layering violation in SCSI, every issuer of discard request must supply an empty page", you create havoc all over the Linux codebase. md, dm, drbd, xvd, virtio --- whatever you think of, will be allocating a dummy page when constructing a discard request.

If the layering violation spans only scsi code, it can be eventually fixed, but this, much worse "layering violation" that will be spanning all block device midlayers, won't ever be fixed.

Imagine for example --- a discard request arrivers at a dm-snapshot device. The driver splits it into chunks, remaps each chunk to the physical chunk, submits the requests, the elevator merges adjacent requests and submits fewer bigger requests to the device. Now, if you had to allocate a zeroed page each time you are splitting the request, that would exhaust memory and burn cpu needlessly. You delete a 100MB file? --- fine, allocate a 100MB of zeroed pages.

So I say --- let there be a layering violation in the scsi code, but don't put this problem with a page allocation to all the other bio midlayer developers.

Quoted 99 lines · Mikulas Patocka

Quoted 96 lines · James Bottomley

linux-scsi cc added, since it's a SCSI patch.

Quoted 57 lines · Mike Snitzer

Fix leaks introduced via "block: don't allocate a payload for discard request" commit a1d949f5f44.

sd_done() is not called for REQ_TYPE_BLOCK_PC commands so cleanup discard request's payload directly in scsi_finish_command().

Also cleanup page allocated for discard payload in scsi_setup_discard_cmnd's scsi_setup_blk_pc_cmnd error path.

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

---
 block/blk-core.c       |   23 +++++++++++++++++++++++
 drivers/scsi/scsi.c    |    8 ++++++++
 drivers/scsi/sd.c      |   18 ++++++++----------
 include/linux/blkdev.h |    1 +
 4 files changed, 40 insertions(+), 10 deletions(-)

block/blk-core.c +23 -0

index 98b4cee..07925aa 100644
@@ -1167,6 +1167,29 @@ void blk_add_request_payload(struct request *rq, struct page *page,
 }
 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;

drivers/scsi/scsi.c +0 -0

index ad0ed21..69c7ea4 100644
@@ -851,6 +851,14 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
 		 */

drivers/scsi/scsi.c void scsi_finish_command(struct scsi_cmnd *cmd) · line 861

Quoted 10 lines

drivers/scsi/scsi.c +8 -0

 		if (good_bytes == old_good_bytes)
 			good_bytes -= scsi_get_resid(cmd);
+	} else if (cmd->request->cmd_flags & REQ_DISCARD) {
+		/*
+		 * If this is a discard request that originated from the kernel
+		 * we need to free our payload here.  Note that we need to check
+		 * the request flag as the normal payload rules apply for
+		 * pass-through UNMAP / WRITE SAME requests.
+		 */
+		__free_page(bio_page(cmd->request->bio));

This is another layering violation: the page is allocated in the Upper layer and freed in the mid-layer.

I really hate these growing contortions for discard. They're a clear signal that we haven't implemented it right.

So let's first work out how it should be done. I really like Tomo's idea of doing discard through the normal REQ_TYPE_FS route, which means we can control the setup in prep and the tear down in done, all confined to the ULD.

Where I think I'm at is partially what Christoph says: The command transformation belongs in the ULD so that's where the allocation and deallocation should be, and partly what Tomo says in that we should eliminate the special case paths.

The payload vs actual request size should be a red herring if we've got everything correct: only the ULD cares about the request parameters. Once we've got everything set up, the mid layer and LLD should only care about the parameters in the command, so we can confine the size changing part to the ULD doing the discard.

Could someone take a stab at this and see if it works without layering violations or any other problematic signals?

Quoted 5 lines

Quoted 3 lines

Thanks,

James

Well, I think that you overestimate the importance of scsi code too much.

Not, I think, a deadly sin for a SCSI maintainer.

Quoted 4 lines

There is a layering violation in the code. So what --- you either fix the layering violation or let it be there and grind your teeth on it. But in either case, that layering violation won't affect anyone except scsi developers.

A layering violation is a signal of bad design wherever it occurs, so that wasn't a SCSI centric argument.

Quoted 5 lines

On the other hand, if you say "because we want to avoid layering violation in SCSI, every issuer of discard request must supply an empty page", you create havoc all over the Linux codebase. md, dm, drbd, xvd, virtio --- whatever you think of, will be allocating a dummy page when constructing a discard request.

Since I didn't actually say any of that, I suggest you re-read text you quoted above. The phrase "The command transformation belongs in the ULD so that's where the allocation and deallocation should be" might be a relevant one to concentrate on.

Quoted 11 lines

If the layering violation spans only scsi code, it can be eventually fixed, but this, much worse "layering violation" that will be spanning all block device midlayers, won't ever be fixed.

Imagine for example --- a discard request arrivers at a dm-snapshot device. The driver splits it into chunks, remaps each chunk to the physical chunk, submits the requests, the elevator merges adjacent requests and submits fewer bigger requests to the device. Now, if you had to allocate a zeroed page each time you are splitting the request, that would exhaust memory and burn cpu needlessly. You delete a 100MB file? --- fine, allocate a 100MB of zeroed pages.

This is a straw man: You've tried to portray a position I've never taken as mine then attack it ... with what is effectively another bogus argument.

It's not an either/or choice. I've asked the relevant parties to combine the approaches and see if a REQ_TYPE_FS path that does the allocations in the appropriate place, likely the ULD, produces a good design.

Quoted 11 lines

If the layering violation spans only scsi code, it can be eventually fixed, but this, much worse "layering violation" that will be spanning all block device midlayers, won't ever be fixed.

Imagine for example --- a discard request arrivers at a dm-snapshot device. The driver splits it into chunks, remaps each chunk to the physical chunk, submits the requests, the elevator merges adjacent requests and submits fewer bigger requests to the device. Now, if you had to allocate a zeroed page each time you are splitting the request, that would exhaust memory and burn cpu needlessly. You delete a 100MB file? --- fine, allocate a 100MB of zeroed pages.

This is a straw man: You've tried to portray a position I've never taken as mine then attack it ... with what is effectively another bogus argument.

It's not an either/or choice.

I've asked the relevant parties to
combine the approaches and see if a REQ_TYPE_FS path that does the allocations in the appropriate place, likely the ULD, produces a good design.

Quoted 3 lines

So I say --- let there be a layering violation in the scsi code, but don't put this problem with a page allocation to all the other bio midlayer developers.

Thanks for explaining that you have nothing to contribute, I'll make sure you're not on my list of relevant parties.

Quoted 12 lines

Quoted 11 lines

If the layering violation spans only scsi code, it can be eventually fixed, but this, much worse "layering violation" that will be spanning all block device midlayers, won't ever be fixed.

Imagine for example --- a discard request arrivers at a dm-snapshot device. The driver splits it into chunks, remaps each chunk to the physical chunk, submits the requests, the elevator merges adjacent requests and submits fewer bigger requests to the device. Now, if you had to allocate a zeroed page each time you are splitting the request, that would exhaust memory and burn cpu needlessly. You delete a 100MB file? --- fine, allocate a 100MB of zeroed pages.

Quoted 5 lines

This is a straw man: You've tried to portray a position I've never taken as mine then attack it ... with what is effectively another bogus argument.

It's not an either/or choice.

It is either/or choice. If the interface isn't fixed NOW, the existing flawed zeroed-page-allocation interface gets into RHEL

and I and others will have to support it for 7 years.

Quoted 4 lines

I've asked the relevant parties to
combine the approaches and see if a REQ_TYPE_FS path that does the allocations in the appropriate place, likely the ULD, produces a good design.

OK, but before you do this research, fix the interface.

Quoted 6 lines

Quoted 3 lines

So I say --- let there be a layering violation in the scsi code, but don't put this problem with a page allocation to all the other bio midlayer developers.

Thanks for explaining that you have nothing to contribute, I'll make sure you're not on my list of relevant parties.

You misunderstand what I meant. You admit that there are design problems in SCSI.

So don't burden other developers with these problems. Don't force the others to allocate dummy pages just because you want a cleaner scsi code.

You intend to fix the design of SCSI and then remove the dummy pages. But by the time you finish it, it will be already late and there will be midlayer drivers allocating these dummy pages.

What I mean is that "layering violation" inside one driver is smaller problem than misdesigned interface between drivers. So accept the patch that creates "layering violation" but cleans up the interface.

Mikulas

Quoted 12 lines · Mikulas Patocka

Quoted 12 lines

Quoted 11 lines

If the layering violation spans only scsi code, it can be eventually fixed, but this, much worse "layering violation" that will be spanning all block device midlayers, won't ever be fixed.

Imagine for example --- a discard request arrivers at a dm-snapshot device. The driver splits it into chunks, remaps each chunk to the physical chunk, submits the requests, the elevator merges adjacent requests and submits fewer bigger requests to the device. Now, if you had to allocate a zeroed page each time you are splitting the request, that would exhaust memory and burn cpu needlessly. You delete a 100MB file? --- fine, allocate a 100MB of zeroed pages.

Quoted 8 lines

Quoted 5 lines

This is a straw man: You've tried to portray a position I've never taken as mine then attack it ... with what is effectively another bogus argument.

It's not an either/or choice.

It is either/or choice. If the interface isn't fixed NOW, the existing flawed zeroed-page-allocation interface gets into RHEL

That's a false dichotomy. You might see an either apply this hack now or support the interface choice with RHEL, but upstream has the option to fix stuff correctly. RHEL has never needed my blessing to apply random crap to their kernel before ... why is this patch any different?

Quoted 1 line

and I and others will have to support it for 7 years.

It's called a business model ... I believe it's what they pay you for.

Quoted 6 lines

Quoted 4 lines

I've asked the relevant parties to
combine the approaches and see if a REQ_TYPE_FS path that does the allocations in the appropriate place, likely the ULD, produces a good design.

OK, but before you do this research, fix the interface.

So even in the RHEL world, I think you'd find that analysing the problem *before* comping up with a fix is a good way of doing things.

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

So I say --- let there be a layering violation in the scsi code, but don't put this problem with a page allocation to all the other bio midlayer developers.

Thanks for explaining that you have nothing to contribute, I'll make sure you're not on my list of relevant parties.

You misunderstand what I meant. You admit that there are design problems in SCSI.

No I didn't.

And the rest of this rubbish is based on that false premise. It might help you to take off your SCSI antipathy and see this as a system
problem: it actually originates in block and spills out from there. Thus it requires a system solution.

Quoted 53 lines · James Bottomley

Quoted 12 lines · Mikulas Patocka

Quoted 12 lines

Quoted 11 lines

If the layering violation spans only scsi code, it can be eventually fixed, but this, much worse "layering violation" that will be spanning all block device midlayers, won't ever be fixed.

Imagine for example --- a discard request arrivers at a dm-snapshot device. The driver splits it into chunks, remaps each chunk to the physical chunk, submits the requests, the elevator merges adjacent requests and submits fewer bigger requests to the device. Now, if you had to allocate a zeroed page each time you are splitting the request, that would exhaust memory and burn cpu needlessly. You delete a 100MB file? --- fine, allocate a 100MB of zeroed pages.

Quoted 8 lines

Quoted 5 lines

This is a straw man: You've tried to portray a position I've never taken as mine then attack it ... with what is effectively another bogus argument.

It's not an either/or choice.

It is either/or choice. If the interface isn't fixed NOW, the existing flawed zeroed-page-allocation interface gets into RHEL

That's a false dichotomy. You might see an either apply this hack now or support the interface choice with RHEL, but upstream has the option to fix stuff correctly. RHEL has never needed my blessing to apply random crap to their kernel before ... why is this patch any different?

Quoted 1 line

and I and others will have to support it for 7 years.

It's called a business model ... I believe it's what they pay you for.

Quoted 6 lines

Quoted 4 lines

I've asked the relevant parties to
combine the approaches and see if a REQ_TYPE_FS path that does the allocations in the appropriate place, likely the ULD, produces a good design.

OK, but before you do this research, fix the interface.

So even in the RHEL world, I think you'd find that analysing the problem *before* comping up with a fix is a good way of doing things.

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

So I say --- let there be a layering violation in the scsi code, but don't put this problem with a page allocation to all the other bio midlayer developers.

Thanks for explaining that you have nothing to contribute, I'll make sure you're not on my list of relevant parties.

You misunderstand what I meant. You admit that there are design problems in SCSI.

No I didn't.

Quoted 4 lines

And the rest of this rubbish is based on that false premise. It might help you to take off your SCSI antipathy and see this as a system
problem: it actually originates in block and spills out from there. Thus it requires a system solution.

As fun as it is for the others monitoring these lists to see redhat.com vs suse.de banter I think framing this discussion like you (and Mikulas) continue to do is a complete distraction.

I tried to elevate (and defuse) the discussion yesterday. But simply
put: patches speak volumes. I look forward to working with Tomo, hch and anyone else who has something to contribute that moves us toward a real fix for discards.

Quoted 2 lines

It is either/or choice. If the interface isn't fixed NOW, the existing flawed zeroed-page-allocation interface gets into RHEL

That's a false dichotomy. You might see an either apply this hack now or support the interface choice with RHEL, but upstream has the option to fix stuff correctly. RHEL has never needed my blessing to apply random crap to their kernel before ... why is this patch any different?

And the rest of this rubbish is based on that false premise. It might help you to take off your SCSI antipathy and see this as a system
problem: it actually originates in block and spills out from there. Thus it requires a system solution.

James

Quoted 7 lines

Quoted 2 lines

It is either/or choice. If the interface isn't fixed NOW, the existing flawed zeroed-page-allocation interface gets into RHEL

That's a false dichotomy. You might see an either apply this hack now or support the interface choice with RHEL, but upstream has the option to fix stuff correctly. RHEL has never needed my blessing to apply random crap to their kernel before ... why is this patch any different?

We can't apply non-upstream patches (except few exceptions such as dm-raid45). It makes sense, non-upstream patches have smaller test coverage.

Quoted 6 lines

And the rest of this rubbish is based on that false premise. It might help you to take off your SCSI antipathy and see this as a system
problem: it actually originates in block and spills out from there. Thus it requires a system solution.

James

Imagine this: I take a FPGA PCI board, I design a storage controller on it and this controller will need 3 pages to process a discard request. Now I
say: I refuse to allocate these 3 pages in the driver because the driver would look ugly --- instead, I demand that everyone in the Linux kernel who creates a discard request must attach 3 pages to the request for my driver.

Do you think it is correct behavior? Would you accept such a driver? I guess you wouldn't! But this is the same thing that you are doing with SCSI.

Now lets take it a bit further and I say "I may clean up the driver for my controller one day, when I do it, I remove that 3-page requirement --- and then, everyone who allocated those pages will have to change his code and remove the allocations".

And this is what you are intending to do with SCSI.

Mikulas,

Jens has already queued up a comprehensive fix (3 patches) that James and Tomo developed. Please stop the hostility.. it has no place.

Others,
I'd encourage you to not respond to this thread further ;)

Regards,
Mike

But this is the same thing that you are doing with SCSI.

drivers/scsi/scsi.c void scsi_finish_command(struct scsi_cmnd *cmd) · line 861

Quoted 7 lines

drivers/scsi/scsi.c +7 -0

+		/*
+		 * If this is a discard request that originated from the kernel
+		 * we need to free our payload here.  Note that we need to check
+		 * the request flag as the normal payload rules apply for
+		 * pass-through UNMAP / WRITE SAME requests.
+		 */
+		__free_page(bio_page(cmd->request->bio));

This is another layering violation: the page is allocated in the Upper layer and freed in the mid-layer.

May I ask a silly question? Why the dynamic allocation?

Why not have a const-static single global page at the block-layer somewhere that will be used for all discard-type operations and be done with it once and for all. A single page can be used for any size bio , any number of concurrent discards, any ZERO needed operation. It can also be used by other operations like padding and others. In fact isn't there one for the libsata padding?

Quoted 7 lines · Boaz Harrosh

May I ask a silly question? Why the dynamic allocation?

Why not have a const-static single global page at the block-layer somewhere that will be used for all discard-type operations and be done with it once and for all. A single page can be used for any size bio , any number of concurrent discards, any ZERO needed operation. It can also be used by other operations like padding and others. In fact isn't there one for the libsata padding?

for UNMAP we need to write into the payload. And for ATA TRIM we need to write into the WRITE SAME payload.

That's another layering violation
for those looking for them, btw..

OK, Thanks, I see. Is it one of these operations, (like we have in OSD) where the CDB information spills into the payload? like the scatter-gather and extent lists and such.

Do we actually use a WRITE_SAME which is not zero? for what use?

Quoted 3 lines · Boaz Harrosh

OK, Thanks, I see. Is it one of these operations, (like we have in OSD) where the CDB information spills into the payload? like the scatter-gather and extent lists and such.

For UNMAP the payload is a list of block number / length pairs, while the CDB itself doesn't contain any information like that. It's a rather awkward command.

Quoted 1 line

Do we actually use a WRITE_SAME which is not zero? for what use?

The kernel doesn't issue any WRITE SAME without the unmap bit set.

Quoted 9 lines · Christoph Hellwig

Quoted 3 lines · Boaz Harrosh

OK, Thanks, I see. Is it one of these operations, (like we have in OSD) where the CDB information spills into the payload? like the scatter-gather and extent lists and such.

For UNMAP the payload is a list of block number / length pairs, while the CDB itself doesn't contain any information like that. It's a rather awkward command.

How big can that be? could we, maybe, use the sense_buffer, properly allocated already?

Quoted 2 lines

Quoted 1 line

Do we actually use a WRITE_SAME which is not zero? for what use?

Quoted 1 line

The kernel doesn't issue any WRITE SAME without the unmap bit set.

So if the unmap bit is set then the page can just be zero, right?

I still think a static zero-page is a worth while optimization. And block-drivers can take care with special needs with a private mem_pool or something. For the discard-type user and generic block layer the page is just an implementation specific residue, No?

Don't alloc discard bio with a biovec in blkdev_issue_discard. Doing so means bio_has_data() will not be true until the SCSI layer adds the payload to the discard request via blk_add_request_payload.

bio_{enable,disable}_inline_vecs are not expected to be widely used so they were exported using EXPORT_SYMBOL_GPL.

Quoted 6 lines · Mike Snitzer

Don't alloc discard bio with a biovec in blkdev_issue_discard. Doing so means bio_has_data() will not be true until the SCSI layer adds the payload to the discard request via blk_add_request_payload.

bio_{enable,disable}_inline_vecs are not expected to be widely used so they were exported using EXPORT_SYMBOL_GPL.

Why do we need them exported at all?

Also some comments on these functions would be useful.

Otherwise it looks good to me.

Don't alloc discard bio with a biovec in blkdev_issue_discard. Doing so means bio_has_data() will not be true until the SCSI layer adds the payload to the discard request via blk_add_request_payload.

bio_{enable,disable}_inline_vecs are not expected to be widely used so they were exported using EXPORT_SYMBOL_GPL.

This patch avoids the need for the following VM accounting fix for
discards: http://lkml.org/lkml/2010/6/23/361

NOTE: Jens, you said you applied Tao Ma's fix but I cannot see it in your linux-2.6-block's for-next or for-2.6.36... as such I didn't revert it in this patch.

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

---
 block/blk-core.c    |    2 ++
 block/blk-lib.c     |    2 +-
 fs/bio.c            |   19 +++++++++++++++++--
 include/linux/bio.h |    3 +++
 4 files changed, 23 insertions(+), 3 deletions(-)

Quoted 4 lines · On Sat, 26 Jun 2010 15:56:51 -0400 Mike Snitzer <[email protected]> wrote:

Don't alloc discard bio with a biovec in blkdev_issue_discard. Doing so means bio_has_data() will not be true until the SCSI layer adds the payload to the discard request via blk_add_request_payload.

Quoted 5 lines

bio_{enable,disable}_inline_vecs are not expected to be widely used so they were exported using EXPORT_SYMBOL_GPL.

This patch avoids the need for the following VM accounting fix for
discards: http://lkml.org/lkml/2010/6/23/361

Why do we need to avoid the above fix?

Surely, the above fix is hacky but much simpler than this patch.

Quoted 7 lines · FUJITA Tomonori

Quoted 4 lines · On Sat, 26 Jun 2010 15:56:51 -0400 Mike Snitzer <[email protected]> wrote:

Don't alloc discard bio with a biovec in blkdev_issue_discard. Doing so means bio_has_data() will not be true until the SCSI layer adds the payload to the discard request via blk_add_request_payload.

Quoted 7 lines

Quoted 5 lines

bio_{enable,disable}_inline_vecs are not expected to be widely used so they were exported using EXPORT_SYMBOL_GPL.

This patch avoids the need for the following VM accounting fix for
discards: http://lkml.org/lkml/2010/6/23/361

Why do we need to avoid the above fix?

We don't _need_ to. We avoid the need for it as a side-effect of the cleanup that my patch provides.

Quoted 1 line

Surely, the above fix is hacky but much simpler than this patch.

My patch wasn't meant as an alternative to Tao Ma's patch. Again, it just obviates the need for it.

Your tolerance for "hacky" is difficult to understand. On the one-hand (PATCH 1/2) you have no tolerance for "hacky" fixes for leaks (that introduce a short-term SCSI layering violation).

But in this case
you're perfectly fine with BIO_RW_DISCARD special casing?

Quoted 21 lines · On Mon, 28 Jun 2010 08:29:55 -0400 Mike Snitzer <[email protected]> wrote:

Quoted 7 lines · FUJITA Tomonori

Quoted 4 lines · On Sat, 26 Jun 2010 15:56:51 -0400 Mike Snitzer <[email protected]> wrote:

Don't alloc discard bio with a biovec in blkdev_issue_discard. Doing so means bio_has_data() will not be true until the SCSI layer adds the payload to the discard request via blk_add_request_payload.

Quoted 7 lines

Quoted 5 lines

bio_{enable,disable}_inline_vecs are not expected to be widely used so they were exported using EXPORT_SYMBOL_GPL.

This patch avoids the need for the following VM accounting fix for
discards: http://lkml.org/lkml/2010/6/23/361

Why do we need to avoid the above fix?

We don't _need_ to. We avoid the need for it as a side-effect of the cleanup that my patch provides.

Quoted 8 lines

Quoted 1 line

Surely, the above fix is hacky but much simpler than this patch.

My patch wasn't meant as an alternative to Tao Ma's patch. Again, it just obviates the need for it.

Your tolerance for "hacky" is difficult to understand. On the one-hand (PATCH 1/2) you have no tolerance for "hacky" fixes for leaks (that introduce a short-term SCSI layering violation).

Sorry, if not clear enough.

- SCSI layering violation is bad.

- A 'short term' solution always turns out to be a long solution. We should have a clean solution from the start.

- Complicating the SCSI I/O completion is bad (already complicated enough).

...

And the 'leaks' bug is still in -next. No need to fix it in a hacky way. We can just drop it from -next.

Quoted 2 lines

But in this case
you're perfectly fine with BIO_RW_DISCARD special casing?

BIO_RW_DISCARD special is already everywhere in the block layer. I prefer to have the less. However as long as it's in the block layer, I can live with it. After all, that's the block layer thing.

At least, it looks much better this patch. This patch is really hacky (as Jens said).

Don't alloc discard bio with a biovec in blkdev_issue_discard. Doing so means bio_has_data() will not be true until the SCSI layer adds the payload to the discard request via blk_add_request_payload.

bio_{enable,disable}_inline_vecs are not expected to be widely used so they were exported using EXPORT_SYMBOL_GPL.

This patch avoids the need for the following VM accounting fix for
discards: http://lkml.org/lkml/2010/6/23/361
NOTE: Jens, you said you applied Tao Ma's fix but I cannot see it in your linux-2.6-block's for-next or for-2.6.36... as such I didn't revert it in this patch.

Quoted 3 lines · Mike Snitzer

Don't alloc discard bio with a biovec in blkdev_issue_discard. Doing so means bio_has_data() will not be true until the SCSI layer adds the payload to the discard request via blk_add_request_payload.

Sorry, this looks horrible.

What is the point of this?!

Quoted 2 lines

bio_{enable,disable}_inline_vecs are not expected to be widely used so they were exported using EXPORT_SYMBOL_GPL.

Never export anything that doesn't have an in-kernel modular user.

Quoted 5 lines

This patch avoids the need for the following VM accounting fix for
discards: http://lkml.org/lkml/2010/6/23/361
NOTE: Jens, you said you applied Tao Ma's fix but I cannot see it in your linux-2.6-block's for-next or for-2.6.36... as such I didn't revert it in this patch.

It's in the for-linus branch, that is stuff headed for the current kernel.

Quoted 1 line · Jens Axboe

On 2010-06-26 21:56, Mike Snitzer wrote:

Quoted 5 lines

Quoted 3 lines

Don't alloc discard bio with a biovec in blkdev_issue_discard. Doing so means bio_has_data() will not be true until the SCSI layer adds the payload to the discard request via blk_add_request_payload.

Sorry, this looks horrible.

Your judgment isn't giving me much to work with... not sure where I go with "horrible".

The horrible part is working around that issue by fiddling with the assignment of the internal vec. THAT looks like a horrible solution to that problem.

How about just adding a check to bio_has_data() for non-zero bio->bi_vcnt?

Quoted 4 lines · Jens Axboe

The horrible part is working around that issue by fiddling with the assignment of the internal vec. THAT looks like a horrible solution to that problem.

Quoted 2 lines

How about just adding a check to bio_has_data() for non-zero bio->bi_vcnt?

The question is how a discard request from the block layer should look like. With Mike's patch we have the same situation as for a barrier
request: absolutely no data transferred and no indicator of it. IHMO that's much better than any partially constructed request. And yes, that means enabling the payload later in the driver.

The other option would be to not reuse the request at all and just allocate a new request and use that from sd_prep_fn. That's what I tried to implement first, but I couldn't get it to work. Given all the issue we have with the current approach I'm almost tempted to try that again.

Quoted 4 lines · Mike Snitzer

Quoted 1 line · Jens Axboe

On 2010-06-26 21:56, Mike Snitzer wrote:

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

Don't alloc discard bio with a biovec in blkdev_issue_discard. Doing so means bio_has_data() will not be true until the SCSI layer adds the payload to the discard request via blk_add_request_payload.

Sorry, this looks horrible.

Your judgment isn't giving me much to work with... not sure where I go with "horrible".

The horrible part is working around that issue by fiddling with the assignment of the internal vec. THAT looks like a horrible solution to that problem.

How about just adding a check to bio_has_data() for non-zero bio->bi_vcnt?