[PATCH 0/3] kernfs: do less work under the kernfs_rwsem write lock

[PATCH 0/3] kernfs: do less work under the kernfs_rwsem write lock

[Shakeel Butt]

kernfs_rwsem is per kernfs root, so sysfs and cgroupfs have one each for
the whole machine, and every create, remove and rename takes it for
writing.  Whatever a writer does while holding it, everyone else waits
for.  These three patches take work out of that section.

Patch 1: kernfs_add_one() links the node in, drops the lock, then calls
kernfs_activate(), which takes it again.  sysfs does not create nodes
deactivated, so it pays two write locks for every file and directory.
Activate before dropping the lock.  lock_stat says 1200 -> 830 write
acquisitions for creating and destroying five dummy netdevs.

Patch 2: kernfs_rename_ns() calls kstrdup_const() under the lock.  It is
the only GFP_KERNEL allocation under the write lock in kernfs, so a
rename can enter reclaim with the lock held.  Allocate before the lock.

Patch 3: kernfs_rename_ns() also frees the old name under the lock with
kfree_rcu_mightsleep().  When its batching allocation fails, which is
what happens under memory pressure, it falls back to synchronize_rcu().
A rename can then wait out a grace period with the lock held.  Free
after the unlock.

Tested with the kernfs selftests under KASAN and lockdep.

Shakeel Butt (3):
  kernfs: activate a new node without dropping kernfs_rwsem
  kernfs: allocate the new name outside kernfs_rwsem
  kernfs: free the old name outside kernfs_rwsem

 fs/kernfs/dir.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)


base-commit: 7f24d0f60b1087dbc28d68398957c18774e5a7d8
-- 
2.53.0-Meta

[PATCH 1/3] kernfs: activate a new node without dropping kernfs_rwsem

[Shakeel Butt]

kernfs_add_one() links the node in, drops the kernfs_rwsem write lock,
then calls kernfs_activate(), which takes it again.  Roots that do not
set KERNFS_ROOT_CREATE_DEACTIVATED, such as sysfs, therefore pay two
write locks for every file, directory and symlink created.

A new node has no children, so kernfs_activate() would walk only that
node.  Call kernfs_activate_one() before dropping the lock instead.  Its
two WARN_ON_ONCE()s still hold: the node was just linked, and nothing
can have changed its active count yet.  This also closes the window
where a node is linked but not yet activated.

lock_stat, creating and destroying five dummy netdevs:

                                 before    after
  kernfs_rwsem write acquires     1200      830

Assisted-by: LLM
Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 fs/kernfs/dir.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 07abf59f0264..8953e8a07537 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -30,6 +30,8 @@ static char kernfs_pr_cont_buf[PATH_MAX];	/* protected by pr_cont_lock */
 
 #define rb_to_kn(X) rb_entry((X), struct kernfs_node, rb)
 
+static void kernfs_activate_one(struct kernfs_node *kn);
+
 static bool __kernfs_active(struct kernfs_node *kn)
 {
 	return atomic_read(&kn->active) >= 0;
@@ -861,7 +863,6 @@ int kernfs_add_one(struct kernfs_node *kn)
 	}
 
 	up_write(&root->kernfs_iattr_rwsem);
-	up_write(&root->kernfs_rwsem);
 
 	/*
 	 * Activate the new node unless CREATE_DEACTIVATED is requested.
@@ -869,9 +870,15 @@ int kernfs_add_one(struct kernfs_node *kn)
 	 * activating the node with kernfs_activate().  A node which hasn't
 	 * been activated is not visible to userland and its removal won't
 	 * trigger deactivation.
+	 *
+	 * @kn has no children yet, so kernfs_activate() would walk only @kn.
+	 * Do it here rather than dropping the write lock and taking it again
+	 * for every new node.
 	 */
-	if (!(kernfs_root(kn)->flags & KERNFS_ROOT_CREATE_DEACTIVATED))
-		kernfs_activate(kn);
+	if (!(root->flags & KERNFS_ROOT_CREATE_DEACTIVATED))
+		kernfs_activate_one(kn);
+
+	up_write(&root->kernfs_rwsem);
 	return 0;
 
 out_unlock:
-- 
2.53.0-Meta

[PATCH 2/3] kernfs: allocate the new name outside kernfs_rwsem

[Shakeel Butt]

kernfs_rename_ns() calls kstrdup_const() while holding the kernfs_rwsem
write lock.  It is the only GFP_KERNEL allocation under that lock in
kernfs, so a rename can enter reclaim while every create, remove and
rename in the hierarchy waits behind it.

Copy the name before taking the lock and free the copy again if the
rename turns out not to need it.  A rename that keeps the name, hits a
collision or finds the node gone now does one extra kstrdup_const();
renames are rare.  -ENOMEM is still reported at the same point as
before, so nothing changes about which error a caller sees.

Assisted-by: LLM
Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 fs/kernfs/dir.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 8953e8a07537..3bbdd9a8acc8 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -1819,6 +1819,7 @@ int kernfs_rename_ns(struct kernfs_node *kn, struct kernfs_node *new_parent,
 		     const char *new_name, const struct ns_common *new_ns)
 {
 	struct kernfs_node *old_parent;
+	const char *dup_name = NULL;
 	struct kernfs_root *root;
 	const char *old_name;
 	bool reparent;
@@ -1828,6 +1829,9 @@ int kernfs_rename_ns(struct kernfs_node *kn, struct kernfs_node *new_parent,
 	if (!rcu_access_pointer(kn->__parent))
 		return -EINVAL;
 
+	if (new_name)
+		dup_name = kstrdup_const(new_name, GFP_KERNEL);
+
 	root = kernfs_root(kn);
 	down_write(&root->kernfs_rwsem);
 
@@ -1859,9 +1863,10 @@ int kernfs_rename_ns(struct kernfs_node *kn, struct kernfs_node *new_parent,
 	/* rename kernfs_node */
 	if (strcmp(old_name, new_name) != 0) {
 		error = -ENOMEM;
-		new_name = kstrdup_const(new_name, GFP_KERNEL);
-		if (!new_name)
+		if (!dup_name)
 			goto out;
+		new_name = dup_name;
+		dup_name = NULL;
 	} else {
 		new_name = NULL;
 	}
@@ -1901,6 +1906,7 @@ int kernfs_rename_ns(struct kernfs_node *kn, struct kernfs_node *new_parent,
 	error = 0;
  out:
 	up_write(&root->kernfs_rwsem);
+	kfree_const(dup_name);
 	return error;
 }
 
-- 
2.53.0-Meta

[PATCH 3/3] kernfs: free the old name outside kernfs_rwsem

[Shakeel Butt]

kernfs_rename_ns() frees the replaced name with kfree_rcu_mightsleep()
while still holding the kernfs_rwsem write lock.  If the batching
allocation fails, which is what happens under memory pressure,
kvfree_call_rcu() falls back to a full synchronize_rcu() before freeing.
A rename can then wait out a grace period with the write lock held, and
every create, remove and rename in the hierarchy waits with it.

The name is already unpublished by then, so nothing needs the free to
happen under the lock.  Move it past the unlock.

Assisted-by: LLM
Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 fs/kernfs/dir.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 1fa288a48d7c..b071071e51b0 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -1820,6 +1820,7 @@ int kernfs_rename_ns(struct kernfs_node *kn, struct kernfs_node *new_parent,
 {
 	struct kernfs_node *old_parent;
 	const char *dup_name = NULL;
+	const char *put_name = NULL;
 	struct kernfs_root *root;
 	const char *old_name;
 	bool reparent;
@@ -1905,12 +1906,14 @@ int kernfs_rename_ns(struct kernfs_node *kn, struct kernfs_node *new_parent,
 	kernfs_link_sibling(kn);
 
 	if (new_name && !is_kernel_rodata((unsigned long)old_name))
-		kfree_rcu_mightsleep(old_name);
+		put_name = old_name;
 
 	error = 0;
  out:
 	up_write(&root->kernfs_rwsem);
 	kfree_const(dup_name);
+	if (put_name)
+		kfree_rcu_mightsleep(put_name);
 	return error;
 }
 
-- 
2.53.0-Meta