[PATCH] dm rq: Avoid that request processing stalls sporadically

[PATCH] dm rq: Avoid that request processing stalls sporadically

[Bart Van Assche]

If the .queue_rq() implementation of a block driver returns
BLK_STS_RESOURCE then that block driver is responsible for
rerunning the queue once the condition that caused it to return
BLK_STS_RESOURCE has been cleared. The dm-mpath driver tells the
dm core to requeue a request if e.g. not enough memory is
available for cloning a request or if the underlying path is
busy. Since the dm-mpath driver does not receive any kind of
notification if the condition that caused it to return "requeue"
is cleared, the only solution to avoid that dm-mpath request
processing stalls is to call blk_mq_delay_run_hw_queue(). Hence
this patch.

Fixes: ec3eaf9a6731 ("dm mpath: don't call blk_mq_delay_run_hw_queue() in case of BLK_STS_RESOURCE")
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Ming Lei <ming.lei@redhat.com>
---
 drivers/md/dm-rq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
index f16096af879a..c59c59cfd2a5 100644
--- a/drivers/md/dm-rq.c
+++ b/drivers/md/dm-rq.c
@@ -761,6 +761,7 @@ static blk_status_t dm_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
 		/* Undo dm_start_request() before requeuing */
 		rq_end_stats(md, rq);
 		rq_completed(md, rq_data_dir(rq), false);
+		blk_mq_delay_run_hw_queue(hctx, 100/*ms*/);
 		return BLK_STS_RESOURCE;
 	}
 
-- 
2.15.1

Re: dm rq: Avoid that request processing stalls sporadically

[Mike Snitzer]

On Thu, Jan 18 2018 at 11:37am -0500,
Bart Van Assche <bart.vanassche@wdc.com> wrote:

> If the .queue_rq() implementation of a block driver returns
> BLK_STS_RESOURCE then that block driver is responsible for
> rerunning the queue once the condition that caused it to return
> BLK_STS_RESOURCE has been cleared. The dm-mpath driver tells the
> dm core to requeue a request if e.g. not enough memory is
> available for cloning a request or if the underlying path is
> busy. Since the dm-mpath driver does not receive any kind of
> notification if the condition that caused it to return "requeue"
> is cleared, the only solution to avoid that dm-mpath request
> processing stalls is to call blk_mq_delay_run_hw_queue(). Hence
> this patch.
> 
> Fixes: ec3eaf9a6731 ("dm mpath: don't call blk_mq_delay_run_hw_queue() in case of BLK_STS_RESOURCE")
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/md/dm-rq.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> index f16096af879a..c59c59cfd2a5 100644
> --- a/drivers/md/dm-rq.c
> +++ b/drivers/md/dm-rq.c
> @@ -761,6 +761,7 @@ static blk_status_t dm_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
>  		/* Undo dm_start_request() before requeuing */
>  		rq_end_stats(md, rq);
>  		rq_completed(md, rq_data_dir(rq), false);
> +		blk_mq_delay_run_hw_queue(hctx, 100/*ms*/);
>  		return BLK_STS_RESOURCE;
>  	}
>  
> -- 
> 2.15.1
> 

Sorry but we need to understand why you still need this.

The issue you say it was originally intended to fix _should_ be
addressed with this change:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.16&id=4dd6edd23e7ea971efddc303f9e67eb79e95808e

So if you still feel you need to blindly kick the queue then it is very
likely a bug in blk-mq (either its RESTART hueristics or whatever
internal implementation is lacking)

Did you try Ming's RFC patch to "fixup RESTART" before resorting to the
above again?, see: https://patchwork.kernel.org/patch/10172315/

Thanks,
Mike

Re: dm rq: Avoid that request processing stalls sporadically

[Bart Van Assche]

T24gVGh1LCAyMDE4LTAxLTE4IGF0IDExOjUwIC0wNTAwLCBNaWtlIFNuaXR6ZXIgd3JvdGU6DQo+
IFRoZSBpc3N1ZSB5b3Ugc2F5IGl0IHdhcyBvcmlnaW5hbGx5IGludGVuZGVkIHRvIGZpeCBfc2hv
dWxkXyBiZQ0KPiBhZGRyZXNzZWQgd2l0aCB0aGlzIGNoYW5nZToNCj4gaHR0cHM6Ly9naXQua2Vy
bmVsLm9yZy9wdWIvc2NtL2xpbnV4L2tlcm5lbC9naXQvZGV2aWNlLW1hcHBlci9saW51eC1kbS5n
aXQvY29tbWl0Lz9oPWRtLTQuMTYmaWQ9NGRkNmVkZDIzZTdlYTk3MWVmZGRjMzAzZjllNjdlYjc5
ZTk1ODA4ZQ0KDQpIZWxsbyBNaWtlLA0KDQpTb3JyeSBidXQgSSdtIG5vdCBjb252aW5jZWQgdGhh
dCB0aGF0IHBhdGNoIGlzIHN1ZmZpY2llbnQuIFRoYXQgcGF0Y2ggaGVscHMNCmlmIC5lbmRfaW8o
KSBpcyBjYWxsZWQgd2l0aCBzdGF0dXMgQkxLX1NUU19SRVNPVVJDRSBhbmQgYWxzbyBpZg0KYmxr
X2luc2VydF9jbG9uZWRfcmVxdWVzdCgpIHJldHVybnMgdGhlIC5xdWV1ZV9ycSgpIHJldHVybiB2
YWx1ZS4gSXQgZG9lcyBub3QNCmhlbHAgaWYgLnF1ZXVlX3JxKCkgcmV0dXJucyBCTEtfU1RTX1JF
U09VUkNFIGFuZCB0aGF0IHJldHVybiB2YWx1ZSBnZXRzDQppZ25vcmVkLiBJIHRoaW5rIHRoYXQg
Y2FuIGhhcHBlbiBhcyBmb2xsb3dzOg0KLSBSZXF1ZXN0IGNsb25pbmcgaW4gbXVsdGlwYXRoX2Ns
b25lX2FuZF9tYXAoKSBzdWNjZWVkcyBhbmQgdGhhdCBmdW5jdGlvbg0KICByZXR1cm5zIERNX01B
UElPX1JFTUFQUEVELg0KLSBkbV9kaXNwYXRjaF9jbG9uZV9yZXF1ZXN0KCkgY2FsbHMgYmxrX2lu
c2VydF9jbG9uZWRfcmVxdWVzdCgpLg0KLSBibGtfaW5zZXJ0X2Nsb25lZF9yZXF1ZXN0KCkgY2Fs
bHMgYmxrX21xX3JlcXVlc3RfZGlyZWN0X2lzc3VlKCksIHdoaWNoDQogIHJlc3VsdHMgaW4gYSBj
YWxsIG9mIF9fYmxrX21xX3RyeV9pc3N1ZV9kaXJlY3RseSgpLg0KLSBfX2Jsa19tcV90cnlfaXNz
dWVfZGlyZWN0bHkoKSBjYWxscyBibGtfbXFfc2NoZWRfaW5zZXJ0X3JlcXVlc3QoKS4gSW4gdGhp
cw0KICBjYXNlIHRoZSBCTEtfU1RTX1JFU09VUkNFIHJldHVybmVkIGJ5IHRoZSAucXVldWVfcnEo
KSBpbXBsZW1lbnRhdGlvbiBvZiB0aGUNCiAgdW5kZXJseWluZyBwYXRoIHdpbGwgYmUgaWdub3Jl
ZC4NCg0KUGxlYXNlIG5vdGUgdGhhdCBJIGhhdmUgbm90IHRyaWVkIHRvIGZpbmQgYWxsIHBhdGhz
IHRoYXQgaWdub3JlIHRoZQ0KLnF1ZXVlX3JxKCkgcmV0dXJuIHZhbHVlLg0KDQpUaGFua3MsDQoN
CkJhcnQu

Re: [PATCH] dm rq: Avoid that request processing stalls sporadically

[Ming Lei]

On Thu, Jan 18, 2018 at 08:37:07AM -0800, Bart Van Assche wrote:
> If the .queue_rq() implementation of a block driver returns
> BLK_STS_RESOURCE then that block driver is responsible for
> rerunning the queue once the condition that caused it to return
> BLK_STS_RESOURCE has been cleared. The dm-mpath driver tells the
> dm core to requeue a request if e.g. not enough memory is
> available for cloning a request or if the underlying path is
> busy. Since the dm-mpath driver does not receive any kind of
> notification if the condition that caused it to return "requeue"
> is cleared, the only solution to avoid that dm-mpath request
> processing stalls is to call blk_mq_delay_run_hw_queue(). Hence
> this patch.
> 
> Fixes: ec3eaf9a6731 ("dm mpath: don't call blk_mq_delay_run_hw_queue() in case of BLK_STS_RESOURCE")
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/md/dm-rq.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> index f16096af879a..c59c59cfd2a5 100644
> --- a/drivers/md/dm-rq.c
> +++ b/drivers/md/dm-rq.c
> @@ -761,6 +761,7 @@ static blk_status_t dm_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
>  		/* Undo dm_start_request() before requeuing */
>  		rq_end_stats(md, rq);
>  		rq_completed(md, rq_data_dir(rq), false);
> +		blk_mq_delay_run_hw_queue(hctx, 100/*ms*/);
>  		return BLK_STS_RESOURCE;
>  	}
>  

Nak.

It still takes a bit time to add this request to hctx->dispatch_list
from here, so suppose the time is longer than 100ms because of interrupt
, preemption or whatever, this request can't be observed in the scheduled
run queue(__blk_mq_run_hw_queue).

Not mention it is just a ugly workaround, which degrades performance
a lot.

-- 
Ming

Re: [PATCH] dm rq: Avoid that request processing stalls sporadically

[Bart Van Assche]

T24gRnJpLCAyMDE4LTAxLTE5IGF0IDA4OjExICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gT24g
VGh1LCBKYW4gMTgsIDIwMTggYXQgMDg6Mzc6MDdBTSAtMDgwMCwgQmFydCBWYW4gQXNzY2hlIHdy
b3RlOg0KPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL21kL2RtLXJxLmMgYi9kcml2ZXJzL21kL2Rt
LXJxLmMNCj4gPiBpbmRleCBmMTYwOTZhZjg3OWEuLmM1OWM1OWNmZDJhNSAxMDA2NDQNCj4gPiAt
LS0gYS9kcml2ZXJzL21kL2RtLXJxLmMNCj4gPiArKysgYi9kcml2ZXJzL21kL2RtLXJxLmMNCj4g
PiBAQCAtNzYxLDYgKzc2MSw3IEBAIHN0YXRpYyBibGtfc3RhdHVzX3QgZG1fbXFfcXVldWVfcnEo
c3RydWN0IGJsa19tcV9od19jdHggKmhjdHgsDQo+ID4gIAkJLyogVW5kbyBkbV9zdGFydF9yZXF1
ZXN0KCkgYmVmb3JlIHJlcXVldWluZyAqLw0KPiA+ICAJCXJxX2VuZF9zdGF0cyhtZCwgcnEpOw0K
PiA+ICAJCXJxX2NvbXBsZXRlZChtZCwgcnFfZGF0YV9kaXIocnEpLCBmYWxzZSk7DQo+ID4gKwkJ
YmxrX21xX2RlbGF5X3J1bl9od19xdWV1ZShoY3R4LCAxMDAvKm1zKi8pOw0KPiA+ICAJCXJldHVy
biBCTEtfU1RTX1JFU09VUkNFOw0KPiA+ICAJfQ0KPiA+ICANCj4gDQo+IE5hay4NCg0KVGhpcyBw
YXRjaCBmaXhlcyBhIHJlZ3Jlc3Npb24gdGhhdCB3YXMgaW50cm9kdWNlZCBieSB5b3UuIFlvdSBz
aG91bGQga25vdw0KdGhhdCByZWdyZXNzaW9ucyBhcmUgbm90IGFjY2VwdGFibGUuIElmIHlvdSBk
b24ndCBhZ3JlZSB3aXRoIHRoaXMgcGF0Y2gsDQpwbGVhc2UgZml4IHRoZSByb290IGNhdXNlLg0K
DQpCYXJ0Lg==

Re: [PATCH] dm rq: Avoid that request processing stalls sporadically

[Ming Lei]

On Fri, Jan 19, 2018 at 12:14:24AM +0000, Bart Van Assche wrote:
> On Fri, 2018-01-19 at 08:11 +0800, Ming Lei wrote:
> > On Thu, Jan 18, 2018 at 08:37:07AM -0800, Bart Van Assche wrote:
> > > diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> > > index f16096af879a..c59c59cfd2a5 100644
> > > --- a/drivers/md/dm-rq.c
> > > +++ b/drivers/md/dm-rq.c
> > > @@ -761,6 +761,7 @@ static blk_status_t dm_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
> > >  		/* Undo dm_start_request() before requeuing */
> > >  		rq_end_stats(md, rq);
> > >  		rq_completed(md, rq_data_dir(rq), false);
> > > +		blk_mq_delay_run_hw_queue(hctx, 100/*ms*/);
> > >  		return BLK_STS_RESOURCE;
> > >  	}
> > >  
> > 
> > Nak.
> 
> This patch fixes a regression that was introduced by you. You should know
> that regressions are not acceptable. If you don't agree with this patch,
> please fix the root cause.

Yesterday I sent a patch, did you test that?

-- 
Ming

Re: [PATCH] dm rq: Avoid that request processing stalls sporadically

[Bart Van Assche]

T24gRnJpLCAyMDE4LTAxLTE5IGF0IDA4OjE4ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gT24g
RnJpLCBKYW4gMTksIDIwMTggYXQgMTI6MTQ6MjRBTSArMDAwMCwgQmFydCBWYW4gQXNzY2hlIHdy
b3RlOg0KPiA+IE9uIEZyaSwgMjAxOC0wMS0xOSBhdCAwODoxMSArMDgwMCwgTWluZyBMZWkgd3Jv
dGU6DQo+ID4gPiBPbiBUaHUsIEphbiAxOCwgMjAxOCBhdCAwODozNzowN0FNIC0wODAwLCBCYXJ0
IFZhbiBBc3NjaGUgd3JvdGU6DQo+ID4gPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL21kL2RtLXJx
LmMgYi9kcml2ZXJzL21kL2RtLXJxLmMNCj4gPiA+ID4gaW5kZXggZjE2MDk2YWY4NzlhLi5jNTlj
NTljZmQyYTUgMTAwNjQ0DQo+ID4gPiA+IC0tLSBhL2RyaXZlcnMvbWQvZG0tcnEuYw0KPiA+ID4g
PiArKysgYi9kcml2ZXJzL21kL2RtLXJxLmMNCj4gPiA+ID4gQEAgLTc2MSw2ICs3NjEsNyBAQCBz
dGF0aWMgYmxrX3N0YXR1c190IGRtX21xX3F1ZXVlX3JxKHN0cnVjdCBibGtfbXFfaHdfY3R4ICpo
Y3R4LA0KPiA+ID4gPiAgCQkvKiBVbmRvIGRtX3N0YXJ0X3JlcXVlc3QoKSBiZWZvcmUgcmVxdWV1
aW5nICovDQo+ID4gPiA+ICAJCXJxX2VuZF9zdGF0cyhtZCwgcnEpOw0KPiA+ID4gPiAgCQlycV9j
b21wbGV0ZWQobWQsIHJxX2RhdGFfZGlyKHJxKSwgZmFsc2UpOw0KPiA+ID4gPiArCQlibGtfbXFf
ZGVsYXlfcnVuX2h3X3F1ZXVlKGhjdHgsIDEwMC8qbXMqLyk7DQo+ID4gPiA+ICAJCXJldHVybiBC
TEtfU1RTX1JFU09VUkNFOw0KPiA+ID4gPiAgCX0NCj4gPiA+ID4gIA0KPiA+ID4gDQo+ID4gPiBO
YWsuDQo+ID4gDQo+ID4gVGhpcyBwYXRjaCBmaXhlcyBhIHJlZ3Jlc3Npb24gdGhhdCB3YXMgaW50
cm9kdWNlZCBieSB5b3UuIFlvdSBzaG91bGQga25vdw0KPiA+IHRoYXQgcmVncmVzc2lvbnMgYXJl
IG5vdCBhY2NlcHRhYmxlLiBJZiB5b3UgZG9uJ3QgYWdyZWUgd2l0aCB0aGlzIHBhdGNoLA0KPiA+
IHBsZWFzZSBmaXggdGhlIHJvb3QgY2F1c2UuDQo+IA0KPiBZZXN0ZXJkYXkgSSBzZW50IGEgcGF0
Y2gsIGRpZCB5b3UgdGVzdCB0aGF0Pw0KDQpZZXMsIEkgZGlkLiBJdCBjYXVzZWQgcXVldWUgc3Rh
bGxzIHRoYXQgd2VyZSBzbyBiYWQgdGhhdCBzZW5kaW5nICJraWNrIiB0byB0aGUNCmRlYnVnZnMg
InN0YXRlIiBhdHRyaWJ1dGUgd2FzIG5vdCBzdWZmaWNpZW50IHRvIHJlc29sdmUgdGhlIHF1ZXVl
IHN0YWxsLg0KDQpCYXJ0Lg==

Re: [PATCH] dm rq: Avoid that request processing stalls sporadically

[Jens Axboe]

On 1/18/18 5:18 PM, Ming Lei wrote:
> On Fri, Jan 19, 2018 at 12:14:24AM +0000, Bart Van Assche wrote:
>> On Fri, 2018-01-19 at 08:11 +0800, Ming Lei wrote:
>>> On Thu, Jan 18, 2018 at 08:37:07AM -0800, Bart Van Assche wrote:
>>>> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
>>>> index f16096af879a..c59c59cfd2a5 100644
>>>> --- a/drivers/md/dm-rq.c
>>>> +++ b/drivers/md/dm-rq.c
>>>> @@ -761,6 +761,7 @@ static blk_status_t dm_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
>>>>  		/* Undo dm_start_request() before requeuing */
>>>>  		rq_end_stats(md, rq);
>>>>  		rq_completed(md, rq_data_dir(rq), false);
>>>> +		blk_mq_delay_run_hw_queue(hctx, 100/*ms*/);
>>>>  		return BLK_STS_RESOURCE;
>>>>  	}
>>>>  
>>>
>>> Nak.
>>
>> This patch fixes a regression that was introduced by you. You should know
>> that regressions are not acceptable. If you don't agree with this patch,
>> please fix the root cause.
> 
> Yesterday I sent a patch, did you test that?

That patch isn't going to be of much help. It might prevent you from
completely stalling, but it might take you tens of seconds to get there.

On top of that, it's a rolling timer that gets added when IO is queued,
and re-added if IO is pending when it fires. If the latter case is not
true, then it won't get armed again. So if IO fails to queue without
any other IO pending, you're pretty much in the same situation as if
you marked the queue as restart. Nobody is going to be noticing
either of those conditions.

-- 
Jens Axboe

Re: dm rq: Avoid that request processing stalls sporadically

[Ming Lei]

On Thu, Jan 18, 2018 at 05:13:53PM +0000, Bart Van Assche wrote:
> On Thu, 2018-01-18 at 11:50 -0500, Mike Snitzer wrote:
> > The issue you say it was originally intended to fix _should_ be
> > addressed with this change:
> > https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.16&id=4dd6edd23e7ea971efddc303f9e67eb79e95808e
> 
> Hello Mike,
> 
> Sorry but I'm not convinced that that patch is sufficient. That patch helps
> if .end_io() is called with status BLK_STS_RESOURCE and also if
> blk_insert_cloned_request() returns the .queue_rq() return value. It does not
> help if .queue_rq() returns BLK_STS_RESOURCE and that return value gets
> ignored.

The return value from .queue_rq() is handled by blk-mq, why do you think
it can be ignored? Please see blk_mq_dispatch_rq_list().

> I think that can happen as follows:
> - Request cloning in multipath_clone_and_map() succeeds and that function
>   returns DM_MAPIO_REMAPPED.
> - dm_dispatch_clone_request() calls blk_insert_cloned_request().
> - blk_insert_cloned_request() calls blk_mq_request_direct_issue(), which
>   results in a call of __blk_mq_try_issue_directly().
> - __blk_mq_try_issue_directly() calls blk_mq_sched_insert_request(). In this

This only happens iff queue is stopped or quiesced, then we return
BLK_STS_OK to blk-mq via .queue_rq(), please see __blk_mq_try_issue_directly(),
how does this cause IO hang? 

>   case the BLK_STS_RESOURCE returned by the .queue_rq() implementation of the
>   underlying path will be ignored.

No, this case won't return BLK_STS_RESOURCE.

-- 
Ming

Re: dm rq: Avoid that request processing stalls sporadically

[Bart Van Assche]

T24gRnJpLCAyMDE4LTAxLTE5IGF0IDA4OjI2ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gTm8s
IHRoaXMgY2FzZSB3b24ndCByZXR1cm4gQkxLX1NUU19SRVNPVVJDRS4NCg0KSXQncyBwb3NzaWJs
ZSB0aGF0IG15IGF0dGVtcHQgdG8gcmV2ZXJzZSBlbmdpbmVlciB0aGUgbGF0ZXN0IGJsay1tcSBj
aGFuZ2VzDQp3YXMgd3JvbmcuIEJ1dCB0aGUgcXVldWUgc3RhbGwgaXMgcmVhbCBhbmQgbmVlZHMg
dG8gYmUgZml4ZWQuDQoNCkJhcnQu

Re: [PATCH] dm rq: Avoid that request processing stalls sporadically

[Ming Lei]

On Thu, Jan 18, 2018 at 05:24:29PM -0700, Jens Axboe wrote:
> On 1/18/18 5:18 PM, Ming Lei wrote:
> > On Fri, Jan 19, 2018 at 12:14:24AM +0000, Bart Van Assche wrote:
> >> On Fri, 2018-01-19 at 08:11 +0800, Ming Lei wrote:
> >>> On Thu, Jan 18, 2018 at 08:37:07AM -0800, Bart Van Assche wrote:
> >>>> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> >>>> index f16096af879a..c59c59cfd2a5 100644
> >>>> --- a/drivers/md/dm-rq.c
> >>>> +++ b/drivers/md/dm-rq.c
> >>>> @@ -761,6 +761,7 @@ static blk_status_t dm_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
> >>>>  		/* Undo dm_start_request() before requeuing */
> >>>>  		rq_end_stats(md, rq);
> >>>>  		rq_completed(md, rq_data_dir(rq), false);
> >>>> +		blk_mq_delay_run_hw_queue(hctx, 100/*ms*/);
> >>>>  		return BLK_STS_RESOURCE;
> >>>>  	}
> >>>>  
> >>>
> >>> Nak.
> >>
> >> This patch fixes a regression that was introduced by you. You should know
> >> that regressions are not acceptable. If you don't agree with this patch,
> >> please fix the root cause.
> > 
> > Yesterday I sent a patch, did you test that?
> 
> That patch isn't going to be of much help. It might prevent you from
> completely stalling, but it might take you tens of seconds to get there.

Yeah, that is true, and why I marked it as RFC, but the case is so rare to
happen.

> 
> On top of that, it's a rolling timer that gets added when IO is queued,
> and re-added if IO is pending when it fires. If the latter case is not
> true, then it won't get armed again. So if IO fails to queue without
> any other IO pending, you're pretty much in the same situation as if

No IO pending detection may take a bit time, we can do it after
BLK_STS_RESOURCE is returned and SCHED_RESTART is set. I have done
this way in the following patch, what do you think of it?

https://github.com/ming1/linux/commit/f866ce0b97b0ae22d033881da7eb07706fd458b4

-- 
Ming

Re: [PATCH] dm rq: Avoid that request processing stalls sporadically

[Jens Axboe]

On 1/18/18 5:35 PM, Ming Lei wrote:
> On Thu, Jan 18, 2018 at 05:24:29PM -0700, Jens Axboe wrote:
>> On 1/18/18 5:18 PM, Ming Lei wrote:
>>> On Fri, Jan 19, 2018 at 12:14:24AM +0000, Bart Van Assche wrote:
>>>> On Fri, 2018-01-19 at 08:11 +0800, Ming Lei wrote:
>>>>> On Thu, Jan 18, 2018 at 08:37:07AM -0800, Bart Van Assche wrote:
>>>>>> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
>>>>>> index f16096af879a..c59c59cfd2a5 100644
>>>>>> --- a/drivers/md/dm-rq.c
>>>>>> +++ b/drivers/md/dm-rq.c
>>>>>> @@ -761,6 +761,7 @@ static blk_status_t dm_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
>>>>>>  		/* Undo dm_start_request() before requeuing */
>>>>>>  		rq_end_stats(md, rq);
>>>>>>  		rq_completed(md, rq_data_dir(rq), false);
>>>>>> +		blk_mq_delay_run_hw_queue(hctx, 100/*ms*/);
>>>>>>  		return BLK_STS_RESOURCE;
>>>>>>  	}
>>>>>>  
>>>>>
>>>>> Nak.
>>>>
>>>> This patch fixes a regression that was introduced by you. You should know
>>>> that regressions are not acceptable. If you don't agree with this patch,
>>>> please fix the root cause.
>>>
>>> Yesterday I sent a patch, did you test that?
>>
>> That patch isn't going to be of much help. It might prevent you from
>> completely stalling, but it might take you tens of seconds to get there.
> 
> Yeah, that is true, and why I marked it as RFC, but the case is so rare to
> happen.

You don't know that. If the resource is very limited, or someone else
is gobbling up all of it, then it may trigger quite often. Granted,
in that case, you need some way of signaling the freeing of those
resources to make it efficient.

>> On top of that, it's a rolling timer that gets added when IO is queued,
>> and re-added if IO is pending when it fires. If the latter case is not
>> true, then it won't get armed again. So if IO fails to queue without
>> any other IO pending, you're pretty much in the same situation as if
> 
> No IO pending detection may take a bit time, we can do it after
> BLK_STS_RESOURCE is returned and SCHED_RESTART is set. I have done
> this way in the following patch, what do you think of it?
> 
> https://github.com/ming1/linux/commit/f866ce0b97b0ae22d033881da7eb07706fd458b4

I think it's overly complicated. As I wrote in a lengthier email earlier
today, just ensure that we have a unique return code from the driver for
when it aborts queuing an IO due to a resource shortage that isn't tied
to it's own consumption of it. When we get that return, do a delayed
queue run after X amount of time to ensure we always try again. If you
want to get fancy (later on), you could track the frequency of such
returns and complain if it's too high.

There's no point in adding a lot of code to check whether we need to run
the queue or not. Just always do it. We won't be doing it more than 100
times per second for the worst case of the condition taking a while to
clear, if we stick to the 10ms re-run time.

-- 
Jens Axboe

Re: [PATCH] dm rq: Avoid that request processing stalls sporadically

[Ming Lei]

On Thu, Jan 18, 2018 at 05:49:10PM -0700, Jens Axboe wrote:
> On 1/18/18 5:35 PM, Ming Lei wrote:
> > On Thu, Jan 18, 2018 at 05:24:29PM -0700, Jens Axboe wrote:
> >> On 1/18/18 5:18 PM, Ming Lei wrote:
> >>> On Fri, Jan 19, 2018 at 12:14:24AM +0000, Bart Van Assche wrote:
> >>>> On Fri, 2018-01-19 at 08:11 +0800, Ming Lei wrote:
> >>>>> On Thu, Jan 18, 2018 at 08:37:07AM -0800, Bart Van Assche wrote:
> >>>>>> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> >>>>>> index f16096af879a..c59c59cfd2a5 100644
> >>>>>> --- a/drivers/md/dm-rq.c
> >>>>>> +++ b/drivers/md/dm-rq.c
> >>>>>> @@ -761,6 +761,7 @@ static blk_status_t dm_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
> >>>>>>  		/* Undo dm_start_request() before requeuing */
> >>>>>>  		rq_end_stats(md, rq);
> >>>>>>  		rq_completed(md, rq_data_dir(rq), false);
> >>>>>> +		blk_mq_delay_run_hw_queue(hctx, 100/*ms*/);
> >>>>>>  		return BLK_STS_RESOURCE;
> >>>>>>  	}
> >>>>>>  
> >>>>>
> >>>>> Nak.
> >>>>
> >>>> This patch fixes a regression that was introduced by you. You should know
> >>>> that regressions are not acceptable. If you don't agree with this patch,
> >>>> please fix the root cause.
> >>>
> >>> Yesterday I sent a patch, did you test that?
> >>
> >> That patch isn't going to be of much help. It might prevent you from
> >> completely stalling, but it might take you tens of seconds to get there.
> > 
> > Yeah, that is true, and why I marked it as RFC, but the case is so rare to
> > happen.
> 
> You don't know that. If the resource is very limited, or someone else
> is gobbling up all of it, then it may trigger quite often. Granted,
> in that case, you need some way of signaling the freeing of those
> resources to make it efficient.
> 
> >> On top of that, it's a rolling timer that gets added when IO is queued,
> >> and re-added if IO is pending when it fires. If the latter case is not
> >> true, then it won't get armed again. So if IO fails to queue without
> >> any other IO pending, you're pretty much in the same situation as if
> > 
> > No IO pending detection may take a bit time, we can do it after
> > BLK_STS_RESOURCE is returned and SCHED_RESTART is set. I have done
> > this way in the following patch, what do you think of it?
> > 
> > https://github.com/ming1/linux/commit/f866ce0b97b0ae22d033881da7eb07706fd458b4
> 
> I think it's overly complicated. As I wrote in a lengthier email earlier
> today, just ensure that we have a unique return code from the driver for
> when it aborts queuing an IO due to a resource shortage that isn't tied
> to it's own consumption of it. When we get that return, do a delayed
> queue run after X amount of time to ensure we always try again. If you
> want to get fancy (later on), you could track the frequency of such
> returns and complain if it's too high.

Suppose the new introduced return code is BLK_STS_NO_DEV_RESOURCE.

This way may degrade performance, for example, DM-MPATH returns
BLK_STS_NO_DEV_RESOURCE when blk_get_request() returns NULL, blk-mq
handles it by blk_mq_delay_run_hw_queue(10ms). Then blk_mq_sched_restart()
is just triggered when one DM-MPATH request is completed, that means one
request of underlying queue is completed too, but blk_mq_sched_restart()
still can't make progress during the 10ms.

That means we should only run blk_mq_delay_run_hw_queue(10ms/100ms) when
the queue is idle.

I will post out the patch in github for discussion.

Thanks,
Ming