[PATCH 0/2] speed up /proc/filesystems

[PATCH 0/2] speed up /proc/filesystems

[Mateusz Guzik]

I accidentally found out it is used a lot *and* is incredibly slow.

Part of it is procfs protecting the file from going away on each op,
other part is content generatin being dog slow.

Turns out procfs did not provide an interface to mark files as
permanent. I added easiest hack I could think of to remedy the problem,
I am not going to argue how to do it.

Mateusz Guzik (2):
  proc: add a helper for marking files as permanent by external
    consumers
  fs: cache the string generated by reading /proc/filesystems

 fs/filesystems.c        | 148 +++++++++++++++++++++++++++++++++++++---
 fs/proc/generic.c       |   6 ++
 include/linux/proc_fs.h |   1 +
 3 files changed, 147 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH 1/2] proc: add a helper for marking files as permanent by external consumers

[Mateusz Guzik]

This avoids refing trips on use, added with VFS in mind but should be
helpful elsewhere as well.

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
 fs/proc/generic.c       | 6 ++++++
 include/linux/proc_fs.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index a3e22803cddf..ae86554bb271 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -668,6 +668,12 @@ struct proc_dir_entry *proc_create_single_data(const char *name, umode_t mode,
 }
 EXPORT_SYMBOL(proc_create_single_data);
 
+void proc_make_permanent(struct proc_dir_entry *de)
+{
+	pde_make_permanent(de);
+}
+EXPORT_SYMBOL(proc_make_permanent);
+
 void proc_set_size(struct proc_dir_entry *de, loff_t size)
 {
 	de->size = size;
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index ea62201c74c4..0b6fcef4099a 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -105,6 +105,7 @@ struct proc_dir_entry *proc_create_single_data(const char *name, umode_t mode,
 		int (*show)(struct seq_file *, void *), void *data);
 #define proc_create_single(name, mode, parent, show) \
 	proc_create_single_data(name, mode, parent, show, NULL)
+void proc_make_permanent(struct proc_dir_entry *);
  
 extern struct proc_dir_entry *proc_create_data(const char *, umode_t,
 					       struct proc_dir_entry *,
-- 
2.43.0

[PATCH 2/2] fs: cache the string generated by reading /proc/filesystems

[Mateusz Guzik]

It is being read surprisingly often (e.g., by mkdir, ls and even sed!).

This is lock-protected pointer chasing over a linked list to pay for
sprintf for every fs (32 on my boxen).

Instead cache the result.

While here mark the file as permanent to avoid atomic refs on each op
(which can also degrade to taking a spinlock).

open+read+close cycle single-threaded (ops/s):
before:	450929
after:	982308 (+117%)

Here the main bottleneck is memcg.

open+read+close cycle with 20 processes (ops/s):
before:	578654
after:	3163961 (+446%)

The main bottleneck *before* is spinlock acquire in procfs eliminated by
marking the file as permanent. The main bottleneck *after* is the
spurious lockref trip on open.

The file looks like a sterotypical C from the 90s, right down to an
open-code and slightly obfuscated linked list. I intentionally did not
clean up any of it -- I think the file will be best served by a Rust
rewrite when the time comes.

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
 fs/filesystems.c | 148 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 140 insertions(+), 8 deletions(-)

diff --git a/fs/filesystems.c b/fs/filesystems.c
index 58b9067b2391..5d4649fd8788 100644
--- a/fs/filesystems.c
+++ b/fs/filesystems.c
@@ -34,6 +34,23 @@
 static struct file_system_type *file_systems;
 static DEFINE_RWLOCK(file_systems_lock);
 
+#ifdef CONFIG_PROC_FS
+static unsigned long file_systems_gen;
+
+struct file_systems_string {
+	struct rcu_head rcufree;
+	unsigned long gen;
+	size_t len;
+	char string[];
+};
+static struct file_systems_string *file_systems_string;
+static void invalidate_filesystems_string(void);
+#else
+static void invalidate_filesystems_string(void)
+{
+}
+#endif
+
 /* WARNING: This can be used only if we _already_ own a reference */
 struct file_system_type *get_filesystem(struct file_system_type *fs)
 {
@@ -83,10 +100,12 @@ int register_filesystem(struct file_system_type * fs)
 		return -EBUSY;
 	write_lock(&file_systems_lock);
 	p = find_filesystem(fs->name, strlen(fs->name));
-	if (*p)
+	if (*p) {
 		res = -EBUSY;
-	else
+	} else {
 		*p = fs;
+		invalidate_filesystems_string();
+	}
 	write_unlock(&file_systems_lock);
 	return res;
 }
@@ -115,6 +134,7 @@ int unregister_filesystem(struct file_system_type * fs)
 		if (fs == *tmp) {
 			*tmp = fs->next;
 			fs->next = NULL;
+			invalidate_filesystems_string();
 			write_unlock(&file_systems_lock);
 			synchronize_rcu();
 			return 0;
@@ -234,25 +254,137 @@ int __init list_bdev_fs_names(char *buf, size_t size)
 }
 
 #ifdef CONFIG_PROC_FS
-static int filesystems_proc_show(struct seq_file *m, void *v)
+/*
+ * The fs list gets queried a lot by userspace, including rather surprising
+ * programs (would you guess *sed* is on the list?). In order to reduce the
+ * overhead we cache the resulting string, which normally hangs around below
+ * 512 bytes in size.
+ *
+ * As the list almost never changes, its creation is not particularly optimized
+ * for simplicity.
+ *
+ * We sort it out on read in order to not introduce a failure point for fs
+ * registration (in principle we may be unable to alloc memory for the list).
+ */
+static void invalidate_filesystems_string(void)
 {
-	struct file_system_type * tmp;
+	struct file_systems_string *fss;
 
-	read_lock(&file_systems_lock);
+	lockdep_assert_held_write(&file_systems_lock);
+	file_systems_gen++;
+	fss = file_systems_string;
+	WRITE_ONCE(file_systems_string, NULL);
+	kfree_rcu(fss, rcufree);
+}
+
+static noinline int regen_filesystems_string(void)
+{
+	struct file_system_type *tmp;
+	struct file_systems_string *old, *new;
+	size_t newlen, usedlen;
+	unsigned long gen;
+
+retry:
+	lockdep_assert_not_held(&file_systems_lock);
+
+	newlen = 0;
+	write_lock(&file_systems_lock);
+	gen = file_systems_gen;
+	tmp = file_systems;
+	/* pre-calc space for "%s\t%s\n" for each fs */
+	while (tmp) {
+		if (!(tmp->fs_flags & FS_REQUIRES_DEV))
+			newlen += strlen("nodev");
+		newlen += strlen("\t");
+		newlen += strlen(tmp->name);
+		newlen += strlen("\n");
+		tmp = tmp->next;
+	}
+	write_unlock(&file_systems_lock);
+
+	new = kmalloc(offsetof(struct file_systems_string, string) + newlen + 1,
+		      GFP_KERNEL);
+	if (!new)
+		return -ENOMEM;
+
+	new->gen = gen;
+	new->len = newlen;
+	new->string[newlen] = '\0';
+	write_lock(&file_systems_lock);
+	old = file_systems_string;
+
+	/*
+	 * Did someone beat us to it?
+	 */
+	if (old && old->gen == file_systems_gen) {
+		write_unlock(&file_systems_lock);
+		kfree(new);
+		return 0;
+	}
+
+	/*
+	 * Did the list change in the meantime?
+	 */
+	if (gen != file_systems_gen) {
+		write_unlock(&file_systems_lock);
+		kfree(new);
+		goto retry;
+	}
+
+	/*
+	 * Populated the string.
+	 *
+	 * We know we have just enough space because we calculated the right
+	 * size the previous time we had the lock and confirmed the list has
+	 * not changed after reacquiring it.
+	 */
+	usedlen = 0;
 	tmp = file_systems;
 	while (tmp) {
-		seq_printf(m, "%s\t%s\n",
+		usedlen += sprintf(&new->string[usedlen], "%s\t%s\n",
 			(tmp->fs_flags & FS_REQUIRES_DEV) ? "" : "nodev",
 			tmp->name);
 		tmp = tmp->next;
 	}
-	read_unlock(&file_systems_lock);
+	BUG_ON(new->len != strlen(new->string));
+
+	/*
+	 * Pairs with consume fence in READ_ONCE() in filesystems_proc_show().
+	 */
+	smp_store_release(&file_systems_string, new);
+	write_unlock(&file_systems_lock);
+	kfree_rcu(old, rcufree);
 	return 0;
 }
 
+static int filesystems_proc_show(struct seq_file *m, void *v)
+{
+	struct file_systems_string *fss;
+
+	for (;;) {
+		scoped_guard(rcu) {
+			/*
+			 * Pairs with smp_store_release() in regen_filesystems_string().
+			 */
+			fss = READ_ONCE(file_systems_string);
+			if (likely(fss)) {
+				seq_write(m, fss->string, fss->len);
+				return 0;
+			}
+		}
+
+		int err = regen_filesystems_string();
+		if (unlikely(err))
+			return err;
+	}
+}
+
 static int __init proc_filesystems_init(void)
 {
-	proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
+	struct proc_dir_entry *pde;
+
+	pde = proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
+	proc_make_permanent(pde);
 	return 0;
 }
 module_init(proc_filesystems_init);
-- 
2.43.0

Re: [PATCH 2/2] fs: cache the string generated by reading /proc/filesystems

[Andreas Schwab]

On Mär 29 2025, Mateusz Guzik wrote:

> It is being read surprisingly often (e.g., by mkdir, ls and even sed!).

It is part of libselinux (selinuxfs_exits), called by its library
initializer.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH 2/2] fs: cache the string generated by reading /proc/filesystems

[Christoph Hellwig]

On Sat, Mar 29, 2025 at 09:53:16PM +0100, Andreas Schwab wrote:
> On Mär 29 2025, Mateusz Guzik wrote:
> 
> > It is being read surprisingly often (e.g., by mkdir, ls and even sed!).
> 
> It is part of libselinux (selinuxfs_exits), called by its library
> initializer.

Can we please fix libselinux instead of working around this really
broken behavior in the kernel?

Re: [PATCH 2/2] fs: cache the string generated by reading /proc/filesystems

[Mateusz Guzik]

On Fri, Apr 4, 2025 at 10:41 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Sat, Mar 29, 2025 at 09:53:16PM +0100, Andreas Schwab wrote:
> > On Mär 29 2025, Mateusz Guzik wrote:
> >
> > > It is being read surprisingly often (e.g., by mkdir, ls and even sed!).
> >
> > It is part of libselinux (selinuxfs_exits), called by its library
> > initializer.
>
> Can we please fix libselinux instead of working around this really
> broken behavior in the kernel?
>

That's a fair point, I'm going to ask them about it. If they fix it
then I'll probably self-NAK the patch.

-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH 2/2] fs: cache the string generated by reading /proc/filesystems

[Mateusz Guzik]

On Sat, Apr 5, 2025 at 6:55 AM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> On Fri, Apr 4, 2025 at 10:41 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Sat, Mar 29, 2025 at 09:53:16PM +0100, Andreas Schwab wrote:
> > > On Mär 29 2025, Mateusz Guzik wrote:
> > >
> > > > It is being read surprisingly often (e.g., by mkdir, ls and even sed!).
> > >
> > > It is part of libselinux (selinuxfs_exits), called by its library
> > > initializer.
> >
> > Can we please fix libselinux instead of working around this really
> > broken behavior in the kernel?
> >
>
> That's a fair point, I'm going to ask them about it. If they fix it
> then I'll probably self-NAK the patch.
>

for interested parties: https://github.com/SELinuxProject/selinux/issues/468

-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH 2/2] fs: cache the string generated by reading /proc/filesystems

[Christian Brauner]

On Sat, Mar 29, 2025 at 08:28:21PM +0100, Mateusz Guzik wrote:
> It is being read surprisingly often (e.g., by mkdir, ls and even sed!).
> 
> This is lock-protected pointer chasing over a linked list to pay for
> sprintf for every fs (32 on my boxen).
> 
> Instead cache the result.
> 
> While here mark the file as permanent to avoid atomic refs on each op
> (which can also degrade to taking a spinlock).
> 
> open+read+close cycle single-threaded (ops/s):
> before:	450929
> after:	982308 (+117%)
> 
> Here the main bottleneck is memcg.
> 
> open+read+close cycle with 20 processes (ops/s):
> before:	578654
> after:	3163961 (+446%)
> 
> The main bottleneck *before* is spinlock acquire in procfs eliminated by
> marking the file as permanent. The main bottleneck *after* is the
> spurious lockref trip on open.
> 
> The file looks like a sterotypical C from the 90s, right down to an
> open-code and slightly obfuscated linked list. I intentionally did not
> clean up any of it -- I think the file will be best served by a Rust
> rewrite when the time comes.
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> ---

> +	pde = proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
> +	proc_make_permanent(pde);
>  	return 0;

This all looks good enough for me especially if it's really that much of
a bottleneck for some workloads. But the above part is broken because
proc_create_single() may return NULL afair and that means
proc_make_permanent()->pde_make_permanent() will NULL-deref.

I'll fix that up locally.

Re: [PATCH 2/2] fs: cache the string generated by reading /proc/filesystems

[Mateusz Guzik]

On Tue, Apr 1, 2025 at 12:31 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Sat, Mar 29, 2025 at 08:28:21PM +0100, Mateusz Guzik wrote:
> > It is being read surprisingly often (e.g., by mkdir, ls and even sed!).
> >
> > This is lock-protected pointer chasing over a linked list to pay for
> > sprintf for every fs (32 on my boxen).
> >
> > Instead cache the result.
> >
> > While here mark the file as permanent to avoid atomic refs on each op
> > (which can also degrade to taking a spinlock).
> >
> > open+read+close cycle single-threaded (ops/s):
> > before:       450929
> > after:        982308 (+117%)
> >
> > Here the main bottleneck is memcg.
> >
> > open+read+close cycle with 20 processes (ops/s):
> > before:       578654
> > after:        3163961 (+446%)
> >
> > The main bottleneck *before* is spinlock acquire in procfs eliminated by
> > marking the file as permanent. The main bottleneck *after* is the
> > spurious lockref trip on open.
> >
> > The file looks like a sterotypical C from the 90s, right down to an
> > open-code and slightly obfuscated linked list. I intentionally did not
> > clean up any of it -- I think the file will be best served by a Rust
> > rewrite when the time comes.
> >
> > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> > ---
>
> > +     pde = proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
> > +     proc_make_permanent(pde);
> >       return 0;
>
> This all looks good enough for me especially if it's really that much of
> a bottleneck for some workloads. But the above part is broken because
> proc_create_single() may return NULL afair and that means
> proc_make_permanent()->pde_make_permanent() will NULL-deref.
>
> I'll fix that up locally.

oh huh, indeed my bad. But in that case it should perhaps complain? WARN_ON?

I would argue if this fails then something really went wrong.

-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH 0/2] speed up /proc/filesystems

[Christian Brauner]

On Sat, 29 Mar 2025 20:28:19 +0100, Mateusz Guzik wrote:
> I accidentally found out it is used a lot *and* is incredibly slow.
> 
> Part of it is procfs protecting the file from going away on each op,
> other part is content generatin being dog slow.
> 
> Turns out procfs did not provide an interface to mark files as
> permanent. I added easiest hack I could think of to remedy the problem,
> I am not going to argue how to do it.
> 
> [...]

Applied to the vfs-6.16.procfs branch of the vfs/vfs.git tree.
Patches in the vfs-6.16.procfs branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.16.procfs

[1/2] proc: add a helper for marking files as permanent by external consumers
      https://git.kernel.org/vfs/vfs/c/6040503a448b
[2/2] fs: cache the string generated by reading /proc/filesystems
      https://git.kernel.org/vfs/vfs/c/9750cdeb327d

Re: [PATCH 1/2] proc: add a helper for marking files as permanent by external consumers

[Alexey Dobriyan]

> +void proc_make_permanent(struct proc_dir_entry *de)
> +{
> + pde_make_permanent(de);
> +}
> +EXPORT_SYMBOL(proc_make_permanent);

no, no, no, no

this is wrong!

marking should be done in the context of a module!

the reason it is not exported is because the aren't safeguards against
module misuse

the flag is supposed to be used in case where
a) PDE itself is never removed and,
b) all the code supporting is never removed,
so that locking can be skipped

this it fine to mark /proc/filesystems because kernel controls it

this is fine to mark /proc/aaa if all module does is to write some
info to it and deletes it during rmmod

but it is not fine to mark /proc/aaa/bbb if "bbb" is created/deleted
while module is running,
locking _must_ be done in this case

Re: [PATCH 1/2] proc: add a helper for marking files as permanent by external consumers

[Mateusz Guzik]

On Tue, Apr 1, 2025 at 1:14 PM Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> > +void proc_make_permanent(struct proc_dir_entry *de)
> > +{
> > + pde_make_permanent(de);
> > +}
> > +EXPORT_SYMBOL(proc_make_permanent);
>
> no, no, no, no
>
> this is wrong!
>
> marking should be done in the context of a module!
>
> the reason it is not exported is because the aren't safeguards against
> module misuse
>
> the flag is supposed to be used in case where
> a) PDE itself is never removed and,
> b) all the code supporting is never removed,
> so that locking can be skipped
>
> this it fine to mark /proc/filesystems because kernel controls it
>
> this is fine to mark /proc/aaa if all module does is to write some
> info to it and deletes it during rmmod
>
> but it is not fine to mark /proc/aaa/bbb if "bbb" is created/deleted
> while module is running,
> locking _must_ be done in this case

Well I'm unhappy to begin with, but did not want to do anything
churn-inducing. The above looks like a minimal solution to me.

The pde_ marking things are in an internal header and I did not want
to move them around.

If anything I'm surprised there is no mechanism to get this done (I
assumed there would be a passable flags argument, but got nothing).

What I need here is that /proc/filesystems thing sorted out, as in this call:
> proc_create_single("filesystems", 0, NULL, filesystems_proc_show);

Would you be ok with adding proc_create_single_permanent() which hides
the logic and is not exported to modules?

-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH 1/2] proc: add a helper for marking files as permanent by external consumers

[Mateusz Guzik]

On Tue, Apr 1, 2025 at 2:17 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> On Tue, Apr 1, 2025 at 1:14 PM Alexey Dobriyan <adobriyan@gmail.com> wrote:
> >
> > > +void proc_make_permanent(struct proc_dir_entry *de)
> > > +{
> > > + pde_make_permanent(de);
> > > +}
> > > +EXPORT_SYMBOL(proc_make_permanent);
> >
> > no, no, no, no
> >
> > this is wrong!
> >
> > marking should be done in the context of a module!
> >
> > the reason it is not exported is because the aren't safeguards against
> > module misuse
> >
> > the flag is supposed to be used in case where
> > a) PDE itself is never removed and,
> > b) all the code supporting is never removed,
> > so that locking can be skipped
> >
> > this it fine to mark /proc/filesystems because kernel controls it
> >
> > this is fine to mark /proc/aaa if all module does is to write some
> > info to it and deletes it during rmmod
> >
> > but it is not fine to mark /proc/aaa/bbb if "bbb" is created/deleted
> > while module is running,
> > locking _must_ be done in this case
>
> Well I'm unhappy to begin with

unhappy with the API :)

> but did not want to do anything
> churn-inducing. The above looks like a minimal solution to me.
>
> The pde_ marking things are in an internal header and I did not want
> to move them around.
>
> If anything I'm surprised there is no mechanism to get this done (I
> assumed there would be a passable flags argument, but got nothing).
>
> What I need here is that /proc/filesystems thing sorted out, as in this call:
> > proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
>
> Would you be ok with adding proc_create_single_permanent() which hides
> the logic and is not exported to modules?
>

This still does not add a 'flags' argument, but given limited number
of consumers perhaps it is fine?

I'm not going to push for any specific solution as long as
/proc/filesystems gets to shed the overhead.

If you don't like the idea of proc_create_single_permanent(), then
perhaps it would be least back-and-forth inducing if you did whatever
change which you think is fine and then I just use it? There is
absolutely no rush.
-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH 1/2] proc: add a helper for marking files as permanent by external consumers

[Alexey Dobriyan]

>  static int __init proc_filesystems_init(void)
>  {
> - proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
> + struct proc_dir_entry *pde;
> +
> + pde = proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
> + proc_make_permanent(pde);

The only function which should be used is pde_make_permanent()
so that the flag is silently turned off when the code is modular.

Code is fine as-is in this very case but when people start copying it
to real modules
they will start mark PDEs as permanent when it is not true!