From: Bart Van Assche <Bart.VanAssche@wdc.com>
To: "ming.lei@redhat.com" <ming.lei@redhat.com>
Cc: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
"hch@infradead.org" <hch@infradead.org>,
"jthumshirn@suse.de" <jthumshirn@suse.de>,
"martin.petersen@oracle.com" <martin.petersen@oracle.com>,
"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
"axboe@fb.com" <axboe@fb.com>,
"oleksandr@natalenko.name" <oleksandr@natalenko.name>,
"jejb@linux.vnet.ibm.com" <jejb@linux.vnet.ibm.com>,
"tj@kernel.org" <tj@kernel.org>
Subject: Re: [PATCH V3 7/8] block: allow to allocate req with REQF_PREEMPT when queue is preempt frozen
Date: Mon, 4 Sep 2017 15:40:35 +0000 [thread overview]
Message-ID: <1504539634.3189.15.camel@wdc.com> (raw)
In-Reply-To: <20170904071653.GD7959@ming.t460p>
T24gTW9uLCAyMDE3LTA5LTA0IGF0IDE1OjE2ICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gT24g
TW9uLCBTZXAgMDQsIDIwMTcgYXQgMDQ6MTM6MjZBTSArMDAwMCwgQmFydCBWYW4gQXNzY2hlIHdy
b3RlOg0KPiA+IEFsbG93aW5nIGJsa19nZXRfcmVxdWVzdCgpIHRvIHN1Y2NlZWQgYWZ0ZXIgdGhl
IERZSU5HIGZsYWcgaGFzIGJlZW4gc2V0IGlzDQo+ID4gY29tcGxldGVseSB3cm9uZyBiZWNhdXNl
IHRoYXQgY291bGQgcmVzdWx0IGluIGEgcmVxdWVzdCBiZWluZyBxdWV1ZWQgYWZ0ZXINCj4gPiB0
aGUgREVBRCBmbGFnIGhhcyBiZWVuIHNldCwgcmVzdWx0aW5nIGluIGVpdGhlciBhIGhhbmdpbmcg
cmVxdWVzdCBvciBhIGtlcm5lbA0KPiA+IGNyYXNoLiBUaGlzIGlzIHdoeSBpdCdzIGNvbXBsZXRl
bHkgd3JvbmcgdG8gYWRkIGEgYmxrX3F1ZXVlX2VudGVyX2xpdmUoKSBjYWxsDQo+ID4gaW4gYmxr
X29sZF9nZXRfcmVxdWVzdCgpIG9yIGJsa19tcV9hbGxvY19yZXF1ZXN0KCkuIEhlbmNlIG15IE5B
SyBmb3IgYW55DQo+ID4gcGF0Y2ggdGhhdCBhZGRzIGEgYmxrX3F1ZXVlX2VudGVyX2xpdmUoKSBj
YWxsIHRvIGFueSBmdW5jdGlvbiBjYWxsZWQgZnJvbQ0KPiA+IGJsa19nZXRfcmVxdWVzdCgpLiBU
aGF0IGluY2x1ZGVzIHRoZSBwYXRjaCBhdCB0aGUgc3RhcnQgb2YgdGhpcyBlLW1haWwgdGhyZWFk
Lg0KPg0KPiBTZWUgYWJvdmUsIHRoaXMgcGF0Y2ggY2hhbmdlcyBub3RoaW5nIGFib3V0IHRoaXMg
ZmFjdCwgcGxlYXNlIGxvb2sgYXQNCj4gdGhlIHBhdGNoIGNhcmVmdWxseSBuZXh0IHRpbWUganVz
dCBiZWZvcmUgcG9zdGluZyB5b3VyIGxvbmcgY29tbWVudC4NCg0KQXJlIHlvdSByZWFsbHkgc3Vy
ZSB0aGF0IHlvdXIgcGF0Y2ggZG9lcyBub3QgYWxsb3cgYmxrX2dldF9yZXF1ZXN0KCkgdG8NCnN1
Y2NlZWQgYWZ0ZXIgdGhlIERZSU5HIGZsYWcgaGFzIGJlZW4gc2V0PyBibGtfbXFfYWxsb2NfcmVx
dWVzdCgpIGNhbGxzIGJvdGgNCmJsa19xdWV1ZV9pc19wcmVlbXB0X2Zyb3plbigpIGFuZCBibGtf
cXVldWVfZW50ZXJfbGl2ZSgpIHdpdGhvdXQgaG9sZGluZw0KYW55IGxvY2suIEEgdGhyZWFkIHRo
YXQgaXMgcnVubmluZyBjb25jdXJyZW50bHkgd2l0aCBibGtfbXFfZ2V0X3JlcXVlc3QoKQ0KY2Fu
IHVuZnJlZXplIHRoZSBxdWV1ZSBhZnRlciBibGtfcXVldWVfaXNfcHJlZW1wdF9mcm96ZW4oKSBy
ZXR1cm5lZCBhbmQNCmJlZm9yZSBibGtfcXVldWVfZW50ZXJfbGl2ZSgpIGlzIGNhbGxlZC4gVGhp
cyBtZWFucyB0aGF0IHdpdGggeW91ciBwYXRjaA0Kc2VyaWVzIGFwcGxpZWQgYmxrX2dldF9yZXF1
ZXN0KCkgY2FuIHN1Y2NlZWQgYWZ0ZXIgdGhlIERZSU5HIGZsYWcgaGFzIGJlZW4NCnNldCwgd2hp
Y2ggaXMgc29tZXRoaW5nIHdlIGRvbid0IHdhbnQuIEFkZGl0aW9uYWxseSwgSSBkb24ndCB0aGlu
ayB3ZSB3YW50DQp0byBpbnRyb2R1Y2UgYW55IGtpbmQgb2YgbG9ja2luZyBpbiBibGtfbXFfZ2V0
X3JlcXVlc3QoKSBiZWNhdXNlIHRoYXQgd291bGQNCmJlIGEgc2VyaWFsaXphdGlvbiBwb2ludC4N
Cg0KSGF2ZSB5b3UgY29uc2lkZXJlZCB0byB1c2UgdGhlIGJsay1tcSAicmVzZXJ2ZWQgcmVxdWVz
dCIgbWVjaGFuaXNtIHRvIGF2b2lkDQpzdGFydmF0aW9uIG9mIHBvd2VyIG1hbmFnZW1lbnQgcmVx
dWVzdHMgaW5zdGVhZCBvZiBtYWtpbmcgdGhlIGJsb2NrIGxheWVyDQpldmVuIG1vcmUgY29tcGxp
Y2F0ZWQgdGhhbiBpdCBhbHJlYWR5IGlzPw0KDQpOb3RlOiBleHRlbmRpbmcgYmxrX21xX2ZyZWV6
ZS91bmZyZWV6ZV9xdWV1ZSgpIHRvIHRoZSBsZWdhY3kgYmxvY2sgbGF5ZXINCmNvdWxkIGJlIHVz
ZWZ1bCB0byBtYWtlIHNjc2lfd2FpdF9mb3JfcXVldWVjb21tYW5kKCkgbW9yZSBlbGVnYW50LiBI
b3dldmVyLA0KSSBkb24ndCB0aGluayB3ZSBzaG91bGQgc3BlbmQgb3VyIHRpbWUgb24gbGVnYWN5
IGJsb2NrIGxheWVyIC8gU0NTSSBjb3JlDQpjaGFuZ2VzLiBUaGUgY29kZSBJJ20gcmVmZXJyaW5n
IHRvIGlzIHRoZSBmb2xsb3dpbmc6DQoNCi8qKg0KICogc2NzaV93YWl0X2Zvcl9xdWV1ZWNvbW1h
bmQoKSAtIHdhaXQgZm9yIG9uZ29pbmcgcXVldWVjb21tYW5kKCkgY2FsbHMNCiAqIEBzZGV2OiBT
Q1NJIGRldmljZSBwb2ludGVyLg0KICoNCiAqIFdhaXQgdW50aWwgdGhlIG9uZ29pbmcgc2hvc3Qt
Pmhvc3R0LT5xdWV1ZWNvbW1hbmQoKSBjYWxscyB0aGF0IGFyZQ0KICogaW52b2tlZCBmcm9tIHNj
c2lfcmVxdWVzdF9mbigpIGhhdmUgZmluaXNoZWQuDQogKi8NCnN0YXRpYyB2b2lkIHNjc2lfd2Fp
dF9mb3JfcXVldWVjb21tYW5kKHN0cnVjdCBzY3NpX2RldmljZSAqc2RldikNCnsNCglXQVJOX09O
X09OQ0Uoc2Rldi0+aG9zdC0+dXNlX2Jsa19tcSk7DQoNCgl3aGlsZSAoc2NzaV9yZXF1ZXN0X2Zu
X2FjdGl2ZShzZGV2KSkNCgkJbXNsZWVwKDIwKTsNCn0NCg0KQmFydC4=
WARNING: multiple messages have this Message-ID (diff)
From: Bart Van Assche <Bart.VanAssche@wdc.com>
To: "ming.lei@redhat.com" <ming.lei@redhat.com>
Cc: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
"hch@infradead.org" <hch@infradead.org>,
"jthumshirn@suse.de" <jthumshirn@suse.de>,
"martin.petersen@oracle.com" <martin.petersen@oracle.com>,
"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
"axboe@fb.com" <axboe@fb.com>,
"oleksandr@natalenko.name" <oleksandr@natalenko.name>,
"jejb@linux.vnet.ibm.com" <jejb@linux.vnet.ibm.com>,
"tj@kernel.org" <tj@kernel.org>
Subject: Re: [PATCH V3 7/8] block: allow to allocate req with REQF_PREEMPT when queue is preempt frozen
Date: Mon, 4 Sep 2017 15:40:35 +0000 [thread overview]
Message-ID: <1504539634.3189.15.camel@wdc.com> (raw)
In-Reply-To: <20170904071653.GD7959@ming.t460p>
On Mon, 2017-09-04 at 15:16 +0800, Ming Lei wrote:
> On Mon, Sep 04, 2017 at 04:13:26AM +0000, Bart Van Assche wrote:
> > Allowing blk_get_request() to succeed after the DYING flag has been set is
> > completely wrong because that could result in a request being queued after
> > the DEAD flag has been set, resulting in either a hanging request or a kernel
> > crash. This is why it's completely wrong to add a blk_queue_enter_live() call
> > in blk_old_get_request() or blk_mq_alloc_request(). Hence my NAK for any
> > patch that adds a blk_queue_enter_live() call to any function called from
> > blk_get_request(). That includes the patch at the start of this e-mail thread.
>
> See above, this patch changes nothing about this fact, please look at
> the patch carefully next time just before posting your long comment.
Are you really sure that your patch does not allow blk_get_request() to
succeed after the DYING flag has been set? blk_mq_alloc_request() calls both
blk_queue_is_preempt_frozen() and blk_queue_enter_live() without holding
any lock. A thread that is running concurrently with blk_mq_get_request()
can unfreeze the queue after blk_queue_is_preempt_frozen() returned and
before blk_queue_enter_live() is called. This means that with your patch
series applied blk_get_request() can succeed after the DYING flag has been
set, which is something we don't want. Additionally, I don't think we want
to introduce any kind of locking in blk_mq_get_request() because that would
be a serialization point.
Have you considered to use the blk-mq "reserved request" mechanism to avoid
starvation of power management requests instead of making the block layer
even more complicated than it already is?
Note: extending blk_mq_freeze/unfreeze_queue() to the legacy block layer
could be useful to make scsi_wait_for_queuecommand() more elegant. However,
I don't think we should spend our time on legacy block layer / SCSI core
changes. The code I'm referring to is the following:
/**
* scsi_wait_for_queuecommand() - wait for ongoing queuecommand() calls
* @sdev: SCSI device pointer.
*
* Wait until the ongoing shost->hostt->queuecommand() calls that are
* invoked from scsi_request_fn() have finished.
*/
static void scsi_wait_for_queuecommand(struct scsi_device *sdev)
{
WARN_ON_ONCE(sdev->host->use_blk_mq);
while (scsi_request_fn_active(sdev))
msleep(20);
}
Bart.
next prev parent reply other threads:[~2017-09-04 15:40 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-02 13:08 [PATCH V3 0/8] block/scsi: safe SCSI quiescing Ming Lei
2017-09-02 13:08 ` [PATCH V3 1/8] blk-mq: rename blk_mq_unfreeze_queue as blk_unfreeze_queue Ming Lei
2017-09-02 13:08 ` [PATCH V3 2/8] blk-mq: rename blk_mq_freeze_queue as blk_freeze_queue Ming Lei
2017-09-02 13:08 ` [PATCH V3 3/8] blk-mq: only run hw queues for blk-mq Ming Lei
2017-09-02 13:08 ` [PATCH V3 4/8] blk-mq: rename blk_mq_freeze_queue_wait as blk_freeze_queue_wait Ming Lei
2017-09-02 13:08 ` [PATCH V3 5/8] block: tracking request allocation with q_usage_counter Ming Lei
2017-09-02 13:08 ` [PATCH V3 6/8] block: introduce preempt version of blk_[freeze|unfreeze]_queue Ming Lei
2017-09-04 15:21 ` Bart Van Assche
2017-09-04 15:21 ` Bart Van Assche
2017-09-04 16:20 ` Ming Lei
2017-09-02 13:08 ` [PATCH V3 7/8] block: allow to allocate req with REQF_PREEMPT when queue is preempt frozen Ming Lei
2017-09-02 13:12 ` Ming Lei
2017-09-04 4:13 ` Bart Van Assche
2017-09-04 4:13 ` Bart Van Assche
2017-09-04 7:16 ` Ming Lei
2017-09-04 15:40 ` Bart Van Assche [this message]
2017-09-04 15:40 ` Bart Van Assche
2017-09-04 16:08 ` Ming Lei
2017-09-04 16:18 ` Bart Van Assche
2017-09-04 16:18 ` Bart Van Assche
2017-09-04 16:28 ` Ming Lei
2017-09-05 1:40 ` Bart Van Assche
2017-09-05 1:40 ` Bart Van Assche
2017-09-05 2:23 ` Ming Lei
2017-09-08 3:08 ` Ming Lei
2017-09-08 17:28 ` Bart Van Assche
2017-09-08 17:28 ` Bart Van Assche
2017-09-09 7:21 ` Ming Lei
2017-09-02 13:08 ` [PATCH V3 8/8] SCSI: preempt freeze block queue when SCSI device is put into quiesce Ming Lei
2017-09-02 14:47 ` [PATCH V3 0/8] block/scsi: safe SCSI quiescing Oleksandr Natalenko
2017-09-02 14:47 ` Oleksandr Natalenko
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=1504539634.3189.15.camel@wdc.com \
--to=bart.vanassche@wdc.com \
--cc=axboe@fb.com \
--cc=hch@infradead.org \
--cc=jejb@linux.vnet.ibm.com \
--cc=jthumshirn@suse.de \
--cc=linux-block@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=ming.lei@redhat.com \
--cc=oleksandr@natalenko.name \
--cc=tj@kernel.org \
/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.