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

[Gao Xiang]

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.

> 
> 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.

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.

Thanks,
Gao Xiang

> 
> Thanks,
> Amir.