Re: [PATCH v5 4/5] fs: use backing_file container for internal files with "fake" f_path

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux