Patchwork [1/2] kobject: introduce kobj_completion

login
register
mail settings
Submitter Jeff Mahoney
Date Sept. 10, 2013, 5:44 p.m.
Message ID <522F5A6A.2000201@suse.com>
Download mbox | patch
Permalink /patch/273943/
State Not Applicable
Headers show

Comments

Jeff Mahoney - Sept. 10, 2013, 5:44 p.m.
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 <jeffm@suse.com>
---
 include/linux/kobj_completion.h |   18 +++++++++++++++
 lib/kobject.c                   |   47 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)
Greg KH - Sept. 10, 2013, 6:06 p.m.
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 <jeffm@suse.com>
> ---
>  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 <linux/kobject.h>
> +#include <linux/completion.h>
> +
> +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 <linux/kobject.h>
> +#include <linux/kobj_completion.h>
>  #include <linux/string.h>
>  #include <linux/export.h>
>  #include <linux/stat.h>
> @@ -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?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Mahoney - Sept. 10, 2013, 6:33 p.m.
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 <jeffm@suse.com>
>> ---
>>  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 <linux/kobject.h>
>> +#include <linux/completion.h>
>> +
>> +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 <linux/kobject.h>
>> +#include <linux/kobj_completion.h>
>>  #include <linux/string.h>
>>  #include <linux/export.h>
>>  #include <linux/stat.h>
>> @@ -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?

-Jeff
Jeff Mahoney - Sept. 10, 2013, 6:44 p.m.
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 <jeffm@suse.com>
>>> ---
>>>  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 <linux/kobject.h>
>>> +#include <linux/completion.h>
>>> +
>>> +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 <linux/kobject.h>
>>> +#include <linux/kobj_completion.h>
>>>  #include <linux/string.h>
>>>  #include <linux/export.h>
>>>  #include <linux/stat.h>
>>> @@ -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?

-Jeff
Greg KH - Sept. 10, 2013, 6:49 p.m.
On Tue, Sep 10, 2013 at 02:33:00PM -0400, 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 <jeffm@suse.com>
> >> ---
> >>  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 <linux/kobject.h>
> >> +#include <linux/completion.h>
> >> +
> >> +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 <linux/kobject.h>
> >> +#include <linux/kobj_completion.h>
> >>  #include <linux/string.h>
> >>  #include <linux/export.h>
> >>  #include <linux/stat.h>
> >> @@ -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?

Yes, documenting it would be good, as it sure confused me :)

As for the overall idea, I have no objection to it, lots of other people
have done this same "pattern" in places, so this could be used to clean
up that code as well in the future.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg KH - Sept. 11, 2013, 4:50 a.m.
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 <jeffm@suse.com>
> >>> ---
> >>>  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 <linux/kobject.h>
> >>> +#include <linux/completion.h>
> >>> +
> >>> +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 <linux/kobject.h>
> >>> +#include <linux/kobj_completion.h>
> >>>  #include <linux/string.h>
> >>>  #include <linux/export.h>
> >>>  #include <linux/stat.h>
> >>> @@ -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?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Mahoney - Sept. 11, 2013, 4:53 p.m.
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 <jeffm@suse.com>
>>>>> ---
>>>>>  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 <linux/kobject.h>
>>>>> +#include <linux/completion.h>
>>>>> +
>>>>> +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 <linux/kobject.h>
>>>>> +#include <linux/kobj_completion.h>
>>>>>  #include <linux/string.h>
>>>>>  #include <linux/export.h>
>>>>>  #include <linux/stat.h>
>>>>> @@ -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.

Thanks.

-Jeff
Greg KH - Sept. 11, 2013, 4:59 p.m.
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 <jeffm@suse.com>
> >>>>> ---
> >>>>>  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 <linux/kobject.h>
> >>>>> +#include <linux/completion.h>
> >>>>> +
> >>>>> +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 <linux/kobject.h>
> >>>>> +#include <linux/kobj_completion.h>
> >>>>>  #include <linux/string.h>
> >>>>>  #include <linux/export.h>
> >>>>>  #include <linux/stat.h>
> >>>>> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- /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 <linux/kobject.h>
+#include <linux/completion.h>
+
+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 <linux/kobject.h>
+#include <linux/kobj_completion.h>
 #include <linux/string.h>
 #include <linux/export.h>
 #include <linux/stat.h>
@@ -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);
+	wait_for_completion(&kc->kc_unregister);
+}
+EXPORT_SYMBOL_GPL(kobj_completion_del_and_wait);
+
+/**
  * kset_register - initialize and add a kset.
  * @k: kset.
  */