Re: dm: sysfs add empty release function to avoid debug warning

Re: dm: sysfs add empty release function to avoid debug warning

[Greg KH]

On Tue, Dec 15, 2009 at 05:35:08PM +0000, James Bottomley wrote:
> commit: d2bb7df8cac647b92f51fb84ae735771e7adbfa7
> From: Milan Broz <mbroz@redhat.com>
> Date: Thu, 10 Dec 2009 23:51:53 +0000
> Subject: [PATCH] dm: sysfs add empty release function to avoid debug warning
> 
> This patch just removes an unnecessary warning:
>  kobject: 'dm': does not have a release() function,
>  it is broken and must be fixed.
> 
> The kobject is embedded in mapped device struct, so
> code does not need to release memory explicitly here.

Oh no you did not!

Come on people, do you think that the kernel is just randomly spitting
out warnings because it fricken feels like it!

No, please revert this patch now before someone does it for you.

Please, this is totally and completly wrong.  And if you feel that it is
needed, then your design is wrong and it needs to be fixed.

And note, as per the Documentation/kobject.txt file, you were warned
about this public mocking, next time it will be harsher.

Heh, and you tried to get this into a -stable kernel release, as if.

Patch left below so that others can mock.

bah,

greg k-h

> 
> Cc: stable@kernel.org
> Signed-off-by: Milan Broz <mbroz@redhat.com>
> Signed-off-by: Alasdair G Kergon <agk@redhat.com>
> ---
>  drivers/md/dm-sysfs.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/md/dm-sysfs.c b/drivers/md/dm-sysfs.c
> index 4b04590..b000de3 100644
> --- a/drivers/md/dm-sysfs.c
> +++ b/drivers/md/dm-sysfs.c
> @@ -80,12 +80,20 @@ static struct sysfs_ops dm_sysfs_ops = {
>  };
>  
>  /*
> + * The sysfs structure is embedded in md struct, nothing to do here
> + */
> +static void dm_sysfs_release(struct kobject *kobj)
> +{
> +}
> +
> +/*
>   * dm kobject is embedded in mapped_device structure
>   * no need to define release function here
>   */
>  static struct kobj_type dm_ktype = {
>  	.sysfs_ops	= &dm_sysfs_ops,
>  	.default_attrs	= dm_attrs,
> +	.release	= dm_sysfs_release
>  };
>  
>  /*
> 
> _______________________________________________
> stable mailing list
> stable@linux.kernel.org
> http://linux.kernel.org/mailman/listinfo/stable

Re: dm: sysfs add empty release function to avoid debug warning

[Milan Broz]

On 12/16/2009 01:47 AM, Greg KH wrote:
> On Tue, Dec 15, 2009 at 05:35:08PM +0000, James Bottomley wrote:
>> commit: d2bb7df8cac647b92f51fb84ae735771e7adbfa7
>> From: Milan Broz <mbroz@redhat.com>
>> Date: Thu, 10 Dec 2009 23:51:53 +0000
>> Subject: [PATCH] dm: sysfs add empty release function to avoid debug warning
>>
>> This patch just removes an unnecessary warning:
>>  kobject: 'dm': does not have a release() function,
>>  it is broken and must be fixed.
>>
>> The kobject is embedded in mapped device struct, so
>> code does not need to release memory explicitly here.
> 

> Please, this is totally and completly wrong.  And if you feel that it is
> needed, then your design is wrong and it needs to be fixed.

There are several places in kernel, where kobject have not defined release
method. Yes, something is wrong here.
So quietly ignoring warning is ok? Why is not there BUG_ON(!release) then?

The sysfs attributes here just represents attributes of block device object,
this device is always removed before release here is called.

So if there is preferred to do another alloc/free, no problem.

> And note, as per the Documentation/kobject.txt file, you were warned
> about this public mocking, next time it will be harsher.
> 
> Heh, and you tried to get this into a -stable kernel release, as if.

btw I did'n added stable@, someone added that later. But it is not relevant. 

> Patch left below so that others can mock.

Enjoy;-)

Milan

Re: dm: sysfs add empty release function to avoid debug warning

[Greg KH]

On Wed, Dec 16, 2009 at 10:44:41AM +0100, Milan Broz wrote:
> On 12/16/2009 01:47 AM, Greg KH wrote:
> > On Tue, Dec 15, 2009 at 05:35:08PM +0000, James Bottomley wrote:
> >> commit: d2bb7df8cac647b92f51fb84ae735771e7adbfa7
> >> From: Milan Broz <mbroz@redhat.com>
> >> Date: Thu, 10 Dec 2009 23:51:53 +0000
> >> Subject: [PATCH] dm: sysfs add empty release function to avoid debug warning
> >>
> >> This patch just removes an unnecessary warning:
> >>  kobject: 'dm': does not have a release() function,
> >>  it is broken and must be fixed.
> >>
> >> The kobject is embedded in mapped device struct, so
> >> code does not need to release memory explicitly here.
> > 
> 
> > Please, this is totally and completly wrong.  And if you feel that it is
> > needed, then your design is wrong and it needs to be fixed.
> 
> There are several places in kernel, where kobject have not defined release
> method. Yes, something is wrong here.

Where are those instances, becides the use of static kobjects, which is
being worked on?

That still does not make this change acceptable, it is incorrect.

> So quietly ignoring warning is ok? Why is not there BUG_ON(!release) then?

I was trying to be nice and not crash your machine and give you the
opportunity to fix it easily.

> The sysfs attributes here just represents attributes of block device object,
> this device is always removed before release here is called.

Then why use a kobject at all?  Your reference counting will be all
wrong if you use it incorrectly like this.

> So if there is preferred to do another alloc/free, no problem.

Please do.

Also, with the addition of this patch, your comments for the kobject are
now incorrect as well.

thanks,

greg k-h

Re: [dm-devel] Re: dm: sysfs add empty release function to avoid debug warning

[Zdenek Kabelac]

Dne 16.12.2009 14:45, Greg KH napsal(a):
> On Wed, Dec 16, 2009 at 10:44:41AM +0100, Milan Broz wrote:
>> On 12/16/2009 01:47 AM, Greg KH wrote:
>>> On Tue, Dec 15, 2009 at 05:35:08PM +0000, James Bottomley wrote:
>>>> commit: d2bb7df8cac647b92f51fb84ae735771e7adbfa7
>>>> From: Milan Broz <mbroz@redhat.com>
>>>> Date: Thu, 10 Dec 2009 23:51:53 +0000
>>>> Subject: [PATCH] dm: sysfs add empty release function to avoid debug warning
>>>>
>>>> This patch just removes an unnecessary warning:
>>>>  kobject: 'dm': does not have a release() function,
>>>>  it is broken and must be fixed.
>>>>
>>>> The kobject is embedded in mapped device struct, so
>>>> code does not need to release memory explicitly here.
>>>
>>
>>> Please, this is totally and completly wrong.  And if you feel that it is
>>> needed, then your design is wrong and it needs to be fixed.
>>
>> There are several places in kernel, where kobject have not defined release
>> method. Yes, something is wrong here.
> 
> Where are those instances, becides the use of static kobjects, which is
> being worked on?
> 
> That still does not make this change acceptable, it is incorrect.
> 
>> So quietly ignoring warning is ok? Why is not there BUG_ON(!release) then?
> 
> I was trying to be nice and not crash your machine and give you the
> opportunity to fix it easily.
> 
>> The sysfs attributes here just represents attributes of block device object,
>> this device is always removed before release here is called.
> 
> Then why use a kobject at all?  Your reference counting will be all
> wrong if you use it incorrectly like this.
> 
>> So if there is preferred to do another alloc/free, no problem.
> 
> Please do.
> 
> Also, with the addition of this patch, your comments for the kobject are
> now incorrect as well.
> 

Here is list of just some errors I get on my 2.6.32 linux kernel.
(error message itself is removed)
Messages are printed during module removal/reboot.

'aead' (ffffffffa017c050): does not have a release() function, it is broken
and must be fixed.
'bridge'
'cdrom'
'cfg80211'
'crypto_hash'
'cryptomgr'
'dm_log'
'dm_mirror'
'dm_mod'
'dm_multipath'
'dm_region_hash'
'ehci_hcd'
'e1000e'
'iptable_filter'
'iptable_nat'
'ip_tables'
'ipt_MASQUERADE'
'ipt_REJECT'
'iTCO_vendor_support'
'iTCO_wdt'
'iwlcore'
'iwl3945'
'iwl3945'
'kvm'
'kvm_intel'
'kvm0'
'llc'
'mac80211'
'microcode'
'mmc_core'
'mmc'
'nf_conntrack'
'nf_conntrack_ipv4'
'nf_defrag_ipv4'
'nf_nat'
'pcompress'
'psmouse'
'rtc_cmos'
'rtc_core'
'rtc_lib'
'scsi_wait_scan'
'sdhci'
'sdhci_pci'
'sdio'
'snd_mixer_oss'
'snd_pcm_oss'
'snd_seq_device'
'snd_seq'
'snd_seq_midi_event'
'snd_seq_oss'
'sr_mod'
'stp'
'x_tables'
'xt_state'
'xt_tcpudp'

Zdenek

Re: [dm-devel] Re: dm: sysfs add empty release function to avoid debug warning

[Greg KH]

On Wed, Dec 16, 2009 at 05:32:21PM +0100, Zdenek Kabelac wrote:
> Dne 16.12.2009 14:45, Greg KH napsal(a):
> > On Wed, Dec 16, 2009 at 10:44:41AM +0100, Milan Broz wrote:
> >> On 12/16/2009 01:47 AM, Greg KH wrote:
> >>> On Tue, Dec 15, 2009 at 05:35:08PM +0000, James Bottomley wrote:
> >>>> commit: d2bb7df8cac647b92f51fb84ae735771e7adbfa7
> >>>> From: Milan Broz <mbroz@redhat.com>
> >>>> Date: Thu, 10 Dec 2009 23:51:53 +0000
> >>>> Subject: [PATCH] dm: sysfs add empty release function to avoid debug warning
> >>>>
> >>>> This patch just removes an unnecessary warning:
> >>>>  kobject: 'dm': does not have a release() function,
> >>>>  it is broken and must be fixed.
> >>>>
> >>>> The kobject is embedded in mapped device struct, so
> >>>> code does not need to release memory explicitly here.
> >>>
> >>
> >>> Please, this is totally and completly wrong.  And if you feel that it is
> >>> needed, then your design is wrong and it needs to be fixed.
> >>
> >> There are several places in kernel, where kobject have not defined release
> >> method. Yes, something is wrong here.
> > 
> > Where are those instances, becides the use of static kobjects, which is
> > being worked on?
> > 
> > That still does not make this change acceptable, it is incorrect.
> > 
> >> So quietly ignoring warning is ok? Why is not there BUG_ON(!release) then?
> > 
> > I was trying to be nice and not crash your machine and give you the
> > opportunity to fix it easily.
> > 
> >> The sysfs attributes here just represents attributes of block device object,
> >> this device is always removed before release here is called.
> > 
> > Then why use a kobject at all?  Your reference counting will be all
> > wrong if you use it incorrectly like this.
> > 
> >> So if there is preferred to do another alloc/free, no problem.
> > 
> > Please do.
> > 
> > Also, with the addition of this patch, your comments for the kobject are
> > now incorrect as well.
> > 
> 
> Here is list of just some errors I get on my 2.6.32 linux kernel.
> (error message itself is removed)
> Messages are printed during module removal/reboot.
> 
> 'aead' (ffffffffa017c050): does not have a release() function, it is broken
> and must be fixed.
> 'bridge'
> 'cdrom'

<snip>

You will find that all of these are modules, right?  That's being worked
on still.

Again, please fix your code not to have an empty release function.  Will
you revert this change and fix it properly?

thanks,

greg k-h

Re: [dm-devel] Re: dm: sysfs add empty release function to avoid debug warning

[Milan Broz]

On 12/16/2009 06:46 PM, Greg KH wrote:
> You will find that all of these are modules, right?  That's being worked
> on still.

probably. I see that this implementation is in kernel since import to git...

What's the right solution in this particular case?
It is exactly the same situation (using embedded kobj like module handle does).

In DM, the whole problem here is that kobject is used for attribute representation
of mapped device, mapped device struct is using its own reference counting
and can dissappear before the kobject. So it must properly solve this situation.

> Again, please fix your code not to have an empty release function.  Will
> you revert this change and fix it properly?

Sure, working on it. Proper fix will replace this when ready.

Milan

Re: [dm-devel] Re: dm: sysfs add empty release function to avoid debug warning

[Greg KH]

On Wed, Dec 16, 2009 at 07:40:27PM +0100, Milan Broz wrote:
> On 12/16/2009 06:46 PM, Greg KH wrote:
> > You will find that all of these are modules, right?  That's being worked
> > on still.
> 
> probably. I see that this implementation is in kernel since import to git...

And you will also note that I didn't paper over the issue by trying to
provide an empty release function :)

> What's the right solution in this particular case?
> It is exactly the same situation (using embedded kobj like module handle does).
> 
> In DM, the whole problem here is that kobject is used for attribute representation
> of mapped device, mapped device struct is using its own reference counting
> and can dissappear before the kobject. So it must properly solve this situation.

Then dynamically create the kobject, as it obviously has a different
lifespan.

Or do something else, but as it is now, you can not have two reference
counts with different lifespan rules, within the same object, and expect
things to work properly.

thanks,

greg k-h

Re: [dm-devel] Re: dm: sysfs add empty release function to avoid debug warning

[Milan Broz]

On 12/17/2009 01:30 AM, Greg KH wrote:
> On Wed, Dec 16, 2009 at 07:40:27PM +0100, Milan Broz wrote:
>> On 12/16/2009 06:46 PM, Greg KH wrote:
>>> You will find that all of these are modules, right?  That's being worked
>>> on still.
>>
>> probably. I see that this implementation is in kernel since import to git...
> 
> And you will also note that I didn't paper over the issue by trying to
> provide an empty release function :)

Alasdair, can we request revert of that last patch for now
(which introduced the empty release function)?

Milan

Re: [dm-devel] Re: dm: sysfs add empty release function to avoid debug warning

[Greg KH]

On Thu, Dec 17, 2009 at 04:43:07PM +0100, Milan Broz wrote:
> On 12/17/2009 01:30 AM, Greg KH wrote:
> > On Wed, Dec 16, 2009 at 07:40:27PM +0100, Milan Broz wrote:
> >> On 12/16/2009 06:46 PM, Greg KH wrote:
> >>> You will find that all of these are modules, right?  That's being worked
> >>> on still.
> >>
> >> probably. I see that this implementation is in kernel since import to git...
> > 
> > And you will also note that I didn't paper over the issue by trying to
> > provide an empty release function :)
> 
> Alasdair, can we request revert of that last patch for now
> (which introduced the empty release function)?

Ping?  Any word on the revert?

thanks,

greg k-h

Re: [dm-devel] Re: dm: sysfs add empty release function to avoid debug warning

[Zdenek Kabelac]

Dne 16.12.2009 18:46, Greg KH napsal(a):
> On Wed, Dec 16, 2009 at 05:32:21PM +0100, Zdenek Kabelac wrote:
>> Dne 16.12.2009 14:45, Greg KH napsal(a):
>>> On Wed, Dec 16, 2009 at 10:44:41AM +0100, Milan Broz wrote:
>>>> On 12/16/2009 01:47 AM, Greg KH wrote:
>>>>> On Tue, Dec 15, 2009 at 05:35:08PM +0000, James Bottomley wrote:
>>>>>> commit: d2bb7df8cac647b92f51fb84ae735771e7adbfa7
>>>>>> From: Milan Broz <mbroz@redhat.com>
>>>>>> Date: Thu, 10 Dec 2009 23:51:53 +0000
>>>>>> Subject: [PATCH] dm: sysfs add empty release function to avoid debug warning
>>>>>>
>>>>>> This patch just removes an unnecessary warning:
>>>>>>  kobject: 'dm': does not have a release() function,
>>>>>>  it is broken and must be fixed.
>>>>>>
>>>>>> The kobject is embedded in mapped device struct, so
>>>>>> code does not need to release memory explicitly here.
>>>>>
>>>>
>>>>> Please, this is totally and completly wrong.  And if you feel that it is
>>>>> needed, then your design is wrong and it needs to be fixed.
>>>>
>>>> There are several places in kernel, where kobject have not defined release
>>>> method. Yes, something is wrong here.
>>>
>>> Where are those instances, becides the use of static kobjects, which is
>>> being worked on?
>>>
>>> That still does not make this change acceptable, it is incorrect.
>>>
>>>> So quietly ignoring warning is ok? Why is not there BUG_ON(!release) then?
>>>
>>> I was trying to be nice and not crash your machine and give you the
>>> opportunity to fix it easily.
>>>
>>>> The sysfs attributes here just represents attributes of block device object,
>>>> this device is always removed before release here is called.
>>>
>>> Then why use a kobject at all?  Your reference counting will be all
>>> wrong if you use it incorrectly like this.
>>>
>>>> So if there is preferred to do another alloc/free, no problem.
>>>
>>> Please do.
>>>
>>> Also, with the addition of this patch, your comments for the kobject are
>>> now incorrect as well.
>>>
>>
>> Here is list of just some errors I get on my 2.6.32 linux kernel.
>> (error message itself is removed)
>> Messages are printed during module removal/reboot.
>>
>> 'aead' (ffffffffa017c050): does not have a release() function, it is broken
>> and must be fixed.
>> 'bridge'
>> 'cdrom'
> 
> <snip>
> 
> You will find that all of these are modules, right?  That's being worked
> on still.
> 

Yes - true in most cases - here are exceptions:

fill_kobj_path: path = '/devices/system/kvm/kvm0'
fill_kobj_path: path = '/devices/system/kvm'
fill_kobj_path: path = '/bus/sdio'
fill_kobj_path: path = '/bus/mmc'

Please note, these are only modules in use on my system in use - so there is
probably more of them...

Zdenek