[PATCH 1/2] NFS: Don't revalidate the directory permissions on a lookup failure

[PATCH 1/2] NFS: Don't revalidate the directory permissions on a lookup failure

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

There should be no reason to expect the directory permissions to change
just because the directory contents changed or a negative lookup timed
out. So let's avoid doing a full call to nfs_mark_for_revalidate() in
that case.
Furthermore, if this is a negative dentry, and we haven't actually done
a new lookup, then we have no reason yet to believe the directory has
changed at all. So let's remove the gratuitous directory inode
invalidation altogether when called from
nfs_lookup_revalidate_negative().

Reported-by: Geert Jansen <gerardu@amazon.com>
Fixes: 5ceb9d7fdaaf ("NFS: Refactor nfs_lookup_revalidate()")
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/dir.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 19a9f434442f..6350873cb8bd 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1401,6 +1401,15 @@ int nfs_lookup_verify_inode(struct inode *inode, unsigned int flags)
 	goto out;
 }
 
+static void nfs_mark_dir_for_revalidate(struct inode *inode)
+{
+	struct nfs_inode *nfsi = NFS_I(inode);
+
+	spin_lock(&inode->i_lock);
+	nfsi->cache_validity |= NFS_INO_REVAL_PAGECACHE;
+	spin_unlock(&inode->i_lock);
+}
+
 /*
  * We judge how long we want to trust negative
  * dentries by looking at the parent inode mtime.
@@ -1435,7 +1444,6 @@ nfs_lookup_revalidate_done(struct inode *dir, struct dentry *dentry,
 			__func__, dentry);
 		return 1;
 	case 0:
-		nfs_mark_for_revalidate(dir);
 		if (inode && S_ISDIR(inode->i_mode)) {
 			/* Purge readdir caches. */
 			nfs_zap_caches(inode);
@@ -1525,6 +1533,8 @@ nfs_lookup_revalidate_dentry(struct inode *dir, struct dentry *dentry,
 	nfs_free_fattr(fattr);
 	nfs_free_fhandle(fhandle);
 	nfs4_label_free(label);
+	if (!ret)
+		nfs_mark_dir_for_revalidate(dir);
 	return nfs_lookup_revalidate_done(dir, dentry, inode, ret);
 }
 
@@ -1567,7 +1577,7 @@ nfs_do_lookup_revalidate(struct inode *dir, struct dentry *dentry,
 		error = nfs_lookup_verify_inode(inode, flags);
 		if (error) {
 			if (error == -ESTALE)
-				nfs_zap_caches(dir);
+				nfs_mark_dir_for_revalidate(dir);
 			goto out_bad;
 		}
 		nfs_advise_use_readdirplus(dir);
@@ -2064,7 +2074,7 @@ nfs_add_or_obtain(struct dentry *dentry, struct nfs_fh *fhandle,
 	dput(parent);
 	return d;
 out_error:
-	nfs_mark_for_revalidate(dir);
+	nfs_mark_dir_for_revalidate(dir);
 	d = ERR_PTR(error);
 	goto out;
 }
-- 
2.29.2

[PATCH 2/2] NFS: Don't gratuitously clear the inode cache when lookup failed

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

The fact that the lookup revalidation failed, does not mean that the
inode contents have changed.

Fixes: 5ceb9d7fdaaf ("NFS: Refactor nfs_lookup_revalidate()")
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/dir.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 6350873cb8bd..deb6ad0622ed 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1444,18 +1444,14 @@ nfs_lookup_revalidate_done(struct inode *dir, struct dentry *dentry,
 			__func__, dentry);
 		return 1;
 	case 0:
-		if (inode && S_ISDIR(inode->i_mode)) {
-			/* Purge readdir caches. */
-			nfs_zap_caches(inode);
-			/*
-			 * We can't d_drop the root of a disconnected tree:
-			 * its d_hash is on the s_anon list and d_drop() would hide
-			 * it from shrink_dcache_for_unmount(), leading to busy
-			 * inodes on unmount and further oopses.
-			 */
-			if (IS_ROOT(dentry))
-				return 1;
-		}
+		/*
+		 * We can't d_drop the root of a disconnected tree:
+		 * its d_hash is on the s_anon list and d_drop() would hide
+		 * it from shrink_dcache_for_unmount(), leading to busy
+		 * inodes on unmount and further oopses.
+		 */
+		if (inode && IS_ROOT(dentry))
+			return 1;
 		dfprintk(LOOKUPCACHE, "NFS: %s(%pd2) is invalid\n",
 				__func__, dentry);
 		return 0;
-- 
2.29.2

Re: [PATCH 1/2] NFS: Don't revalidate the directory permissions on a lookup failure

[Geert Jansen]

On Tue, Mar 02, 2021 at 11:28:35PM -0500, trondmy@kernel.org wrote:

> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> There should be no reason to expect the directory permissions to change
> just because the directory contents changed or a negative lookup timed
> out. So let's avoid doing a full call to nfs_mark_for_revalidate() in
> that case.
> Furthermore, if this is a negative dentry, and we haven't actually done
> a new lookup, then we have no reason yet to believe the directory has
> changed at all. So let's remove the gratuitous directory inode
> invalidation altogether when called from
> nfs_lookup_revalidate_negative().

Thanks! I tested this patch and 2/2 from this series, and I can confirm
that it addresses the issue that we were seeing.

Tested-by: Geert Jansen <gerardu@amazon.com>

> Reported-by: Geert Jansen <gerardu@amazon.com>
> Fixes: 5ceb9d7fdaaf ("NFS: Refactor nfs_lookup_revalidate()")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfs/dir.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 19a9f434442f..6350873cb8bd 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -1401,6 +1401,15 @@ int nfs_lookup_verify_inode(struct inode *inode, unsigned int flags)
>         goto out;
>  }
> 
> +static void nfs_mark_dir_for_revalidate(struct inode *inode)
> +{
> +       struct nfs_inode *nfsi = NFS_I(inode);
> +
> +       spin_lock(&inode->i_lock);
> +       nfsi->cache_validity |= NFS_INO_REVAL_PAGECACHE;
> +       spin_unlock(&inode->i_lock);
> +}
> +
>  /*
>   * We judge how long we want to trust negative
>   * dentries by looking at the parent inode mtime.
> @@ -1435,7 +1444,6 @@ nfs_lookup_revalidate_done(struct inode *dir, struct dentry *dentry,
>                         __func__, dentry);
>                 return 1;
>         case 0:
> -               nfs_mark_for_revalidate(dir);
>                 if (inode && S_ISDIR(inode->i_mode)) {
>                         /* Purge readdir caches. */
>                         nfs_zap_caches(inode);
> @@ -1525,6 +1533,8 @@ nfs_lookup_revalidate_dentry(struct inode *dir, struct dentry *dentry,
>         nfs_free_fattr(fattr);
>         nfs_free_fhandle(fhandle);
>         nfs4_label_free(label);
> +       if (!ret)
> +               nfs_mark_dir_for_revalidate(dir);
>         return nfs_lookup_revalidate_done(dir, dentry, inode, ret);
>  }
> 
> @@ -1567,7 +1577,7 @@ nfs_do_lookup_revalidate(struct inode *dir, struct dentry *dentry,
>                 error = nfs_lookup_verify_inode(inode, flags);
>                 if (error) {
>                         if (error == -ESTALE)
> -                               nfs_zap_caches(dir);
> +                               nfs_mark_dir_for_revalidate(dir);
>                         goto out_bad;
>                 }
>                 nfs_advise_use_readdirplus(dir);
> @@ -2064,7 +2074,7 @@ nfs_add_or_obtain(struct dentry *dentry, struct nfs_fh *fhandle,
>         dput(parent);
>         return d;
>  out_error:
> -       nfs_mark_for_revalidate(dir);
> +       nfs_mark_dir_for_revalidate(dir);
>         d = ERR_PTR(error);
>         goto out;
>  }
> --
> 2.29.2
>