If we skip lookup revalidataion while holding a delegation, we might miss
that the file has changed directories on the server. This can happen if
the file is moved in between the client caching a dentry and obtaining a
delegation. That can be reproduced on a single system with this bash:
mkdir -p /exports/dir{1,2}
exportfs -o rw localhost:/exports
mount -t nfs -ov4.1,noac localhost:/exports /mnt/localhost
touch /exports/dir1/fubar
cat /mnt/localhost/dir1/fubar
mv /mnt/localhost/dir{1,2}/fubar
mv /exports/dir{2,1}/fubar
cat /mnt/localhost/dir1/fubar
mv /mnt/localhost/dir{1,2}/fubar
In this example, the final `mv` will stat source and destination and find
they are the same dentry.
To fix this without giving up the increased lookup performance that holding
a delegation provides, let's revalidate the dentry only once after
obtaining a delegation by placing a magic value in the dentry's verifier.
Suggested-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
Signed-off-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
---
fs/nfs/dir.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index e180033e35cf..e9d07dcd6d6f 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -949,6 +949,7 @@ static int nfs_fsync_dir(struct file *filp, loff_t start, loff_t end,
return 0;
}
+#define NFS_DELEGATION_VERF 0xfeeddeaf
/**
* nfs_force_lookup_revalidate - Mark the directory as having changed
* @dir: pointer to directory inode
@@ -962,6 +963,8 @@ static int nfs_fsync_dir(struct file *filp, loff_t start, loff_t end,
void nfs_force_lookup_revalidate(struct inode *dir)
{
NFS_I(dir)->cache_change_attribute++;
+ if (NFS_I(dir)->cache_change_attribute == NFS_DELEGATION_VERF)
+ NFS_I(dir)->cache_change_attribute++;
}
EXPORT_SYMBOL_GPL(nfs_force_lookup_revalidate);
@@ -1133,6 +1136,7 @@ nfs_lookup_revalidate_dentry(struct inode *dir, struct dentry *dentry,
struct nfs_fh *fhandle;
struct nfs_fattr *fattr;
struct nfs4_label *label;
+ unsigned long verifier;
int ret;
ret = -ENOMEM;
@@ -1154,6 +1158,11 @@ nfs_lookup_revalidate_dentry(struct inode *dir, struct dentry *dentry,
if (nfs_refresh_inode(inode, fattr) < 0)
goto out;
+ if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
+ verifier = NFS_DELEGATION_VERF;
+ else
+ verifier = nfs_save_change_attribute(dir);
+
nfs_setsecurity(inode, fattr, label);
nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
@@ -1167,6 +1176,15 @@ nfs_lookup_revalidate_dentry(struct inode *dir, struct dentry *dentry,
return nfs_lookup_revalidate_done(dir, dentry, inode, ret);
}
+static int nfs_delegation_matches_dentry(struct inode *dir,
+ struct dentry *dentry, struct inode *inode)
+{
+ if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ) &&
+ dentry->d_time == NFS_DELEGATION_VERF)
+ return 1;
+ return 0;
+}
+
/*
* This is called every time the dcache has a lookup hit,
* and we should check whether we can really trust that
@@ -1197,7 +1215,7 @@ nfs_do_lookup_revalidate(struct inode *dir, struct dentry *dentry,
goto out_bad;
}
- if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
+ if (nfs_delegation_matches_dentry(dir, dentry, inode))
return nfs_lookup_revalidate_delegated(dir, dentry, inode);
/* Force a full look up iff the parent directory has changed */
@@ -1635,7 +1653,7 @@ nfs4_do_lookup_revalidate(struct inode *dir, struct dentry *dentry,
if (inode == NULL)
goto full_reval;
- if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
+ if (nfs_delegation_matches_dentry(dir, dentry, inode))
return nfs_lookup_revalidate_delegated(dir, dentry, inode);
/* NFS only supports OPEN on regular files */
--
2.20.1
