Re: [PATCH] VFS: change try_lookup_noperm() to skip revalidation

[Steve French]

Tested-by: Steve French <stfrench@xxxxxxxxxxxxx>

I verified that it fixed the performance regression in generic/676
(see e.g. a full test run this morning with the patch on 6.16-rc1
http://smb311-linux-testing.southcentralus.cloudapp.azure.com/#/builders/3/builds/497),
the test took 10:48 vs. 23 to 30 minutes without the patch.

I also saw similar performance yesterday with 6.16-rc1 with reverting
the patch ("Use try_lookup_noperm() instead of d_hash_and_lookup()
outside of VFS"
) For that run test generic/676 took 9:32.

On Mon, Jun 9, 2025 at 8:04 PM NeilBrown <neil@xxxxxxxxxx> wrote:
>
>
> The recent change from using d_hash_and_lookup() to using
> try_lookup_noperm() inadvertently introduce a d_revalidate() call when
> the lookup was successful.  Steven French reports that this resulted in
> worse than halving of performance in some cases.
>
> Prior to the offending patch the only caller of try_lookup_noperm() was
> autofs which does not need the d_revalidate().  So it is safe to remove
> the d_revalidate() call providing we stop using try_lookup_noperm() to
> implement lookup_noperm().
>
> The "try_" in the name is strongly suggestive that the caller isn't
> expecting much effort, so it seems reasonable to avoid the effort of
> d_revalidate().
>
> Fixes: 06c567403ae5 ("Use try_lookup_noperm() instead of d_hash_and_lookup() outside of VFS")
> Reported-by: Steve French <smfrench@xxxxxxxxx>
> Link: https://lore.kernel.org/all/CAH2r5mu5SfBrdc2CFHwzft8=n9koPMk+Jzwpy-oUMx-wCRCesQ@xxxxxxxxxxxxxx/
> Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
> ---
>  fs/namei.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 4bb889fc980b..f761cafaeaad 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2917,7 +2917,8 @@ static int lookup_one_common(struct mnt_idmap *idmap,
>   * @base:      base directory to lookup from
>   *
>   * Look up a dentry by name in the dcache, returning NULL if it does not
> - * currently exist.  The function does not try to create a dentry.
> + * currently exist.  The function does not try to create a dentry and if one
> + * is found it doesn't try to revalidate it.
>   *
>   * Note that this routine is purely a helper for filesystem usage and should
>   * not be called by generic code.  It does no permission checking.
> @@ -2933,7 +2934,7 @@ struct dentry *try_lookup_noperm(struct qstr *name, struct dentry *base)
>         if (err)
>                 return ERR_PTR(err);
>
> -       return lookup_dcache(name, base, 0);
> +       return d_lookup(base, name);
>  }
>  EXPORT_SYMBOL(try_lookup_noperm);
>
> @@ -3057,14 +3058,22 @@ EXPORT_SYMBOL(lookup_one_positive_unlocked);
>   * Note that this routine is purely a helper for filesystem usage and should
>   * not be called by generic code. It does no permission checking.
>   *
> - * Unlike lookup_noperm, it should be called without the parent
> + * Unlike lookup_noperm(), it should be called without the parent
>   * i_rwsem held, and will take the i_rwsem itself if necessary.
> + *
> + * Unlike try_lookup_noperm() it *does* revalidate the dentry if it already
> + * existed.
>   */
>  struct dentry *lookup_noperm_unlocked(struct qstr *name, struct dentry *base)
>  {
>         struct dentry *ret;
> +       int err;
>
> -       ret = try_lookup_noperm(name, base);
> +       err = lookup_noperm_common(name, base);
> +       if (err)
> +               return ERR_PTR(err);
> +
> +       ret = lookup_dcache(name, base, 0);
>         if (!ret)
>                 ret = lookup_slow(name, base, 0);
>         return ret;
> --
> 2.49.0
>


-- 
Thanks,

Steve