[PATCH v2 0/9] nfs: prepare for VFS locking changes.

[PATCH v2 0/9] nfs: prepare for VFS locking changes.

[NeilBrown]

sashiko reported some problem with the previous posting.  A lot
were false positives as it used the wrong base.  However:
3/9 had inverted the return code from nfs_neg_need_reval() - it returns
    1 for "must revalidate" unlike d_revalidate which returns 0.
8/9 didn't handle and error from nfs_lookup() correctly - it
    should be fatal rather than triggering a retry.

Original intro below.
Thanks,
NeilBrown

I am working on changes to directory locking.  The medium term goal is
to lift d_alloc_parallel() locking above i_rwsem, so d_alloc_paralle()
can be run unlocked, and consequently cannot be called when ->i_rwsem is
held.

The requires:
 1/ the d_alloc_parallel() not be called while i_rwsem is held.  NFS does
    this in a few places. New interfaces d_alloc_trylock() and d_duplicate()
    are now available and can be used instead.

 2/ a dentry must not be d_drop()ed while an operation is ongoing, as an
    unlocked d_alloc_parallel() would then be able to create a new
    dentry with the same name.  In particular d_rehash() must be phased
    out as it won't be safe.  Various techiniques are used to avoid the
    need for d_drop(), particularly that d_splice_alias() is now able to
    splice a hashed alias.

Further, the first two patches here fixes bugs that I found while
developing and reviewing these changes.

The patches depend on patches recently added to the vfs tree.  I would
prefer these land in that tree too with an Acked-by or similar from NFS
maintainers.

Thanks,
NeilBrown

 [PATCH v2 1/9] nfs: fix open-blocking with d_fsdata
 [PATCH v2 2/9] nfs: fix nfs_call_unlink()
 [PATCH v2 3/9] nfs: remove d_drop()/d_alloc_parallel() from
 [PATCH v2 4/9] nfs: use d_splice_alias() in nfs_link()
 [PATCH v2 5/9] nfs: don't d_drop() before d_splice_alias()
 [PATCH v2 6/9] nfs: don't d_drop() before d_splice_alias() in
 [PATCH v2 7/9] nfs: Use d_alloc_trylock() in nfs_prime_dcache()
 [PATCH v2 8/9] nfs: use d_alloc_trylock() in silly-rename
 [PATCH v2 9/9] nfs: use d_duplicate()

[PATCH v2 1/9] nfs: fix open-blocking with d_fsdata

[NeilBrown]

From: NeilBrown <neil@brown.name>

The logic described in block_revalidate() that ->d_lock must be taken in
lookup_open() missed the fact that sometimes lookup_open() isn't called.

If open_last_lookups() is called while still in RCU-walk and
lookup_fast() finds a dentry, it does not take ->d_lock or increment
d_count() before calling d_revalidate() - only afterwards.

So on that path d_count() can be incremented and the file opened *after*
nfs_unlink() has checked d_count() and set NFS_FSDATA_BLOCKED.  So an
application could have the file open when it is unlinked on the server.

To fix this we need block_revalidate() to ensure the RCU-walk path
is blocked as well. This is easily done with
  write_seqcount_invalidate(&dentry->d_seq);

legitimize_path() will increment d_count() and then check d_seq.
The write_seqcount_invalidate() call with cause that check to fail if
nfs_unlink() has already seen d_count() being 1.  So the RCU-walk will
fail with -ECHILD and it will be retried as a ref-walk.

Also the smp_load_acquire() in __nfs_lookup_revalidate() is pointless.
wait_var_event() contains the required barriers (a spinlock in
prepare_to_wait_event()) to ensure the correct value is read after any
wakeup.

That barrier doesn't apply for the very first test of the condition in
wait_var_event) but the locking of ->d_lock described above provides
sufficient barriers.

Fixes: 99bc9f2eb3f7 ("NFS: add barriers when testing for NFS_FSDATA_BLOCKED")
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfs/dir.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 49394123bd09..4f1bf45b0c70 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1837,8 +1837,7 @@ __nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 	} else {
 		/* Wait for unlink to complete - see unblock_revalidate() */
 		wait_var_event(&dentry->d_fsdata,
-			       smp_load_acquire(&dentry->d_fsdata)
-			       != NFS_FSDATA_BLOCKED);
+			       dentry->d_fsdata != NFS_FSDATA_BLOCKED);
 	}
 	return 0;
 }
@@ -1857,12 +1856,15 @@ static void block_revalidate(struct dentry *dentry)
 	kfree(dentry->d_fsdata);
 
 	/* Any new reference that could lead to an open
-	 * will take ->d_lock in lookup_open() -> d_lookup().
-	 * Holding this lock ensures we cannot race with
-	 * __nfs_lookup_revalidate() and removes and need
-	 * for further barriers.
+	 * will either:
+	 *  - take ->d_lock in lookup_open() -> d_lookup() or
+	 *  - will check d_seq in legitimize_mnt()
+	 *
+	 * Holding this lock and invalidating ->d_seq ensures we cannot
+	 * race with __nfs_lookup_revalidate().
 	 */
 	lockdep_assert_held(&dentry->d_lock);
+	write_seqcount_invalidate(&dentry->d_seq);
 
 	dentry->d_fsdata = NFS_FSDATA_BLOCKED;
 }

base-commit: 3879f51857325da9bf3cfb073280257cd16ae067
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 2/9] nfs: fix nfs_call_unlink()

[NeilBrown]

From: NeilBrown <neil@brown.name>

nfs_call_unlink() calls d_alloc_parallel() on a dentry which has not had
the d_hash calculated.  d_alloc_parallel() assume this is calculated and
doesn't calculate it itself.
So this will never find an existing dentry and so will not detect
the races it aims to detect.

This dentry ("alias") is never hashed so there is no lasting
inconsistency in the dcache.

Fixes: 565277f63c61 ("NFS: Fix a race in sillyrename")
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfs/unlink.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
index c8d712204e64..11a46a993e11 100644
--- a/fs/nfs/unlink.c
+++ b/fs/nfs/unlink.c
@@ -125,6 +125,9 @@ static int nfs_call_unlink(struct dentry *dentry, struct inode *inode, struct nf
 	struct dentry *alias;
 
 	down_read_non_owner(&NFS_I(dir)->rmdir_sem);
+	data->args.name.hash = full_name_hash(dentry->d_parent,
+					      data->args.name.name,
+					      data->args.name.len);
 	alias = d_alloc_parallel(dentry->d_parent, &data->args.name);
 	if (IS_ERR(alias)) {
 		up_read_non_owner(&NFS_I(dir)->rmdir_sem);
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 3/9] nfs: remove d_drop()/d_alloc_parallel() from nfs_atomic_open()

[NeilBrown]

From: NeilBrown <neil@brown.name>

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.
[Note that when nfs_neg_need_reval() returns 1, ->d_revalidate returns 0].

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

There is still a d_drop() which allowed d_splice_alias() to succeed.
This will be removed in a future patch.

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 4f1bf45b0c70..f219cc1a5fc5 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 the server
+		 * on each open.  By invalidating the dentry we
+		 * also ensure nfs_atomic_open() always has exclusive
+		 * access to the dentry.
+		 */
+		return 1;
 	if (NFS_SERVER(dir)->flags & NFS_MOUNT_LOOKUP_CACHE_NONEG)
 		return 1;
 	/* Case insensitive server? Revalidate negative dentries */
@@ -2113,7 +2120,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;
 
@@ -2158,17 +2164,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))
@@ -2219,10 +2214,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:
@@ -2245,13 +2236,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

[PATCH v2 4/9] nfs: use d_splice_alias() in nfs_link()

[NeilBrown]

From: NeilBrown <neil@brown.name>

When filename_linkat() calls filename_create() which ultimately
calls ->lookup, the flags LOOKUP_CREATE|LOOKUP_EXCL are passed.
nfs_lookup() treats this as an exclusive create (which it is) and
skips the ->lookup, leaving the dentry unchanged.

Currently that means nfs_link() can get a hashed dentry (if the name was
already in the cache) or an unhashed dentry (if it wasn't). As none of
d_add(), d_instantiate(), d_splice_alias() could handle both of these,
nfs_link() calls d_drop() and then d_add().

Recent changes to d_splice_alias() mean that it *can* work with either
hashed or unhashed dentries.  Future changes to locking mean that it
will be unsafe to d_drop() a dentry while an operation (in this case
"link()") is still ongoing.

So change to use d_splice_alias(), and not to d_drop() until an error is
detected (as in that case was can't be sure what is actually on the server).

Also update the comment for nfs_is_exclusive_create() to note that
link(), mkdir(), mknod(), symlink() all appear as exclusive creates.
Those other than link() already used d_splice_alias() via
nfs_add_or_obtain().

Possibly nfs_is_exclusive_create() should return true even with NFSv2
when the request isn't an open (not LOOKUP_OPEN).

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

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index f219cc1a5fc5..b7cb14310bcf 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1570,6 +1570,9 @@ static int nfs_check_verifier(struct inode *dir, struct dentry *dentry,
 /*
  * Use intent information to check whether or not we're going to do
  * an O_EXCL create using this path component.
+ * Note that link(), mkdir(), mknod(), symlink() all appear as
+ * exclusive creation.  Regular file creation could be distinguished
+ * with LOOKUP_OPEN.
  */
 static int nfs_is_exclusive_create(struct inode *dir, unsigned int flags)
 {
@@ -2699,14 +2702,15 @@ nfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *dentry)
 		old_dentry, dentry);
 
 	trace_nfs_link_enter(inode, dir, dentry);
-	d_drop(dentry);
 	if (S_ISREG(inode->i_mode))
 		nfs_sync_inode(inode);
 	error = NFS_PROTO(dir)->link(inode, dir, &dentry->d_name);
 	if (error == 0) {
 		nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
 		ihold(inode);
-		d_add(dentry, inode);
+		d_splice_alias(inode, dentry);
+	} else {
+		d_drop(dentry);
 	}
 	trace_nfs_link_exit(inode, dir, dentry, error);
 	return error;
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 5/9] nfs: don't d_drop() before d_splice_alias()

[NeilBrown]

From: NeilBrown <neil@brown.name>

nfs_add_or_obtain() is used, often via nfs_instantiate(), to attach a
newly created inode to the appropriate dentry - or to provide an
alternate dentry.
It has to drop the dentry first, which is problematic for proposed
locking changes.

As d_splice_alias() now works with hashed dentries, the d_drop() is no
longer needed.

However we still d_drop() on error as the status of the name is
uncertain.

_nfs4_open_and_get_state() is only used for files so we should be able to
use d_instantiate().  However as that depends on the server for
correctness, it is safer to stay with the current code pattern and use
d_splice_alias() there too.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfs/dir.c      | 3 +--
 fs/nfs/nfs4proc.c | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index b7cb14310bcf..a0d22445dd06 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -2346,8 +2346,6 @@ nfs_add_or_obtain(struct dentry *dentry, struct nfs_fh *fhandle,
 	struct dentry *d;
 	int error;
 
-	d_drop(dentry);
-
 	if (fhandle->size == 0) {
 		error = NFS_PROTO(dir)->lookup(dir, dentry, &dentry->d_name,
 					       fhandle, fattr);
@@ -2368,6 +2366,7 @@ nfs_add_or_obtain(struct dentry *dentry, struct nfs_fh *fhandle,
 	dput(parent);
 	return d;
 out_error:
+	d_drop(dentry);
 	d = ERR_PTR(error);
 	goto out;
 }
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 04b1987115d5..8f07f34f899e 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3113,7 +3113,6 @@ static int _nfs4_open_and_get_state(struct nfs4_opendata *opendata,
 	nfs_set_verifier(dentry, dir_verifier);
 	if (d_really_is_negative(dentry)) {
 		struct dentry *alias;
-		d_drop(dentry);
 		alias = d_splice_alias(igrab(state->inode), dentry);
 		/* d_splice_alias() can't fail here - it's a non-directory */
 		if (alias) {
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 6/9] nfs: don't d_drop() before d_splice_alias() in atomic_create.

[NeilBrown]

From: NeilBrown <neil@brown.name>

When atomic_create fails with -ENOENT we currently d_drop() the dentry
and then re-add it (d_splice_alias()) with a NULL inode.
This drop-and-re-add will not work with proposed locking changes.

As d_splice_alias() now supports hashed dentries, we don't need the
d_drop() until it is determined that some other error has occurred.

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

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index a0d22445dd06..04f6aee65446 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -2180,7 +2180,6 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
 		err = PTR_ERR(inode);
 		trace_nfs_atomic_open_exit(dir, ctx, open_flags, err);
 		put_nfs_open_context(ctx);
-		d_drop(dentry);
 		switch (err) {
 		case -ENOENT:
 			if (nfs_server_capable(dir, NFS_CAP_CASE_INSENSITIVE))
@@ -2189,7 +2188,7 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
 				dir_verifier = nfs_save_change_attribute(dir);
 			nfs_set_verifier(dentry, dir_verifier);
 			d_splice_alias(NULL, dentry);
-			break;
+			goto out;
 		case -EISDIR:
 		case -ENOTDIR:
 			if (open_flags & __O_REGULAR) {
@@ -2205,6 +2204,7 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
 		default:
 			break;
 		}
+		d_drop(dentry);
 		goto out;
 	}
 	file->f_mode |= FMODE_CAN_ODIRECT;
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 7/9] nfs: Use d_alloc_trylock() in nfs_prime_dcache()

[NeilBrown]

From: NeilBrown <neil@brown.name>

NFS uses the results of readdir to prime the dcache.  Using
d_alloc_parallel() can block if there is a concurrent lookup.  Blocking
in that case is pointless as the lookup will add info to the dcache and
there is no value in the readdir waiting to see if it should add the
info too.

Also this call to d_alloc_parallel() is made while the parent
directory is locked.  A proposed change to locking will lock the parent
later, after d_alloc_parallel().  This means it won't be safe to wait in
d_alloc_parallel() while holding the directory lock.

So change to use d_alloc_trylock(), which removes the need for
calculating the hash or doing a preliminary lookup.

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

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 04f6aee65446..e40ea471fa34 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -749,15 +749,12 @@ void nfs_prime_dcache(struct dentry *parent, struct nfs_entry *entry,
 		if (filename.len == 2 && filename.name[1] == '.')
 			return;
 	}
-	filename.hash = full_name_hash(parent, filename.name, filename.len);
 
-	dentry = d_lookup(parent, &filename);
 again:
-	if (!dentry) {
-		dentry = d_alloc_parallel(parent, &filename);
-		if (IS_ERR(dentry))
-			return;
-	}
+	dentry = d_alloc_trylock(parent, &filename);
+	if (IS_ERR(dentry))
+		return;
+
 	if (!d_in_lookup(dentry)) {
 		/* Is there a mountpoint here? If so, just exit */
 		if (!nfs_fsid_equal(&NFS_SB(dentry->d_sb)->fsid,
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 8/9] nfs: use d_alloc_trylock() in silly-rename

[NeilBrown]

From: NeilBrown <neil@brown.name>

Rather than performing a normal lookup (which will be awkward with
future locking changes) use d_alloc_trylock() to find a dentry for an
unused name, and then open-code the rest of lookup_slow() to see if it
is free on the server.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfs/unlink.c | 57 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 20 deletions(-)

diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
index 11a46a993e11..7e01f3d2207b 100644
--- a/fs/nfs/unlink.c
+++ b/fs/nfs/unlink.c
@@ -451,7 +451,7 @@ nfs_sillyrename(struct inode *dir, struct dentry *dentry)
 	static unsigned int sillycounter;
 	unsigned char silly[SILLYNAME_LEN + 1];
 	unsigned long long fileid;
-	struct dentry *sdentry;
+	struct dentry *sdentry, *old;
 	struct inode *inode = d_inode(dentry);
 	struct rpc_task *task;
 	int            error = -EBUSY;
@@ -468,26 +468,43 @@ nfs_sillyrename(struct inode *dir, struct dentry *dentry)
 
 	fileid = d_inode(dentry)->i_ino;
 
-	sdentry = NULL;
-	do {
+newname:
+	sillycounter++;
+	scnprintf(silly, sizeof(silly),
+		  SILLYNAME_PREFIX "%0*llx%0*x",
+		  SILLYNAME_FILEID_LEN, fileid,
+		  SILLYNAME_COUNTER_LEN, sillycounter);
+
+	dfprintk(VFS, "NFS: trying to rename %pd to %s\n", dentry, silly);
+	sdentry = d_alloc_trylock(dentry->d_parent, &QSTR(silly));
+	if (sdentry == ERR_PTR(-EWOULDBLOCK))
+		/* Name currently being looked up */
+		goto newname;
+	if (IS_ERR(sdentry))
+		goto out;
+	if (!d_in_lookup(sdentry)) {
+		if (d_really_is_negative(sdentry)) {
+			/* try to get an in-lookup dentry */
+			d_drop(sdentry);
+			sillycounter--;
+		}
 		dput(sdentry);
-		sillycounter++;
-		scnprintf(silly, sizeof(silly),
-			  SILLYNAME_PREFIX "%0*llx%0*x",
-			  SILLYNAME_FILEID_LEN, fileid,
-			  SILLYNAME_COUNTER_LEN, sillycounter);
-
-		dfprintk(VFS, "NFS: trying to rename %pd to %s\n",
-				dentry, silly);
-
-		sdentry = lookup_noperm(&QSTR(silly), dentry->d_parent);
-		/*
-		 * N.B. Better to return EBUSY here ... it could be
-		 * dangerous to delete the file while it's in use.
-		 */
-		if (IS_ERR(sdentry))
-			goto out;
-	} while (d_inode(sdentry) != NULL); /* need negative lookup */
+		goto newname;
+	}
+	/* This name isn't known locally - check on server */
+	old = nfs_lookup(dir, sdentry, 0);
+	d_lookup_done(sdentry);
+	/*
+	 * N.B. Better to return EBUSY here ... it could be
+	 * dangerous to delete the file while it's in use.
+	 */
+	if (IS_ERR(old))
+		goto out;
+	if (old || d_is_positive(sdentry)) {
+		dput(old);
+		dput(sdentry);
+		goto newname;
+	}
 
 	ihold(inode);
 
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 9/9] nfs: use d_duplicate()

[NeilBrown]

From: NeilBrown <neil@brown.name>

As preparation for d_alloc_parallel() being allowed without the
directory locked, use d_duplicate() to duplicate a dentry for silly
rename.

This results in an in-lookup name, so d_lookup_done() must be called.

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

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index e40ea471fa34..3adb8ce727f8 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -2801,11 +2801,9 @@ int nfs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
 			spin_unlock(&new_dentry->d_lock);
 
 			/* copy the target dentry's name */
-			dentry = d_alloc(new_dentry->d_parent,
-					 &new_dentry->d_name);
+			dentry = d_duplicate(new_dentry);
 			if (!dentry)
 				goto out;
-
 			/* silly-rename the existing target ... */
 			err = nfs_sillyrename(new_dir, new_dentry);
 			if (err)
@@ -2870,8 +2868,10 @@ int nfs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
 		nfs_dentry_handle_enoent(old_dentry);
 
 	/* new dentry created? */
-	if (dentry)
+	if (dentry) {
+		d_lookup_done(dentry);
 		dput(dentry);
+	}
 	return error;
 }
 EXPORT_SYMBOL_GPL(nfs_rename);
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH v2 0/9] nfs: prepare for VFS locking changes.

[Anna Schumaker]

On Mon, Sep 28, 2026, at 10:21 PM, NeilBrown wrote:
> sashiko reported some problem with the previous posting.  A lot
> were false positives as it used the wrong base.  However:
> 3/9 had inverted the return code from nfs_neg_need_reval() - it returns
>     1 for "must revalidate" unlike d_revalidate which returns 0.
> 8/9 didn't handle and error from nfs_lookup() correctly - it
>     should be fatal rather than triggering a retry.
>
> Original intro below.
> Thanks,
> NeilBrown
>
> I am working on changes to directory locking.  The medium term goal is
> to lift d_alloc_parallel() locking above i_rwsem, so d_alloc_paralle()
> can be run unlocked, and consequently cannot be called when ->i_rwsem is
> held.
>
> The requires:
>  1/ the d_alloc_parallel() not be called while i_rwsem is held.  NFS does
>     this in a few places. New interfaces d_alloc_trylock() and d_duplicate()
>     are now available and can be used instead.
>
>  2/ a dentry must not be d_drop()ed while an operation is ongoing, as an
>     unlocked d_alloc_parallel() would then be able to create a new
>     dentry with the same name.  In particular d_rehash() must be phased
>     out as it won't be safe.  Various techiniques are used to avoid the
>     need for d_drop(), particularly that d_splice_alias() is now able to
>     splice a hashed alias.
>
> Further, the first two patches here fixes bugs that I found while
> developing and reviewing these changes.
>
> The patches depend on patches recently added to the vfs tree.  I would
> prefer these land in that tree too with an Acked-by or similar from NFS
> maintainers.

These look okay to me:

Acked-by: Anna Schumaker <anna.schumaker@hammerspace.com>

>
> Thanks,
> NeilBrown
>
>  [PATCH v2 1/9] nfs: fix open-blocking with d_fsdata
>  [PATCH v2 2/9] nfs: fix nfs_call_unlink()
>  [PATCH v2 3/9] nfs: remove d_drop()/d_alloc_parallel() from
>  [PATCH v2 4/9] nfs: use d_splice_alias() in nfs_link()
>  [PATCH v2 5/9] nfs: don't d_drop() before d_splice_alias()
>  [PATCH v2 6/9] nfs: don't d_drop() before d_splice_alias() in
>  [PATCH v2 7/9] nfs: Use d_alloc_trylock() in nfs_prime_dcache()
>  [PATCH v2 8/9] nfs: use d_alloc_trylock() in silly-rename
>  [PATCH v2 9/9] nfs: use d_duplicate()

Re: [PATCH v2 3/9] nfs: remove d_drop()/d_alloc_parallel() from nfs_atomic_open()

[John Stoffel]

>>>>> "NeilBrown" == NeilBrown  <neilb@ownmail.net> writes:

> From: NeilBrown <neil@brown.name>
> 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.

This paragraph is confusing, and I'm not sure which thread can use the
result of the first lookup from this description.  

> 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.
> [Note that when nfs_neg_need_reval() returns 1, ->d_revalidate returns 0].

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

> There is still a d_drop() which allowed d_splice_alias() to succeed.
> This will be removed in a future patch.

> 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 4f1bf45b0c70..f219cc1a5fc5 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 the server
> +		 * on each open.  By invalidating the dentry we
> +		 * also ensure nfs_atomic_open() always has exclusive
> +		 * access to the dentry.
> +		 */
> +		return 1;
>  	if (NFS_SERVER(dir)->flags & NFS_MOUNT_LOOKUP_CACHE_NONEG)
>  		return 1;
>  	/* Case insensitive server? Revalidate negative dentries */
> @@ -2113,7 +2120,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;
 
> @@ -2158,17 +2164,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))
> @@ -2219,10 +2214,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:
> @@ -2245,13 +2236,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

Re: [PATCH v2 3/9] nfs: remove d_drop()/d_alloc_parallel() from nfs_atomic_open()

[NeilBrown]

On Thu, 01 Oct 2026, John Stoffel wrote:
> >>>>> "NeilBrown" == NeilBrown  <neilb@ownmail.net> writes:
> 
> > From: NeilBrown <neil@brown.name>
> > 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.
> 
> This paragraph is confusing, and I'm not sure which thread can use the
> result of the first lookup from this description.  

Unfortunately it isn't clear to me what it is that isn't clear to you
:-)

When O_CREAT is not requested, calls to ->atomic_open are protected only
by a shared lock on the parent so if the dcache contains a negative
dentry for a given name then two ->atomic_open calls on that name can
happen concurrently.  That can be a problem as they both might try to
instantiate the dentry with an inode, and the second to do that will
trigger a BUG.

This is fixed by discarding (d_drop()) the dentry and using
d_alloc_parallel() to allocate a new dentry.  d_alloc_parallel()
effects a lock.  Only one thread will get an in_lookup dentry.  The
other will block until the dentry has been instantiated, and will then
be given the non-in_lookup dentry.

The current code does all this in nfs_atomic_open().  The new code does
the d_drop and the d_alloc_parallel() in VFS code were I can manage the
locking rules more easily.

I thought that is what I said above - though more briefly of course.  If
you can point me more precisely to what is not clear in the original, I
can try to improve it.

Thanks,
NeilBrown

Re: [PATCH v2 3/9] nfs: remove d_drop()/d_alloc_parallel() from nfs_atomic_open()

[John Stoffel]

>>>>> "NeilBrown" == NeilBrown  <neilb@ownmail.net> writes:

> On Thu, 01 Oct 2026, John Stoffel wrote:
>> >>>>> "NeilBrown" == NeilBrown  <neilb@ownmail.net> writes:
>> 
>> > From: NeilBrown <neil@brown.name>
>> > 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.
>> 
>> This paragraph is confusing, and I'm not sure which thread can use the
>> result of the first lookup from this description.  

> Unfortunately it isn't clear to me what it is that isn't clear to you
> :-)

Sorry!  I wasn't clear, probably because I'm not great at the VFS
stuff and was just reacting more to my parsing of the language.  It's
probably not a big deal honestly.  

> When O_CREAT is not requested, calls to ->atomic_open are protected only
> by a shared lock on the parent so if the dcache contains a negative
> dentry for a given name then two ->atomic_open calls on that name can
> happen concurrently.  That can be a problem as they both might try to
> instantiate the dentry with an inode, and the second to do that will
> trigger a BUG.

> This is fixed by discarding (d_drop()) the dentry and using
> d_alloc_parallel() to allocate a new dentry.  d_alloc_parallel()
> effects a lock.  Only one thread will get an in_lookup dentry.  The
> other will block until the dentry has been instantiated, and will then
> be given the non-in_lookup dentry.

> The current code does all this in nfs_atomic_open().  The new code does
> the d_drop and the d_alloc_parallel() in VFS code were I can manage the
> locking rules more easily.

> I thought that is what I said above - though more briefly of course.  If
> you can point me more precisely to what is not clear in the original, I
> can try to improve it.

> Thanks,
> NeilBrown