Re: [PATCH RESEND] slub: return correct error on slab_sysfs_init

[Jeff Liu <jeff.liu@oracle.com>]

On 06/21/2014 06:30 AM, David Rientjes wrote:
> On Fri, 20 Jun 2014, Jeff Liu wrote:
> 
>> At that time, I thought it would be ENOMEM because I was review another patch
>> for adding sysfs support to XFS where we return ENOMEM in this case:
>> http://www.spinics.net/lists/xfs/msg28343.html
>>
>> This drives to me to think why it should be ENOMEM rather than ERR_PTR since
>> it seems most likely kset_create_and_add() would fails due to other reasons.
>> Hence I looked through kernel sources and figured out most subsystems are return
>> ENOMEM, maybe those subsystems are refers to the kset example code at:
>> samples/kobject/kset-example.c
>>
> 
> If you're going to ignore other emails in this thread, then you're not 
> going to make a very strong argument.

No, I was not intended to ignore your comments, instead, I appreciate your
review since it took up your time and time is valuable to everybody.
But the time zone is too late to me yesterday, and I need to refresh my
head to read through the whole call chains in kset_create_and_add().

> 
> kset_create_and_add() can return NULL for reasons OTHER than just ENOMEM.  
> It can also be returned for EEXIST because something registered the 
> kobject with the same name.  During init, which is what you're modifying 
> here, the liklihood is higher that it will return EEXIST rather than 
> ENOMEM otherwise there are much bigger problems than return value.

Agree, now it's clear EEXIST is returned if we trying to create a new kobject
with the same name and with the same parent_kobj, i.e,

kset_create_and_add()
  kset_register()
    kset_register()
      kobject_add_internal()
        create_dir()
          sysfs_create_dir_ns()->sysfs_warn_dup()...

And also, the likelihood is higher to some extent, indeed.

> 
>> So my original motivation is just to make the slub sysfs init error handling in
>> accordance to other subsystems(nitpick) and it does not affect the kernel behaviour.
>>
> 
> Why should slub match the other incorrect behavior?
> 
> What you're never addressing is WHY you are even making this change or
> even care about the return value.  Show userspace breakage that depends on 
> this.

For the consistency with other subsystems, but now I don't think it's right
due to above reason.

>> Combine with Greg's comments, as such, maybe the changelog would looks like
>> the following?
>>
>> GregKH: the only reason for failure would be out of memory on kset_create_and_add().
>> return -ENOMEM than -ENOSYS if the call is failed which is consistent with other
>> subsystems in this situation.
>>
> 
> Bullshit.  Read the above.
  ^^^^^^^
I assume that you spoke like that because I have not reply to you in time, I can
understand if so.  Otherwise, don't talk to me like that no matter who you are!

> 
> If you want to return PTR_ERR() when this fails and fixup all the callers, 
> then propose that patch.  Until then, it's a pretty simple rule: if you 
> don't have an errno, don't assume the reason for failure.

As I mentioned previously, Greg don't like to fixup kobjects API via PTR_ERR().
For me, I neither want to propose PTR_ERR to kobject nor try to push the current
slub fix, because it's make no sense to slub with either errno.


Cheers,
-Jeff

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>