[PATCH RFC 11/12] nfs: look a name up again once its removal has unhashed it

[Jonathan Flynn]

An open or lookup of a name can wait for a removal of the same name made
with the directory locked shared, and is then ordered after it.  When
that removal succeeds while the waiter still holds the name's cache
entry, the entry is only taken out of the cache, not emptied, so the
waiter is left holding an entry for a name that no longer exists.
Revalidation can then accept it: an open without O_EXCL is sent to the
server for the removed file, and if the client holds a delegation for
the directory, an exclusive create of the name fails with "file exists".

When NFSv4's revalidation has waited for such a removal and finds the
entry out of the cache once the wait ends, have it answer that the entry
is invalid, so that the name is looked up again;
__nfs_lookup_revalidate() now reports whether the mark it waited for was
the shared one.  Without such a wait, an entry out of the cache goes
through the usual checks, as before.  Its holder may be unable to look
the name up again: overlayfs revalidates the entries of its lower layers
itself, and an answer of "invalid" for one that a transient error took
out of the cache would leave the overlay reporting it stale until it
dropped its own entry.  A lookup that took the entry while the removal
was in flight but finds the mark already clear is not covered either;
it overlapped the removal, so the old entry is still an answer it may
be given.

This relies on the VFS taking the entry out of the cache before it calls
->unlink_shared_done(), which clears the mark, and on the wait reading
the mark with acquire ordering, which pairs with the release in the
clear: a lookup that finds the mark clear, whether it slept or not, then
also sees the entry out of the cache.  A removal made with the directory
locked exclusive, and a rename onto the name, can clear the mark before
the entry is taken out of the cache; say so at the wait.

Signed-off-by: Jonathan Flynn <jonathan.flynn@xxxxxxxxxxxxxxx>
---
 fs/nfs/dir.c | 50 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 0b6de87030..1f255001a1 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1889,6 +1889,11 @@ nfs_do_lookup_revalidate(struct inode *dir, const struct qstr *name,
 	return nfs_lookup_revalidate_done(dir, dentry, inode, error);
 }
 
+/*
+ * Answers -ECHILD under LOOKUP_RCU while a removal of the name is in
+ * flight.  Otherwise waits for any such removal to end, and answers 1 if
+ * the one it found was made with the parent held shared, else 0.
+ */
 static int
 __nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 {
@@ -1898,15 +1903,28 @@ __nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 			return -ECHILD;
 	} else {
 		void **fsdata = &dentry->d_fsdata;
+		void *mark;
 
-		/* Wait for unlink to complete - see unblock_revalidate().
-		 * smp_load_acquire() ensures ->d_fsdata is read after
-		 * ->d_seq was read, matching block_revalidate() which
-		 * sets ->d_fsdata before ->d_seq.
+		/*
+		 * Wait for unlink to complete - see unblock_revalidate().  One
+		 * load, as there are two values to test.  smp_load_acquire()
+		 * ensures ->d_fsdata is read after ->d_seq was read, matching
+		 * __block_revalidate(), which sets ->d_fsdata before ->d_seq.
+		 * The acquire in the wait pairs with the release in
+		 * unblock_revalidate(): a waiter that finds the mark clear
+		 * sees what the remover did before clearing it.  For a removal
+		 * made with the parent held shared, that includes the unhash of
+		 * a dentry it took away, as the VFS calls d_delete() before
+		 * ->unlink_shared_done(); nfs_unlink() and nfs_rename() may
+		 * clear the mark first and unhash after.
 		 */
+		mark = smp_load_acquire(fsdata);
+		if (!nfs_fsdata_blocked(mark))
+			return 0;
 		wait_var_event(fsdata,
-			       /* one load: there are two values to test */
+			       /* acquire: pairs with unblock_revalidate() */
 			       !nfs_fsdata_blocked(smp_load_acquire(fsdata)));
+		return nfs_fsdata_blocked_shared(mark);
 	}
 	return 0;
 }
@@ -1914,7 +1932,7 @@ __nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 static int nfs_lookup_revalidate(struct inode *dir, const struct qstr *name,
 				 struct dentry *dentry, unsigned int flags)
 {
-	if (__nfs_lookup_revalidate(dentry, flags))
+	if (__nfs_lookup_revalidate(dentry, flags) < 0)
 		return -ECHILD;
 	return nfs_do_lookup_revalidate(dir, name, dentry, flags);
 }
@@ -2329,10 +2347,28 @@ nfs4_lookup_revalidate(struct inode *dir, const struct qstr *name,
 		       struct dentry *dentry, unsigned int flags)
 {
 	struct inode *inode;
+	int ret;
 
-	if (__nfs_lookup_revalidate(dentry, flags))
+	ret = __nfs_lookup_revalidate(dentry, flags);
+	if (ret < 0)
 		return -ECHILD;
 
+	/*
+	 * A removal made with the parent held shared that succeeded while
+	 * this lookup waited for it, holding the dentry, has left the dentry
+	 * positive but unhashed: d_delete() empties a dentry only for its last
+	 * user.  The dentry no longer stands for its name, so have the caller
+	 * look the name up again; the checks below could accept it, since
+	 * with a directory delegation the removal stamped it delegated.  Only
+	 * ref-walk waits, so this never answers 0 under LOOKUP_RCU.  A dentry
+	 * found unhashed without such a wait is left to the checks below, as
+	 * before: its holder may be unable to look the name up again, as
+	 * overlayfs is for the dentries of its lower layers, which it
+	 * revalidates itself.
+	 */
+	if (ret && d_unhashed(dentry))
+		return 0;
+
 	trace_nfs_lookup_revalidate_enter(dir, dentry, flags);
 
 	if (!(flags & LOOKUP_OPEN) || (flags & LOOKUP_DIRECTORY))
-- 
2.43.0