Message-id: <175314044347.2234665.1726134532379221703@noble.neil.brown.name>
Date: Tue, 22 Jul 2025 09:27:23 +1000
From: "NeilBrown" <neil@...wn.name>
To: "Amir Goldstein" <amir73il@...il.com>
Cc: "Linus Torvalds" <torvalds@...ux-foundation.org>,
 "Alexander Viro" <viro@...iv.linux.org.uk>,
 "Christian Brauner" <brauner@...nel.org>, "Jan Kara" <jack@...e.cz>,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/7] VFS: introduce dentry_lookup() and friends

On Mon, 21 Jul 2025, Amir Goldstein wrote:
> On Mon, Jul 21, 2025 at 10:55 AM NeilBrown <neil@...wn.name> wrote:
> >
> > dentry_lookup() combines locking the directory and performing a lookup
> > prior to a change to the directory.
> > Abstracting this prepares for changing the locking requirements.
> >
> > dentry_lookup_noperm() does the same without needing a mnt_idmap and
> > without checking permissions.  This is useful for internal filesystem
> > management (e.g.  creating virtual files in response to events) and in
> > other cases similar to lookup_noperm().
> >
> > dentry_lookup_hashed() also does no permissions checking and assumes
> > that the hash of the name has already been stored in the qstr.
> 
> That's a very confusing choice of name because _hashed() (to me) sounds
> like the opposite of d_unhashed() which is not at all the case.

True.  But maybe the confusion what already there.
You can "d_add()" a dentry and later "d_drop()" the dentry and if the
dentry isn't between those two operations, then it is "d_unhashed()"
which leaks out the implementation detail (hash table) for dentry
lookup. Maybe d_unhashed() should be d_added() with inverted meaning?

There is only one user of this interface outside of namei.c so I could
unexported to keep the confusion local.  That would mean
ksmbd_vfs_path_lookup() would hav to use dentry_lookup_noperm() which
would recalculate the hash which vfs_path_parent_lookup() already
calculated (and we cannot simply tell it not to bother calculating).
Actually it already uses lookup_noperm_unlocked() in the
don't-need-a-lock-branch which recalculates the hash.....

Would making that name static ease your concern?

> 
> > This is useful following filename_parentat().
> >
> > These are intended to be paired with done_dentry_lookup() which provides
> > the inverse of putting the dentry and unlocking.
> >
> > Like lookup_one_qstr_excl(), dentry_lookup() returns -ENOENT if
> > LOOKUP_CREATE was NOT given and the name cannot be found,, and returns
> > -EEXIST if LOOKUP_EXCL WAS given and the name CAN be found.
> >
> > These functions replace all uses of lookup_one_qstr_excl() in namei.c
> > except for those used for rename.
> >
> > Some of the variants should possibly be inlines in a header.
> >
> > Signed-off-by: NeilBrown <neil@...wn.name>
> > ---
> >  fs/namei.c            | 158 ++++++++++++++++++++++++++++++------------
> >  include/linux/namei.h |   8 ++-
> >  2 files changed, 119 insertions(+), 47 deletions(-)
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 950a0d0d54da..f292df61565a 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -1714,17 +1714,98 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> >  }
> >  EXPORT_SYMBOL(lookup_one_qstr_excl);
> >
> > +/**
> > + * dentry_lookup_hashed - lookup and lock a name prior to dir ops
> > + * @last: the name in the given directory
> > + * @base: the directory in which the name is to be found
> > + * @lookup_flags: %LOOKUP_xxx flags
> > + *
> > + * The name is looked up and necessary locks are taken so that
> > + * the name can be created or removed.
> > + * The "necessary locks" are currently the inode node lock on @base.
> > + * The name @last is expected to already have the hash calculated.
> > + * No permission checks are performed.
> > + * Returns: the dentry, suitably locked, or an ERR_PTR().
> > + */
> > +struct dentry *dentry_lookup_hashed(struct qstr *last,
> > +                                   struct dentry *base,
> > +                                   unsigned int lookup_flags)
> > +{
> > +       struct dentry *dentry;
> > +
> > +       inode_lock_nested(base->d_inode, I_MUTEX_PARENT);
> > +
> > +       dentry = lookup_one_qstr_excl(last, base, lookup_flags);
> > +       if (IS_ERR(dentry))
> > +               inode_unlock(base->d_inode);
> > +       return dentry;
> > +}
> > +EXPORT_SYMBOL(dentry_lookup_hashed);
> 
> Observation:
> 
> This part could be factored out of
> __kern_path_locked()/kern_path_locked_negative()

This patch does exactly that....

> 
> If you do that in patch 2 while introducing done_dentry_lookup() then
> it also makes
> a lot of sense to balance the introduced done_dentry_lookup() with the
> factored out
> helper __dentry_lookup_locked() or whatever its name is.

I don't think I want a __dentry_lookup_locked().  The lock and the
lookup need to be tightly connected.
But maybe I cold introduce dentry_lookup_hashed() in patch 2 ...
Or maybe call it __dentry_lookup() ??

Thanks,
NeilBrown


> 
> Thanks,
> Amir.
> 

Powered by blists - more mailing lists