[PATCH] block: recalculate segment count for multi-segment discard requests correctly

When a stacked block device inserts a request into another block device
using blk_insert_cloned_request, the request's nr_phys_segments field gets
recalculated by a call to blk_recalc_rq_segments in
blk_cloned_rq_check_limits. But blk_recalc_rq_segments does not know how to
handle multi-segment discards. For disk types which can handle
multi-segment discards like nvme, this results in discard requests which
claim a single segment when it should report several, triggering a warning
in nvme and causing nvme to fail the discard from the invalid state.

 WARNING: CPU: 5 PID: 191 at drivers/nvme/host/core.c:700 nvme_setup_discard+0x170/0x1e0 [nvme_core]
 ...
 nvme_setup_cmd+0x217/0x270 [nvme_core]
 nvme_loop_queue_rq+0x51/0x1b0 [nvme_loop]
 __blk_mq_try_issue_directly+0xe7/0x1b0
 blk_mq_request_issue_directly+0x41/0x70
 ? blk_account_io_start+0x40/0x50
 dm_mq_queue_rq+0x200/0x3e0
 blk_mq_dispatch_rq_list+0x10a/0x7d0
 ? __sbitmap_queue_get+0x25/0x90
 ? elv_rb_del+0x1f/0x30
 ? deadline_remove_request+0x55/0xb0
 ? dd_dispatch_request+0x181/0x210
 __blk_mq_do_dispatch_sched+0x144/0x290
 ? bio_attempt_discard_merge+0x134/0x1f0
 __blk_mq_sched_dispatch_requests+0x129/0x180
 blk_mq_sched_dispatch_requests+0x30/0x60
 __blk_mq_run_hw_queue+0x47/0xe0
 __blk_mq_delay_run_hw_queue+0x15b/0x170
 blk_mq_sched_insert_requests+0x68/0xe0
 blk_mq_flush_plug_list+0xf0/0x170
 blk_finish_plug+0x36/0x50
 xlog_cil_committed+0x19f/0x290 [xfs]
 xlog_cil_process_committed+0x57/0x80 [xfs]
 xlog_state_do_callback+0x1e0/0x2a0 [xfs]
 xlog_ioend_work+0x2f/0x80 [xfs]
 process_one_work+0x1b6/0x350
 worker_thread+0x53/0x3e0
 ? process_one_work+0x350/0x350
 kthread+0x11b/0x140
 ? __kthread_bind_mask+0x60/0x60
 ret_from_fork+0x22/0x30

This patch fixes blk_recalc_rq_segments to be aware of devices which can
have multi-segment discards. It calculates the correct discard segment
count by counting the number of bio as each discard bio is considered its
own segment.

Signed-off-by: David Jeffery <redacted>
Tested-by: Laurence Oberman <redacted>
---
 block/blk-merge.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 808768f6b174..fe7358bd5d09 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c

@@ -382,6 +382,13 @@ unsigned int blk_recalc_rq_segments(struct request *rq)
 
 	switch (bio_op(rq->bio)) {
 	case REQ_OP_DISCARD:
+		if (queue_max_discard_segments(rq->q) > 1) {
+			struct bio *bio = rq->bio;
+			for_each_bio(bio)
+				nr_phys_segs++;
+			return nr_phys_segs;
+		}
+		/* fall through */
 	case REQ_OP_SECURE_ERASE:
 	case REQ_OP_WRITE_ZEROES:
 		return 0;

-- 
2.29.2

On Mon, Feb 01, 2021 at 11:48:50AM -0500, David Jeffery wrote:

REQ_OP_SECURE_ERASE needs to be covered since block layer treats
the two in very similar way from discard viewpoint.

Also single range discard should be fixed too, since block layer
thinks single-range discard req segment is 1. Otherwise, the warning in
virtblk_setup_discard_write_zeroes() still may be triggered, at least.


-- 
Ming

On Tue, Feb 02, 2021 at 11:33:43AM +0800, Ming Lei wrote:

On Mon, Feb 01, 2021 at 11:48:50AM -0500, David Jeffery wrote:

quoted

When a stacked block device inserts a request into another block device
using blk_insert_cloned_request, the request's nr_phys_segments field gets
recalculated by a call to blk_recalc_rq_segments in
blk_cloned_rq_check_limits. But blk_recalc_rq_segments does not know how to
handle multi-segment discards. For disk types which can handle
multi-segment discards like nvme, this results in discard requests which
claim a single segment when it should report several, triggering a warning
in nvme and causing nvme to fail the discard from the invalid state.

 WARNING: CPU: 5 PID: 191 at drivers/nvme/host/core.c:700 nvme_setup_discard+0x170/0x1e0 [nvme_core]
 ...
 nvme_setup_cmd+0x217/0x270 [nvme_core]
 nvme_loop_queue_rq+0x51/0x1b0 [nvme_loop]
 __blk_mq_try_issue_directly+0xe7/0x1b0
 blk_mq_request_issue_directly+0x41/0x70
 ? blk_account_io_start+0x40/0x50
 dm_mq_queue_rq+0x200/0x3e0
 blk_mq_dispatch_rq_list+0x10a/0x7d0
 ? __sbitmap_queue_get+0x25/0x90
 ? elv_rb_del+0x1f/0x30
 ? deadline_remove_request+0x55/0xb0
 ? dd_dispatch_request+0x181/0x210
 __blk_mq_do_dispatch_sched+0x144/0x290
 ? bio_attempt_discard_merge+0x134/0x1f0
 __blk_mq_sched_dispatch_requests+0x129/0x180
 blk_mq_sched_dispatch_requests+0x30/0x60
 __blk_mq_run_hw_queue+0x47/0xe0
 __blk_mq_delay_run_hw_queue+0x15b/0x170
 blk_mq_sched_insert_requests+0x68/0xe0
 blk_mq_flush_plug_list+0xf0/0x170
 blk_finish_plug+0x36/0x50
 xlog_cil_committed+0x19f/0x290 [xfs]
 xlog_cil_process_committed+0x57/0x80 [xfs]
 xlog_state_do_callback+0x1e0/0x2a0 [xfs]
 xlog_ioend_work+0x2f/0x80 [xfs]
 process_one_work+0x1b6/0x350
 worker_thread+0x53/0x3e0
 ? process_one_work+0x350/0x350
 kthread+0x11b/0x140
 ? __kthread_bind_mask+0x60/0x60
 ret_from_fork+0x22/0x30

This patch fixes blk_recalc_rq_segments to be aware of devices which can
have multi-segment discards. It calculates the correct discard segment
count by counting the number of bio as each discard bio is considered its
own segment.

Signed-off-by: David Jeffery <redacted>
Tested-by: Laurence Oberman <redacted>
---
 block/blk-merge.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 808768f6b174..fe7358bd5d09 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c

@@ -382,6 +382,13 @@ unsigned int blk_recalc_rq_segments(struct request *rq)
 
 	switch (bio_op(rq->bio)) {
 	case REQ_OP_DISCARD:
+		if (queue_max_discard_segments(rq->q) > 1) {
+			struct bio *bio = rq->bio;
+			for_each_bio(bio)
+				nr_phys_segs++;
+			return nr_phys_segs;
+		}
+		/* fall through */
 	case REQ_OP_SECURE_ERASE:

REQ_OP_SECURE_ERASE needs to be covered since block layer treats
the two in very similar way from discard viewpoint.

Also single range discard should be fixed too, since block layer
thinks single-range discard req segment is 1. Otherwise, the warning in
virtblk_setup_discard_write_zeroes() still may be triggered, at least.


-- 
Ming

The return 0 does seem to be an old relic that does not make sense anymore.
Moving REQ_OP_SECURE_ERASE to be with discard and removing the old return 0,
is this what you had in mind?

 

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 808768f6b174..68458aa01b05 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c

@@ -383,8 +383,14 @@ unsigned int blk_recalc_rq_segments(struct request *rq)
 	switch (bio_op(rq->bio)) {
 	case REQ_OP_DISCARD:
 	case REQ_OP_SECURE_ERASE:
+		if (queue_max_discard_segments(rq->q) > 1) {
+			struct bio *bio = rq->bio;
+			for_each_bio(bio)
+				nr_phys_segs++;
+			return nr_phys_segs;
+		}
+		/* fall through */
 	case REQ_OP_WRITE_ZEROES:
-		return 0;
 	case REQ_OP_WRITE_SAME:
 		return 1;
 	}

--

David Jeffery

On Tue, Feb 02, 2021 at 03:43:55PM -0500, David Jeffery wrote:

WRITE_SAME uses same buffer, so the nr_segment is still one; WRITE_ZERO
doesn't need extra payload, so nr_segments is zero, see
blk_bio_write_zeroes_split(), blk_bio_write_same_split, attempt_merge()
and blk_rq_merge_ok().

-- 
Ming

On 2/2/21 18:39, Ming Lei wrote:

I don't think returning 1 for write-zeroes is right,
did you test this patch with write-zeores enabled controller with
the right fs that triggers this behavior ?

On 2/2/21 18:39, Ming Lei wrote:

Also, you need to add a new line after declaration of bio in the above
code block.

On Wed, 2021-02-03 at 03:15 +0000, Chaitanya Kulkarni wrote:

On 2/2/21 18:39, Ming Lei wrote:

quoted

+		/* fall through */
 	case REQ_OP_WRITE_ZEROES:
-		return 0;

I don't think returning 1 for write-zeroes is right,
did you test this patch with write-zeores enabled controller with
the right fs that triggers this behavior ?

I tested the first iteration of the patch fully mounting an XFS file
system with -o discard and creating and deleting files.
That was our specific RHEL8 failure we were handling here with David's
first submission.

I can test his most recent, I have not done that yet.
Again, please follow up with exactly what you want based against
David's patch and I can test that.

Regards
Laurence 

On Wed, 2021-02-03 at 08:50 -0500, Laurence Oberman wrote:

On Wed, 2021-02-03 at 03:15 +0000, Chaitanya Kulkarni wrote:

quoted

On 2/2/21 18:39, Ming Lei wrote:

quoted

+		/* fall through */
 	case REQ_OP_WRITE_ZEROES:
-		return 0;

I don't think returning 1 for write-zeroes is right,
did you test this patch with write-zeores enabled controller with
the right fs that triggers this behavior ?

I tested the first iteration of the patch fully mounting an XFS file
system with -o discard and creating and deleting files.
That was our specific RHEL8 failure we were handling here with
David's
first submission.

I can test his most recent, I have not done that yet.
Again, please follow up with exactly what you want based against
David's patch and I can test that.

Regards
Laurence 

So if I understand what it is you were wanting I will test this now.

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 808768f6b174..a9bd958c07c4 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c

@@ -383,6 +383,13 @@ unsigned int blk_recalc_rq_segments(struct request

*rq)
        switch (bio_op(rq->bio)) {
        case REQ_OP_DISCARD:
        case REQ_OP_SECURE_ERASE:
+                if (queue_max_discard_segments(rq->q) > 1) {
+                        struct bio *bio = rq->bio;

+                        for_each_bio(bio)
+                                nr_phys_segs++;
+                        return nr_phys_segs;
+                }
+                /* fall through */
        case REQ_OP_WRITE_ZEROES:
                return 0;
        case REQ_OP_WRITE_SAME:

On Wed, Feb 03, 2021 at 10:35:17AM +0800, Ming Lei wrote:

On Tue, Feb 02, 2021 at 03:43:55PM -0500, David Jeffery wrote:

quoted

The return 0 does seem to be an old relic that does not make sense anymore.
Moving REQ_OP_SECURE_ERASE to be with discard and removing the old return 0,
is this what you had in mind?

 

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 808768f6b174..68458aa01b05 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c

@@ -383,8 +383,14 @@ unsigned int blk_recalc_rq_segments(struct request *rq)
 	switch (bio_op(rq->bio)) {
 	case REQ_OP_DISCARD:
 	case REQ_OP_SECURE_ERASE:
+		if (queue_max_discard_segments(rq->q) > 1) {
+			struct bio *bio = rq->bio;
+			for_each_bio(bio)
+				nr_phys_segs++;
+			return nr_phys_segs;
+		}
+		/* fall through */
 	case REQ_OP_WRITE_ZEROES:
-		return 0;
 	case REQ_OP_WRITE_SAME:
 		return 1;

WRITE_SAME uses same buffer, so the nr_segment is still one; WRITE_ZERO
doesn't need extra payload, so nr_segments is zero, see
blk_bio_write_zeroes_split(), blk_bio_write_same_split, attempt_merge()
and blk_rq_merge_ok().

I thought you mentioned virtio-blk because of how some drivers handle
zeroing and discarding similarly and wanted to align the segment count with
discard behavior for WRITE_ZEROES too. (Though that would also need an update
to blk_bio_write_zeroes_split as you pointed out.)  So you want me to leave
WRITE_ZEROES behavior alone and let blk_rq_nr_discard_segments() keep doing
the hiding of a 0 rq->nr_phys_segments as 1 segment in the WRITE_ZEROES treated
as a discard case?

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 808768f6b174..756473295f19 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c

@@ -383,6 +383,14 @@ unsigned int blk_recalc_rq_segments(struct request *rq)
 	switch (bio_op(rq->bio)) {
 	case REQ_OP_DISCARD:
 	case REQ_OP_SECURE_ERASE:
+		if (queue_max_discard_segments(rq->q) > 1) {
+			struct bio *bio = rq->bio;
+
+			for_each_bio(bio)
+				nr_phys_segs++;
+			return nr_phys_segs;
+		}
+		return 1;
 	case REQ_OP_WRITE_ZEROES:
 		return 0;
 	case REQ_OP_WRITE_SAME:

--

David Jeffery

On Wed, Feb 03, 2021 at 11:23:37AM -0500, David Jeffery wrote:

On Wed, Feb 03, 2021 at 10:35:17AM +0800, Ming Lei wrote:

quoted

On Tue, Feb 02, 2021 at 03:43:55PM -0500, David Jeffery wrote:

quoted

The return 0 does seem to be an old relic that does not make sense anymore.
Moving REQ_OP_SECURE_ERASE to be with discard and removing the old return 0,
is this what you had in mind?

 

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 808768f6b174..68458aa01b05 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c

@@ -383,8 +383,14 @@ unsigned int blk_recalc_rq_segments(struct request *rq)
 	switch (bio_op(rq->bio)) {
 	case REQ_OP_DISCARD:
 	case REQ_OP_SECURE_ERASE:
+		if (queue_max_discard_segments(rq->q) > 1) {
+			struct bio *bio = rq->bio;
+			for_each_bio(bio)
+				nr_phys_segs++;
+			return nr_phys_segs;
+		}
+		/* fall through */
 	case REQ_OP_WRITE_ZEROES:
-		return 0;
 	case REQ_OP_WRITE_SAME:
 		return 1;

WRITE_SAME uses same buffer, so the nr_segment is still one; WRITE_ZERO
doesn't need extra payload, so nr_segments is zero, see
blk_bio_write_zeroes_split(), blk_bio_write_same_split, attempt_merge()
and blk_rq_merge_ok().

I thought you mentioned virtio-blk because of how some drivers handle
zeroing and discarding similarly and wanted to align the segment count with
discard behavior for WRITE_ZEROES too. (Though that would also need an update

virtio-blk is just one example which supports both single discard range
and multiple discard range, meantime virtblk_setup_discard_write_zeroes()
simply maps write zero into discard directly.

Just found blk_rq_nr_discard_segments() returns >=1 segments always, so
looks your patch is enough for avoiding the warning.

This patch returns 1 for single-range discard explicitly. However, it
isn't necessary because of blk_rq_nr_discard_segments().

Maybe we can align to blk_bio_discard_split() in future, but that can be
done as cleanup.

Thanks,
Ming

On Mon, Feb 01, 2021 at 11:48:50AM -0500, David Jeffery wrote:

blk_rq_nr_discard_segments() always returns >=1 segments, so no similar
issue in case of single range discard.

Reviewed-by: Ming Lei <redacted>

And it can be thought as:

Fixes: 1e739730c5b9 ("block: optionally merge discontiguous discard bios into a single request")


-- 
Ming

On Thu, 2021-02-04 at 10:27 +0800, Ming Lei wrote:

On Mon, Feb 01, 2021 at 11:48:50AM -0500, David Jeffery wrote:

quoted

When a stacked block device inserts a request into another block
device
using blk_insert_cloned_request, the request's nr_phys_segments
field gets
recalculated by a call to blk_recalc_rq_segments in
blk_cloned_rq_check_limits. But blk_recalc_rq_segments does not
know how to
handle multi-segment discards. For disk types which can handle
multi-segment discards like nvme, this results in discard requests
which
claim a single segment when it should report several, triggering a
warning
in nvme and causing nvme to fail the discard from the invalid
state.

 WARNING: CPU: 5 PID: 191 at drivers/nvme/host/core.c:700
nvme_setup_discard+0x170/0x1e0 [nvme_core]
 ...
 nvme_setup_cmd+0x217/0x270 [nvme_core]
 nvme_loop_queue_rq+0x51/0x1b0 [nvme_loop]
 __blk_mq_try_issue_directly+0xe7/0x1b0
 blk_mq_request_issue_directly+0x41/0x70
 ? blk_account_io_start+0x40/0x50
 dm_mq_queue_rq+0x200/0x3e0
 blk_mq_dispatch_rq_list+0x10a/0x7d0
 ? __sbitmap_queue_get+0x25/0x90
 ? elv_rb_del+0x1f/0x30
 ? deadline_remove_request+0x55/0xb0
 ? dd_dispatch_request+0x181/0x210
 __blk_mq_do_dispatch_sched+0x144/0x290
 ? bio_attempt_discard_merge+0x134/0x1f0
 __blk_mq_sched_dispatch_requests+0x129/0x180
 blk_mq_sched_dispatch_requests+0x30/0x60
 __blk_mq_run_hw_queue+0x47/0xe0
 __blk_mq_delay_run_hw_queue+0x15b/0x170
 blk_mq_sched_insert_requests+0x68/0xe0
 blk_mq_flush_plug_list+0xf0/0x170
 blk_finish_plug+0x36/0x50
 xlog_cil_committed+0x19f/0x290 [xfs]
 xlog_cil_process_committed+0x57/0x80 [xfs]
 xlog_state_do_callback+0x1e0/0x2a0 [xfs]
 xlog_ioend_work+0x2f/0x80 [xfs]
 process_one_work+0x1b6/0x350
 worker_thread+0x53/0x3e0
 ? process_one_work+0x350/0x350
 kthread+0x11b/0x140
 ? __kthread_bind_mask+0x60/0x60
 ret_from_fork+0x22/0x30

This patch fixes blk_recalc_rq_segments to be aware of devices
which can
have multi-segment discards. It calculates the correct discard
segment
count by counting the number of bio as each discard bio is
considered its
own segment.

Signed-off-by: David Jeffery <redacted>
Tested-by: Laurence Oberman <redacted>
---
 block/blk-merge.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 808768f6b174..fe7358bd5d09 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c

@@ -382,6 +382,13 @@ unsigned int blk_recalc_rq_segments(struct

request *rq)
 
 	switch (bio_op(rq->bio)) {
 	case REQ_OP_DISCARD:
+		if (queue_max_discard_segments(rq->q) > 1) {
+			struct bio *bio = rq->bio;
+			for_each_bio(bio)
+				nr_phys_segs++;
+			return nr_phys_segs;
+		}
+		/* fall through */
 	case REQ_OP_SECURE_ERASE:
 	case REQ_OP_WRITE_ZEROES:
 		return 0;

blk_rq_nr_discard_segments() always returns >=1 segments, so no
similar
issue in case of single range discard.

Reviewed-by: Ming Lei <redacted>

And it can be thought as:

Fixes: 1e739730c5b9 ("block: optionally merge discontiguous discard
bios into a single request")

Great, can we get enough acks and push this through its urgent for me
Reviewed-by: Laurence Oberman <redacted>

On Thu, 2021-02-04 at 11:43 -0500, Laurence Oberman wrote:

On Thu, 2021-02-04 at 10:27 +0800, Ming Lei wrote:

quoted

On Mon, Feb 01, 2021 at 11:48:50AM -0500, David Jeffery wrote:

quoted

When a stacked block device inserts a request into another block
device
using blk_insert_cloned_request, the request's nr_phys_segments
field gets
recalculated by a call to blk_recalc_rq_segments in
blk_cloned_rq_check_limits. But blk_recalc_rq_segments does not
know how to
handle multi-segment discards. For disk types which can handle
multi-segment discards like nvme, this results in discard
requests
which
claim a single segment when it should report several, triggering
a
warning
in nvme and causing nvme to fail the discard from the invalid
state.

 WARNING: CPU: 5 PID: 191 at drivers/nvme/host/core.c:700
nvme_setup_discard+0x170/0x1e0 [nvme_core]
 ...
 nvme_setup_cmd+0x217/0x270 [nvme_core]
 nvme_loop_queue_rq+0x51/0x1b0 [nvme_loop]
 __blk_mq_try_issue_directly+0xe7/0x1b0
 blk_mq_request_issue_directly+0x41/0x70
 ? blk_account_io_start+0x40/0x50
 dm_mq_queue_rq+0x200/0x3e0
 blk_mq_dispatch_rq_list+0x10a/0x7d0
 ? __sbitmap_queue_get+0x25/0x90
 ? elv_rb_del+0x1f/0x30
 ? deadline_remove_request+0x55/0xb0
 ? dd_dispatch_request+0x181/0x210
 __blk_mq_do_dispatch_sched+0x144/0x290
 ? bio_attempt_discard_merge+0x134/0x1f0
 __blk_mq_sched_dispatch_requests+0x129/0x180
 blk_mq_sched_dispatch_requests+0x30/0x60
 __blk_mq_run_hw_queue+0x47/0xe0
 __blk_mq_delay_run_hw_queue+0x15b/0x170
 blk_mq_sched_insert_requests+0x68/0xe0
 blk_mq_flush_plug_list+0xf0/0x170
 blk_finish_plug+0x36/0x50
 xlog_cil_committed+0x19f/0x290 [xfs]
 xlog_cil_process_committed+0x57/0x80 [xfs]
 xlog_state_do_callback+0x1e0/0x2a0 [xfs]
 xlog_ioend_work+0x2f/0x80 [xfs]
 process_one_work+0x1b6/0x350
 worker_thread+0x53/0x3e0
 ? process_one_work+0x350/0x350
 kthread+0x11b/0x140
 ? __kthread_bind_mask+0x60/0x60
 ret_from_fork+0x22/0x30

This patch fixes blk_recalc_rq_segments to be aware of devices
which can
have multi-segment discards. It calculates the correct discard
segment
count by counting the number of bio as each discard bio is
considered its
own segment.

Signed-off-by: David Jeffery <redacted>
Tested-by: Laurence Oberman <redacted>
---
 block/blk-merge.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 808768f6b174..fe7358bd5d09 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c

@@ -382,6 +382,13 @@ unsigned int blk_recalc_rq_segments(struct

request *rq)
 
 	switch (bio_op(rq->bio)) {
 	case REQ_OP_DISCARD:
+		if (queue_max_discard_segments(rq->q) > 1) {
+			struct bio *bio = rq->bio;
+			for_each_bio(bio)
+				nr_phys_segs++;
+			return nr_phys_segs;
+		}
+		/* fall through */
 	case REQ_OP_SECURE_ERASE:
 	case REQ_OP_WRITE_ZEROES:
 		return 0;

blk_rq_nr_discard_segments() always returns >=1 segments, so no
similar
issue in case of single range discard.

Reviewed-by: Ming Lei <redacted>

And it can be thought as:

Fixes: 1e739730c5b9 ("block: optionally merge discontiguous discard
bios into a single request")

Great, can we get enough acks and push this through its urgent for me
Reviewed-by: Laurence Oberman <redacted>

Hate to ping again, but we cant take this into RHEL unless its
upstream, can we get enough acks to get this in.

Many Thanks
Laurence 

Hi Jens, when you get a moment, could you take a quick look at this one for ack?

On Thu, Feb 4, 2021 at 11:49 AM Laurence Oberman [off-list ref] wrote:

On Thu, 2021-02-04 at 10:27 +0800, Ming Lei wrote:

quoted

On Mon, Feb 01, 2021 at 11:48:50AM -0500, David Jeffery wrote:

quoted

When a stacked block device inserts a request into another block
device
using blk_insert_cloned_request, the request's nr_phys_segments
field gets
recalculated by a call to blk_recalc_rq_segments in
blk_cloned_rq_check_limits. But blk_recalc_rq_segments does not
know how to
handle multi-segment discards. For disk types which can handle
multi-segment discards like nvme, this results in discard requests
which
claim a single segment when it should report several, triggering a
warning
in nvme and causing nvme to fail the discard from the invalid
state.

 WARNING: CPU: 5 PID: 191 at drivers/nvme/host/core.c:700
nvme_setup_discard+0x170/0x1e0 [nvme_core]
 ...
 nvme_setup_cmd+0x217/0x270 [nvme_core]
 nvme_loop_queue_rq+0x51/0x1b0 [nvme_loop]
 __blk_mq_try_issue_directly+0xe7/0x1b0
 blk_mq_request_issue_directly+0x41/0x70
 ? blk_account_io_start+0x40/0x50
 dm_mq_queue_rq+0x200/0x3e0
 blk_mq_dispatch_rq_list+0x10a/0x7d0
 ? __sbitmap_queue_get+0x25/0x90
 ? elv_rb_del+0x1f/0x30
 ? deadline_remove_request+0x55/0xb0
 ? dd_dispatch_request+0x181/0x210
 __blk_mq_do_dispatch_sched+0x144/0x290
 ? bio_attempt_discard_merge+0x134/0x1f0
 __blk_mq_sched_dispatch_requests+0x129/0x180
 blk_mq_sched_dispatch_requests+0x30/0x60
 __blk_mq_run_hw_queue+0x47/0xe0
 __blk_mq_delay_run_hw_queue+0x15b/0x170
 blk_mq_sched_insert_requests+0x68/0xe0
 blk_mq_flush_plug_list+0xf0/0x170
 blk_finish_plug+0x36/0x50
 xlog_cil_committed+0x19f/0x290 [xfs]
 xlog_cil_process_committed+0x57/0x80 [xfs]
 xlog_state_do_callback+0x1e0/0x2a0 [xfs]
 xlog_ioend_work+0x2f/0x80 [xfs]
 process_one_work+0x1b6/0x350
 worker_thread+0x53/0x3e0
 ? process_one_work+0x350/0x350
 kthread+0x11b/0x140
 ? __kthread_bind_mask+0x60/0x60
 ret_from_fork+0x22/0x30

This patch fixes blk_recalc_rq_segments to be aware of devices
which can
have multi-segment discards. It calculates the correct discard
segment
count by counting the number of bio as each discard bio is
considered its
own segment.

Signed-off-by: David Jeffery <redacted>
Tested-by: Laurence Oberman <redacted>
---
 block/blk-merge.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 808768f6b174..fe7358bd5d09 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c

@@ -382,6 +382,13 @@ unsigned int blk_recalc_rq_segments(struct

request *rq)

    switch (bio_op(rq->bio)) {
    case REQ_OP_DISCARD:
+           if (queue_max_discard_segments(rq->q) > 1) {
+                   struct bio *bio = rq->bio;
+                   for_each_bio(bio)
+                           nr_phys_segs++;
+                   return nr_phys_segs;
+           }
+           /* fall through */
    case REQ_OP_SECURE_ERASE:
    case REQ_OP_WRITE_ZEROES:
            return 0;

blk_rq_nr_discard_segments() always returns >=1 segments, so no
similar
issue in case of single range discard.

Reviewed-by: Ming Lei <redacted>

And it can be thought as:

Fixes: 1e739730c5b9 ("block: optionally merge discontiguous discard
bios into a single request")

Great, can we get enough acks and push this through its urgent for me
Reviewed-by: Laurence Oberman <redacted>