Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Benjamin Coddington <bcodding@redhat.com>
To:     Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <Anna.Schumaker@netapp.com>
Cc:     linux-nfs@vger.kernel.org
Subject: [PATCH v2] NFS: Revalidate once when holding a delegation
Date:   Tue, 28 Jan 2020 10:26:57 -0500
Message-Id: <bcb5ffd399c4434730e6d100a5b7cae5e207244e.1580225161.git.bcodding@redhat.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

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 holdi=
ng
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@hammerspace.com>
Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 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 st=
art, loff_t end,
 	return 0;
 }
=20
+#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 st=
art, loff_t end,
 void nfs_force_lookup_revalidate(struct inode *dir)
 {
 	NFS_I(dir)->cache_change_attribute++;
+	if (NFS_I(dir)->cache_change_attribute =3D=3D NFS_DELEGATION_VERF)
+		NFS_I(dir)->cache_change_attribute++;
 }
 EXPORT_SYMBOL_GPL(nfs_force_lookup_revalidate);
=20
@@ -1133,6 +1136,7 @@ nfs_lookup_revalidate_dentry(struct inode *dir, str=
uct dentry *dentry,
 	struct nfs_fh *fhandle;
 	struct nfs_fattr *fattr;
 	struct nfs4_label *label;
+	unsigned long verifier;
 	int ret;
=20
 	ret =3D -ENOMEM;
@@ -1154,6 +1158,11 @@ nfs_lookup_revalidate_dentry(struct inode *dir, st=
ruct dentry *dentry,
 	if (nfs_refresh_inode(inode, fattr) < 0)
 		goto out;
=20
+	if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
+		verifier =3D NFS_DELEGATION_VERF;
+	else
+		verifier =3D nfs_save_change_attribute(dir);
+
 	nfs_setsecurity(inode, fattr, label);
 	nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
=20
@@ -1167,6 +1176,15 @@ nfs_lookup_revalidate_dentry(struct inode *dir, st=
ruct dentry *dentry,
 	return nfs_lookup_revalidate_done(dir, dentry, inode, ret);
 }
=20
+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 =3D=3D 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;
 	}
=20
-	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);
=20
 	/* 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 =3D=3D NULL)
 		goto full_reval;
=20
-	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);
=20
 	/* NFS only supports OPEN on regular files */
--=20
2.20.1