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@xxxxxxxxxx> 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@xxxxxxxxx>
> > ---
>
> > +     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>