[PATCH RFC 07/12] nfs: add a second mark for a removal made with the parent locked shared

[Jonathan Flynn]

Prepare the client to remove a file while its directory is locked
shared.  While a REMOVE of a cached name, or a RENAME onto it, is in
flight, the client stores NFS_FSDATA_BLOCKED in the dentry's
->d_fsdata, and ->d_revalidate() waits for it to clear.  Add a second
mark, NFS_FSDATA_BLOCKED_SHARED, for a removal made with the directory
locked shared.  ->d_revalidate() waits on either, and nfs_call_unlink()
does not hand a pending silly-delete to a dentry carrying either.  The
WARN_ON()s in nfs_unlink() and nfs_rename() still test for
NFS_FSDATA_BLOCKED alone: they run with the directory locked exclusive,
where the new mark cannot be present.  Document every value ->d_fsdata
can hold.

Nothing stores the new mark yet, so nothing changes.  The lookup check
is shared with NFSv2 and NFSv3, whose behavior is unchanged.

Signed-off-by: Jonathan Flynn <jonathan.flynn@xxxxxxxxxxxxxxx>
---
 fs/nfs/dir.c           | 11 +++++---
 fs/nfs/internal.h      | 58 ++++++++++++++++++++++++++++++++++++++++++
 fs/nfs/unlink.c        | 16 ++++++------
 include/linux/nfs_fs.h | 21 +++++++++++++++
 4 files changed, 94 insertions(+), 12 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index dc7784b666..68a1e578e1 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1839,17 +1839,20 @@ static int
 __nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 {
 	if (flags & LOOKUP_RCU) {
-		if (dentry->d_fsdata == NFS_FSDATA_BLOCKED)
+		/* One load: no lock is held and there are two values */
+		if (nfs_fsdata_blocked(READ_ONCE(dentry->d_fsdata)))
 			return -ECHILD;
 	} else {
+		void **fsdata = &dentry->d_fsdata;
+
 		/* 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_var_event(&dentry->d_fsdata,
-			       smp_load_acquire(&dentry->d_fsdata)
-			       != NFS_FSDATA_BLOCKED);
+		wait_var_event(fsdata,
+			       /* one load: there are two values to test */
+			       !nfs_fsdata_blocked(smp_load_acquire(fsdata)));
 	}
 	return 0;
 }
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 34b8aff4e0..7cda8b7b3f 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -44,6 +44,64 @@ static inline bool nfs_lookup_is_soft_revalidate(const struct dentry *dentry)
 	return true;
 }
 
+/*
+ * ->d_fsdata holds NFS_FSDATA_BLOCKED or NFS_FSDATA_BLOCKED_SHARED while a
+ * removal of the dentry's name is in flight: the first when the remover
+ * holds the parent's i_rwsem exclusive, the second when it holds it shared,
+ * and the second until the VFS ends the removal, after any delete event for
+ * it (see the client's ->unlink_shared_done()).
+ * ->d_revalidate() waits for either to clear.  The two differ in what the
+ * dentry is to the removal.
+ *
+ * Under an exclusive parent no resolver of the name can make a new dentry
+ * for it, so a victim that gets unhashed is harmless.  (That holds while
+ * the VFS calls d_alloc_parallel() under the parent's i_rwsem, as it does
+ * today; a VFS that allocates in-lookup dentries before taking i_rwsem
+ * would end it.  nfs_call_unlink() calls it without i_rwsem, for a
+ * silly-renamed name, but the dentry it makes there is never hashed, and
+ * goes once its REMOVE is done.)  Under a shared parent the hashed dentry
+ * is what every resolver of the name is handed - d_alloc_parallel() looks
+ * in the hash before it creates in-lookup state - and so it is what routes
+ * a lookup or open of the name through ->d_revalidate(), where a resolver
+ * that consults it after the sentinel is stored waits.  Unhash it, and a
+ * lookup or open of the name allocates a fresh dentry, finds no removal in
+ * flight and can reach the server ahead of the REMOVE: an open then gets
+ * the file being removed, which the REMOVE goes on to delete while this
+ * client holds it open, without the silly-rename that would otherwise have
+ * kept it.  Such a dentry is the removal's anchor.  A path in the client
+ * that unhashes a dentry it does not own, or that reaches one without
+ * ->d_revalidate(), has to leave an anchor hashed or wait for its sentinel
+ * to clear.
+ *
+ * The shared sentinel is stored and cleared within one shared hold of the
+ * parent, so it is never present while the parent is held exclusive.  That
+ * is why the two tests made with the parent exclusive, the WARN_ON()s in
+ * nfs_unlink() and nfs_rename(), still compare against NFS_FSDATA_BLOCKED
+ * alone.  They are the only bare compares against a sentinel; every other
+ * test uses the helpers below.
+ */
+static inline bool nfs_fsdata_blocked(const void *fsdata)
+{
+	return fsdata == NFS_FSDATA_BLOCKED ||
+	       fsdata == NFS_FSDATA_BLOCKED_SHARED;
+}
+
+static inline bool nfs_fsdata_blocked_shared(const void *fsdata)
+{
+	return fsdata == NFS_FSDATA_BLOCKED_SHARED;
+}
+
+/*
+ * Is a removal of this name in flight?  The caller holds ->d_lock, which
+ * the sentinels are stored under but not cleared under:
+ * unblock_revalidate() clears them without it, hence READ_ONCE().
+ */
+static inline bool nfs_dentry_is_blocked(struct dentry *dentry)
+{
+	lockdep_assert_held(&dentry->d_lock);
+	return nfs_fsdata_blocked(READ_ONCE(dentry->d_fsdata));
+}
+
 static inline fmode_t flags_to_mode(int flags)
 {
 	fmode_t res = (__force fmode_t)flags & FMODE_EXEC;
diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
index 679ef22637..a2845198d6 100644
--- a/fs/nfs/unlink.c
+++ b/fs/nfs/unlink.c
@@ -152,13 +152,13 @@ static int nfs_call_unlink(struct dentry *dentry, struct inode *inode, struct nf
 		 */
 		spin_lock(&alias->d_lock);
 		/*
-		 * Not if a removal of this name already owns the alias:
-		 * ->d_fsdata == NFS_FSDATA_BLOCKED means nfs_unlink() has a
-		 * REMOVE of exactly this name, or nfs_rename() a RENAME onto
-		 * it, in flight and ->d_revalidate() is waiting on that field,
-		 * so storing over it would lift the block early and the later
-		 * unblock_revalidate() would store NULL over @data.  Tested
-		 * under the same ->d_lock block_revalidate() stores under.
+		 * Not if a removal of this name already owns the alias: a
+		 * sentinel in ->d_fsdata means that a REMOVE of exactly this
+		 * name, or a RENAME onto it, is in flight, whichever way its
+		 * parent is locked, and ->d_revalidate() is waiting on that
+		 * field, so storing over it would lift the block early and
+		 * the later unblock_revalidate() would store NULL over @data.
+		 * Tested under the same ->d_lock the sentinel is stored under.
 		 * Skip rather than wait: that removal makes this sillydelete
 		 * redundant if it succeeds, and a wait here would sleep in a
 		 * dput() path -- the nfsiod callback that releases a
@@ -170,7 +170,7 @@ static int nfs_call_unlink(struct dentry *dentry, struct inode *inode, struct nf
 		if (d_really_is_positive(alias) &&
 		    !nfs_compare_fh(NFS_FH(inode), NFS_FH(d_inode(alias))) &&
 		    !(alias->d_flags & DCACHE_NFSFS_RENAMED) &&
-		    alias->d_fsdata != NFS_FSDATA_BLOCKED) {
+		    !nfs_dentry_is_blocked(alias)) {
 			devname_garbage = alias->d_fsdata;
 			alias->d_fsdata = data;
 			alias->d_flags |= DCACHE_NFSFS_RENAMED;
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index d2c716322c..b45952727c 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -678,8 +678,29 @@ static inline bool nfs_ooo_test(struct nfs_inode *nfsi)
  * but deleted on the server resulting in ESTALE.
  * So use ->d_fsdata to record when the unlink is happening
  * and block dentry revalidation while it is set.
+ *
+ * ->d_fsdata of an NFS dentry holds one of:
+ *  - NULL;
+ *  - a mount's device name, a string kept on the mount's root dentry.
+ *    Whatever stores over it frees it, and ->d_release() frees it
+ *    otherwise;
+ *  - a pending silly-delete, a struct nfs_unlinkdata, exactly while
+ *    DCACHE_NFSFS_RENAMED is set, which is what tells it apart;
+ *  - NFS_FSDATA_BLOCKED, while a REMOVE of the name, or a RENAME onto it,
+ *    is in flight with the parent's i_rwsem held exclusive.  It outlives
+ *    that hold only when a rename is killed, until its RENAME completes;
+ *  - NFS_FSDATA_BLOCKED_SHARED, from the store in the client's
+ *    ->unlink_shared(), with the parent's i_rwsem held shared, until its
+ *    ->unlink_shared_done(), which the VFS calls after any delete event
+ *    for the removal, whether or not it succeeded, and before it drops
+ *    that hold.  The dentry may be negative, or positive and unhashed,
+ *    by then.
+ * Both sentinels are stored under ->d_lock and cleared by
+ * unblock_revalidate(), and ->d_revalidate() waits while either is set.
+ * fs/nfs/internal.h says what else has to respect them.
  */
 #define NFS_FSDATA_BLOCKED ((void*)1)
+#define NFS_FSDATA_BLOCKED_SHARED ((void *)2)
 
 # undef ifdebug
 # ifdef NFS_DEBUG
-- 
2.43.0