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

[Gao Xiang]

On 2025/7/5 21:53, Amir Goldstein wrote:
> 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.

Agreed, thank you Amir!

Thanks,
Gao Xiang

> 
> Thanks,
> Amir.