linux-kernel - Re: [PATCH v2] erofs: reliably distinguish block based and fscache mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <bd839e9f-d3f2-f99d-34f7-e819a84c51cf@huawei.com>
Date: Thu, 18 Apr 2024 16:16:31 +0800
From: Baokun Li <libaokun1@...wei.com>
To: Jingbo Xu <jefflexu@...ux.alibaba.com>, <linux-erofs@...ts.ozlabs.org>,
	<xiang@...nel.org>, <chao@...nel.org>, <huyue2@...lpad.com>,
	<viro@...iv.linux.org.uk>, <brauner@...nel.org>,
	<linux-kernel@...r.kernel.org>, <yangerkun@...wei.com>, <houtao1@...wei.com>,
	Baokun Li <libaokun1@...wei.com>
Subject: Re: [PATCH v2] erofs: reliably distinguish block based and fscache
 mode

On 2024/4/18 15:49, Gao Xiang wrote:
> Hi,
>
> On Thu, Apr 18, 2024 at 02:12:39PM +0800, Baokun Li wrote:
>> On 2024/4/18 13:50, Jingbo Xu wrote:
>>> On 4/18/24 11:36 AM, Baokun Li wrote:
>>>> On 2024/4/18 10:16, Jingbo Xu wrote:
>>>>> Hi Baokun,
>>>>>
>>>>> Thanks for catching this and move forward fixing this!
>>>> Hi Jingbo，
>>>>
>>>> Thanks for your review！
>>>>
>>>>> On 4/17/24 2:55 PM, Baokun Li wrote:

SNIP

>>>>>
>>>>> Instead of allocating the erofs_sb_info in fill_super() allocate it
>>>>> during erofs_get_tree() and ensure that erofs can always have the info
>>>>> available during erofs_kill_sb().
>>>>> I'm not sure if allocating erofs_sb_info in erofs_init_fs_context() will
>>>>> be better, as I see some filesystems (e.g. autofs) do this way.  Maybe
>>>>> another potential advantage of doing this way is that erofs_fs_context
>>>>> is not needed anymore and we can use sbi directly.
>>>> Yes, except for some extra memory usage when remounting,
>>>> this idea sounds great. Let me send a version of v3 to get rid
>>>> of erofs_fs_context.
>>> I'm not sure if Gao Xaing also prefers this.  I think it would be better
>>> to wait and listen for his thoughts before we sending v3.
>>   Okay, there's no rush on this.
> I checked days ago, for example, XFS is also worked in this way.
> And .reconfigure() needs to be carefully handled too.

Ok, I'll implement it in the next iteration.

>>>>>> +static void erofs_ctx_to_info(struct fs_context *fc)
>>>>>>     {
>>>>>>         struct erofs_fs_context *ctx = fc->fs_private;
>>>>>> +    struct erofs_sb_info *sbi = fc->s_fs_info;
>>>>>> +
>>>>>> +    sbi->opt = ctx->opt;
>>>>>> +    sbi->devs = ctx->devs;
>>>>>> +    ctx->devs = NULL;
>>>>>> +    sbi->fsid = ctx->fsid;
>>>>>> +    ctx->fsid = NULL;
>>>>>> +    sbi->domain_id = ctx->domain_id;
>>>>>> +    ctx->domain_id = NULL;
>>>>>> +}
>>>>> I'm not sure if abstracting this logic into a seperate helper really
>>>>> helps understanding the code as the logic itself is quite simple and
>>>>> easy to be understood. Usually it's a hint of over-abstraction when a
>>>>> simple helper has only one caller.
>>>>>
>>>> Static functions that have only one caller are compiled inline, so we
>>>> don't have to worry about how that affects the code.
>>>>
>>>> The reason these codes are encapsulated in a separate function is so
>>>> that the code reader understands that these codes are integrated
>>>> as a whole, and that we shouldn't have to move one or two of these
>>>> lines individually.
>>>>
>>>> But after we get rid of erofs_fs_context, those won't be needed
>>>> anymore.
>>> Yeah, I understand. It's only coding style concerns.
>>>
>>>
>>>
>> Okay, thanks！
> I'm fine to get rid of those (erofs_fs_context) as long as the codebase
> is more clearer and simple.  BTW, for the current codebase, I also think
> it's unneeded to have a separate helper called once without extra actual
> meaning...
>
> Thanks,
> Gao Xiang
>
Ok, this helper function will be gone in the next iteration.

Thanks for the review!
-- 
With Best Regards,
Baokun Li