On Tue, Jul 22, 2025 at 1:27 AM NeilBrown <neil@xxxxxxxxxx> wrote:
>
> On Mon, 21 Jul 2025, Amir Goldstein wrote:
> > On Mon, Jul 21, 2025 at 10:55 AM NeilBrown <neil@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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() ??
__dentry_lookup() sounds good to me.
Thanks,
Amir.
