From: Trond Myklebust <[email protected]>
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 <[email protected]>
Fixes: 5ceb9d7fdaaf ("NFS: Refactor nfs_lookup_revalidate()")
Signed-off-by: Trond Myklebust <[email protected]>
---
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
From: Trond Myklebust <[email protected]>
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 <[email protected]>
---
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
On Tue, Mar 02, 2021 at 11:28:35PM -0500, [email protected] wrote:
> From: Trond Myklebust <[email protected]>
>
> 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 <[email protected]>
> Reported-by: Geert Jansen <[email protected]>
> Fixes: 5ceb9d7fdaaf ("NFS: Refactor nfs_lookup_revalidate()")
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> 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
>