On Thu 18-02-21 12:56:18, Amir Goldstein wrote:
> On Wed, Feb 17, 2021 at 1:25 PM Jan Kara <jack@xxxxxxx> wrote:
> >
> > On Wed 17-02-21 12:52:21, Amir Goldstein wrote:
> > > On Tue, Feb 16, 2021 at 6:02 PM Jan Kara <jack@xxxxxxx> 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 my idea was that if 'list' is the time ordered list and permission
events are *never inserted into the hash* (we don't need them there as
hashed lists are used only for merging), then removal of permission events
is no problem.
> 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.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
