blk-mq: fixup RESTART when queue becomes idle

BLK_STS_RESOURCE can be returned from driver when any resource is running out of. And the resource may not be related with tags, such as kmalloc(GFP_ATOMIC), when queue is idle under this kind of BLK_STS_RESOURCE, restart can't work any more, then IO hang may be caused.

Most of drivers may call kmalloc(GFP_ATOMIC) in IO path, and almost all returns BLK_STS_RESOURCE under this situation. But for dm-mpath, it may be triggered a bit easier since the request pool of underlying queue may be consumed up much easier. But in reality, it is still not easy to trigger it. I run all kinds of test on dm-mpath/scsi-debug with all kinds of scsi_debug parameters, can't trigger this issue at all. But finally it is triggered in Bart's SRP test, which seems made by genius, :-)

[ ... ]

  static void blk_mq_timeout_work(struct work_struct *work)
  {
  	struct request_queue *q =

block/blk-mq.c +0 -0

@@ -966,8 +1045,10 @@ static void blk_mq_timeout_work(struct work_struct *work)
  		 */

Quoted 22 lines · Ming Lei

BLK_STS_RESOURCE can be returned from driver when any resource is running out of. And the resource may not be related with tags, such as kmalloc(GFP_ATOMIC), when queue is idle under this kind of BLK_STS_RESOURCE, restart can't work any more, then IO hang may be caused.

Most of drivers may call kmalloc(GFP_ATOMIC) in IO path, and almost all returns BLK_STS_RESOURCE under this situation. But for dm-mpath, it may be triggered a bit easier since the request pool of underlying queue may be consumed up much easier. But in reality, it is still not easy to trigger it. I run all kinds of test on dm-mpath/scsi-debug with all kinds of scsi_debug parameters, can't trigger this issue at all. But finally it is triggered in Bart's SRP test, which seems made by genius, :-)

[ ... ]

static void blk_mq_timeout_work(struct work_struct *work) {

block/blk-mq.c +0 -0

 	struct request_queue *q =

@@ -966,8 +1045,10 @@ static void blk_mq_timeout_work(struct work_struct *work)
 		 */

block/blk-mq.c static void blk_mq_timeout_work(struct work_struct *work) · line 1054

Quoted 10 lines

block/blk-mq.c +3 -1

 		queue_for_each_hw_ctx(q, hctx, i) {
 			/* the hctx may be unmapped, so check it here */
-			if (blk_mq_hw_queue_mapped(hctx))
+			if (blk_mq_hw_queue_mapped(hctx)) {
 				blk_mq_tag_idle(hctx);
+				blk_mq_fixup_restart(hctx);
+			}
 		}
 	}
 	blk_queue_exit(q);

Hello Ming,

My comments about the above are as follows:
- It can take up to q->rq_timeout jiffies after a .queue_rq() implementation returned BLK_STS_RESOURCE before blk_mq_timeout_work() gets called. However, it can happen that only a few milliseconds after .queue_rq() returned BLK_STS_RESOURCE that the condition that caused it to return BLK_STS_RESOURCE gets cleared. So the above approach can result in long delays during which it will seem like the queue got stuck. Additionally, I think that the block driver should decide how long it takes before a queue is rerun and not the block layer core.

- The lockup that I reported only occurs with the dm driver but not any other block driver. So why to modify the block layer core since this can be fixed by modifying the dm driver?

- A much simpler fix and a fix that is known to work exists, namely inserting a blk_mq_delay_run_hw_queue() call in the dm driver.

Quoted 9 lines · Bart Van Assche

My comments about the above are as follows:
- It can take up to q->rq_timeout jiffies after a .queue_rq() implementation returned BLK_STS_RESOURCE before blk_mq_timeout_work() gets called. However, it can happen that only a few milliseconds after .queue_rq() returned BLK_STS_RESOURCE that the condition that caused it to return BLK_STS_RESOURCE gets cleared. So the above approach can result in long delays during which it will seem like the queue got stuck. Additionally, I think that the block driver should decide how long it takes before a queue is rerun and not the block layer core.

So configure q->rq_timeout to be shorter? Which is configurable though blk_mq_tag_set's 'timeout' member. It apparently defaults to 30 * HZ.

That is the problem with timeouts, there is generally no one size fits all.

Quoted 3 lines

- The lockup that I reported only occurs with the dm driver but not any other block driver. So why to modify the block layer core since this can be fixed by modifying the dm driver?

Hard to know it is only DM's blk-mq that is impacted. That is the only blk-mq driver that you're testing like this (that is also able to handle faults, etc).

Quoted 2 lines

- A much simpler fix and a fix that is known to work exists, namely inserting a blk_mq_delay_run_hw_queue() call in the dm driver.

Because your "much simpler" fix actively hurts performance, as is detailed in this header:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.16&id=ec3eaf9a673106f66606896aed6ddd20180b02ec

Quoted 12 lines · Mike Snitzer

Quoted 9 lines · Bart Van Assche

My comments about the above are as follows:
- It can take up to q->rq_timeout jiffies after a .queue_rq() implementation returned BLK_STS_RESOURCE before blk_mq_timeout_work() gets called. However, it can happen that only a few milliseconds after .queue_rq() returned BLK_STS_RESOURCE that the condition that caused it to return BLK_STS_RESOURCE gets cleared. So the above approach can result in long delays during which it will seem like the queue got stuck. Additionally, I think that the block driver should decide how long it takes before a queue is rerun and not the block layer core.

Quoted 5 lines

So configure q->rq_timeout to be shorter? Which is configurable though blk_mq_tag_set's 'timeout' member. It apparently defaults to 30 * HZ.

That is the problem with timeouts, there is generally no one size fits all.

Sorry but I think that would be wrong. The delay after which a queue is rerun should not be coupled to the request timeout. These two should be independent.

Quoted 7 lines

Quoted 3 lines

- The lockup that I reported only occurs with the dm driver but not any other block driver. So why to modify the block layer core since this can be fixed by modifying the dm driver?

Hard to know it is only DM's blk-mq that is impacted. That is the only blk-mq driver that you're testing like this (that is also able to handle faults, etc).

That's not correct. I'm also testing the SCSI core, which is one of the most complicated block drivers.

Quoted 6 lines

Quoted 2 lines

- A much simpler fix and a fix that is known to work exists, namely inserting a blk_mq_delay_run_hw_queue() call in the dm driver.

Because your "much simpler" fix actively hurts performance, as is detailed in this header:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.16&id=ec3eaf9a673106f66606896aed6ddd20180b02ec

We are close to the start of the merge window so I think it's better to fall back to an old approach that is known to work than to keep a new approach that is known not to work. Additionally, the performance issue you referred to only affects IOPS and bandwidth more than 1% with the lpfc driver and that is because the queue depth it supports is much lower than for other SCSI HBAs, namely 3 instead of 64.

1%!? Where are you getting that number? Ming has detailed more significant performance gains than 1%.. and not just on lpfc (though you keep seizing on lpfc because of the low queue_depth of 3).

This is all very tiresome.

Quoted 1 line

This is all very tiresome.

Yes, this is tiresome. It is very annoying to me that others keep introducing so many regressions in such important parts of the kernel. It is also annoying to me that I get blamed if I report a regression instead of seeing that the regression gets fixed.

Quoted 6 lines · Bart Van Assche

Quoted 1 line

This is all very tiresome.

Yes, this is tiresome. It is very annoying to me that others keep introducing so many regressions in such important parts of the kernel. It is also annoying to me that I get blamed if I report a regression instead of seeing that the regression gets fixed.

I agree, it sucks that any change there introduces the regression. I'm fine with doing the delay insert again until a new patch is proven to be better.

From the original topic of this email, we have conditions that can cause the driver to not be able to submit an IO. A set of those conditions can only happen if IO is in flight, and those cases we have covered just fine. Another set can potentially trigger without IO being in flight. These are cases where a non-device resource is unavailable at the time of submission. This might be iommu running out of space, for instance, or it might be a memory allocation of some sort. For these cases, we don't get any notification when the shortage clears. All we can do is ensure that we restart operations at some point in the future. We're SOL at that point, but we have to ensure that we make forward progress.

That last set of conditions better not be a a common occurence, since performance is down the toilet at that point. I don't want to introduce hot path code to rectify it. Have the driver return if that happens in a way that is DIFFERENT from needing a normal restart. The driver knows if this is a resource that will become available when IO completes on this device or not. If we get that return, we have a generic run-again delay.

This basically becomes the same as doing the delay queue thing from DM, but just in a generic fashion.

For Bart's test the underlying scsi-mq driver is what is regularly hitting this case in __blk_mq_try_issue_directly():

        if (blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q))

Quoted 4 lines · Mike Snitzer

For Bart's test the underlying scsi-mq driver is what is regularly hitting this case in __blk_mq_try_issue_directly():

        if (blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q))

Hello Mike,

That code path is not the code path that triggered the lockups that I reported during the past days.

These lockups were all triggered by incorrect handling of .queue_rq() returning BLK_STS_RESOURCE.

Quoted 5 lines · Bart Van Assche

Quoted 4 lines · Mike Snitzer

For Bart's test the underlying scsi-mq driver is what is regularly
hitting this case in __blk_mq_try_issue_directly():

Quoted 7 lines

Quoted 1 line

        if (blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q))

Hello Mike,

That code path is not the code path that triggered the lockups that I reported
during the past days.

If you're hitting blk_mq_sched_insert_request() then you most certainly
are hitting that code path.

If you aren't then what was your earlier email going on about?
https://www.redhat.com/archives/dm-devel/2018-January/msg00372.html

If you were just focusing on that as one possible reason, that isn't very helpful.  By this point you really should _know_ what is triggering
the stall based on the code paths taken.  Please use ftrace's function_graph tracer if need be.

Quoted 2 lines

These lockups were all triggered by incorrect handling of .queue_rq() returning BLK_STS_RESOURCE.

Please be precise, dm_mq_queue_rq()'s return of BLK_STS_RESOURCE? "Incorrect" because it no longer runs blk_mq_delay_run_hw_queue()?

Please try to do more work analyzing the test case that only you can easily run (due to srp_test being a PITA).  And less time lobbying for
a change that you don't understand to _really_ be correct.

We have time to get this right, please stop hyperventilating about "regressions".

Thanks,
Mike

Quoted 4 lines · Bart Van Assche

Quoted 3 lines · Mike Snitzer

For Bart's test the underlying scsi-mq driver is what is regularly hitting this case in __blk_mq_try_issue_directly():

Quoted 4 lines

Quoted 1 line

        if (blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q))

These lockups were all triggered by incorrect handling of .queue_rq() returning BLK_STS_RESOURCE.

Please be precise, dm_mq_queue_rq()'s return of BLK_STS_RESOURCE? "Incorrect" because it no longer runs blk_mq_delay_run_hw_queue()?

Please try to do more work analyzing the test case that only you can easily run (due to srp_test being a PITA).

We have time to get this right, please stop hyperventilating about "regressions".

Quoted 7 lines · Mike Snitzer

Quoted 4 lines · Bart Van Assche

Quoted 3 lines · Mike Snitzer

For Bart's test the underlying scsi-mq driver is what is regularly hitting this case in __blk_mq_try_issue_directly():

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

        if (blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q))

These lockups were all triggered by incorrect handling of .queue_rq() returning BLK_STS_RESOURCE.

Please be precise, dm_mq_queue_rq()'s return of BLK_STS_RESOURCE? "Incorrect" because it no longer runs blk_mq_delay_run_hw_queue()?

In what I wrote I was referring to both dm_mq_queue_rq() and scsi_queue_rq().
With "incorrect" I meant that queue lockups are introduced that make user space processes unkillable. That's a severe bug.

Quoted 2 lines

Please try to do more work analyzing the test case that only you can easily run (due to srp_test being a PITA).

It is not correct that I'm the only one who is able to run that software. Anyone who is willing to merge the latest SRP initiator and target driver patches in his or her tree can run that software in
any VM. I'm working hard
on getting the patches upstream that make it possible to run the srp-test software on a setup that is not equipped with InfiniBand hardware.

Quoted 2 lines

We have time to get this right, please stop hyperventilating about "regressions".

Sorry Mike but that's something I consider as an unfair comment. If Ming and
you work on patches together, it's your job to make sure that no regressions
are introduced. Instead of blaming me because I report these regressions you
should be grateful that I take the time and effort to report these regressions
early. And since you are employed by a large organization that sells Linux support services, your employer should invest in developing test cases that reach a higher coverage of the dm, SCSI and block layer code. I don't think that it's normal that my tests discovered several issues that were not discovered by Red Hat's internal test suite. That's something Red Hat has to
address.

Bart.

Quoted 12 lines · Mike Snitzer

Quoted 8 lines · Bart Van Assche

Quoted 4 lines · Mike Snitzer

For Bart's test the underlying scsi-mq driver is what is regularly hitting this case in __blk_mq_try_issue_directly():

        if (blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q))

These lockups were all triggered by incorrect handling of .queue_rq() returning BLK_STS_RESOURCE.

Quoted 2 lines

Please be precise, dm_mq_queue_rq()'s return of BLK_STS_RESOURCE? "Incorrect" because it no longer runs blk_mq_delay_run_hw_queue()?

In what I wrote I was referring to both dm_mq_queue_rq() and scsi_queue_rq(). With "incorrect" I meant that queue lockups are introduced that make user space processes unkillable. That's a severe bug.

Quoted 2 lines

We have time to get this right, please stop hyperventilating about "regressions".

Sorry Mike but that's something I consider as an unfair comment. If Ming and you work on patches together, it's your job to make sure that no regressions are introduced. Instead of blaming me because I report these regressions you should be grateful that I take the time and effort to report these regressions early. And since you are employed by a large organization that sells Linux support services, your employer should invest in developing test cases that reach a higher coverage of the dm, SCSI and block layer code. I don't think that it's normal that my tests discovered several issues that were not discovered by Red Hat's internal test suite. That's something Red Hat has to address.

Quoted 19 lines · Bart Van Assche

Quoted 11 lines · Mike Snitzer

Quoted 8 lines · Bart Van Assche

Quoted 6 lines · Mike Snitzer

For Bart's test the underlying scsi-mq driver is what is regularly
hitting this case in __blk_mq_try_issue_directly():

        if (blk_mq_hctx_stopped(hctx) ||
blk_queue_quiesced(q))

Quoted 6 lines

Quoted 2 lines

These lockups were all triggered by incorrect handling of .queue_rq() returning BLK_STS_RESOURCE.

Please be precise, dm_mq_queue_rq()'s return of BLK_STS_RESOURCE? "Incorrect" because it no longer runs blk_mq_delay_run_hw_queue()?

Quoted 5 lines

In what I wrote I was referring to both dm_mq_queue_rq() and scsi_queue_rq().
With "incorrect" I meant that queue lockups are introduced that make user
space processes unkillable. That's a severe bug.

And yet Laurence cannot reproduce any such lockups with your test...

Are you absolutely certain this patch doesn't help you?
https://patchwork.kernel.org/patch/10174037/

If it doesn't then that is actually very useful to know.

Quoted 15 lines

Quoted 3 lines

We have time to get this right, please stop hyperventilating about
"regressions".

Sorry Mike but that's something I consider as an unfair comment. If Ming and
you work on patches together, it's your job to make sure that no regressions
are introduced. Instead of blaming me because I report these regressions you
should be grateful that I take the time and effort to report these regressions
early. And since you are employed by a large organization that sells Linux
support services, your employer should invest in developing test

Quoted 8 lines

cases that
reach a higher coverage of the dm, SCSI and block layer code. I don't think
that it's normal that my tests discovered several issues that were not
discovered by Red Hat's internal test suite. That's something Red Hat has to
address.

You have no self-awareness of just how mypoic you're being about this.

I'm not ignoring or blaming you for your test no longer passing.  Far from it.  I very much want to fix this.  But I want it fixed in a way that doesn't paper over the real bug(s) while also introducing blind queue runs that compromise the blk-mq RESTART code's ability to work as
intended.

I'd have thought you could appreciate this.  We need a root cause on this, not hand-waving justifications on why problematic delayed queue runs are correct.

Please just focus on helping Laurence get his very capable testbed to reproduce this issue.  Once we can reproduce these "unkillable" "stalls"
in-house it'll be _much_ easier to analyze and fix.

Thanks,
Mike

Quoted 73 lines · Mike Snitzer

Quoted 19 lines · Bart Van Assche

Quoted 11 lines · Mike Snitzer

Quoted 8 lines · Bart Van Assche

Quoted 6 lines · Mike Snitzer

For Bart's test the underlying scsi-mq driver is what is regularly
hitting this case in __blk_mq_try_issue_directly():

        if (blk_mq_hctx_stopped(hctx) ||

blk_queue_quiesced(q))

Quoted 6 lines

Quoted 2 lines

These lockups were all triggered by incorrect handling of .queue_rq() returning BLK_STS_RESOURCE.

Please be precise, dm_mq_queue_rq()'s return of BLK_STS_RESOURCE? "Incorrect" because it no longer runs blk_mq_delay_run_hw_queue()?

Quoted 5 lines

In what I wrote I was referring to both dm_mq_queue_rq() and scsi_queue_rq().
With "incorrect" I meant that queue lockups are introduced that make user
space processes unkillable. That's a severe bug.

And yet Laurence cannot reproduce any such lockups with your test...

Are you absolutely certain this patch doesn't help you?
https://patchwork.kernel.org/patch/10174037/

If it doesn't then that is actually very useful to know.

Quoted 15 lines

Quoted 3 lines

We have time to get this right, please stop hyperventilating about
"regressions".

Sorry Mike but that's something I consider as an unfair comment. If Ming and
you work on patches together, it's your job to make sure that no regressions
are introduced. Instead of blaming me because I report these regressions you
should be grateful that I take the time and effort to report these regressions
early. And since you are employed by a large organization that sells Linux
support services, your employer should invest in developing test

Quoted 8 lines

cases that
reach a higher coverage of the dm, SCSI and block layer code. I don't think
that it's normal that my tests discovered several issues that were not
discovered by Red Hat's internal test suite. That's something Red Hat has to
address.

You have no self-awareness of just how mypoic you're being about this.

I'm not ignoring or blaming you for your test no longer passing. Far from it. I very much want to fix this. But I want it fixed in a way that doesn't paper over the real bug(s) while also introducing blind queue runs that compromise the blk-mq RESTART code's ability to work as
intended.

I'd have thought you could appreciate this. We need a root cause on this, not hand-waving justifications on why problematic delayed queue runs are correct.

Quoted 7 lines

Please just focus on helping Laurence get his very capable testbed to reproduce this issue. Once we can reproduce these "unkillable" "stalls"
in-house it'll be _much_ easier to analyze and fix.

Thanks,
Mike

Hello Bart

OK, I ran 5 at once of 5 separate mount points.
I am using 4k block sizes
Its solid consistent for me. No stalls no gaps.

[global]
name=02-mq
filename=fio-output-02-mq.txt
rw=randwrite
verify=md5
;rwmixread=60
;rwmixwrite=40
bs=4K
;direct=1
;numjobs=4
;time_based=1
runtime=120

[file1]
size=3G
ioengine=libaio
iodepth=16

I watch I/O and I see it ramp up but fio still runs and it kind of shuts down.

#!/bin/bash
for i in 1 2 3 4 5
do

        cd /data-$i
        fio /root/srp-test.lobe/fio_tests/02-mq 1>/data-$i.out 2>&1 &

done

#Time cpu sys inter ctxsw KBRead Reads KBWrit Writes

17:16:09   34  12 34431  10393      0      0 249872  36094
17:16:10   31  10 13001   2679      0      0  57652   7980
17:16:11   32  11 19473   4416      0      0 143248  17362
17:16:12   31   9  8068   1606      0      0  20088   2026
17:16:13   31   9  7718   1518      0      0  15908   1354
17:16:14   36  14 41132   9710      0      0 686216  46661
17:16:15   39  18 63622  21033      0      0  1246K  75108
17:16:16   38  16 76114   9013      0      0  1146K  82338
17:16:17   33  11 31941   2735      0      0 237340  25034
17:16:18   36  14 23900   4982      0      1  1567K  43244
17:16:19   37  15 55884   4974      0      4  1003K  67253
17:16:20   28  12  7542   4975      0      0  1630K   3266
17:16:21    8   6 14650  34721      0      0  2535K  21206
17:16:22    2   2  6655  15839      0      0  2319K   9897
17:16:23   11  11 37305 134030      0      0  1275K  87010
17:16:24   23  22 59266 119350   6560   1640  1102K  78034
17:16:25   21  17 65699 144462 148052  37013 159900  22120
17:16:26   14   9 80700 164034 216588  54147      4      1
#         <----CPU[HYPER]-----><----------Disks----------->
#Time     cpu sys inter  ctxsw KBRead  Reads KBWrit Writes
17:16:27   14   9 78699 162095 214412  53603      0      0
17:16:28   14   9 75895 155352 204932  51233      0      0
17:16:29   14   9 75377 161871 214136  53534      0      0
17:16:30   16  11 79298 155967 206164  51541      0      0
17:16:31   14   8 82281 165266 218652  54662      0      0
17:16:32   14   9 88635 163302 215876  53970      0      0
17:16:33   14   8 88755 168958 223568  55892      0      0
17:16:34   15   9 82073 154321 203936  50984      0      0
17:16:35   17  10 87004 161467 213308  53327      0      0
17:16:36   15   9 86198 161247 212940  53235      4      1
17:16:37   15   9 83429 162890 215276  53819      0      0
17:16:38   16  10 83272 160611 212264  53066     20      2
17:16:39   16  10 93850 168179 222376  55594      0      0
17:16:40   15   9 86801 167507 221372  55343      0      0
17:16:41   14   8 82688 171249 226560  56640      0      0
17:16:42   14   8 86202 159171 210344  52586      0      0
17:16:43   16  10 86063 156009 206328  51582      0      0
17:16:44   16  10 90927 153961 203584  50896      0      0
17:16:45   15   9 95084 156370 206844  51710    132     13
17:16:46   14   8 85572 158469 209300  52326      4      1
17:16:47   14   8 92075 158469 209600  52400      0      0
17:16:48   16  10 95469 154772 204176  51044      0      0
#         <----CPU[HYPER]-----><----------Disks----------->
#Time     cpu sys inter  ctxsw KBRead  Reads KBWrit Writes
17:16:49   15  10 93856 154182 203568  50897      5      5
17:16:50   15  10 92308 151964 200680  50170      0      0
17:16:51   16  10 94035 155076 204868  51217      0      0
17:16:52   15   9 93949 164969 217980  54495      0      0
17:16:53   14   9 85744 149553 197428  49357      0      0
17:16:54   14   9 83064 155788 205684  51421      0      0
17:16:55   16  10 83579 166084 218480  54620      0      0
17:16:56   15   9 74766 158728 208556  52139      4      1
17:16:57   14   9 82944 162720 215116  53779      0      0
17:16:58   12   7 88529 163814 216440  54110      0      0
17:16:59   14   7 85936 166723 220372  55093      4      1
17:17:00   13   8 86701 153728 203052  50763      0      0
17:17:01   14   9 92859 155963 205972  51493      0      0
17:17:02   13   8 85624 143746 189756  47439      0      0
17:17:03   13   8 85029 142540 188044  47010      0      0
17:17:04   14   8 87291 144790 191148  47788      0      0
17:17:05   14   8 81922 144342 190544  47636      0      0
17:17:06   14   8 81587 161043 212576  53144      8      2
17:17:07   15   8 85080 142285 187688  46922      0      0
17:17:08   13   8 86980 150246 197852  49463      0      0
17:17:09   12   8 81388 144335 190312  47578      4      1
17:17:10   13   8 88436 146957 194040  48510      0      0
#         <----CPU[HYPER]-----><----------Disks----------->
#Time     cpu sys inter  ctxsw KBRead  Reads KBWrit Writes
17:17:11   11   7 78997 137847 181828  45457      0      0
17:17:12   12   8 85709 148847 196708  49177      0      0
17:17:13   17  10 97228 170646 225528  56382      0      0
17:17:14   14   9 96032 173474 229400  57350      0      0
17:17:15   15   9 95340 167951 221848  55462      0      0
17:17:16   16  10 87392 157489 207748  51937     38      5
17:17:17   16  10 84756 152753 201716  50429      0      0
17:17:18   17  10 85109 153000 202080  50520      0      0
17:17:19   16  10 92109 164514 217396  54349     80      5
17:17:20   15   9 91342 155674 205804  51451      0      0
17:17:21   12   7 73921 126577 167060  41765      0      0
17:17:22   13   8 76916 126701 167176  41794      0      0
17:17:23   13   8 85187 151074 199768  49942      0      0
17:17:24   15   9 85737 153664 203168  50792      0      0
17:17:25   15   9 87113 147636 195084  48771      0      0
17:17:26   14   8 89440 157433 208116  52029      4      1
17:17:27   14   9 95290 172224 227036  56759      0      0
17:17:28   15  10 93918 167611 220164  55041      0      0
17:17:29   13   8 82404 149274 197312  49328      0      0
17:17:30   10   7 75961 138276 182668  45667      0      0
17:17:31   12   8 84899 151829 200764  50191      0      0
17:17:32   13   8 81386 141091 186444  46611      0      0
17:17:33   15   9  100K 167489 221640  55410      0      0
17:17:34   14   9 89601 158765 210032  52508      0      0
17:17:35   14   9 92440 155287 205216  51304      0      0
17:17:36   15  10 90042 155934 206168  51542      4      1
17:17:37   14   9 91670 164456 217684  54421      0      0
17:17:38   14   9 78612 167368 221348  55337      0      0
17:17:39   15   9 91325 162599 214616  53654      0      0
17:17:40   14   9 79112 151595 199984  50000     21      8
17:17:41    9   3 32565  60936  79832  19964     32     12

OK, I ran 5 at once of 5 separate mount points.
I am using 4k block sizes
Its solid consistent for me. No stalls no gaps.

[global]
name=02-mq
filename=fio-output-02-mq.txt
rw=randwrite
verify=md5
;rwmixread=60
;rwmixwrite=40
bs=4K
;direct=1
;numjobs=4
;time_based=1
runtime=120

[file1]
size=3G
ioengine=libaio
iodepth=16

I watch I/O and I see it ramp up but fio still runs and it kind of shuts down.

Quoted 3 lines · Laurence Oberman

OK, I ran 5 at once of 5 separate mount points.
I am using 4k block sizes
Its solid consistent for me. No stalls no gaps.

Hi Laurence,

That's great news and thank you for having shared this information but I think
it should be mentioned that you have been running my tree in which some recent
block and dm patches were reverted
(https://github.com/bvanassche/linux/tree/block-scsi-for-next)

Quoted 14 lines

[global]
name=02-mq
filename=fio-output-02-mq.txt
rw=randwrite
verify=md5
;rwmixread=60
;rwmixwrite=40
bs=4K
;direct=1
;numjobs=4
;time_based=1
runtime=120

Quoted 7 lines

[file1]
size=3G
ioengine=libaio
iodepth=16

I watch I/O and I see it ramp up but fio still runs and it kind of shuts down.

In test "file1" I see an I/O size of 3G. Does that mean that the patch that
should fix the sgl_alloc() issue is working?

Thanks,

Bart.

Quoted 36 lines · Bart Van Assche

Quoted 3 lines · Laurence Oberman

OK, I ran 5 at once of 5 separate mount points.
I am using 4k block sizes
Its solid consistent for me. No stalls no gaps.

Hi Laurence,

That's great news and thank you for having shared this information but I think
it should be mentioned that you have been running my tree in which some recent
block and dm patches were reverted
(https://github.com/bvanassche/linux/tree/block-scsi-for-next)

Quoted 14 lines

[global]
name=02-mq
filename=fio-output-02-mq.txt
rw=randwrite
verify=md5
;rwmixread=60
;rwmixwrite=40
bs=4K
;direct=1
;numjobs=4
;time_based=1
runtime=120

Quoted 7 lines

[file1]
size=3G
ioengine=libaio
iodepth=16

I watch I/O and I see it ramp up but fio still runs and it kind of shuts down.

Quoted 7 lines

In test "file1" I see an I/O size of 3G. Does that mean that the patch that
should fix the sgl_alloc() issue is working?

Thanks,

Bart.

Hello Bart Thank you.

OK, so booting into Mike tree now and then hopefully I get the lockups. Can you give me some idea of what to look for.
I assume I/O just stops.

I want to get this happening in-house so we have an avenue to fix this. Following getting the stall I will attend to the slg patch test for the SRPT side.

Please note I am not running latest SRPT right now as you know.

When you do have a solid test case, please please submit a blktests test case for it! This needs to be something we can regularly in testing.

And yet Laurence cannot reproduce any such lockups with your test...

Are you absolutely certain this patch doesn't help you?
https://patchwork.kernel.org/patch/10174037/

If it doesn't then that is actually very useful to know.

Please just focus on helping Laurence get his very capable testbed to reproduce this issue. Once we can reproduce these "unkillable" "stalls" in-house it'll be _much_ easier to analyze and fix.

Quoted 1 line · Mike Snitzer

And yet Laurence cannot reproduce any such lockups with your test...

Hmm ... maybe I misunderstood Laurence but I don't think that Laurence has already succeeded at running an unmodified version of my tests. In one of the e-mails Laurence sent me this morning I read that he modified these scripts to get past a kernel module unload failure that was reported while starting these tests. So the next step is to check which changes were made to the test scripts and also whether the test results are still valid.

Quoted 4 lines

Are you absolutely certain this patch doesn't help you?
https://patchwork.kernel.org/patch/10174037/

If it doesn't then that is actually very useful to know.

The first I tried this morning is to run the srp-test software against a merge of Jens' for-next branch and your dm-4.16 branch. Since I noticed that the dm queue locked up I reinserted a blk_mq_delay_run_hw_queue() call in the dm code. Since even that was not sufficient I tried to kick the queues via debugfs (for s in /sys/kernel/debug/block/*/state; do echo kick >$s; done). Since that was not sufficient to resolve the queue stall I reverted the following tree patches that are in Jens' tree:
* "blk-mq: improve DM's blk-mq IO merging via blk_insert_cloned_request feedback"
* "blk-mq-sched: remove unused 'can_block' arg from blk_mq_sched_insert_request"
* "blk-mq: don't dispatch request in blk_mq_request_direct_issue if queue is busy"

Only after I had done this the srp-test software ran again without triggering dm queue lockups.

Quoted 26 lines · Bart Van Assche

Quoted 1 line · Mike Snitzer

And yet Laurence cannot reproduce any such lockups with your test...

Hmm ... maybe I misunderstood Laurence but I don't think that Laurence has already succeeded at running an unmodified version of my tests. In one of the e-mails Laurence sent me this morning I read that he modified these scripts to get past a kernel module unload failure that was reported while starting these tests. So the next step is to check which changes were made to the test scripts and also whether the test results are still valid.

Quoted 4 lines

Are you absolutely certain this patch doesn't help you?
https://patchwork.kernel.org/patch/10174037/

If it doesn't then that is actually very useful to know.

The first I tried this morning is to run the srp-test software against a merge of Jens' for-next branch and your dm-4.16 branch. Since I noticed that the dm queue locked up I reinserted a blk_mq_delay_run_hw_queue() call in the dm code. Since even that was not sufficient I tried to kick the queues via debugfs (for s in /sys/kernel/debug/block/*/state; do echo kick >$s; done). Since that was not sufficient to resolve the queue stall I reverted the following tree patches that are in Jens' tree:
* "blk-mq: improve DM's blk-mq IO merging via blk_insert_cloned_request feedback"
* "blk-mq-sched: remove unused 'can_block' arg from blk_mq_sched_insert_request"
* "blk-mq: don't dispatch request in blk_mq_request_direct_issue if queue is busy"

Quoted 2 lines

Only after I had done this the srp-test software ran again without triggering dm queue lockups.

Given that Ming's notifier-based patchset needs more development time I think we're unfortunately past the point where we can comfortably wait for that to be ready.

So we need to explore alternatives to fixing this IO stall regression.

Rather than attempt the above block reverts (which is an incomplete listing given newer changes): might we develop a more targeted code change to neutralize commit 396eaf21ee ("blk-mq: improve DM's blk-mq IO merging via blk_insert_cloned_request feedback")? -- which, given Bart's findings above, seems to be the most problematic block commit.

To that end, assuming I drop this commit from dm-4.16:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.16&id=316a795ad388e0c3ca613454851a28079d917a92

Here is my proposal for putting this regression behind us for 4.16 (Ming's line of development would continue and hopefully be included in 4.17):

From: Mike Snitzer <[email protected]>
Date: Tue, 23 Jan 2018 09:40:22 +0100
Subject: [PATCH] block: neutralize blk_insert_cloned_request IO stall regression

The series of blk-mq changes intended to improve sequential IO performace (through improved merging with dm-mapth blk-mq stacked on underlying blk-mq device). Unfortunately these changes have caused dm-mpath blk-mq IO stalls when blk_mq_request_issue_directly()'s call to q->mq_ops->queue_rq() fails (due to device-specific resource unavailability).

Fix this by reverting back to how blk_insert_cloned_request() functioned prior to commit 396eaf21ee -- by using blk_mq_request_bypass_insert() instead of blk_mq_request_issue_directly().

In the future, this commit should be reverted as the first change in a followup series of changes that implements a comprehensive solution to allowing an underlying blk-mq queue's resource limitation to trigger the upper blk-mq queue to run once that underlying limited resource is replenished.

Fixes 396eaf21ee ("blk-mq: improve DM's blk-mq IO merging via blk_insert_cloned_request feedback")

Signed-off-by Mike Snitzer <[email protected]>

---
 block/blk-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

block/blk-core.c +0 -0

index cdae69be68e9..a224f282b4a6 100644

Hi Mike,

Quoted 36 lines · Mike Snitzer

Quoted 26 lines · Bart Van Assche

Quoted 1 line · Mike Snitzer

And yet Laurence cannot reproduce any such lockups with your test...

Hmm ... maybe I misunderstood Laurence but I don't think that Laurence has already succeeded at running an unmodified version of my tests. In one of the e-mails Laurence sent me this morning I read that he modified these scripts to get past a kernel module unload failure that was reported while starting these tests. So the next step is to check which changes were made to the test scripts and also whether the test results are still valid.

Quoted 4 lines

Are you absolutely certain this patch doesn't help you?
https://patchwork.kernel.org/patch/10174037/

If it doesn't then that is actually very useful to know.

The first I tried this morning is to run the srp-test software against a merge of Jens' for-next branch and your dm-4.16 branch. Since I noticed that the dm queue locked up I reinserted a blk_mq_delay_run_hw_queue() call in the dm code. Since even that was not sufficient I tried to kick the queues via debugfs (for s in /sys/kernel/debug/block/*/state; do echo kick >$s; done). Since that was not sufficient to resolve the queue stall I reverted the following tree patches that are in Jens' tree:
* "blk-mq: improve DM's blk-mq IO merging via blk_insert_cloned_request feedback"
* "blk-mq-sched: remove unused 'can_block' arg from blk_mq_sched_insert_request"
* "blk-mq: don't dispatch request in blk_mq_request_direct_issue if queue is busy"

Quoted 2 lines

Only after I had done this the srp-test software ran again without triggering dm queue lockups.

Given that Ming's notifier-based patchset needs more development time I think we're unfortunately past the point where we can comfortably wait for that to be ready.

Quoted 1 line

So we need to explore alternatives to fixing this IO stall regression.

The fix for IO stall doesn't need the notifier-based patchset, and only the 1st patch is enough for fixing the IO stall. And it is a generic issue, which need generic solution, that is the conclusion made by Jens and me.

	https://marc.info/?l=linux-kernel&m=151638176727612&w=2

And the notifier-based patchset is for solving the performance issue reported by Jens:

- run IO on dm-mpath
- run background IO on low depth underlying queue
- then IO performance on dm-mpath is extremely slow

I will send out the 1st patch of 'blk-mq: introduce BLK_STS_DEV_RESOURCE' soon, but the notifier-based patchset shouldn't be very urgent, since the above test case isn't usual in reality.

Quoted 5 lines

Rather than attempt the above block reverts (which is an incomplete listing given newer changes): might we develop a more targeted code change to neutralize commit 396eaf21ee ("blk-mq: improve DM's blk-mq IO merging via blk_insert_cloned_request feedback")? -- which, given Bart's findings above, seems to be the most problematic block commit.

The stall isn't related with commit 396eaf21ee too.

Quoted 7 lines

To that end, assuming I drop this commit from dm-4.16:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.16&id=316a795ad388e0c3ca613454851a28079d917a92

Here is my proposal for putting this regression behind us for 4.16 (Ming's line of development would continue and hopefully be included in 4.17):

Actually notifier based approach is ready, even cache for clone is ready too, but the test result isn't good enough on random IO on Jens's above case, and sequential IO is much better with both cache clone and notifier based allocation(much better than non-mq). And follows the tree if anyone is interested:

https://github.com/ming1/linux/commits/v4.15-rc-block-dm-mpath

Now looks there is still one issue: the notifier can come early, just before the request is added to hctx->dispatch_list, and performance still gets hurt, especially on random IO in Jens's case. But queue won't stall, :-)

Quoted 28 lines

From: Mike Snitzer <[email protected]>
Date: Tue, 23 Jan 2018 09:40:22 +0100
Subject: [PATCH] block: neutralize blk_insert_cloned_request IO stall regression

The series of blk-mq changes intended to improve sequential IO performace (through improved merging with dm-mapth blk-mq stacked on underlying blk-mq device). Unfortunately these changes have caused dm-mpath blk-mq IO stalls when blk_mq_request_issue_directly()'s call to q->mq_ops->queue_rq() fails (due to device-specific resource unavailability).

Fix this by reverting back to how blk_insert_cloned_request() functioned prior to commit 396eaf21ee -- by using blk_mq_request_bypass_insert() instead of blk_mq_request_issue_directly().

In the future, this commit should be reverted as the first change in a followup series of changes that implements a comprehensive solution to allowing an underlying blk-mq queue's resource limitation to trigger the upper blk-mq queue to run once that underlying limited resource is replenished.

Fixes 396eaf21ee ("blk-mq: improve DM's blk-mq IO merging via blk_insert_cloned_request feedback")

Signed-off-by Mike Snitzer <[email protected]>

---
 block/blk-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

block/blk-core.c blk_status_t blk_insert_cloned_request(struct request_queue *q, struct request * · line 2525

Quoted 12 lines

block/blk-core.c +2 -1

index cdae69be68e9..a224f282b4a6 100644
@@ -2520,7 +2520,8 @@ blk_status_t blk_insert_cloned_request(struct request_queue *q, struct request *
 		 * bypass a potential scheduler on the bottom device for
 		 * insert.
 		 */
-		return blk_mq_request_issue_directly(rq);
+		blk_mq_request_bypass_insert(rq, true);
+		return BLK_STS_OK;
 	}

If this patch is for fixing IO stall on DM, it isn't needed, and actually it can't fix the IO stall issue.

Quoted 39 lines · Ming Lei

Hi Mike,

Quoted 36 lines · Mike Snitzer

Quoted 26 lines · Bart Van Assche

Quoted 1 line · Mike Snitzer

And yet Laurence cannot reproduce any such lockups with your test...

Hmm ... maybe I misunderstood Laurence but I don't think that Laurence has already succeeded at running an unmodified version of my tests. In one of the e-mails Laurence sent me this morning I read that he modified these scripts to get past a kernel module unload failure that was reported while starting these tests. So the next step is to check which changes were made to the test scripts and also whether the test results are still valid.

Quoted 4 lines

Are you absolutely certain this patch doesn't help you?
https://patchwork.kernel.org/patch/10174037/

If it doesn't then that is actually very useful to know.

The first I tried this morning is to run the srp-test software against a merge of Jens' for-next branch and your dm-4.16 branch. Since I noticed that the dm queue locked up I reinserted a blk_mq_delay_run_hw_queue() call in the dm code. Since even that was not sufficient I tried to kick the queues via debugfs (for s in /sys/kernel/debug/block/*/state; do echo kick >$s; done). Since that was not sufficient to resolve the queue stall I reverted the following tree patches that are in Jens' tree:
* "blk-mq: improve DM's blk-mq IO merging via blk_insert_cloned_request feedback"
* "blk-mq-sched: remove unused 'can_block' arg from blk_mq_sched_insert_request"
* "blk-mq: don't dispatch request in blk_mq_request_direct_issue if queue is busy"

Quoted 2 lines

Only after I had done this the srp-test software ran again without triggering dm queue lockups.

Given that Ming's notifier-based patchset needs more development time I think we're unfortunately past the point where we can comfortably wait for that to be ready.

Quoted 8 lines

Quoted 1 line

So we need to explore alternatives to fixing this IO stall regression.

The fix for IO stall doesn't need the notifier-based patchset, and only the 1st patch is enough for fixing the IO stall. And it is a generic issue, which need generic solution, that is the conclusion made by Jens and me.

      https://marc.info/?l=linux-kernel&m=151638176727612&w=2

That's fine if Bart verifies it.

Quoted 68 lines

And the notifier-based patchset is for solving the performance issue reported by Jens:

- run IO on dm-mpath
- run background IO on low depth underlying queue
- then IO performance on dm-mpath is extremely slow

I will send out the 1st patch of 'blk-mq: introduce BLK_STS_DEV_RESOURCE' soon, but the notifier-based patchset shouldn't be very urgent, since the above test case isn't usual in reality.

Quoted 5 lines

Rather than attempt the above block reverts (which is an incomplete listing given newer changes): might we develop a more targeted code change to neutralize commit 396eaf21ee ("blk-mq: improve DM's blk-mq IO merging via blk_insert_cloned_request feedback")? -- which, given Bart's findings above, seems to be the most problematic block commit.

The stall isn't related with commit 396eaf21ee too.

Quoted 7 lines

To that end, assuming I drop this commit from dm-4.16:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.16&id=316a795ad388e0c3ca613454851a28079d917a92

Here is my proposal for putting this regression behind us for 4.16 (Ming's line of development would continue and hopefully be included in 4.17):

Actually notifier based approach is ready, even cache for clone is ready too, but the test result isn't good enough on random IO on Jens's above case, and sequential IO is much better with both cache clone and notifier based allocation(much better than non-mq). And follows the tree if anyone is interested:

https://github.com/ming1/linux/commits/v4.15-rc-block-dm-mpath

Now looks there is still one issue: the notifier can come early, just before the request is added to hctx->dispatch_list, and performance still gets hurt, especially on random IO in Jens's case. But queue won't stall, :-)

Quoted 28 lines

From: Mike Snitzer <[email protected]>
Date: Tue, 23 Jan 2018 09:40:22 +0100
Subject: [PATCH] block: neutralize blk_insert_cloned_request IO stall regression

The series of blk-mq changes intended to improve sequential IO performace (through improved merging with dm-mapth blk-mq stacked on underlying blk-mq device). Unfortunately these changes have caused dm-mpath blk-mq IO stalls when blk_mq_request_issue_directly()'s call to q->mq_ops->queue_rq() fails (due to device-specific resource unavailability).

Fix this by reverting back to how blk_insert_cloned_request() functioned prior to commit 396eaf21ee -- by using blk_mq_request_bypass_insert() instead of blk_mq_request_issue_directly().

In the future, this commit should be reverted as the first change in a followup series of changes that implements a comprehensive solution to allowing an underlying blk-mq queue's resource limitation to trigger the upper blk-mq queue to run once that underlying limited resource is replenished.

Fixes 396eaf21ee ("blk-mq: improve DM's blk-mq IO merging via blk_insert_cloned_request feedback")

Signed-off-by Mike Snitzer <[email protected]>

---
 block/blk-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Quoted 15 lines

block/blk-core.c blk_status_t blk_insert_cloned_request(struct request_queue *q, struct request * · line 2525

Quoted 12 lines

block/blk-core.c +2 -1

index cdae69be68e9..a224f282b4a6 100644
@@ -2520,7 +2520,8 @@ blk_status_t blk_insert_cloned_request(struct request_queue *q, struct request *
             * bypass a potential scheduler on the bottom device for
             * insert.
             */
-           return blk_mq_request_issue_directly(rq);
+           blk_mq_request_bypass_insert(rq, true);
+           return BLK_STS_OK;
    }

If this patch is for fixing IO stall on DM, it isn't needed, and actually it can't fix the IO stall issue.

It should "fix it" in conjunction with this:

https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.16&id=87b7e73546b55f4a3a37d4726daedd9a17a20509

(Bart already said as much, I just effectively enabled the equivalent of his reverts)

Quoted 50 lines · Mike Snitzer

Quoted 46 lines · Ming Lei

Hi Mike,

Quoted 33 lines · Mike Snitzer

From: Mike Snitzer <[email protected]>
Date: Tue, 23 Jan 2018 09:40:22 +0100
Subject: [PATCH] block: neutralize blk_insert_cloned_request IO stall regression

The series of blk-mq changes intended to improve sequential IO performace (through improved merging with dm-mapth blk-mq stacked on underlying blk-mq device). Unfortunately these changes have caused dm-mpath blk-mq IO stalls when blk_mq_request_issue_directly()'s call to q->mq_ops->queue_rq() fails (due to device-specific resource unavailability).

Fix this by reverting back to how blk_insert_cloned_request() functioned prior to commit 396eaf21ee -- by using blk_mq_request_bypass_insert() instead of blk_mq_request_issue_directly().

In the future, this commit should be reverted as the first change in a followup series of changes that implements a comprehensive solution to allowing an underlying blk-mq queue's resource limitation to trigger the upper blk-mq queue to run once that underlying limited resource is replenished.

Fixes 396eaf21ee ("blk-mq: improve DM's blk-mq IO merging via blk_insert_cloned_request feedback")

Signed-off-by Mike Snitzer <[email protected]>

---
 block/blk-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

block/blk-core.c +0 -0

index cdae69be68e9..a224f282b4a6 100644

block/blk-core.c blk_status_t blk_insert_cloned_request(struct request_queue *q, struct request * · line 2525

Quoted 7 lines

block/blk-core.c +2 -1

@@ -2520,7 +2520,8 @@ blk_status_t blk_insert_cloned_request(struct request_queue *q, struct request *
             * bypass a potential scheduler on the bottom device for
             * insert.
             */
-           return blk_mq_request_issue_directly(rq);
+           blk_mq_request_bypass_insert(rq, true);
+           return BLK_STS_OK;
    }

If this patch is for fixing IO stall on DM, it isn't needed, and actually it can't fix the IO stall issue.

Quoted 6 lines

It should "fix it" in conjunction with this:

https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.16&id=87b7e73546b55f4a3a37d4726daedd9a17a20509

(Bart already said as much, I just effectively enabled the equivalent of his reverts)

If the generic solution is take, Bart's revert isn't needed.

Quoted 26 lines · Bart Van Assche

Quoted 1 line · Mike Snitzer

And yet Laurence cannot reproduce any such lockups with your test...

Hmm ... maybe I misunderstood Laurence but I don't think that Laurence has already succeeded at running an unmodified version of my tests. In one of the e-mails Laurence sent me this morning I read that he modified these scripts to get past a kernel module unload failure that was reported while starting these tests. So the next step is to check which changes were made to the test scripts and also whether the test results are still valid.

Quoted 4 lines

Are you absolutely certain this patch doesn't help you?
https://patchwork.kernel.org/patch/10174037/

If it doesn't then that is actually very useful to know.

The first I tried this morning is to run the srp-test software against a merge of Jens' for-next branch and your dm-4.16 branch. Since I noticed that the dm queue locked up I reinserted a blk_mq_delay_run_hw_queue() call in the dm code. Since even that was not sufficient I tried to kick the queues via debugfs (for s in /sys/kernel/debug/block/*/state; do echo kick >$s; done). Since that was not sufficient to resolve the queue stall I reverted the following tree patches that are in Jens' tree:
* "blk-mq: improve DM's blk-mq IO merging via blk_insert_cloned_request feedback"
* "blk-mq-sched: remove unused 'can_block' arg from blk_mq_sched_insert_request"
* "blk-mq: don't dispatch request in blk_mq_request_direct_issue if queue is busy"

Quoted 2 lines

Only after I had done this the srp-test software ran again without triggering dm queue lockups.

Quoted 6 lines · Bart Van Assche

Quoted 1 line

This is all very tiresome.

Yes, this is tiresome. It is very annoying to me that others keep introducing so many regressions in such important parts of the kernel. It is also annoying to me that I get blamed if I report a regression instead of seeing that the regression gets fixed.

I agree, it sucks that any change there introduces the regression. I'm fine with doing the delay insert again until a new patch is proven to be better.

From the original topic of this email, we have conditions that can cause the driver to not be able to submit an IO. A set of those conditions can only happen if IO is in flight, and those cases we have covered just fine. Another set can potentially trigger without IO being in flight. These are cases where a non-device resource is unavailable at the time of submission. This might be iommu running out of space, for instance, or it might be a memory allocation of some sort. For these cases, we don't get any notification when the shortage clears. All we can do is ensure that we restart operations at some point in the future. We're SOL at that point, but we have to ensure that we make forward progress.

That last set of conditions better not be a a common occurence, since performance is down the toilet at that point. I don't want to introduce hot path code to rectify it. Have the driver return if that happens in a way that is DIFFERENT from needing a normal restart. The driver knows if this is a resource that will become available when IO completes on this device or not. If we get that return, we have a generic run-again delay.

This basically becomes the same as doing the delay queue thing from DM, but just in a generic fashion.

Quoted 8 lines · Jens Axboe

Quoted 6 lines · Bart Van Assche

Quoted 1 line

This is all very tiresome.

Yes, this is tiresome. It is very annoying to me that others keep introducing so many regressions in such important parts of the kernel. It is also annoying to me that I get blamed if I report a regression instead of seeing that the regression gets fixed.

Quoted 3 lines

I agree, it sucks that any change there introduces the regression. I'm fine with doing the delay insert again until a new patch is proven to be better.

That way is still buggy as I explained, since rerun queue before adding request to hctx->dispatch_list isn't correct. Who can make sure the request is visible when __blk_mq_run_hw_queue() is called?

Not mention this way will cause performance regression again.

Quoted 10 lines

From the original topic of this email, we have conditions that can cause the driver to not be able to submit an IO. A set of those conditions can only happen if IO is in flight, and those cases we have covered just fine. Another set can potentially trigger without IO being in flight. These are cases where a non-device resource is unavailable at the time of submission. This might be iommu running out of space, for instance, or it might be a memory allocation of some sort. For these cases, we don't get any notification when the shortage clears. All we can do is ensure that we restart operations at some point in the future. We're SOL at that point, but we have to ensure that we make forward progress.

Right, it is a generic issue, not DM-specific one, almost all drivers call kmalloc(GFP_ATOMIC) in IO path.

IMO, there is enough time for figuring out a generic solution before 4.16 release.

Quoted 6 lines

That last set of conditions better not be a a common occurence, since performance is down the toilet at that point. I don't want to introduce hot path code to rectify it. Have the driver return if that happens in a way that is DIFFERENT from needing a normal restart. The driver knows if this is a resource that will become available when IO completes on this device or not. If we get that return, we have a generic run-again delay.

Now most of times both NVMe and SCSI won't return BLK_STS_RESOURCE, and it should be DM-only which returns STS_RESOURCE so often.

Quoted 9 lines · Ming Lei

Quoted 8 lines · Jens Axboe

Quoted 6 lines · Bart Van Assche

Quoted 1 line

This is all very tiresome.

Yes, this is tiresome. It is very annoying to me that others keep introducing so many regressions in such important parts of the kernel. It is also annoying to me that I get blamed if I report a regression instead of seeing that the regression gets fixed.

Quoted 7 lines

Quoted 3 lines

I agree, it sucks that any change there introduces the regression. I'm fine with doing the delay insert again until a new patch is proven to be better.

That way is still buggy as I explained, since rerun queue before adding request to hctx->dispatch_list isn't correct. Who can make sure the request is visible when __blk_mq_run_hw_queue() is called?

That race basically doesn't exist for a 10ms gap.

Quoted 1 line

Not mention this way will cause performance regression again.

How so? It's _exactly_ the same as what you are proposing, except mine will potentially run the queue when it need not do so. But given that these are random 10ms queue kicks because we are screwed, it should not matter. The key point is that it only should be if we have NO better options. If it's a frequently occurring event that we have to return BLK_STS_RESOURCE, then we need to get a way to register an event for when that condition clears. That event will then kick the necessary queue(s).

Quoted 12 lines

Quoted 11 lines

From the original topic of this email, we have conditions that can cause the driver to not be able to submit an IO. A set of those conditions can only happen if IO is in flight, and those cases we have covered just fine. Another set can potentially trigger without IO being in flight. These are cases where a non-device resource is unavailable at the time of submission. This might be iommu running out of space, for instance, or it might be a memory allocation of some sort. For these cases, we don't get any notification when the shortage clears. All we can do is ensure that we restart operations at some point in the future. We're SOL at that point, but we have to ensure that we make forward progress.

Quoted 2 lines

Right, it is a generic issue, not DM-specific one, almost all drivers call kmalloc(GFP_ATOMIC) in IO path.

GFP_ATOMIC basically never fails, unless we are out of memory. The

exception is higher order allocations. If a driver has a higher order atomic allocation in its IO path, the device driver writer needs to be taken out behind the barn and shot. Simple as that. It will NEVER work well in a production environment. Witness the disaster that so many NIC driver writers have learned.

This is NOT the case we care about here. It's resources that are more readily depleted because other devices are using them. If it's a high frequency or generally occurring event, then we simply must have a callback to restart the queue from that. The condition then becomes identical to device private starvation, the only difference being from where we restart the queue.

Quoted 2 lines

IMO, there is enough time for figuring out a generic solution before 4.16 release.

I would hope so, but the proposed solutions have not filled me with a lot of confidence in the end result so far.

Quoted 7 lines

Quoted 6 lines

That last set of conditions better not be a a common occurence, since performance is down the toilet at that point. I don't want to introduce hot path code to rectify it. Have the driver return if that happens in a way that is DIFFERENT from needing a normal restart. The driver knows if this is a resource that will become available when IO completes on this device or not. If we get that return, we have a generic run-again delay.

Quoted 2 lines

Now most of times both NVMe and SCSI won't return BLK_STS_RESOURCE, and it should be DM-only which returns STS_RESOURCE so often.

Where does the dm STS_RESOURCE error usually come from - what's exact resource are we running out of?

Please see queue_delayed_work_on(), hctx->run_work is shared by all scheduling, once blk_mq_delay_run_hw_queue(100ms) returns, no new scheduling can make progress during the 100ms.

Quoted 3 lines · Ming Lei

Please see queue_delayed_work_on(), hctx->run_work is shared by all scheduling, once blk_mq_delay_run_hw_queue(100ms) returns, no new scheduling can make progress during the 100ms.

How about addressing that as follows:

block/blk-mq.c +0 -0

index f7515dd95a36..57f8379a476d 100644

Quoted 3 lines · Ming Lei

Please see queue_delayed_work_on(), hctx->run_work is shared by all scheduling, once blk_mq_delay_run_hw_queue(100ms) returns, no new scheduling can make progress during the 100ms.

How about addressing that as follows:

block/blk-mq.c +3 -3

index f7515dd95a36..57f8379a476d 100644
@@ -1403,9 +1403,9 @@ static void __blk_mq_delay_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async,
 		put_cpu();
 	}
 
-	kblockd_schedule_delayed_work_on(blk_mq_hctx_next_cpu(hctx),
-					 &hctx->run_work,
-					 msecs_to_jiffies(msecs));
+	kblockd_mod_delayed_work_on(blk_mq_hctx_next_cpu(hctx),
+				    &hctx->run_work,
+				    msecs_to_jiffies(msecs));
 }
 
 void blk_mq_delay_run_hw_queue(struct blk_mq_hw_ctx *hctx, unsigned long msecs)

Bart.

Quoted 25 lines · Bart Van Assche

Quoted 3 lines · Ming Lei

Please see queue_delayed_work_on(), hctx->run_work is shared by all scheduling, once blk_mq_delay_run_hw_queue(100ms) returns, no new scheduling can make progress during the 100ms.

How about addressing that as follows:

block/blk-mq.c +3 -3

index f7515dd95a36..57f8379a476d 100644
@@ -1403,9 +1403,9 @@ static void __blk_mq_delay_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async,
 		put_cpu();
 	}
 
-	kblockd_schedule_delayed_work_on(blk_mq_hctx_next_cpu(hctx),
-					 &hctx->run_work,
-					 msecs_to_jiffies(msecs));
+	kblockd_mod_delayed_work_on(blk_mq_hctx_next_cpu(hctx),
+				    &hctx->run_work,
+				    msecs_to_jiffies(msecs));
 }
 
 void blk_mq_delay_run_hw_queue(struct blk_mq_hw_ctx *hctx, unsigned long msecs)

Quoted 1 line

Bart.

Yes, this one together with Jen's suggestion with returning BLK_STS_NO_DEV_RESOURCE should fix this issue.

Could you cook a fix for this issue? Otherwise I am happy to do that.

Quoted 22 lines · Jens Axboe

Quoted 9 lines · Ming Lei

Quoted 8 lines · Jens Axboe

Quoted 6 lines · Bart Van Assche

Quoted 1 line

This is all very tiresome.

Yes, this is tiresome. It is very annoying to me that others keep introducing so many regressions in such important parts of the kernel. It is also annoying to me that I get blamed if I report a regression instead of seeing that the regression gets fixed.

Quoted 7 lines

Quoted 3 lines

I agree, it sucks that any change there introduces the regression. I'm fine with doing the delay insert again until a new patch is proven to be better.

That way is still buggy as I explained, since rerun queue before adding request to hctx->dispatch_list isn't correct. Who can make sure the request is visible when __blk_mq_run_hw_queue() is called?

That race basically doesn't exist for a 10ms gap.

Quoted 1 line

Not mention this way will cause performance regression again.

Quoted 8 lines

How so? It's _exactly_ the same as what you are proposing, except mine will potentially run the queue when it need not do so. But given that these are random 10ms queue kicks because we are screwed, it should not matter. The key point is that it only should be if we have NO better options. If it's a frequently occurring event that we have to return BLK_STS_RESOURCE, then we need to get a way to register an event for when that condition clears. That event will then kick the necessary queue(s).

Please see queue_delayed_work_on(), hctx->run_work is shared by all scheduling, once blk_mq_delay_run_hw_queue(100ms) returns, no new scheduling can make progress during the 100ms.

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

From the original topic of this email, we have conditions that can cause the driver to not be able to submit an IO. A set of those conditions can only happen if IO is in flight, and those cases we have covered just fine. Another set can potentially trigger without IO being in flight. These are cases where a non-device resource is unavailable at the time of submission. This might be iommu running out of space, for instance, or it might be a memory allocation of some sort. For these cases, we don't get any notification when the shortage clears. All we can do is ensure that we restart operations at some point in the future. We're SOL at that point, but we have to ensure that we make forward progress.

Quoted 4 lines

Quoted 2 lines

Right, it is a generic issue, not DM-specific one, almost all drivers call kmalloc(GFP_ATOMIC) in IO path.

GFP_ATOMIC basically never fails, unless we are out of memory. The

I guess GFP_KERNEL may never fail, but GFP_ATOMIC failure might be possible, and it is mentioned[1] there is such code in mm allocation path, also OOM can happen too.

if (some randomly generated condition) && (request is atomic)

      return NULL;

[1] https://lwn.net/Articles/276731/

Quoted 26 lines

exception is higher order allocations. If a driver has a higher order atomic allocation in its IO path, the device driver writer needs to be taken out behind the barn and shot. Simple as that. It will NEVER work well in a production environment. Witness the disaster that so many NIC driver writers have learned.

This is NOT the case we care about here. It's resources that are more readily depleted because other devices are using them. If it's a high frequency or generally occurring event, then we simply must have a callback to restart the queue from that. The condition then becomes identical to device private starvation, the only difference being from where we restart the queue.

Quoted 2 lines

IMO, there is enough time for figuring out a generic solution before 4.16 release.

I would hope so, but the proposed solutions have not filled me with a lot of confidence in the end result so far.

Quoted 7 lines

Quoted 6 lines

That last set of conditions better not be a a common occurence, since performance is down the toilet at that point. I don't want to introduce hot path code to rectify it. Have the driver return if that happens in a way that is DIFFERENT from needing a normal restart. The driver knows if this is a resource that will become available when IO completes on this device or not. If we get that return, we have a generic run-again delay.

Quoted 5 lines

Quoted 2 lines

Now most of times both NVMe and SCSI won't return BLK_STS_RESOURCE, and it should be DM-only which returns STS_RESOURCE so often.

Where does the dm STS_RESOURCE error usually come from - what's exact resource are we running out of?

It is from blk_get_request(underlying queue), see
multipath_clone_and_map().

Quoted 32 lines · Ming Lei

Quoted 22 lines · Jens Axboe

Quoted 9 lines · Ming Lei

Quoted 8 lines · Jens Axboe

Quoted 6 lines · Bart Van Assche

Quoted 1 line

This is all very tiresome.

Yes, this is tiresome. It is very annoying to me that others keep introducing so many regressions in such important parts of the kernel. It is also annoying to me that I get blamed if I report a regression instead of seeing that the regression gets fixed.

Quoted 7 lines

Quoted 3 lines

I agree, it sucks that any change there introduces the regression. I'm fine with doing the delay insert again until a new patch is proven to be better.

That way is still buggy as I explained, since rerun queue before adding request to hctx->dispatch_list isn't correct. Who can make sure the request is visible when __blk_mq_run_hw_queue() is called?

That race basically doesn't exist for a 10ms gap.

Quoted 1 line

Not mention this way will cause performance regression again.

Quoted 8 lines

How so? It's _exactly_ the same as what you are proposing, except mine will potentially run the queue when it need not do so. But given that these are random 10ms queue kicks because we are screwed, it should not matter. The key point is that it only should be if we have NO better options. If it's a frequently occurring event that we have to return BLK_STS_RESOURCE, then we need to get a way to register an event for when that condition clears. That event will then kick the necessary queue(s).

Quoted 3 lines

Please see queue_delayed_work_on(), hctx->run_work is shared by all scheduling, once blk_mq_delay_run_hw_queue(100ms) returns, no new scheduling can make progress during the 100ms.

That's a bug, plain and simple. If someone does "run this queue in 100ms" and someone else comes in and says "run this queue now", the correct outcome is running this queue now.

Quoted 16 lines

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

From the original topic of this email, we have conditions that can cause the driver to not be able to submit an IO. A set of those conditions can only happen if IO is in flight, and those cases we have covered just fine. Another set can potentially trigger without IO being in flight. These are cases where a non-device resource is unavailable at the time of submission. This might be iommu running out of space, for instance, or it might be a memory allocation of some sort. For these cases, we don't get any notification when the shortage clears. All we can do is ensure that we restart operations at some point in the future. We're SOL at that point, but we have to ensure that we make forward progress.

Quoted 4 lines

Quoted 2 lines

Right, it is a generic issue, not DM-specific one, almost all drivers call kmalloc(GFP_ATOMIC) in IO path.

GFP_ATOMIC basically never fails, unless we are out of memory. The

Quoted 8 lines

I guess GFP_KERNEL may never fail, but GFP_ATOMIC failure might be possible, and it is mentioned[1] there is such code in mm allocation path, also OOM can happen too.

if (some randomly generated condition) && (request is atomic)

      return NULL;

[1] https://lwn.net/Articles/276731/

That article is 10 years old. Once you run large scale production, you see what the real failures are. Fact is, for zero order allocation, if the atomic alloc fails the shit has really hit the fan. In that case, a delay of 10ms is not your main issue. It's a total red herring when you compare to the frequency of what Bart is seeing. It's noise, and irrelevant here. For an atomic zero order allocation failure, doing a short random sleep is perfectly fine.

Quoted 26 lines

Quoted 26 lines

exception is higher order allocations. If a driver has a higher order atomic allocation in its IO path, the device driver writer needs to be taken out behind the barn and shot. Simple as that. It will NEVER work well in a production environment. Witness the disaster that so many NIC driver writers have learned.

This is NOT the case we care about here. It's resources that are more readily depleted because other devices are using them. If it's a high frequency or generally occurring event, then we simply must have a callback to restart the queue from that. The condition then becomes identical to device private starvation, the only difference being from where we restart the queue.

Quoted 2 lines

IMO, there is enough time for figuring out a generic solution before 4.16 release.

I would hope so, but the proposed solutions have not filled me with a lot of confidence in the end result so far.

Quoted 7 lines

Quoted 6 lines

That last set of conditions better not be a a common occurence, since performance is down the toilet at that point. I don't want to introduce hot path code to rectify it. Have the driver return if that happens in a way that is DIFFERENT from needing a normal restart. The driver knows if this is a resource that will become available when IO completes on this device or not. If we get that return, we have a generic run-again delay.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Now most of times both NVMe and SCSI won't return BLK_STS_RESOURCE, and it should be DM-only which returns STS_RESOURCE so often.

Where does the dm STS_RESOURCE error usually come from - what's exact resource are we running out of?

It is from blk_get_request(underlying queue), see
multipath_clone_and_map().

That's what I thought. So for a low queue depth underlying queue, it's quite possible that this situation can happen. Two potential solutions I see:

1) As described earlier in this thread, having a mechanism for being notified when the scarce resource becomes available. It would not be hard to tap into the existing sbitmap wait queue for that.

2) Have dm set BLK_MQ_F_BLOCKING and just sleep on the resource allocation. I haven't read the dm code to know if this is a possibility or not.

I'd probably prefer #1. It's a classic case of trying to get the request, and if it fails, add ourselves to the sbitmap tag wait queue head, retry, and bail if that also fails. Connecting the scarce resource and the consumer is the only way to really fix this, without bogus arbitrary delays.

Quoted 18 lines

Quoted 5 lines

Quoted 2 lines

Where does the dm STS_RESOURCE error usually come from - what's exact resource are we running out of?

It is from blk_get_request(underlying queue), see
multipath_clone_and_map().

That's what I thought. So for a low queue depth underlying queue, it's quite possible that this situation can happen. Two potential solutions I see:

1) As described earlier in this thread, having a mechanism for being notified when the scarce resource becomes available. It would not be hard to tap into the existing sbitmap wait queue for that.

2) Have dm set BLK_MQ_F_BLOCKING and just sleep on the resource allocation. I haven't read the dm code to know if this is a possibility or not.

Quoted 5 lines

I'd probably prefer #1. It's a classic case of trying to get the request, and if it fails, add ourselves to the sbitmap tag wait queue head, retry, and bail if that also fails. Connecting the scarce resource and the consumer is the only way to really fix this, without bogus arbitrary delays.

Right, as I have replied to Bart, using mod_delayed_work_on() with returning BLK_STS_NO_DEV_RESOURCE(or sort of name) for the scarce resource should fix this issue.

Quoted 24 lines · Ming Lei

Quoted 18 lines

Quoted 5 lines

Quoted 2 lines

Where does the dm STS_RESOURCE error usually come from - what's exact resource are we running out of?

It is from blk_get_request(underlying queue), see
multipath_clone_and_map().

That's what I thought. So for a low queue depth underlying queue, it's quite possible that this situation can happen. Two potential solutions I see:

1) As described earlier in this thread, having a mechanism for being notified when the scarce resource becomes available. It would not be hard to tap into the existing sbitmap wait queue for that.

2) Have dm set BLK_MQ_F_BLOCKING and just sleep on the resource allocation. I haven't read the dm code to know if this is a possibility or not.

Quoted 5 lines

I'd probably prefer #1. It's a classic case of trying to get the request, and if it fails, add ourselves to the sbitmap tag wait queue head, retry, and bail if that also fails. Connecting the scarce resource and the consumer is the only way to really fix this, without bogus arbitrary delays.

Quoted 3 lines

Right, as I have replied to Bart, using mod_delayed_work_on() with returning BLK_STS_NO_DEV_RESOURCE(or sort of name) for the scarce resource should fix this issue.

It'll fix the forever stall, but it won't really fix it, as we'll slow down the dm device by some random amount.

A simple test case would be to have a null_blk device with a queue depth of one, and dm on top of that. Start a fio job that runs two jobs: one that does IO to the underlying device, and one that does IO to the dm device. If the job on the dm device runs substantially slower than the one to the underlying device, then the problem isn't really fixed.

Quoted 29 lines · Jens Axboe

Quoted 24 lines · Ming Lei

Quoted 18 lines

Quoted 5 lines

Quoted 2 lines

Where does the dm STS_RESOURCE error usually come from - what's exact resource are we running out of?

It is from blk_get_request(underlying queue), see
multipath_clone_and_map().

That's what I thought. So for a low queue depth underlying queue, it's quite possible that this situation can happen. Two potential solutions I see:

1) As described earlier in this thread, having a mechanism for being notified when the scarce resource becomes available. It would not be hard to tap into the existing sbitmap wait queue for that.

2) Have dm set BLK_MQ_F_BLOCKING and just sleep on the resource allocation. I haven't read the dm code to know if this is a possibility or not.

Quoted 5 lines

I'd probably prefer #1. It's a classic case of trying to get the request, and if it fails, add ourselves to the sbitmap tag wait queue head, retry, and bail if that also fails. Connecting the scarce resource and the consumer is the only way to really fix this, without bogus arbitrary delays.

Quoted 3 lines

Right, as I have replied to Bart, using mod_delayed_work_on() with returning BLK_STS_NO_DEV_RESOURCE(or sort of name) for the scarce resource should fix this issue.

Quoted 8 lines

It'll fix the forever stall, but it won't really fix it, as we'll slow down the dm device by some random amount.

A simple test case would be to have a null_blk device with a queue depth of one, and dm on top of that. Start a fio job that runs two jobs: one that does IO to the underlying device, and one that does IO to the dm device. If the job on the dm device runs substantially slower than the one to the underlying device, then the problem isn't really fixed.

I remembered that I tried this test on scsi-debug & dm-mpath over scsi-debug, seems not observed this issue, could you explain a bit why IO over dm-mpath may be slower? Because both two IO contexts call same get_request(), and in theory dm-mpath should be a bit quicker since it uses direct issue for underlying queue, without io scheduler involved.

Quoted 39 lines · Ming Lei

Quoted 37 lines · Jens Axboe

Quoted 24 lines · Ming Lei

Quoted 18 lines

Quoted 5 lines

Quoted 2 lines

Where does the dm STS_RESOURCE error usually come from - what's exact resource are we running out of?

It is from blk_get_request(underlying queue), see
multipath_clone_and_map().

That's what I thought. So for a low queue depth underlying queue, it's quite possible that this situation can happen. Two potential solutions I see:

1) As described earlier in this thread, having a mechanism for being notified when the scarce resource becomes available. It would not be hard to tap into the existing sbitmap wait queue for that.

2) Have dm set BLK_MQ_F_BLOCKING and just sleep on the resource allocation. I haven't read the dm code to know if this is a possibility or not.

Quoted 5 lines

I'd probably prefer #1. It's a classic case of trying to get the request, and if it fails, add ourselves to the sbitmap tag wait queue head, retry, and bail if that also fails. Connecting the scarce resource and the consumer is the only way to really fix this, without bogus arbitrary delays.

Quoted 3 lines

Right, as I have replied to Bart, using mod_delayed_work_on() with returning BLK_STS_NO_DEV_RESOURCE(or sort of name) for the scarce resource should fix this issue.

It'll fix the forever stall, but it won't really fix it, as we'll slow down the dm device by some random amount.

A simple test case would be to have a null_blk device with a queue depth of one, and dm on top of that. Start a fio job that runs two jobs: one that does IO to the underlying device, and one that does IO to the dm device. If the job on the dm device runs substantially slower than the one to the underlying device, then the problem isn't really fixed.

Quoted 5 lines

I remembered that I tried this test on scsi-debug & dm-mpath over scsi-debug, seems not observed this issue, could you explain a bit why IO over dm-mpath may be slower? Because both two IO contexts call same get_request(), and in theory dm-mpath should be a bit quicker since it uses direct issue for underlying queue, without io scheduler involved.

Because if you lose the race for getting the request, you'll have some arbitrary delay before trying again, potentially. Compared to the direct

Quoted 46 lines · Jens Axboe

Quoted 44 lines · Ming Lei

Quoted 29 lines · Jens Axboe

Quoted 24 lines · Ming Lei

Quoted 18 lines

Quoted 5 lines

Quoted 2 lines

Where does the dm STS_RESOURCE error usually come from - what's exact resource are we running out of?

It is from blk_get_request(underlying queue), see
multipath_clone_and_map().

That's what I thought. So for a low queue depth underlying queue, it's quite possible that this situation can happen. Two potential solutions I see:

1) As described earlier in this thread, having a mechanism for being notified when the scarce resource becomes available. It would not be hard to tap into the existing sbitmap wait queue for that.

2) Have dm set BLK_MQ_F_BLOCKING and just sleep on the resource allocation. I haven't read the dm code to know if this is a possibility or not.

Quoted 5 lines

I'd probably prefer #1. It's a classic case of trying to get the request, and if it fails, add ourselves to the sbitmap tag wait queue head, retry, and bail if that also fails. Connecting the scarce resource and the consumer is the only way to really fix this, without bogus arbitrary delays.

Quoted 3 lines

Right, as I have replied to Bart, using mod_delayed_work_on() with returning BLK_STS_NO_DEV_RESOURCE(or sort of name) for the scarce resource should fix this issue.

Quoted 8 lines

It'll fix the forever stall, but it won't really fix it, as we'll slow down the dm device by some random amount.

A simple test case would be to have a null_blk device with a queue depth of one, and dm on top of that. Start a fio job that runs two jobs: one that does IO to the underlying device, and one that does IO to the dm device. If the job on the dm device runs substantially slower than the one to the underlying device, then the problem isn't really fixed.

I remembered that I tried this test on scsi-debug & dm-mpath over scsi-debug, seems not observed this issue, could you explain a bit why IO over dm-mpath may be slower? Because both two IO contexts call same get_request(), and in theory dm-mpath should be a bit quicker since it uses direct issue for underlying queue, without io scheduler involved.

Quoted 2 lines

Because if you lose the race for getting the request, you'll have some arbitrary delay before trying again, potentially. Compared to the direct

But the restart still works, one request is completed, then the queue is return immediately because we use mod_delayed_work_on(0), so looks no such issue.

Quoted 50 lines · Ming Lei

Quoted 48 lines · Jens Axboe

Quoted 39 lines · Ming Lei

Quoted 29 lines · Jens Axboe

Quoted 24 lines · Ming Lei

Quoted 18 lines

Quoted 5 lines

Quoted 2 lines

Where does the dm STS_RESOURCE error usually come from - what's exact resource are we running out of?

It is from blk_get_request(underlying queue), see
multipath_clone_and_map().

That's what I thought. So for a low queue depth underlying queue, it's quite possible that this situation can happen. Two potential solutions I see:

1) As described earlier in this thread, having a mechanism for being notified when the scarce resource becomes available. It would not be hard to tap into the existing sbitmap wait queue for that.

2) Have dm set BLK_MQ_F_BLOCKING and just sleep on the resource allocation. I haven't read the dm code to know if this is a possibility or not.

Quoted 5 lines

I'd probably prefer #1. It's a classic case of trying to get the request, and if it fails, add ourselves to the sbitmap tag wait queue head, retry, and bail if that also fails. Connecting the scarce resource and the consumer is the only way to really fix this, without bogus arbitrary delays.

Quoted 3 lines

Right, as I have replied to Bart, using mod_delayed_work_on() with returning BLK_STS_NO_DEV_RESOURCE(or sort of name) for the scarce resource should fix this issue.

Quoted 8 lines

It'll fix the forever stall, but it won't really fix it, as we'll slow down the dm device by some random amount.

A simple test case would be to have a null_blk device with a queue depth of one, and dm on top of that. Start a fio job that runs two jobs: one that does IO to the underlying device, and one that does IO to the dm device. If the job on the dm device runs substantially slower than the one to the underlying device, then the problem isn't really fixed.

Quoted 5 lines

I remembered that I tried this test on scsi-debug & dm-mpath over scsi-debug, seems not observed this issue, could you explain a bit why IO over dm-mpath may be slower? Because both two IO contexts call same get_request(), and in theory dm-mpath should be a bit quicker since it uses direct issue for underlying queue, without io scheduler involved.

Because if you lose the race for getting the request, you'll have some arbitrary delay before trying again, potentially. Compared to the direct

Quoted 3 lines

But the restart still works, one request is completed, then the queue is return immediately because we use mod_delayed_work_on(0), so looks no such issue.

There are no pending requests for this case, nothing to restart the queue. When you fail that blk_get_request(), you are idle, nothing is pending.

Quoted 55 lines · Jens Axboe

Quoted 53 lines · Ming Lei

Quoted 40 lines · Jens Axboe

Quoted 39 lines · Ming Lei

Quoted 37 lines · Jens Axboe

Quoted 24 lines · Ming Lei

Quoted 18 lines

Quoted 5 lines

Quoted 2 lines

Where does the dm STS_RESOURCE error usually come from - what's exact resource are we running out of?

It is from blk_get_request(underlying queue), see
multipath_clone_and_map().

That's what I thought. So for a low queue depth underlying queue, it's quite possible that this situation can happen. Two potential solutions I see:

1) As described earlier in this thread, having a mechanism for being notified when the scarce resource becomes available. It would not be hard to tap into the existing sbitmap wait queue for that.

2) Have dm set BLK_MQ_F_BLOCKING and just sleep on the resource allocation. I haven't read the dm code to know if this is a possibility or not.

Quoted 5 lines

I'd probably prefer #1. It's a classic case of trying to get the request, and if it fails, add ourselves to the sbitmap tag wait queue head, retry, and bail if that also fails. Connecting the scarce resource and the consumer is the only way to really fix this, without bogus arbitrary delays.

Quoted 3 lines

Right, as I have replied to Bart, using mod_delayed_work_on() with returning BLK_STS_NO_DEV_RESOURCE(or sort of name) for the scarce resource should fix this issue.

It'll fix the forever stall, but it won't really fix it, as we'll slow down the dm device by some random amount.

A simple test case would be to have a null_blk device with a queue depth of one, and dm on top of that. Start a fio job that runs two jobs: one that does IO to the underlying device, and one that does IO to the dm device. If the job on the dm device runs substantially slower than the one to the underlying device, then the problem isn't really fixed.

Quoted 8 lines

Quoted 5 lines

I remembered that I tried this test on scsi-debug & dm-mpath over scsi-debug, seems not observed this issue, could you explain a bit why IO over dm-mpath may be slower? Because both two IO contexts call same get_request(), and in theory dm-mpath should be a bit quicker since it uses direct issue for underlying queue, without io scheduler involved.

Because if you lose the race for getting the request, you'll have some arbitrary delay before trying again, potentially. Compared to the direct

But the restart still works, one request is completed, then the queue is return immediately because we use mod_delayed_work_on(0), so looks no such issue.

Quoted 3 lines

There are no pending requests for this case, nothing to restart the queue. When you fail that blk_get_request(), you are idle, nothing is pending.

I think we needn't worry about that, once a device is attached to dm-rq, it can't be mounted any more, and usually user don't use the device directly and by dm-mpath at the same time.

Quoted 11 lines · Ming Lei

Quoted 6 lines · Jens Axboe

Quoted 1 line · Ming Lei

On Fri, Jan 19, 2018 at 09:19:24AM -0700, Jens Axboe wrote:

There are no pending requests for this case, nothing to restart the queue. When you fail that blk_get_request(), you are idle, nothing is pending.

I think we needn't worry about that, once a device is attached to dm-rq, it can't be mounted any more, and usually user don't use the device directly and by dm-mpath at the same time.

Even if it doesn't happen for a normal dm setup, it is a case that needs to be handled. The request allocation is just one example of a wider scope resource that can be unavailable. If the driver returns NO_DEV_RESOURCE (or whatever name), it will be a possibility that the device itself is currently idle.

Quoted 13 lines · Jens Axboe

Quoted 11 lines · Ming Lei

Quoted 6 lines · Jens Axboe

Quoted 1 line · Ming Lei

On Fri, Jan 19, 2018 at 09:19:24AM -0700, Jens Axboe wrote:

There are no pending requests for this case, nothing to restart the queue. When you fail that blk_get_request(), you are idle, nothing is pending.

I think we needn't worry about that, once a device is attached to dm-rq, it can't be mounted any more, and usually user don't use the device directly and by dm-mpath at the same time.

Quoted 5 lines

Even if it doesn't happen for a normal dm setup, it is a case that needs to be handled. The request allocation is just one example of a wider scope resource that can be unavailable. If the driver returns NO_DEV_RESOURCE (or whatever name), it will be a possibility that the device itself is currently idle.

How would a driver's resources be exhausted yet the device is idle (so as not to be able to benefit from RESTART)?

Quoted 16 lines · Mike Snitzer

Quoted 13 lines · Jens Axboe

Quoted 11 lines · Ming Lei

Quoted 6 lines · Jens Axboe

Quoted 1 line · Ming Lei

On Fri, Jan 19, 2018 at 09:19:24AM -0700, Jens Axboe wrote:

There are no pending requests for this case, nothing to restart the queue. When you fail that blk_get_request(), you are idle, nothing is pending.

I think we needn't worry about that, once a device is attached to dm-rq, it can't be mounted any more, and usually user don't use the device directly and by dm-mpath at the same time.

Quoted 8 lines

Quoted 5 lines

Even if it doesn't happen for a normal dm setup, it is a case that needs to be handled. The request allocation is just one example of a wider scope resource that can be unavailable. If the driver returns NO_DEV_RESOURCE (or whatever name), it will be a possibility that the device itself is currently idle.

How would a driver's resources be exhausted yet the device is idle (so as not to be able to benefit from RESTART)?

I've outlined a number of these examples already. Another case might be:

1) Device is idle
2) Device gets request
3) Device attempts to DMA map
4) DMA map fails because the IOMMU is out of space (nic is using it all)
5) Device returns STS_RESOURCE
6) Queue is marked as needing a restart

All's well, except there is no IO on this device that will notice the restart bit and retry the operation.

Replace IOMMU failure with any other resource that the driver might need for an IO, which isn't tied to a device specific resource. blk_get_request() on dm is an example, as is any allocation failure occurring in the queue IO path for the driver.

Quoted 35 lines · Jens Axboe

Quoted 16 lines · Mike Snitzer

Quoted 13 lines · Jens Axboe

Quoted 11 lines · Ming Lei

Quoted 6 lines · Jens Axboe

Quoted 1 line · Ming Lei

On Fri, Jan 19, 2018 at 09:19:24AM -0700, Jens Axboe wrote:

There are no pending requests for this case, nothing to restart the queue. When you fail that blk_get_request(), you are idle, nothing is pending.

I think we needn't worry about that, once a device is attached to dm-rq, it can't be mounted any more, and usually user don't use the device directly and by dm-mpath at the same time.

Quoted 8 lines

Quoted 5 lines

Even if it doesn't happen for a normal dm setup, it is a case that needs to be handled. The request allocation is just one example of a wider scope resource that can be unavailable. If the driver returns NO_DEV_RESOURCE (or whatever name), it will be a possibility that the device itself is currently idle.

How would a driver's resources be exhausted yet the device is idle (so as not to be able to benefit from RESTART)?

I've outlined a number of these examples already. Another case might be:

1) Device is idle
2) Device gets request
3) Device attempts to DMA map
4) DMA map fails because the IOMMU is out of space (nic is using it all)
5) Device returns STS_RESOURCE
6) Queue is marked as needing a restart

Quoted 7 lines

All's well, except there is no IO on this device that will notice the restart bit and retry the operation.

Replace IOMMU failure with any other resource that the driver might need for an IO, which isn't tied to a device specific resource. blk_get_request() on dm is an example, as is any allocation failure occurring in the queue IO path for the driver.

Yeah, if we decide to take the approach of introducing NO_DEV_RESOURCE, all the current STS_RESOURCE for non-device resource allocation(kmalloc, dma map, get_request, ...) should be converted to NO_DEV_RESOURCE.

And it is a generic issue, which need generic solution.

Seems running queue after arbitrary in this case is the only way we thought of, or other solutions?

If the decision is made, let's make/review the patch, :-)

Quoted 39 lines · Ming Lei

Quoted 38 lines · Jens Axboe

Quoted 16 lines · Mike Snitzer

Quoted 13 lines · Jens Axboe

Quoted 11 lines · Ming Lei

Quoted 6 lines · Jens Axboe

Quoted 1 line · Ming Lei

On Fri, Jan 19, 2018 at 09:19:24AM -0700, Jens Axboe wrote:

There are no pending requests for this case, nothing to restart the queue. When you fail that blk_get_request(), you are idle, nothing is pending.

I think we needn't worry about that, once a device is attached to dm-rq, it can't be mounted any more, and usually user don't use the device directly and by dm-mpath at the same time.

Quoted 8 lines

Quoted 5 lines

Even if it doesn't happen for a normal dm setup, it is a case that needs to be handled. The request allocation is just one example of a wider scope resource that can be unavailable. If the driver returns NO_DEV_RESOURCE (or whatever name), it will be a possibility that the device itself is currently idle.

How would a driver's resources be exhausted yet the device is idle (so as not to be able to benefit from RESTART)?

I've outlined a number of these examples already. Another case might be:

1) Device is idle
2) Device gets request
3) Device attempts to DMA map
4) DMA map fails because the IOMMU is out of space (nic is using it all)
5) Device returns STS_RESOURCE
6) Queue is marked as needing a restart

All's well, except there is no IO on this device that will notice the restart bit and retry the operation.

Quoted 8 lines

Quoted 4 lines

Replace IOMMU failure with any other resource that the driver might need for an IO, which isn't tied to a device specific resource. blk_get_request() on dm is an example, as is any allocation failure occurring in the queue IO path for the driver.

Yeah, if we decide to take the approach of introducing NO_DEV_RESOURCE, all the current STS_RESOURCE for non-device resource allocation(kmalloc, dma map, get_request, ...) should be converted to NO_DEV_RESOURCE.

Quoted 55 lines · Jens Axboe

Quoted 53 lines · Ming Lei

Quoted 40 lines · Jens Axboe

Quoted 39 lines · Ming Lei

Quoted 37 lines · Jens Axboe

Quoted 24 lines · Ming Lei

Quoted 18 lines

Quoted 5 lines

Quoted 2 lines

Where does the dm STS_RESOURCE error usually come from - what's exact resource are we running out of?

It is from blk_get_request(underlying queue), see
multipath_clone_and_map().

That's what I thought. So for a low queue depth underlying queue, it's quite possible that this situation can happen. Two potential solutions I see:

1) As described earlier in this thread, having a mechanism for being notified when the scarce resource becomes available. It would not be hard to tap into the existing sbitmap wait queue for that.

2) Have dm set BLK_MQ_F_BLOCKING and just sleep on the resource allocation. I haven't read the dm code to know if this is a possibility or not.

Quoted 5 lines

I'd probably prefer #1. It's a classic case of trying to get the request, and if it fails, add ourselves to the sbitmap tag wait queue head, retry, and bail if that also fails. Connecting the scarce resource and the consumer is the only way to really fix this, without bogus arbitrary delays.

Quoted 3 lines

Right, as I have replied to Bart, using mod_delayed_work_on() with returning BLK_STS_NO_DEV_RESOURCE(or sort of name) for the scarce resource should fix this issue.

It'll fix the forever stall, but it won't really fix it, as we'll slow down the dm device by some random amount.

A simple test case would be to have a null_blk device with a queue depth of one, and dm on top of that. Start a fio job that runs two jobs: one that does IO to the underlying device, and one that does IO to the dm device. If the job on the dm device runs substantially slower than the one to the underlying device, then the problem isn't really fixed.

Quoted 8 lines

Quoted 5 lines

I remembered that I tried this test on scsi-debug & dm-mpath over scsi-debug, seems not observed this issue, could you explain a bit why IO over dm-mpath may be slower? Because both two IO contexts call same get_request(), and in theory dm-mpath should be a bit quicker since it uses direct issue for underlying queue, without io scheduler involved.

Because if you lose the race for getting the request, you'll have some arbitrary delay before trying again, potentially. Compared to the direct

But the restart still works, one request is completed, then the queue is return immediately because we use mod_delayed_work_on(0), so looks no such issue.

Quoted 3 lines

There are no pending requests for this case, nothing to restart the queue. When you fail that blk_get_request(), you are idle, nothing is pending.

I think we needn't worry about that, once a device is attached to dm-rq, it can't be mounted any more, and usually user don't use the device directly and by dm-mpath at the same time.

Quoted 56 lines · Ming Lei

Quoted 55 lines · Jens Axboe

Quoted 53 lines · Ming Lei

Quoted 40 lines · Jens Axboe

Quoted 39 lines · Ming Lei

Quoted 37 lines · Jens Axboe

Quoted 24 lines · Ming Lei

Quoted 18 lines

Quoted 5 lines

Quoted 2 lines

Where does the dm STS_RESOURCE error usually come from - what's exact resource are we running out of?

It is from blk_get_request(underlying queue), see
multipath_clone_and_map().

That's what I thought. So for a low queue depth underlying queue, it's quite possible that this situation can happen. Two potential solutions I see:

1) As described earlier in this thread, having a mechanism for being notified when the scarce resource becomes available. It would not be hard to tap into the existing sbitmap wait queue for that.

2) Have dm set BLK_MQ_F_BLOCKING and just sleep on the resource allocation. I haven't read the dm code to know if this is a possibility or not.

Quoted 5 lines

I'd probably prefer #1. It's a classic case of trying to get the request, and if it fails, add ourselves to the sbitmap tag wait queue head, retry, and bail if that also fails. Connecting the scarce resource and the consumer is the only way to really fix this, without bogus arbitrary delays.

Quoted 3 lines

Right, as I have replied to Bart, using mod_delayed_work_on() with returning BLK_STS_NO_DEV_RESOURCE(or sort of name) for the scarce resource should fix this issue.

It'll fix the forever stall, but it won't really fix it, as we'll slow down the dm device by some random amount.

A simple test case would be to have a null_blk device with a queue depth of one, and dm on top of that. Start a fio job that runs two jobs: one that does IO to the underlying device, and one that does IO to the dm device. If the job on the dm device runs substantially slower than the one to the underlying device, then the problem isn't really fixed.

Quoted 8 lines

Quoted 5 lines

I remembered that I tried this test on scsi-debug & dm-mpath over scsi-debug, seems not observed this issue, could you explain a bit why IO over dm-mpath may be slower? Because both two IO contexts call same get_request(), and in theory dm-mpath should be a bit quicker since it uses direct issue for underlying queue, without io scheduler involved.

Because if you lose the race for getting the request, you'll have some arbitrary delay before trying again, potentially. Compared to the direct

But the restart still works, one request is completed, then the queue is return immediately because we use mod_delayed_work_on(0), so looks no such issue.

Quoted 7 lines

Quoted 3 lines

There are no pending requests for this case, nothing to restart the queue. When you fail that blk_get_request(), you are idle, nothing is pending.

I think we needn't worry about that, once a device is attached to dm-rq, it can't be mounted any more, and usually user don't use the device directly and by dm-mpath at the same time.

Here's an example of that, using my current block tree (merged into master). The setup is dm-mpath on top of null_blk, the latter having just a single request. Both are mq devices.

Fio direct 4k random reads on dm_mq: ~250K iops

Start dd on underlying device (or partition on same device), just doing sequential reads.

Fio direct 4k random reads on dm_mq with dd running: 9 iops

No schedulers involved.

https://i.imgur.com/WTDnnwE.gif

Quoted 9 lines · Ming Lei

Quoted 4 lines · Jens Axboe

There are no pending requests for this case, nothing to restart the queue. When you fail that blk_get_request(), you are idle, nothing is pending.

I think we needn't worry about that, once a device is attached to dm-rq, it can't be mounted any more, and usually user don't use the device directly and by dm-mpath at the same time.

Here's an example of that, using my current block tree (merged into master). The setup is dm-mpath on top of null_blk, the latter having just a single request. Both are mq devices.

Fio direct 4k random reads on dm_mq: ~250K iops

Start dd on underlying device (or partition on same device), just doing sequential reads.

Fio direct 4k random reads on dm_mq with dd running: 9 iops

No schedulers involved.

https://i.imgur.com/WTDnnwE.gif

Quoted 56 lines · Ming Lei

Quoted 55 lines · Jens Axboe

Quoted 53 lines · Ming Lei

Quoted 40 lines · Jens Axboe

Quoted 39 lines · Ming Lei

Quoted 37 lines · Jens Axboe

Quoted 24 lines · Ming Lei

Quoted 18 lines

Quoted 5 lines

Quoted 2 lines

Where does the dm STS_RESOURCE error usually come from - what's exact resource are we running out of?

It is from blk_get_request(underlying queue), see
multipath_clone_and_map().

That's what I thought. So for a low queue depth underlying queue, it's quite possible that this situation can happen. Two potential solutions I see:

1) As described earlier in this thread, having a mechanism for being notified when the scarce resource becomes available. It would not be hard to tap into the existing sbitmap wait queue for that.

2) Have dm set BLK_MQ_F_BLOCKING and just sleep on the resource allocation. I haven't read the dm code to know if this is a possibility or not.

Quoted 5 lines

I'd probably prefer #1. It's a classic case of trying to get the request, and if it fails, add ourselves to the sbitmap tag wait queue head, retry, and bail if that also fails. Connecting the scarce resource and the consumer is the only way to really fix this, without bogus arbitrary delays.

Quoted 3 lines

Right, as I have replied to Bart, using mod_delayed_work_on() with returning BLK_STS_NO_DEV_RESOURCE(or sort of name) for the scarce resource should fix this issue.

It'll fix the forever stall, but it won't really fix it, as we'll slow down the dm device by some random amount.

A simple test case would be to have a null_blk device with a queue depth of one, and dm on top of that. Start a fio job that runs two jobs: one that does IO to the underlying device, and one that does IO to the dm device. If the job on the dm device runs substantially slower than the one to the underlying device, then the problem isn't really fixed.

Quoted 8 lines

Quoted 5 lines

I remembered that I tried this test on scsi-debug & dm-mpath over scsi-debug, seems not observed this issue, could you explain a bit why IO over dm-mpath may be slower? Because both two IO contexts call same get_request(), and in theory dm-mpath should be a bit quicker since it uses direct issue for underlying queue, without io scheduler involved.

Because if you lose the race for getting the request, you'll have some arbitrary delay before trying again, potentially. Compared to the direct

But the restart still works, one request is completed, then the queue is return immediately because we use mod_delayed_work_on(0), so looks no such issue.

Quoted 7 lines

Quoted 3 lines

There are no pending requests for this case, nothing to restart the queue. When you fail that blk_get_request(), you are idle, nothing is pending.

I think we needn't worry about that, once a device is attached to dm-rq, it can't be mounted any more, and usually user don't use the device directly and by dm-mpath at the same time.

Here's an example of that, using my current block tree (merged into master). The setup is dm-mpath on top of null_blk, the latter having just a single request. Both are mq devices.

Fio direct 4k random reads on dm_mq: ~250K iops

Start dd on underlying device (or partition on same device), just doing sequential reads.

Fio direct 4k random reads on dm_mq with dd running: 9 iops

No schedulers involved.

https://i.imgur.com/WTDnnwE.gif

Quoted 76 lines · Jens Axboe

Quoted 56 lines · Ming Lei

Quoted 55 lines · Jens Axboe

Quoted 53 lines · Ming Lei

Quoted 40 lines · Jens Axboe

Quoted 39 lines · Ming Lei

Quoted 37 lines · Jens Axboe

Quoted 24 lines · Ming Lei

Quoted 18 lines

Quoted 5 lines

Quoted 2 lines

Where does the dm STS_RESOURCE error usually come from - what's exact resource are we running out of?

It is from blk_get_request(underlying queue), see
multipath_clone_and_map().

That's what I thought. So for a low queue depth underlying queue, it's quite possible that this situation can happen. Two potential solutions I see:

1) As described earlier in this thread, having a mechanism for being notified when the scarce resource becomes available. It would not be hard to tap into the existing sbitmap wait queue for that.

2) Have dm set BLK_MQ_F_BLOCKING and just sleep on the resource allocation. I haven't read the dm code to know if this is a possibility or not.

Quoted 5 lines

I'd probably prefer #1. It's a classic case of trying to get the request, and if it fails, add ourselves to the sbitmap tag wait queue head, retry, and bail if that also fails. Connecting the scarce resource and the consumer is the only way to really fix this, without bogus arbitrary delays.

Quoted 3 lines

Right, as I have replied to Bart, using mod_delayed_work_on() with returning BLK_STS_NO_DEV_RESOURCE(or sort of name) for the scarce resource should fix this issue.

It'll fix the forever stall, but it won't really fix it, as we'll slow down the dm device by some random amount.

A simple test case would be to have a null_blk device with a queue depth of one, and dm on top of that. Start a fio job that runs two jobs: one that does IO to the underlying device, and one that does IO to the dm device. If the job on the dm device runs substantially slower than the one to the underlying device, then the problem isn't really fixed.

Quoted 8 lines

Quoted 5 lines

I remembered that I tried this test on scsi-debug & dm-mpath over scsi-debug, seems not observed this issue, could you explain a bit why IO over dm-mpath may be slower? Because both two IO contexts call same get_request(), and in theory dm-mpath should be a bit quicker since it uses direct issue for underlying queue, without io scheduler involved.

Because if you lose the race for getting the request, you'll have some arbitrary delay before trying again, potentially. Compared to the direct

But the restart still works, one request is completed, then the queue is return immediately because we use mod_delayed_work_on(0), so looks no such issue.

Quoted 7 lines

Quoted 3 lines

There are no pending requests for this case, nothing to restart the queue. When you fail that blk_get_request(), you are idle, nothing is pending.

I think we needn't worry about that, once a device is attached to dm-rq, it can't be mounted any more, and usually user don't use the device directly and by dm-mpath at the same time.

Here's an example of that, using my current block tree (merged into master). The setup is dm-mpath on top of null_blk, the latter having just a single request. Both are mq devices.

Fio direct 4k random reads on dm_mq: ~250K iops

Start dd on underlying device (or partition on same device), just doing sequential reads.

Fio direct 4k random reads on dm_mq with dd running: 9 iops

Quoted 3 lines

No schedulers involved.

https://i.imgur.com/WTDnnwE.gif

If null_blk's timer mode is used with a bit delay introduced, I guess the effect from direct access to underlying queue shouldn't be so serious. But it still won't be good as direct access.

Another way may be to introduce a variants blk_get_request(), such as blk_get_request_with_notify(), then pass the current dm-rq's hctx to it, and use the tag's waitqueue to handle that. But the change can be a bit big.

Quoted 10 lines · Ming Lei

Quoted 10 lines

Quoted 5 lines

Quoted 2 lines

Where does the dm STS_RESOURCE error usually come from - what's exact resource are we running out of?

It is from blk_get_request(underlying queue), see
multipath_clone_and_map().

That's what I thought. So for a low queue depth underlying queue, it's quite possible that this situation can happen. Two potential solutions I see:

Quoted 7 lines

Quoted 7 lines

1) As described earlier in this thread, having a mechanism for being notified when the scarce resource becomes available. It would not be hard to tap into the existing sbitmap wait queue for that.

2) Have dm set BLK_MQ_F_BLOCKING and just sleep on the resource allocation. I haven't read the dm code to know if this is a possibility or not.

Quoted 6 lines

Quoted 5 lines

I'd probably prefer #1. It's a classic case of trying to get the request, and if it fails, add ourselves to the sbitmap tag wait queue head, retry, and bail if that also fails. Connecting the scarce resource and the consumer is the only way to really fix this, without bogus arbitrary delays.

Quoted 3 lines

Right, as I have replied to Bart, using mod_delayed_work_on() with returning BLK_STS_NO_DEV_RESOURCE(or sort of name) for the scarce resource should fix this issue.

It'll fix the forever stall, but it won't really fix it, as we'll slow down the dm device by some random amount.

A simple test case would be to have a null_blk device with a queue depth of one, and dm on top of that. Start a fio job that runs two jobs: one that does IO to the underlying device, and one that does IO to the dm device. If the job on the dm device runs substantially slower than the one to the underlying device, then the problem isn't really fixed.

That said, I'm fine with ensuring that we make forward progress always first, and then we can come up with a proper solution to the issue. The forward progress guarantee will be needed for the more rare failure cases, like allocation failures. nvme needs that too, for instance, for the discard range struct allocation.

Quoted 11 lines · Jens Axboe

Quoted 10 lines · Ming Lei

Quoted 10 lines

Quoted 5 lines

Quoted 2 lines

Where does the dm STS_RESOURCE error usually come from - what's exact resource are we running out of?

It is from blk_get_request(underlying queue), see
multipath_clone_and_map().

That's what I thought. So for a low queue depth underlying queue, it's quite possible that this situation can happen. Two potential solutions I see:

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

1) As described earlier in this thread, having a mechanism for being notified when the scarce resource becomes available. It would not be hard to tap into the existing sbitmap wait queue for that.

2) Have dm set BLK_MQ_F_BLOCKING and just sleep on the resource allocation. I haven't read the dm code to know if this is a possibility or not.

Right, #2 is _not_ the way forward. Historically request-based DM used its own mempool for requests, this was to be able to have some measure of control and resiliency in the face of low memory conditions that might be affecting the broader system.

Then Christoph switched over to adding per-request-data; which ushered in the use of blk_get_request using ATOMIC allocations. I like the result of that line of development. But taking the next step of setting BLK_MQ_F_BLOCKING is highly unfortunate (especially in that this dm-mpath.c code is common to old .request_fn and blk-mq, at least the call to blk_get_request is). Ultimately dm-mpath like to avoid blocking for a request because for this dm-mpath device we have multiple queues to allocate from if need be (provided we have an active-active storage network topology).

Quoted 5 lines

A simple test case would be to have a null_blk device with a queue depth of one, and dm on top of that. Start a fio job that runs two jobs: one that does IO to the underlying device, and one that does IO to the dm device. If the job on the dm device runs substantially slower than the one to the underlying device, then the problem isn't really fixed.

Not sure DM will allow the underlying device to be opened (due to master/slave ownership that is part of loading a DM table)?

Quoted 5 lines

That said, I'm fine with ensuring that we make forward progress always first, and then we can come up with a proper solution to the issue. The forward progress guarantee will be needed for the more rare failure cases, like allocation failures. nvme needs that too, for instance, for the discard range struct allocation.

Yeap, I'd be OK with that too. We'd be better for revisted this and then have some time to develop the ultimate robust fix (#1, callback from above).

Quoted 27 lines · Mike Snitzer

Quoted 11 lines · Jens Axboe

Quoted 10 lines · Ming Lei

Quoted 10 lines

Quoted 5 lines

Quoted 2 lines

Where does the dm STS_RESOURCE error usually come from - what's exact resource are we running out of?

It is from blk_get_request(underlying queue), see
multipath_clone_and_map().

That's what I thought. So for a low queue depth underlying queue, it's quite possible that this situation can happen. Two potential solutions I see:

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

1) As described earlier in this thread, having a mechanism for being notified when the scarce resource becomes available. It would not be hard to tap into the existing sbitmap wait queue for that.

2) Have dm set BLK_MQ_F_BLOCKING and just sleep on the resource allocation. I haven't read the dm code to know if this is a possibility or not.

Right, #2 is _not_ the way forward. Historically request-based DM used its own mempool for requests, this was to be able to have some measure of control and resiliency in the face of low memory conditions that might be affecting the broader system.

Quoted 9 lines

Then Christoph switched over to adding per-request-data; which ushered in the use of blk_get_request using ATOMIC allocations. I like the result of that line of development. But taking the next step of setting BLK_MQ_F_BLOCKING is highly unfortunate (especially in that this dm-mpath.c code is common to old .request_fn and blk-mq, at least the call to blk_get_request is). Ultimately dm-mpath like to avoid blocking for a request because for this dm-mpath device we have multiple queues to allocate from if need be (provided we have an active-active storage network topology).

If you can go to multiple devices, obviously it should not block on a single device. That's only true for the case where you can only go to one device, blocking at that point would probably be fine. Or if all your paths are busy, then blocking would also be OK.

That's what I thought. So for a low queue depth underlying queue, it's quite possible that this situation can happen. Two potential solutions I see:

1) As described earlier in this thread, having a mechanism for being notified when the scarce resource becomes available. It would not be hard to tap into the existing sbitmap wait queue for that.

2) Have dm set BLK_MQ_F_BLOCKING and just sleep on the resource allocation. I haven't read the dm code to know if this is a possibility or not.

I'd probably prefer #1. It's a classic case of trying to get the request, and if it fails, add ourselves to the sbitmap tag wait queue head, retry, and bail if that also fails. Connecting the scarce resource and the consumer is the only way to really fix this, without bogus arbitrary delays.

Now most of times both NVMe and SCSI won't return BLK_STS_RESOURCE, and it should be DM-only which returns STS_RESOURCE so often.

Quoted 2 lines · Ming Lei

Now most of times both NVMe and SCSI won't return BLK_STS_RESOURCE, and it should be DM-only which returns STS_RESOURCE so often.

That's wrong at least for SCSI. See also https://marc.info/?l=linux-block&m=151578329417076.

For other scenario's, e.g. if a SCSI initiator submits a
SCSI request over a fabric and the SCSI target replies with "BUSY" then the

SCSI core will end the I/O request with status BLK_STS_RESOURCE after the maximum number of retries has been reached (see also scsi_io_completion()). In that last case, if a SCSI target sends a "BUSY" reply over the wire back to the initiator, there is no other approach for the SCSI initiator to figure out whether it can queue another request than to resubmit the request. The worst possible strategy is to resubmit a request immediately because that will cause a significant fraction of the fabric bandwidth to be used just for replying "BUSY" to requests that can't be processed immediately.

Could you explain a bit when SCSI target replies with BUSY very often?

Inside initiator, we have limited the max per-LUN requests and per-host requests already before calling .queue_rq().