Re: [PATCH 0/7] Performance improvement for fanotify merge

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Feb 17, 2021 at 1:25 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 17-02-21 12:52:21, Amir Goldstein wrote:
> > On Tue, Feb 16, 2021 at 6:02 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > Hi Amir!
> > >
> > > Looking at the patches I've got one idea:
> > >
> > > Currently you have fsnotify_event like:
> > >
> > > struct fsnotify_event {
> > >         struct list_head list;
> > >         unsigned int key;
> > >         unsigned int next_bucket;
> > > };
> > >
> > > And 'list' is used for hashed queue list, next_bucket is used to simulate
> > > single queue out of all the individual lists. The option I'm considering
> > > is:
> > >
> > > struct fsnotify_event {
> > >         struct list_head list;
> > >         struct fsnotify_event *hash_next;
> > >         unsigned int key;
> > > };
> > >
> > > So 'list' would stay to be used for the single queue of events like it was
> > > before your patches. 'hash_next' would be used for list of events in the
> > > hash chain. The advantage of this scheme would be somewhat more obvious
> > > handling,
> >
> > I can agree to that.
> >
> > > also we can handle removal of permission events (they won't be
> > > hashed so there's no risk of breaking hash-chain in the middle, removal
> > > from global queue is easy as currently).
> >
> > Ok. but I do not really see a value in hashing non-permission events
> > for high priority groups, so this is not a strong argument.
>
> The reason why I thought it is somewhat beneficial is that someone might be
> using higher priority fanotify group just for watching non-permission
> events because so far the group priority makes little difference. And
> conceptually it isn't obvious (from userspace POV) why higher priority
> groups should be merging events less efficiently...
>

So I implemented your suggestion with ->next_event, but it did not
end up with being able to remove from the middle of the queue.
The thing is we know that permission events are on list #0, but what
we need to find out when removing a permission event is the previous
event in timeline order and we do not have that information.
So I stayed with hashed queue only for group priority 0.

Pushed partly tested result to fanotify_merge branch.

Will post after testing unless you have reservations.

Thanks,
Amir.