From: Bart Van Assche <Bart.VanAssche@wdc.com>
To: "hch@infradead.org" <hch@infradead.org>,
"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
"snitzer@redhat.com" <snitzer@redhat.com>,
"ming.lei@redhat.com" <ming.lei@redhat.com>,
"axboe@kernel.dk" <axboe@kernel.dk>
Cc: "dm-devel@redhat.com" <dm-devel@redhat.com>,
"jejb@linux.vnet.ibm.com" <jejb@linux.vnet.ibm.com>,
"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
"martin.petersen@oracle.com" <martin.petersen@oracle.com>,
"loberman@redhat.com" <loberman@redhat.com>
Subject: Re: [PATCH 1/5] blk-mq: introduce BLK_STS_DEV_RESOURCE
Date: Mon, 22 Jan 2018 16:49:54 +0000 [thread overview]
Message-ID: <1516639793.2545.14.camel@sandisk.com> (raw)
In-Reply-To: <20180122033550.27855-2-ming.lei@redhat.com>
T24gTW9uLCAyMDE4LTAxLTIyIGF0IDExOjM1ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gQEAg
LTEyODAsMTAgKzEyODIsMTggQEAgYm9vbCBibGtfbXFfZGlzcGF0Y2hfcnFfbGlzdChzdHJ1Y3Qg
cmVxdWVzdF9xdWV1ZSAqcSwgc3RydWN0IGxpc3RfaGVhZCAqbGlzdCwNCj4gIAkJICogLSBTb21l
IGJ1dCBub3QgYWxsIGJsb2NrIGRyaXZlcnMgc3RvcCBhIHF1ZXVlIGJlZm9yZQ0KPiAgCQkgKiAg
IHJldHVybmluZyBCTEtfU1RTX1JFU09VUkNFLiBUd28gZXhjZXB0aW9ucyBhcmUgc2NzaS1tcQ0K
PiAgCQkgKiAgIGFuZCBkbS1ycS4NCj4gKwkJICoNCj4gKwkJICogSWYgZHJpdmVycyByZXR1cm4g
QkxLX1NUU19SRVNPVVJDRSBhbmQgU19TQ0hFRF9SRVNUQVJUDQo+ICsJCSAqIGJpdCBpcyBzZXQs
IHJ1biBxdWV1ZSBhZnRlciAxMG1zIGZvciBhdm9pZGluZyBJTyBoYW5nDQo+ICsJCSAqIGJlY2F1
c2UgdGhlIHF1ZXVlIG1heSBiZSBpZGxlIGFuZCB0aGUgUkVTVEFSVCBtZWNoYW5pc20NCj4gKwkJ
ICogY2FuJ3Qgd29yayBhbnkgbW9yZS4NCj4gIAkJICovDQo+IC0JCWlmICghYmxrX21xX3NjaGVk
X25lZWRzX3Jlc3RhcnQoaGN0eCkgfHwNCj4gKwkJbmVlZHNfcmVzdGFydCA9IGJsa19tcV9zY2hl
ZF9uZWVkc19yZXN0YXJ0KGhjdHgpOw0KPiArCQlpZiAoIW5lZWRzX3Jlc3RhcnQgfHwNCj4gIAkJ
ICAgIChub190YWcgJiYgbGlzdF9lbXB0eV9jYXJlZnVsKCZoY3R4LT5kaXNwYXRjaF93YWl0LmVu
dHJ5KSkpDQo+ICAJCQlibGtfbXFfcnVuX2h3X3F1ZXVlKGhjdHgsIHRydWUpOw0KPiArCQllbHNl
IGlmIChuZWVkc19yZXN0YXJ0ICYmIChyZXQgPT0gQkxLX1NUU19SRVNPVVJDRSkpDQo+ICsJCQli
bGtfbXFfZGVsYXlfcnVuX2h3X3F1ZXVlKGhjdHgsIDEwKTsNCj4gIAl9DQoNCkluIG15IG9waW5p
b24gdGhlcmUgYXJlIHR3byBwcm9ibGVtcyB3aXRoIHRoZSBhYm92ZSBjaGFuZ2VzOg0KKiBPbmx5
IHRoZSBibG9jayBkcml2ZXIgYXV0aG9yIGNhbiBrbm93IHdoYXQgYSBnb29kIGNob2ljZSBpcyBm
b3IgdGhlIHRpbWUNCiAgYWZ0ZXIgd2hpY2ggdG8gcmVydW4gdGhlIHF1ZXVlLiBTbyBJIHRoaW5r
IG1vdmluZyB0aGUgcmVydW4gZGVsYXkgKDEwIG1zKQ0KICBjb25zdGFudCBmcm9tIGJsb2NrIGRy
aXZlcnMgaW50byB0aGUgY29yZSBpcyBhIHN0ZXAgYmFja3dhcmRzIGluc3RlYWQgb2YgYQ0KICBz
dGVwIGZvcndhcmRzLg0KKiBUaGUgcHVycG9zZSBvZiB0aGUgQkxLX01RX1NfU0NIRURfUkVTVEFS
VCBmbGFnIGlzIHRvIGRldGVjdCB3aGV0aGVyIG9yIG5vdA0KICBhbnkgb2YgdGhlIHF1ZXVlIHJ1
bnMgdHJpZ2dlcmVkIGJ5IGZyZWVpbmcgYSB0YWcgaGFwcGVuZWQgY29uY3VycmVudGx5LiBJDQog
IGRvbid0IHRoaW5rIHRoYXQgdGhlcmUgaXMgYW55IHJlbGF0aW9uc2hpcCBiZXR3ZWVuIHF1ZXVl
IHJ1bnMgaGFwcGVuaW5nIGFsbA0KICBvciBub3QgY29uY3VycmVudGx5IGFuZCB0aGUgY2hhbmNl
IHRoYXQgZHJpdmVyIHJlc291cmNlcyBiZWNvbWUgYXZhaWxhYmxlLg0KICBTbyBkZWNpZGluZyB3
aGV0aGVyIG9yIG5vdCBhIHF1ZXVlIHNob3VsZCBiZSByZXJ1biBiYXNlZCBvbiB0aGUgdmFsdWUg
b2YNCiAgdGhlIEJMS19NUV9TX1NDSEVEX1JFU1RBUlQgZmxhZyBzZWVtcyB3cm9uZyB0byBtZS4N
Cg0KPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9zY3NpL3Njc2lfbGliLmMgYi9kcml2ZXJzL3Njc2kv
c2NzaV9saWIuYw0KPiBpbmRleCBkOWNhMWRmYWIxNTQuLjU1YmUyNTUwYzU1NSAxMDA2NDQNCj4g
LS0tIGEvZHJpdmVycy9zY3NpL3Njc2lfbGliLmMNCj4gKysrIGIvZHJpdmVycy9zY3NpL3Njc2lf
bGliLmMNCj4gQEAgLTIwMzAsOSArMjAzMCw5IEBAIHN0YXRpYyBibGtfc3RhdHVzX3Qgc2NzaV9x
dWV1ZV9ycShzdHJ1Y3QgYmxrX21xX2h3X2N0eCAqaGN0eCwNCj4gIAljYXNlIEJMS19TVFNfT0s6
DQo+ICAJCWJyZWFrOw0KPiAgCWNhc2UgQkxLX1NUU19SRVNPVVJDRToNCj4gLQkJaWYgKGF0b21p
Y19yZWFkKCZzZGV2LT5kZXZpY2VfYnVzeSkgPT0gMCAmJg0KPiAtCQkgICAgIXNjc2lfZGV2aWNl
X2Jsb2NrZWQoc2RldikpDQo+IC0JCQlibGtfbXFfZGVsYXlfcnVuX2h3X3F1ZXVlKGhjdHgsIFND
U0lfUVVFVUVfREVMQVkpOw0KPiArCQlpZiAoYXRvbWljX3JlYWQoJnNkZXYtPmRldmljZV9idXN5
KSB8fA0KPiArCQkgICAgc2NzaV9kZXZpY2VfYmxvY2tlZChzZGV2KSkNCj4gKwkJCXJldCA9IEJM
S19TVFNfREVWX1JFU09VUkNFOw0KPiAgCQlicmVhazsNCj4gIAlkZWZhdWx0Og0KPiAgCQkvKg0K
DQpUaGUgYWJvdmUgaW50cm9kdWNlcyB0d28gY2hhbmdlcyB0aGF0IGhhdmUgbm90IGJlZW4gbWVu
dGlvbmVkIGluIHRoZQ0KZGVzY3JpcHRpb24gb2YgdGhpcyBwYXRjaDoNCi0gVGhlIHF1ZXVlIHJl
cnVubmluZyBkZWxheSBpcyBjaGFuZ2VkIGZyb20gMyBtcyBpbnRvIDEwIG1zLiBXaGVyZSBpcyB0
aGUNCiAgZXhwbGFuYXRpb24gb2YgdGhpcyBjaGFuZ2U/IERvZXMgdGhpcyBjaGFuZ2UgaGF2ZSBh
IHBvc2l0aXZlIG9yIG5lZ2F0aXZlDQogIHBlcmZvcm1hbmNlIGltcGFjdD8NCi0gVGhlIGFib3Zl
IG1vZGlmaWVzIGEgZ3VhcmFudGVlZCBxdWV1ZSByZXJ1biBpbnRvIGEgcXVldWUgcmVydW4gdGhh
dA0KICBtYXkgb3IgbWF5IG5vdCBoYXBwZW4sIGRlcGVuZGluZyBvbiB3aGV0aGVyIG9yIG5vdCBt
dWx0aXBsZSB0YWdzIGdldCBmcmVlZA0KICBjb25jdXJyZW50bHkgKHJldHVybiBCTEtfU1RTX0RF
Vl9SRVNPVVJDRSkuIFNvcnJ5IGJ1dCBJIHRoaW5rIHRoYXQncyB3cm9uZy4NCg0KQmFydC4=
WARNING: multiple messages have this Message-ID (diff)
From: Bart Van Assche <Bart.VanAssche@wdc.com>
To: "hch@infradead.org" <hch@infradead.org>,
"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
"snitzer@redhat.com" <snitzer@redhat.com>,
"ming.lei@redhat.com" <ming.lei@redhat.com>,
"axboe@kernel.dk" <axboe@kernel.dk>
Cc: "dm-devel@redhat.com" <dm-devel@redhat.com>,
"jejb@linux.vnet.ibm.com" <jejb@linux.vnet.ibm.com>,
"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
"martin.petersen@oracle.com" <martin.petersen@oracle.com>,
"loberman@redhat.com" <loberman@redhat.com>
Subject: Re: [PATCH 1/5] blk-mq: introduce BLK_STS_DEV_RESOURCE
Date: Mon, 22 Jan 2018 16:49:54 +0000 [thread overview]
Message-ID: <1516639793.2545.14.camel@sandisk.com> (raw)
In-Reply-To: <20180122033550.27855-2-ming.lei@redhat.com>
On Mon, 2018-01-22 at 11:35 +0800, Ming Lei wrote:
> @@ -1280,10 +1282,18 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
> * - Some but not all block drivers stop a queue before
> * returning BLK_STS_RESOURCE. Two exceptions are scsi-mq
> * and dm-rq.
> + *
> + * If drivers return BLK_STS_RESOURCE and S_SCHED_RESTART
> + * bit is set, run queue after 10ms for avoiding IO hang
> + * because the queue may be idle and the RESTART mechanism
> + * can't work any more.
> */
> - if (!blk_mq_sched_needs_restart(hctx) ||
> + needs_restart = blk_mq_sched_needs_restart(hctx);
> + if (!needs_restart ||
> (no_tag && list_empty_careful(&hctx->dispatch_wait.entry)))
> blk_mq_run_hw_queue(hctx, true);
> + else if (needs_restart && (ret == BLK_STS_RESOURCE))
> + blk_mq_delay_run_hw_queue(hctx, 10);
> }
In my opinion there are two problems with the above changes:
* Only the block driver author can know what a good choice is for the time
after which to rerun the queue. So I think moving the rerun delay (10 ms)
constant from block drivers into the core is a step backwards instead of a
step forwards.
* The purpose of the BLK_MQ_S_SCHED_RESTART flag is to detect whether or not
any of the queue runs triggered by freeing a tag happened concurrently. I
don't think that there is any relationship between queue runs happening all
or not concurrently and the chance that driver resources become available.
So deciding whether or not a queue should be rerun based on the value of
the BLK_MQ_S_SCHED_RESTART flag seems wrong to me.
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index d9ca1dfab154..55be2550c555 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2030,9 +2030,9 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
> case BLK_STS_OK:
> break;
> case BLK_STS_RESOURCE:
> - if (atomic_read(&sdev->device_busy) == 0 &&
> - !scsi_device_blocked(sdev))
> - blk_mq_delay_run_hw_queue(hctx, SCSI_QUEUE_DELAY);
> + if (atomic_read(&sdev->device_busy) ||
> + scsi_device_blocked(sdev))
> + ret = BLK_STS_DEV_RESOURCE;
> break;
> default:
> /*
The above introduces two changes that have not been mentioned in the
description of this patch:
- The queue rerunning delay is changed from 3 ms into 10 ms. Where is the
explanation of this change? Does this change have a positive or negative
performance impact?
- The above modifies a guaranteed queue rerun into a queue rerun that
may or may not happen, depending on whether or not multiple tags get freed
concurrently (return BLK_STS_DEV_RESOURCE). Sorry but I think that's wrong.
Bart.
next prev parent reply other threads:[~2018-01-22 16:49 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-22 3:35 [PATCH 0/5] blk-mq & dm: fix IO hang and deal with one performance issue Ming Lei
2018-01-22 3:35 ` [PATCH 1/5] blk-mq: introduce BLK_STS_DEV_RESOURCE Ming Lei
2018-01-22 16:32 ` Christoph Hellwig
2018-01-22 16:49 ` Bart Van Assche [this message]
2018-01-22 16:49 ` Bart Van Assche
2018-01-23 0:57 ` Ming Lei
2018-01-23 16:17 ` Bart Van Assche
2018-01-23 16:26 ` Ming Lei
2018-01-23 16:37 ` Bart Van Assche
2018-01-23 16:41 ` Ming Lei
2018-01-23 16:47 ` Bart Van Assche
2018-01-23 16:47 ` Bart Van Assche
2018-01-23 16:49 ` Ming Lei
2018-01-23 16:54 ` Bart Van Assche
2018-01-23 16:54 ` Bart Van Assche
2018-01-23 16:59 ` Ming Lei
2018-01-23 16:59 ` Ming Lei
2018-01-23 22:01 ` Bart Van Assche
2018-01-23 22:01 ` Bart Van Assche
2018-01-24 2:31 ` Ming Lei
2018-01-22 3:35 ` [PATCH 2/5] dm-rq: handle dispatch exception in dm_dispatch_clone_request() Ming Lei
2018-01-22 3:35 ` Ming Lei
2018-01-22 3:35 ` [PATCH 3/5] dm-rq: return BLK_STS_* from map_request() Ming Lei
2018-01-22 3:35 ` Ming Lei
2018-01-22 5:35 ` Ming Lei
2018-01-22 3:35 ` [PATCH 4/5] blk-mq: introduce blk_get_request_notify Ming Lei
2018-01-22 10:19 ` Ming Lei
2018-01-22 17:13 ` Bart Van Assche
2018-01-22 17:13 ` Bart Van Assche
2018-01-23 1:29 ` Ming Lei
2018-01-22 3:35 ` [PATCH 5/5] dm-mpath: use blk_mq_alloc_request_notify for allocating blk-mq req Ming Lei
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1516639793.2545.14.camel@sandisk.com \
--to=bart.vanassche@wdc.com \
--cc=axboe@kernel.dk \
--cc=dm-devel@redhat.com \
--cc=hch@infradead.org \
--cc=jejb@linux.vnet.ibm.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=loberman@redhat.com \
--cc=martin.petersen@oracle.com \
--cc=ming.lei@redhat.com \
--cc=snitzer@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.