Re: [PATCH 2/7] fsnotify: support hashed notification queue

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Feb 17, 2021 at 5:42 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Feb 17, 2021 at 3:48 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 17-02-21 14:33:46, Amir Goldstein wrote:
> > > On Tue, Feb 16, 2021 at 5:02 PM Jan Kara <jack@suse.cz> wrote:
> > > > > @@ -300,10 +301,16 @@ static long inotify_ioctl(struct file *file, unsigned int cmd,
> > > > >       switch (cmd) {
> > > > >       case FIONREAD:
> > > > >               spin_lock(&group->notification_lock);
> > > > > -             list_for_each_entry(fsn_event, &group->notification_list,
> > > > > -                                 list) {
> > > > > -                     send_len += sizeof(struct inotify_event);
> > > > > -                     send_len += round_event_name_len(fsn_event);
> > > > > +             list = fsnotify_first_notification_list(group);
> > > > > +             /*
> > > > > +              * With multi queue, send_len will be a lower bound
> > > > > +              * on total events size.
> > > > > +              */
> > > > > +             if (list) {
> > > > > +                     list_for_each_entry(fsn_event, list, list) {
> > > > > +                             send_len += sizeof(struct inotify_event);
> > > > > +                             send_len += round_event_name_len(fsn_event);
> > > > > +                     }
> > > >
> > > > As I write below IMO we should enable hashed queues also for inotify (is
> > > > there good reason not to?)
> > >
> > > I see your perception of inotify_merge() is the same as mine was
> > > when I wrote a patch to support hashed queues for inotify.
> > > It is only after that I realized that inotify_merge() only ever merges
> > > with the last event and I dropped that patch.
> > > I see no reason to change this long time behavior.
> >
> > Ah, I even briefly looked at that code but didn't notice it merges only
> > with the last event. I agree that hashing for inotify doesn't make sense
> > then.
> >
> > Hum, if the hashing and merging is specific to fanotify and as we decided
> > to keep the event->list for the global event list, we could easily have the
> > hash table just in fanotify private group data and hash->next pointer in
> > fanotify private part of the event? Maybe that would even result in a more
> > compact code?
> >
>
> Maybe, I am not so sure. I will look into it.
>

I ended up doing something slightly different:
- The hash table and lists remained in fsnotify (and in a prep patch)
- event->key remains in fsnotify_event (and event->mask moved too)
- backend gets a callback insert() from fsnotify_add_event() to do it's thing
- event->next is in fanotify_event
- fanotify_insert() callback takes care of chaining all events by timeline

Hope you will like the result (pushed it to fanotify_merge branch).

Thanks,
Amir.