Re: next bio iters break discard?

[Kent Overstreet]

On Tue, Jan 14, 2014 at 03:17:32PM -0500, Martin K. Petersen wrote:
> >>>>> "Kent" == Kent Overstreet <kmo@xxxxxxxxxxxxx> writes:
> 
> >> IOW, DISCARD, WRITE SAME and the impending COPY requests do not have
> >> a 1:1 mapping between the block range worked on and the size of any
> >> bvecs attached. Your recent changes must have changed the way we
> >> handled that in the past.
> 
> Kent> Yeah - but with WRITE_SAME bios, wouldn't we at least have to
> Kent> check that they're writing the same data to merge them?
> 
> We do. Check blk_write_same_mergeable().

Aha, I missed that.

Side note, one of the things on my todo list/wishlist is make a separate
enum for bio type - bare flush, normal read/write, scsi command, discard
and write same. In particular with bi_size meaning different things
depending on the type of the command, I think having an enum we can
switch off of where appropriate instead of a bunch of random flags will
make things a hell of a lot saner.

Looking more at this code, I'm not sure it did what we wanted before for
REQ_DISCARD and REQ_WRITE_SAME bios/requests - I suspect for
REQ_WRITE_SAME requests that had been merged it overcounted.

There's also that horrible hack in the scsi (?) code Christoph added for
discards - where the driver adds a page to the bio - if the driver is
counting the number of segments _after_ that god only knows what is
supposed to happen.

Does the below patch look like what we want? I'm assuming that if
multiple WRITE_SAME bios are merged, since they're all writing the same
data we can consider the entire request to be a single segment.

commit 1755e7ffc5745591d37b8956ce2512f4052a104a
Author: Kent Overstreet <kmo@xxxxxxxxxxxxx>
Date:   Tue Jan 14 14:22:01 2014 -0800

    block: Explicitly handle discard/write same when counting segments

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 8f8adaa..7d977f8 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -21,6 +21,12 @@ static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
 	if (!bio)
 		return 0;
 
+	if (bio->bi_rw & REQ_DISCARD)
+		return 0;
+
+	if (bio->bi_rw & REQ_WRITE_SAME)
+		return 1;
+
 	fbio = bio;
 	cluster = blk_queue_cluster(q);
 	seg_size = 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/