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

[Baokun Li <libaokun1@huawei.com>]

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