Harden the hand-off of a pending silly-delete. When the last user of a silly-renamed file goes away, the client can move the delete it still owes onto another cached entry for the same name. Leave that entry alone if a removal of the name, or a rename onto it, is already in flight on it. Moving the delete there would overwrite the mark that makes lookups of the name wait for that removal: the wait would end early, the removal could report success without removing anything, the pending delete would be leaked, and a later release of the entry would dereference a NULL pointer. If that removal succeeds, the silly-delete was redundant; if it fails, the silly-renamed file stays on the server, which is the cost of skipping rather than waiting. The mark and the hand-off have shared ->d_fsdata since the commit named below, which reasoned that no silly-delete is pending on an entry being removed or renamed over, but not that a hand-off could arrive while that removal is in flight. The hand-off has stayed unreachable since then only because nfs_call_unlink() passed d_alloc_parallel() a name whose hash it had not computed, so the lookup for the other entry never found it. NeilBrown's "nfs: fix nfs_call_unlink()" computes that hash, which makes the hand-off reachable even though every removal holds the parent directory exclusive: the hand-off does not take that lock. That fix adds no test of the mark; this is the test, and a tree that takes that fix needs this one too. It does not depend on any other patch. This was found by an AI coding assistant (Claude Code) reading the code; Sashiko's review of NeilBrown's patch reports it too. The hand-off itself has been reproduced with that patch applied; its race with a removal or rename has not. Fixes: 3c59366c207e ("NFS: don't unhash dentry during unlink/rename") Signed-off-by: Jonathan Flynn --- Notes: For reviewers and review tools; not part of the changelog. Known and not fixed by this series: a hand-off that lands after nfs_unlink() or nfs_rename() clears the mark, and before the VFS's d_delete() or nfs_rename()'s d_move(). The cover letter describes it, with what it costs and a fix offered. fs/nfs/unlink.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c index 0eddc7ee11..491ebd8582 100644 --- a/fs/nfs/unlink.c +++ b/fs/nfs/unlink.c @@ -142,9 +142,26 @@ static int nfs_call_unlink(struct dentry *dentry, struct inode *inode, struct nf * the sillyrename information to the aliased dentry. */ 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. + * 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 + * silly-rename among its callers -- for the length of another + * task's RPC, with ->d_lock dropped and these tests redone. + * If that removal then fails, the silly-renamed file stays on + * the server: the price of skipping. + */ 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_flags & DCACHE_NFSFS_RENAMED) && + alias->d_fsdata != NFS_FSDATA_BLOCKED) { devname_garbage = alias->d_fsdata; alias->d_fsdata = data; alias->d_flags |= DCACHE_NFSFS_RENAMED; -- 2.43.0