[PATCH v2] kernfs: allocate inodes and dentry outside kernfs_rwsem

[PATCH v2] kernfs: allocate inodes and dentry outside kernfs_rwsem

[Shakeel Butt]

In the Meta fleet, on 6.16 with the kernfs changes queued for 7.4
backported, we still see long cgroupfs stalls:

- oomd and below waited 72 seconds in kernfs_fop_readdir() while an out
  of memory container created cgroups.  They resumed when the kernel OOM
  killer killed the container's workload.

- oomd waited 15 minutes in kernfs_iop_lookup().  PID 1 waited for
  kernfs_rwsem in cgroup_rmdir() while holding cgroup_mutex.

- A lock profiler sampling part of the fleet saw kernfs_iop_lookup()
  hold kernfs_rwsem across memcg reclaim on 26 hosts in 29 days, for up
  to 625 ms.

The profiler shows where these stalls come from.  kernfs_iop_lookup()
and the export ops allocate a new inode while holding kernfs_rwsem for
read.  The inode is charged to the caller's memcg.  If that memcg is at
memory.max, the allocation can stay in reclaim for a long time,
sometimes until an OOM kill.  A writer then queues on the lock and every
new reader queues behind it.  On cgroup2 this blocks every cgroupfs user
on the machine.

Such lookups are common in a memcg that is out of memory.  Its reclaim
drops its kernfs dentries fast, and the files of a new cgroup have no
inodes yet.

Allocate the inode with the lock dropped.  kernfs_iget() first looks for
an existing inode.  If there is none, it pins the node, drops the lock,
allocates, takes the lock again, and adds the inode only if the node is
still active.  So __kernfs_remove() still finds every inode it must
clear i_nlink on, as commit dc1a0cc95737 ("kernfs: don't lose
IN_DELETE_SELF when decoding a file handle") needs.

kernfs_get_parent_dentry() also held the lock across d_obtain_alias(),
which allocates a dentry.  Hold it only around kernfs_iget(), as
__kernfs_fh_to_dentry() already does.

Tested on a 176-CPU machine.  A task in a full 256 MB memcg with slow
swap looks up the files of new cgroups.  Worst waits in 60 seconds:

                                  before    after
  oomd, for kernfs_rwsem          577 ms   0.7 ms
  below, for kernfs_rwsem         412 ms   1.0 ms
  systemd, for kernfs_rwsem       449 ms    17 us
  readdir of another directory   1154 ms   3.2 ms

The lookup itself is still slow.  The VFS still holds its directory's
lock, so other work in that one directory still waits.  Cold sysfs and
cgroupfs walks are not slower.

kernfs_fill_super() still allocates the root inode under the lock.  It
runs once per superblock.

Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---
Changes since v1:
- dentry allocation outside kernfs_rwsem (Sashiko).
- Couple of comments updated.

 fs/kernfs/dir.c             | 11 +++---
 fs/kernfs/inode.c           | 69 +++++++++++++++++++++++++++++++++++++
 fs/kernfs/kernfs-internal.h |  2 ++
 fs/kernfs/mount.c           | 18 +++++-----
 4 files changed, 88 insertions(+), 12 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 7e65854a09e4..70c114911063 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -37,7 +37,7 @@ static bool __kernfs_active(struct kernfs_node *kn)
 	return atomic_read(&kn->active) >= 0;
 }
 
-static bool kernfs_active(struct kernfs_node *kn)
+bool kernfs_active(struct kernfs_node *kn)
 {
 	lockdep_assert_held(&kernfs_root(kn)->kernfs_rwsem);
 	return __kernfs_active(kn);
@@ -1269,9 +1269,12 @@ static struct dentry *kernfs_iop_lookup(struct inode *dir,
 			up_read(&root->kernfs_rwsem);
 			return NULL;
 		}
-		inode = kernfs_get_inode(dir->i_sb, kn);
-		if (!inode)
-			inode = ERR_PTR(-ENOMEM);
+		inode = kernfs_iget(dir->i_sb, kn);
+		if (!inode) {
+			/* deactivated while kernfs_iget() dropped the lock */
+			up_read(&root->kernfs_rwsem);
+			return NULL;
+		}
 	}
 	/*
 	 * Needed for negative dentry validation.
diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index 237dcdd73fc2..6c64bff0fc9f 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -256,6 +256,75 @@ struct inode *kernfs_get_inode(struct super_block *sb, struct kernfs_node *kn)
 	return inode;
 }
 
+static int kernfs_iget_test(struct inode *inode, void *data)
+{
+	return inode->i_ino == *(u64 *)data;
+}
+
+static int kernfs_iget_set(struct inode *inode, void *data)
+{
+	inode->i_ino = *(u64 *)data;
+	return 0;
+}
+
+/**
+ * kernfs_iget - get the inode of @kn, allocating it outside kernfs_rwsem
+ * @sb: super block
+ * @kn: kernfs_node to get the inode for
+ *
+ * Unlike kernfs_get_inode(), this never allocates with kernfs_rwsem held.
+ * If @kn has no inode yet, the lock is dropped while allocating one, as
+ * the allocation can stall in memcg reclaim.  The new inode is added only
+ * if @kn is still active when the lock is taken again, so
+ * __kernfs_remove() always finds it to clear i_nlink.
+ *
+ * Only @kn is pinned while the lock is dropped.  Its name and parent can
+ * change in that time.
+ *
+ * Context: May sleep.  Called with kernfs_rwsem held for read.  Returns
+ * with it held, but may have dropped and retaken it.
+ *
+ * Return: the inode, %NULL if @kn is inactive or became inactive while
+ * the lock was dropped, or ERR_PTR(-ENOMEM).
+ */
+struct inode *kernfs_iget(struct super_block *sb, struct kernfs_node *kn)
+{
+	struct kernfs_root *root = kernfs_root(kn);
+	u64 ino = kernfs_ino(kn);
+	struct inode *inode, *new;
+
+	lockdep_assert_held_read(&root->kernfs_rwsem);
+
+	inode = ilookup(sb, ino);
+	if (inode || !kernfs_active(kn))
+		return inode;
+
+	kernfs_get(kn);
+	up_read(&root->kernfs_rwsem);
+	new = new_inode(sb);
+	down_read(&root->kernfs_rwsem);
+
+	if (!new) {
+		inode = ERR_PTR(-ENOMEM);
+	} else if (!kernfs_active(kn)) {
+		inode = NULL;
+	} else {
+		inode = inode_insert5(new, ino, kernfs_iget_test,
+				      kernfs_iget_set, &ino);
+		if (inode == new) {
+			kernfs_init_inode(kn, inode);
+			new = NULL;
+		} else if (!inode) {
+			inode = ERR_PTR(-ENOMEM);
+		}
+	}
+
+	kernfs_put(kn);
+	if (new)
+		iput(new);
+	return inode;
+}
+
 /*
  * The kernfs_node serves as both an inode and a directory entry for
  * kernfs.  To prevent the kernfs inode numbers from being freed
diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
index 1609c1519698..6cc19c9ef47b 100644
--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h
@@ -186,6 +186,7 @@ int kernfs_iop_getattr(struct mnt_idmap *idmap,
 		       u32 request_mask, unsigned int query_flags);
 ssize_t kernfs_iop_listxattr(struct dentry *dentry, char *buf, size_t size);
 int __kernfs_setattr(struct kernfs_node *kn, const struct iattr *iattr);
+struct inode *kernfs_iget(struct super_block *sb, struct kernfs_node *kn);
 
 /*
  * dir.c
@@ -194,6 +195,7 @@ extern const struct dentry_operations kernfs_dops;
 extern const struct file_operations kernfs_dir_fops;
 extern const struct inode_operations kernfs_dir_iops;
 
+bool kernfs_active(struct kernfs_node *kn);
 struct kernfs_node *kernfs_get_active(struct kernfs_node *kn);
 void kernfs_put_active(struct kernfs_node *kn);
 int kernfs_add_one(struct kernfs_node *kn);
diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
index c15ba6357162..ccb7dc575c7c 100644
--- a/fs/kernfs/mount.c
+++ b/fs/kernfs/mount.c
@@ -125,11 +125,9 @@ static struct dentry *__kernfs_fh_to_dentry(struct super_block *sb,
 	}
 
 	/*
-	 * Hold kernfs_rwsem across the lookup as well as kernfs_get_inode().
-	 * __kernfs_remove() deactivates the subtree and clears i_nlink on its
-	 * inodes under the write lock, so under the read lock either
-	 * kernfs_find_and_get_node_by_id() refuses the node, or the inode is
-	 * in the inode hash before the ilookup() pass goes looking for it.
+	 * Hold kernfs_rwsem from the lookup through kernfs_iget().  Then
+	 * __kernfs_remove() either finds the inode to clear i_nlink, or the
+	 * node is already inactive and no inode is created.
 	 */
 	scoped_guard(rwsem_read, &info->root->kernfs_rwsem) {
 		kn = kernfs_find_and_get_node_by_id(info->root, id);
@@ -146,7 +144,7 @@ static struct dentry *__kernfs_fh_to_dentry(struct super_block *sb,
 				return ERR_PTR(-ESTALE);
 		}
 
-		inode = kernfs_get_inode(sb, kn);
+		inode = kernfs_iget(sb, kn);
 		kernfs_put(kn);
 	}
 
@@ -171,9 +169,13 @@ static struct dentry *kernfs_get_parent_dentry(struct dentry *child)
 {
 	struct kernfs_node *kn = kernfs_dentry_node(child);
 	struct kernfs_root *root = kernfs_root(kn);
+	struct inode *inode;
+
+	scoped_guard(rwsem_read, &root->kernfs_rwsem) {
+		inode = kernfs_iget(child->d_sb, kernfs_parent(kn));
+	}
 
-	guard(rwsem_read)(&root->kernfs_rwsem);
-	return d_obtain_alias(kernfs_get_inode(child->d_sb, kernfs_parent(kn)));
+	return d_obtain_alias(inode);
 }
 
 static const struct export_operations kernfs_export_ops = {
-- 
2.53.0-Meta

Re: [PATCH v2] kernfs: allocate inodes and dentry outside kernfs_rwsem

[Tejun Heo]

Hello, Shakeel.

On Fri, Oct 16, 2026 at 09:43:45AM -0700, Shakeel Butt wrote:
> Allocate the inode with the lock dropped.  kernfs_iget() first looks for
> an existing inode.  If there is none, it pins the node, drops the lock,
> allocates, takes the lock again, and adds the inode only if the node is
> still active.  So __kernfs_remove() still finds every inode it must
> clear i_nlink on, as commit dc1a0cc95737 ("kernfs: don't lose
> IN_DELETE_SELF when decoding a file handle") needs.

Would it be simpler to allocate the inode before taking kernfs_rwsem and
free it if it ends up unused, as 8b64f32fee95 ("kernfs: allocate the new
name outside kernfs_rwsem") and 5ad03af18434 ("kernfs: allocate the open
node outside the open file mutex") do? Then the lock doesn't have to be
dropped, and there's no need for the pin and the active recheck. The
unused cases (negative lookups, inactive nodes, nodes which already have
an inode) are exceptions, and an inode alloc and free isn't expensive.

The helper would then just run under kernfs_rwsem. Maybe
kernfs_get_inode_locked() would be a better name for it?

kernfs_fill_super() can preallocate the same way, which gets rid of the
last inode allocation under kernfs_rwsem. cgroup_may_write() allocates
the inode under cgroup_mutex and could allocate before grabbing the
mutex too.

> kernfs_get_parent_dentry() also held the lock across d_obtain_alias(),
> which allocates a dentry.  Hold it only around kernfs_iget(), as
> __kernfs_fh_to_dentry() already does.

This also adds the active test to ->get_parent. A parent which is being
removed and has no inode now gets -ESTALE instead of a new inode with
i_nlink 1, which closes the window dc1a0cc95737 ("kernfs: don't lose
IN_DELETE_SELF when decoding a file handle") left open. Can you mention
it in the description?

Thanks.

-- 
tejun

Re: [PATCH v2] kernfs: allocate inodes and dentry outside kernfs_rwsem

[Shakeel Butt]

On Fri, Oct 16, 2026 at 10:33:20AM -1000, Tejun Heo wrote:
> Hello, Shakeel.
> 
> On Fri, Oct 16, 2026 at 09:43:45AM -0700, Shakeel Butt wrote:
> > Allocate the inode with the lock dropped.  kernfs_iget() first looks for
> > an existing inode.  If there is none, it pins the node, drops the lock,
> > allocates, takes the lock again, and adds the inode only if the node is
> > still active.  So __kernfs_remove() still finds every inode it must
> > clear i_nlink on, as commit dc1a0cc95737 ("kernfs: don't lose
> > IN_DELETE_SELF when decoding a file handle") needs.
> 
> Would it be simpler to allocate the inode before taking kernfs_rwsem and
> free it if it ends up unused, as 8b64f32fee95 ("kernfs: allocate the new
> name outside kernfs_rwsem") and 5ad03af18434 ("kernfs: allocate the open
> node outside the open file mutex") do? Then the lock doesn't have to be
> dropped, and there's no need for the pin and the active recheck. The
> unused cases (negative lookups, inactive nodes, nodes which already have
> an inode) are exceptions, and an inode alloc and free isn't expensive.

Thanks for taking a look and yes, it is much better and simpler.

> 
> The helper would then just run under kernfs_rwsem. Maybe
> kernfs_get_inode_locked() would be a better name for it?

Ack

> 
> kernfs_fill_super() can preallocate the same way, which gets rid of the
> last inode allocation under kernfs_rwsem.

Ack

> cgroup_may_write() allocates
> the inode under cgroup_mutex and could allocate before grabbing the
> mutex too.

Let me send this one as a separate patch.

> 
> > kernfs_get_parent_dentry() also held the lock across d_obtain_alias(),
> > which allocates a dentry.  Hold it only around kernfs_iget(), as
> > __kernfs_fh_to_dentry() already does.
> 
> This also adds the active test to ->get_parent. A parent which is being
> removed and has no inode now gets -ESTALE instead of a new inode with
> i_nlink 1, which closes the window dc1a0cc95737 ("kernfs: don't lose
> IN_DELETE_SELF when decoding a file handle") left open. Can you mention
> it in the description?

Yes, I will do.

Thanks again for the review.