[PATCH V3 0/3] block: fix discard merge for single max discard segment

[PATCH V3 0/3] block: fix discard merge for single max discard segment

[Ming Lei]

Hello,

The 1st patch checks max discard segment limit in request merge code,
and one discard request failure is fixed for virtio_blk.

The 2nd patch fixes handling of single max discard segment for virtio_blk,
and potential memory corruption is fixed.

The 3rd patch renames the confusing blk_discard_mergable().

The biggest problem is that virtio_blk handles discard request in
multi-range style, however it(at least qemu) may claim that multi range
discard isn't supported by returning 1 for max discard segment.

V3:
	- one patch style change in patch 1, as suggested by Christoph
	- add reviewed-by tag

V2:
	- add comment
	- warn on mismatched bios and discard segment number
	- rename blk_discard_mergable


Ming Lei (3):
  block: respect queue limit of max discard segment
  block: virtio_blk: fix handling single range discard request
  block: rename blk_discard_mergable as blk_discard_support_multi_range

 block/blk-merge.c          | 22 ++++++++++++++++------
 drivers/block/virtio_blk.c | 31 +++++++++++++++++++++++--------
 2 files changed, 39 insertions(+), 14 deletions(-)

Cc: Christoph Hellwig <hch@lst.de>
Cc: Changpeng Liu <changpeng.liu@intel.com>
Cc: Daniel Verkamp <dverkamp@chromium.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Stefano Garzarella <sgarzare@redhat.com>
-- 
2.25.2

[PATCH V3 1/3] block: respect queue limit of max discard segment

[Ming Lei]

When queue_max_discard_segments(q) is 1, blk_discard_mergable() will
return false for discard request, then normal request merge is applied.
However, only queue_max_segments() is checked, so max discard segment
limit isn't respected.

Check max discard segment limit in the request merge code for fixing
the issue.

Discard request failure of virtio_blk is fixed.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
Fixes: 69840466086d ("block: fix the DISCARD request merge")
Reviewed-by: Christoph Hellwig <hch@lst.de>
Cc: Stefano Garzarella <sgarzare@redhat.com>
---
 block/blk-merge.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 6529e3aab001..7af1f3668a91 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -533,10 +533,17 @@ int __blk_rq_map_sg(struct request_queue *q, struct request *rq,
 }
 EXPORT_SYMBOL(__blk_rq_map_sg);
 
+static inline unsigned int blk_rq_get_max_segments(struct request *rq)
+{
+	if (req_op(rq) == REQ_OP_DISCARD)
+		return queue_max_discard_segments(rq->q);
+	return queue_max_segments(rq->q);
+}
+
 static inline int ll_new_hw_segment(struct request *req, struct bio *bio,
 		unsigned int nr_phys_segs)
 {
-	if (req->nr_phys_segments + nr_phys_segs > queue_max_segments(req->q))
+	if (req->nr_phys_segments + nr_phys_segs > blk_rq_get_max_segments(req))
 		goto no_merge;
 
 	if (blk_integrity_merge_bio(req->q, req, bio) == false)
@@ -624,7 +631,7 @@ static int ll_merge_requests_fn(struct request_queue *q, struct request *req,
 		return 0;
 
 	total_phys_segments = req->nr_phys_segments + next->nr_phys_segments;
-	if (total_phys_segments > queue_max_segments(q))
+	if (total_phys_segments > blk_rq_get_max_segments(req))
 		return 0;
 
 	if (blk_integrity_merge_rq(q, req, next) == false)
-- 
2.25.2

[PATCH V3 2/3] block: virtio_blk: fix handling single range discard request

[Ming Lei]

1f23816b8eb8 ("virtio_blk: add discard and write zeroes support") starts
to support multi-range discard for virtio-blk. However, the virtio-blk
disk may report max discard segment as 1, at least that is exactly what
qemu is doing.

So far, block layer switches to normal request merge if max discard segment
limit is 1, and multiple bios can be merged to single segment. This way may
cause memory corruption in virtblk_setup_discard_write_zeroes().

Fix the issue by handling single max discard segment in straightforward
way.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
Fixes: 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support")
Reviewed-by: Christoph Hellwig <hch@lst.de>
Cc: Changpeng Liu <changpeng.liu@intel.com>
Cc: Daniel Verkamp <dverkamp@chromium.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/block/virtio_blk.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 63b213e00b37..b2e48dac1ebd 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -126,16 +126,31 @@ static int virtblk_setup_discard_write_zeroes(struct request *req, bool unmap)
 	if (!range)
 		return -ENOMEM;
 
-	__rq_for_each_bio(bio, req) {
-		u64 sector = bio->bi_iter.bi_sector;
-		u32 num_sectors = bio->bi_iter.bi_size >> SECTOR_SHIFT;
-
-		range[n].flags = cpu_to_le32(flags);
-		range[n].num_sectors = cpu_to_le32(num_sectors);
-		range[n].sector = cpu_to_le64(sector);
-		n++;
+	/*
+	 * Single max discard segment means multi-range discard isn't
+	 * supported, and block layer only runs contiguity merge like
+	 * normal RW request. So we can't reply on bio for retrieving
+	 * each range info.
+	 */
+	if (queue_max_discard_segments(req->q) == 1) {
+		range[0].flags = cpu_to_le32(flags);
+		range[0].num_sectors = cpu_to_le32(blk_rq_sectors(req));
+		range[0].sector = cpu_to_le64(blk_rq_pos(req));
+		n = 1;
+	} else {
+		__rq_for_each_bio(bio, req) {
+			u64 sector = bio->bi_iter.bi_sector;
+			u32 num_sectors = bio->bi_iter.bi_size >> SECTOR_SHIFT;
+
+			range[n].flags = cpu_to_le32(flags);
+			range[n].num_sectors = cpu_to_le32(num_sectors);
+			range[n].sector = cpu_to_le64(sector);
+			n++;
+		}
 	}
 
+	WARN_ON_ONCE(n != segments);
+
 	req->special_vec.bv_page = virt_to_page(range);
 	req->special_vec.bv_offset = offset_in_page(range);
 	req->special_vec.bv_len = sizeof(*range) * segments;
-- 
2.25.2

[PATCH V3 3/3] block: rename blk_discard_mergable as blk_discard_support_multi_range

[Ming Lei]

Name of blk_discard_mergable() is very confusing, and this function
actually means if the queue supports multi_range discard. Also there
are two kinds of discard merge:

1) multi range discard, bios in one request won't have to be contiguous,
and actually each bio is thought as one range

2) single range discard, all bios in one request have to be contiguous
just like normal RW request's merge

Rename blk_discard_mergable() for not confusing people, and avoiding
to introduce bugs in future.

Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-merge.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 7af1f3668a91..47a03d2eed24 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -693,8 +693,11 @@ static void blk_account_io_merge_request(struct request *req)
  * needn't to be contiguous.
  * Otherwise, the bios/requests will be handled as same as
  * others which should be contiguous.
+ *
+ * queue_max_discard_segments() is > 1, the queue supports multi range
+ * discard.
  */
-static inline bool blk_discard_mergable(struct request *req)
+static inline bool blk_discard_support_multi_range(struct request *req)
 {
 	if (req_op(req) == REQ_OP_DISCARD &&
 	    queue_max_discard_segments(req->q) > 1)
@@ -705,7 +708,7 @@ static inline bool blk_discard_mergable(struct request *req)
 static enum elv_merge blk_try_req_merge(struct request *req,
 					struct request *next)
 {
-	if (blk_discard_mergable(req))
+	if (blk_discard_support_multi_range(req))
 		return ELEVATOR_DISCARD_MERGE;
 	else if (blk_rq_pos(req) + blk_rq_sectors(req) == blk_rq_pos(next))
 		return ELEVATOR_BACK_MERGE;
@@ -791,7 +794,7 @@ static struct request *attempt_merge(struct request_queue *q,
 
 	req->__data_len += blk_rq_bytes(next);
 
-	if (!blk_discard_mergable(req))
+	if (!blk_discard_support_multi_range(req))
 		elv_merge_requests(q, req, next);
 
 	/*
@@ -887,7 +890,7 @@ bool blk_rq_merge_ok(struct request *rq, struct bio *bio)
 
 enum elv_merge blk_try_merge(struct request *rq, struct bio *bio)
 {
-	if (blk_discard_mergable(rq))
+	if (blk_discard_support_multi_range(rq))
 		return ELEVATOR_DISCARD_MERGE;
 	else if (blk_rq_pos(rq) + blk_rq_sectors(rq) == bio->bi_iter.bi_sector)
 		return ELEVATOR_BACK_MERGE;
-- 
2.25.2

Re: [PATCH V3 3/3] block: rename blk_discard_mergable as blk_discard_support_multi_range

[Christoph Hellwig]

On Mon, Aug 17, 2020 at 05:52:41PM +0800, Ming Lei wrote:
> Name of blk_discard_mergable() is very confusing, and this function
> actually means if the queue supports multi_range discard. Also there
> are two kinds of discard merge:
> 
> 1) multi range discard, bios in one request won't have to be contiguous,
> and actually each bio is thought as one range
> 
> 2) single range discard, all bios in one request have to be contiguous
> just like normal RW request's merge
> 
> Rename blk_discard_mergable() for not confusing people, and avoiding
> to introduce bugs in future.

I agree the current name is not good, but I find the new one pretty
clumsy too.  Not sure the rename is worth it, but also no strict NAK
from me.

Re: [PATCH V3 0/3] block: fix discard merge for single max discard segment

[Jens Axboe]

On 8/17/20 2:52 AM, Ming Lei wrote:
> Hello,
> 
> The 1st patch checks max discard segment limit in request merge code,
> and one discard request failure is fixed for virtio_blk.
> 
> The 2nd patch fixes handling of single max discard segment for virtio_blk,
> and potential memory corruption is fixed.
> 
> The 3rd patch renames the confusing blk_discard_mergable().
> 
> The biggest problem is that virtio_blk handles discard request in
> multi-range style, however it(at least qemu) may claim that multi range
> discard isn't supported by returning 1 for max discard segment.

I have applied this, but dropped patch #3. We can do the rename for 5.10
if need be.

-- 
Jens Axboe

Re: [PATCH V3 2/3] block: virtio_blk: fix handling single range discard request

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

On Mon, Aug 17, 2020 at 05:52:40PM +0800, Ming Lei wrote:
> 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support") starts
> to support multi-range discard for virtio-blk. However, the virtio-blk
> disk may report max discard segment as 1, at least that is exactly what
> qemu is doing.
> 
> So far, block layer switches to normal request merge if max discard segment
> limit is 1, and multiple bios can be merged to single segment. This way may
> cause memory corruption in virtblk_setup_discard_write_zeroes().
> 
> Fix the issue by handling single max discard segment in straightforward
> way.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> Fixes: 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support")
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Cc: Changpeng Liu <changpeng.liu@intel.com>
> Cc: Daniel Verkamp <dverkamp@chromium.org>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  drivers/block/virtio_blk.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)

Thanks for fixing this!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]