Re: [PATCH] Add missing blk_trace_remove_sysfs to be in pair with blk_trace_init_sysfs

[Jens Axboe <jens.axboe@oracle.com>]

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