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