On Sat, Jul 5, 2025 at 2:53 PM Gao Xiang <hsiang...@linux.alibaba.com> wrote: > > > > 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. >
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.
