On Fri, 07 Feb 2025, Christian Brauner wrote:
> On Thu, Feb 06, 2025 at 04:42:45PM +1100, NeilBrown wrote:
> > lookup_and_lock() combines locking the directory and performing a lookup
> > prior to a change to the directory.
> > Abstracting this prepares for changing the locking requirements.
> >
> > done_lookup_and_lock() provides the inverse of putting the dentry and
> > unlocking.
> >
> > For "silly_rename" we will need to lookup_and_lock() in a directory that
> > is already locked. For this purpose we add LOOKUP_PARENT_LOCKED.
> >
> > Like lookup_len_qstr(), lookup_and_lock() 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() in namei.c
> > except for those used for rename.
> >
> > The name might seem backwards as the lock happens before the lookup.
> > A future patch will change this so that only a shared lock is taken
> > before the lookup, and an exclusive lock on the dentry is taken after a
> > successful lookup. So the order "lookup" then "lock" will make sense.
> >
> > This functionality is exported as lookup_and_lock_one() which takes a
> > name and len rather than a qstr.
> >
> > Signed-off-by: NeilBrown <neilb@xxxxxxx>
> > ---
> > fs/namei.c | 102 ++++++++++++++++++++++++++++--------------
> > include/linux/namei.h | 15 ++++++-
> > 2 files changed, 83 insertions(+), 34 deletions(-)
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 69610047f6c6..3c0feca081a2 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -1715,6 +1715,41 @@ struct dentry *lookup_one_qstr(const struct qstr *name,
> > }
> > EXPORT_SYMBOL(lookup_one_qstr);
> >
> > +static struct dentry *lookup_and_lock_nested(const struct qstr *last,
> > + struct dentry *base,
> > + unsigned int lookup_flags,
> > + unsigned int subclass)
> > +{
> > + struct dentry *dentry;
> > +
> > + if (!(lookup_flags & LOOKUP_PARENT_LOCKED))
> > + inode_lock_nested(base->d_inode, subclass);
> > +
> > + dentry = lookup_one_qstr(last, base, lookup_flags);
> > + if (IS_ERR(dentry) && !(lookup_flags & LOOKUP_PARENT_LOCKED)) {
> > + inode_unlock(base->d_inode);
>
> Nit: The indentation here is wrong and the {} aren't common practice.
Thanks.
>
> > + }
> > + return dentry;
> > +}
> > +
> > +static struct dentry *lookup_and_lock(const struct qstr *last,
> > + struct dentry *base,
> > + unsigned int lookup_flags)
> > +{
> > + return lookup_and_lock_nested(last, base, lookup_flags,
> > + I_MUTEX_PARENT);
> > +}
> > +
> > +void done_lookup_and_lock(struct dentry *base, struct dentry *dentry,
> > + unsigned int lookup_flags)
>
> Did you mean done_lookup_and_unlock()?
No. The thing that we are done with is "lookup_and_lock()".
This matches "done_path_create()" which doesn't create anything.
On the other hand we have d_lookup_done() which puts _done at the end.
Or end_name_hash(). ->write_end(), finish_automount()
I guess I could accept done_lookup_and_unlock() if you prefer that.
>
> > +{
> > + d_lookup_done(dentry);
> > + dput(dentry);
> > + if (!(lookup_flags & LOOKUP_PARENT_LOCKED))
> > + inode_unlock(base->d_inode);
> > +}
> > +EXPORT_SYMBOL(done_lookup_and_lock);
> > +
> > /**
> > * lookup_fast - do fast lockless (but racy) lookup of a dentry
> > * @nd: current nameidata
> > @@ -2754,12 +2789,9 @@ static struct dentry *__kern_path_locked(int dfd, struct filename *name, struct
> > path_put(path);
> > return ERR_PTR(-EINVAL);
> > }
> > - inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
> > - d = lookup_one_qstr(&last, path->dentry, 0);
> > - if (IS_ERR(d)) {
> > - inode_unlock(path->dentry->d_inode);
> > + d = lookup_and_lock(&last, path->dentry, 0);
> > + if (IS_ERR(d))
> > path_put(path);
> > - }
> > return d;
> > }
> >
> > @@ -3053,6 +3085,22 @@ struct dentry *lookup_positive_unlocked(const char *name,
> > }
> > EXPORT_SYMBOL(lookup_positive_unlocked);
> >
> > +struct dentry *lookup_and_lock_one(struct mnt_idmap *idmap,
> > + const char *name, int len, struct dentry *base,
> > + unsigned int lookup_flags)
> > +{
> > + struct qstr this;
> > + int err;
> > +
> > + if (!idmap)
> > + idmap = &nop_mnt_idmap;
>
> The callers should pass nop_mnt_idmap. That's how every function that
> takes this argument works. This is a lot more explicit than magically
> fixing this up in the function.
OK.
Thanks,
NeilBrown
