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() ??
Thanks,
NeilBrown
>
> Thanks,
> Amir.
>
