Re: [PATCH v3 10/19] VFS/ovl: add d_alloc_noblock_return()

NeilBrown
2026-04-28T00:34:58Z

On Mon, 27 Apr 2026, Amir Goldstein wrote:
> On Mon, Apr 27, 2026 at 6:07 AM NeilBrown <neilb@ownmail.net> wrote:
> >
> > From: NeilBrown <neil@brown.name>
> >
> > ovl_lookup currently needs to check if a dentry with the same name has
> > already been added to the dcache as readdir might need to do. This
> > is an unnecessary performance cost to manage a rare race.
> >
> > If ovl could know which in-lookup dentries raced with readdir, it could
> > limit the extra lookup to just those.
> >
> > So add d_alloc_noblock_return() which provides the in-lookup dentry when
> > it returns -EWOULDBLOCK.
> >
> > ovl_readdir() can use this this and flag the dentry such that
> > ovl_lookup() and easily check if a repeat lookup is needed.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
>
> Very nice!
>
> One nit about the API
>
> > ---
> > fs/dcache.c | 50 ++++++++++++++++++++++++++++++++++++----
> > fs/overlayfs/namei.c | 23 ++++++++++--------
> > fs/overlayfs/overlayfs.h | 2 ++
> > fs/overlayfs/readdir.c | 7 ++++--
> > include/linux/dcache.h | 2 ++
> > 5 files changed, 68 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index a2ddfe811df3..2f11257b725b 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -2749,7 +2749,8 @@ enum alloc_para {
> > static inline
> > struct dentry *__d_alloc_parallel(struct dentry *parent,
> > const struct qstr *name,
> > - enum alloc_para how)
> > + enum alloc_para how,
> > + struct dentry **dentryp)
> > {
> > unsigned int hash = name->hash;
> > struct hlist_bl_head *b = in_lookup_hash(parent, hash);
> > @@ -2836,7 +2837,10 @@ struct dentry *__d_alloc_parallel(struct dentry *parent,
> > case ALLOC_PARA_FAIL:
> > spin_unlock(&dentry->d_lock);
> > dput(new);
> > - dput(dentry);
> > + if (dentryp)
> > + *dentryp = dentry;
> > + else
> > + dput(dentry);
> > return ERR_PTR(-EWOULDBLOCK);
> > case ALLOC_PARA_WAIT:
> > wait_var_event_spinlock(&dentry->d_flags,
> > @@ -2899,7 +2903,7 @@ struct dentry *__d_alloc_parallel(struct dentry *parent,
> > struct dentry *d_alloc_parallel(struct dentry *parent,
> > const struct qstr *name)
> > {
> > - return __d_alloc_parallel(parent, name, ALLOC_PARA_WAIT);
> > + return __d_alloc_parallel(parent, name, ALLOC_PARA_WAIT, NULL);
> > }
> > EXPORT_SYMBOL(d_alloc_parallel);
> >
> > @@ -2931,11 +2935,49 @@ struct dentry *d_alloc_noblock(struct dentry *parent,
> >
> > de = try_lookup_noperm(name, parent);
> > if (!de)
> > - de = __d_alloc_parallel(parent, name, ALLOC_PARA_FAIL);
> > + de = __d_alloc_parallel(parent, name, ALLOC_PARA_FAIL, NULL);
> > return de;
> > }
> > EXPORT_SYMBOL(d_alloc_noblock);
> >
> > +/**
> > + * d_alloc_noblock_return() - find or allocate a new dentry
> > + * @parent - dentry of the parent
> > + * @name - name of the dentry within that parent.
> > + * @dentryp - place to store the blocking dentry
> > + *
> > + * A new dentry is allocated and, providing it is unique, added to the
> > + * relevant index.
> > + * If an existing dentry is found with the same parent/name that is
> > + * not d_in_lookup() then that is returned instead.
> > + * If the existing dentry is d_in_lookup(), d_alloc_noblock()
> > + * returns with error %-EWOULDBLOCK and the blocking dentry is passed
> > + * in @dentryp. The dentry must be dput() by the caller.
>
> This contract is a bit subtle.
> We have plenty of contracts where the caller must dput() in case of success
> or in case of error, but must dput in case of a specific error that
> sounds fragile.
>
> How about:
> * If the existing dentry is d_in_lookup(), d_alloc_noblock()
> * returns with error %-EWOULDBLOCK and the blocking dentry is passed
> * in @dentryp. Regardless of the returned error, if @dentryp is set by this
> * function, the returned dentry must be dput() by the caller.

That is sensible, though I've used slightly different words.

Thanks,
NeilBrown

>
> Thanks,
> Amir.
>
> > + *
> > + * Thus if the returned dentry is d_in_lookup() then the caller has
> > + * exclusive access until it completes the lookup.
> > + * If the returned dentry is not d_in_lookup() then a lookup has
> > + * already completed.
> > + *
> > + * The @name need not already have ->hash set.
> > + *
> > + * Returns: the dentry, whether found or allocated, or an error
> > + * %-ENOMEM, %-EWOULDBLOCK, and anything returned by ->d_hash().
> > + */
> > +struct dentry *d_alloc_noblock_return(struct dentry *parent,
> > + struct qstr *name,
> > + struct dentry **dentryp)
> > +{
> > + struct dentry *de;
> > +
> > + de = try_lookup_noperm(name, parent);
> > + if (!de)
> > + de = __d_alloc_parallel(parent, name, ALLOC_PARA_FAIL,
> > + dentryp);
> > + return de;
> > +}
> > +EXPORT_SYMBOL(d_alloc_noblock_return);
> > +
> > /*
> > * - Unhash the dentry
> > * - Retrieve and clear the waitqueue head in dentry
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index 69032eb2b1e2..524e661c3c8d 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -1400,16 +1400,19 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> > if (dentry->d_name.len > ofs->namelen)
> > return ERR_PTR(-ENAMETOOLONG);
> >
> > - /*
> > - * The existance of this in-lookup dentry might have forced
> > - * readdir to do the lookup with a new dentry. If so we must
> > - * return that one.
> > - */
> > - alias = try_lookup_noperm(&QSTR_LEN(dentry->d_name.name,
> > - dentry->d_name.len),
> > - dentry->d_parent);
> > - if (alias && !IS_ERR(alias))
> > - return alias;
> > + if (ovl_dentry_test_flag(OVL_E_RACED_READDIR, dentry)) {
> > + ovl_dentry_clear_flag(OVL_E_RACED_READDIR, dentry);
> > + /*
> > + * The existance of this in-lookup dentry might have
> > + * forced readdir to do the lookup with a new dentry.
> > + * If so we must return that one.
> > + */
> > + alias = try_lookup_noperm(&QSTR_LEN(dentry->d_name.name,
> > + dentry->d_name.len),
> > + dentry->d_parent);
> > + if (alias && !IS_ERR(alias))
> > + return alias;
> > + }
> >
> > with_ovl_creds(dentry->d_sb)
> > err = ovl_lookup_layers(&ctx, &d);
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index b75df37f70ac..bd6f1a25aca1 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -71,6 +71,8 @@ enum ovl_entry_flag {
> > OVL_E_CONNECTED,
> > /* Lower stack may contain xwhiteout entries */
> > OVL_E_XWHITEOUTS,
> > + /* dentry was found in-lookup during readdir */
> > + OVL_E_RACED_READDIR,
> > };
> >
> > enum {
> > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> > index e03b32491941..e483bd939a8c 100644
> > --- a/fs/overlayfs/readdir.c
> > +++ b/fs/overlayfs/readdir.c
> > @@ -553,7 +553,7 @@ static int ovl_cache_update(const struct path *path, struct ovl_cache_entry *p,
> > {
> > struct dentry *dir = path->dentry;
> > struct ovl_fs *ofs = OVL_FS(dir->d_sb);
> > - struct dentry *this = NULL;
> > + struct dentry *this = NULL, *in_lookup;
> > enum ovl_path_type type;
> > u64 ino = p->real_ino;
> > int xinobits = ovl_xino_bits(ofs);
> > @@ -574,7 +574,8 @@ static int ovl_cache_update(const struct path *path, struct ovl_cache_entry *p,
> > }
> > }
> > /* This checks also for xwhiteouts */
> > - this = d_alloc_noblock(dir, &QSTR_LEN(p->name, p->len));
> > + this = d_alloc_noblock_return(dir, &QSTR_LEN(p->name, p->len),
> > + &in_lookup);
> > if (this == ERR_PTR(-EWOULDBLOCK)) {
> > /*
> > * Some other thead is looking up this name and will
> > @@ -583,6 +584,8 @@ static int ovl_cache_update(const struct path *path, struct ovl_cache_entry *p,
> > * lookup gets a turn it will find and return this
> > * dentry.
> > */
> > + ovl_dentry_set_flag(OVL_E_RACED_READDIR, in_lookup);
> > + dput(in_lookup);
> > this = d_alloc_name(dir, p->name);
> > }
> > if (!IS_ERR(this) && !d_unhashed(this)) {
> > diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> > index 662b98185337..db7cdcbac775 100644
> > --- a/include/linux/dcache.h
> > +++ b/include/linux/dcache.h
> > @@ -258,6 +258,8 @@ extern struct dentry * d_alloc(struct dentry *, const struct qstr *);
> > extern struct dentry * d_alloc_anon(struct super_block *);
> > extern struct dentry * d_alloc_parallel(struct dentry *, const struct qstr *);
> > extern struct dentry * d_alloc_noblock(struct dentry *, struct qstr *);
> > +extern struct dentry * d_alloc_noblock_return(struct dentry *, struct qstr *,
> > + struct dentry **);
> > extern struct dentry * d_splice_alias(struct inode *, struct dentry *);
> > struct dentry *d_duplicate(struct dentry *dentry);
> > /* weird procfs mess; *NOT* exported */
> > --
> > 2.50.0.107.gf914562f5916.dirty
> >
>