Messages in this thread | | | Date | Fri, 25 Sep 2009 06:16:33 +0200 | From | Jens Axboe <> | Subject | Re: [PATCH] Add missing blk_trace_remove_sysfs to be in pair with blk_trace_init_sysfs |
| |
On Thu, Sep 24 2009, Zdenek Kabelac wrote: > 2009/9/24 Li Zefan <lizf@cn.fujitsu.com>: > >>>> Subject: [PATCH] Add missing blk_trace_remove_sysfs to be in pair with > >>>> blk_trace_init_sysfs > >>>> From: Zdenek Kabelac <zkabelac@redhat.com> > >>>> > >>>> Adds missing blk_trace_remove_sysfs() to be in pair with > >>>> blk_trace_init_sysfs() introduced in commit > >>>> 1d54ad6da9192fed5dd3b60224d9f2dfea0dcd82. > >>>> > >>>> Problem was noticed via kmemleak backtrace when some sysfs entries > >>>> were note properly destroyed during device removal: > >>>> > >>> Thanks for reporting and fixing this! > >>> > >>>> @@ -465,6 +466,7 @@ void blk_unregister_queue(struct gendisk *disk) > >>>> > >>>> kobject_uevent(&q->kobj, KOBJ_REMOVE); > >>>> kobject_del(&q->kobj); > >>>> + blk_trace_remove_sysfs(disk_to_dev(disk)); > >>> This should be moved outside of 'if'. > >>> > >> > >> I was not really sure about the proper place - if it could be placed > >> before if() or after the if(){} - as I've not checked in depth > > > > Just use the reverse order against blk_register_queue() should be fine. > > Yes - I think the order of release is correct. > > > > >> connection between kobj and sysfs. It's somewhat unclear why all the > >> kobject operation are only within this if(){} block - so I've thought > >> there is some reason... > >> IMHO only elv_unregister_queue() should be probably in the if(){} block. > >> > > > > Seems it's a bug to put kobject_put(dev->kobj) in the if block. > > > > I created a stacked device (mdadm) and kmemleak still reported leaks > > even after I fixed the blktrace issue. And then I moved kobejct_put() > > outside the if, no more leaks reports. > > > > Ok - I didn't have a testcase where the request_fn would be NULL. > So in this case I propose this patch: > > --- > Add missing blk_trace_remove_sysfs to be in pair with blk_trace_init_sysfs > introduced in commit 1d54ad6da9192fed5dd3b60224d9f2dfea0dcd82. > Release kobject also in case the request_fn is NULL. > > Problem was noticed via kmemleak backtrace when some sysfs entries were > note properly destroyed during device removal:
Thanks, this looks good now, applied.
-- Jens Axboe
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |