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

[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