On 4/6/23 19:05, Greg KH wrote:
> On Thu, Apr 06, 2023 at 05:30:56PM +0800, Yangtao Li wrote:
>> Use kobject_is_added() instead of local `s_sysfs_registered` variables.
>> BTW kill kobject_del() directly, because kobject_put() actually covers
>> kobject removal automatically.
>>
>> Signed-off-by: Yangtao Li <frank.li@xxxxxxxx>
>> ---
>> fs/zonefs/sysfs.c | 11 +++++------
>> fs/zonefs/zonefs.h | 1 -
>> 2 files changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/zonefs/sysfs.c b/fs/zonefs/sysfs.c
>> index 8ccb65c2b419..f0783bf7a25c 100644
>> --- a/fs/zonefs/sysfs.c
>> +++ b/fs/zonefs/sysfs.c
>> @@ -101,8 +101,6 @@ int zonefs_sysfs_register(struct super_block *sb)
>> return ret;
>> }
>>
>> - sbi->s_sysfs_registered = true;
>
> You know this, why do you need to have a variable tell you this or not?
If kobject_init_and_add() fails, zonefs_sysfs_register() returns an error and
fill_super will also return that error. vfs will then call kill_super, which
calls zonefs_sysfs_unregister(). For that case, we need to know that we actually
added the kobj.
>
>> -
>> return 0;
>> }
>>
>> @@ -110,12 +108,13 @@ void zonefs_sysfs_unregister(struct super_block *sb)
>> {
>> struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
>>
>> - if (!sbi || !sbi->s_sysfs_registered)
>
> How can either of these ever be true? Note, sbi should be passed here
> to this function, not the super block as that is now unregistered from
> the system. Looks like no one has really tested this codepath that much
> :(
>
>> + if (!sbi)
>> return;
>
> this can not ever be true, right?
Yes it can, if someone attempt to mount a non zoned device. In that case,
fill_super returns early without setting sb->s_fs_info but vfs still calls
kill_super.
>
>
>>
>> - kobject_del(&sbi->s_kobj);
>> - kobject_put(&sbi->s_kobj);
>> - wait_for_completion(&sbi->s_kobj_unregister);
>> + if (kobject_is_added(&sbi->s_kobj)) {
>
> Again, not needed.
>
> thanks,
>
> greg k-h
