linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V2 0/3] block: fix discard merge for single max discard segment
@ 2020-08-11 23:44 Ming Lei
  2020-08-11 23:44 ` [PATCH V2 1/3] block: respect queue limit of " Ming Lei
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Ming Lei @ 2020-08-11 23:44 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, Ming Lei, Christoph Hellwig, Changpeng Liu,
	Daniel Verkamp, Michael S . Tsirkin, Stefan Hajnoczi,
	Stefano Garzarella

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.

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          | 21 +++++++++++++++------
 drivers/block/virtio_blk.c | 31 +++++++++++++++++++++++--------
 2 files changed, 38 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH V2 1/3] block: respect queue limit of max discard segment
  2020-08-11 23:44 [PATCH V2 0/3] block: fix discard merge for single max discard segment Ming Lei
@ 2020-08-11 23:44 ` Ming Lei
  2020-08-13 15:41   ` Christoph Hellwig
  2020-08-11 23:44 ` [PATCH V2 2/3] block: virtio_blk: fix handling single range discard request Ming Lei
  2020-08-11 23:44 ` [PATCH V2 3/3] block: rename blk_discard_mergable as blk_discard_support_multi_range Ming Lei
  2 siblings, 1 reply; 9+ messages in thread
From: Ming Lei @ 2020-08-11 23:44 UTC (permalink / raw)
  To: Jens Axboe; +Cc: linux-block, Ming Lei, Christoph Hellwig, Stefano Garzarella

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")
Cc: Christoph Hellwig <hch@lst.de>
Cc: Stefano Garzarella <sgarzare@redhat.com>
---
 block/blk-merge.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 5196dc145270..d18fb88ca8bd 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -533,10 +533,16 @@ 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)
+{
+	return req_op(rq) == REQ_OP_DISCARD ?
+		queue_max_discard_segments(rq->q) : 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 +630,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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH V2 2/3] block: virtio_blk: fix handling single range discard request
  2020-08-11 23:44 [PATCH V2 0/3] block: fix discard merge for single max discard segment Ming Lei
  2020-08-11 23:44 ` [PATCH V2 1/3] block: respect queue limit of " Ming Lei
@ 2020-08-11 23:44 ` Ming Lei
  2020-08-12  2:07   ` Baolin Wang
  2020-08-13 15:43   ` Christoph Hellwig
  2020-08-11 23:44 ` [PATCH V2 3/3] block: rename blk_discard_mergable as blk_discard_support_multi_range Ming Lei
  2 siblings, 2 replies; 9+ messages in thread
From: Ming Lei @ 2020-08-11 23:44 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, Ming Lei, Christoph Hellwig, Changpeng Liu,
	Daniel Verkamp, Michael S . Tsirkin, Stefan Hajnoczi,
	Stefano Garzarella

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")
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>
---
 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH V2 3/3] block: rename blk_discard_mergable as blk_discard_support_multi_range
  2020-08-11 23:44 [PATCH V2 0/3] block: fix discard merge for single max discard segment Ming Lei
  2020-08-11 23:44 ` [PATCH V2 1/3] block: respect queue limit of " Ming Lei
  2020-08-11 23:44 ` [PATCH V2 2/3] block: virtio_blk: fix handling single range discard request Ming Lei
@ 2020-08-11 23:44 ` Ming Lei
  2 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2020-08-11 23:44 UTC (permalink / raw)
  To: Jens Axboe; +Cc: linux-block, Ming Lei, Christoph Hellwig

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 d18fb88ca8bd..23eb46a99c9d 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -692,8 +692,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)
@@ -704,7 +707,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;
@@ -790,7 +793,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);
 
 	/*
@@ -886,7 +889,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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH V2 2/3] block: virtio_blk: fix handling single range discard request
  2020-08-11 23:44 ` [PATCH V2 2/3] block: virtio_blk: fix handling single range discard request Ming Lei
@ 2020-08-12  2:07   ` Baolin Wang
  2020-08-12  2:52     ` Ming Lei
  2020-08-13 15:43   ` Christoph Hellwig
  1 sibling, 1 reply; 9+ messages in thread
From: Baolin Wang @ 2020-08-12  2:07 UTC (permalink / raw)
  To: Ming Lei
  Cc: Jens Axboe, linux-block, Christoph Hellwig, Changpeng Liu,
	Daniel Verkamp, Michael S . Tsirkin, Stefan Hajnoczi,
	Stefano Garzarella

Hi Ming,

On Wed, Aug 12, 2020 at 07:44:19AM +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")
> 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>
> ---
>  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);

I wonder should we return an error if the discard segments are
incorrect like NVMe did[1]? In case the DMA may do some serious
damages in this case.

[1]
https://elixir.bootlin.com/linux/v5.8-rc7/source/drivers/nvme/host/core.c#L638

> +
>  	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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH V2 2/3] block: virtio_blk: fix handling single range discard request
  2020-08-12  2:07   ` Baolin Wang
@ 2020-08-12  2:52     ` Ming Lei
  2020-08-12  6:38       ` Baolin Wang
  0 siblings, 1 reply; 9+ messages in thread
From: Ming Lei @ 2020-08-12  2:52 UTC (permalink / raw)
  To: Baolin Wang
  Cc: Jens Axboe, linux-block, Christoph Hellwig, Changpeng Liu,
	Daniel Verkamp, Michael S . Tsirkin, Stefan Hajnoczi,
	Stefano Garzarella

On Wed, Aug 12, 2020 at 10:07:06AM +0800, Baolin Wang wrote:
> Hi Ming,
> 
> On Wed, Aug 12, 2020 at 07:44:19AM +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")
> > 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>
> > ---
> >  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);
> 
> I wonder should we return an error if the discard segments are
> incorrect like NVMe did[1]? In case the DMA may do some serious
> damages in this case.

It is an unlikely case:

1) if queue_max_discard_segments() is 1, the warning can't be triggered

2) otherwise, ELEVATOR_DISCARD_MERGE is always handled in bio_attempt_discard_merge(),
and segment number is really same with number of bios in the request.

If the warning is triggered, it is simply one serious bug in block
layer.

BTW, suppose the warning is triggered:

1) if n < segments, it is simply one warning

2) if n > segments, no matter if something like nvme_setup_discard() is
done, serious memory corruption issue has been caused.

So it doesn't matter to handle it in nvme's style.

Thanks,
Ming


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH V2 2/3] block: virtio_blk: fix handling single range discard request
  2020-08-12  2:52     ` Ming Lei
@ 2020-08-12  6:38       ` Baolin Wang
  0 siblings, 0 replies; 9+ messages in thread
From: Baolin Wang @ 2020-08-12  6:38 UTC (permalink / raw)
  To: Ming Lei
  Cc: Jens Axboe, linux-block, Christoph Hellwig, Changpeng Liu,
	Daniel Verkamp, Michael S . Tsirkin, Stefan Hajnoczi,
	Stefano Garzarella

On Wed, Aug 12, 2020 at 10:52:58AM +0800, Ming Lei wrote:
> On Wed, Aug 12, 2020 at 10:07:06AM +0800, Baolin Wang wrote:
> > Hi Ming,
> > 
> > On Wed, Aug 12, 2020 at 07:44:19AM +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")
> > > 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>
> > > ---
> > >  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);
> > 
> > I wonder should we return an error if the discard segments are
> > incorrect like NVMe did[1]? In case the DMA may do some serious
> > damages in this case.
> 
> It is an unlikely case:
> 
> 1) if queue_max_discard_segments() is 1, the warning can't be triggered
> 
> 2) otherwise, ELEVATOR_DISCARD_MERGE is always handled in bio_attempt_discard_merge(),
> and segment number is really same with number of bios in the request.
> 
> If the warning is triggered, it is simply one serious bug in block
> layer.
> 
> BTW, suppose the warning is triggered:
> 
> 1) if n < segments, it is simply one warning
> 
> 2) if n > segments, no matter if something like nvme_setup_discard() is
> done, serious memory corruption issue has been caused.
> 
> So it doesn't matter to handle it in nvme's style.

OK. Sounds reasonable. Thanks.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH V2 1/3] block: respect queue limit of max discard segment
  2020-08-11 23:44 ` [PATCH V2 1/3] block: respect queue limit of " Ming Lei
@ 2020-08-13 15:41   ` Christoph Hellwig
  0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2020-08-13 15:41 UTC (permalink / raw)
  To: Ming Lei; +Cc: Jens Axboe, linux-block, Christoph Hellwig, Stefano Garzarella

On Wed, Aug 12, 2020 at 07:44:18AM +0800, Ming Lei wrote:
> 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")
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  block/blk-merge.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 5196dc145270..d18fb88ca8bd 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -533,10 +533,16 @@ 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)
> +{
> +	return req_op(rq) == REQ_OP_DISCARD ?
> +		queue_max_discard_segments(rq->q) : queue_max_segments(rq->q);

I'd go for a good old if statement here..

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH V2 2/3] block: virtio_blk: fix handling single range discard request
  2020-08-11 23:44 ` [PATCH V2 2/3] block: virtio_blk: fix handling single range discard request Ming Lei
  2020-08-12  2:07   ` Baolin Wang
@ 2020-08-13 15:43   ` Christoph Hellwig
  1 sibling, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2020-08-13 15:43 UTC (permalink / raw)
  To: Ming Lei
  Cc: Jens Axboe, linux-block, Christoph Hellwig, Changpeng Liu,
	Daniel Verkamp, Michael S . Tsirkin, Stefan Hajnoczi,
	Stefano Garzarella

On Wed, Aug 12, 2020 at 07:44:19AM +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.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-08-13 15:43 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-11 23:44 [PATCH V2 0/3] block: fix discard merge for single max discard segment Ming Lei
2020-08-11 23:44 ` [PATCH V2 1/3] block: respect queue limit of " Ming Lei
2020-08-13 15:41   ` Christoph Hellwig
2020-08-11 23:44 ` [PATCH V2 2/3] block: virtio_blk: fix handling single range discard request Ming Lei
2020-08-12  2:07   ` Baolin Wang
2020-08-12  2:52     ` Ming Lei
2020-08-12  6:38       ` Baolin Wang
2020-08-13 15:43   ` Christoph Hellwig
2020-08-11 23:44 ` [PATCH V2 3/3] block: rename blk_discard_mergable as blk_discard_support_multi_range Ming Lei

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).