[PATCH RFC 2/4] erofs: introduce page cache share feature

[Amir Goldstein]

On Sat, Jul 5, 2025 at 2:53 PM Gao Xiang <hsiangkao at linux.alibaba.com> wrote:
>
>
>
> On 2025/7/5 20:34, Amir Goldstein wrote:
> > On Sat, Jul 5, 2025 at 12:58 PM Gao Xiang <hsiangkao at linux.alibaba.com> wrote:
> >>
> >> Hi Amir,
> >>
> >> On 2025/7/5 16:25, Amir Goldstein wrote:
> >>
> >> ...
> >>
> >>>
> >>> 2. When is it ok to use the backing_file helpers?
> >>>
> >>> The current patch allocates a struct file with
> >>> alloc_file_pseudo() and not a struct backing_file,
> >>> so using the backing_file helpers is going to be awkward and
> >>> misleading and I think in this case it will we wize to refrain
> >>> from using a local var name backing_file.
> >>>
> >>> The thing that you need to ask yourself is do you want
> >>> backing_file_set_user_path() for the erofs shared inode.
> >>
> >> Yes, agreed, that should be improved as you said.
> >>
> >>>
> >>> That means, what do you want users to see when they
> >>> look at /proc/self/map_files symlinks.
> >>>
> >>> With the current patch set I believe they will see a symlink to
> >>> something like "[erofs_pcs_f]" for any mapped file
> >>> which is somewhat odd.
> >>
> >> Agreed.
> >>
> >>>
> >>> I think it would have been nice if users saw something like
> >>> "[erofs_pcs_md5digest:XXXXXXX]"
> >>
> >> But if we use `overlay.metacopy` for example, it's not
> >> a string anymore. IOWs, I'd like to support binary
> >> footprint too.
> >>
> >> And I tend to avoid md5 calcuation or something in
> >> the kernel.  The kernel just uses footprint xattrs
> >> instead.
> >>
> >>> IMO, making the page cache dedupe visible in map_files is
> >>> a very nice feature.
> >>>> Overlayfs took the approach that users are expecting to see
> >>> the actual path of the (non-anonymous) file that they mapped
> >>> when looking at map_files symlinks.
> >>> If you do not display the path to erofs mount in map_files,
> >>> then lsof will not be able to blame a process with mapped files
> >>> as the reason for keeping erofs mount busy.
> >>
> >> I think users should see `the actual path` rather than
> >> "[erofs_pcs_xxxxx]" too, but I don't have any chance to
> >> check this part yet.
> >>
> >> If it's possible for overlayfs, I guess it's possible for
> >> erofs page cache sharing, maybe?
> >>
> >
> > Yes, I am sorry if that wasn't clear.
> > If you want the map_files to show the user mapped file's path,
> > you can use the backing_file helpers and more specifically
> > backing_file_open() and all should work as in overlayfs.
>
> Yep, makes sense, and a quick look I think we should leverage
> `file_real_path()` to reveal the user-shown file's path.
>
> But I also think erofs don't need every backingfile infra as
> you said we don't need to override_creds and call in the
> underlay fs but that use erofs io directly instead.
>

True.

> >
> > Obviously, you'd need to wrap the back_file helper with
> > erofs specific logic, such as don't allow dio and such.
>
> I think maybe only `struct backing_file` is really needed
> to support the user mapped file's path.
>
> Other thing it seems a overkill to use `fs/backing-file.c`
> for inode page cache use cases.

True.

>
> Also, maybe I misunderstand your point. I think DIO can
> work too, .read_iter() should be per-sb and we may just
> use the original DIO logic and pass down iocb and iov etc?
> Since only mmap i/o needs to be handled via anon inodes.
>

The title of the patch set is page cache sharing
and DIO has nothing to do with page cache so I thought it
wasn't relevant.

If the inodes are also sharing the backing disk blocks,
then I guess you can do dio either on the shared inode
or the original inode, but it doesn't matter much.

I meant that the only part of backing_file_read_iter()
for which you may want to consider the helper is the aio
part, but since aio is only for directo io, I think there is
no real benefit of erofs to use the helper.

Thanks,
Amir.