From: Bart Van Assche <Bart.VanAssche@wdc.com>
To: "tj@kernel.org" <tj@kernel.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
"nborisov@suse.com" <nborisov@suse.com>,
"axboe@kernel.dk" <axboe@kernel.dk>, "shli@fb.com" <shli@fb.com>,
"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
"00moses.alexander00@gmail.com" <00moses.alexander00@gmail.com>,
"arnd@arndb.de" <arnd@arndb.de>,
"joseph.qi@linux.alibaba.com" <joseph.qi@linux.alibaba.com>
Subject: Re: [PATCH v2] blk-cgroup: remove entries in blkg_tree before queue release
Date: Wed, 11 Apr 2018 17:06:41 +0000 [thread overview]
Message-ID: <398bad36e2f01e37645a36d052d62136766ee88d.camel@wdc.com> (raw)
In-Reply-To: <20180411170018.GL793541@devbig577.frc2.facebook.com>
T24gV2VkLCAyMDE4LTA0LTExIGF0IDEwOjAwIC0wNzAwLCB0akBrZXJuZWwub3JnIHdyb3RlOg0K
PiBPbiBXZWQsIEFwciAxMSwgMjAxOCBhdCAwNDo0Mjo1NVBNICswMDAwLCBCYXJ0IFZhbiBBc3Nj
aGUgd3JvdGU6DQo+ID4gT24gV2VkLCAyMDE4LTA0LTExIGF0IDA3OjU2IC0wNzAwLCBUZWp1biBI
ZW8gd3JvdGU6DQo+ID4gPiBBbmQgbG9va2luZyBhdCB0aGUgY2hhbmdlLCBpdCBsb29rcyBsaWtl
IHRoZSByaWdodCB0aGluZyB3ZSBzaG91bGQNCj4gPiA+IGhhdmUgZG9uZSBpcyBjYWNoaW5nIEBs
b2NrIG9uIHRoZSBwcmludF9ibGtnIHNpZGUgYW5kIHdoZW4gc3dpdGNoaW5nDQo+ID4gPiBsb2Nr
cyBtYWtlIHN1cmUgYm90aCBsb2NrcyBhcmUgaGVsZC4gIElPVywgZG8gdGhlIGZvbGxvd2luZyBp
bg0KPiA+ID4gYmxrX2NsZWFudXBfcXVldWUoKQ0KPiA+ID4gDQo+ID4gPiAJc3Bpbl9sb2NrX2ly
cShsb2NrKTsNCj4gPiA+IAlpZiAocS0+cXVldWVfbG9jayAhPSAmcS0+X19xdWV1ZV9sb2NrKSB7
DQo+ID4gPiAJCXNwaW5fbG9jaygmcS0+X19xdWV1ZV9sb2NrKTsNCj4gPiA+IAkJcS0+cXVldWVf
bG9jayA9ICZxLT5fX3F1ZXVlX2xvY2s7DQo+ID4gPiAJCXNwaW5fdW5sb2NrKCZxLT5fX3F1ZXVl
X2xvY2spOw0KPiA+ID4gCX0NCj4gPiA+IAlzcGluX3VubG9ja19pcnEobG9jayk7DQo+ID4gPiAN
Cj4gPiA+IE90aGVyd2lzZSwgdGhlcmUgY2FuIGJlIHR3byBsb2NrIGhvbGRlcnMgdGhpbmtpbmcg
dGhleSBoYXZlIGV4Y2x1c2l2ZQ0KPiA+ID4gYWNjZXNzIHRvIHRoZSByZXF1ZXN0X3F1ZXVlLg0K
PiA+IA0KPiA+IEkgdGhpbmsgdGhhdCdzIGEgYmFkIGlkZWEuIEEgYmxvY2sgZHJpdmVyIGlzIGFs
bG93ZWQgdG8gZGVzdHJveSB0aGUNCj4gPiBzcGlubG9jayBpdCBhc3NvY2lhdGVkIHdpdGggdGhl
IHJlcXVlc3QgcXVldWUgYXMgc29vbiBhcyBibGtfY2xlYW51cF9xdWV1ZSgpDQo+ID4gaGFzIGZp
bmlzaGVkLiBJZiB0aGUgYmxvY2sgY2dyb3VwIGNvbnRyb2xsZXIgd291bGQgY2FjaGUgYSBwb2lu
dGVyIHRvIHRoZQ0KPiA+IGJsb2NrIGRyaXZlciBzcGlubG9jayB0aGVuIHRoYXQgY291bGQgY2F1
c2UgdGhlIGNncm91cCBjb2RlIHRvIGF0dGVtcHQgdG8NCj4gPiBsb2NrIGEgc3BpbmxvY2sgYWZ0
ZXIgaXQgaGFzIGJlZW4gZGVzdHJveWVkLiBJIGRvbid0IHRoaW5rIHdlIG5lZWQgdGhhdCBraW5k
DQo+ID4gb2YgcmFjZSBjb25kaXRpb25zLg0KPiANCj4gSSBzZWUsIGJ1dCB0aGF0IHByb2JsZW0g
aXMgdGhlcmUgd2l0aCBvciB3aXRob3V0IGNhY2hpbmcgYXMgbG9uZyBhcyB3ZQ0KPiBoYXZlIHF1
ZXVfbG9jayB1c2FnZSB3aGljaCByZWFjaCBiZXlvbmQgY2xlYW51cF9xdWV1ZSwgcmlnaHQ/ICBX
aGV0aGVyDQo+IHRoYXQgdXNlciBjYWNoZXMgdGhlIGxvY2sgZm9yIG1hdGNoaW5nIHVubG9ja2lu
ZyBvciBub3QgZG9lc24ndCByZWFsbHkNCj4gY2hhbmdlIHRoZSBzaXR1YXRpb24uDQo+IA0KPiBT
aG9ydCBvZiBhZGRpbmcgcHJvdGVjdGlvbiBhcm91bmQgcXVldWVfbG9jayBzd2l0Y2hpbmcsIEkg
Y2FuJ3QgdGhpbmsNCj4gb2YgYSBzb2x1dGlvbiB0aG8uICBQcm9iYWJseSB0aGUgcmlnaHQgdGhp
bmcgdG8gZG8gaXMgYWRkaW5nIHF1ZXVlDQo+IGxvY2svdW5sb2NrIGhlbHBlcnMgd2hpY2ggYXJl
IHNhZmUgdG8gdXNlIGJleW9uZCBjbGVhbnVwX3F1ZXVlLg0KDQpIZWxsbyBUZWp1biwNCg0KQSBz
aW1wbGUgYW5kIGVmZmVjdGl2ZSBzb2x1dGlvbiBpcyB0byBkaXNzb2NpYXRlIGEgcmVxdWVzdCBx
dWV1ZSBmcm9tIHRoZQ0KYmxvY2sgY2dyb3VwIGNvbnRyb2xsZXIgYmVmb3JlIGJsa19jbGVhbnVw
X3F1ZXVlKCkgcmV0dXJucy4gVGhpcyBpcyB3aHkgY29tbWl0DQphMDYzMDU3ZDdjNzMgKCJibG9j
azogRml4IGEgcmFjZSBiZXR3ZWVuIHJlcXVlc3QgcXVldWUgcmVtb3ZhbCBhbmQgdGhlIGJsb2Nr
DQpjZ3JvdXAgY29udHJvbGxlciIpIG1vdmVkIHRoZSBibGtjZ19leGl0X3F1ZXVlKCkgY2FsbCBm
cm9tIF9fYmxrX3JlbGVhc2VfcXVldWUoKQ0KaW50byBibGtfY2xlYW51cF9xdWV1ZSgpLg0KDQpU
aGFua3MsDQoNCkJhcnQuDQoNCg0KDQoNCg==
WARNING: multiple messages have this Message-ID (diff)
From: Bart Van Assche <Bart.VanAssche@wdc.com>
To: "tj@kernel.org" <tj@kernel.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
"nborisov@suse.com" <nborisov@suse.com>,
"axboe@kernel.dk" <axboe@kernel.dk>, "shli@fb.com" <shli@fb.com>,
"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
"00moses.alexander00@gmail.com" <00moses.alexander00@gmail.com>,
"arnd@arndb.de" <arnd@arndb.de>,
"joseph.qi@linux.alibaba.com" <joseph.qi@linux.alibaba.com>
Subject: Re: [PATCH v2] blk-cgroup: remove entries in blkg_tree before queue release
Date: Wed, 11 Apr 2018 17:06:41 +0000 [thread overview]
Message-ID: <398bad36e2f01e37645a36d052d62136766ee88d.camel@wdc.com> (raw)
In-Reply-To: <20180411170018.GL793541@devbig577.frc2.facebook.com>
On Wed, 2018-04-11 at 10:00 -0700, tj@kernel.org wrote:
> On Wed, Apr 11, 2018 at 04:42:55PM +0000, Bart Van Assche wrote:
> > On Wed, 2018-04-11 at 07:56 -0700, Tejun Heo wrote:
> > > And looking at the change, it looks like the right thing we should
> > > have done is caching @lock on the print_blkg side and when switching
> > > locks make sure both locks are held. IOW, do the following in
> > > blk_cleanup_queue()
> > >
> > > spin_lock_irq(lock);
> > > if (q->queue_lock != &q->__queue_lock) {
> > > spin_lock(&q->__queue_lock);
> > > q->queue_lock = &q->__queue_lock;
> > > spin_unlock(&q->__queue_lock);
> > > }
> > > spin_unlock_irq(lock);
> > >
> > > Otherwise, there can be two lock holders thinking they have exclusive
> > > access to the request_queue.
> >
> > I think that's a bad idea. A block driver is allowed to destroy the
> > spinlock it associated with the request queue as soon as blk_cleanup_queue()
> > has finished. If the block cgroup controller would cache a pointer to the
> > block driver spinlock then that could cause the cgroup code to attempt to
> > lock a spinlock after it has been destroyed. I don't think we need that kind
> > of race conditions.
>
> I see, but that problem is there with or without caching as long as we
> have queu_lock usage which reach beyond cleanup_queue, right? Whether
> that user caches the lock for matching unlocking or not doesn't really
> change the situation.
>
> Short of adding protection around queue_lock switching, I can't think
> of a solution tho. Probably the right thing to do is adding queue
> lock/unlock helpers which are safe to use beyond cleanup_queue.
Hello Tejun,
A simple and effective solution is to dissociate a request queue from the
block cgroup controller before blk_cleanup_queue() returns. This is why commit
a063057d7c73 ("block: Fix a race between request queue removal and the block
cgroup controller") moved the blkcg_exit_queue() call from __blk_release_queue()
into blk_cleanup_queue().
Thanks,
Bart.
next prev parent reply other threads:[~2018-04-11 17:06 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-07 10:21 [PATCH v2] blk-cgroup: remove entries in blkg_tree before queue release Alexandru Moise
2018-04-09 22:09 ` Tejun Heo
2018-04-11 10:12 ` Alexandru Moise
2018-04-11 14:20 ` Tejun Heo
2018-04-11 14:28 ` Alexandru Moise
2018-04-11 14:46 ` Tejun Heo
2018-04-11 14:51 ` Tejun Heo
2018-04-11 14:56 ` Tejun Heo
2018-04-11 16:42 ` Bart Van Assche
2018-04-11 16:42 ` Bart Van Assche
2018-04-11 17:00 ` tj
2018-04-11 17:06 ` Bart Van Assche [this message]
2018-04-11 17:06 ` Bart Van Assche
2018-04-11 17:15 ` tj
2018-04-11 17:26 ` Bart Van Assche
2018-04-11 17:26 ` Bart Van Assche
2018-04-11 17:30 ` tj
2018-04-11 15:54 ` Bart Van Assche
2018-04-11 15:54 ` Bart Van Assche
2018-04-11 19:00 ` Alexandru Moise
2018-04-11 19:55 ` Bart Van Assche
2018-04-11 19:57 ` tj
2018-04-11 20:00 ` Bart Van Assche
2018-04-11 20:00 ` Bart Van Assche
2018-04-11 20:02 ` tj
2018-04-11 20:23 ` Bart Van Assche
2018-04-11 20:23 ` Bart Van Assche
2018-04-11 21:23 ` Alexandru Moise
2018-04-11 21:28 ` Bart Van Assche
2018-04-11 21:28 ` Bart Van Assche
2018-04-11 22:58 ` Alexandru Moise
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=398bad36e2f01e37645a36d052d62136766ee88d.camel@wdc.com \
--to=bart.vanassche@wdc.com \
--cc=00moses.alexander00@gmail.com \
--cc=arnd@arndb.de \
--cc=axboe@kernel.dk \
--cc=gregkh@linuxfoundation.org \
--cc=joseph.qi@linux.alibaba.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nborisov@suse.com \
--cc=shli@fb.com \
--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.