From: Bart Van Assche <Bart.VanAssche@wdc.com>
To: "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
"hch@infradead.org" <hch@infradead.org>,
"jejb@linux.vnet.ibm.com" <jejb@linux.vnet.ibm.com>,
"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
"axboe@fb.com" <axboe@fb.com>,
"ming.lei@redhat.com" <ming.lei@redhat.com>,
"martin.petersen@oracle.com" <martin.petersen@oracle.com>
Cc: Bart Van Assche <Bart.VanAssche@wdc.com>,
"jthumshirn@suse.de" <jthumshirn@suse.de>,
"oleksandr@natalenko.name" <oleksandr@natalenko.name>,
"cavery@redhat.com" <cavery@redhat.com>
Subject: Re: [PATCH V4 08/10] block: allow to allocate req with RQF_PREEMPT when queue is preempt frozen
Date: Mon, 11 Sep 2017 16:03:55 +0000 [thread overview]
Message-ID: <1505145834.2802.17.camel@wdc.com> (raw)
In-Reply-To: <20170911111021.25810-9-ming.lei@redhat.com>
T24gTW9uLCAyMDE3LTA5LTExIGF0IDE5OjEwICswODAwLCBNaW5nIExlaSB3cm90ZToNCj4gQEAg
LTc4Nyw2ICs3ODcsMzUgQEAgaW50IGJsa19xdWV1ZV9lbnRlcihzdHJ1Y3QgcmVxdWVzdF9xdWV1
ZSAqcSwgdW5zaWduZWQgZmxhZ3MpDQo+ICAJCWlmIChwZXJjcHVfcmVmX3RyeWdldF9saXZlKCZx
LT5xX3VzYWdlX2NvdW50ZXIpKQ0KPiAgCQkJcmV0dXJuIDA7DQo+ICANCj4gKwkJLyoNCj4gKwkJ
ICogSWYgcXVldWUgaXMgcHJlZW1wdCBmcm96ZW4gYW5kIGNhbGxlciBuZWVkIHRvIGFsbG9jYXRl
DQo+ICsJCSAqIHJlcXVlc3QgZm9yIFJRRl9QUkVFTVBULCB3ZSBncmFiIHRoZSAucV91c2FnZV9j
b3VudGVyDQo+ICsJCSAqIHVuY29uZGl0aW9uYWxseSBhbmQgcmV0dXJuIHN1Y2Nlc3NmdWxseS4N
Cj4gKwkJICoNCj4gKwkJICogVGhlcmUgaXNuJ3QgcmFjZSB3aXRoIHF1ZXVlIGNsZWFudXAgYmVj
YXVzZToNCj4gKwkJICoNCj4gKwkJICogMSkgaXQgaXMgZ3VhcmFudGVlZCB0aGF0IHByZWVtcHQg
ZnJlZXplIGNhbid0IGJlDQo+ICsJCSAqIHN0YXJ0ZWQgYWZ0ZXIgcXVldWUgaXMgc2V0IGFzIGR5
aW5nDQo+ICsJCSAqDQo+ICsJCSAqIDIpIG5vcm1hbCBmcmVlemUgcnVucyBleGNsdXNpdmVseSB3
aXRoIHByZWVtcHQNCj4gKwkJICogZnJlZXplLCBzbyBldmVuIGFmdGVyIHF1ZXVlIGlzIHNldCBh
cyBkeWluZw0KPiArCQkgKiBhZnRlcndhcmRzLCBibGtfcXVldWVfY2xlYW51cCgpIHdvbid0IG1v
dmUgb24NCj4gKwkJICogdW50aWwgcHJlZW1wdCBmcmVlemUgaXMgZG9uZQ0KPiArCQkgKg0KPiAr
CQkgKiAzKSBibGtfcXVldWVfZHlpbmcoKSBuZWVkbid0IHRvIGJlIGNoZWNrZWQgaGVyZQ0KPiAr
CQkgKiAJLSBmb3IgbGVnYWN5IHBhdGgsIGl0IHdpbGwgYmUgY2hlY2tlZCBpbg0KPiArCQkgKiAJ
X19nZXRfcmVxdWVzdCgpDQoNCkZvciB0aGUgbGVnYWN5IGJsb2NrIGxheWVyIGNvcmUsIHdoYXQg
ZG8geW91IHRoaW5rIHdpbGwgaGFwcGVuIGlmIHRoZQ0KImR5aW5nIiBzdGF0ZSBpcyBzZXQgYnkg
YW5vdGhlciB0aHJlYWQgYWZ0ZXIgX19nZXRfcmVxdWVzdCgpIGhhcyBwYXNzZWQgdGhlDQpibGtf
cXVldWVfZHlpbmcoKSBjaGVjaz8NCg0KPiArCQkgKiAJLSBibGstbXEgZGVwZW5kcyBvbiBkcml2
ZXIgdG8gaGFuZGxlIGR5aW5nIHdlbGwNCj4gKwkJICogCWJlY2F1c2UgaXQgaXMgbm9ybWFsIGZv
ciBxdWV1ZSB0byBiZSBzZXQgYXMgZHlpbmcNCj4gKwkJICogCWp1c3QgYmV0d2VlbiBibGtfcXVl
dWVfZW50ZXIoKSBhbmQgYWxsb2NhdGluZyBuZXcNCj4gKwkJICogCXJlcXVlc3QuDQoNClRoZSBh
Ym92ZSBjb21tZW50IGlzIG5vdCBjb3JyZWN0LiBUaGUgYmxvY2sgbGF5ZXIgY29yZSBoYW5kbGVz
IHRoZSAiZHlpbmciDQpzdGF0ZS4gQmxvY2sgZHJpdmVycyBvdGhlciB0aGFuIGRtLW1wYXRoIHNo
b3VsZCBub3QgaGF2ZSB0byBxdWVyeSB0aGlzIHN0YXRlDQpkaXJlY3RseS4NCg0KPiArCQkgKi8N
Cj4gKwkJaWYgKChmbGFncyAmIEJMS19SRVFfUFJFRU1QVCkgJiYNCj4gKwkJCQlibGtfcXVldWVf
aXNfcHJlZW1wdF9mcm96ZW4ocSkpIHsNCj4gKwkJCWJsa19xdWV1ZV9lbnRlcl9saXZlKHEpOw0K
PiArCQkJcmV0dXJuIDA7DQo+ICsJCX0NCj4gKw0KDQpTb3JyeSBidXQgdG8gbWUgaXQgbG9va3Mg
bGlrZSB0aGUgYWJvdmUgY29kZSBpbnRyb2R1Y2VzIGEgcmFjZSBjb25kaXRpb24NCmJldHdlZW4g
YmxrX3F1ZXVlX2NsZWFudXAoKSBhbmQgYmxrX2dldF9yZXF1ZXN0KCkgZm9yIGF0IGxlYXN0IGJs
ay1tcS4NCkNvbnNpZGVyIGUuZy4gdGhlIGZvbGxvd2luZyBzY2VuYXJpbzoNCiogQSBmaXJzdCB0
aHJlYWQgcHJlZW1wdC1mcmVlemVzIGEgcmVxdWVzdCBxdWV1ZS4NCiogQSBzZWNvbmQgdGhyZWFk
IGNhbGxzIGJsa19nZXRfcmVxdWVzdCgpIHdpdGggQkxLX1JFUV9QUkVFTVBUIHNldC4gVGhhdA0K
ICByZXN1bHRzIGluIGEgY2FsbCBvZiBibGtfcXVldWVfaXNfcHJlZW1wdF9mcm96ZW4oKS4NCiog
QSBjb250ZXh0IHN3aXRjaCBvY2N1cnMgdG8gdGhlIGZpcnN0IHRocmVhZC4NCiogVGhlIGZpcnN0
IHRocmVhZCBwcmVlbXB0LXVuZnJlZXplcyB0aGUgc2FtZSByZXF1ZXN0IHF1ZXVlIGFuZCBjYWxs
cw0KICBibGtfcXVldWVfY2xlYW51cCgpLiBUaGF0IGxhc3QgZnVuY3Rpb24gY2hhbmdlcyB0aGUg
cmVxdWVzdCBxdWV1ZSBzdGF0ZQ0KICBpbnRvIERZSU5HIGFuZCB3YWl0cyB1bnRpbCBhbGwgcGVu
ZGluZyByZXF1ZXN0cyBoYXZlIGZpbmlzaGVkLg0KKiBUaGUgc2Vjb25kIHRocmVhZCBjb250aW51
ZXMgYW5kIGNhbGxzIGJsa19xdWV1ZV9lbnRlcl9saXZlKCksIGFsbG9jYXRlcw0KICBhIHJlcXVl
c3QgYW5kIHN1Ym1pdHMgaXQuDQoNCkluIG90aGVyIHdvcmRzLCBhIHJlcXVlc3QgZ2V0cyBzdWJt
aXR0ZWQgYWdhaW5zdCBhIGR5aW5nIHF1ZXVlLiBUaGlzIG11c3QNCm5vdCBoYXBwZW4uIFNlZSBh
bHNvIG15IGV4cGxhbmF0aW9uIG9mIHF1ZXVlIHNodXRkb3duIGZyb20gYSBmZXcgZGF5cyBhZ28N
CihodHRwczovL21hcmMuaW5mby8/bD1saW51eC1ibG9jayZtPTE1MDQ0OTg0NTgzMTc4OSkuDQoN
CkJhcnQu
WARNING: multiple messages have this Message-ID (diff)
From: Bart Van Assche <Bart.VanAssche@wdc.com>
To: "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
"hch@infradead.org" <hch@infradead.org>,
"jejb@linux.vnet.ibm.com" <jejb@linux.vnet.ibm.com>,
"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
"axboe@fb.com" <axboe@fb.com>,
"ming.lei@redhat.com" <ming.lei@redhat.com>,
"martin.petersen@oracle.com" <martin.petersen@oracle.com>
Cc: Bart Van Assche <Bart.VanAssche@wdc.com>,
"jthumshirn@suse.de" <jthumshirn@suse.de>,
"oleksandr@natalenko.name" <oleksandr@natalenko.name>,
"cavery@redhat.com" <cavery@redhat.com>
Subject: Re: [PATCH V4 08/10] block: allow to allocate req with RQF_PREEMPT when queue is preempt frozen
Date: Mon, 11 Sep 2017 16:03:55 +0000 [thread overview]
Message-ID: <1505145834.2802.17.camel@wdc.com> (raw)
In-Reply-To: <20170911111021.25810-9-ming.lei@redhat.com>
On Mon, 2017-09-11 at 19:10 +0800, Ming Lei wrote:
> @@ -787,6 +787,35 @@ int blk_queue_enter(struct request_queue *q, unsigned flags)
> if (percpu_ref_tryget_live(&q->q_usage_counter))
> return 0;
>
> + /*
> + * If queue is preempt frozen and caller need to allocate
> + * request for RQF_PREEMPT, we grab the .q_usage_counter
> + * unconditionally and return successfully.
> + *
> + * There isn't race with queue cleanup because:
> + *
> + * 1) it is guaranteed that preempt freeze can't be
> + * started after queue is set as dying
> + *
> + * 2) normal freeze runs exclusively with preempt
> + * freeze, so even after queue is set as dying
> + * afterwards, blk_queue_cleanup() won't move on
> + * until preempt freeze is done
> + *
> + * 3) blk_queue_dying() needn't to be checked here
> + * - for legacy path, it will be checked in
> + * __get_request()
For the legacy block layer core, what do you think will happen if the
"dying" state is set by another thread after __get_request() has passed the
blk_queue_dying() check?
> + * - blk-mq depends on driver to handle dying well
> + * because it is normal for queue to be set as dying
> + * just between blk_queue_enter() and allocating new
> + * request.
The above comment is not correct. The block layer core handles the "dying"
state. Block drivers other than dm-mpath should not have to query this state
directly.
> + */
> + if ((flags & BLK_REQ_PREEMPT) &&
> + blk_queue_is_preempt_frozen(q)) {
> + blk_queue_enter_live(q);
> + return 0;
> + }
> +
Sorry but to me it looks like the above code introduces a race condition
between blk_queue_cleanup() and blk_get_request() for at least blk-mq.
Consider e.g. the following scenario:
* A first thread preempt-freezes a request queue.
* A second thread calls blk_get_request() with BLK_REQ_PREEMPT set. That
results in a call of blk_queue_is_preempt_frozen().
* A context switch occurs to the first thread.
* The first thread preempt-unfreezes the same request queue and calls
blk_queue_cleanup(). That last function changes the request queue state
into DYING and waits until all pending requests have finished.
* The second thread continues and calls blk_queue_enter_live(), allocates
a request and submits it.
In other words, a request gets submitted against a dying queue. This must
not happen. See also my explanation of queue shutdown from a few days ago
(https://marc.info/?l=linux-block&m=150449845831789).
Bart.
next prev parent reply other threads:[~2017-09-11 16:04 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-11 11:10 [PATCH V4 0/10] block/scsi: safe SCSI quiescing Ming Lei
2017-09-11 11:10 ` [PATCH V4 01/10] blk-mq: only run hw queues for blk-mq Ming Lei
2017-09-11 11:10 ` [PATCH V4 02/10] block: tracking request allocation with q_usage_counter Ming Lei
2017-09-11 11:10 ` [PATCH V4 03/10] blk-mq: rename blk_mq_[freeze|unfreeze]_queue Ming Lei
2017-09-11 11:10 ` [PATCH V4 04/10] blk-mq: rename blk_mq_freeze_queue_wait as blk_freeze_queue_wait Ming Lei
2017-09-11 11:10 ` [PATCH V4 05/10] block: rename .mq_freeze_wq and .mq_freeze_depth Ming Lei
2017-09-11 11:10 ` [PATCH V4 06/10] block: pass flags to blk_queue_enter() Ming Lei
2017-09-11 11:10 ` [PATCH V4 07/10] block: introduce preempt version of blk_[freeze|unfreeze]_queue Ming Lei
2017-09-11 11:10 ` [PATCH V4 08/10] block: allow to allocate req with RQF_PREEMPT when queue is preempt frozen Ming Lei
2017-09-11 16:03 ` Bart Van Assche [this message]
2017-09-11 16:03 ` Bart Van Assche
2017-09-12 3:40 ` Ming Lei
2017-09-13 16:48 ` Ming Lei
2017-09-13 17:28 ` Bart Van Assche
2017-09-13 17:28 ` Bart Van Assche
2017-09-13 17:48 ` Ming Lei
2017-09-13 19:07 ` Bart Van Assche
2017-09-13 19:07 ` Bart Van Assche
2017-09-14 1:15 ` Ming Lei
2017-09-14 13:37 ` Bart Van Assche
2017-09-14 13:37 ` Bart Van Assche
2017-09-14 16:18 ` Ming Lei
2017-09-11 11:10 ` [PATCH V4 09/10] SCSI: transport_spi: resume a quiesced device Ming Lei
2017-09-11 11:10 ` [PATCH V4 10/10] SCSI: preempt freeze block queue when SCSI device is put into quiesce Ming Lei
2017-09-11 21:24 ` [PATCH V4 0/10] block/scsi: safe SCSI quiescing Oleksandr Natalenko
2017-09-11 21:24 ` Oleksandr Natalenko
2017-09-12 19:03 ` Cathy Avery
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=1505145834.2802.17.camel@wdc.com \
--to=bart.vanassche@wdc.com \
--cc=axboe@fb.com \
--cc=cavery@redhat.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 \
/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.