[PATCH RFC 00/12] vfs, nfs: concurrent create and unlink in one directory

[PATCH RFC 00/12] vfs, nfs: concurrent create and unlink in one directory

[Jonathan Flynn]

Hi Neil,

Thank you for this series; it is what makes these patches possible.  I
had them on your v3 and have rebased them onto v4.  They extend it so
that processes on one NFSv4 client can create and remove files in one
directory concurrently.

  operations per second   parent exclusive   parent shared   ratio
  creates                            2,616          29,939   11.4x
  removals                           2,157          32,360   15.0x

One client, 96 processes in one directory, each creating files (open
O_CREAT, a small write, close) for a set time, then removing them all;
NFSv4.2 to a pNFS metadata server that is not knfsd; your v4, the 7.4
VFS changes it builds on, the three silly-delete fixes and this series
on a 7.3-based kernel, among other patches.  Each row switched only its
own path, by a test-only module parameter not in this series, with the
other path on; one run each.  The creates row comes from 300 s of
creates.  The removals row comes from 120 s of creates, and each of its
runs removed about 3.7 million files; the removal rates vary with that
number, so it is given.  With the parent held exclusive, operations go
one server round trip at a time.  2/12 quotes an earlier measurement,
on a 7.1.13-based kernel.

How it works.  2/12 lets a filesystem have the parent locked shared for
O_CREAT (a flag in a new ->iop_flags field), and 6/12 for unlink(2)
(->unlink_shared() and ->unlink_shared_done()).  The dentry then
excludes the name: for a create, its in-lookup state, all
nfs_atomic_open() needs since your 3/9; for a removal, an NFS mark that
lookups, opens and creates of the name wait on until
->unlink_shared_done(), which the VFS calls after the fsnotify delete
event, if any.  What NFS cannot order that way, such as an open file,
it hands back to ->unlink() under the exclusive lock; where the server
reports itself case-insensitive or not case-preserving, NFS sets
SB_I_NAMES_COLLIDE (3/12) and both keep the exclusive lock.  Both paths
call d_alloc_parallel() under the shared lock, as lookup does.  When
your directory-locking changes lift d_alloc_parallel() above i_rwsem,
these calls have to move with lookup's; the exclusion does not change,
and the opt-ins go once the VFS stops taking the parent lock (2/12,
6/12).

Patch flow:
  prereq   1  store re-read server capabilities in one step, for 3
  create   2  VFS opt-in to take the parent lock shared for O_CREAT
           3  NFS sets SB_I_NAMES_COLLIDE where names can collide
           4  NFSv4 directories opt in for O_CREAT
  remove   5  factor vfs_unlink() into a helper, no functional change
           6  VFS opt-in: ->unlink_shared(), ->unlink_shared_done()
           7  a second ->d_fsdata mark, for removals under a shared lock
           8  keep a dentry carrying that mark hashed
           9  no new dentries from a listing that a removal overlapped
          10  the removal method, handing back what it cannot order
          11  look a name up again once its removal has unhashed it
          12  NFSv4 directories opt in for unlink

Three fixes for the silly-delete hand-off, which your 2/9 makes
reachable, are posted separately, in reply to 2/9.

Tested on the same kernel over NFSv4.2, with the new paths on and off:
removal races (with listings, and with tasks killed); O_EXCL creates
racing unlink, with a kernel-side check that no revalidation trusted a
removed entry; inotify event order; two clients; and an overlayfs mount
whose lower file another client renamed away and back.  All passed,
with no kernel warning or hung task, except the two-client test's check
that nothing was left behind: on this kernel and on one with an earlier
revision of this series, it found one silly-renamed file in three of
four runs, with the new paths off as well as on, so they are not the
cause.  Each time, the file left behind was not the one that had been
silly-renamed: the silly-rename's RENAME, which goes by name, had moved
another file of that name onto the hidden name.  A PROVE_LOCKING and
KASAN build ran the removal, killed-task and O_EXCL tests with no
report.  A case-insensitive server, where two spellings of a name never
made two files, was tested on that earlier revision only; that code has
not changed since.  Not run: fstests, knfsd as the server, NFSv4.0 or
4.1, directory delegations (the server grants none), CONFIG_DEBUG_VFS, a
weakly ordered CPU.

One question: are the opt-ins in 2/12 and 6/12 acceptable as interim
interfaces until the directory-locking changes land?

The series applies to 83f32c773255 in Linus's tree on top of your v4
and the three silly-delete fixes.  fs/ compiles there without warnings
at every commit, with and without CONFIG_DEBUG_VFS and lockdep; I have
run it only on the 7.3-based kernel above.

Tools: these patches, their changelogs, the race tests and this letter
were written with Claude Code, using Anthropic's Claude Opus 5 and
Claude Opus 5.5 models, at my direction over many sessions: I chose
among the designs it proposed and directed each revision.  It found, by
reading the code, the bug that 1/12 fixes: while the client reads a
server's capabilities again, a capability the server still offers can
look missing.  The three silly-delete fixes, posted separately, say
which of their problems it found and how they were tested.

Thanks,
Jonathan

Jonathan Flynn (12):
  nfs: update a server's capabilities in one step
  vfs: allow the parent lock to be shared for O_CREAT
  nfs: tell the VFS when two names can be one entry on the server
  nfs: concurrent O_CREAT in one NFSv4 directory
  vfs: factor file removal into a helper, no functional change
  vfs: let a filesystem remove files with the parent directory locked
    shared
  nfs: add a second mark for a removal made with the parent locked
    shared
  nfs: keep the dentry of a shared-parent removal hashed
  nfs: do not cache a name from a listing that a removal overlapped
  nfs: add the removal method for a parent directory locked shared
  nfs: look a name up again once its removal has unhashed it
  nfs: remove files with the parent directory locked shared on NFSv4

 .../filesystems/directory-locking.rst         |  60 +++
 Documentation/filesystems/locking.rst         | 259 ++++++++++++-
 Documentation/filesystems/porting.rst         |  35 ++
 Documentation/filesystems/vfs.rst             |  25 ++
 fs/namei.c                                    | 360 +++++++++++++++++-
 fs/nfs/client.c                               |   1 +
 fs/nfs/dir.c                                  | 300 ++++++++++++++-
 fs/nfs/inode.c                                |   2 +
 fs/nfs/internal.h                             | 211 ++++++++++
 fs/nfs/nfs4file.c                             |   9 +-
 fs/nfs/nfs4proc.c                             |  35 +-
 fs/nfs/nfstrace.h                             |   2 +
 fs/nfs/super.c                                |   2 +
 fs/nfs/unlink.c                               |  16 +-
 include/linux/fs.h                            |  72 ++++
 include/linux/fs/super_types.h                |   1 +
 include/linux/nfs_fs.h                        |  28 ++
 17 files changed, 1361 insertions(+), 57 deletions(-)


base-commit: 83f32c773255720a661aa45d83e36278135f23f5
prerequisite-patch-id: 3405b4d6ab8b0bf24ba63a898083d617deec4f24
prerequisite-patch-id: 096657d6d80d7ca533ccb574d2af65611ede4af2
prerequisite-patch-id: 1a2319c14739918aef86116647fea28ae85ad4df
prerequisite-patch-id: 725cb18c3936d50141ef27dafc159bf50b64a5bd
prerequisite-patch-id: b39cdd352b97e2cd247311fd5db620dbff227bd7
prerequisite-patch-id: f9f3f0e945cadf96113d4ac9ab8f7b0c207af4e5
prerequisite-patch-id: 4c09a15390bb556d79bea1ee733f52a58bec80ef
prerequisite-patch-id: 7c026717a97b2c86a169539822f5d14650f68f8a
prerequisite-patch-id: a702d54495d0f3d7f88b82411d884dcf5aacf9a0
prerequisite-patch-id: a7e7263e8916612f3e4b767946b5d9259029f50e
prerequisite-patch-id: a8e5414df9d9912dcdf9bb1dbb2811ce326a1e1f
prerequisite-patch-id: e8050b998d5790f3bb4311bc65011735ed7d8b2f
-- 
2.43.0

[PATCH RFC 01/12] nfs: update a server's capabilities in one step

[Jonathan Flynn]

When the client reads a server's capabilities again, on remount, on
migration, or when a later mount reuses a filesystem that is already
mounted, it clears a group of them and then sets again, one at a time,
each one the server still offers.  Anything that tests one of them on
another CPU in between sees it missing: a case-insensitive server looks
case-sensitive, a server that supports security labels looks as if it
does not.

Build the new set first and store it once, as the NFSv3 path already
does.

This was found by an AI coding assistant (Claude Code) reviewing how
the client derives these capabilities, and has not been reproduced.

Signed-off-by: Jonathan Flynn <jonathan.flynn@hammerspace.com>
---
 fs/nfs/nfs4proc.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 22a2a4009d..113c199a6f 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3896,6 +3896,7 @@ static bool nfs4_server_delegtime_capable(struct nfs4_server_caps_res *res)
 
 static int _nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh *fhandle)
 {
+	unsigned int caps;
 	u32 minorversion = server->nfs_client->cl_minorversion;
 	u32 bitmask[3] = {
 		[0] = FATTR4_WORD0_SUPPORTED_ATTRS,
@@ -3953,7 +3954,13 @@ static int _nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh *f
 				     res.attr_bitmask[2];
 		}
 		memcpy(server->attr_bitmask, res.attr_bitmask, sizeof(server->attr_bitmask));
-		server->caps &=
+		/*
+		 * Build the new set in a local and store it once: a reader of
+		 * server->caps during the probe, such as a remount or migration
+		 * of a mounted filesystem, must not see these capabilities
+		 * cleared and then set again one by one.
+		 */
+		caps = server->caps &
 			~(NFS_CAP_ACLS | NFS_CAP_HARDLINKS | NFS_CAP_SYMLINKS |
 			  NFS_CAP_SECURITY_LABEL | NFS_CAP_FS_LOCATIONS |
 			  NFS_CAP_OPEN_XOR | NFS_CAP_DELEGTIME |
@@ -3961,22 +3968,22 @@ static int _nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh *f
 		server->fattr_valid = NFS_ATTR_FATTR_V4;
 		if (res.attr_bitmask[0] & FATTR4_WORD0_ACL &&
 				res.acl_bitmask & ACL4_SUPPORT_ALLOW_ACL)
-			server->caps |= NFS_CAP_ACLS;
+			caps |= NFS_CAP_ACLS;
 		if (res.has_links != 0)
-			server->caps |= NFS_CAP_HARDLINKS;
+			caps |= NFS_CAP_HARDLINKS;
 		if (res.has_symlinks != 0)
-			server->caps |= NFS_CAP_SYMLINKS;
+			caps |= NFS_CAP_SYMLINKS;
 		if (res.case_insensitive)
-			server->caps |= NFS_CAP_CASE_INSENSITIVE;
+			caps |= NFS_CAP_CASE_INSENSITIVE;
 		if ((res.attr_bitmask[0] & FATTR4_WORD0_CASE_PRESERVING) &&
 		    !res.case_preserving)
-			server->caps |= NFS_CAP_CASE_NONPRESERVING;
+			caps |= NFS_CAP_CASE_NONPRESERVING;
 #ifdef CONFIG_NFS_V4_SECURITY_LABEL
 		if (res.attr_bitmask[2] & FATTR4_WORD2_SECURITY_LABEL)
-			server->caps |= NFS_CAP_SECURITY_LABEL;
+			caps |= NFS_CAP_SECURITY_LABEL;
 #endif
 		if (res.attr_bitmask[0] & FATTR4_WORD0_FS_LOCATIONS)
-			server->caps |= NFS_CAP_FS_LOCATIONS;
+			caps |= NFS_CAP_FS_LOCATIONS;
 		if (!(res.attr_bitmask[0] & FATTR4_WORD0_FILEID))
 			server->fattr_valid &= ~NFS_ATTR_FATTR_FILEID;
 		if (!(res.attr_bitmask[1] & FATTR4_WORD1_MODE))
@@ -4009,9 +4016,10 @@ static int _nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh *f
 
 		if (res.open_caps.oa_share_access_want[0] &
 		    NFS4_SHARE_WANT_OPEN_XOR_DELEGATION)
-			server->caps |= NFS_CAP_OPEN_XOR;
+			caps |= NFS_CAP_OPEN_XOR;
 		if (nfs4_server_delegtime_capable(&res))
-			server->caps |= NFS_CAP_DELEGTIME;
+			caps |= NFS_CAP_DELEGTIME;
+		WRITE_ONCE(server->caps, caps);
 
 		memcpy(server->cache_consistency_bitmask, res.attr_bitmask, sizeof(server->cache_consistency_bitmask));
 		server->cache_consistency_bitmask[0] &= FATTR4_WORD0_CHANGE|FATTR4_WORD0_SIZE;
-- 
2.43.0

[PATCH RFC 02/12] vfs: allow the parent lock to be shared for O_CREAT

[Jonathan Flynn]

The VFS chooses the mode of the parent directory's lock for an open from
the open flags alone: O_CREAT takes it exclusive, everything else
shared.  That choice is made before any filesystem method is reachable,
so a filesystem has nowhere to say that it does not need the exclusive
lock for a create.  For a network filesystem the consequence is direct:
a create is a server round trip, and the exclusive parent lock
serializes those round trips, so creates in one directory proceed one
round trip at a time however many threads are creating.

Add an ->iop_flags field to struct inode_operations, mirroring
->fop_flags, with one flag, IOPF_ATOMIC_OPEN_CREATE_SHARED.  It is
transitional: it chooses how the parent is locked for a create, and goes
away once the VFS no longer takes the parent lock for one.  When a
directory's operations set it and provide ->atomic_open(), lookup_open()
takes the parent shared for an O_CREAT open as well, and then passes
->atomic_open() only an in-lookup dentry; dentry_create(), which nfsd
uses, is still called with the parent held exclusive.  A filesystem
that does not set the flag sees no change: the field is zero.  The
field is the last member of a cacheline-aligned structure, so it
occupies padding that was already there and no existing member moves.

With the parent held shared, exclusion for the name being created
comes from the in-lookup protocol that lookup already relies on under a
shared lock: the first thread to resolve a name publishes an in-lookup
dentry that a second finds and waits on, and the name enters the main
hash once.  Every other directory-modifying operation -- create without
an open, link, unlink, symlink, mkdir, rmdir, mknod and rename -- still
takes the parent exclusive, so a shared create runs concurrently only
with other creates and with what already holds the parent shared:
lookup, readdir and opens without O_CREAT.  Unlink joins them only
where a filesystem also provides ->unlink_shared(), added later in
this series.

One case keeps the exclusive lock even where the flag is set: a
superblock on which two names the dcache tells apart can be one
directory entry, as on a case-insensitive server whose client does not
fold case.  The in-lookup protocol would exclude a second create of
"foo" but not a create of "FOO", so such a filesystem sets
SB_I_NAMES_COLLIDE in ->s_iflags, and may set or clear it while
mounted; creates in its directories then take the parent exclusive.

The VFS has one gap of its own to close.  A cached negative dentry that
revalidates reaches the atomic-open method already hashed, outside the
in-lookup protocol: safe under the exclusive lock, but under a shared
one two threads could each hash a dentry for the name, leaving two
hashed dentries for one name with nothing to detect it.  So, for a
create with the parent held shared, reject such a dentry and go round
the existing loop, so that ->atomic_open() is then only ever handed an
in-lookup dentry.  On the filesystem that opts in later in this series
this happens on every such create over a cached negative dentry, not
only in a race, at the cost of a dentry allocation and an unhash but no
extra round trip.  The rest of what the exclusive lock provided, for
the filesystem's own per-directory state and for a plain open of the
same name, becomes the filesystem's responsibility, as
Documentation/filesystems/locking.rst now states.

Since NeilBrown's "VFS: move mnt_want_write() and locking into
lookup_open()", lookup_open() takes and drops the parent lock itself,
so the flag is read there, once per pass, and decides both the lock
mode and the rejection, with nothing threaded through a signature.
With CONFIG_DEBUG_VFS, lookup_open() also warns if a create made with
the parent held shared is about to hand ->atomic_open() a dentry that
is not in-lookup; lockdep cannot see a missing exclusion, so nothing
else would catch one.

This relies on in-lookup dentries being allocated with the parent's
->i_rwsem held, as d_alloc_parallel() is here.  NeilBrown's
"multiple-filesystems: prepare for changes to VFS locking" plans to
invert that order; if it does, the lookup of the name being created
moves ahead of the shared parent lock, and the exclusion, which is the
dentry's in-lookup state and not the parent lock, is unchanged.

Measured with "nfs: concurrent O_CREAT in one NFSv4 directory" on a
7.1.13-based kernel carrying an earlier revision of both patches, using
a test-only switch, not part of this series, to choose the lock mode at
run time: with 60 processes on one client creating files (open with
O_CREAT, a small write, close) in one directory, creates went from
about 2,700 a second with the parent held exclusive to about 29,700
with it held shared.  The shared rate stayed about flat from one to
four clients, because one client already reached the server's limit.

Signed-off-by: Jonathan Flynn <jonathan.flynn@hammerspace.com>
---

Notes:
    For reviewers and review tools; not part of the changelog.
    
    An automated review of this patch (Claude Code running Sashiko's
    review prompts) reported that locking.rst misdescribes a d_rehash()
    of the in-lookup dentry, because a concurrent d_alloc_parallel()
    would find that dentry on the in-lookup chain and wait on it.  It
    was checked and dropped: atomic_open() calls d_lookup_done() as soon
    as ->atomic_open() returns, and that takes the dentry off the
    in-lookup chain without the ->i_dir_seq window.  A
    d_alloc_parallel() that sampled ->i_dir_seq and missed the hash
    before the d_rehash(), and scans the chain after the
    d_lookup_done(), neither retries nor waits, and hashes a second
    dentry for the name: the outcome the text describes.

 .../filesystems/directory-locking.rst         | 19 ++++
 Documentation/filesystems/locking.rst         | 81 ++++++++++++++++-
 fs/namei.c                                    | 86 +++++++++++++++++--
 include/linux/fs.h                            | 35 ++++++++
 include/linux/fs/super_types.h                |  1 +
 5 files changed, 214 insertions(+), 8 deletions(-)

diff --git a/Documentation/filesystems/directory-locking.rst b/Documentation/filesystems/directory-locking.rst
index 6fdf0b02df..316f10cb3d 100644
--- a/Documentation/filesystems/directory-locking.rst
+++ b/Documentation/filesystems/directory-locking.rst
@@ -25,6 +25,25 @@ For our purposes all operations fall in 6 classes:
 
 	* lock the directory we are accessing (exclusive)
 
+	An O_CREAT open is the one exception: if the filesystem has set
+	IOPF_ATOMIC_OPEN_CREATE_SHARED in ->iop_flags and its superblock has
+	not set SB_I_NAMES_COLLIDE, its ->atomic_open() for such an open is
+	called with the directory locked shared, and exclusion for the name
+	being created comes from the in-lookup state of the dentry instead -
+	see Documentation/filesystems/locking.rst.
+	That takes an existing lock in shared mode; it adds no lock and no
+	new ordering, so the ordering analysis below is unaffected.  That
+	holds while d_alloc_parallel() is called with the parent's ->i_rwsem
+	held, as it is here; once in-lookup dentries are taken before
+	->i_rwsem, the create's lookup has to move ahead of the shared parent
+	lock with them.
+
+	Such a create can sleep waiting for an in-lookup dentry while
+	holding the parent shared.  That is not a new wait: a shared-mode
+	lookup already does it, and no thread that has published an
+	in-lookup dentry needs the parent's ->i_rwsem to finish with it, so
+	the waiter cannot be blocking the thread that must wake it.
+
 3. object removal.  Locking rules:
 
 	* lock the parent (exclusive)
diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
index f7363ad770..c04df4c746 100644
--- a/Documentation/filesystems/locking.rst
+++ b/Documentation/filesystems/locking.rst
@@ -120,7 +120,7 @@ listxattr:	no
 fiemap:		no
 update_time:	no
 sync_lazytime:	no
-atomic_open:	shared (exclusive if O_CREAT is set in open flags)
+atomic_open:	shared (exclusive if O_CREAT unless the fs opts in)
 tmpfile:	no
 fileattr_get:	no or exclusive
 fileattr_set:	exclusive
@@ -135,6 +135,85 @@ get_offset_ctx  no
 	involved.
 	->rename() has ->i_rwsem exclusive on any subdirectory that changes parent.
 
+	->atomic_open() with O_CREAT set is called with ->i_rwsem on the
+	parent held exclusive, unless the filesystem has set
+	IOPF_ATOMIC_OPEN_CREATE_SHARED in the ->iop_flags of that
+	directory's ->i_op; then lookup_open() calls it with ->i_rwsem held
+	shared, as ->atomic_open() without O_CREAT already is;
+	dentry_create(), which nfsd uses, is still called with it held
+	exclusive.  The flag is a property of the ->i_op instance, so it
+	applies to every directory using it.  It is ignored unless the
+	filesystem also provides ->atomic_open(): without that method the
+	parent is still taken exclusive, so ->create() is never called with
+	->i_rwsem held shared.
+
+	IOPF_ATOMIC_OPEN_CREATE_SHARED is transitional: it chooses how the
+	parent is locked for a create, and goes away once the VFS no longer
+	takes the parent lock for one.
+
+	Setting IOPF_ATOMIC_OPEN_CREATE_SHARED is a statement that the
+	filesystem does not rely on the exclusive parent lock to exclude
+	other creates in the same directory, or to serialize its own
+	per-directory state.  Creates in one directory may then run
+	concurrently with each other, with an ->atomic_open() for the same
+	or another name without O_CREAT, and with ->lookup() and
+	->iterate_shared().  What excludes two creates of the same name is
+	the in-lookup protocol: whenever ->atomic_open() is called with
+	O_CREAT set and ->i_rwsem held shared, the VFS guarantees that the
+	dentry passed is in-lookup (d_in_lookup() is true), so a concurrent
+	lookup or create of that name finds it in the in-lookup hash and
+	waits for it.  With ->i_rwsem held exclusive - on a superblock that
+	has set SB_I_NAMES_COLLIDE, or through dentry_create() - the dentry
+	need not be in-lookup and may be a hashed negative one, as before.
+	Nothing else changes: ->create(), ->link(), ->unlink(), ->mkdir(),
+	->rmdir(), ->mknod(), ->symlink() and ->rename() still take
+	->i_rwsem exclusive on the parent, so a create holding it shared
+	still excludes them.
+
+	The flag is also ignored, and the parent taken exclusive, on a
+	superblock that has set SB_I_NAMES_COLLIDE in ->s_iflags.  A
+	filesystem sets that bit when two names the dcache compares as
+	different - byte for byte, or through ->d_hash() and ->d_compare()
+	where it has them - can be one directory entry, as they are on a
+	case-insensitive server whose client does not fold case in the
+	dcache.  The in-lookup protocol is per name as the dcache compares
+	names, so there it would exclude a second create of "foo" and not a
+	create of "FOO".  A filesystem whose ->d_compare() already makes such
+	names equal, as a casefolded directory's does, has no reason to set
+	it.  The bit may change while the filesystem is mounted.  The VFS
+	reads it for each open before it takes the parent lock, so an open
+	that read it clear may still be running shared when it is set.
+
+	A filesystem that sets the flag must publish its result on the
+	dentry passed to ->atomic_open() with d_splice_alias() or d_add(),
+	which hash it and end the in-lookup state together, inside the
+	parent's ->i_dir_seq window, or with d_add_ci(), which leaves it
+	unhashed and publishes a dentry of the case-exact name that way.
+	It must not hash it with d_rehash().  d_rehash() takes ->d_lock and
+	adds the name to the dentry hash, nothing more: the in-lookup state
+	does not end, no ->i_dir_seq window is taken, and the waiters are
+	not woken.  A concurrent d_alloc_parallel() for the name then
+	neither finds a reason to retry nor has anything to wait for, and
+	hashes a second dentry for it.  The result is two hashed dentries
+	for one name with independent inode state, lookups getting either,
+	and nothing in the VFS detecting the condition.
+
+	The guarantee is for the O_CREAT opener only.  Under the shared lock
+	an O_CREAT open can run concurrently with a plain open of the same
+	name, and only the O_CREAT one is handed an in-lookup dentry; the
+	other may be holding a dentry it found hashed and revalidated.  That
+	pairing is not new - two plain opens of one name can already reach
+	->atomic_open() that way under the same shared lock - and the VFS
+	does not close it.  A filesystem setting this flag is responsible
+	for it, as it is today.  One way, which NFS uses, is for
+	->d_revalidate() to report a cached negative dentry invalid when
+	LOOKUP_OPEN is set without LOOKUP_CREATE: lookup_open() then drops
+	it and calls d_alloc_parallel() itself, so the plain open is also
+	handed an in-lookup dentry, or waits on the creator's.  Calling
+	d_alloc_parallel() from ->atomic_open() instead relies on in-lookup
+	dentries being allocated under ->i_rwsem, which is planned to
+	change.
+
 See Documentation/filesystems/directory-locking.rst for more detailed discussion
 of the locking scheme for directory operations.
 
diff --git a/fs/namei.c b/fs/namei.c
index 96c6594039..ce10a408d3 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4414,12 +4414,49 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry
 	return dentry;
 }
 
+/*
+ * Can this directory's ->atomic_open() be given an O_CREAT open with
+ * i_rwsem held shared?  The method must be present: without it
+ * lookup_open() goes on to break the directory lease and call ->create(),
+ * and the in-lookup protocol covers neither, so a filesystem that sets the
+ * flag and supplies no ->atomic_open keeps the exclusive lock rather than
+ * silently losing exclusion.
+ *
+ * Nor on a superblock that has set SB_I_NAMES_COLLIDE.  The in-lookup state
+ * that stands in for the exclusive lock is per name as the dcache compares
+ * names, so where the filesystem makes one entry of two names the dcache
+ * holds apart, it excludes a second create of "foo" but not a create of
+ * "FOO", while the exclusive lock excluded both.  The filesystem may learn
+ * that about a superblock only after a directory has taken its ->i_op, and
+ * may learn it again, so it is a superblock bit and not an ops-table flag.
+ * It is tested last, so only an O_CREAT open in a directory that has opted
+ * in reads it.
+ *
+ * The bit is read once per pass of lookup_open(), before the parent lock is
+ * taken.  An open that read it clear may still run shared after the
+ * filesystem has set it, until that open completes; every open that reads
+ * it set takes the parent exclusive and so excludes the shared ones.
+ */
+static inline bool atomic_open_create_shared(struct inode *dir)
+{
+	const struct inode_operations *iop = dir->i_op;
+
+	return iop->atomic_open &&
+	       (iop->iop_flags & IOPF_ATOMIC_OPEN_CREATE_SHARED) &&
+	       !(READ_ONCE(dir->i_sb->s_iflags) & SB_I_NAMES_COLLIDE);
+}
+
 /*
  * Look up and maybe create and open the last component.
  *
- * Takes the parent inode lock itself, exclusive if O_CREAT was requested and
- * shared otherwise, and drops it again before returning.  The caller must not
- * hold it.
+ * Takes the parent inode lock itself, and drops it again before returning;
+ * the caller must not hold it.  O_CREAT normally takes it exclusive, every
+ * other open shared.  A directory whose ->atomic_open() has declared
+ * IOPF_ATOMIC_OPEN_CREATE_SHARED takes it shared for O_CREAT as well,
+ * unless its superblock has set SB_I_NAMES_COLLIDE, and
+ * the dentry handed to ->atomic_open() is then guaranteed to be in-lookup:
+ * that in-lookup state, not the parent lock, is what excludes another opener
+ * of the same name.
  *
  * On success returns the dentry of the last component.  If FMODE_OPENED is set
  * on file->f_mode the file was also opened and attached to @file; otherwise
@@ -4440,6 +4477,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 	int error, create_error;
 	umode_t mode;
 	bool got_write;
+	bool create_shared;
 	unsigned int shared_flag;
 
 retry:
@@ -4447,7 +4485,22 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 	got_write = false;
 	mode = op->mode;
 	create_error = 0;
-	shared_flag = (open_flag & O_CREAT) ? 0 : LOOKUP_SHARED;
+	/*
+	 * One evaluation of atomic_open_create_shared(), which reads ->i_op
+	 * and the superblock's SB_I_NAMES_COLLIDE, decides the lock mode, the
+	 * unlock at out: and the in-lookup guarantee this function then owes
+	 * ->atomic_open(), so the three cannot disagree.  It is after retry:,
+	 * so a delegation retry re-reads it for the lock it is about to take.
+	 *
+	 * LOOKUP_SHARED states how the parent is locked, so it follows the
+	 * lock and not O_CREAT.  create_shared is the narrower question --
+	 * O_CREAT holding the parent shared -- and only that case may reject
+	 * a hashed negative dentry below.
+	 */
+	create_shared = (open_flag & O_CREAT) &&
+			atomic_open_create_shared(dir_inode);
+	shared_flag = (!(open_flag & O_CREAT) || create_shared) ?
+		      LOOKUP_SHARED : 0;
 
 	if (open_flag & (O_CREAT | O_TRUNC | O_WRONLY | O_RDWR)) {
 		got_write = !mnt_want_write(nd->path.mnt);
@@ -4478,10 +4531,23 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 			break;
 
 		error = d_revalidate(dir_inode, &nd->last, dentry, nd->flags);
-		if (likely(error > 0))
-			break;
-		if (error)
+		if (unlikely(error < 0))
 			goto out_dput;
+		/*
+		 * A valid dentry would normally end the loop here.  With the
+		 * parent held shared a negative one cannot: hashed and not
+		 * in-lookup, it leaves ->atomic_open() no exclusion for this
+		 * name, because __d_add() takes the i_dir_seq window only for
+		 * an in-lookup dentry but rehashes any dentry it finds
+		 * unhashed, so once this one has been dropped a second opener
+		 * can hash the same name independently.
+		 * Reject it and go round; d_alloc_parallel() then returns an
+		 * in-lookup dentry.  On a negative, d_invalidate() is an
+		 * unhash and nothing more.  A positive is kept: it is returned
+		 * just below and never reaches ->atomic_open().
+		 */
+		if (likely(error > 0) && (!create_shared || dentry->d_inode))
+			break;
 		d_invalidate(dentry);
 		dput(dentry);
 		dentry = NULL;
@@ -4519,6 +4585,12 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 	if (create_error)
 		open_flag &= ~O_CREAT;
 	if (dir_inode->i_op->atomic_open) {
+		/*
+		 * With the parent held shared, this dentry being in-lookup is
+		 * what excludes another opener of the same name.  The loop
+		 * above established it and nothing since can have cleared it.
+		 */
+		VFS_WARN_ON_ONCE(create_shared && !d_in_lookup(dentry));
 		if (nd->flags & LOOKUP_DIRECTORY)
 			open_flag |= O_DIRECTORY;
 		dentry = atomic_open(&nd->path, dentry, file, open_flag, mode,
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3e240579e2..f14520194b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1995,6 +1995,8 @@ enum fs_update_time {
 	FS_UPD_CMTIME,
 };
 
+typedef unsigned int __bitwise iop_flags_t;
+
 struct inode_operations {
 	struct dentry * (*lookup) (struct inode *,struct dentry *, unsigned int);
 	const char * (*get_link) (struct dentry *, struct inode *, struct delayed_call *);
@@ -2038,8 +2040,41 @@ struct inode_operations {
 			    struct dentry *dentry, struct file_kattr *fa);
 	int (*fileattr_get)(struct dentry *dentry, struct file_kattr *fa);
 	struct offset_ctx *(*get_offset_ctx)(struct inode *inode);
+	iop_flags_t iop_flags;
 } ____cacheline_aligned;
 
+/*
+ * ->atomic_open() may be called with the parent directory's i_rwsem held
+ * shared even when O_CREAT is set.  A filesystem setting this accepts that,
+ * for such an open, the only exclusion against another opener of the same
+ * name is the in-lookup state of the dentry it is given:
+ *
+ *  - that dentry is always in-lookup (DCACHE_PAR_LOOKUP), so a concurrent
+ *    d_alloc_parallel() for the name finds it and waits on it;
+ *  - it must be hashed only through d_add() or d_splice_alias(), which end
+ *    the in-lookup state under the parent's i_dir_seq (d_add_ci() leaves it
+ *    unhashed and publishes the case-exact name instead).  Hashing it
+ *    with d_rehash() instead loses the exclusion: the name becomes findable
+ *    without the in-lookup state ending, so a concurrent d_alloc_parallel()
+ *    neither waits nor retries and a second dentry for the name can be
+ *    hashed;
+ *  - d_drop() on it does not break that: it unhashes from the main hash
+ *    only, leaving the in-lookup hash entry and the waiters in place;
+ *  - no other directory-modifying operation becomes concurrent.  ->create,
+ *    ->link, ->unlink, ->symlink, ->mkdir, ->rmdir, ->mknod and ->rename
+ *    still run with i_rwsem held exclusive, so an O_CREAT open is
+ *    serialized against all of them.  It runs concurrently with other
+ *    O_CREAT opens, with an ->atomic_open() for the same or another name
+ *    without O_CREAT, and with ->lookup() and ->iterate_shared(), which
+ *    already run with i_rwsem held shared.
+ *
+ * Ignored unless the filesystem also provides ->atomic_open, and ignored on
+ * a superblock with SB_I_NAMES_COLLIDE set in ->s_iflags: there the in-lookup
+ * state of "foo" does not exclude a create of "FOO", which the filesystem
+ * may make the same entry, so the parent is taken exclusive.
+ */
+#define IOPF_ATOMIC_OPEN_CREATE_SHARED	((__force iop_flags_t)(1 << 0))
+
 /* Did the driver provide valid mmap hook configuration? */
 static inline bool can_mmap_file(struct file *file)
 {
diff --git a/include/linux/fs/super_types.h b/include/linux/fs/super_types.h
index ecd96aeb1c..91f1522a0d 100644
--- a/include/linux/fs/super_types.h
+++ b/include/linux/fs/super_types.h
@@ -352,5 +352,6 @@ struct super_block {
 #define SB_I_NOIDMAP	0x00002000	/* No idmapped mounts on this superblock */
 #define SB_I_ALLOW_HSM	0x00004000	/* Allow HSM events on this superblock */
 #define SB_I_NO_DATA_INTEGRITY	0x00008000 /* fs cannot guarantee data persistence on sync */
+#define SB_I_NAMES_COLLIDE	0x00010000 /* names the dcache tells apart can be one entry */
 
 #endif /* _LINUX_FS_SUPER_TYPES_H */
-- 
2.43.0

[PATCH RFC 03/12] nfs: tell the VFS when two names can be one entry on the server

[Jonathan Flynn]

A server that reports itself case-insensitive, or as not preserving
case, can make one entry of two names that the client caches apart,
because the client compares names byte for byte.  Mark the filesystem
on such a server with SB_I_NAMES_COLLIDE, from "vfs: allow the parent
lock to be shared for O_CREAT", so that the VFS keeps the exclusive
directory lock for creates there: the per-name exclusion that replaces
the lock for a create made with it held shared covers one byte string,
not the names the server treats as equal.  The client learns this when
it reads the server's capabilities, which it does again on remount, on
migration and when a later mount reuses the filesystem, and it updates
the mark each time.

This is a conservative fallback.  A server that declares itself
case-insensitive should itself treat the two names as one, but one that
did not was seen making two files of one name when such creates arrived
together.  No filesystem relies on the mark yet, so nothing changes.

Signed-off-by: Jonathan Flynn <jonathan.flynn@hammerspace.com>
---
 fs/nfs/client.c   |  1 +
 fs/nfs/internal.h | 35 +++++++++++++++++++++++++++++++++++
 fs/nfs/nfs4proc.c |  1 +
 fs/nfs/super.c    |  2 ++
 4 files changed, 39 insertions(+)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index ea99b3c105..30206b309c 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -975,6 +975,7 @@ static int nfs_probe_fsinfo(struct nfs_server *server, struct nfs_fh *mntfh, str
 			server->caps &= ~(NFS_CAP_CASE_INSENSITIVE |
 					  NFS_CAP_CASE_NONPRESERVING);
 		}
+		nfs_server_publish_name_caps(server);
 	}
 
 	if (clp->rpc_ops->discover_trunking != NULL &&
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 5af6a032f1..34b8aff4e0 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -224,6 +224,41 @@ int nfs_init_server_rpcclient(struct nfs_server *, const struct rpc_timeout *t,
 struct nfs_server *nfs_alloc_server(void);
 void nfs_server_copy_userdata(struct nfs_server *, struct nfs_server *);
 
+/*
+ * Tell the VFS whether this server can make one directory entry of two names
+ * that differ as byte strings.  NFS installs no ->d_hash or ->d_compare, so
+ * the dcache compares names byte for byte, while a server that is
+ * case-insensitive, or does not preserve case, makes "foo" and "FOO" one
+ * entry.  On such a server SB_I_NAMES_COLLIDE keeps the parent of an O_CREAT
+ * open locked exclusive; see atomic_open_create_shared() in fs/namei.c.
+ *
+ * server->caps is derived again on every probe of the server: at mount, at
+ * remount, on migration, and when a later mount reuses this superblock
+ * (nfs4_proc_get_root()).  So this is called wherever the case bits of
+ * server->caps are written, once they have their final value, and when the
+ * superblock is set up, because the first probe runs before it exists.  The
+ * value is what the probed file handle reported, normally the mount root's;
+ * the homogeneous attribute is not fetched, so a server whose answer differs
+ * below the root is described by the root.  Migration runs the probe from
+ * the state manager without ->s_umount, so the update is atomic, and it is
+ * made only when the value changes.  Two probes of one server at once can
+ * leave the bit as the earlier of them derived it; that needs the server's
+ * answer to change between them, and server->caps has the same race.
+ */
+static inline void nfs_server_publish_name_caps(struct nfs_server *server)
+{
+	struct super_block *sb = server->super;
+	unsigned long bits = 0;
+
+	if (!sb)
+		return;
+	if (READ_ONCE(server->caps) & (NFS_CAP_CASE_INSENSITIVE |
+				       NFS_CAP_CASE_NONPRESERVING))
+		bits = SB_I_NAMES_COLLIDE;
+	if ((READ_ONCE(sb->s_iflags) & SB_I_NAMES_COLLIDE) != bits)
+		set_mask_bits(&sb->s_iflags, SB_I_NAMES_COLLIDE, bits);
+}
+
 extern void nfs_put_client(struct nfs_client *);
 extern void nfs_free_client(struct nfs_client *);
 void nfs_cb_idr_remove(struct nfs_client *clp);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 113c199a6f..fac8450b65 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4020,6 +4020,7 @@ static int _nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh *f
 		if (nfs4_server_delegtime_capable(&res))
 			caps |= NFS_CAP_DELEGTIME;
 		WRITE_ONCE(server->caps, caps);
+		nfs_server_publish_name_caps(server);
 
 		memcpy(server->cache_consistency_bitmask, res.attr_bitmask, sizeof(server->cache_consistency_bitmask));
 		server->cache_consistency_bitmask[0] &= FATTR4_WORD0_CHANGE|FATTR4_WORD0_SIZE;
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 23292680ad..e70cef88fb 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1082,6 +1082,8 @@ static void nfs_fill_super(struct super_block *sb, struct nfs_fs_context *ctx)
 			sb->s_export_op = &nfs_export_ops;
 		break;
 	}
+	/* The first probe of the caps ran before this superblock existed */
+	nfs_server_publish_name_caps(server);
 
 	sb->s_magic = NFS_SUPER_MAGIC;
 
-- 
2.43.0

[PATCH RFC 04/12] nfs: concurrent O_CREAT in one NFSv4 directory

[Jonathan Flynn]

Set IOPF_ATOMIC_OPEN_CREATE_SHARED in nfs4_dir_inode_operations, so
that the VFS takes the parent shared rather than exclusive for an
O_CREAT open in an NFSv4 directory.  Creates of different names in one
directory then stop serializing on the parent inode, except on a server
that reports itself case-insensitive or not case-preserving, where
"nfs: tell the VFS when two names can be one entry on the server" has
set SB_I_NAMES_COLLIDE and creates keep the exclusive lock.

This is the only user of "vfs: allow the parent lock to be shared for
O_CREAT", and what it needs from it is the guarantee that the dentry
reaching the atomic-open method for a create is in-lookup.  NFSv4 keeps
the dentry the VFS gives it when O_CREAT is set, and publishes the new
inode on that dentry; because it is in-lookup, the dentry is not in the
main hash, so dropping it is a no-op and publishing it is permitted.
The name enters the hash exactly once, and until it does, a concurrent
lookup or create of the same name finds the in-lookup dentry and waits.

NFS does not use the exclusive parent lock to serialize its own
per-directory state, which is what that patch asks an opting-in
filesystem to promise.  Both pieces of state a create touches are
already under finer locks: the directory's change attribute and the
cache-validity bookkeeping that moves with it are updated under the
directory's own spinlock, and a dentry's directory verifier is stored
under that dentry's lock.  Neither needs the parent lock held
exclusive, or held at all.  The verifier is also ordered so that losing
a race can only cost work: a verifier offered against a directory state
it no longer reflects is declined rather than stored, so the failure
direction is an extra revalidation, never a dentry claiming stale
validity.

Lookup and readdir already run with the parent held shared, and
readdir already instantiates dentries there under that lock:
nfs_prime_dcache() takes an in-lookup dentry with d_alloc_trylock(),
skipping a name another thread holds in-lookup, and invalidates and
retries a hashed dentry that no longer matches the listing.  A create
joins concurrency NFS already handles.

NeilBrown's "nfs: remove d_drop()/d_alloc_parallel() from
nfs_atomic_open()" states that the method has exclusive access to the
dentry in one of two ways: O_CREAT, where the parent is held exclusive,
or a dentry carrying DCACHE_PAR_LOOKUP.  Setting the flag removes the
first of those for open(2), which reaches the method through
lookup_open(); nfsd reaches it through dentry_create(), which still
holds the parent exclusive.  "vfs: allow the parent lock to be shared
for O_CREAT" supplies the second in its place: with the parent held
shared for a create, a hashed negative dentry is rejected and the
lookup retried, so an in-lookup dentry is what reaches the method.
NFS's invalidation of a cached negative for LOOKUP_OPEN and that
rejection do not overlap: nfs_neg_need_reval() returns 0 for
LOOKUP_CREATE before it can reach the LOOKUP_OPEN case that invalidates
a cached negative, so the invalidation that covers a plain open never
fires for a create, and the rejection in "vfs: allow the parent lock to
be shared for O_CREAT" never fires for anything else.  The opener newly
in company with a shared create is one opening the same name without
O_CREAT, and that is the opener the invalidation covers.

The flag goes on the NFSv4 directory operations only.  NFSv2 and NFSv3
reach a different atomic-open path, which has not been analysed or
exercised under a shared parent lock, so it does not get the flag;
adding it later is one line.  Referral and automount directories are
unaffected, since they are given different operations vectors.

Every other directory-modifying operation still takes the parent
exclusive; for unlink that changes when "nfs: remove files with the
parent directory locked shared on NFSv4", later in this series, gives
NFSv4 ->unlink_shared().

The measured effect, from a 7.1.13-based kernel carrying an earlier
revision of both patches, is in "vfs: allow the parent lock to be
shared for O_CREAT".

Signed-off-by: Jonathan Flynn <jonathan.flynn@hammerspace.com>
---
 fs/nfs/nfs4proc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index fac8450b65..808bb8f1c1 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -10869,6 +10869,7 @@ static const struct inode_operations nfs4_dir_inode_operations = {
 	.setattr	= nfs_setattr,
 	.listxattr	= nfs4_listxattr,
 	.fileattr_get	= nfs_fileattr_get,
+	.iop_flags	= IOPF_ATOMIC_OPEN_CREATE_SHARED,
 };
 
 static const struct inode_operations nfs4_file_inode_operations = {
-- 
2.43.0

[PATCH RFC 05/12] vfs: factor file removal into a helper, no functional change

[Jonathan Flynn]

Move the body of vfs_unlink() into a static helper, __vfs_unlink(),
that calls the method it is passed in place of ->unlink(), or
->unlink() when passed NULL.  vfs_unlink() becomes a wrapper that
passes NULL, so every caller behaves exactly as before; its kerneldoc,
including the requirement that the caller hold the parent directory's
lock exclusive, does not change.

This prepares for a second removal method, called with the parent
directory's lock held shared, which a following patch adds.

Signed-off-by: Jonathan Flynn <jonathan.flynn@hammerspace.com>
---
 fs/namei.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index ce10a408d3..1b3a605ded 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -5679,6 +5679,12 @@ SYSCALL_DEFINE1(rmdir, const char __user *, pathname)
 	return filename_rmdir(AT_FDCWD, name);
 }
 
+static int __vfs_unlink(const struct mnt_idmap *idmap, struct inode *dir,
+			struct dentry *dentry,
+			struct delegated_inode *delegated_inode,
+			int (*method)(struct inode *dir,
+				      struct dentry *dentry));
+
 /**
  * vfs_unlink - unlink a filesystem object
  * @idmap:	idmap of the mount the inode was found from
@@ -5706,6 +5712,20 @@ SYSCALL_DEFINE1(rmdir, const char __user *, pathname)
  */
 int vfs_unlink(const struct mnt_idmap *idmap, struct inode *dir,
 	       struct dentry *dentry, struct delegated_inode *delegated_inode)
+{
+	return __vfs_unlink(idmap, dir, dentry, delegated_inode, NULL);
+}
+EXPORT_SYMBOL(vfs_unlink);
+
+/*
+ * The body of vfs_unlink().  @method, if not NULL, is called in place of
+ * dir->i_op->unlink(); vfs_unlink() passes NULL.
+ */
+static int __vfs_unlink(const struct mnt_idmap *idmap, struct inode *dir,
+			struct dentry *dentry,
+			struct delegated_inode *delegated_inode,
+			int (*method)(struct inode *dir,
+				      struct dentry *dentry))
 {
 	struct inode *target = dentry->d_inode;
 	int error = may_delete_dentry(idmap, dir, dentry, false);
@@ -5730,7 +5750,10 @@ int vfs_unlink(const struct mnt_idmap *idmap, struct inode *dir,
 			error = try_break_deleg(target, 0, delegated_inode);
 			if (error)
 				goto out;
-			error = dir->i_op->unlink(dir, dentry);
+			if (method)
+				error = method(dir, dentry);
+			else
+				error = dir->i_op->unlink(dir, dentry);
 			if (!error) {
 				dont_mount(dentry);
 				detach_mounts(dentry);
@@ -5750,7 +5773,6 @@ int vfs_unlink(const struct mnt_idmap *idmap, struct inode *dir,
 
 	return error;
 }
-EXPORT_SYMBOL(vfs_unlink);
 
 /*
  * Make sure that the actual truncation of the file will occur outside its
-- 
2.43.0

[PATCH RFC 06/12] vfs: let a filesystem remove files with the parent directory locked shared

[Jonathan Flynn]

Let removals of different files in one directory run concurrently on a
filesystem that orders them itself.  Today they are serialized by the
directory's lock even when a server does the ordering, so tasks removing
their own files from one shared directory wait for one another.

A filesystem opts in by providing two new inode operations alongside
->unlink(): ->unlink_shared() is called to unlink a file with the
directory locked shared, and ->unlink_shared_done() after any delete
event for that removal and before the lock is dropped, to end the
exclusion for the name that ->unlink_shared() set up.  The existing
->unlink() is still called only with the lock held exclusive, by every
caller it has today, so neither removal method has to ask how the lock
is held.  ->unlink_shared() can hand a removal back by returning
UNLINK_NEED_EXCL, having done nothing; the VFS then retries it with the
lock held exclusive, through ->unlink().  A directory found by the
shared lookup is retried the same way without the method being called,
unless the name ends in a slash, which is answered -EISDIR from the
type alone, as before.  On a superblock marked SB_I_NAMES_COLLIDE,
where two names the dcache tells apart can be one entry, removals take
the lock exclusive from the start, as O_CREAT opens do there.

Because the exclusion lasts until the delete event for the removal has
been emitted, an O_CREAT open of the same name made with the directory
locked shared, as IOPF_ATOMIC_OPEN_CREATE_SHARED allows, emits its
create event after that one.  A create that arrives only once the
removal has unhashed the dentry is not ordered against it while that
event is being emitted.

The locking documentation sets out what the shared lock still
guarantees, what the filesystem takes on in its place, and the costs.
A handed-back removal goes through its permission, security and
delegation checks, and its audit record, twice.  Removals made with the
lock shared bypass vfs_unlink(), so a probe on it no longer sees them;
the security hooks and notifications still run.  The victim is looked
up through d_alloc_parallel() under the shared lock, and the argument
that this adds no deadlock assumes, as today, that nothing holding an
in-lookup dentry waits for the directory's lock.

No filesystem provides the methods yet, so nothing changes.

Signed-off-by: Jonathan Flynn <jonathan.flynn@hammerspace.com>
---
 .../filesystems/directory-locking.rst         |  41 +++
 Documentation/filesystems/locking.rst         | 180 +++++++++++-
 Documentation/filesystems/porting.rst         |  35 +++
 Documentation/filesystems/vfs.rst             |  25 ++
 fs/namei.c                                    | 260 +++++++++++++++++-
 include/linux/fs.h                            |  39 ++-
 6 files changed, 563 insertions(+), 17 deletions(-)

diff --git a/Documentation/filesystems/directory-locking.rst b/Documentation/filesystems/directory-locking.rst
index 316f10cb3d..2d9533f824 100644
--- a/Documentation/filesystems/directory-locking.rst
+++ b/Documentation/filesystems/directory-locking.rst
@@ -50,6 +50,47 @@ For our purposes all operations fall in 6 classes:
 	* find the victim
 	* lock the victim (exclusive)
 
+	unlink(2) of a non-directory is the one exception: if the
+	filesystem provides ->unlink_shared() together with ->unlink() and
+	->unlink_shared_done(), and its superblock has not set
+	SB_I_NAMES_COLLIDE, the parent is locked shared for it, the victim
+	is found through d_alloc_parallel() and locked exclusive as before,
+	and exclusion for the name comes from the filesystem instead - see
+	Documentation/filesystems/locking.rst.  That takes an existing lock
+	in shared mode and in the existing order, parent before victim; it
+	adds no lock and no new ordering.  That holds while
+	d_alloc_parallel() is called with the parent's ->i_rwsem held, as it
+	is here; once in-lookup dentries are taken before ->i_rwsem, the
+	lookup of the victim has to move ahead of the shared parent lock with
+	them.  If the filesystem declines, or the victim turns out to be a
+	directory, everything is dropped and the removal is done again under
+	the rules above; a name ending in a slash is instead answered -EISDIR
+	or -ENOTDIR from the victim's type alone, as before.
+
+	The victim's parent and name are stable under the shared lock
+	because it is not a directory.  Splicing (see below) moves only
+	directory aliases, and can do so holding the old parent no more
+	than shared, which is why a directory found by the shared lookup is
+	let go with nothing but its type examined.  Any other change of a
+	dentry's parent or name is made by __d_move() for a rename, and
+	every rename holds the parents involved exclusive.  That includes a
+	filesystem renaming the victim to carry out a removal, which
+	->unlink_shared() must therefore hand back.  With the victim's
+	parent and name stable, the ordering analysis below is unaffected.
+
+	Such a removal can make a lookup of its name wait in
+	->d_revalidate() while that lookup holds the parent shared.  The
+	wake comes after the VFS has emitted the delete event, which the
+	removal reaches having let the victim go: by then it needs ->d_lock
+	and the victim's ->i_lock for the unhash, and the notification
+	subsystem's own locks and an allocation for the event.  A waiter in
+	->d_revalidate() holds the parent shared and a reference on the
+	dentry and takes no lock of its own, so it still cannot be blocking
+	the thread that must wake it; the wait is as long as the removal and
+	its delete event together.  An O_CREAT open made under the exception
+	in class 2 can wait there in the same way, and for the same reason
+	cannot be blocking the remover either.
+
 4. link creation.  Locking rules:
 
 	* lock the parent (exclusive)
diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
index c04df4c746..1783f24052 100644
--- a/Documentation/filesystems/locking.rst
+++ b/Documentation/filesystems/locking.rst
@@ -65,6 +65,8 @@ prototypes::
 	struct dentry * (*lookup) (struct inode *,struct dentry *, unsigned int);
 	int (*link) (struct dentry *,struct inode *,struct dentry *);
 	int (*unlink) (struct inode *,struct dentry *);
+	int (*unlink_shared) (struct inode *,struct dentry *);
+	void (*unlink_shared_done) (struct inode *,struct dentry *);
 	int (*symlink) (const struct mnt_idmap *, struct inode *,struct dentry *,const char *);
 	struct dentry *(*mkdir) (const struct mnt_idmap *, struct inode *,struct dentry *,umode_t);
 	int (*rmdir) (struct inode *,struct dentry *);
@@ -107,6 +109,7 @@ mknod:		exclusive
 symlink:	exclusive
 mkdir:		exclusive
 unlink:		exclusive (both)
+unlink_shared:	shared on parent, exclusive on victim (see below)
 rmdir:		exclusive (both)(see below)
 rename:		exclusive (both parents, some children)	(see below)
 readlink:	no
@@ -128,12 +131,16 @@ get_offset_ctx  no
 ==============	==================================================
 
 
-	Additionally, ->rmdir(), ->unlink() and ->rename() have ->i_rwsem
-	exclusive on victim.
+	Additionally, ->rmdir(), ->unlink(), ->unlink_shared() and ->rename()
+	have ->i_rwsem exclusive on victim.
 	cross-directory ->rename() has (per-superblock) ->s_vfs_rename_sem.
-	->unlink() and ->rename() have ->i_rwsem exclusive on all non-directories
-	involved.
+	->unlink(), ->unlink_shared() and ->rename() have ->i_rwsem exclusive
+	on all non-directories involved.
 	->rename() has ->i_rwsem exclusive on any subdirectory that changes parent.
+	->unlink_shared_done() has ->i_rwsem shared on the parent and nothing
+	on the victim.  It is not an operation on the directory but the end of
+	one, and is described with ->unlink_shared() below rather than listed
+	above.
 
 	->atomic_open() with O_CREAT set is called with ->i_rwsem on the
 	parent held exclusive, unless the filesystem has set
@@ -168,7 +175,8 @@ get_offset_ctx  no
 	Nothing else changes: ->create(), ->link(), ->unlink(), ->mkdir(),
 	->rmdir(), ->mknod(), ->symlink() and ->rename() still take
 	->i_rwsem exclusive on the parent, so a create holding it shared
-	still excludes them.
+	still excludes them.  It does not exclude ->unlink_shared(), if the
+	filesystem provides that as well; see below.
 
 	The flag is also ignored, and the parent taken exclusive, on a
 	superblock that has set SB_I_NAMES_COLLIDE in ->s_iflags.  A
@@ -214,6 +222,168 @@ get_offset_ctx  no
 	dentries being allocated under ->i_rwsem, which is planned to
 	change.
 
+	->unlink_shared() is an optional second removal method.  For a
+	directory whose ->i_op provides it together with ->unlink() and
+	->unlink_shared_done(), filename_unlinkat() - the path of unlink(2),
+	and of unlinkat(2) without AT_REMOVEDIR - takes ->i_rwsem on the
+	parent shared and calls ->unlink_shared() where it used to call
+	->unlink().  ->unlink() itself is still called only with the parent
+	held exclusive: by the retry described below, and by every other
+	caller of vfs_unlink(), none of which changes.  So each of the two
+	methods knows the mode of the parent lock from having been called,
+	and neither needs to ask the lock.  On a superblock that has set
+	SB_I_NAMES_COLLIDE, ->unlink_shared() is not called, for the reason
+	given for O_CREAT above: the exclusion it installs is per name as
+	the dcache compares names.  The VFS reads the bit before it takes
+	the parent lock, so a removal that read it clear may still call
+	->unlink_shared() after it is set; a filesystem that sets the bit
+	while mounted must hand such a removal back.
+
+	->unlink_shared_done() is the other half of the method and is
+	mandatory with it: a directory whose ->i_op has one without the
+	other has not opted in, and its removals go to ->unlink() with the
+	parent held exclusive as before.  The VFS calls it for every call of
+	->unlink_shared() that did not return a positive value, and for no
+	other, with ->i_rwsem on the parent still held shared and nothing
+	held on the victim, after any delete event for the removal and
+	before the parent lock is dropped.  It is where the exclusion
+	->unlink_shared() installed for the name ends; see the order of
+	notifications below.
+
+	With the parent held shared the VFS still provides this much.  The
+	dentry was resolved under this hold of the lock through
+	d_alloc_parallel(); it is positive, is not a directory, and is
+	referenced until after the method returns.  ->i_rwsem on the victim
+	is held exclusive across the call, as it is for ->unlink(), so two
+	removals of one inode are serialized.  ->create(), ->link(),
+	->symlink(), ->mkdir(), ->mknod(), ->rmdir() and ->rename() still
+	take the parent exclusive and so are still excluded.  And while the
+	dentry stays hashed, no second dentry for its name can be hashed:
+	d_alloc_parallel() returns a hashed dentry rather than creating
+	in-lookup state for the name, so a lookup or an open that resolves
+	the name calls ->d_revalidate() on that dentry, and a hashed
+	positive dentry is never passed to ->atomic_open().
+
+	What the VFS no longer provides is exclusion of ->lookup(), of any
+	->atomic_open() called with the parent held shared (with O_CREAT
+	too, if the filesystem has set IOPF_ATOMIC_OPEN_CREATE_SHARED), of
+	->iterate_shared() or of a second ->unlink_shared() for the same
+	name.  Nor does it keep the dentry hashed: a lookup whose
+	->d_revalidate() fails d_invalidate()s it, and d_delete() on behalf
+	of another remover of the name unhashes it while it is still
+	positive.  Nor is anything except a lookup of the name routed
+	through ->d_revalidate(): the filesystem's own users of d_lookup()
+	and d_alloc_parallel(), readdir priming among them, reach the
+	dentry directly.
+
+	The filesystem therefore orders the removal itself against every
+	lookup, open and create of the same name that the parent lock no
+	longer excludes, and against its own directory listings: a listing
+	that the backing store may have produced before a removal took
+	effect - one begun before the removal, or while it was in flight -
+	must not instantiate the removed name, and must not refresh the
+	inode being removed, once that removal may have completed.
+	Whatever it anchors that ordering on is found through the dcache,
+	which compares names as the filesystem's dentry operations do, byte
+	for byte without them.  If the backing store treats two names as
+	one object where those do not compare equal, through case folding
+	or Unicode normalization, the removal of one name is not ordered
+	against a lookup or open of the other that the parent lock no
+	longer excludes.  Such removals must be handed back, and where the
+	filesystem cannot tell that they can happen, that is a limitation
+	it documents.
+
+	One arrangement that provides the ordering is a per-dentry marker
+	that ->d_revalidate() waits on, kept by these rules:
+
+	- the marker is installed under ->d_lock, and only on a dentry that
+	  in that same ->d_lock section is still hashed, is still positive
+	  and has no reference but the caller's.  A resolver that can fail
+	  ->d_revalidate(), and so have the dentry unhashed, takes its
+	  reference before it consults ->d_revalidate(), so with a count of
+	  one every resolver that consults ->d_revalidate() after the marker
+	  is installed finds it and waits.  In that same ->d_lock section,
+	  after installing the marker and not before, invalidate ->d_seq
+	  (write_seqcount_invalidate(), whose barrier orders only earlier
+	  stores): an RCU-walk that consulted ->d_revalidate() before the
+	  marker was installed then fails its ->d_seq recheck when it takes
+	  its reference, and retries in ref-walk, where it finds the marker
+	  and waits.  The one resolver this does not reach is an open by
+	  file handle, whose reference comes from d_obtain_alias() without a
+	  ->d_revalidate(); the exclusive parent lock never excluded it
+	  either;
+	- RCU-walk consults ->d_revalidate() without a reference and cannot
+	  wait, so for the rule above to hold ->d_revalidate() must answer
+	  -ECHILD under LOOKUP_RCU while the marker is set, and must never
+	  answer 0 under LOOKUP_RCU.  -ECHILD sends the walk to ref-walk,
+	  where the consult is repeated with a reference held;
+	- none of the filesystem's own paths but the remover's unhashes the
+	  dentry while the marker is set, and the remover does so only once
+	  the backing store has answered its removal; none hands the dentry
+	  to a caller as the result of a lookup without waiting for the
+	  marker.  Erring toward leaving the dentry hashed is safe: a later
+	  resolver of the name waits for the removal, after which the
+	  remover's own d_delete() normally unhashes it, and an open by path
+	  that fails on it with -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, which tells ->d_revalidate() not to trust its cache;
+	- the marker is cleared in ->unlink_shared_done() and not before
+	  ->unlink_shared() returns, so that it outlasts the delete event the
+	  VFS emits for the removal;
+	- anything else - a second reference, a dentry that is no longer
+	  hashed, a removal the filesystem would carry out by renaming - is
+	  answered with UNLINK_NEED_EXCL.
+
+	UNLINK_NEED_EXCL is a positive value that only ->unlink_shared() may
+	return.  It says that this removal cannot be made with the parent
+	held shared and that nothing was done.  The VFS then drops the lock,
+	takes it exclusive, looks the name up again and calls ->unlink(),
+	which cannot ask again.  A directory found by the shared lookup is
+	sent the same way without the method being called, unless the name
+	ends in a slash, which is answered -EISDIR from its type alone; so
+	->unlink_shared() is never passed a directory.  A removal that the
+	method hands back runs the permission check, the audit record, both
+	security hooks and both delegation checks once in each pass, and its
+	name is looked up twice.
+
+	A lookup that holds the parent shared can now wait in
+	->d_revalidate() for such a removal, for as long as the removal
+	takes; before, it would have waited for the parent lock instead.  If
+	the filesystem's wait there is uninterruptible, so is this one.
+
+	The order of notifications for one name is what
+	->unlink_shared_done() keeps.  ->unlink_shared() returns with its
+	exclusion for the name still installed; the VFS emits the delete
+	event for the removal, then calls ->unlink_shared_done() to end it,
+	then drops the parent lock.  A creator of the name that the call
+	releases emits its create event after that delete event whichever
+	mode it holds the parent in: one that takes the parent exclusive is
+	waiting for a lock that is held until after the call, and one that
+	holds it shared - an O_CREAT ->atomic_open(), once the filesystem
+	has set IOPF_ATOMIC_OPEN_CREATE_SHARED - is waiting for the
+	exclusion itself.  A filesystem that ends its exclusion inside
+	->unlink_shared() instead loses that order against a creator of the
+	second kind: the create event can come before the delete event of
+	the removal it waited for, and a watcher then concludes that the
+	name is gone while it exists.  That is why ->unlink_shared_done() is
+	not optional.
+
+	What this does not order is a creator that never met the exclusion.
+	When d_delete_notify() unhashes the name before it emits the event -
+	when another reference is held at the delete, or the
+	fs.dentry-negative sysctl is set - a resolver that arrives after the
+	unhash finds no dentry for the name, makes its own, and is ordered
+	against the removal by nothing; the window is the span of the event
+	itself.  Nor does any of this decide what a creator released by
+	->unlink_shared_done() goes on to resolve: the dentry it was waiting
+	on may still be positive, and whether ->d_revalidate() then sends it
+	to the backing store is the filesystem's answer and not the VFS's.
+
+	->unlink_shared() is meant as a transitional interface.  It lets one
+	filesystem at a time stop relying on the exclusive parent lock for
+	removal, and ->unlink() and ->unlink_shared() can become one again
+	once removal locks the target dentry and not the parent.
+
 See Documentation/filesystems/directory-locking.rst for more detailed discussion
 of the locking scheme for directory operations.
 
diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
index c38f5df755..029c178e2f 100644
--- a/Documentation/filesystems/porting.rst
+++ b/Documentation/filesystems/porting.rst
@@ -1422,3 +1422,38 @@ helpers take a const struct mnt_idmap *. mnt_idmap() and file_mnt_idmap()
 return one. The idmapping is immutable so nothing should have modified it
 anyway. References are taken and dropped via mnt_idmap_get() and
 mnt_idmap_put() as before, both accept a const pointer.
+
+---
+
+**informational**
+
+->unlink_shared() is added, as an optional inode operation.  A filesystem
+that does not provide it sees no change: ->unlink() is still called with the
+parent's ->i_rwsem held exclusive.  One that provides it has unlink(2) call
+it with the parent held shared instead, and takes on ordering a removal
+against the lookups and opens of the same name that the lock no longer
+excludes, and against its own directory listings.  It may return
+UNLINK_NEED_EXCL, having done nothing, to have ->unlink() called for that
+removal with the parent held exclusive after all; ->unlink() has to stay
+correct for that case and for every other caller of vfs_unlink().
+
+->unlink_shared() is paired with ->unlink_shared_done() and needs it: a
+filesystem that provides one without the other has not opted in.  Whatever
+->unlink_shared() installed to order the removal against the resolvers of the
+name is still installed when it returns, and ->unlink_shared_done() ends it,
+after any delete event the VFS emits for the removal.  The VFS calls it for
+every call of ->unlink_shared() that did not return a positive value, and for
+no other.
+
+For such a filesystem the ->iterate_shared() entry above no longer holds in
+full.  ->iterate_shared() is not excluded from that removal, and for dcache
+pre-seeding d_alloc_parallel() alone is not enough: a listing that may have
+been produced before a removal took effect, including one begun while the
+removal was in flight, must not instantiate the removed name, or refresh the
+inode being removed, once that removal may have completed.  See
+Documentation/filesystems/locking.rst.
+
+If the filesystem has also set IOPF_ATOMIC_OPEN_CREATE_SHARED, the opens
+that the lock no longer excludes include O_CREAT ones.  That flag already
+makes the ->iterate_shared() entry untrue for those opens, which hold the
+parent shared alongside ->iterate_shared().
diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst
index 1824462a43..a6eeac44e1 100644
--- a/Documentation/filesystems/vfs.rst
+++ b/Documentation/filesystems/vfs.rst
@@ -419,6 +419,8 @@ As of kernel 2.6.22, the following members are defined:
 		struct dentry * (*lookup) (struct inode *,struct dentry *, unsigned int);
 		int (*link) (struct dentry *,struct inode *,struct dentry *);
 		int (*unlink) (struct inode *,struct dentry *);
+		int (*unlink_shared) (struct inode *,struct dentry *);
+		void (*unlink_shared_done) (struct inode *,struct dentry *);
 		int (*symlink) (const struct mnt_idmap *, struct inode *,struct dentry *,const char *);
 		struct dentry *(*mkdir) (const struct mnt_idmap *, struct inode *,struct dentry *,umode_t);
 		int (*rmdir) (struct inode *,struct dentry *);
@@ -481,6 +483,29 @@ otherwise noted.
 	called by the unlink(2) system call.  Only required if you want
 	to support deleting inodes
 
+``unlink_shared``
+	optional.  As ``unlink``, but called by the unlink(2) system
+	call with the parent directory's i_rwsem held shared, not
+	exclusive, and never for a directory.  Providing it, together
+	with ``unlink`` and ``unlink_shared_done``, is how a filesystem
+	opts in to concurrent removals in one directory; the filesystem
+	then itself orders a removal against the lookups and opens of the
+	same name that the lock no longer excludes, and against its own
+	directory listings.  It may return UNLINK_NEED_EXCL, having done
+	nothing, and the VFS then calls ``unlink`` for that removal with
+	the lock held exclusive.  ``unlink`` is still what every other
+	caller uses.  ``unlink_shared`` is not called on a superblock that
+	has set SB_I_NAMES_COLLIDE, except by a removal that read the bit
+	before it was set.  See Documentation/filesystems/locking.rst.
+
+``unlink_shared_done``
+	required with ``unlink_shared``.  Called after every call of
+	``unlink_shared`` that did not return a positive value, whether
+	the removal succeeded or not, after any delete event for it and
+	before the parent directory's i_rwsem is dropped, to end the
+	exclusion for the name that ``unlink_shared`` set up.  See
+	Documentation/filesystems/locking.rst.
+
 ``symlink``
 	called by the symlink(2) system call.  Only required if you want
 	to support symlinks.  You will probably need to call
diff --git a/fs/namei.c b/fs/namei.c
index 1b3a605ded..0028a18fb1 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2958,6 +2958,116 @@ void end_dirop(struct dentry *de)
 }
 EXPORT_SYMBOL(end_dirop);
 
+/*
+ * Resolve @name in @base for a removal, with the parent's i_rwsem held and
+ * possibly only shared.  Unlike lookup_one_qstr_excl() this uses the
+ * in-lookup protocol -- d_alloc_parallel() rather than d_alloc() plus
+ * ->lookup() -- because only that excludes a second resolver of the same
+ * name when the parent lock does not.  A negative result is -ENOENT;
+ * LOOKUP_CREATE and LOOKUP_EXCL have no meaning here.
+ *
+ * A directory is returned like anything else, and the caller must let go
+ * of it having looked at no more than its type: __d_unalias() can move or
+ * rename a directory alias holding its parent's i_rwsem no more than
+ * shared, which a shared hold does not exclude.  A non-directory's parent
+ * and name change only in __d_move() for a rename or a silly-rename, both
+ * of which hold the parent exclusive.
+ */
+static struct dentry *lookup_one_qstr_shared(const struct qstr *name,
+					     struct dentry *base,
+					     unsigned int flags)
+{
+	/* LOOKUP_SHARED: the parent is held shared, as for lookup_slow() */
+	struct dentry *dentry = __lookup_slow(name, base, flags | LOOKUP_SHARED);
+
+	if (IS_ERR(dentry))
+		return dentry;
+	if (d_is_negative(dentry)) {
+		dput(dentry);
+		return ERR_PTR(-ENOENT);
+	}
+	return dentry;
+}
+
+/**
+ * start_dirop_shared - begin a removal with the parent held shared
+ * @parent:       the dentry of the parent in which the operation will occur
+ * @name:         a qstr holding the name within that parent
+ * @lookup_flags: intent and other lookup flags
+ *
+ * As start_dirop(), except that the parent's i_rwsem is taken shared, so
+ * what excludes a concurrent operation on this name is not the lock.  It is
+ * the returned dentry -- positive, referenced for as long as the caller
+ * holds it, and resolved under this hold through d_alloc_parallel() -- and
+ * whatever the filesystem builds on that, because the VFS does not keep the
+ * dentry hashed.  Only for a directory whose filesystem provides
+ * ->unlink_shared().
+ *
+ * end_dirop_shared() must be used to finish, not end_dirop().
+ *
+ * Returns: a positive dentry, which may be a directory, or an error.
+ */
+static struct dentry *start_dirop_shared(struct dentry *parent,
+					 struct qstr *name,
+					 unsigned int lookup_flags)
+{
+	struct inode *dir = d_inode(parent);
+	struct dentry *dentry;
+
+	/* I_MUTEX_PARENT, as __start_dirop(): __vfs_unlink() nests
+	 * inode_lock() on the victim inside this one.
+	 */
+	inode_lock_shared_nested(dir, I_MUTEX_PARENT);
+	dentry = lookup_one_qstr_shared(name, parent, lookup_flags);
+	if (IS_ERR(dentry))
+		inode_unlock_shared(dir);
+	return dentry;
+}
+
+/**
+ * end_dirop_shared - signal completion of a dirop started shared
+ * @parent: the parent that was passed to start_dirop_shared()
+ * @de: the dentry which was returned by start_dirop_shared()
+ *
+ * As end_dirop(), for the shared lock.  If @de is an error, nothing
+ * happens.  The lock dropped is the one start_dirop_shared() took, not
+ * re-derived from @de->d_parent as end_dirop() does: it is the exclusive
+ * parent lock that makes a child's d_parent stable, and this is not it.
+ */
+static void end_dirop_shared(struct dentry *parent, struct dentry *de)
+{
+	if (!IS_ERR(de)) {
+		inode_unlock_shared(d_inode(parent));
+		dput(de);
+	}
+}
+
+/*
+ * The pair filename_unlinkat() begins and ends a removal with: the shared
+ * pair above when @unlink_shared is the method it read, start_dirop() and
+ * end_dirop() when it is NULL.  The caller passes the same pointer to both.
+ */
+static struct dentry *start_unlink_dirop(struct dentry *parent,
+					 struct qstr *name,
+					 unsigned int lookup_flags,
+					 int (*unlink_shared)(struct inode *,
+							      struct dentry *))
+{
+	if (unlink_shared)
+		return start_dirop_shared(parent, name, lookup_flags);
+	return start_dirop(parent, name, lookup_flags);
+}
+
+static void end_unlink_dirop(struct dentry *parent, struct dentry *de,
+			     int (*unlink_shared)(struct inode *,
+						  struct dentry *))
+{
+	if (unlink_shared)
+		end_dirop_shared(parent, de);
+	else
+		end_dirop(de);
+}
+
 /* does lookup, returns the object with parent locked */
 struct dentry *start_removing_path(const char *name, struct path *path)
 {
@@ -5683,7 +5793,9 @@ static int __vfs_unlink(const struct mnt_idmap *idmap, struct inode *dir,
 			struct dentry *dentry,
 			struct delegated_inode *delegated_inode,
 			int (*method)(struct inode *dir,
-				      struct dentry *dentry));
+				      struct dentry *dentry),
+			void (*done)(struct inode *dir,
+				     struct dentry *dentry));
 
 /**
  * vfs_unlink - unlink a filesystem object
@@ -5713,21 +5825,40 @@ static int __vfs_unlink(const struct mnt_idmap *idmap, struct inode *dir,
 int vfs_unlink(const struct mnt_idmap *idmap, struct inode *dir,
 	       struct dentry *dentry, struct delegated_inode *delegated_inode)
 {
-	return __vfs_unlink(idmap, dir, dentry, delegated_inode, NULL);
+	return __vfs_unlink(idmap, dir, dentry, delegated_inode, NULL, NULL);
 }
 EXPORT_SYMBOL(vfs_unlink);
 
 /*
  * The body of vfs_unlink().  @method, if not NULL, is called in place of
- * dir->i_op->unlink(); vfs_unlink() passes NULL.
+ * dir->i_op->unlink(); vfs_unlink() passes NULL, so ->unlink() is still
+ * called only with dir->i_rwsem held exclusive.  vfs_unlink_shared() passes
+ * the directory's ->unlink_shared(), which its caller read before taking
+ * the lock, to choose the shared mode it then took.  It is passed in, not
+ * read here, so that the lock mode and the method come from one read of
+ * ->i_op.  A positive return from it means that nothing was removed:
+ * everything that follows a removal is skipped, and the value is handed
+ * back.
+ *
+ * @done is the directory's ->unlink_shared_done(), from that same read of
+ * ->i_op: both or neither.  It is called when @method has been called and
+ * has not handed the removal back, after any event for the removal is out
+ * and before the caller drops the parent's lock, to end the exclusion for
+ * the name that @method returned still holding.  A creator of the name that
+ * the call releases therefore cannot have its create event out first.  The
+ * paths that do not reach @method installed no exclusion and so make no
+ * call: this is the one function that knows which of the two happened.
  */
 static int __vfs_unlink(const struct mnt_idmap *idmap, struct inode *dir,
 			struct dentry *dentry,
 			struct delegated_inode *delegated_inode,
 			int (*method)(struct inode *dir,
-				      struct dentry *dentry))
+				      struct dentry *dentry),
+			void (*done)(struct inode *dir,
+				     struct dentry *dentry))
 {
 	struct inode *target = dentry->d_inode;
+	bool call_done = false;
 	int error = may_delete_dentry(idmap, dir, dentry, false);
 
 	if (error)
@@ -5750,10 +5881,13 @@ static int __vfs_unlink(const struct mnt_idmap *idmap, struct inode *dir,
 			error = try_break_deleg(target, 0, delegated_inode);
 			if (error)
 				goto out;
-			if (method)
+			if (method) {
 				error = method(dir, dentry);
-			else
+				/* A positive return left nothing to end. */
+				call_done = error <= 0;
+			} else {
 				error = dir->i_op->unlink(dir, dentry);
+			}
 			if (!error) {
 				dont_mount(dentry);
 				detach_mounts(dentry);
@@ -5771,9 +5905,43 @@ static int __vfs_unlink(const struct mnt_idmap *idmap, struct inode *dir,
 		d_delete_notify(dir, dentry);
 	}
 
+	/*
+	 * After the event, so that the exclusion the method kept for the name
+	 * outlasts it: a creator of the name waiting for that exclusion is
+	 * still waiting while the event is delivered, and one waiting for the
+	 * parent's lock is waiting for a lock the caller still holds.
+	 */
+	if (call_done)
+		done(dir, dentry);
+
 	return error;
 }
 
+/*
+ * As vfs_unlink(), for a caller that read @unlink_shared, the directory's
+ * ->unlink_shared(), to choose the lock mode, and then took dir->i_rwsem
+ * shared.  @dentry must be a non-directory that start_dirop_shared()
+ * resolved under that hold.  Returns a positive value, UNLINK_NEED_EXCL,
+ * with nothing removed, if the filesystem could not do it with the lock
+ * shared; the caller then does it again through vfs_unlink() with the lock
+ * held exclusive.  @unlink_shared_done is the directory's
+ * ->unlink_shared_done(), from the same read of ->i_op, and goes with it.
+ * Not exported: filename_unlinkat() is the only caller.
+ */
+static int vfs_unlink_shared(const struct mnt_idmap *idmap, struct inode *dir,
+			     struct dentry *dentry,
+			     struct delegated_inode *delegated_inode,
+			     int (*unlink_shared)(struct inode *dir,
+						  struct dentry *dentry),
+			     void (*unlink_shared_done)(struct inode *dir,
+							struct dentry *dentry))
+{
+	/* A directory is never kept under the shared lock. */
+	VFS_WARN_ON_ONCE(d_is_dir(dentry));
+	return __vfs_unlink(idmap, dir, dentry, delegated_inode,
+			    unlink_shared, unlink_shared_done);
+}
+
 /*
  * Make sure that the actual truncation of the file will occur outside its
  * directory's i_rwsem.  Truncate can take a long time if there is a lot of
@@ -5790,7 +5958,16 @@ int filename_unlinkat(int dfd, struct filename *name)
 	struct inode *inode;
 	struct delegated_inode delegated_inode = { };
 	unsigned int lookup_flags = 0;
+	const struct inode_operations *iop;
+	int (*unlink_shared)(struct inode *dir, struct dentry *dentry);
+	void (*unlink_shared_done)(struct inode *dir, struct dentry *dentry);
+	bool force_excl = false;
 retry:
+	/*
+	 * An -ESTALE re-resolution starts over: an escalation a previous pass
+	 * asked for applied to the dentry that pass found, not to this one.
+	 */
+	force_excl = false;
 	error = filename_parentat(dfd, name, lookup_flags, &path, &last, &type);
 	if (error)
 		return error;
@@ -5803,7 +5980,33 @@ int filename_unlinkat(int dfd, struct filename *name)
 	if (error)
 		goto exit_path_put;
 retry_deleg:
-	dentry = start_dirop(path.dentry, &last, lookup_flags);
+	/*
+	 * One read of ->i_op decides the lock mode, the lookup protocol, the
+	 * method that is called and the unlock.  What is read is the method
+	 * itself, and that is what is passed down, so the method that chose
+	 * the lock mode is the method that is called, and nothing below goes
+	 * back to ->i_op for it.  It is read after retry_deleg: so that each
+	 * acquisition of the lock reads it afresh.  The exclusive retry needs
+	 * ->unlink(), so a filesystem that provides only ->unlink_shared() has
+	 * not opted in.  Nor has one that provides it without
+	 * ->unlink_shared_done(): the exclusion the first installs for the name
+	 * is ended by the second, and there is nowhere else to end it, so the
+	 * three are taken together or not at all.  Nor on a superblock that
+	 * has set SB_I_NAMES_COLLIDE: the exclusion the method installs is per
+	 * name as the dcache compares names, as an O_CREAT open's is, and it
+	 * is tested last for the same reason; see atomic_open_create_shared().
+	 * An O_CREAT open follows the same rule in lookup_open().
+	 */
+	iop = path.dentry->d_inode->i_op;
+	unlink_shared = force_excl ? NULL : iop->unlink_shared;
+	unlink_shared_done = iop->unlink_shared_done;
+	if (!iop->unlink || !unlink_shared || !unlink_shared_done ||
+	    (READ_ONCE(path.dentry->d_sb->s_iflags) & SB_I_NAMES_COLLIDE)) {
+		unlink_shared = NULL;
+		unlink_shared_done = NULL;
+	}
+	dentry = start_unlink_dirop(path.dentry, &last, lookup_flags,
+				    unlink_shared);
 	error = PTR_ERR(dentry);
 	if (IS_ERR(dentry))
 		goto exit_drop_write;
@@ -5814,24 +6017,59 @@ int filename_unlinkat(int dfd, struct filename *name)
 			error = -EISDIR;
 		else
 			error = -ENOTDIR;
-		end_dirop(dentry);
+		end_unlink_dirop(path.dentry, dentry, unlink_shared);
 		goto exit_drop_write;
 	}
+	/*
+	 * A directory is not kept under the shared lock.  __d_unalias() can
+	 * move or rename a directory alias while holding its parent no more
+	 * than shared, and may_delete_dentry() looks at ->d_parent in a
+	 * BUG_ON() before it answers -EISDIR.  Nothing above looked at more
+	 * than the type, and end_dirop_shared() unlocks the parent it is
+	 * given.  Do it again with the lock held exclusive, where the answer
+	 * is the one it has always been.
+	 */
+	if (unlink_shared && d_is_dir(dentry)) {
+		end_dirop_shared(path.dentry, dentry);
+		force_excl = true;
+		goto retry_deleg;
+	}
 	inode = dentry->d_inode;
 	ihold(inode);
 	error = security_path_unlink(&path, dentry);
 	if (error)
 		goto exit_end_dirop;
-	error = vfs_unlink(mnt_idmap(path.mnt), path.dentry->d_inode,
-			   dentry, &delegated_inode);
+	if (unlink_shared)
+		error = vfs_unlink_shared(mnt_idmap(path.mnt),
+					  path.dentry->d_inode, dentry,
+					  &delegated_inode, unlink_shared,
+					  unlink_shared_done);
+	else
+		error = vfs_unlink(mnt_idmap(path.mnt), path.dentry->d_inode,
+				   dentry, &delegated_inode);
 exit_end_dirop:
-	end_dirop(dentry);
+	end_unlink_dirop(path.dentry, dentry, unlink_shared);
 	iput(inode);	/* truncate the inode here */
 	if (is_delegated(&delegated_inode)) {
 		error = break_deleg_wait(&delegated_inode);
 		if (!error)
 			goto retry_deleg;
 	}
+	/*
+	 * UNLINK_NEED_EXCL: ->unlink_shared() cannot do this one with the
+	 * parent held shared, and did nothing.  Do it again with the lock
+	 * held exclusive, through ->unlink(), which cannot ask again; that
+	 * bounds this.  Every positive value is treated alike, so none
+	 * reaches user space from the shared pass.  Being positive, it is
+	 * neither an errno nor the delegation return handled above, which is
+	 * made before the method is called.
+	 */
+	if (unlink_shared && unlikely(error > 0)) {
+		force_excl = true;
+		goto retry_deleg;
+	}
+	/* Only ->unlink_shared() may return a positive value. */
+	VFS_WARN_ON_ONCE(error > 0);
 exit_drop_write:
 	mnt_drop_write(path.mnt);
 exit_path_put:
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f14520194b..c3dea996c0 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2041,8 +2041,41 @@ struct inode_operations {
 	int (*fileattr_get)(struct dentry *dentry, struct file_kattr *fa);
 	struct offset_ctx *(*get_offset_ctx)(struct inode *inode);
 	iop_flags_t iop_flags;
+	/* Last, not beside ->unlink, so that no other member moves */
+	int (*unlink_shared)(struct inode *dir, struct dentry *dentry);
+	void (*unlink_shared_done)(struct inode *dir, struct dentry *dentry);
 } ____cacheline_aligned;
 
+/**
+ * enum unlink_shared_returns - positive returns of ->unlink_shared()
+ *
+ * @UNLINK_NEED_EXCL: this removal cannot be made with the parent's i_rwsem
+ *	held shared, and nothing was done.  The VFS drops the lock, takes it
+ *	exclusive, looks the name up again and calls ->unlink() for it, which
+ *	cannot ask again.
+ *
+ * ->unlink_shared() is ->unlink() for a filesystem that can remove a
+ * non-directory with the parent's i_rwsem held shared; providing it, along
+ * with ->unlink() and ->unlink_shared_done(), is the opt-in.  The VFS calls
+ * it only with the lock held shared and goes on calling ->unlink() only with
+ * the lock held exclusive, so neither method has to ask which mode it was
+ * given.  It returns 0, a negative errno, or a value from this enum, which
+ * no other inode operation may return.  What the VFS still provides under
+ * the shared lock, and what the filesystem owes for the exclusion the lock
+ * no longer gives, is set out in Documentation/filesystems/locking.rst.
+ *
+ * ->unlink_shared() returns with whatever it installed to order the removal
+ * against the resolvers of the name still installed, and
+ * ->unlink_shared_done() ends it.  The VFS calls that after any delete
+ * event for the removal and before it drops the parent's i_rwsem, for every
+ * call of ->unlink_shared() that did not return a positive value and for no
+ * other, so a filesystem providing one without the other has not opted in
+ * and its removals keep the exclusive lock.
+ */
+enum unlink_shared_returns {
+	UNLINK_NEED_EXCL	= 1,
+};
+
 /*
  * ->atomic_open() may be called with the parent directory's i_rwsem held
  * shared even when O_CREAT is set.  A filesystem setting this accepts that,
@@ -2066,7 +2099,11 @@ struct inode_operations {
  *    serialized against all of them.  It runs concurrently with other
  *    O_CREAT opens, with an ->atomic_open() for the same or another name
  *    without O_CREAT, and with ->lookup() and ->iterate_shared(), which
- *    already run with i_rwsem held shared.
+ *    already run with i_rwsem held shared.  A filesystem that also
+ *    provides ->unlink_shared() adds the removal of a non-directory to
+ *    that list, the same name included: it orders the two itself, and
+ *    ->unlink_shared_done() is what keeps the order of their
+ *    notifications for one name.  See enum unlink_shared_returns above.
  *
  * Ignored unless the filesystem also provides ->atomic_open, and ignored on
  * a superblock with SB_I_NAMES_COLLIDE set in ->s_iflags: there the in-lookup
-- 
2.43.0

[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@hammerspace.com>
---
 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

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

[Jonathan Flynn]

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

[PATCH RFC 09/12] nfs: do not cache a name from a listing that a removal overlapped

[Jonathan Flynn]

With a removal holding the directory lock shared, a directory listing,
which holds it shared too, can run beside it, and the server can build
the listing before the removal while the client reads it after.  The
client would then put the removed name back in its cache as a valid
entry, and keep it for as long as it trusts its cached view of the
directory.

Count, per directory, the removals made with the directory locked shared:
those waiting for the server's answer, and those that have had it.  A
listing notes the second count before it is requested, and adds no new
name to the cache while a removal is waiting or once one has been
answered since.  Positive entries already in the cache are covered by
"nfs: keep the dentry of a shared-parent removal hashed".

Nothing counts a removal yet, so nothing changes.  NFSv2 and NFSv3
listings note the count too, and NFSv3 listings that return attributes
make the same check; the behavior of both is unchanged.

Signed-off-by: Jonathan Flynn <jonathan.flynn@hammerspace.com>
---
 fs/nfs/dir.c           | 16 +++++++++--
 fs/nfs/inode.c         |  2 ++
 fs/nfs/internal.h      | 63 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/nfs_fs.h |  8 ++++++
 4 files changed, 87 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 2a4c059064..d9ba3cb1d7 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -165,6 +165,7 @@ struct nfs_readdir_descriptor {
 
 	__be32		verf[NFS_DIR_VERIFIER_SIZE];
 	unsigned long	dir_verifier;
+	unsigned long	shared_removal_seq;
 	unsigned long	timestamp;
 	unsigned long	gencount;
 	unsigned long	attr_gencount;
@@ -603,6 +604,7 @@ static int nfs_readdir_xdr_filler(struct nfs_readdir_descriptor *desc,
 	timestamp = jiffies;
 	gencount = nfs_inc_attr_generation_counter();
 	desc->dir_verifier = nfs_save_change_attribute(inode);
+	desc->shared_removal_seq = nfs_dir_shared_removal_seq(inode);
 	error = NFS_PROTO(inode)->readdir(&arg, &res);
 	if (error < 0) {
 		/* We requested READDIRPLUS, but the server doesn't grok it */
@@ -723,7 +725,7 @@ static void nfs_lookup_advise_force_readdirplus(struct inode *dir,
 
 static
 void nfs_prime_dcache(struct dentry *parent, struct nfs_entry *entry,
-		unsigned long dir_verifier)
+		      unsigned long dir_verifier, unsigned long removal_seq)
 {
 	struct qstr filename = QSTR_INIT(entry->name, entry->len);
 	struct dentry *dentry;
@@ -825,6 +827,15 @@ void nfs_prime_dcache(struct dentry *parent, struct nfs_entry *entry,
 		d_lookup_done(dentry);
 		goto out;
 	}
+	/*
+	 * Make no dentry for a name that a removal under a shared parent may
+	 * have taken away after the server built this listing; see
+	 * nfs_dir_shared_removal_since().
+	 */
+	if (nfs_dir_shared_removal_since(d_inode(parent), removal_seq)) {
+		d_lookup_done(dentry);
+		goto out;
+	}
 
 	nfs_set_verifier(dentry, dir_verifier);
 	inode = nfs_fhget(dentry->d_sb, entry->fh, entry->fattr);
@@ -853,7 +864,8 @@ static int nfs_readdir_entry_decode(struct nfs_readdir_descriptor *desc,
 	ret = xdr_decode(desc, entry, stream);
 	if (ret || !desc->plus)
 		return ret;
-	nfs_prime_dcache(file_dentry(desc->file), entry, desc->dir_verifier);
+	nfs_prime_dcache(file_dentry(desc->file), entry, desc->dir_verifier,
+			 desc->shared_removal_seq);
 	return 0;
 }
 
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index e0a2e92952..7885216a48 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -426,6 +426,8 @@ static void nfs_inode_init_dir(struct nfs_inode *nfsi)
 	nfsi->cache_change_attribute = 0;
 	memset(nfsi->cookieverf, 0, sizeof(nfsi->cookieverf));
 	init_rwsem(&nfsi->rmdir_sem);
+	nfsi->shared_removal_seq = 0;
+	nfsi->shared_removals = 0;
 }
 
 /*
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 907ea27ea2..9cd8e5e808 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -152,6 +152,69 @@ static inline void nfs_d_drop_unless_anchored(struct dentry *dentry)
 	spin_unlock(&dentry->d_lock);
 }
 
+/*
+ * readdir holds the directory shared, so a listing can run beside a removal
+ * made under a shared parent, and the server can build the listing before
+ * the REMOVE while the client decodes it after.  Once the removal is done
+ * its anchor no longer stands in the way - the remover's d_delete() leaves
+ * the dentry unhashed or negative - and priming the dcache from such a
+ * listing would put the removed name back, as a positive dentry with a
+ * valid verifier.
+ *
+ * So a remover counts itself in ->shared_removals from before it sends its
+ * REMOVE until the reply is in, and then advances ->shared_removal_seq,
+ * before anything it does can unhash the anchor.  readdir samples the
+ * sequence before it sends the READDIR, and makes no new dentry from the
+ * listing while a removal is counted or once the sequence has moved.  A
+ * listing whose sample is the advanced value was sent after the server had
+ * answered the REMOVE.  The count makes priming stand aside even if the
+ * anchor were to be lost while the REMOVE is in flight.  A dentry that
+ * priming finds hashed is left to the test in nfs_prime_dcache(), which
+ * leaves it alone while it is anchored and once it has been unhashed;
+ * one that the removal left negative is dropped there, and its name
+ * then gets no new dentry from such a listing either.
+ *
+ * Both fields change under the directory's ->i_lock, and the test reads
+ * them under it.  The sample is taken without the lock.
+ */
+static inline unsigned long nfs_dir_shared_removal_seq(struct inode *dir)
+{
+	return READ_ONCE(NFS_I(dir)->shared_removal_seq);
+}
+
+/*
+ * Has a removal under a shared parent been in flight in @dir since @seq was
+ * sampled, or is one in flight now?
+ */
+static inline bool nfs_dir_shared_removal_since(struct inode *dir,
+						unsigned long seq)
+{
+	struct nfs_inode *nfsi = NFS_I(dir);
+	bool since;
+
+	spin_lock(&dir->i_lock);
+	since = nfsi->shared_removals || nfsi->shared_removal_seq != seq;
+	spin_unlock(&dir->i_lock);
+	return since;
+}
+
+static inline void nfs_dir_start_shared_removal(struct inode *dir)
+{
+	spin_lock(&dir->i_lock);
+	NFS_I(dir)->shared_removals++;
+	spin_unlock(&dir->i_lock);
+}
+
+static inline void nfs_dir_end_shared_removal(struct inode *dir)
+{
+	struct nfs_inode *nfsi = NFS_I(dir);
+
+	spin_lock(&dir->i_lock);
+	nfsi->shared_removals--;
+	WRITE_ONCE(nfsi->shared_removal_seq, nfsi->shared_removal_seq + 1);
+	spin_unlock(&dir->i_lock);
+}
+
 static inline fmode_t flags_to_mode(int flags)
 {
 	fmode_t res = (__force fmode_t)flags & FMODE_EXEC;
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index b45952727c..3e86449485 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -205,6 +205,14 @@ struct nfs_inode {
 			/* Readers: in-flight sillydelete RPC calls */
 			/* Writers: rmdir */
 			struct rw_semaphore	rmdir_sem;
+			/*
+			 * Removals made with i_rwsem held shared: how many
+			 * have finished, and how many are waiting for the
+			 * reply to their REMOVE.  Both change under
+			 * ->i_lock; see fs/nfs/internal.h.
+			 */
+			unsigned long	shared_removal_seq;
+			unsigned int	shared_removals;
 		};
 		/* Regular file */
 		struct {
-- 
2.43.0

[PATCH RFC 10/12] nfs: add the removal method for a parent directory locked shared

[Jonathan Flynn]

Add the client's method for removing a file while its directory is
locked shared.  It handles one case: a cached name that nothing else is
using.  It marks the cached entry and sends the REMOVE, which the
listing check of "nfs: do not cache a name from a listing that a removal
overlapped" counts as waiting until the protocol's ->remove() returns,
and as answered after.  A second method, which the VFS calls after any
delete event for the removal, clears the mark, so that a create of the
same name that waited on it emits its create event after that delete
event, not before.  Everything else, such as an open file that would
have to be renamed aside, or an entry that is no longer hashed or no
longer names a file, is handed back untouched and done with the
directory locked exclusive, as today.  The VFS does not call the method
on a server that reports itself case-insensitive or not case-preserving,
and the method hands back any removal that reaches it there.  A warning
fires if the marked entry is found dropped from the cache when the
removal succeeds.

Two new trace events record the attempts made this way and the
hand-backs, so a trace shows whether removals took this path.  Make
block_revalidate() a wrapper around a new __block_revalidate(), which
stores the mark it is given; the wrapper still stores
NFS_FSDATA_BLOCKED, and now warns if it finds NFS_FSDATA_BLOCKED_SHARED.
The existing removal method is otherwise unchanged, and no directory
uses the new one yet, so nothing changes.

Signed-off-by: Jonathan Flynn <jonathan.flynn@hammerspace.com>
---
 fs/nfs/dir.c           | 183 +++++++++++++++++++++++++++++++++++++++--
 fs/nfs/internal.h      |  19 +++--
 fs/nfs/nfstrace.h      |   2 +
 include/linux/nfs_fs.h |  11 ++-
 4 files changed, 196 insertions(+), 19 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index d9ba3cb1d7..0b6de87030 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1919,12 +1919,12 @@ static int nfs_lookup_revalidate(struct inode *dir, const struct qstr *name,
 	return nfs_do_lookup_revalidate(dir, name, dentry, flags);
 }
 
-static void block_revalidate(struct dentry *dentry)
+static void __block_revalidate(struct dentry *dentry, void *sentinel)
 {
 	/* old devname - just in case */
 	kfree(dentry->d_fsdata);
 
-	dentry->d_fsdata = NFS_FSDATA_BLOCKED;
+	dentry->d_fsdata = sentinel;
 
 	/* Any new reference that could lead to an open
 	 * will either:
@@ -1933,14 +1933,13 @@ static void block_revalidate(struct dentry *dentry)
 	 *    after taking the reference.
 	 *
 	 * Holding this lock and invalidating ->d_seq *after* recording
-	 * NFS_FSDATA_BLOCKED ensures we cannot race with
-	 * __nfs_lookup_revalidate().
-	 * If __nfs_lookup_revalidate() doesn't see NFS_FSDATA_BLOCKED then:
+	 * @sentinel ensures we cannot race with __nfs_lookup_revalidate().
+	 * If __nfs_lookup_revalidate() doesn't see @sentinel then:
 	 * In the ref-walk case - d_lookup() will have incremented
 	 * d_count() before we check it.
 	 * In the RCU-walk case - ->d_seq must have been sampled before
 	 * we call write_seqcount_invalidate() below (because it was sampled
-	 * before we set NFS_FSDATA_BLOCKED) so d_count() will have been
+	 * before we set @sentinel) so d_count() will have been
 	 * incremented or the d_seq check will fail in legitimize_path()
 	 * (or both).
 	 */
@@ -1949,6 +1948,16 @@ static void block_revalidate(struct dentry *dentry)
 
 }
 
+static void block_revalidate(struct dentry *dentry)
+{
+	/*
+	 * The caller holds the parent's i_rwsem exclusive, and a shared
+	 * remover's sentinel does not outlive its shared hold of that lock.
+	 */
+	WARN_ON_ONCE(nfs_fsdata_blocked_shared(dentry->d_fsdata));
+	__block_revalidate(dentry, NFS_FSDATA_BLOCKED);
+}
+
 static void unblock_revalidate(struct dentry *dentry)
 {
 	store_release_wake_up(&dentry->d_fsdata, NULL);
@@ -2689,6 +2698,168 @@ int nfs_unlink(struct inode *dir, struct dentry *dentry)
 }
 EXPORT_SYMBOL_GPL(nfs_unlink);
 
+/*
+ * ->unlink_shared(): remove a file with the parent's i_rwsem held shared.
+ *
+ * The lock then excludes no lookup of this name, and no open of it but
+ * one that takes the parent exclusive.  What orders those behind the
+ * REMOVE is the dentry.  While it is hashed it is what every resolver of
+ * the name is handed, and NFS_FSDATA_BLOCKED_SHARED in ->d_fsdata makes
+ * each one that consults ->d_revalidate() after it is stored wait there
+ * until the removal is over: the REMOVE, and the VFS's report of it (see
+ * nfs_unlink_shared_done()).  That works only for a dentry in the one
+ * clean state tested below.  Anything else is handed back with
+ * UNLINK_NEED_EXCL and is done by nfs_unlink() with the parent held
+ * exclusive, as it always has been.  Nothing here silly-renames, and
+ * nothing here waits for another removal.
+ *
+ * An RCU-walk that consulted ->d_revalidate() before the store does not
+ * get past it: __block_revalidate() invalidates ->d_seq after the store,
+ * in the same ->d_lock section, so the walk fails its recheck and waits
+ * in ref-walk.  The one resolver that does get past, as it gets past the
+ * exclusive lock, is set out below.
+ *
+ * NFSv4 sets IOPF_ATOMIC_OPEN_CREATE_SHARED, so an O_CREAT open of the
+ * name takes the parent shared too, and the lock does not exclude it
+ * either.  While the dentry is hashed, such an open is handed it like any
+ * other resolver, and waits as they do.
+ *
+ * The sentinel is not cleared here.  nfs_unlink_shared_done() clears it, and
+ * the VFS calls that after any event for the removal is out, so that an open
+ * released by the clear cannot create the name and have its create event out
+ * before the delete event of the removal it waited for.
+ */
+int nfs_unlink_shared(struct inode *dir, struct dentry *dentry)
+{
+	int error;
+
+	dfprintk(VFS, "NFS: unlink_shared(%s/%llu, %pd)\n", dir->i_sb->s_id,
+		 dir->i_ino, dentry);
+
+	lockdep_assert_held_read(&dir->i_rwsem);
+	trace_nfs_unlink_shared_enter(dir, dentry);
+
+	/*
+	 * The anchor is keyed on the dcache name, and NFS installs no
+	 * ->d_hash or ->d_compare, so names are compared byte for byte: the
+	 * anchor assumes that the server keeps one object per byte string.
+	 * On a server for which "FOO" and "foo" are one object the client
+	 * can hold two independent hashed dentries, and blocking one of them
+	 * makes no resolver of the other wait.  A server that reports itself
+	 * case-insensitive, or as not preserving case, has SB_I_NAMES_COLLIDE
+	 * on its superblock (nfs_server_publish_name_caps()), and the VFS does
+	 * not call this method there; the test here hands back a removal whose
+	 * caller read the bit before it was set.  A server that makes one
+	 * object of other names that differ as byte strings, as Unicode
+	 * normalization does, is not detected, and a removal there is not
+	 * ordered against an open of another spelling; that is a limitation
+	 * of this method.
+	 */
+	if (READ_ONCE(dir->i_sb->s_iflags) & SB_I_NAMES_COLLIDE)
+		goto need_excl;
+
+	spin_lock(&dentry->d_lock);
+	/*
+	 * The clean state, read in the ->d_lock section the sentinel is
+	 * stored in, so that nothing can come between the two.
+	 *
+	 * A count of one is the interlock everything else rests on.  A second
+	 * reference either calls for a silly-rename, which ends in d_move()
+	 * and needs the parent exclusive, or belongs to a resolver that may
+	 * be past its ->d_revalidate() consult and would then unhash the
+	 * dentry without having seen the sentinel.  Every resolver that can
+	 * do that takes its reference before it consults, so with a count of
+	 * one here every resolver that consults from now on finds the
+	 * sentinel and waits.  The readdir guard in nfs_prime_dcache()
+	 * depends on this test and the store staying in one ->d_lock
+	 * section.
+	 *
+	 * One resolver escapes, as it escapes the exclusive lock: an open by
+	 * file handle, whose reference comes from d_obtain_alias() with no
+	 * ->d_revalidate().  It may go on to fail its open, reaching
+	 * nfs_d_drop_unless_anchored(), to open the file that the REMOVE then
+	 * deletes, or to get -ESTALE from the server.  An RCU-walk that
+	 * consulted ->d_revalidate() before the store does not escape; see
+	 * above.
+	 *
+	 * A victim that is no longer hashed and positive is no anchor.
+	 * Nothing between the lookup and here re-tests either, and with the
+	 * parent shared a failed ->d_revalidate() in another lookup, or
+	 * another remover's d_delete(), may have unhashed it since.  That is
+	 * not -ENOENT: a name invalidated locally may still exist on the
+	 * server.  A pending silly-delete on the dentry, or a block already
+	 * present, is for the exclusive pass to answer as well, and both
+	 * tests have to stay ahead of the store: __block_revalidate()
+	 * kfree()s ->d_fsdata, which while DCACHE_NFSFS_RENAMED is set is
+	 * the queued struct nfs_unlinkdata, and a store over another
+	 * sentinel would end that block early.
+	 */
+	if (d_count(dentry) > 1 ||
+	    (dentry->d_flags & DCACHE_NFSFS_RENAMED) ||
+	    nfs_dentry_is_blocked(dentry) ||
+	    d_unhashed(dentry) || d_really_is_negative(dentry)) {
+		spin_unlock(&dentry->d_lock);
+		goto need_excl;
+	}
+	__block_revalidate(dentry, NFS_FSDATA_BLOCKED_SHARED);
+	spin_unlock(&dentry->d_lock);
+
+	trace_nfs_unlink_enter(dir, dentry);
+	/*
+	 * The REMOVE of nfs_safe_remove(), less its silly-renamed case, which
+	 * the tests above hand back; the protocol's ->remove() drops the link
+	 * count itself.  It is copied and not called so that the directory's
+	 * removal sequence moves as soon as the server has answered, whatever
+	 * the answer, and before anything here can unhash the anchor: on
+	 * -ENOENT, nfs_dentry_handle_enoent() does.  See
+	 * nfs_dir_shared_removal_since().
+	 */
+	trace_nfs_remove_enter(dir, dentry);
+	nfs_dir_start_shared_removal(dir);
+	error = NFS_PROTO(dir)->remove(dir, dentry);
+	nfs_dir_end_shared_removal(dir);
+	/* Nothing may unhash an anchor while its REMOVE is in flight */
+	WARN_ON_ONCE(!error && d_unhashed(dentry));
+	if (error == -ENOENT)
+		nfs_dentry_handle_enoent(dentry);
+	trace_nfs_remove_exit(dir, dentry, error);
+
+	nfs_dentry_remove_handle_error(dir, dentry, error);
+	trace_nfs_unlink_exit(dir, dentry, error);
+	return error;
+
+need_excl:
+	trace_nfs_unlink_need_excl(dir, dentry);
+	return UNLINK_NEED_EXCL;
+}
+EXPORT_SYMBOL_GPL(nfs_unlink_shared);
+
+/*
+ * ->unlink_shared_done(): end the exclusion nfs_unlink_shared() installed.
+ *
+ * The VFS calls this for a call of nfs_unlink_shared() that did not hand the
+ * removal back, with the parent still held shared and the event for the
+ * removal, if there was one, already out.  Clearing the sentinel here rather
+ * than at the end of nfs_unlink_shared() is what keeps the notifications for
+ * the name in order: an O_CREAT open released by the clear would otherwise be
+ * able to create the name and have its create event out first.
+ *
+ * Every such call stored the sentinel, both of the tests that hand a removal
+ * back being made before the store, so there is nothing to test for here.
+ * Nor can anything have replaced it: nfs_unlink_shared() anchors only on a
+ * dentry with no reference but the caller's, which the caller still holds, so
+ * no second remover can anchor on it and nfs_d_release() cannot run;
+ * nfs_call_unlink() skips a dentry that is blocked; and every other writer of
+ * the field needs either the parent held exclusive or DCACHE_NFSFS_RENAMED,
+ * which is one of the cases handed back above.
+ */
+void nfs_unlink_shared_done(struct inode *dir, struct dentry *dentry)
+{
+	lockdep_assert_held_read(&dir->i_rwsem);
+	unblock_revalidate(dentry);
+}
+EXPORT_SYMBOL_GPL(nfs_unlink_shared_done);
+
 /*
  * To create a symbolic link, most file systems instantiate a new inode,
  * add a page to it containing the path, then write it out to the disk
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 9cd8e5e808..2600ae7865 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -49,7 +49,7 @@ static inline bool nfs_lookup_is_soft_revalidate(const struct dentry *dentry)
  * 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()).
+ * it (see nfs_unlink_shared_done()).
  * ->d_revalidate() waits for either to clear.  The two differ in what the
  * dentry is to the removal.
  *
@@ -73,12 +73,15 @@ static inline bool nfs_lookup_is_soft_revalidate(const struct dentry *dentry)
  * ->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.
+ * The shared sentinel is stored in nfs_unlink_shared() and cleared in
+ * nfs_unlink_shared_done(), which the VFS calls while it still holds the
+ * parent shared, so it is stored and cleared within one shared hold of the
+ * parent and is never present while the parent is held exclusive.  That
+ * is why the WARN_ON()s in nfs_unlink() and nfs_rename(), made with the
+ * parent exclusive, still compare against NFS_FSDATA_BLOCKED alone, and
+ * why block_revalidate(), which both call, warns if it finds
+ * NFS_FSDATA_BLOCKED_SHARED.  Those WARN_ON()s are the only bare compares
+ * against a sentinel; every other test uses the helpers below.
  */
 static inline bool nfs_fsdata_blocked(const void *fsdata)
 {
@@ -611,6 +614,8 @@ struct dentry *nfs_mkdir(const struct mnt_idmap *, struct inode *, struct dentry
 			 umode_t);
 int nfs_rmdir(struct inode *, struct dentry *);
 int nfs_unlink(struct inode *, struct dentry *);
+int nfs_unlink_shared(struct inode *dir, struct dentry *dentry);
+void nfs_unlink_shared_done(struct inode *dir, struct dentry *dentry);
 int nfs_symlink(const struct mnt_idmap *, struct inode *, struct dentry *,
 		const char *);
 int nfs_link(struct dentry *, struct inode *, struct dentry *);
diff --git a/fs/nfs/nfstrace.h b/fs/nfs/nfstrace.h
index a32c76df72..99393ebc7e 100644
--- a/fs/nfs/nfstrace.h
+++ b/fs/nfs/nfstrace.h
@@ -748,6 +748,8 @@ DEFINE_NFS_DIRECTORY_EVENT(nfs_remove_enter);
 DEFINE_NFS_DIRECTORY_EVENT_DONE(nfs_remove_exit);
 DEFINE_NFS_DIRECTORY_EVENT(nfs_unlink_enter);
 DEFINE_NFS_DIRECTORY_EVENT_DONE(nfs_unlink_exit);
+DEFINE_NFS_DIRECTORY_EVENT(nfs_unlink_shared_enter);
+DEFINE_NFS_DIRECTORY_EVENT(nfs_unlink_need_excl);
 DEFINE_NFS_DIRECTORY_EVENT(nfs_symlink_enter);
 DEFINE_NFS_DIRECTORY_EVENT_DONE(nfs_symlink_exit);
 
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 3e86449485..7ccdc5b867 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -697,12 +697,11 @@ static inline bool nfs_ooo_test(struct nfs_inode *nfsi)
  *  - 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.
+ *  - NFS_FSDATA_BLOCKED_SHARED, from the store in nfs_unlink_shared(),
+ *    with the parent's i_rwsem held shared, until nfs_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.
-- 
2.43.0

[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@hammerspace.com>
---
 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

[PATCH RFC 12/12] nfs: remove files with the parent directory locked shared on NFSv4

[Jonathan Flynn]

Let removals of different files in one NFSv4 directory proceed at the
same time, instead of queuing one behind another on the directory's
lock.  The server orders removals of different names.  The client
orders a removal against lookups and opens of the same name through its
cached entry for that name, and against listings through that entry and
a count of such removals kept per directory, as set out in the changes
before this one.  A removal it cannot order that way is done with the
directory locked exclusive, as before.  On a server that reports itself
case-insensitive or not case-preserving, every removal is done that way
from the start.  A server that treats names differing in other ways as
one, as Unicode normalization does, is not detected, and there a
removal is not ordered against a lookup or open of another spelling.

This covers every NFSv4 minor version, NFSv4.0 included: nothing here
depends on sessions.  NFSv2 and NFSv3 are unchanged.

Signed-off-by: Jonathan Flynn <jonathan.flynn@hammerspace.com>
---
 fs/nfs/nfs4proc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index fbf9e97815..0acc03dfe6 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -10860,6 +10860,8 @@ static const struct inode_operations nfs4_dir_inode_operations = {
 	.atomic_open	= nfs_atomic_open,
 	.link		= nfs_link,
 	.unlink		= nfs_unlink,
+	.unlink_shared	= nfs_unlink_shared,
+	.unlink_shared_done = nfs_unlink_shared_done,
 	.symlink	= nfs_symlink,
 	.mkdir		= nfs_mkdir,
 	.rmdir		= nfs_rmdir,
-- 
2.43.0

Re: [PATCH RFC 00/12] vfs, nfs: concurrent create and unlink in one directory

[NeilBrown]

On Fri, 30 Oct 2026, Jonathan Flynn wrote:
> Hi Neil,
> 
> Thank you for this series; it is what makes these patches possible.  I
> had them on your v3 and have rebased them onto v4.  They extend it so
> that processes on one NFSv4 client can create and remove files in one
> directory concurrently.
> 
>   operations per second   parent exclusive   parent shared   ratio
>   creates                            2,616          29,939   11.4x
>   removals                           2,157          32,360   15.0x

Thanks for the stats.  This confirms that this a useful project to
continue on :-)

But I think you are racing ahead and that we do this "properly" without
per-fs flags etc.

Reflecting on me current plans, I see that rmdir and rename require
the most work.  It would make sense (I think) to delay those and
focus on create/unlink which are relatively straight-forward.

Once all the current prep work lands I want to list d_alloc_parallel()
out of i_rwsem and then add dentry locking.  Then we can move 
i_rwsem locking into ->atomic_open and ->create and ->lookup and
->unlink, and then you can remove that locking for NFS.

I think trying to take a short cut would slow the whole process down.
The 'create' changes you made aren't too bad, but the unlink work is
too complex I think.

But I'm really happy that you've written this proof-of-concept.

Thanks,
NeilBrown


> 
> One client, 96 processes in one directory, each creating files (open
> O_CREAT, a small write, close) for a set time, then removing them all;
> NFSv4.2 to a pNFS metadata server that is not knfsd; your v4, the 7.4
> VFS changes it builds on, the three silly-delete fixes and this series
> on a 7.3-based kernel, among other patches.  Each row switched only its
> own path, by a test-only module parameter not in this series, with the
> other path on; one run each.  The creates row comes from 300 s of
> creates.  The removals row comes from 120 s of creates, and each of its
> runs removed about 3.7 million files; the removal rates vary with that
> number, so it is given.  With the parent held exclusive, operations go
> one server round trip at a time.  2/12 quotes an earlier measurement,
> on a 7.1.13-based kernel.
> 
> How it works.  2/12 lets a filesystem have the parent locked shared for
> O_CREAT (a flag in a new ->iop_flags field), and 6/12 for unlink(2)
> (->unlink_shared() and ->unlink_shared_done()).  The dentry then
> excludes the name: for a create, its in-lookup state, all
> nfs_atomic_open() needs since your 3/9; for a removal, an NFS mark that
> lookups, opens and creates of the name wait on until
> ->unlink_shared_done(), which the VFS calls after the fsnotify delete
> event, if any.  What NFS cannot order that way, such as an open file,
> it hands back to ->unlink() under the exclusive lock; where the server
> reports itself case-insensitive or not case-preserving, NFS sets
> SB_I_NAMES_COLLIDE (3/12) and both keep the exclusive lock.  Both paths
> call d_alloc_parallel() under the shared lock, as lookup does.  When
> your directory-locking changes lift d_alloc_parallel() above i_rwsem,
> these calls have to move with lookup's; the exclusion does not change,
> and the opt-ins go once the VFS stops taking the parent lock (2/12,
> 6/12).
> 
> Patch flow:
>   prereq   1  store re-read server capabilities in one step, for 3
>   create   2  VFS opt-in to take the parent lock shared for O_CREAT
>            3  NFS sets SB_I_NAMES_COLLIDE where names can collide
>            4  NFSv4 directories opt in for O_CREAT
>   remove   5  factor vfs_unlink() into a helper, no functional change
>            6  VFS opt-in: ->unlink_shared(), ->unlink_shared_done()
>            7  a second ->d_fsdata mark, for removals under a shared lock
>            8  keep a dentry carrying that mark hashed
>            9  no new dentries from a listing that a removal overlapped
>           10  the removal method, handing back what it cannot order
>           11  look a name up again once its removal has unhashed it
>           12  NFSv4 directories opt in for unlink
> 
> Three fixes for the silly-delete hand-off, which your 2/9 makes
> reachable, are posted separately, in reply to 2/9.
> 
> Tested on the same kernel over NFSv4.2, with the new paths on and off:
> removal races (with listings, and with tasks killed); O_EXCL creates
> racing unlink, with a kernel-side check that no revalidation trusted a
> removed entry; inotify event order; two clients; and an overlayfs mount
> whose lower file another client renamed away and back.  All passed,
> with no kernel warning or hung task, except the two-client test's check
> that nothing was left behind: on this kernel and on one with an earlier
> revision of this series, it found one silly-renamed file in three of
> four runs, with the new paths off as well as on, so they are not the
> cause.  Each time, the file left behind was not the one that had been
> silly-renamed: the silly-rename's RENAME, which goes by name, had moved
> another file of that name onto the hidden name.  A PROVE_LOCKING and
> KASAN build ran the removal, killed-task and O_EXCL tests with no
> report.  A case-insensitive server, where two spellings of a name never
> made two files, was tested on that earlier revision only; that code has
> not changed since.  Not run: fstests, knfsd as the server, NFSv4.0 or
> 4.1, directory delegations (the server grants none), CONFIG_DEBUG_VFS, a
> weakly ordered CPU.
> 
> One question: are the opt-ins in 2/12 and 6/12 acceptable as interim
> interfaces until the directory-locking changes land?
> 
> The series applies to 83f32c773255 in Linus's tree on top of your v4
> and the three silly-delete fixes.  fs/ compiles there without warnings
> at every commit, with and without CONFIG_DEBUG_VFS and lockdep; I have
> run it only on the 7.3-based kernel above.
> 
> Tools: these patches, their changelogs, the race tests and this letter
> were written with Claude Code, using Anthropic's Claude Opus 5 and
> Claude Opus 5.5 models, at my direction over many sessions: I chose
> among the designs it proposed and directed each revision.  It found, by
> reading the code, the bug that 1/12 fixes: while the client reads a
> server's capabilities again, a capability the server still offers can
> look missing.  The three silly-delete fixes, posted separately, say
> which of their problems it found and how they were tested.
> 
> Thanks,
> Jonathan
> 
> Jonathan Flynn (12):
>   nfs: update a server's capabilities in one step
>   vfs: allow the parent lock to be shared for O_CREAT
>   nfs: tell the VFS when two names can be one entry on the server
>   nfs: concurrent O_CREAT in one NFSv4 directory
>   vfs: factor file removal into a helper, no functional change
>   vfs: let a filesystem remove files with the parent directory locked
>     shared
>   nfs: add a second mark for a removal made with the parent locked
>     shared
>   nfs: keep the dentry of a shared-parent removal hashed
>   nfs: do not cache a name from a listing that a removal overlapped
>   nfs: add the removal method for a parent directory locked shared
>   nfs: look a name up again once its removal has unhashed it
>   nfs: remove files with the parent directory locked shared on NFSv4
> 
>  .../filesystems/directory-locking.rst         |  60 +++
>  Documentation/filesystems/locking.rst         | 259 ++++++++++++-
>  Documentation/filesystems/porting.rst         |  35 ++
>  Documentation/filesystems/vfs.rst             |  25 ++
>  fs/namei.c                                    | 360 +++++++++++++++++-
>  fs/nfs/client.c                               |   1 +
>  fs/nfs/dir.c                                  | 300 ++++++++++++++-
>  fs/nfs/inode.c                                |   2 +
>  fs/nfs/internal.h                             | 211 ++++++++++
>  fs/nfs/nfs4file.c                             |   9 +-
>  fs/nfs/nfs4proc.c                             |  35 +-
>  fs/nfs/nfstrace.h                             |   2 +
>  fs/nfs/super.c                                |   2 +
>  fs/nfs/unlink.c                               |  16 +-
>  include/linux/fs.h                            |  72 ++++
>  include/linux/fs/super_types.h                |   1 +
>  include/linux/nfs_fs.h                        |  28 ++
>  17 files changed, 1361 insertions(+), 57 deletions(-)
> 
> 
> base-commit: 83f32c773255720a661aa45d83e36278135f23f5
> prerequisite-patch-id: 3405b4d6ab8b0bf24ba63a898083d617deec4f24
> prerequisite-patch-id: 096657d6d80d7ca533ccb574d2af65611ede4af2
> prerequisite-patch-id: 1a2319c14739918aef86116647fea28ae85ad4df
> prerequisite-patch-id: 725cb18c3936d50141ef27dafc159bf50b64a5bd
> prerequisite-patch-id: b39cdd352b97e2cd247311fd5db620dbff227bd7
> prerequisite-patch-id: f9f3f0e945cadf96113d4ac9ab8f7b0c207af4e5
> prerequisite-patch-id: 4c09a15390bb556d79bea1ee733f52a58bec80ef
> prerequisite-patch-id: 7c026717a97b2c86a169539822f5d14650f68f8a
> prerequisite-patch-id: a702d54495d0f3d7f88b82411d884dcf5aacf9a0
> prerequisite-patch-id: a7e7263e8916612f3e4b767946b5d9259029f50e
> prerequisite-patch-id: a8e5414df9d9912dcdf9bb1dbb2811ce326a1e1f
> prerequisite-patch-id: e8050b998d5790f3bb4311bc65011735ed7d8b2f
> -- 
> 2.43.0
> 
> 

Re: [PATCH RFC 03/12] nfs: tell the VFS when two names can be one entry on the server

[NeilBrown]

On Fri, 30 Oct 2026, Jonathan Flynn wrote:
> A server that reports itself case-insensitive, or as not preserving
> case, can make one entry of two names that the client caches apart,
> because the client compares names byte for byte.  Mark the filesystem
> on such a server with SB_I_NAMES_COLLIDE, from "vfs: allow the parent
> lock to be shared for O_CREAT", so that the VFS keeps the exclusive
> directory lock for creates there: the per-name exclusion that replaces
> the lock for a create made with it held shared covers one byte string,
> not the names the server treats as equal.  The client learns this when
> it reads the server's capabilities, which it does again on remount, on
> migration and when a later mount reuses the filesystem, and it updates
> the mark each time.
> 
> This is a conservative fallback.  A server that declares itself
> case-insensitive should itself treat the two names as one, but one that
> did not was seen making two files of one name when such creates arrived
> together.  No filesystem relies on the mark yet, so nothing changes.

We don't normally work around server bugs in the client.  We tell the
server maintainers to fix their server.

But we *certainly* don't work around them in the VFS with
SB_I_NAMES_COLLIDE.  That sort of fs behaviour might be tolerable for
files but it is a real problem for directories!

Once we move directory locking into the filesystem, the NFS client code
might choose to add extra locking for servers that might be problematic.
It would then be entirely in the NFS code then, and up to the NFS
maintainers to decide what to do.
But this logic does not, I think, belong in the VFS layer at all.

This just confirms me in my preference to continue on the path I have
planned.

Hopefully next week I'll put together an RFC for the next steps, and
we pursue the prep patches that haven't been acked yet.

Thanks,
NeilBrown


> 
> Signed-off-by: Jonathan Flynn <jonathan.flynn@hammerspace.com>
> ---
>  fs/nfs/client.c   |  1 +
>  fs/nfs/internal.h | 35 +++++++++++++++++++++++++++++++++++
>  fs/nfs/nfs4proc.c |  1 +
>  fs/nfs/super.c    |  2 ++
>  4 files changed, 39 insertions(+)
> 
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index ea99b3c105..30206b309c 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -975,6 +975,7 @@ static int nfs_probe_fsinfo(struct nfs_server *server, struct nfs_fh *mntfh, str
>  			server->caps &= ~(NFS_CAP_CASE_INSENSITIVE |
>  					  NFS_CAP_CASE_NONPRESERVING);
>  		}
> +		nfs_server_publish_name_caps(server);
>  	}
>  
>  	if (clp->rpc_ops->discover_trunking != NULL &&
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index 5af6a032f1..34b8aff4e0 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -224,6 +224,41 @@ int nfs_init_server_rpcclient(struct nfs_server *, const struct rpc_timeout *t,
>  struct nfs_server *nfs_alloc_server(void);
>  void nfs_server_copy_userdata(struct nfs_server *, struct nfs_server *);
>  
> +/*
> + * Tell the VFS whether this server can make one directory entry of two names
> + * that differ as byte strings.  NFS installs no ->d_hash or ->d_compare, so
> + * the dcache compares names byte for byte, while a server that is
> + * case-insensitive, or does not preserve case, makes "foo" and "FOO" one
> + * entry.  On such a server SB_I_NAMES_COLLIDE keeps the parent of an O_CREAT
> + * open locked exclusive; see atomic_open_create_shared() in fs/namei.c.
> + *
> + * server->caps is derived again on every probe of the server: at mount, at
> + * remount, on migration, and when a later mount reuses this superblock
> + * (nfs4_proc_get_root()).  So this is called wherever the case bits of
> + * server->caps are written, once they have their final value, and when the
> + * superblock is set up, because the first probe runs before it exists.  The
> + * value is what the probed file handle reported, normally the mount root's;
> + * the homogeneous attribute is not fetched, so a server whose answer differs
> + * below the root is described by the root.  Migration runs the probe from
> + * the state manager without ->s_umount, so the update is atomic, and it is
> + * made only when the value changes.  Two probes of one server at once can
> + * leave the bit as the earlier of them derived it; that needs the server's
> + * answer to change between them, and server->caps has the same race.
> + */
> +static inline void nfs_server_publish_name_caps(struct nfs_server *server)
> +{
> +	struct super_block *sb = server->super;
> +	unsigned long bits = 0;
> +
> +	if (!sb)
> +		return;
> +	if (READ_ONCE(server->caps) & (NFS_CAP_CASE_INSENSITIVE |
> +				       NFS_CAP_CASE_NONPRESERVING))
> +		bits = SB_I_NAMES_COLLIDE;
> +	if ((READ_ONCE(sb->s_iflags) & SB_I_NAMES_COLLIDE) != bits)
> +		set_mask_bits(&sb->s_iflags, SB_I_NAMES_COLLIDE, bits);
> +}
> +
>  extern void nfs_put_client(struct nfs_client *);
>  extern void nfs_free_client(struct nfs_client *);
>  void nfs_cb_idr_remove(struct nfs_client *clp);
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 113c199a6f..fac8450b65 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -4020,6 +4020,7 @@ static int _nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh *f
>  		if (nfs4_server_delegtime_capable(&res))
>  			caps |= NFS_CAP_DELEGTIME;
>  		WRITE_ONCE(server->caps, caps);
> +		nfs_server_publish_name_caps(server);
>  
>  		memcpy(server->cache_consistency_bitmask, res.attr_bitmask, sizeof(server->cache_consistency_bitmask));
>  		server->cache_consistency_bitmask[0] &= FATTR4_WORD0_CHANGE|FATTR4_WORD0_SIZE;
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 23292680ad..e70cef88fb 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -1082,6 +1082,8 @@ static void nfs_fill_super(struct super_block *sb, struct nfs_fs_context *ctx)
>  			sb->s_export_op = &nfs_export_ops;
>  		break;
>  	}
> +	/* The first probe of the caps ran before this superblock existed */
> +	nfs_server_publish_name_caps(server);
>  
>  	sb->s_magic = NFS_SUPER_MAGIC;
>  
> -- 
> 2.43.0
> 
>