[PATCH 0/2] block: discard merge fix & improvement

Hello Guys,

The 1st patch fixes request merge for single-range discard request.

The 2nd one improves request merge for multi-range discard request,
so that any continuous bios can be merged to one range.


Ming Lei (2):
  block: fix discard request merge
  block: support bio merge for multi-range discard

 block/blk-merge.c          | 20 +++++++++------
 drivers/block/virtio_blk.c |  9 ++++---
 drivers/nvme/host/core.c   |  8 +++---
 include/linux/blkdev.h     | 51 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 72 insertions(+), 16 deletions(-)

Cc: Wang Shanker <redacted>
-- 
2.31.1

ll_new_hw_segment() is reached only in case of single range discard
merge, and we don't have max discard segment size limit actually, so
it is wrong to run the following check:

if (req->nr_phys_segments + nr_phys_segs > blk_rq_get_max_segments(req))

it may be always false since req->nr_phys_segments is initialized as
one, and bio's segment count is still 1, blk_rq_get_max_segments(reg)
is 1 too.

Fix the issue by not doing the check and bypassing the calculation of
discard request's nr_phys_segments.

Based on analysis from Wang Shanker.

Reported-by: Wang Shanker <redacted>
Signed-off-by: Ming Lei <redacted>
---
 block/blk-merge.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 4d97fb6dd226..bcdff1879c34 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c

@@ -559,10 +559,14 @@ static inline unsigned int blk_rq_get_max_segments(struct request *rq)
 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 > blk_rq_get_max_segments(req))
+	if (blk_integrity_merge_bio(req->q, req, bio) == false)
 		goto no_merge;
 
-	if (blk_integrity_merge_bio(req->q, req, bio) == false)
+	/* discard request merge won't add new segment */
+	if (req_op(req) == REQ_OP_DISCARD)
+		return 1;
+
+	if (req->nr_phys_segments + nr_phys_segs > blk_rq_get_max_segments(req))
 		goto no_merge;
 
 	/*

-- 
2.31.1

On Wed, Jun 09, 2021 at 08:45:55AM +0800, Ming Lei wrote:

I'd rather handle this by returning UINT_MAX for discard requests from
blk_rq_get_max_segments given that bio_attempt_discard_merge is only used
for the !DISCARD_MERGE case anyway.

On Tue, Jun 22, 2021 at 08:53:09AM +0200, Christoph Hellwig wrote:

On Wed, Jun 09, 2021 at 08:45:55AM +0800, Ming Lei wrote:

quoted

index 4d97fb6dd226..bcdff1879c34 100644

--- a/block/blk-merge.c
+++ b/block/blk-merge.c

@@ -559,10 +559,14 @@ static inline unsigned int blk_rq_get_max_segments(struct request *rq)
 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 > blk_rq_get_max_segments(req))
+	if (blk_integrity_merge_bio(req->q, req, bio) == false)
 		goto no_merge;
 
-	if (blk_integrity_merge_bio(req->q, req, bio) == false)
+	/* discard request merge won't add new segment */
+	if (req_op(req) == REQ_OP_DISCARD)
+		return 1;
+
+	if (req->nr_phys_segments + nr_phys_segs > blk_rq_get_max_segments(req))

I'd rather handle this by returning UINT_MAX for discard requests from
blk_rq_get_max_segments given that bio_attempt_discard_merge is only used
for the !DISCARD_MERGE case anyway.

This way makes very big req->nr_phys_segments or overflows it, and may break
drivers.

At least it breaks blk_rq_nr_discard_segments(), which looks easy to fix. But we
need to audit every driver to use req->nr_phys_segments directly in case of
discard request.



Thanks,
Ming

So far multi-range discard treats each bio as one segment(range) of single
discard request. This way becomes not efficient if lots of small sized
discard bios are submitted, and one example is raid456.

Support bio merge for multi-range discard for improving lots of small
sized discard bios.

Turns out it is easy to support it:

1) always try to merge bio first

2) run into multi-range discard only if bio merge can't be done

3) add rq_for_each_discard_range() for retrieving each range(segment)
of discard request

Reported-by: Wang Shanker <redacted>
Signed-off-by: Ming Lei <redacted>
---
 block/blk-merge.c          | 12 ++++-----
 drivers/block/virtio_blk.c |  9 ++++---
 drivers/nvme/host/core.c   |  8 +++---
 include/linux/blkdev.h     | 51 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 66 insertions(+), 14 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index bcdff1879c34..65210e9a8efa 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c

@@ -724,10 +724,10 @@ 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))
-		return ELEVATOR_DISCARD_MERGE;
-	else if (blk_rq_pos(req) + blk_rq_sectors(req) == blk_rq_pos(next))
+	if (blk_rq_pos(req) + blk_rq_sectors(req) == blk_rq_pos(next))
 		return ELEVATOR_BACK_MERGE;
+	else if (blk_discard_mergable(req))
+		return ELEVATOR_DISCARD_MERGE;
 
 	return ELEVATOR_NO_MERGE;
 }

@@ -908,12 +908,12 @@ 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))
-		return ELEVATOR_DISCARD_MERGE;
-	else if (blk_rq_pos(rq) + blk_rq_sectors(rq) == bio->bi_iter.bi_sector)
+	if (blk_rq_pos(rq) + blk_rq_sectors(rq) == bio->bi_iter.bi_sector)
 		return ELEVATOR_BACK_MERGE;
 	else if (blk_rq_pos(rq) - bio_sectors(bio) == bio->bi_iter.bi_sector)
 		return ELEVATOR_FRONT_MERGE;
+	else if (blk_discard_mergable(rq))
+		return ELEVATOR_DISCARD_MERGE;
 	return ELEVATOR_NO_MERGE;
 }
 

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index b9fa3ef5b57c..970cb0d8acaa 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c

@@ -116,7 +116,6 @@ static int virtblk_setup_discard_write_zeroes(struct request *req, bool unmap)
 	unsigned short segments = blk_rq_nr_discard_segments(req);
 	unsigned short n = 0;
 	struct virtio_blk_discard_write_zeroes *range;
-	struct bio *bio;
 	u32 flags = 0;
 
 	if (unmap)

@@ -138,9 +137,11 @@ static int virtblk_setup_discard_write_zeroes(struct request *req, bool unmap)
 		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;
+		struct req_discard_range r;
+
+		rq_for_each_discard_range(r, req) {
+			u64 sector = r.sector;
+			u32 num_sectors = r.size >> SECTOR_SHIFT;
 
 			range[n].flags = cpu_to_le32(flags);
 			range[n].num_sectors = cpu_to_le32(num_sectors);

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 24bcae88587a..4b0a39360ce9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c

@@ -813,7 +813,7 @@ static blk_status_t nvme_setup_discard(struct nvme_ns *ns, struct request *req,
 {
 	unsigned short segments = blk_rq_nr_discard_segments(req), n = 0;
 	struct nvme_dsm_range *range;
-	struct bio *bio;
+	struct req_discard_range r;
 
 	/*
 	 * Some devices do not consider the DSM 'Number of Ranges' field when

@@ -835,9 +835,9 @@ static blk_status_t nvme_setup_discard(struct nvme_ns *ns, struct request *req,
 		range = page_address(ns->ctrl->discard_page);
 	}
 
-	__rq_for_each_bio(bio, req) {
-		u64 slba = nvme_sect_to_lba(ns, bio->bi_iter.bi_sector);
-		u32 nlb = bio->bi_iter.bi_size >> ns->lba_shift;
+	rq_for_each_discard_range(r, req) {
+		u64 slba = nvme_sect_to_lba(ns, r.sector);
+		u32 nlb = r.size >> ns->lba_shift;
 
 		if (n < segments) {
 			range[n].cattr = cpu_to_le32(0);

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index d66d0da72529..bd9d22269a7b 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h

@@ -1007,6 +1007,57 @@ static inline unsigned int blk_rq_stats_sectors(const struct request *rq)
 	return rq->stats_sectors;
 }
 
+struct req_discard_range {
+	sector_t	sector;
+	unsigned int	size;
+
+	/*
+	 * internal field: driver don't use it, and it always points to
+	 * next bio to be processed
+	 */
+	struct bio *__bio;
+};
+
+static inline void req_init_discard_range_iter(const struct request *rq,
+		struct req_discard_range *range)
+{
+	range->__bio = rq->bio;
+}
+
+/* return true if @range stores one valid discard range */
+static inline bool req_get_discard_range(struct req_discard_range *range)
+{
+	struct bio *bio;
+
+	if (!range->__bio)
+		return false;
+
+	bio = range->__bio;
+	range->sector = bio->bi_iter.bi_sector;
+	range->size = bio->bi_iter.bi_size;
+	range->__bio = bio->bi_next;
+
+	while (range->__bio) {
+		struct bio *bio = range->__bio;
+
+		if (range->sector + (range->size >> SECTOR_SHIFT) !=
+				bio->bi_iter.bi_sector)
+			break;
+
+		/*
+		 * ->size won't overflow because req->__data_len is defined
+		 *  as 'unsigned int'
+		 */
+		range->size += bio->bi_iter.bi_size;
+		range->__bio = bio->bi_next;
+	}
+	return true;
+}
+
+#define rq_for_each_discard_range(range, rq) \
+	for (req_init_discard_range_iter((rq), &range); \
+			req_get_discard_range(&range);)
+
 #ifdef CONFIG_BLK_DEV_ZONED
 
 /* Helper to convert BLK_ZONE_ZONE_XXX to its string format XXX */

-- 
2.31.1

Shall we adjust how req->nr_phys_segments is calculated in 
bio_attempt_discard_merge() so that multiple contiguous bio's can
be seen as one segment?

On Wed, Jun 09, 2021 at 11:05:59AM +0800, Wang Shanker wrote:

quoted

2021年06月09日 08:45，Ming Lei [off-list ref] 写道：

So far multi-range discard treats each bio as one segment(range) of single
discard request. This way becomes not efficient if lots of small sized
discard bios are submitted, and one example is raid456.

Support bio merge for multi-range discard for improving lots of small
sized discard bios.

Turns out it is easy to support it:

1) always try to merge bio first

2) run into multi-range discard only if bio merge can't be done

3) add rq_for_each_discard_range() for retrieving each range(segment)
of discard request

Reported-by: Wang Shanker <redacted>
Signed-off-by: Ming Lei <redacted>
---
block/blk-merge.c          | 12 ++++-----
drivers/block/virtio_blk.c |  9 ++++---
drivers/nvme/host/core.c   |  8 +++---
include/linux/blkdev.h     | 51 ++++++++++++++++++++++++++++++++++++++
4 files changed, 66 insertions(+), 14 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index bcdff1879c34..65210e9a8efa 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c

@@ -724,10 +724,10 @@ 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))
-		return ELEVATOR_DISCARD_MERGE;
-	else if (blk_rq_pos(req) + blk_rq_sectors(req) == blk_rq_pos(next))
+	if (blk_rq_pos(req) + blk_rq_sectors(req) == blk_rq_pos(next))
		return ELEVATOR_BACK_MERGE;
+	else if (blk_discard_mergable(req))

Shall we adjust how req->nr_phys_segments is calculated in 
bio_attempt_discard_merge() so that multiple contiguous bio's can
be seen as one segment?

I think it isn't necessary, because we try to merge discard IOs first
just like plain IO. So when bio_attempt_discard_merge() is reached, it
means that IOs can't be merged, so req->nr_phys_segments should be
increased by 1.


Thanks,
Ming

2021年06月09日 14:12，Ming Lei [off-list ref] 写道：

On Wed, Jun 09, 2021 at 11:05:59AM +0800, Wang Shanker wrote:

quoted

quoted

2021年06月09日 08:45，Ming Lei [off-list ref] 写道：

So far multi-range discard treats each bio as one segment(range) of single
discard request. This way becomes not efficient if lots of small sized
discard bios are submitted, and one example is raid456.

Support bio merge for multi-range discard for improving lots of small
sized discard bios.

Turns out it is easy to support it:

1) always try to merge bio first

2) run into multi-range discard only if bio merge can't be done

3) add rq_for_each_discard_range() for retrieving each range(segment)
of discard request

Reported-by: Wang Shanker <redacted>
Signed-off-by: Ming Lei <redacted>
---
block/blk-merge.c          | 12 ++++-----
drivers/block/virtio_blk.c |  9 ++++---
drivers/nvme/host/core.c   |  8 +++---
include/linux/blkdev.h     | 51 ++++++++++++++++++++++++++++++++++++++
4 files changed, 66 insertions(+), 14 deletions(-)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index bcdff1879c34..65210e9a8efa 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c

@@ -724,10 +724,10 @@ 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))
-		return ELEVATOR_DISCARD_MERGE;
-	else if (blk_rq_pos(req) + blk_rq_sectors(req) == blk_rq_pos(next))
+	if (blk_rq_pos(req) + blk_rq_sectors(req) == blk_rq_pos(next))
		return ELEVATOR_BACK_MERGE;
+	else if (blk_discard_mergable(req))

Shall we adjust how req->nr_phys_segments is calculated in 
bio_attempt_discard_merge() so that multiple contiguous bio's can
be seen as one segment?

I think it isn't necessary, because we try to merge discard IOs first
just like plain IO. So when bio_attempt_discard_merge() is reached, it
means that IOs can't be merged, so req->nr_phys_segments should be
increased by 1.

You are right. And by applying the series, I can confirm contiguous bio's
are getting merged into ~20M requests. It's much better than before. I 
guess it might be the scheduler that prevents further merging. So it 
seems that the root solution for raid456 is required since bio merging cannot
solve all the problem.

Thanks again for your work.

Cheers,

Miao Wang

On Wed, Jun 09, 2021 at 08:45:56AM +0800, Ming Lei wrote:

Oh well, this breaks my previous suggestions.

+		struct req_discard_range r;
+
+		rq_for_each_discard_range(r, req) {

... and I can't say I like this rather complex iterator.

What is the problem of just fixing the raid code to not submit stupidly
small discard requests instead?

2021年06月22日 14:55，Christoph Hellwig [off-list ref] 写道：

On Wed, Jun 09, 2021 at 08:45:56AM +0800, Ming Lei wrote:

quoted

diff --git a/block/blk-merge.c b/block/blk-merge.c
index bcdff1879c34..65210e9a8efa 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c

@@ -724,10 +724,10 @@ 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))
-		return ELEVATOR_DISCARD_MERGE;
-	else if (blk_rq_pos(req) + blk_rq_sectors(req) == blk_rq_pos(next))
+	if (blk_rq_pos(req) + blk_rq_sectors(req) == blk_rq_pos(next))
		return ELEVATOR_BACK_MERGE;

Oh well, this breaks my previous suggestions.

quoted

+		struct req_discard_range r;
+
+		rq_for_each_discard_range(r, req) {

... and I can't say I like this rather complex iterator.

What is the problem of just fixing the raid code to not submit stupidly
small discard requests instead?

Yes, raid code should indeed be fixed. However, the way discard
requests are getting merged is not correct. 

I understand your concern about complex iterator. I believe it is because
the difference between discard requests and normal read/write requests. 
Discard requests are without data and allowed to be non-contiguous, while
normal requests, which blk-merge was originally dealing with, are with 
data buffers and should be contiguous. Actually, correct me if wrong, the 
current request struct lacks the ability to express non-contiguous bio's.
Ming's patch, by introducing the rq_for_each_discard_range iterator, 
partially enables the request struct to express non-contiguous bio's.

Cheers, 

Miao Wang

On Wed, Jun 09, 2021 at 08:45:54AM +0800, Ming Lei wrote:

Hello Guys,

The 1st patch fixes request merge for single-range discard request.

The 2nd one improves request merge for multi-range discard request,
so that any continuous bios can be merged to one range.


Ming Lei (2):
  block: fix discard request merge
  block: support bio merge for multi-range discard

 block/blk-merge.c          | 20 +++++++++------
 drivers/block/virtio_blk.c |  9 ++++---
 drivers/nvme/host/core.c   |  8 +++---
 include/linux/blkdev.h     | 51 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 72 insertions(+), 16 deletions(-)

Cc: Wang Shanker <redacted>

Hello Guys,

Ping...


Thanks,
Ming

On Tue, Jun 15, 2021 at 06:00:11AM +0800, Ming Lei wrote:

On Wed, Jun 09, 2021 at 08:45:54AM +0800, Ming Lei wrote:

quoted

Hello Guys,

The 1st patch fixes request merge for single-range discard request.

The 2nd one improves request merge for multi-range discard request,
so that any continuous bios can be merged to one range.


Ming Lei (2):
  block: fix discard request merge
  block: support bio merge for multi-range discard

 block/blk-merge.c          | 20 +++++++++------
 drivers/block/virtio_blk.c |  9 ++++---
 drivers/nvme/host/core.c   |  8 +++---
 include/linux/blkdev.h     | 51 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 72 insertions(+), 16 deletions(-)

Cc: Wang Shanker <redacted>

Hello Guys,

Ping...

Hello,

Any comments on the two patches?


Thanks,
Ming