linux-kernel - Re: next bio iters break discard?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 14 Jan 2014 14:24:24 -0800
From:	Kent Overstreet <kmo@...erainc.com>
To:	"Martin K. Petersen" <martin.petersen@...cle.com>
Cc:	Hugh Dickins <hughd@...gle.com>, Jens Axboe <axboe@...nel.dk>,
	Shaohua Li <shli@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: next bio iters break discard?

On Tue, Jan 14, 2014 at 03:17:32PM -0500, Martin K. Petersen wrote:
> >>>>> "Kent" == Kent Overstreet <kmo@...erainc.com> 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@...erainc.com>
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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/