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
