On Thu, Jun 23, 2022 at 12:49 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Wed 22-06-22 21:28:23, Amir Goldstein wrote:
> > On Wed, Jun 22, 2022 at 7:00 PM Jan Kara <jack@xxxxxxx> wrote:
> > >
> > > On Mon 20-06-22 16:45:50, Amir Goldstein wrote:
> > > > Setting flags FAN_ONDIR FAN_EVENT_ON_CHILD in ignore mask has no effect.
> > > > The FAN_EVENT_ON_CHILD flag in mask implicitly applies to ignore mask and
> > > > ignore mask is always implicitly applied to events on directories.
> > > >
> > > > Define a mark flag that replaces this legacy behavior with logic of
> > > > applying the ignore mask according to event flags in ignore mask.
> > > >
> > > > Implement the new logic to prepare for supporting an ignore mask that
> > > > ignores events on children and ignore mask that does not ignore events
> > > > on directories.
> > > >
> > > > To emphasize the change in terminology, also rename ignored_mask mark
> > > > member to ignore_mask and use accessor to get only ignored events or
> > > > events and flags.
> > > >
> > > > This change in terminology finally aligns with the "ignore mask"
> > > > language in man pages and in most of the comments.
> > > >
> > > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > >
> > > ..
> > >
> > > > @@ -423,7 +425,8 @@ static bool fsnotify_iter_select_report_types(
> > > > * But is *this mark* watching children?
> > > > */
> > > > if (type == FSNOTIFY_ITER_TYPE_PARENT &&
> > > > - !(mark->mask & FS_EVENT_ON_CHILD))
> > > > + !(mark->mask & FS_EVENT_ON_CHILD) &&
> > > > + !(fsnotify_ignore_mask(mark) & FS_EVENT_ON_CHILD))
> > > > continue;
> > >
> > > So now we have in ->report_mask the FSNOTIFY_ITER_TYPE_PARENT if either
> > > ->mask or ->ignore_mask have FS_EVENT_ON_CHILD set. But I see nothing that
> > > would stop us from applying say ->mask to the set of events we are
> > > interested in if FS_EVENT_ON_CHILD is set only in ->ignore_mask? And
> >
> > I think I spent some time thinking about this and came to a conclusion that
> > 1. It is hard to get all the cases right
> > 2. It is a micro optimization
> >
> > The implication is that the user can set an ignore mask of an object, get no
> > events but still cause performance penalty. Right?
> > So just don't do that...
>
> So I was more afraid that this actually results in generating events we
> should not generate. For example consider dir 'd' with mask FS_OPEN and
> ignore_mask FS_MODIFY | FS_EVENT_ON_CHILD. Now open("d/file") happens so
> FS_OPEN is generated for d/file. We select FSNOTIFY_ITER_TYPE_PARENT in the
> ->report_mask because of the ignore_mask on 'd' and pass the iter to
> fanotify_handle_event(). There fanotify_group_event_mask() will include
> FS_OPEN to marks_mask and conclude event should be reported. But there's no
> mark that should result in reporting this...
>
I see... I think the FS_EVENT_ON_CHILD needs to be added to send_to_group()
just like it knows about FS_ISDIR. Will need to look into it and see which tests
we are missing.
> The problem is that with the introduction of FSNOTIFY_ITER_TYPE_PARENT we
> started to rely on that type being set only when the event on child should
> be reported to parent and now you break that AFAICT.
>
The idea behind FSNOTIFY_ITER_TYPE_PARENT is that the event handlers
have all the information to make the right decision based on mask/ignored_mask
and flags. I guess the implementation is incorrect though.
Well spotted!
Thanks,
Amir.
