From: Greg KH Subject: Re: [PATCH 1/2] kobject: introduce kobj_completion Date: Wed, 11 Sep 2013 09:59:42 -0700 Message-ID: <20130911165942.GA29193@kroah.com> References: <522F5A6A.2000201@suse.com> <20130910180627.GA4274@kroah.com> <522F65DC.1010303@suse.com> <522F6882.2070900@suse.com> <20130911045042.GA31241@kroah.com> <5230A019.5030101@suse.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: ext4 development , Theodore Ts'o To: Jeff Mahoney Return-path: Received: from out3-smtp.messagingengine.com ([66.111.4.27]:38091 "EHLO out3-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751327Ab3IKQ7r (ORCPT ); Wed, 11 Sep 2013 12:59:47 -0400 Content-Disposition: inline In-Reply-To: <5230A019.5030101@suse.com> Sender: linux-ext4-owner@vger.kernel.org List-ID: On Wed, Sep 11, 2013 at 12:53:45PM -0400, Jeff Mahoney wrote: > On 9/11/13 12:50 AM, Greg KH wrote: > > On Tue, Sep 10, 2013 at 02:44:18PM -0400, Jeff Mahoney wrote: > >> On 9/10/13 2:33 PM, Jeff Mahoney wrote: > >>> On 9/10/13 2:06 PM, Greg KH wrote: > >>>> On Tue, Sep 10, 2013 at 01:44:10PM -0400, Jeff Mahoney wrote: > >>>>> ext4 exports per-filesystem information via sysfs. The lifetime rules > >>>>> have historically been painful for this but the solution has been to pair > >>>>> the kobject with a completion and call complete in the kobject's > >>>>> release function. > >>>>> > >>>>> Since this is a pattern I've used in btrfs as well, it makes sense to > >>>>> turn the pairing into a convenience structure with a standard API. > >>>>> > >>>>> Signed-off-by: Jeff Mahoney > >>>>> --- > >>>>> include/linux/kobj_completion.h | 18 +++++++++++++++ > >>>>> lib/kobject.c | 47 ++++++++++++++++++++++++++++++++++++++++ > >>>>> 2 files changed, 65 insertions(+) > >>>>> > >>>>> --- /dev/null 1970-01-01 00:00:00.000000000 +0000 > >>>>> +++ b/include/linux/kobj_completion.h 2013-09-10 12:58:03.530554144 -0400 > >>>>> @@ -0,0 +1,18 @@ > >>>>> +#ifndef _KOBJ_COMPLETION_H_ > >>>>> +#define _KOBJ_COMPLETION_H_ > >>>>> + > >>>>> +#include > >>>>> +#include > >>>>> + > >>>>> +struct kobj_completion { > >>>>> + struct kobject kc_kobj; > >>>>> + struct completion kc_unregister; > >>>>> +}; > >>>>> + > >>>>> +#define kobj_to_kobj_completion(kobj) \ > >>>>> + container_of(kobj, struct kobj_completion, kc_kobj) > >>>>> + > >>>>> +void kobj_completion_init(struct kobj_completion *kc, struct kobj_type *ktype); > >>>>> +void kobj_completion_release(struct kobject *kobj); > >>>>> +void kobj_completion_del_and_wait(struct kobj_completion *kc); > >>>>> +#endif /* _KOBJ_COMPLETION_H_ */ > >>>>> --- a/lib/kobject.c 2013-09-10 12:57:54.198666613 -0400 > >>>>> +++ b/lib/kobject.c 2013-09-10 13:16:31.750607946 -0400 > >>>>> @@ -13,6 +13,7 @@ > >>>>> */ > >>>>> > >>>>> #include > >>>>> +#include > >>>>> #include > >>>>> #include > >>>>> #include > >>>>> @@ -711,6 +712,52 @@ const struct sysfs_ops kobj_sysfs_ops = > >>>>> }; > >>>>> > >>>>> /** > >>>>> + * kobj_completion_init - initialize a kobj_completion object. > >>>>> + * @kc: kobj_completion > >>>>> + * @ktype: type of kobject to initialize > >>>>> + * > >>>>> + * kobj_completion structures can be embedded within structures with different > >>>>> + * lifetime rules. During the release of the enclosing object, we can > >>>>> + * wait on the release of the kobject so that we don't free it while it's > >>>>> + * still busy. > >>>>> + */ > >>>>> +void kobj_completion_init(struct kobj_completion *kc, struct kobj_type *ktype) > >>>>> +{ > >>>>> + init_completion(&kc->kc_unregister); > >>>>> + kobject_init(&kc->kc_kobj, ktype); > >>>>> +} > >>>>> +EXPORT_SYMBOL_GPL(kobj_completion_init); > >>>>> + > >>>>> +/** > >>>>> + * kobj_completion_release - release a kobj_completion object > >>>>> + * @kobj: kobject embedded in kobj_completion > >>>>> + * > >>>>> + * Used with kobject_release to notify waiters that the kobject has been > >>>>> + * released. > >>>>> + */ > >>>>> +void kobj_completion_release(struct kobject *kobj) > >>>>> +{ > >>>>> + struct kobj_completion *kc = kobj_to_kobj_completion(kobj); > >>>>> + complete(&kc->kc_unregister); > >>>>> +} > >>>>> +EXPORT_SYMBOL_GPL(kobj_completion_release); > >>>>> + > >>>>> +/** > >>>>> + * kobj_completion_del_and_wait - release the kobject and wait for it > >>>>> + * @kc: kobj_completion object to release > >>>>> + * > >>>>> + * Delete the kobject from sysfs and drop the reference count. Then wait > >>>>> + * until any outstanding references are also dropped. > >>>>> + */ > >>>>> +void kobj_completion_del_and_wait(struct kobj_completion *kc) > >>>>> +{ > >>>>> + kobject_del(&kc->kc_kobj); > >>>>> + kobject_put(&kc->kc_kobj); > >>>> > >>>> Why the extra kobject_put() call? Who added this extra reference to the > >>>> object? > >>> > >>> There's an assumption that kobject_add will have been called on the > >>> initialized kobject. If it hasn't been called, the object can just be > >>> deleted without the completion. It makes the calling code easier to > >>> read, so would it work for you if I documented that assumption in > >>> _del_and_wait? > >> > >> Actually, that's not it. The refcounting works out in my tests. > >> > >> The kobject's refcount is initialized to 1. kobject_add doesn't > >> increment it and kobject_del doesn't decrement it. So we need the > >> kobject_put to trigger the release function. > >> > >> Or am I missing something here? > > > > Ugh, no, you are right, this is all good as-is. Do you want to take > > these through the ext4 tree, or do you want me to take them? > > I'd prefer to get this one in now and wait until Ted's available to > discuss the ext4 change. My btrfs changes depend on this as well and I'd > like to get those moving now. > > I have a newer version with an updated comment that explains the > lifetime a bit better that I'll send separately as a single patch. Ok, I'll be glad to queue that one up to get into 3.12 so that you can push the other patches separately. thanks, greg k-h