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

On Mon, Apr 27, 2026 at 6:07 AM NeilBrown <[email protected]> wrote:
>
> From: NeilBrown <[email protected]>
>
> 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 <[email protected]>

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.

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
>