Skip to contentSkip to conversation

NFS: Fix stale negative cache on failed nfs_mkdir()

PATCH RESEND In review Jimmy Zuber 2 participants 2 messages Sep 22

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH RESEND 1/1 NFS: Fix stale negative cache on failed nfs_mkdir()

Since commit 8376583b84a1 ("nfs: change mkdir inode_operation to return alternate dentry if needed"), nfs_mkdir() no longer drops the dentry on RPC failure. A negative cache value can then result in spurious ENOENT results for further operations within that directory, such as open or stat.

We observed this breaking some applications where an EEXIST on mkdir caused the application to assume the directory to exist, only to fail operations within that directory due to the retained negative cache entry. The other create paths, nfs_create() and nfs_mknod(), still drop the dentry on failure. This patch restores the previous behavior.

Fixes 8376583b84a1 ("nfs: change mkdir inode_operation to return alternate dentry if needed")
Cc [email protected] # v6.15+

---

Resend of:
https://lore.kernel.org/linux-nfs/[email protected]/

No change to the patch itself; rebased onto v7.3-rc4 and Cc'ing NeilBrown as the author of the Fixes: commit. nfs_mkdir() in v7.3-rc4 still returns the error without unhashing @dentry, and vfs_mkdir()'s error path only calls end_creating()/end_dirop(), which unlocks the parent and dputs the dentry, so the hashed negative dentry survives in the dcache.

We reproduced this against a local knfsd export over NFSv4.1. The export is mounted several times with "nosharecache" so that each mount acts as an independent client with its own dcache:

  # /etc/exports
  /srv/nfs 127.0.0.1(rw,sync,no_subtree_check,no_root_squash,fsid=0)
  for i in $(seq 0 5); do
      mount -t nfs4 -o vers=4.1,nosharecache,acdirmin=30,acdirmax=60 \
          127.0.0.1:/ /mnt/nfs$i
  done

For each of 40 new cache/<id> directories, every mount stats the directory to seed a negative entry, then all mounts race to mkdir it. The winner writes a file and renames it, and the losers that got EEXIST then read that file back. On a vanilla v6.17 kernel, the 200 loser reads were:

unpatched: read OK 0 / ENOENT 200
patched: read OK 200 / ENOENT 0

 fs/nfs/dir.c | 2 ++
 1 file changed, 2 insertions(+)
fs/nfs/dir.c +2 -0
index 49394123b..a26c459ff 100644
@@ -2490,6 +2490,8 @@ struct dentry *nfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	trace_nfs_mkdir_enter(dir, dentry);
 	ret = NFS_PROTO(dir)->mkdir(dir, dentry, &attr);
 	trace_nfs_mkdir_exit(dir, dentry, PTR_ERR_OR_ZERO(ret));
+	if (IS_ERR(ret))
+		d_drop(dentry);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(nfs_mkdir);

base-commit: c4e9f74da4389a6b3e505d329d99232915a6b9cd

Signature · 1 line
2.50.1
Reviewed-by on v1 1/1
#1 lore raw
Quoted 12 lines · Jimmy Zuber

Since commit 8376583b84a1 ("nfs: change mkdir inode_operation to return alternate dentry if needed"), nfs_mkdir() no longer drops the dentry on RPC failure. A negative cache value can then result in spurious ENOENT results for further operations within that directory, such as open or stat.

We observed this breaking some applications where an EEXIST on mkdir caused the application to assume the directory to exist, only to fail operations within that directory due to the retained negative cache entry. The other create paths, nfs_create() and nfs_mknod(), still drop the dentry on failure. This patch restores the previous behavior.

Quoted 3 lines
Fixes 8376583b84a1 ("nfs: change mkdir inode_operation to return alternate dentry if needed")
Cc [email protected] # v6.15+

Thanks for finding and reporting this!

NeilBrown

Quoted 53 lines

---

Resend of:
https://lore.kernel.org/linux-nfs/[email protected]/

No change to the patch itself; rebased onto v7.3-rc4 and Cc'ing NeilBrown as the author of the Fixes: commit. nfs_mkdir() in v7.3-rc4 still returns the error without unhashing @dentry, and vfs_mkdir()'s error path only calls end_creating()/end_dirop(), which unlocks the parent and dputs the dentry, so the hashed negative dentry survives in the dcache.

We reproduced this against a local knfsd export over NFSv4.1. The export is mounted several times with "nosharecache" so that each mount acts as an independent client with its own dcache:

  # /etc/exports
  /srv/nfs 127.0.0.1(rw,sync,no_subtree_check,no_root_squash,fsid=0)
  for i in $(seq 0 5); do
      mount -t nfs4 -o vers=4.1,nosharecache,acdirmin=30,acdirmax=60 \
          127.0.0.1:/ /mnt/nfs$i
  done

For each of 40 new cache/<id> directories, every mount stats the directory to seed a negative entry, then all mounts race to mkdir it. The winner writes a file and renames it, and the losers that got EEXIST then read that file back. On a vanilla v6.17 kernel, the 200 loser reads were:

unpatched: read OK 0 / ENOENT 200
patched: read OK 200 / ENOENT 0

 fs/nfs/dir.c | 2 ++
 1 file changed, 2 insertions(+)
fs/nfs/dir.c +2 -0
index 49394123b..a26c459ff 100644
@@ -2490,6 +2490,8 @@ struct dentry *nfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	trace_nfs_mkdir_enter(dir, dentry);
 	ret = NFS_PROTO(dir)->mkdir(dir, dentry, &attr);
 	trace_nfs_mkdir_exit(dir, dentry, PTR_ERR_OR_ZERO(ret));
+	if (IS_ERR(ret))
+		d_drop(dentry);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(nfs_mkdir);

base-commit: c4e9f74da4389a6b3e505d329d99232915a6b9cd

Signature · 1 line
2.50.1