[PATCH v2 3/3] fs: store real path instead of fake path in backing file f_path

[Amir Goldstein]

On Mon, Oct 9, 2023 at 10:48 AM Al Viro <viro at zeniv.linux.org.uk> wrote:
>
> On Sat, Oct 07, 2023 at 11:44:33AM +0300, Amir Goldstein wrote:
>
> > -             if (real_path->mnt)
> > -                     mnt_put_write_access(real_path->mnt);
> > +             if (user_path->mnt)
> > +                     mnt_put_write_access(user_path->mnt);
> >       }
> >  }
>
> Again, how can the predicates be ever false here?  We should *not*
> have struct path with NULL .mnt unless it's {NULL, NULL} pair.
>
> For the record, struct path with NULL .dentry and non-NULL .mnt
> *IS* possible, but only in a very narrow area - if, during
> an attempt to fall back from rcu pathwalk to normal one we
> have __legitimize_path() successfully validate (== grab) the
> reference to mount, but fail to validate dentry.  In that
> case we need to drop mount, but not dentry when we get to
> cleanup (pretty much as soon as we drop rcu_read_lock()).
> That gets indicated by clearing path->dentry, and only
> while we are propagating the error back to the point where
> references would be dropped.  No filesystem code should
> ever see struct path instances in that state.
>
> Please, don't make the things more confusing; "incomplete"
> struct path like that are very much not normal (and this
> variety is flat-out impossible).
>
>

No problem.
I will remove the conditional.

> > @@ -34,9 +34,18 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
> >       struct dentry *real = NULL, *lower;
> >       int err;
> >
> > -     /* It's an overlay file */
> > +     /*
> > +      * vfs is only expected to call d_real() with NULL from d_real_inode()
> > +      * and with overlay inode from file_dentry() on an overlay file.
> > +      *
> > +      * TODO: remove @inode argument from d_real() API, remove code in this
> > +      * function that deals with non-NULL @inode and remove d_real() call
> > +      * from file_dentry().
> > +      */
> >       if (inode && d_inode(dentry) == inode)
> >               return dentry;
> > +     else
> > +             WARN_ON_ONCE(inode);
> >
> >       if (!d_is_reg(dentry)) {
> >               if (!inode || inode == d_inode(dentry))
>                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>         BTW, that condition is confusing as hell (both before and
> after this patch).  AFAICS, it's a pointlessly obfuscated
>                 if (!inode)
> Look: we get to evaluating that test only if we hadn't buggered
> off on
>         if (inode && d_inode(dentry) == inode)
>                 return dentry;
> above.  Which means that either inode is NULL (in which case the
> evaluation yields true as soon as we see that !inode is true) or
> it's neither NULL nor equal to d_inode(dentry).  In which case
> we see that !inode is false and proceed yield false *after*
> comparing inode with d_inode(dentry) and seeing that they
> are not equal.
>
> <checks history>
> e8c985bace13 "ovl: deal with overlay files in ovl_d_real()"
> had introduced the first check, and nobody noticed that the
> older check below could've been simplified.  Oh, well...
>

Absolutely right.
I can remove the pointless condition.

FWIW, the next step after dust from this patch set settles
is to make file_dentry(f) := ((f)->f_path.dentry) and remove
the non-NULL inode case from ->d_real() interface altogether,
so this confusing check was going to go away soon anyway.

> > -static inline const struct path *file_real_path(struct file *f)
> > +static inline const struct path *f_path(struct file *f)
> >  {
> > -     if (unlikely(f->f_mode & FMODE_BACKING))
> > -             return backing_file_real_path(f);
> >       return &f->f_path;
> >  }
>
> Bad name, IMO - makes grepping harder and... what the hell do
> we need it for, anyway?  You have only one caller, and no
> obvious reason why it would be worse off as path = &file->f_path...

It's not important. I don't mind dropping it.

If you dislike that name f_path(), I guess you are not a fan of
d_inode() either...

FYI, I wanted to do a file_path() accessor to be consistent with
file_inode() and file_dentry(), alas file_path() is used for something
completely different.

I find it confusing that {file,dentry,d}_path() do not return a path
but a path string, but whatever.

Thanks,
Amir.