Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 18 Apr 2024 16:16:31 +0800 | Subject | Re: [PATCH v2] erofs: reliably distinguish block based and fscache mode | From | Baokun Li <> |
| |
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
| ![\](/images/icornerr.gif) |