Re: [REGRESSION] nfs: Large amounts of GETATTR calls after file renaming on v5.10.241

[Trond Myklebust <trondmy@kernel.org>]

On Sun, 2025-11-23 at 07:23 +0100, gregkh@linuxfoundation.org wrote:
> On Sat, Nov 22, 2025 at 10:43:29AM -0500, Trond Myklebust wrote:
> > On Sat, 2025-11-22 at 07:56 +0100,
> > gregkh@linuxfoundation.org wrote:
> > > On Fri, Nov 21, 2025 at 06:56:31PM +0000, Ahmed, Aaron wrote:
> > > > Hi,
> > > > 
> > > > We have had customers report a regression on kernels versions
> > > > 5.10.241 and above in which file renaming causes large amounts
> > > > of
> > > > GETATTR calls to made due to inode revalidation. This
> > > > regression
> > > > was pinpointed via bisected to commit 7378c7adf31d ("NFS: Don't
> > > > set
> > > > NFS_INO_REVAL_PAGECACHE in the inode cache validity") which is
> > > > a
> > > > backport of 36a9346c2252 (“NFS: Don't set
> > > > NFS_INO_REVAL_PAGECACHE
> > > > in the inode cache validity”). 
> > > > 
> > > > We were able to reproduce It with this script:
> > > > REPRO_PATH=/mnt/efs/repro
> > > > do_read()
> > > > {
> > > >     for x in {1..50}
> > > >     do
> > > >         cat $1 > /dev/null
> > > >     done
> > > >     grep GETATTR /proc/self/mountstats
> > > > }
> > > > 
> > > > echo foo > $REPRO_PATH/bar
> > > > echo "After create, before read:"
> > > > grep GETATTR /proc/self/mountstats
> > > > 
> > > > echo "First read:"
> > > > do_read $REPRO_PATH/bar
> > > > 
> > > > echo "Sleeping 5s, reading again (should look the same):"
> > > > sleep 5
> > > > do_read $REPRO_PATH/bar
> > > > 
> > > > mv $REPRO_PATH/bar $REPRO_PATH/baz
> > > > echo "Moved file, reading again:"
> > > > do_read $REPRO_PATH/baz
> > > > 
> > > > echo "Immediately performing another set of reads:"
> > > > do_read $REPRO_PATH/baz
> > > > 
> > > > echo "Cleanup, removing test file"
> > > > rm $REPRO_PATH/baz
> > > > which performs a few read/writes. On kernels without the
> > > > regression
> > > > the number of GETATTR calls remains the same while on affected
> > > > kernels the amount increases after reading renamed file. 
> > > > 
> > > > This original commit comes from a series of patches providing
> > > > attribute revalidation updates [1].  However, many of these
> > > > patches
> > > > are missing in v.5.10.241+. Specifically, 13c0b082b6a9 (“NFS:
> > > > Replace use of NFS_INO_REVAL_PAGECACHE when checking cache
> > > > validity”) seems like a prerequisite patch and would help
> > > > remedy
> > > > the regression.
> > > 
> > > Can you please send the needed backports to resolve this issue as
> > > you
> > > can test and verify that this resolves the problem?
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > Ah... If I'm correctly reading the changelog
> > in 
> > https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fc
> > dn.kernel.org%2Fpub%2Flinux%2Fkernel%2Fv5.x%2FChangeLog-
> > 5.10.241&data=05%7C02%7Ctrondmy%40hammerspace.com%7Cd99de46ea1054d1
> > a48c408de2a58f676%7C0d4fed5c3a7046fe9430ece41741f59e%7C0%7C0%7C6389
> > 94758780825772%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYi
> > OiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C
> > 0%7C%7C%7C&sdata=EbT6VZycJOQ7ktWkCq5jDYG9NdhP7vlkiK%2FuK9nIp9k%3D&r
> > eserved=0, it
> > looks as if commit 36a9346c2252 got pulled in by the stable patch
> > automation as a dependency for commit b01f21cacde9 ("NFS: Fix the
> > setting of capabilities when automounting a new filesystem").
> > 
> > I do agree with Aaron's assessment that patch does depend on the
> > rest
> > of the series that was merged into Linux 5.13. It cannot be cherry-
> > picked on its own.
> > 
> > However what exactly was the dependency that caused it to be pulled
> > into 5.10.241 in the first place? Was that logged anywhere?
> > I just checked out v5.10.241 and applied "git revert --signoff
> > 36a9346c2252", and that appears to work just fine, and I don't see
> > any
> > other obvious code dependency there, so I'm curious about what
> > happened.
> 
> As the commit says:
> 	Stable-dep-of: b01f21cacde9 ("NFS: Fix the setting of
> capabilities when automounting a new filesystem")
> 
> 
If that's all we have, then I suggest just applying the following
revert.

8<------------------------------------------------------------------
From 3ae48f4eccfe9dcfeb9b3ff3670cadaed7b35c9e Mon Sep 17 00:00:00 2001
Message-ID: <3ae48f4eccfe9dcfeb9b3ff3670cadaed7b35c9e.1763918082.git.trond.myklebust@hammerspace.com>
From: Trond Myklebust <trond.myklebust@hammerspace.com>
Date: Sat, 22 Nov 2025 10:28:09 -0500
Subject: [PATCH] Revert "NFS: Don't set NFS_INO_REVAL_PAGECACHE in the inode
 cache validity"

This reverts commit 36a9346c225270262d9f34e66c91aa1723fa903f.

The above commit was incorrectly labelled as a dependency for commit
b01f21cacde9 ("NFS: Fix the setting of capabilities when automounting a
new filesystem")
A revert is needed, since the incorrectly applied commit depends upon a
series of other patches that were merged into Linux 5.13, but have not
been applied to the 5.10 stable series.

Reported-by: "Ahmed, Aaron" <aarnahmd@amazon.com>
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/inode.c    | 6 ++++--
 fs/nfs/nfs4proc.c | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index da8d727eb09d..3e3114a9d193 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -217,12 +217,11 @@ static void nfs_set_cache_invalid(struct inode *inode, unsigned long flags)
 			flags &= ~NFS_INO_INVALID_OTHER;
 		flags &= ~(NFS_INO_INVALID_CHANGE
 				| NFS_INO_INVALID_SIZE
+				| NFS_INO_REVAL_PAGECACHE
 				| NFS_INO_INVALID_XATTR);
 	} else if (flags & NFS_INO_REVAL_PAGECACHE)
 		flags |= NFS_INO_INVALID_CHANGE | NFS_INO_INVALID_SIZE;
 
-	flags &= ~NFS_INO_REVAL_PAGECACHE;
-
 	if (!nfs_has_xattr_cache(nfsi))
 		flags &= ~NFS_INO_INVALID_XATTR;
 	if (inode->i_mapping->nrpages == 0)
@@ -1901,6 +1900,7 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 	nfsi->cache_validity &= ~(NFS_INO_INVALID_ATTR
 			| NFS_INO_INVALID_ATIME
 			| NFS_INO_REVAL_FORCED
+			| NFS_INO_REVAL_PAGECACHE
 			| NFS_INO_INVALID_BLOCKS);
 
 	/* Do atomic weak cache consistency updates */
@@ -1942,6 +1942,7 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 	} else {
 		nfsi->cache_validity |= save_cache_validity &
 				(NFS_INO_INVALID_CHANGE
+				| NFS_INO_REVAL_PAGECACHE
 				| NFS_INO_REVAL_FORCED);
 		cache_revalidated = false;
 	}
@@ -1987,6 +1988,7 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 	} else {
 		nfsi->cache_validity |= save_cache_validity &
 				(NFS_INO_INVALID_SIZE
+				| NFS_INO_REVAL_PAGECACHE
 				| NFS_INO_REVAL_FORCED);
 		cache_revalidated = false;
 	}
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 973b708ff332..2c33436bb27e 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1210,6 +1210,7 @@ nfs4_update_changeattr_locked(struct inode *inode,
 		| cache_validity;
 
 	if (cinfo->atomic && cinfo->before == inode_peek_iversion_raw(inode)) {
+		nfsi->cache_validity &= ~NFS_INO_REVAL_PAGECACHE;
 		nfsi->attrtimeo_timestamp = jiffies;
 	} else {
 		if (S_ISDIR(inode->i_mode)) {
-- 
2.51.1