Received-SPF: pass (google.com: domain of linux-kernel+bounces-149703-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <bd839e9f-d3f2-f99d-34f7-e819a84c51cf@huawei.com>
Date: Thu, 18 Apr 2024 16:16:31 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.1.2
Subject: Re: [PATCH v2] erofs: reliably distinguish block based and fscache
 mode
Content-Language: en-US
To: Jingbo Xu <jefflexu@linux.alibaba.com>, <linux-erofs@lists.ozlabs.org>,
	<xiang@kernel.org>, <chao@kernel.org>, <huyue2@coolpad.com>,
	<viro@zeniv.linux.org.uk>, <brauner@kernel.org>,
	<linux-kernel@vger.kernel.org>, <yangerkun@huawei.com>, <houtao1@huawei.com>,
	Baokun Li <libaokun1@huawei.com>
References: <20240417065513.3409744-1-libaokun1@huawei.com>
 <71e66b02-9c2b-4981-83e1-8af72d6c0975@linux.alibaba.com>
 <7fdf4bff-2d3d-bdc0-5446-caa58aeca314@huawei.com>
 <fb65c7d0-c348-409e-b977-07616d28b279@linux.alibaba.com>
 <48b21671-19ae-0dbd-96cd-7300fd600c9b@huawei.com> <ZiDQgmN0hC2uIiPA@debian>
From: Baokun Li <libaokun1@huawei.com>
In-Reply-To: <ZiDQgmN0hC2uIiPA@debian>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit

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