Merge patch series "kernfs: remove kernfs_rwsem from dentry revalidation"

Shakeel Butt <shakeel.butt@linux.dev> says:

kernfs: remove kernfs_rwsem from dentry revalidation

At Meta, we are seeing important system daemons that poll cgroupfs and
sysfs geth stuck in kernfs_dop_revalidate() for minutes. The two that
hurt most are the ones we can least afford to lose: oomd, which decides
what to kill when a machine runs out of memory, and below[1], which
records the telemetry used to understand what happened afterwards.

kernfs_dop_revalidate() takes kernfs_rwsem for read once per path component
of every walk into a kernfs mount. Linux rwsems do not permit reader lock
stealing once a writer is queued, so one writer -- a cgroup created or
destroyed, a device renamed -- parks the entire incoming reader stream in
uninterruptible sleep. Daemons polling cgroup files in a loop are exactly
the workload that turns this into a convoy, and cgroup churn is exactly
what a busy machine does.

Nothing the callback reads needs the semaphore. kn->active is an atomic_t
already tested lock-free elsewhere, kn->__parent and kn->name are RCU
pointers, kn->ns can be compared rather than dereferenced, and
parent->dir.rev is a plain counter.

  1/4 uses the parent inode and name the VFS already passes to
      ->d_revalidate() rather than recovering them from mutable dentry
      fields, comparing the name by explicit length
  2/4 annotates the directory revision counter for lockless access
  3/4 compares namespace tags by pointer
  4/4 removes kernfs_rwsem from the callback

LOOKUP_RCU still returns -ECHILD. kernfs_iop_permission() forces every walk
out of RCU-walk before children are revalidated, so lifting it here would
have no effect until that path is fixed; left to a separate series.

Readers walking cgroupfs and sysfs while another thread churns cgroups,
renames netdevs and adds/removes devices, on an 8-CPU VM:

                    kernfs_rwsem read   contentions   revalidate among
                        acquisitions                  top call sites
  before                  48,593,360     1,744,517    #1 and #2
  after                    1,280,280       429,846    absent

Reader path-walk throughput improved 35-53% over the same workload.

Tested against an unpatched control of the same tree, built and booted with
KASAN, KCSAN (default and STRICT), PROVE_LOCKING, PROVE_RCU,
DEBUG_ATOMIC_SLEEP and LOCK_STAT. The deactivated, renamed and
namespace-moved reject paths and negative-dentry invalidation all behave as
before. KCSAN_STRICT over 180s reports no data race involving
kernfs_dop_revalidate() or any field it reads, and there are no KASAN,
lockdep or might-sleep reports across millions of concurrent path walks.
The only kernfs KCSAN reports are in kernfs_refresh_inode(), present
identically on the control and addressed separately.

Link: https://github.com/facebookincubator/below [1]

* patches from https://patch.msgid.link/20260821050507.2161607-1-shakeel.butt@linux.dev:
  kernfs: Remove kernfs_rwsem from dentry revalidation
  kernfs: Avoid namespace dereference in d_revalidate()
  kernfs: Prepare directory revisions for lockless reads
  kernfs: Use VFS lookup context in d_revalidate()

Link: https://patch.msgid.link/20260821050507.2161607-1-shakeel.butt@linux.dev
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>

Author: brauner

fs/kernfs/dir.c

@@ -1171,74 +1171,60 @@ struct kernfs_node *kernfs_create_empty_dir(struct kernfs_node *parent,
 static int kernfs_dop_revalidate(struct inode *dir, const struct qstr *name,
 				 struct dentry *dentry, unsigned int flags)
 {
-	struct kernfs_node *kn, *parent;
-	struct kernfs_root *root;
+	struct kernfs_node *parent = dir->i_private;
+	struct kernfs_node *kn;
+	const char *kn_name;
 
 	if (flags & LOOKUP_RCU)
 		return -ECHILD;
 
 	/* Negative hashed dentry? */
 	if (d_really_is_negative(dentry)) {
-		/* If the kernfs parent node has changed discard and
-		 * proceed to ->lookup.
-		 *
-		 * There's nothing special needed here when getting the
-		 * dentry parent, even if a concurrent rename is in
-		 * progress. That's because the dentry is negative so
-		 * it can only be the target of the rename and it will
-		 * be doing a d_move() not a replace. Consequently the
-		 * dentry d_parent won't change over the d_move().
+		/*
+		 * If the kernfs parent node has changed discard and proceed to
+		 * ->lookup.
 		 *
 		 * Also kernfs negative dentries transitioning from
 		 * negative to positive during revalidate won't happen
 		 * because they are invalidated on containing directory
 		 * changes and the lookup re-done so that a new positive
 		 * dentry can be properly created.
 		 */
-		root = kernfs_root_from_sb(dentry->d_sb);
-		down_read(&root->kernfs_rwsem);
-		parent = kernfs_dentry_node(dentry->d_parent);
-		if (parent) {
-			if (kernfs_dir_changed(parent, dentry)) {
-				up_read(&root->kernfs_rwsem);
-				return 0;
-			}
-		}
-		up_read(&root->kernfs_rwsem);
-
-		/* The kernfs parent node hasn't changed, leave the
-		 * dentry negative and return success.
-		 */
-		return 1;
+		return !kernfs_dir_changed(parent, dentry);
 	}
 
 	kn = kernfs_dentry_node(dentry);
-	root = kernfs_root(kn);
-	down_read(&root->kernfs_rwsem);
+
+	guard(rcu)();
 
 	/* The kernfs node has been deactivated */
-	if (!kernfs_active(kn))
-		goto out_bad;
+	if (!__kernfs_active(kn))
+		return 0;
 
-	parent = kernfs_parent(kn);
 	/* The kernfs node has been moved? */
-	if (kernfs_dentry_node(dentry->d_parent) != parent)
-		goto out_bad;
+	if (kernfs_parent(kn) != parent)
+		return 0;
 
 	/* The kernfs node has been renamed */
-	if (strcmp(dentry->d_name.name, kernfs_rcu_name(kn)) != 0)
-		goto out_bad;
+	kn_name = kernfs_rcu_name(kn);
+	if (name->len != strlen(kn_name) ||
+	    memcmp(name->name, kn_name, name->len))
+		return 0;
 
-	/* The kernfs node has been moved to a different namespace */
-	if (parent && kernfs_ns_enabled(parent) &&
-	    kernfs_ns_id(kernfs_info(dentry->d_sb)->ns) != kernfs_ns_id(kn->ns))
-		goto out_bad;
+	/*
+	 * The kernfs node has been moved to a different namespace.
+	 *
+	 * KERNFS_NS is set by kernfs_enable_ns() while @parent still has no
+	 * children, so it cannot change while a child of @parent is being
+	 * revalidated. The other bits in that word, KERNFS_ACTIVATED and
+	 * KERNFS_REMOVING, are updated under kernfs_rwsem and are not read
+	 * here, so racing with them is intentional and harmless.
+	 */
+	if (data_race(kernfs_ns_enabled(parent)) &&
+	    kernfs_info(dir->i_sb)->ns != READ_ONCE(kn->ns))
+		return 0;
 
-	up_read(&root->kernfs_rwsem);
 	return 1;
-out_bad:
-	up_read(&root->kernfs_rwsem);
-	return 0;
 }
 
 const struct dentry_operations kernfs_dops = {
@@ -1878,15 +1864,15 @@ int kernfs_rename_ns(struct kernfs_node *kn, struct kernfs_node *new_parent,
 
 		rcu_assign_pointer(kn->__parent, new_parent);
 
-		kn->ns = new_ns;
+		WRITE_ONCE(kn->ns, new_ns);
 		if (new_name)
 			rcu_assign_pointer(kn->name, new_name);
 
 		write_unlock_irq(&root->kernfs_rename_lock);
 		kernfs_put(old_parent);
 	} else {
 		/* name assignment is RCU protected, parent is the same */
-		kn->ns = new_ns;
+		WRITE_ONCE(kn->ns, new_ns);
 		if (new_name)
 			rcu_assign_pointer(kn->name, new_name);
 	}

fs/kernfs/kernfs-internal.h

@@ -147,20 +147,19 @@ static inline struct kernfs_node *kernfs_dentry_node(struct dentry *dentry)
 static inline void kernfs_set_rev(struct kernfs_node *parent,
 				  struct dentry *dentry)
 {
-	dentry->d_time = parent->dir.rev;
+	WRITE_ONCE(dentry->d_time, READ_ONCE(parent->dir.rev));
 }
 
 static inline void kernfs_inc_rev(struct kernfs_node *parent)
 {
-	parent->dir.rev++;
+	lockdep_assert_held_write(&parent->dir.root->kernfs_rwsem);
+	WRITE_ONCE(parent->dir.rev, parent->dir.rev + 1);
 }
 
 static inline bool kernfs_dir_changed(struct kernfs_node *parent,
 				      struct dentry *dentry)
 {
-	if (parent->dir.rev != dentry->d_time)
-		return true;
-	return false;
+	return READ_ONCE(parent->dir.rev) != READ_ONCE(dentry->d_time);
 }
 
 extern const struct super_operations kernfs_sops;