| Message ID | 20170308164834.14302-2-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, On Wed, Mar 08, 2017 at 05:48:31PM +0100, Jan Kara wrote: > @@ -710,6 +710,11 @@ static void cgwb_bdi_destroy(struct backing_dev_info *bdi) > */ > atomic_dec(&bdi->usage_cnt); > wait_event(cgwb_release_wait, !atomic_read(&bdi->usage_cnt)); > + /* > + * Grab back our reference so that we hold it when @bdi gets > + * re-registered. > + */ > + atomic_inc(&bdi->usage_cnt); So, this is more re-initializing the ref to the initial state so that it can be re-used, right? Maybe ATOMIC_INIT() is a better choice here just to clarify what's going on? Thanks.
On Wed, 2017-03-08 at 17:55 -0500, Tejun Heo wrote: > Hello, > > On Wed, Mar 08, 2017 at 05:48:31PM +0100, Jan Kara wrote: > > @@ -710,6 +710,11 @@ static void cgwb_bdi_destroy(struct > > backing_dev_info *bdi) > > */ > > atomic_dec(&bdi->usage_cnt); > > wait_event(cgwb_release_wait, !atomic_read(&bdi > > ->usage_cnt)); > > + /* > > + * Grab back our reference so that we hold it when @bdi > > gets > > + * re-registered. > > + */ > > + atomic_inc(&bdi->usage_cnt); > > So, this is more re-initializing the ref to the initial state so that > it can be re-used, right? Maybe ATOMIC_INIT() is a better choice > here just to clarify what's going on? Seconded. Eventually this is going to get converted to a refcount_t and it will dump a spurious warning on the 0->1 transition. We can avoid that by making this a proper initialization. James
On Wed 08-03-17 17:55:42, Tejun Heo wrote: > Hello, > > On Wed, Mar 08, 2017 at 05:48:31PM +0100, Jan Kara wrote: > > @@ -710,6 +710,11 @@ static void cgwb_bdi_destroy(struct backing_dev_info *bdi) > > */ > > atomic_dec(&bdi->usage_cnt); > > wait_event(cgwb_release_wait, !atomic_read(&bdi->usage_cnt)); > > + /* > > + * Grab back our reference so that we hold it when @bdi gets > > + * re-registered. > > + */ > > + atomic_inc(&bdi->usage_cnt); > > So, this is more re-initializing the ref to the initial state so that > it can be re-used, right? Maybe ATOMIC_INIT() is a better choice here > just to clarify what's going on? OK, I was somewhat undecided between these two option but you and James are probably right that re-init is clearer. Honza
diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 6d861d090e9f..6ac932210f56 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -710,6 +710,11 @@ static void cgwb_bdi_destroy(struct backing_dev_info *bdi) */ atomic_dec(&bdi->usage_cnt); wait_event(cgwb_release_wait, !atomic_read(&bdi->usage_cnt)); + /* + * Grab back our reference so that we hold it when @bdi gets + * re-registered. + */ + atomic_inc(&bdi->usage_cnt); } /** @@ -857,6 +862,8 @@ int bdi_register_owner(struct backing_dev_info *bdi, struct device *owner) MINOR(owner->devt)); if (rc) return rc; + /* Leaking owner reference... */ + WARN_ON(bdi->owner); bdi->owner = owner; get_device(owner); return 0;
SCSI can call device_add_disk() several times for one request queue when a device in unbound and bound, creating new gendisk each time. This will lead to bdi being repeatedly registered and unregistered. This was not a big problem until commit 165a5e22fafb "block: Move bdi_unregister() to del_gendisk()" since bdi was only registered repeatedly (bdi_register() handles repeated calls fine, only we ended up leaking reference to gendisk due to overwriting bdi->owner) but unregistered only in blk_cleanup_queue() which didn't get called repeatedly. After 165a5e22fafb we were doing correct bdi_register() - bdi_unregister() cycles however bdi_unregister() is not prepared for it. So make sure bdi_unregister() cleans up bdi in such a way that it is prepared for a possible following bdi_register() call. An easy way to provoke this behavior is to enable CONFIG_DEBUG_TEST_DRIVER_REMOVE and use scsi_debug driver to create a scsi disk which immediately hangs without this fix. Fixes: 165a5e22fafb127ecb5914e12e8c32a1f0d3f820 Signed-off-by: Jan Kara <jack@suse.cz> --- mm/backing-dev.c | 7 +++++++ 1 file changed, 7 insertions(+)