Re: [PATCH v3] fsnotify: optimize the case of no parent watcher

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Jan 24, 2024 at 6:20 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Jan 24, 2024 at 6:08 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Tue 16-01-24 14:53:00, Amir Goldstein wrote:
> > > On Tue, Jan 16, 2024 at 2:04 PM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > On Tue 16-01-24 13:32:47, Amir Goldstein wrote:
> > > > > If parent inode is not watching, check for the event in masks of
> > > > > sb/mount/inode masks early to optimize out most of the code in
> > > > > __fsnotify_parent() and avoid calling fsnotify().
> > > > >
> > > > > Jens has reported that this optimization improves BW and IOPS in an
> > > > > io_uring benchmark by more than 10% and reduces perf reported CPU usage.
> > > > >
> > > > > before:
> > > > >
> > > > > +    4.51%  io_uring  [kernel.vmlinux]  [k] fsnotify
> > > > > +    3.67%  io_uring  [kernel.vmlinux]  [k] __fsnotify_parent
> > > > >
> > > > > after:
> > > > >
> > > > > +    2.37%  io_uring  [kernel.vmlinux]  [k] __fsnotify_parent
> > > > >
> > > > > Reported-and-tested-by: Jens Axboe <axboe@kernel.dk>
> > > > > Link: https://lore.kernel.org/linux-fsdevel/b45bd8ff-5654-4e67-90a6-aad5e6759e0b@kernel.dk/
> > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > > ---
> > > > >
> > > > > Jan,
> > > > >
> > > > > Considering that this change looks like a clear win and it actually
> > > > > the change that you suggested, I cleaned it up a bit and posting for
> > > > > your consideration.
> > > >
> > > > Agreed, I like this. What did you generate this patch against? It does not
> > > > apply on top of current Linus' tree (maybe it needs the change sitting in
> > > > VFS tree - which is fine I can wait until that gets merged)?
> > > >
> > >
> > > Yes, it is on top of Christian's vfs-fixes branch.
> >
> > Merged your improvement now (and I've split off the cleanup into a separate
> > change and dropped the creation of fsnotify_path() which seemed a bit
> > pointless with a single caller). All pushed out.
> >
>

Jan & Jens,

Although Jan has already queued this v3 patch with sufficient performance
improvement for Jens' workloads, I got a performance regression report from
kernel robot on will-it-scale microbenchmark (buffered write loop)
on my fan_pre_content patches, so I tried to improve on the existing solution.

I tried something similar to v1/v2 patches, where the sb keeps accounting
of the number of watchers for specific sub-classes of events.

I've made two major changes:
1. moved to counters into a per-sb state object fsnotify_sb_connector
    as Christian requested
2. The counters are by fanotify classes, not by specific events, so they
    can be used to answer the questions:
a) Are there any fsnotify watchers on this sb?
b) Are there any fanotify permission class listeners on this sb?
c) Are there any fanotify pre-content (a.k.a HSM) class listeners on this sb?

I think that those questions are very relevant in the real world, because
a positive answer to (b) and (c) is quite rare in the real world, so the
overhead on the permission hooks could be completely eliminated in
the common case.

If needed, we can further bisect the class counters per specific painful
events (e.g. FAN_ACCESS*), but there is no need to do that before
we see concrete benchmark results.

Jens,

If you feel like it, you can see if this branch further improves your
workloads:

https://github.com/amir73il/linux/commits/fsnotify-perf/

Jan,

Whenever you have the time, feel free to see if this is a valid direction,
if not for the perf optimization then we are going to need the
fsnotify_sb_connector container for other features as well.

Thanks!
Amir.