Re: [PATCH 03/19] VFS: use d_alloc_parallel() in lookup_one_qstr_excl() and rename it.

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2025-02-07 at 11:04 +1100, NeilBrown wrote:
> On Fri, 07 Feb 2025, Jeff Layton wrote:
> > On Thu, 2025-02-06 at 16:42 +1100, NeilBrown wrote:
> > > lookup_one_qstr_excl() is used for lookups prior to directory
> > > modifications, whether create, unlink, rename, or whatever.
> > > 
> > > To prepare for allowing modification to happen in parallel, change
> > > lookup_one_qstr_excl() to use d_alloc_parallel().
> > > 
> > > To reflect this, name is changed to lookup_one_qtr() - as the directory
> > > may be locked shared.
> > > 
> > > If any for the "intent" LOOKUP flags are passed, the caller must ensure
> > > d_lookup_done() is called at an appropriate time.  If none are passed
> > > then we can be sure ->lookup() will do a real lookup and d_lookup_done()
> > > is called internally.
> > > 
> > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > ---
> > >  fs/namei.c            | 47 +++++++++++++++++++++++++------------------
> > >  fs/smb/server/vfs.c   |  7 ++++---
> > >  include/linux/namei.h |  9 ++++++---
> > >  3 files changed, 37 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index 5cdbd2eb4056..d684102d873d 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -1665,15 +1665,13 @@ static struct dentry *lookup_dcache(const struct qstr *name,
> > >  }
> > >  
> > >  /*
> > > - * Parent directory has inode locked exclusive.  This is one
> > > - * and only case when ->lookup() gets called on non in-lookup
> > > - * dentries - as the matter of fact, this only gets called
> > > - * when directory is guaranteed to have no in-lookup children
> > > - * at all.
> > > + * Parent directory has inode locked: exclusive or shared.
> > > + * If @flags contains any LOOKUP_INTENT_FLAGS then d_lookup_done()
> > > + * must be called after the intended operation is performed - or aborted.
> > >   */
> > > -struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> > > -				    struct dentry *base,
> > > -				    unsigned int flags)
> > > +struct dentry *lookup_one_qstr(const struct qstr *name,
> > > +			       struct dentry *base,
> > > +			       unsigned int flags)
> > >  {
> > >  	struct dentry *dentry = lookup_dcache(name, base, flags);
> > >  	struct dentry *old;
> > > @@ -1686,18 +1684,25 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> > >  	if (unlikely(IS_DEADDIR(dir)))
> > >  		return ERR_PTR(-ENOENT);
> > >  
> > > -	dentry = d_alloc(base, name);
> > > -	if (unlikely(!dentry))
> > > +	dentry = d_alloc_parallel(base, name);
> > > +	if (unlikely(IS_ERR_OR_NULL(dentry)))
> > >  		return ERR_PTR(-ENOMEM);
> > > +	if (!d_in_lookup(dentry))
> > > +		/* Raced with another thread which did the lookup */
> > > +		return dentry;
> > >  
> > >  	old = dir->i_op->lookup(dir, dentry, flags);
> > >  	if (unlikely(old)) {
> > > +		d_lookup_done(dentry);
> > >  		dput(dentry);
> > >  		dentry = old;
> > >  	}
> > > +	if ((flags & LOOKUP_INTENT_FLAGS) == 0)
> > > +		/* ->lookup must have given final answer */
> > > +		d_lookup_done(dentry);
> > 
> > This is kind of an ugly thing for the callers to get right. I think it
> > would be cleaner to just push the d_lookup_done() into all of the
> > callers that don't pass any intent flags, and do away with this.
> 
> I don't understand your concern.  This does not impose on callers,
> rather it relieves them of a burden.  d_lookup_done() is fully
> idempotent so if a caller does call it, there is no harm done.
> 
> In the final result of my series there are 4 callers of this function.
> 1/ lookup_and_lock() which must always be balanced with
>   done_lookup_and_lock(), which calls d_lookup_done()
> 2/ lookup_and_lock_rename() which is similarly balance with
>   done_lookup_and_lock_rename(). 
> 3/ ksmbd_vfs_path_lookup_locked() which passes zero for the flags and so
>    doesn't need d_lookup_done()
> 4/ ksmbd_vfs_rename() which calls d_lookup_done() as required.
> 
> So if I dropped this code it would only affect one caller which would
> need to add a call to d_lookup_done() probably immediately after the
> successful return of lookup_one_qstr().
> While that wouldn't hurt much, I don't see that it would help much
> either.
> 

My concern is about the complex return handling. If the flags are 0,
then I don't need to call d_lookup_done(), but if they aren't 0, then I
do. That's just an easy opportunity to get it wrong if new callers are
added.

My preference would be that the caller must always call d_lookup_done()
on a successful return. If ksmbd_vfs_path_lookup_locked() has to call
it immediately afterward, then that's fine. No need for this special
handling in a generic function, just for a single caller.
-- 
Jeff Layton <jlayton@kernel.org>