[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d732a8f6-4a0a-d7ff-af9c-f377fefd1283@fastmail.com>
Date: Thu, 6 Apr 2023 19:58:38 +0900
From: Damien Le Moal <dlemoal@...tmail.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: Yangtao Li <frank.li@...o.com>, xiang@...nel.org, chao@...nel.org,
huyue2@...lpad.com, jefflexu@...ux.alibaba.com,
damien.lemoal@...nsource.wdc.com, naohiro.aota@....com,
jth@...nel.org, rafael@...nel.org, linux-erofs@...ts.ozlabs.org,
linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 3/3] zonefs: convert to use kobject_is_added()
On 4/6/23 19:26, Greg KH wrote:
> On Thu, Apr 06, 2023 at 07:13:38PM +0900, Damien Le Moal wrote:
>> 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@...o.com>
>>>> ---
>>>> 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.
>
> Ok, but then why not just 0 out the kobject pointer here instead? That
> way you will always know if it's a valid pointer or not and you don't
> have to rely on some other variable? Use the one that you have already :)
but sbi->s_kobj is the kobject itself, not a pointer. I can still zero it out in
case of error to avoid using the added s_sysfs_registered bool. I would need to
check a field of s_kobj though, which is not super clean and makes the code
dependent on kobject internals. Not super nice in my opinion, unless I am
missing something.
> And you really don't even need to check anything, just pass in NULL to
> kobject_del() and friends, it should handle it.>
>>>> -
>>>> 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.
>
> But you already had a sbi pointer in the place that this was called, so
> you "know" if you need to even call into here or not. You are having to
> look up the same pointer multiple times in this call chain, there's no
> need for that.
I am not following here. Either we check that we have sbi here in
zonefs_sysfs_unregister(), or we conditionally call this function in
zonefs_kill_super() with a "if (sbi)". Either way, we need to check since sbi
can be NULL.
>
> thanks,
>
> greg k-h
Powered by blists - more mailing lists