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

From: Amir Goldstein <amir73il@gmail.com>
To: Jan Kara <jack@suse.cz>
Cc: Jens Axboe <axboe@kernel.dk>,
	Christian Brauner <brauner@kernel.org>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v3] fsnotify: optimize the case of no parent watcher
Date: Wed, 14 Feb 2024 15:40:31 +0200	[thread overview]
Message-ID: <CAOQ4uxjS1NNJY0tQXRC3qo3_J4CB4xZpxJc7OCGp1236G6yNFw@mail.gmail.com> (raw)
In-Reply-To: <20240214112310.ovg2w3p6wztuslnw@quack3>

> > > > 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.
>
> OK, I think this idea is sound, I'd just be interested whether the 0-day
> bot (or somebody else) is able to see improvement with this. Otherwise why
> bother :)
>

Exactly.

> > 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.
>
> So firstly the name fsnotify_sb_connector really confuses me. I'd save
> "connector" names to fsnotify_mark_connector. Maybe fsnotify_sb_info?
>

Sure.

> Then I dislike how we have to specialcase superblock in quite a few places
> and add these wrappers and what not. This seems to be mostly caused by the
> fact that you directly embed fsnotify_mark_connector into fsnotify_sb_info.
> What if we just put fsnotify_connp_t there? I understand that this will
> mean one more pointer fetch if there are actually marks attached to the
> superblock and the event mask matches s_fsnotify_mask. But in that case we
> are likely to generate the event anyway so the cost of that compared to
> event generation is negligible?
>

I guess that can work.
I can try it and see if there are any other complications.

> And I'd allocate fsnotify_sb_info on demand from fsnotify_add_mark_locked()
> which means that we need to pass object pointer (in the form of void *)
> instead of fsnotify_connp_t to various mark adding functions (and transform
> it to fsnotify_connp_t only in fsnotify_add_mark_locked() after possibly
> setting up fsnotify_sb_info). Passing void * around is not great but it
> should be fairly limited (and actually reduces the knowledge of fsnotify
> internals outside of the fsnotify core).

Unless I am missing something, I think we only need to pass an extra sb
arg to fsnotify_add_mark_locked()? and it does not sound like a big deal.
For adding an sb mark, connp arg could be NULL, and then we get connp
from sb->fsnotify_sb_info after making sure that it is allocated.

I will get to look at it in ~2 weeks.

Thanks for the quick feedback.
Amir.