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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx>
> > ---
> >  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.

Thanks,
NeilBrown





[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux