[PATCH] dm-mpath: return DM_MAPIO_REQUEUE in case of rq allocation failure

[PATCH] dm-mpath: return DM_MAPIO_REQUEUE in case of rq allocation failure

[Ming Lei]

blk-mq will rerun queue via RESTART after one request is completed,
so not necessary to wait random time for requeuing, we should trust
blk-mq to do it.

More importantly, we need return BLK_STS_RESOURCE to blk-mq
so that dequeue from I/O scheduler can be stopped, then
I/O merge gets improved.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/md/dm-mpath.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 11f273d2f018..0902f7762306 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -504,8 +504,20 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 		if (queue_dying) {
 			atomic_inc(&m->pg_init_in_progress);
 			activate_or_offline_path(pgpath);
+			return DM_MAPIO_DELAY_REQUEUE;
 		}
-		return DM_MAPIO_DELAY_REQUEUE;
+
+		/*
+		 * blk-mq's SCHED_RESTART can cover this requeue, so
+		 * we needn't to deal with it by DELAY_REQUEUE. More
+		 * importantly, we have to return DM_MAPIO_REQUEUE
+		 * so that blk-mq can get the queue busy feedback,
+		 * otherwise I/O merge can be hurt.
+		 */
+		if (q->mq_ops)
+			return DM_MAPIO_REQUEUE;
+		else
+			return DM_MAPIO_DELAY_REQUEUE;
 	}
 	clone->bio = clone->biotail = NULL;
 	clone->rq_disk = bdev->bd_disk;
-- 
2.9.5

Re: [PATCH] dm-mpath: return DM_MAPIO_REQUEUE in case of rq allocation failure

[Bart Van Assche]

T24gRnJpLCAyMDE3LTA5LTIyIGF0IDA5OjM1ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gKwkJ
LyoNCj4gKwkJICogYmxrLW1xJ3MgU0NIRURfUkVTVEFSVCBjYW4gY292ZXIgdGhpcyByZXF1ZXVl
LCBzbw0KPiArCQkgKiB3ZSBuZWVkbid0IHRvIGRlYWwgd2l0aCBpdCBieSBERUxBWV9SRVFVRVVF
LiBNb3JlDQo+ICsJCSAqIGltcG9ydGFudGx5LCB3ZSBoYXZlIHRvIHJldHVybiBETV9NQVBJT19S
RVFVRVVFDQo+ICsJCSAqIHNvIHRoYXQgYmxrLW1xIGNhbiBnZXQgdGhlIHF1ZXVlIGJ1c3kgZmVl
ZGJhY2ssDQo+ICsJCSAqIG90aGVyd2lzZSBJL08gbWVyZ2UgY2FuIGJlIGh1cnQuDQo+ICsJCSAq
Lw0KPiArCQlpZiAocS0+bXFfb3BzKQ0KPiArCQkJcmV0dXJuIERNX01BUElPX1JFUVVFVUU7DQo+
ICsJCWVsc2UNCj4gKwkJCXJldHVybiBETV9NQVBJT19ERUxBWV9SRVFVRVVFOw0KPiAgCX0NCg0K
VGhpcyBwYXRjaCBpcyBpbmZlcmlvciB0byB3aGF0IEkgcG9zdGVkIGJlY2F1c2UgdGhpcyBwYXRj
aCBkb2VzIG5vdCBhdm9pZA0KdGhlIGRlbGF5IGlmIG11bHRpcGxlIExVTnMgYXJlIGFzc29jaWF0
ZWQgd2l0aCB0aGUgc2FtZSBTQ1NJIGhvc3QuIENvbnNpZGVyDQplLmcuIHRoZSBmb2xsb3dpbmcg
Y29uZmlndXJhdGlvbjoNCiogQSBzaW5nbGUgU0NTSSBob3N0IHdpdGggdHdvIFNDU0kgTFVOcyBh
c3NvY2lhdGVkIHRvIHRoYXQgaG9zdCwgZS5nLiAvZGV2L3NkYQ0KICBhbmQgL2Rldi9zZGIuDQoq
IEEgZG0tbXBhdGggaW5zdGFuY2UgaGFzIGJlZW4gY3JlYXRlZCBvbiB0b3Agb2YgL2Rldi9zZGEu
DQpJZiBhbGwgdGFncyBhcmUgaW4gdXNlIGJ5IHJlcXVlc3RzIHF1ZXVlZCB0byAvZGV2L3NkYiwg
bm8gZG0gcmVxdWVzdHMgYXJlIGluDQpwcm9ncmVzcyBhbmQgYSByZXF1ZXN0IGlzIHN1Ym1pdHRl
ZCBhZ2FpbnN0IHRoZSBkbS1tcGF0aCBkZXZpY2UgdGhlbiB0aGUNCmJsa19nZXRfcmVxdWVzdChx
LCBHRlBfQVRPTUlDKSBjYWxsIHdpbGwgZmFpbC4gVGhlIHJlcXVlc3Qgd2lsbCBiZSByZXF1ZXVl
ZA0KYW5kIHRoZSBxdWV1ZSB3aWxsIGJlIHJlcnVuIGFmdGVyIGEgZGVsYXkuDQoNCk15IHBhdGNo
IGRvZXMgbm90IGludHJvZHVjZSBhIGRlbGF5IGluIHRoaXMgY2FzZS4NCg0KQmFydC4=

Re: [PATCH] dm-mpath: return DM_MAPIO_REQUEUE in case of rq allocation failure

[Ming Lei]

On Fri, Sep 22, 2017 at 03:06:16PM +0000, Bart Van Assche wrote:
> On Fri, 2017-09-22 at 09:35 +0800, Ming Lei wrote:
> > +		/*
> > +		 * blk-mq's SCHED_RESTART can cover this requeue, so
> > +		 * we needn't to deal with it by DELAY_REQUEUE. More
> > +		 * importantly, we have to return DM_MAPIO_REQUEUE
> > +		 * so that blk-mq can get the queue busy feedback,
> > +		 * otherwise I/O merge can be hurt.
> > +		 */
> > +		if (q->mq_ops)
> > +			return DM_MAPIO_REQUEUE;
> > +		else
> > +			return DM_MAPIO_DELAY_REQUEUE;
> >  	}
> 
> This patch is inferior to what I posted because this patch does not avoid
> the delay if multiple LUNs are associated with the same SCSI host. Consider
> e.g. the following configuration:
> * A single SCSI host with two SCSI LUNs associated to that host, e.g. /dev/sda
>   and /dev/sdb.
> * A dm-mpath instance has been created on top of /dev/sda.
> If all tags are in use by requests queued to /dev/sdb, no dm requests are in
> progress and a request is submitted against the dm-mpath device then the
> blk_get_request(q, GFP_ATOMIC) call will fail. The request will be requeued
> and the queue will be rerun after a delay.
> 
> My patch does not introduce a delay in this case.

That delay may not matter because SCHED_RESTART will run queue just
after one request is completed.

There is at least one issue with get_request(GFP_NOIO): AIO
performance regression may not be caused, or even AIO may not
be possible. For example, user runs fio(libaio, randread, single
job, queue depth: 64, device: dm-mpath disk), if get_request(GFP_NOIO)
often blocks because of shared tags or out of tag, the actual queue
depth won't reach 64 at all, and may be just 1 in the worst case.
Once the actual queue depth is decreased much, random I/O performance
should be hurt a lot.

-- 
Ming

Re: [PATCH] dm-mpath: return DM_MAPIO_REQUEUE in case of rq allocation failure

[Bart Van Assche]

T24gU2F0LCAyMDE3LTA5LTIzIGF0IDAxOjQ0ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gT24g
RnJpLCBTZXAgMjIsIDIwMTcgYXQgMDM6MDY6MTZQTSArMDAwMCwgQmFydCBWYW4gQXNzY2hlIHdy
b3RlOg0KPiA+IE9uIEZyaSwgMjAxNy0wOS0yMiBhdCAwOTozNSArMDgwMCwgTWluZyBMZWkgd3Jv
dGU6DQo+ID4gPiArCQkvKg0KPiA+ID4gKwkJICogYmxrLW1xJ3MgU0NIRURfUkVTVEFSVCBjYW4g
Y292ZXIgdGhpcyByZXF1ZXVlLCBzbw0KPiA+ID4gKwkJICogd2UgbmVlZG4ndCB0byBkZWFsIHdp
dGggaXQgYnkgREVMQVlfUkVRVUVVRS4gTW9yZQ0KPiA+ID4gKwkJICogaW1wb3J0YW50bHksIHdl
IGhhdmUgdG8gcmV0dXJuIERNX01BUElPX1JFUVVFVUUNCj4gPiA+ICsJCSAqIHNvIHRoYXQgYmxr
LW1xIGNhbiBnZXQgdGhlIHF1ZXVlIGJ1c3kgZmVlZGJhY2ssDQo+ID4gPiArCQkgKiBvdGhlcndp
c2UgSS9PIG1lcmdlIGNhbiBiZSBodXJ0Lg0KPiA+ID4gKwkJICovDQo+ID4gPiArCQlpZiAocS0+
bXFfb3BzKQ0KPiA+ID4gKwkJCXJldHVybiBETV9NQVBJT19SRVFVRVVFOw0KPiA+ID4gKwkJZWxz
ZQ0KPiA+ID4gKwkJCXJldHVybiBETV9NQVBJT19ERUxBWV9SRVFVRVVFOw0KPiA+ID4gIAl9DQo+
ID4gDQo+ID4gVGhpcyBwYXRjaCBpcyBpbmZlcmlvciB0byB3aGF0IEkgcG9zdGVkIGJlY2F1c2Ug
dGhpcyBwYXRjaCBkb2VzIG5vdCBhdm9pZA0KPiA+IHRoZSBkZWxheSBpZiBtdWx0aXBsZSBMVU5z
IGFyZSBhc3NvY2lhdGVkIHdpdGggdGhlIHNhbWUgU0NTSSBob3N0LiBDb25zaWRlcg0KPiA+IGUu
Zy4gdGhlIGZvbGxvd2luZyBjb25maWd1cmF0aW9uOg0KPiA+ICogQSBzaW5nbGUgU0NTSSBob3N0
IHdpdGggdHdvIFNDU0kgTFVOcyBhc3NvY2lhdGVkIHRvIHRoYXQgaG9zdCwgZS5nLiAvZGV2L3Nk
YQ0KPiA+ICAgYW5kIC9kZXYvc2RiLg0KPiA+ICogQSBkbS1tcGF0aCBpbnN0YW5jZSBoYXMgYmVl
biBjcmVhdGVkIG9uIHRvcCBvZiAvZGV2L3NkYS4NCj4gPiBJZiBhbGwgdGFncyBhcmUgaW4gdXNl
IGJ5IHJlcXVlc3RzIHF1ZXVlZCB0byAvZGV2L3NkYiwgbm8gZG0gcmVxdWVzdHMgYXJlIGluDQo+
ID4gcHJvZ3Jlc3MgYW5kIGEgcmVxdWVzdCBpcyBzdWJtaXR0ZWQgYWdhaW5zdCB0aGUgZG0tbXBh
dGggZGV2aWNlIHRoZW4gdGhlDQo+ID4gYmxrX2dldF9yZXF1ZXN0KHEsIEdGUF9BVE9NSUMpIGNh
bGwgd2lsbCBmYWlsLiBUaGUgcmVxdWVzdCB3aWxsIGJlIHJlcXVldWVkDQo+ID4gYW5kIHRoZSBx
dWV1ZSB3aWxsIGJlIHJlcnVuIGFmdGVyIGEgZGVsYXkuDQo+ID4gDQo+ID4gTXkgcGF0Y2ggZG9l
cyBub3QgaW50cm9kdWNlIGEgZGVsYXkgaW4gdGhpcyBjYXNlLg0KPiANCj4gVGhhdCBkZWxheSBt
YXkgbm90IG1hdHRlciBiZWNhdXNlIFNDSEVEX1JFU1RBUlQgd2lsbCBydW4gcXVldWUganVzdA0K
PiBhZnRlciBvbmUgcmVxdWVzdCBpcyBjb21wbGV0ZWQuDQoNCkRpZCB5b3UgdW5kZXJzdGFuZCB3
aGF0IEkgd3JvdGU/IFNDSEVEX1JFU1RBUlQgd2lsbCBiZSBzZXQgZm9yIC9kZXYvc2RiIGJ1dCBu
b3QNCmZvciB0aGUgZG0gcXVldWUuIFRoYXQncyB3aGF0IEkgd2FzIHRyeWluZyB0byBleHBsYWlu
IHRvIHlvdSBpbiBteSBwcmV2aW91cyBlLW1haWwuDQoNCj4gVGhlcmUgaXMgYXQgbGVhc3Qgb25l
IGlzc3VlIHdpdGggZ2V0X3JlcXVlc3QoR0ZQX05PSU8pOiBBSU8NCj4gcGVyZm9ybWFuY2UgcmVn
cmVzc2lvbiBtYXkgbm90IGJlIGNhdXNlZCwgb3IgZXZlbiBBSU8gbWF5IG5vdA0KPiBiZSBwb3Nz
aWJsZS4gRm9yIGV4YW1wbGUsIHVzZXIgcnVucyBmaW8obGliYWlvLCByYW5kcmVhZCwgc2luZ2xl
DQo+IGpvYiwgcXVldWUgZGVwdGg6IDY0LCBkZXZpY2U6IGRtLW1wYXRoIGRpc2spLCBpZiBnZXRf
cmVxdWVzdChHRlBfTk9JTykNCj4gb2Z0ZW4gYmxvY2tzIGJlY2F1c2Ugb2Ygc2hhcmVkIHRhZ3Mg
b3Igb3V0IG9mIHRhZywgdGhlIGFjdHVhbCBxdWV1ZQ0KPiBkZXB0aCB3b24ndCByZWFjaCA2NCBh
dCBhbGwsIGFuZCBtYXkgYmUganVzdCAxIGluIHRoZSB3b3JzdCBjYXNlLg0KPiBPbmNlIHRoZSBh
Y3R1YWwgcXVldWUgZGVwdGggaXMgZGVjcmVhc2VkIG11Y2gsIHJhbmRvbSBJL08gcGVyZm9ybWFu
Y2UNCj4gc2hvdWxkIGJlIGh1cnQgYSBsb3QuDQoNClRoYXQncyB3aHkgd2UgbmVlZCB0byBtb2Rp
Znkgc2NzaV9sbGRfYnVzeSgpLiBJZiBzY3NpX2xsZF9idXN5KCkgd2lsbCBiZQ0KbW9kaWZpZWQg
YXMgSSBwcm9wb3NlZCBpbiBhIHByZXZpb3VzIGUtbWFpbCB0aGVuIGl0IHdpbGwgYmVjb21lIHZl
cnkNCnVubGlrZWx5IHRoYXQgbm8gdGFnIGlzIGF2YWlsYWJsZSB3aGVuIGJsa19nZXRfcmVxdWVz
dCgpIGlzIGNhbGxlZC4gV2l0aCB0aGF0DQpzY3NpX2xsZF9idXN5KCkgbW9kaWZpY2F0aW9uIGl0
IGlzIGV2ZW4gcG9zc2libGUgdGhhdCB3ZSBkb24ndCBuZWVkIHRvIG1vZGlmeQ0KdGhlIGRtLW1w
YXRoIGRyaXZlci4NCg0KQmFydC4=

Re: [PATCH] dm-mpath: return DM_MAPIO_REQUEUE in case of rq allocation failure

[Ming Lei]

On Fri, Sep 22, 2017 at 05:54:48PM +0000, Bart Van Assche wrote:
> On Sat, 2017-09-23 at 01:44 +0800, Ming Lei wrote:
> > On Fri, Sep 22, 2017 at 03:06:16PM +0000, Bart Van Assche wrote:
> > > On Fri, 2017-09-22 at 09:35 +0800, Ming Lei wrote:
> > > > +		/*
> > > > +		 * blk-mq's SCHED_RESTART can cover this requeue, so
> > > > +		 * we needn't to deal with it by DELAY_REQUEUE. More
> > > > +		 * importantly, we have to return DM_MAPIO_REQUEUE
> > > > +		 * so that blk-mq can get the queue busy feedback,
> > > > +		 * otherwise I/O merge can be hurt.
> > > > +		 */
> > > > +		if (q->mq_ops)
> > > > +			return DM_MAPIO_REQUEUE;
> > > > +		else
> > > > +			return DM_MAPIO_DELAY_REQUEUE;
> > > >  	}
> > > 
> > > This patch is inferior to what I posted because this patch does not avoid
> > > the delay if multiple LUNs are associated with the same SCSI host. Consider
> > > e.g. the following configuration:
> > > * A single SCSI host with two SCSI LUNs associated to that host, e.g. /dev/sda
> > >   and /dev/sdb.
> > > * A dm-mpath instance has been created on top of /dev/sda.
> > > If all tags are in use by requests queued to /dev/sdb, no dm requests are in
> > > progress and a request is submitted against the dm-mpath device then the
> > > blk_get_request(q, GFP_ATOMIC) call will fail. The request will be requeued
> > > and the queue will be rerun after a delay.
> > > 
> > > My patch does not introduce a delay in this case.
> > 
> > That delay may not matter because SCHED_RESTART will run queue just
> > after one request is completed.
> 
> Did you understand what I wrote? SCHED_RESTART will be set for /dev/sdb but not
> for the dm queue. That's what I was trying to explain to you in my previous e-mail.

The patch I posted in this thread will set SCHED_RESTART for dm queue.

> 
> > There is at least one issue with get_request(GFP_NOIO): AIO
> > performance regression may not be caused, or even AIO may not
> > be possible. For example, user runs fio(libaio, randread, single
> > job, queue depth: 64, device: dm-mpath disk), if get_request(GFP_NOIO)
> > often blocks because of shared tags or out of tag, the actual queue
> > depth won't reach 64 at all, and may be just 1 in the worst case.
> > Once the actual queue depth is decreased much, random I/O performance
> > should be hurt a lot.
> 
> That's why we need to modify scsi_lld_busy(). If scsi_lld_busy() will be
> modified as I proposed in a previous e-mail then it will become very
> unlikely that no tag is available when blk_get_request() is called. With that
> scsi_lld_busy() modification it is even possible that we don't need to modify
> the dm-mpath driver.

Then post out a whole solution, and I'd like to take a look and test.

-- 
Ming

Re: [PATCH] dm-mpath: return DM_MAPIO_REQUEUE in case of rq allocation failure

[Bart Van Assche]

T24gTW9uLCAyMDE3LTA5LTI1IGF0IDExOjA2ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gT24g
RnJpLCBTZXAgMjIsIDIwMTcgYXQgMDU6NTQ6NDhQTSArMDAwMCwgQmFydCBWYW4gQXNzY2hlIHdy
b3RlOg0KPiA+IE9uIFNhdCwgMjAxNy0wOS0yMyBhdCAwMTo0NCArMDgwMCwgTWluZyBMZWkgd3Jv
dGU6DQo+ID4gPiBPbiBGcmksIFNlcCAyMiwgMjAxNyBhdCAwMzowNjoxNlBNICswMDAwLCBCYXJ0
IFZhbiBBc3NjaGUgd3JvdGU6DQo+ID4gPiA+IE9uIEZyaSwgMjAxNy0wOS0yMiBhdCAwOTozNSAr
MDgwMCwgTWluZyBMZWkgd3JvdGU6DQo+ID4gPiA+ID4gKwkJLyoNCj4gPiA+ID4gPiArCQkgKiBi
bGstbXEncyBTQ0hFRF9SRVNUQVJUIGNhbiBjb3ZlciB0aGlzIHJlcXVldWUsIHNvDQo+ID4gPiA+
ID4gKwkJICogd2UgbmVlZG4ndCB0byBkZWFsIHdpdGggaXQgYnkgREVMQVlfUkVRVUVVRS4gTW9y
ZQ0KPiA+ID4gPiA+ICsJCSAqIGltcG9ydGFudGx5LCB3ZSBoYXZlIHRvIHJldHVybiBETV9NQVBJ
T19SRVFVRVVFDQo+ID4gPiA+ID4gKwkJICogc28gdGhhdCBibGstbXEgY2FuIGdldCB0aGUgcXVl
dWUgYnVzeSBmZWVkYmFjaywNCj4gPiA+ID4gPiArCQkgKiBvdGhlcndpc2UgSS9PIG1lcmdlIGNh
biBiZSBodXJ0Lg0KPiA+ID4gPiA+ICsJCSAqLw0KPiA+ID4gPiA+ICsJCWlmIChxLT5tcV9vcHMp
DQo+ID4gPiA+ID4gKwkJCXJldHVybiBETV9NQVBJT19SRVFVRVVFOw0KPiA+ID4gPiA+ICsJCWVs
c2UNCj4gPiA+ID4gPiArCQkJcmV0dXJuIERNX01BUElPX0RFTEFZX1JFUVVFVUU7DQo+ID4gPiA+
ID4gIAl9DQo+ID4gPiA+IA0KPiA+ID4gPiBUaGlzIHBhdGNoIGlzIGluZmVyaW9yIHRvIHdoYXQg
SSBwb3N0ZWQgYmVjYXVzZSB0aGlzIHBhdGNoIGRvZXMgbm90IGF2b2lkDQo+ID4gPiA+IHRoZSBk
ZWxheSBpZiBtdWx0aXBsZSBMVU5zIGFyZSBhc3NvY2lhdGVkIHdpdGggdGhlIHNhbWUgU0NTSSBo
b3N0LiBDb25zaWRlcg0KPiA+ID4gPiBlLmcuIHRoZSBmb2xsb3dpbmcgY29uZmlndXJhdGlvbjoN
Cj4gPiA+ID4gKiBBIHNpbmdsZSBTQ1NJIGhvc3Qgd2l0aCB0d28gU0NTSSBMVU5zIGFzc29jaWF0
ZWQgdG8gdGhhdCBob3N0LCBlLmcuIC9kZXYvc2RhDQo+ID4gPiA+ICAgYW5kIC9kZXYvc2RiLg0K
PiA+ID4gPiAqIEEgZG0tbXBhdGggaW5zdGFuY2UgaGFzIGJlZW4gY3JlYXRlZCBvbiB0b3Agb2Yg
L2Rldi9zZGEuDQo+ID4gPiA+IElmIGFsbCB0YWdzIGFyZSBpbiB1c2UgYnkgcmVxdWVzdHMgcXVl
dWVkIHRvIC9kZXYvc2RiLCBubyBkbSByZXF1ZXN0cyBhcmUgaW4NCj4gPiA+ID4gcHJvZ3Jlc3Mg
YW5kIGEgcmVxdWVzdCBpcyBzdWJtaXR0ZWQgYWdhaW5zdCB0aGUgZG0tbXBhdGggZGV2aWNlIHRo
ZW4gdGhlDQo+ID4gPiA+IGJsa19nZXRfcmVxdWVzdChxLCBHRlBfQVRPTUlDKSBjYWxsIHdpbGwg
ZmFpbC4gVGhlIHJlcXVlc3Qgd2lsbCBiZSByZXF1ZXVlZA0KPiA+ID4gPiBhbmQgdGhlIHF1ZXVl
IHdpbGwgYmUgcmVydW4gYWZ0ZXIgYSBkZWxheS4NCj4gPiA+ID4gDQo+ID4gPiA+IE15IHBhdGNo
IGRvZXMgbm90IGludHJvZHVjZSBhIGRlbGF5IGluIHRoaXMgY2FzZS4NCj4gPiA+IA0KPiA+ID4g
VGhhdCBkZWxheSBtYXkgbm90IG1hdHRlciBiZWNhdXNlIFNDSEVEX1JFU1RBUlQgd2lsbCBydW4g
cXVldWUganVzdA0KPiA+ID4gYWZ0ZXIgb25lIHJlcXVlc3QgaXMgY29tcGxldGVkLg0KPiA+IA0K
PiA+IERpZCB5b3UgdW5kZXJzdGFuZCB3aGF0IEkgd3JvdGU/IFNDSEVEX1JFU1RBUlQgd2lsbCBi
ZSBzZXQgZm9yIC9kZXYvc2RiIGJ1dCBub3QNCj4gPiBmb3IgdGhlIGRtIHF1ZXVlLiBUaGF0J3Mg
d2hhdCBJIHdhcyB0cnlpbmcgdG8gZXhwbGFpbiB0byB5b3UgaW4gbXkgcHJldmlvdXMgZS1tYWls
Lg0KPiANCj4gVGhlIHBhdGNoIEkgcG9zdGVkIGluIHRoaXMgdGhyZWFkIHdpbGwgc2V0IFNDSEVE
X1JFU1RBUlQgZm9yIGRtIHF1ZXVlLg0KDQpUaGlzIGlzIG5vdCBob3cgY29tbXVuaWNhdGlvbiBv
biBhbiBvcGVuIHNvdXJjZSBtYWlsaW5nIGxpc3QgaXMgYXNzdW1lZCB0byB3b3JrLg0KSWYgeW91
IGtub3cgdGhhdCB5b3UgYXJlIHdyb25nIHlvdSBhcmUgYXNzdW1lZCBlaXRoZXIgdG8gc2h1dCB1
cCBvciB0byBhZG1pdCBpdC4NCkFuZCBpZiB5b3UgZGlzYWdyZWUgd2l0aCB0aGUgZGV0YWlsZWQg
ZXhwbGFuYXRpb24gSSBnYXZlIHlvdSBhcmUgYXNzdW1lZCB0bw0KZXhwbGFpbiBpbiBkZXRhaWwg
d2h5IHlvdSB0aGluayBpdCBpcyB3cm9uZy4NCg0KPiA+ID4gVGhlcmUgaXMgYXQgbGVhc3Qgb25l
IGlzc3VlIHdpdGggZ2V0X3JlcXVlc3QoR0ZQX05PSU8pOiBBSU8NCj4gPiA+IHBlcmZvcm1hbmNl
IHJlZ3Jlc3Npb24gbWF5IG5vdCBiZSBjYXVzZWQsIG9yIGV2ZW4gQUlPIG1heSBub3QNCj4gPiA+
IGJlIHBvc3NpYmxlLiBGb3IgZXhhbXBsZSwgdXNlciBydW5zIGZpbyhsaWJhaW8sIHJhbmRyZWFk
LCBzaW5nbGUNCj4gPiA+IGpvYiwgcXVldWUgZGVwdGg6IDY0LCBkZXZpY2U6IGRtLW1wYXRoIGRp
c2spLCBpZiBnZXRfcmVxdWVzdChHRlBfTk9JTykNCj4gPiA+IG9mdGVuIGJsb2NrcyBiZWNhdXNl
IG9mIHNoYXJlZCB0YWdzIG9yIG91dCBvZiB0YWcsIHRoZSBhY3R1YWwgcXVldWUNCj4gPiA+IGRl
cHRoIHdvbid0IHJlYWNoIDY0IGF0IGFsbCwgYW5kIG1heSBiZSBqdXN0IDEgaW4gdGhlIHdvcnN0
IGNhc2UuDQo+ID4gPiBPbmNlIHRoZSBhY3R1YWwgcXVldWUgZGVwdGggaXMgZGVjcmVhc2VkIG11
Y2gsIHJhbmRvbSBJL08gcGVyZm9ybWFuY2UNCj4gPiA+IHNob3VsZCBiZSBodXJ0IGEgbG90Lg0K
PiA+IA0KPiA+IFRoYXQncyB3aHkgd2UgbmVlZCB0byBtb2RpZnkgc2NzaV9sbGRfYnVzeSgpLiBJ
ZiBzY3NpX2xsZF9idXN5KCkgd2lsbCBiZQ0KPiA+IG1vZGlmaWVkIGFzIEkgcHJvcG9zZWQgaW4g
YSBwcmV2aW91cyBlLW1haWwgdGhlbiBpdCB3aWxsIGJlY29tZSB2ZXJ5DQo+ID4gdW5saWtlbHkg
dGhhdCBubyB0YWcgaXMgYXZhaWxhYmxlIHdoZW4gYmxrX2dldF9yZXF1ZXN0KCkgaXMgY2FsbGVk
LiBXaXRoIHRoYXQNCj4gPiBzY3NpX2xsZF9idXN5KCkgbW9kaWZpY2F0aW9uIGl0IGlzIGV2ZW4g
cG9zc2libGUgdGhhdCB3ZSBkb24ndCBuZWVkIHRvIG1vZGlmeQ0KPiA+IHRoZSBkbS1tcGF0aCBk
cml2ZXIuDQo+IA0KPiBUaGVuIHBvc3Qgb3V0IGEgd2hvbGUgc29sdXRpb24sIGFuZCBJJ2QgbGlr
ZSB0byB0YWtlIGEgbG9vayBhbmQgdGVzdC4NCg0KSSB3aWxsIGRvIHRoYXQgYXMgc29vbiBhcyBJ
IGhhdmUgdGhlIHRpbWUgdG8gcnVuIHNvbWUgbWVhc3VyZW1lbnRzLiBUaGUgcmVzdCBvZg0KdGhp
cyB3ZWVrIEkgd2lsbCBiZSB0cmF2ZWxpbmcuDQoNCkJhcnQu

Re: [PATCH] dm-mpath: return DM_MAPIO_REQUEUE in case of rq allocation failure

[Ming Lei]

On Mon, Sep 25, 2017 at 03:23:16PM +0000, Bart Van Assche wrote:
> On Mon, 2017-09-25 at 11:06 +0800, Ming Lei wrote:
> > On Fri, Sep 22, 2017 at 05:54:48PM +0000, Bart Van Assche wrote:
> > > On Sat, 2017-09-23 at 01:44 +0800, Ming Lei wrote:
> > > > On Fri, Sep 22, 2017 at 03:06:16PM +0000, Bart Van Assche wrote:
> > > > > On Fri, 2017-09-22 at 09:35 +0800, Ming Lei wrote:
> > > > > > +		/*
> > > > > > +		 * blk-mq's SCHED_RESTART can cover this requeue, so
> > > > > > +		 * we needn't to deal with it by DELAY_REQUEUE. More
> > > > > > +		 * importantly, we have to return DM_MAPIO_REQUEUE
> > > > > > +		 * so that blk-mq can get the queue busy feedback,
> > > > > > +		 * otherwise I/O merge can be hurt.
> > > > > > +		 */
> > > > > > +		if (q->mq_ops)
> > > > > > +			return DM_MAPIO_REQUEUE;
> > > > > > +		else
> > > > > > +			return DM_MAPIO_DELAY_REQUEUE;
> > > > > >  	}
> > > > > 
> > > > > This patch is inferior to what I posted because this patch does not avoid
> > > > > the delay if multiple LUNs are associated with the same SCSI host. Consider
> > > > > e.g. the following configuration:
> > > > > * A single SCSI host with two SCSI LUNs associated to that host, e.g. /dev/sda
> > > > >   and /dev/sdb.
> > > > > * A dm-mpath instance has been created on top of /dev/sda.
> > > > > If all tags are in use by requests queued to /dev/sdb, no dm requests are in
> > > > > progress and a request is submitted against the dm-mpath device then the
> > > > > blk_get_request(q, GFP_ATOMIC) call will fail. The request will be requeued
> > > > > and the queue will be rerun after a delay.
> > > > > 
> > > > > My patch does not introduce a delay in this case.
> > > > 
> > > > That delay may not matter because SCHED_RESTART will run queue just
> > > > after one request is completed.
> > > 
> > > Did you understand what I wrote? SCHED_RESTART will be set for /dev/sdb but not
> > > for the dm queue. That's what I was trying to explain to you in my previous e-mail.
> > 
> > The patch I posted in this thread will set SCHED_RESTART for dm queue.
> 
> This is not how communication on an open source mailing list is assumed to work.
> If you know that you are wrong you are assumed either to shut up or to admit it.

You just mentioned 'This patch is inferior' and never explained my patch
is wrong, so please go ahead and show me why this patch(the post in this
thread, also the following link) is wrong.

	https://marc.info/?l=linux-block&m=150604412910113&w=2

I admit both are two ways for the issue, but I don't think my patch
is wrong. Your approach can be a very big change because .queue_rq()
will block, and I also mentioned it might cause AIO regression.

I don't understand why you say there is communication issue since
no much discussion yet in this thread.

Please see the whole discussion:

	https://marc.info/?t=150604442500001&r=1&w=2

> And if you disagree with the detailed explanation I gave you are assumed to
> explain in detail why you think it is wrong.


-- 
Ming

Re: dm-mpath: return DM_MAPIO_REQUEUE in case of rq allocation failure

[Mike Snitzer]

On Mon, Sep 25 2017 at 12:10pm -0400,
Ming Lei <ming.lei@redhat.com> wrote:

> On Mon, Sep 25, 2017 at 03:23:16PM +0000, Bart Van Assche wrote:
> > On Mon, 2017-09-25 at 11:06 +0800, Ming Lei wrote:
> > > On Fri, Sep 22, 2017 at 05:54:48PM +0000, Bart Van Assche wrote:
> > > > On Sat, 2017-09-23 at 01:44 +0800, Ming Lei wrote:
> > > > > On Fri, Sep 22, 2017 at 03:06:16PM +0000, Bart Van Assche wrote:
> > > > > > On Fri, 2017-09-22 at 09:35 +0800, Ming Lei wrote:
> > > > > > > +		/*
> > > > > > > +		 * blk-mq's SCHED_RESTART can cover this requeue, so
> > > > > > > +		 * we needn't to deal with it by DELAY_REQUEUE. More
> > > > > > > +		 * importantly, we have to return DM_MAPIO_REQUEUE
> > > > > > > +		 * so that blk-mq can get the queue busy feedback,
> > > > > > > +		 * otherwise I/O merge can be hurt.
> > > > > > > +		 */
> > > > > > > +		if (q->mq_ops)
> > > > > > > +			return DM_MAPIO_REQUEUE;
> > > > > > > +		else
> > > > > > > +			return DM_MAPIO_DELAY_REQUEUE;
> > > > > > >  	}
> > > > > > 
> > > > > > This patch is inferior to what I posted because this patch does not avoid
> > > > > > the delay if multiple LUNs are associated with the same SCSI host. Consider
> > > > > > e.g. the following configuration:
> > > > > > * A single SCSI host with two SCSI LUNs associated to that host, e.g. /dev/sda
> > > > > >   and /dev/sdb.
> > > > > > * A dm-mpath instance has been created on top of /dev/sda.
> > > > > > If all tags are in use by requests queued to /dev/sdb, no dm requests are in
> > > > > > progress and a request is submitted against the dm-mpath device then the
> > > > > > blk_get_request(q, GFP_ATOMIC) call will fail. The request will be requeued
> > > > > > and the queue will be rerun after a delay.
> > > > > > 
> > > > > > My patch does not introduce a delay in this case.
> > > > > 
> > > > > That delay may not matter because SCHED_RESTART will run queue just
> > > > > after one request is completed.
> > > > 
> > > > Did you understand what I wrote? SCHED_RESTART will be set for /dev/sdb but not
> > > > for the dm queue. That's what I was trying to explain to you in my previous e-mail.
> > > 
> > > The patch I posted in this thread will set SCHED_RESTART for dm queue.
> > 
> > This is not how communication on an open source mailing list is assumed to work.
> > If you know that you are wrong you are assumed either to shut up or to admit it.

Code speaks much better than unnecessarily caustic exchanges.  Not sure
why Bart persists with that.. but that's for him to sort out.
 
> You just mentioned 'This patch is inferior' and never explained my patch
> is wrong, so please go ahead and show me why this patch(the post in this
> thread, also the following link) is wrong.
> 
> 	https://marc.info/?l=linux-block&m=150604412910113&w=2
> 
> I admit both are two ways for the issue, but I don't think my patch
> is wrong. Your approach can be a very big change because .queue_rq()
> will block, and I also mentioned it might cause AIO regression.

I have no interest in changing DM multipath to block in .queue_rq()
So please consider that approach a dead end.

Ming, just iterate on your revised patchset, test and post when you're
happy with it.

Thanks,
Mike

Re: dm-mpath: return DM_MAPIO_REQUEUE in case of rq allocation failure

[Ming Lei]

On Mon, Sep 25, 2017 at 12:17:03PM -0400, Mike Snitzer wrote:
> On Mon, Sep 25 2017 at 12:10pm -0400,
> Ming Lei <ming.lei@redhat.com> wrote:
> 
> > On Mon, Sep 25, 2017 at 03:23:16PM +0000, Bart Van Assche wrote:
> > > On Mon, 2017-09-25 at 11:06 +0800, Ming Lei wrote:
> > > > On Fri, Sep 22, 2017 at 05:54:48PM +0000, Bart Van Assche wrote:
> > > > > On Sat, 2017-09-23 at 01:44 +0800, Ming Lei wrote:
> > > > > > On Fri, Sep 22, 2017 at 03:06:16PM +0000, Bart Van Assche wrote:
> > > > > > > On Fri, 2017-09-22 at 09:35 +0800, Ming Lei wrote:
> > > > > > > > +		/*
> > > > > > > > +		 * blk-mq's SCHED_RESTART can cover this requeue, so
> > > > > > > > +		 * we needn't to deal with it by DELAY_REQUEUE. More
> > > > > > > > +		 * importantly, we have to return DM_MAPIO_REQUEUE
> > > > > > > > +		 * so that blk-mq can get the queue busy feedback,
> > > > > > > > +		 * otherwise I/O merge can be hurt.
> > > > > > > > +		 */
> > > > > > > > +		if (q->mq_ops)
> > > > > > > > +			return DM_MAPIO_REQUEUE;
> > > > > > > > +		else
> > > > > > > > +			return DM_MAPIO_DELAY_REQUEUE;
> > > > > > > >  	}
> > > > > > > 
> > > > > > > This patch is inferior to what I posted because this patch does not avoid
> > > > > > > the delay if multiple LUNs are associated with the same SCSI host. Consider
> > > > > > > e.g. the following configuration:
> > > > > > > * A single SCSI host with two SCSI LUNs associated to that host, e.g. /dev/sda
> > > > > > >   and /dev/sdb.
> > > > > > > * A dm-mpath instance has been created on top of /dev/sda.
> > > > > > > If all tags are in use by requests queued to /dev/sdb, no dm requests are in
> > > > > > > progress and a request is submitted against the dm-mpath device then the
> > > > > > > blk_get_request(q, GFP_ATOMIC) call will fail. The request will be requeued
> > > > > > > and the queue will be rerun after a delay.
> > > > > > > 
> > > > > > > My patch does not introduce a delay in this case.
> > > > > > 
> > > > > > That delay may not matter because SCHED_RESTART will run queue just
> > > > > > after one request is completed.
> > > > > 
> > > > > Did you understand what I wrote? SCHED_RESTART will be set for /dev/sdb but not
> > > > > for the dm queue. That's what I was trying to explain to you in my previous e-mail.
> > > > 
> > > > The patch I posted in this thread will set SCHED_RESTART for dm queue.
> > > 
> > > This is not how communication on an open source mailing list is assumed to work.
> > > If you know that you are wrong you are assumed either to shut up or to admit it.
> 
> Code speaks much better than unnecessarily caustic exchanges.  Not sure
> why Bart persists with that.. but that's for him to sort out.
>  
> > You just mentioned 'This patch is inferior' and never explained my patch
> > is wrong, so please go ahead and show me why this patch(the post in this
> > thread, also the following link) is wrong.
> > 
> > 	https://marc.info/?l=linux-block&m=150604412910113&w=2
> > 
> > I admit both are two ways for the issue, but I don't think my patch
> > is wrong. Your approach can be a very big change because .queue_rq()
> > will block, and I also mentioned it might cause AIO regression.
> 
> I have no interest in changing DM multipath to block in .queue_rq()
> So please consider that approach a dead end.
> 
> Ming, just iterate on your revised patchset, test and post when you're
> happy with it.

Hi Mike,

If you don't consider to change mpath into block in .queue_rq() now,
please take this patch first, and I am sure this way is correct, and
even it can be thought as a fix.


Thanks,
Ming

Re: dm-mpath: return DM_MAPIO_REQUEUE in case of rq allocation failure

[Bart Van Assche]

T24gVHVlLCAyMDE3LTA5LTI2IGF0IDE2OjUwICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gSWYg
eW91IGRvbid0IGNvbnNpZGVyIHRvIGNoYW5nZSBtcGF0aCBpbnRvIGJsb2NrIGluIC5xdWV1ZV9y
cSgpIG5vdywNCj4gcGxlYXNlIHRha2UgdGhpcyBwYXRjaCBmaXJzdCwgYW5kIEkgYW0gc3VyZSB0
aGlzIHdheSBpcyBjb3JyZWN0LCBhbmQNCj4gZXZlbiBpdCBjYW4gYmUgdGhvdWdodCBhcyBhIGZp
eC4NCg0KQXMgSSBleHBsYWluZWQgZWFybGllciwgaXQgd291bGQgYmUgd3JvbmcgdG8gdGFrZSB0
aGlzIHBhdGNoIHVwc3RyZWFtIHdpdGhvdXQNCmhhdmluZyB0ZXN0ZWQgYW5kIG1lYXN1cmVkIHRo
ZSBhbHRlcm5hdGl2ZSBJIGhhZCBwcm9wb3NlZCBmaXJzdA0KKGh0dHBzOi8vd3d3LnNwaW5pY3Mu
bmV0L2xpc3RzL2RtLWRldmVsL21zZzMyMjgyLmh0bWwpLiBNeSBhbHRlcm5hdGl2ZSBoYW5kbGVz
DQpTQ1NJIGhvc3RzIHdpdGggbXVsdGlwbGUgTFVOcyBwcm9wZXJseSBidXQgdGhlIHBhdGNoIGF0
IHRoZSBzdGFydCBvZiB0aGlzDQplLW1haWwgdGhyZWFkIG5vdC4NCg0KQmFydC4=