On Mon, Oct 7, 2024 at 11:35 AM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > On Sun, 6 Oct 2024 at 10:24, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > Hi all, > > > > This is v2 of the code to avoid temporary backing file opens in > > overlayfs, taking into account Al's comments on v1 [1]. > > > > Miklos, > > > > The implementation of ovl_real_file_path() helper is roughly based on > > ovl_real_dir_file(). > > > > do you see any problems with this approach or any races not handled? > > Note that I did have a logical bug in v1 (always choosing the stashed > > upperfile if it exists), so there may be more. > > Stashing the upper file pointer in the lower file's backing struct > feels like a layering violation. > > Wouldn't it be cleaner to just do what directory files do and link > both upper and lower backing files from the ovl file? Maybe it is more straightforward, I can go with that, but it feels like a waste not to use the space in backing_file, so let me first try to convince you otherwise. IMO, this is not a layer violation at all. The way I perceive struct backing_file is as an inheritance from struct file, similar to the way that ovl_inode is an inheritance from vfs_inode. The difference being that backing_file is generic (to be used by ovl/fuse) and does not have per-fs constructor/destructor/user_path() methods, because that would have been over design. You can say that backing_file_user_path() is the layer violation, having the vfs peek into the ovl layer above it, but backing_file_private_ptr() is the opposite - it is used only by the layer that allocated backing_file, so it is just like saying that a struct file has a single private_data, while the inherited generic backing_file can store two private_data pointers. What's wrong with that? Thanks, Amir.
