On Wed, Jan 10, 2024 at 11:08:17AM +0200, Amir Goldstein wrote:
> On Tue, Jan 9, 2024 at 10:24 PM Jens Axboe <axboe@xxxxxxxxx> wrote:
> >
> > On 1/9/24 1:12 PM, Jens Axboe wrote:
> > > On 1/9/24 12:48 PM, Amir Goldstein wrote:
> > >> Commit e43de7f0862b ("fsnotify: optimize the case of no marks of any type")
> > >> optimized the case where there are no fsnotify watchers on any of the
> > >> filesystem's objects.
> > >>
> > >> It is quite common for a system to have a single local filesystem and
> > >> it is quite common for the system to have some inotify watches on some
> > >> config files or directories, so the optimization of no marks at all is
> > >> often not in effect.
> > >>
> > >> Access event watchers are far less common, so optimizing the case of
> > >> no marks with access events could improve performance for more systems,
> > >> especially for the performance sensitive hot io path.
> > >>
> > >> Maintain a per-sb counter of objects that have marks with access
> > >> events in their mask and use that counter to optimize out the call to
> > >> fsnotify() in fsnotify access hooks.
> > >>
> > >> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > >> ---
> > >>
> > >> Jens,
> > >>
> > >> You may want to try if this patch improves performance for your workload
> > >> with SECURITY=Y and FANOTIFY_ACCESS_PERMISSIONS=Y.
> > >
> > > Ran the usual test, and this effectively removes fsnotify from the
> > > profiles, which (as per other email) is between 5-6% of CPU time. So I'd
> > > say it looks mighty appealing!
> >
> > Tried with an IRQ based workload as well, as those are always impacted
> > more by the fsnotify slowness. This patch removes ~8% of useless
> > overhead in that case, so even bigger win there.
> >
>
> Do the IRQ based workloads always go through io_req_io_end()?
> Meaning that unlike the polled io workloads, they also incur the
> overhead of the fsnotify_{modify,access}() hooks?
>
> I remember I asked you once (cannot find where) whether
> io_complete_rw_iopoll() needs the fsnotify hooks and you said that
> it is a highly specialized code path for fast io, whose users will not
> want those access/modify hooks.
>
> Considering the fact that fsnotify_{modify,access}() could just as well
> be bypassed by mmap() read/write, I fully support this reasoning.
>
> Anyway, that explains (to me) why compiling-out the fsnotify_perm()
> hooks took away all the regression that you observed in upstream,
> because I was wondering where the overhead of fsnotify_access() was.
>
> Jan,
>
> What are your thoughts about this optimization patch?
>
> My thoughts are that the optimization is clearly a win, but do we
> really want to waste a full long in super_block for counting access
> event watchers that may never exist?
Meh, not too excited about it. Couldn't you use a flag in s_fsnotify_mask?
Maybe that's what you mean below.
>
> Should we perhaps instead use a flag to say that "access watchers
> existed"?
>
> We could put s_fsnotify_access_watchers inside a struct
> fsnotify_sb_mark_connector and special case alloc/free of
> FSNOTIFY_OBJ_TYPE_SB connector.
Would it make sense without incurring performance impact to move
atomic_long_t s_fsnotify_connectors and your new atomic_long_t into a
struct fsnotify_data that gets allocated when an sb is created? Then we
don't waste space in struct super_block. This is basically a copy-pasta
of the LSM sb->s_security approach.
