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>,
"axboe@fb.com" <axboe@fb.com>,
"ming.lei@redhat.com" <ming.lei@redhat.com>
Cc: "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
"osandov@fb.com" <osandov@fb.com>,
"john.garry@huawei.com" <john.garry@huawei.com>,
"loberman@redhat.com" <loberman@redhat.com>
Subject: Re: [PATCH] SCSI: don't get target/host busy_count in scsi_mq_get_budget()
Date: Mon, 6 Nov 2017 18:04:42 +0000 [thread overview]
Message-ID: <1509991480.2409.54.camel@wdc.com> (raw)
In-Reply-To: <20171104015534.32684-1-ming.lei@redhat.com>
T24gU2F0LCAyMDE3LTExLTA0IGF0IDA5OjU1ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gSXQg
aXMgdmVyeSBleHBlbnNpdmUgdG8gYXRvbWljX2luYy9hdG9taWNfZGVjIHRoZSBob3N0IHdpZGUg
Y291bnRlciBvZg0KPiBob3N0LT5idXN5X2NvdW50LCBhbmQgaXQgc2hvdWxkIGhhdmUgYmVlbiBh
dm9pZGVkIHZpYSBibGstbXEncyBtZWNoYW5pc20NCj4gb2YgZ2V0dGluZyBkcml2ZXIgdGFnLCB3
aGljaCB1c2VzIHRoZSBtb3JlIGVmZmljaWVudCB3YXkgb2Ygc2JpdG1hcCBxdWV1ZS4NCg0KRGlk
IHlvdSBwZXJoYXBzIG1lYW4gdGhlIGhvc3QtPmhvc3RfYnVzeSBjb3VudGVyPyBVbnJlbGF0ZWQg
dG8gdGhpcyBwYXRjaDoNCkkgdGhpbmsgdGhhdCBjb21taXQgNjRkNTEzYWMzMWJkICgic2NzaTog
dXNlIGhvc3Qgd2lkZSB0YWdzIGJ5IGRlZmF1bHQiKSBtYWRlDQp0aGF0IGNvdW50ZXIgc3VwZXJm
bHVvdXMuDQoNCj4gQWxzbyB3ZSBkb24ndCBjaGVjayBhdG9taWNfcmVhZCgmc2Rldi0+ZGV2aWNl
X2J1c3kpIGluIHNjc2lfbXFfZ2V0X2J1ZGdldCgpDQo+IGFuZCBkb24ndCBydW4gcXVldWUgaWYg
dGhlIGNvdW50ZXIgYmVjb21lcyB6ZXJvLCBzbyBJTyBoYW5nIG1heSBiZSBjYXVzZWQNCj4gaWYg
YWxsIHJlcXVlc3RzIGFyZSBjb21wbGV0ZWQganVzdCBiZWZvcmUgdGhlIGN1cnJlbnQgU0NTSSBk
ZXZpY2UNCj4gaXMgYWRkZWQgdG8gc2hvc3QtPnN0YXJ2ZWRfbGlzdC4NCg0KV2hhdCBpcyB0aGUg
cmVsYXRpb25zaGlwIGJldHdlZW4gdGhlIGFib3ZlIGRlc2NyaXB0aW9uIGFuZCB0aGUgY29kZSBj
aGFuZ2VzDQpiZWxvdz8gVGhlIGFib3ZlIGRlc2NyaXB0aW9uIGRvZXMgbm90IGV4cGxhaW4gd2hl
dGhlciB0aGUgc2NzaV9tcV9nZXQvcHV0X2J1ZGdldCgpDQpjaGFuZ2VzIGJlbG93IHByZXZlbnQg
dGhhdCBhbGwgb3V0c3RhbmRpbmcgU0NTSSByZXF1ZXN0cyBjb21wbGV0ZSBiZWZvcmUNCmFub3Ro
ZXIgcXVldWUgaXMgYWRkZWQgdG8gdGhlIHN0YXJ2ZWQgbGlzdC4NCg0KSXMgdGhpcyBwZXJoYXBz
IGFuIGF0dGVtcHQgdG8gbW92ZSB0aGUgY29kZSB0aGF0IGNhbiBhZGQgYSByZXF1ZXN0IHF1ZXVl
IHRvDQoic3RhcnZlZF9saXN0IiBmcm9tIGluc2lkZSBzY3NpX21xX2dldF9idWRnZXQoKSBpbnRv
IHNjc2lfcXVldWVfcnEoKT8gRG9lcw0KdGhpcyBwYXRjaCBtb3JlIHRoYW4gcmVkdWNpbmcgdGhl
IHByb2JhYmlsaXR5IHRoYXQgdGhlIHJhY2UgaXMgZW5jb3VudGVyZWQNCnRoYXQgYSBxdWV1ZSBp
cyBhZGRlZCB0byAic3RhcnZlZF9saXN0IiBhZnRlciBhbGwgcmVxdWVzdHMgaGF2ZSBmaW5pc2hl
ZD8NCg0KPiBGaXhlczogMGRmMjFjODZiZGJmKHNjc2k6IGltcGxlbWVudCAuZ2V0X2J1ZGdldCBh
bmQgLnB1dF9idWRnZXQgZm9yIGJsay1tcSkNCj4gUmVwb3J0ZWQtYnk6IEJhcnQgVmFuIEFzc2No
ZSA8YmFydC52YW5hc3NjaGVAd2RjLmNvbT4NCj4gU2lnbmVkLW9mZi1ieTogTWluZyBMZWkgPG1p
bmcubGVpQHJlZGhhdC5jb20+DQoNClNpbmNlIHRoaXMgaXMgYSBTQ1NJIHBhdGNoIHRoZSBTQ1NJ
IG1haW50YWluZXIsIE1hcnRpbiBQZXRlcnNlbiwgc2hvdWxkIGhhdmUNCmJlZW4gQ2MtZWQgZm9y
IHRoaXMgcGF0Y2guIEFkZGl0aW9uYWxseSwgSSB0aGluayB0aGF0IHRoaXMgcGF0Y2ggc2hvdWxk
IG5vdA0KaGF2ZSBiZWVuIHF1ZXVlZCBieSBKZW5zIGJlZm9yZSBNYXJ0aW4gaGFkIGFwcHJvdmVk
IHRoaXMgcGF0Y2guDQoNCj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvc2NzaS9zY3NpX2xpYi5jIGIv
ZHJpdmVycy9zY3NpL3Njc2lfbGliLmMNCj4gaW5kZXggYTA5OGFmMzA3MGExLi43ZjIxOGVmNjE5
MDAgMTAwNjQ0DQo+IC0tLSBhL2RyaXZlcnMvc2NzaS9zY3NpX2xpYi5jDQo+ICsrKyBiL2RyaXZl
cnMvc2NzaS9zY3NpX2xpYi5jDQo+IEBAIC0xOTQ0LDExICsxOTQ0LDcgQEAgc3RhdGljIHZvaWQg
c2NzaV9tcV9wdXRfYnVkZ2V0KHN0cnVjdCBibGtfbXFfaHdfY3R4ICpoY3R4KQ0KPiAgew0KPiAg
CXN0cnVjdCByZXF1ZXN0X3F1ZXVlICpxID0gaGN0eC0+cXVldWU7DQo+ICAJc3RydWN0IHNjc2lf
ZGV2aWNlICpzZGV2ID0gcS0+cXVldWVkYXRhOw0KPiAtCXN0cnVjdCBTY3NpX0hvc3QgKnNob3N0
ID0gc2Rldi0+aG9zdDsNCj4gIA0KPiAtCWF0b21pY19kZWMoJnNob3N0LT5ob3N0X2J1c3kpOw0K
PiAtCWlmIChzY3NpX3RhcmdldChzZGV2KS0+Y2FuX3F1ZXVlID4gMCkNCj4gLQkJYXRvbWljX2Rl
Yygmc2NzaV90YXJnZXQoc2RldiktPnRhcmdldF9idXN5KTsNCj4gIAlhdG9taWNfZGVjKCZzZGV2
LT5kZXZpY2VfYnVzeSk7DQo+ICAJcHV0X2RldmljZSgmc2Rldi0+c2Rldl9nZW5kZXYpOw0KPiAg
fQ0KDQpzY3NpX21xX2dldC9wdXRfYnVkZ2V0KCkgd2VyZSBpbnRyb2R1Y2VkIHRvIGltcHJvdmUg
c2VxdWVudGlhbCBJL08NCnBlcmZvcm1hbmNlLiBEb2VzIHJlbW92aW5nIHRoZSBTQ1NJIHRhcmdl
dCBidXN5IGNoZWNrIGhhdmUgYSBuZWdhdGl2ZQ0KcGVyZm9ybWFuY2Ugb24gc2VxdWVudGlhbCBJ
L08gZm9yIGUuZy4gdGhlIGlTRVIgaW5pdGlhdG9yIGRyaXZlcj8gVGhlIGlTQ1NJDQpvdmVyIFRD
UCBpbml0aWF0b3IgZHJpdmVyIGlzIG5vdCBhcHByb3ByaWF0ZSBmb3IgdGVzdGluZyBwZXJmb3Jt
YW5jZQ0KcmVncmVzc2lvbnMgYmVjYXVzZSBpdCBsaW1pdHMgdGhlIGlTQ1NJIHBhcmFtZXRlciBN
YXhPdXRzdGFuZGluZ1IyVCB0byBvbmUuDQoNCkJhcnQu
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>,
"axboe@fb.com" <axboe@fb.com>,
"ming.lei@redhat.com" <ming.lei@redhat.com>
Cc: "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
"osandov@fb.com" <osandov@fb.com>,
"john.garry@huawei.com" <john.garry@huawei.com>,
"loberman@redhat.com" <loberman@redhat.com>
Subject: Re: [PATCH] SCSI: don't get target/host busy_count in scsi_mq_get_budget()
Date: Mon, 6 Nov 2017 18:04:42 +0000 [thread overview]
Message-ID: <1509991480.2409.54.camel@wdc.com> (raw)
In-Reply-To: <20171104015534.32684-1-ming.lei@redhat.com>
On Sat, 2017-11-04 at 09:55 +0800, Ming Lei wrote:
> It is very expensive to atomic_inc/atomic_dec the host wide counter of
> host->busy_count, and it should have been avoided via blk-mq's mechanism
> of getting driver tag, which uses the more efficient way of sbitmap queue.
Did you perhaps mean the host->host_busy counter? Unrelated to this patch:
I think that commit 64d513ac31bd ("scsi: use host wide tags by default") made
that counter superfluous.
> Also we don't check atomic_read(&sdev->device_busy) in scsi_mq_get_budget()
> and don't run queue if the counter becomes zero, so IO hang may be caused
> if all requests are completed just before the current SCSI device
> is added to shost->starved_list.
What is the relationship between the above description and the code changes
below? The above description does not explain whether the scsi_mq_get/put_budget()
changes below prevent that all outstanding SCSI requests complete before
another queue is added to the starved list.
Is this perhaps an attempt to move the code that can add a request queue to
"starved_list" from inside scsi_mq_get_budget() into scsi_queue_rq()? Does
this patch more than reducing the probability that the race is encountered
that a queue is added to "starved_list" after all requests have finished?
> Fixes: 0df21c86bdbf(scsi: implement .get_budget and .put_budget for blk-mq)
> Reported-by: Bart Van Assche <bart.vanassche@wdc.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
Since this is a SCSI patch the SCSI maintainer, Martin Petersen, should have
been Cc-ed for this patch. Additionally, I think that this patch should not
have been queued by Jens before Martin had approved this patch.
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index a098af3070a1..7f218ef61900 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1944,11 +1944,7 @@ static void scsi_mq_put_budget(struct blk_mq_hw_ctx *hctx)
> {
> struct request_queue *q = hctx->queue;
> struct scsi_device *sdev = q->queuedata;
> - struct Scsi_Host *shost = sdev->host;
>
> - atomic_dec(&shost->host_busy);
> - if (scsi_target(sdev)->can_queue > 0)
> - atomic_dec(&scsi_target(sdev)->target_busy);
> atomic_dec(&sdev->device_busy);
> put_device(&sdev->sdev_gendev);
> }
scsi_mq_get/put_budget() were introduced to improve sequential I/O
performance. Does removing the SCSI target busy check have a negative
performance on sequential I/O for e.g. the iSER initiator driver? The iSCSI
over TCP initiator driver is not appropriate for testing performance
regressions because it limits the iSCSI parameter MaxOutstandingR2T to one.
Bart.
next prev parent reply other threads:[~2017-11-06 18:04 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-04 1:55 [PATCH] SCSI: don't get target/host busy_count in scsi_mq_get_budget() Ming Lei
2017-11-04 14:19 ` Jens Axboe
2017-11-06 19:45 ` Bart Van Assche
2017-11-06 19:45 ` Bart Van Assche
2017-11-07 2:11 ` Ming Lei
2017-11-07 16:20 ` Bart Van Assche
2017-11-07 16:20 ` Bart Van Assche
2017-11-07 16:29 ` Jens Axboe
2017-11-07 17:10 ` Jens Axboe
2017-11-07 17:36 ` Jens Axboe
2017-11-07 22:06 ` Jens Axboe
2017-11-07 22:34 ` Bart Van Assche
2017-11-07 22:34 ` Bart Van Assche
2017-11-07 22:39 ` Jens Axboe
2017-11-08 0:50 ` Ming Lei
2017-11-08 1:03 ` Ming Lei
2017-11-08 3:01 ` Jens Axboe
2017-11-08 3:12 ` Ming Lei
2017-11-08 3:17 ` Jens Axboe
2017-11-08 3:17 ` Jens Axboe
2017-11-08 6:20 ` Ming Lei
2017-11-08 15:59 ` Ming Lei
2017-11-08 18:19 ` Jens Axboe
2017-11-07 17:34 ` Bart Van Assche
2017-11-07 17:34 ` Bart Van Assche
2017-11-08 0:53 ` Ming Lei
2017-11-08 2:06 ` Ming Lei
2017-11-08 0:39 ` Ming Lei
2017-11-08 2:55 ` Jens Axboe
2017-11-08 2:58 ` Ming Lei
2017-11-08 3:06 ` Jens Axboe
2017-11-08 16:41 ` Bart Van Assche
2017-11-08 16:41 ` Bart Van Assche
2017-11-08 17:57 ` Jens Axboe
2017-11-08 18:22 ` Laurence Oberman
2017-11-08 18:28 ` Jens Axboe
2017-11-09 4:02 ` Ming Lei
2017-11-09 2:05 ` Ming Lei
2017-11-07 10:15 ` Ming Lei
2017-11-07 16:17 ` Bart Van Assche
2017-11-07 16:17 ` Bart Van Assche
2017-11-08 3:12 ` Jens Axboe
2017-11-06 18:04 ` Bart Van Assche [this message]
2017-11-06 18:04 ` Bart Van Assche
2017-11-07 2:19 ` Ming Lei
2017-11-07 3:53 ` Martin K. Petersen
2017-11-07 3:53 ` Martin K. Petersen
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=1509991480.2409.54.camel@wdc.com \
--to=bart.vanassche@wdc.com \
--cc=axboe@fb.com \
--cc=hch@infradead.org \
--cc=john.garry@huawei.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=loberman@redhat.com \
--cc=ming.lei@redhat.com \
--cc=osandov@fb.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.