On 2025/7/5 20:34, Amir Goldstein wrote:
On Sat, Jul 5, 2025 at 12:58 PM Gao Xiang <hsiang...@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.
