On Mon, Oct 9, 2023 at 10:48 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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.
