[PATCH RFC 08/12] nfs: keep the dentry of a shared-parent removal hashed

[Jonathan Flynn <jonathan.flynn@hammerspace.com>]

With the directory locked shared, a removal stays ordered against
lookups and opens of the same name only through the name's dentry,
whose ->d_revalidate() waits while the dentry is marked.  Three paths
act on a cached dentry without that wait: nfs_prime_dcache(), which
unhashes a dentry that a directory listing disagrees with and
otherwise refreshes its inode from the listing; nfs4_file_open() when
the open fails; and recovery of open state.  Make each of them leave
a dentry marked by such a removal alone, so that it stays hashed until
the removal itself deletes it, and so that a listing that may predate
the removal does not stop the removal from dropping the link count.

Have nfs_prime_dcache() also leave alone a dentry that has been
unhashed.  When the listing holds a reference, a successful removal
unhashes the dentry, rather than making it negative, before it clears
the mark, and a refresh from a listing that predates the removal could
then put back the link count that the removal took away.  Without that
reference the removal makes the dentry negative instead, and the
listing treats the name as one it has no dentry for: the case that
"nfs: do not cache a name from a listing that a removal overlapped"
handles.

Of the three, only the listing runs beside such a removal by design,
since it holds the directory shared too.  The other two hold no
directory lock, and can meet a marked dentry only through a reference
taken after it was marked, by a path that does not wait for the mark:
an open by file handle, which NFSv4.1 and later mounts support.  A
lockless path walk that checked the dentry before it was marked fails
its recheck and retries, because marking it also invalidates the
dentry's sequence count.

Nothing marks a dentry this way yet, so the one change is that
nfs_prime_dcache() leaves alone a dentry that something else has
unhashed, rather than refreshing its inode or caching a replacement
for it.

Signed-off-by: Jonathan Flynn <jonathan.flynn@hammerspace.com>
---
 fs/nfs/dir.c      | 42 +++++++++++++++++++++++++++++++++++++++
 fs/nfs/internal.h | 50 +++++++++++++++++++++++++++++++++++++++++++++++
 fs/nfs/nfs4file.c |  9 ++++++++-
 fs/nfs/nfs4proc.c |  3 ++-
 4 files changed, 102 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 68a1e578e1..2a4c059064 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -756,6 +756,48 @@ void nfs_prime_dcache(struct dentry *parent, struct nfs_entry *entry,
 		return;
 
 	if (!d_in_lookup(dentry)) {
+		bool hands_off;
+
+		/*
+		 * Leave alone a dentry that a removal made under a shared
+		 * parent anchors on, or that has been unhashed.  readdir holds
+		 * the parent shared as well, so it can run beside such a
+		 * removal, and while the REMOVE is in flight neither of the
+		 * two things done below is safe: unhashing the dentry would
+		 * let a lookup or open of the name overtake the REMOVE, and
+		 * refreshing its inode from this reply would advance the
+		 * attribute generation that the remover's nfs_drop_nlink()
+		 * compares against, so that the removed file kept its link
+		 * count.  Nor is a refresh safe once the REMOVE has succeeded,
+		 * since this reply can still list the name: on a server that
+		 * reports how its change attribute behaves, nfs_refresh_inode()
+		 * takes attributes with a newer change attribute whatever
+		 * their generation, and would put back the link count that
+		 * the removal took away.
+		 *
+		 * Both are tested once, before anything else is done with the
+		 * dentry, in a ->d_lock section of their own.  This caller has
+		 * held a counted reference since before the test, and the
+		 * remover stores the sentinel only on a dentry whose count it
+		 * reads as one in the section it stores in: either the remover
+		 * sees this reference and hands the removal back, or the
+		 * sentinel was stored first.  If the REMOVE then succeeds, the
+		 * VFS calls d_delete() before ->unlink_shared_done() clears the
+		 * sentinel.  A d_delete() that finds this reference only
+		 * unhashes the dentry, under ->d_lock, so a test that finds the
+		 * sentinel gone comes after that section and sees the unhash.
+		 * One that ran before this reference was taken left the dentry
+		 * negative instead, and the code below drops a negative dentry
+		 * and treats the name as one it has no dentry for.  -ENOENT
+		 * ends either way too, through nfs_dentry_handle_enoent(), and
+		 * any other failure leaves the file in place.
+		 */
+		spin_lock(&dentry->d_lock);
+		hands_off = nfs_dentry_is_shared_anchor(dentry) ||
+			    d_unhashed(dentry);
+		spin_unlock(&dentry->d_lock);
+		if (hands_off)
+			goto out;
 		/* Is there a mountpoint here? If so, just exit */
 		if (!nfs_fsid_equal(&NFS_SB(dentry->d_sb)->fsid,
 					&entry->fattr->fsid))
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 7cda8b7b3f..907ea27ea2 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -102,6 +102,56 @@ static inline bool nfs_dentry_is_blocked(struct dentry *dentry)
 	return nfs_fsdata_blocked(READ_ONCE(dentry->d_fsdata));
 }
 
+/*
+ * Does a removal made under a shared parent anchor on this dentry?  The
+ * caller holds ->d_lock; READ_ONCE() as in nfs_dentry_is_blocked().
+ */
+static inline bool nfs_dentry_is_shared_anchor(struct dentry *dentry)
+{
+	lockdep_assert_held(&dentry->d_lock);
+	return nfs_fsdata_blocked_shared(READ_ONCE(dentry->d_fsdata));
+}
+
+/**
+ * nfs_d_drop_unless_anchored - unhash a dentry unless a removal anchors on it
+ * @dentry: dentry to unhash
+ *
+ * As d_drop(), except that a dentry carrying NFS_FSDATA_BLOCKED_SHARED is
+ * left hashed: a removal of its name is in flight under a shared parent,
+ * and unhashing the dentry would let a lookup or open of the same name
+ * overtake the REMOVE.  The test and the unhash are one ->d_lock section,
+ * and ->d_lock is the lock the sentinel is stored under.  The sentinel is
+ * all that is consulted - not the parent's lock and not its operations
+ * table - and a further condition, if one is ever added here, has to come
+ * after that test, not in front of it.
+ *
+ * The callers hold a reference to the dentry and no lock on the parent.
+ * The remover anchors only on a dentry that nobody else holds, so they can
+ * meet an anchor only if that reference was taken after the sentinel was
+ * stored, by a path that did not then consult ->d_revalidate(), which would
+ * have waited.  An RCU-walk that consulted ->d_revalidate() before the
+ * sentinel was stored is not such a path: the store is followed, in the
+ * same ->d_lock section, by write_seqcount_invalidate(), so the walk's
+ * ->d_seq recheck in legitimize_path() fails and it retries in ref-walk,
+ * where ->d_revalidate() waits.  An open by file handle is such a path:
+ * its reference comes from d_obtain_alias().
+ *
+ * The sentinel is cleared without ->d_lock, so the test can still see it
+ * for an instant after the REMOVE has completed and leave hashed a dentry
+ * that would otherwise have been dropped.  That direction is safe: the
+ * remover's own d_delete() normally unhashes the dentry, and an open by
+ * path that returns -EOPENSTALE is retried by the VFS, first in ref-walk
+ * if it was begun in RCU-walk and, if it fails again, with LOOKUP_REVAL,
+ * whose ->d_revalidate() asks the server.
+ */
+static inline void nfs_d_drop_unless_anchored(struct dentry *dentry)
+{
+	spin_lock(&dentry->d_lock);
+	if (!nfs_dentry_is_shared_anchor(dentry))
+		__d_drop(dentry);
+	spin_unlock(&dentry->d_lock);
+}
+
 static inline fmode_t flags_to_mode(int flags)
 {
 	fmode_t res = (__force fmode_t)flags & FMODE_EXEC;
diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index 9a434f5dda..ccb801ab38 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -101,7 +101,14 @@ nfs4_file_open(struct inode *inode, struct file *filp)
 	return err;
 
 out_drop:
-	d_drop(dentry);
+	/*
+	 * Unhash, unless a removal made under a shared parent anchors on
+	 * the dentry.  For an open by path, -EOPENSTALE makes the VFS redo
+	 * the lookup either way, and that lookup waits for the removal in
+	 * ->d_revalidate(); an open by file handle is not looked up again,
+	 * as it never was.
+	 */
+	nfs_d_drop_unless_anchored(dentry);
 	err = -EOPENSTALE;
 	goto out_put_ctx;
 }
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 808bb8f1c1..fbf9e97815 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2791,8 +2791,9 @@ static int _nfs4_open_expired(struct nfs_open_context *ctx, struct nfs4_state *s
 	 */
 	opendata->o_arg.open_flags = O_DIRECT;
 	ret = nfs4_open_recover(opendata, state);
+	/* Unhash, unless a removal under a shared parent anchors here */
 	if (ret == -ESTALE)
-		d_drop(ctx->dentry);
+		nfs_d_drop_unless_anchored(ctx->dentry);
 	nfs4_opendata_put(opendata);
 	return ret;
 }
-- 
2.43.0