On Fri, Jun 16, 2023 at 10:15 AM Christoph Hellwig <hch@xxxxxx> wrote: > > > - if (!(f->f_mode & FMODE_NOACCOUNT)) > > + if (unlikely(f->f_mode & FMODE_BACKING)) > > + path_put(backing_file_real_path(f)); > > + else > > percpu_counter_dec(&nr_files); > > This is still missing the earlier pointed out fix that we still need > the FMODE_NOACCOUNT check, isn't it? Yes, I forgot and Christian has already fixed this on his branch. > > > + * This is only for kernel internal use, and the allocate file must not be > > + * installed into file tables or such. > > I'd use the same blurb I'd suggest for the previous patch here as well. > > > +/** > > + * backing_file_open - open a backing file for kernel internal use > > + * @path: path of the file to open > > + * @flags: open flags > > + * @path: path of the backing file > > + * @cred: credentials for open > > + * > > + * Open a file whose f_inode != d_inode(f_path.dentry). > > + * the returned struct file is embedded inside a struct backing_file > > + * container which is used to store the @real_path of the inode. > > + * The @real_path can be accessed by backing_file_real_path() and the > > + * real dentry can be accessed with file_dentry(). > > + * The kernel internal backing file is not accounted in nr_files. > > + * This is only for kernel internal use, and must not be installed into > > + * file tables or such. > > + */ > > I still find this comment not very descriptive. Here is my counter > suggestion, which I'm also not totally happy with, and which might not > even be factually correct as I'm trying to understand the use case a bit > better by reading the code: > > * Open a backing file for a stackable file system (e.g. overlayfs). > * For these backing files that reside on the underlying file system, we still > * want to be able to return the path of the upper file in the stackable file > * system. This is done by embedding the returned file into a container > * structure that also stores the path on the upper file system, which can be > * retreived using backing_file_real_path(). It is the other way around. Those ovl files currently in master have the ovl path in f_path and xfs inode in f_inode. After this change, f_path is still ovl and f_inode is still xfs, but backing_file_real_path() can be used to get the xfs path. Using the terminology "upper file in the stackable file" above is different from what "upper file" means in overlayfs terminology. So maybe: * Open a backing file for a stackable filesystem (e.g. overlayfs). * @path may be on the stackable filesystem and backing inode on the * underlying filesystem. In this case, we want to be able to return the * @real_path of the backing inode. This is done by embedding the * returned file into a container structure that also stores the path of the * backing inode on the underlying filesystem, which can be retrieved * using backing_file_real_path(). > * > * The return file is not accounted in nr_files and must not be installed > * into the file descriptor table. > > > +static inline const struct path *f_real_path(struct file *f) > > Question from an earlier revision: why f_real_path and not file_real_path? > I missed this question. I don't really mind. Considering the documentation below, perhaps it should be called file_inode_path() > Also this really needs a kerneldoc comment explaining when it should > be used. How about this: /* * file_real_path - get the path corresponding to f_inode * * When opening a backing file for a stackable filesystem (e.g. overlayfs) * f_path may be on the stackable filesystem and f_inode on the * underlying filesystem. When the path associated with f_inode is * needed, this helper should be used instead of accessing f_path directly. */ Thanks, Amir.
