[PATCH v3 13/19] nfs: remove d_drop()/d_alloc_parallel() from nfs_atomic_open()

NeilBrown
2026-04-27T04:07:53Z

It is important that two non-create NFS "open"s of a negative dentry
don't race. They both have only a shared lock on i_rwsem and so could
run concurrently, but they might both try to call d_splice_alias() at
the same time which is confusing at best.

nfs_atomic_open() currently avoids this by discarding the negative
dentry and creating a new one using d_alloc_parallel(). Only one thread
can successfully get the d_in_lookup() dentry, the other will wait for
the first to finish, and can use the result of that first lookup.

A proposed locking change inverts the order between i_rwsem and
d_alloc_parallel() so it will not be safe to call d_alloc_parallel()
while holding i_rwsem - even shared.

We can achieve the same effect by causing ->d_revalidate to invalidate a
negative dentry when LOOKUP_OPEN is set. Doing this is consistent with
the "close to open" caching semantics of NFS which requires the server
to be queried whenever opening a file - cached information must not be
trusted.

With this change to ->d_revaliate (implemented in nfs_neg_need_reval) we
can be sure that we have exclusive access to any dentry that reaches
nfs_atomic_open(). Either O_CREAT was requested and so the parent is
locked exclusively, or the dentry will have DCACHE_PAR_LOOKUP set.

This means that the d_drop() and d_alloc_parallel() calls in
nfs_atomic_lookup() are no longer needed to provide exclusion

Signed-off-by: NeilBrown <neil@brown.name>
---
fs/nfs/dir.c | 30 +++++++-----------------------
1 file changed, 7 insertions(+), 23 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 9580af999d70..0791fc2d161b 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1656,6 +1656,13 @@ int nfs_neg_need_reval(struct inode *dir, struct dentry *dentry,
{
if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET))
return 0;
+ if (flags & LOOKUP_OPEN)
+ /* close-to-open semantics require we go to server
+ * on each open. By invalidating the dentry we
+ * also ensure nfs_atomic_open() always has exclusive
+ * access to the dentry.
+ */
+ return 0;
if (NFS_SERVER(dir)->flags & NFS_MOUNT_LOOKUP_CACHE_NONEG)
return 1;
/* Case insensitive server? Revalidate negative dentries */
@@ -2111,7 +2118,6 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
struct inode *inode;
unsigned int lookup_flags = 0;
unsigned long dir_verifier;
- bool switched = false;
int created = 0;
int err;

@@ -2156,17 +2162,6 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
attr.ia_size = 0;
}

- if (!(open_flags & O_CREAT) && !d_in_lookup(dentry)) {
- d_drop(dentry);
- switched = true;
- dentry = d_alloc_parallel(dentry->d_parent,
- &dentry->d_name);
- if (IS_ERR(dentry))
- return PTR_ERR(dentry);
- if (unlikely(!d_in_lookup(dentry)))
- return finish_no_open(file, dentry);
- }
-
ctx = create_nfs_open_context(dentry, open_flags, file);
err = PTR_ERR(ctx);
if (IS_ERR(ctx))
@@ -2209,10 +2204,6 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
trace_nfs_atomic_open_exit(dir, ctx, open_flags, err);
put_nfs_open_context(ctx);
out:
- if (unlikely(switched)) {
- d_lookup_done(dentry);
- dput(dentry);
- }
return err;

no_open:
@@ -2235,13 +2226,6 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
res = ERR_PTR(-EOPENSTALE);
}
}
- if (switched) {
- d_lookup_done(dentry);
- if (!res)
- res = dentry;
- else
- dput(dentry);
- }
return finish_no_open(file, res);
}
EXPORT_SYMBOL_GPL(nfs_atomic_open);
--
2.50.0.107.gf914562f5916.dirty