On Wed, Jun 22, 2022 at 6:52 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>
>
> Looks mostly good to me. Just one question / suggestion: You are
> introducing helpers fsnotify_ignore_mask() and fsnotify_ignored_events().
> So shouldn't we be using these helpers as much as possible throughout the
> code? Because in several places I had to check the code around whether
> using mark->ignore_mask directly is actually fine. In particular:
>
> > @@ -315,19 +316,23 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> > return 0;
> > } else if (!(fid_mode & FAN_REPORT_FID)) {
> > /* Do we have a directory inode to report? */
> > - if (!dir && !(event_mask & FS_ISDIR))
> > + if (!dir && !ondir)
> > return 0;
> > }
> >
> > fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
> > - /* Apply ignore mask regardless of mark's ISDIR flag */
> > - marks_ignored_mask |= mark->ignored_mask;
> > + /*
> > + * Apply ignore mask depending on whether FAN_ONDIR flag in
> > + * ignore mask should be checked to ignore events on dirs.
> > + */
> > + if (!ondir || fsnotify_ignore_mask(mark) & FAN_ONDIR)
> > + marks_ignore_mask |= mark->ignore_mask;
> >
> > /*
> > * If the event is on dir and this mark doesn't care about
> > * events on dir, don't send it!
> > */
> > - if (event_mask & FS_ISDIR && !(mark->mask & FS_ISDIR))
> > + if (ondir && !(mark->mask & FAN_ONDIR))
> > continue;
> >
> > marks_mask |= mark->mask;
>
> So for example here I'm wondering whether a helper should not be used...
>
> > @@ -336,7 +341,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> > *match_mask |= 1U << type;
> > }
> >
> > - test_mask = event_mask & marks_mask & ~marks_ignored_mask;
> > + test_mask = event_mask & marks_mask & ~marks_ignore_mask;
>
> Especially because here if say FAN_EVENT_ON_CHILD becomes a part of
> marks_ignore_mask it can result in clearing this flag in the returned
> 'mask' which is likely not what we want if there are some events left
> unignored in the 'mask'?
>
> > @@ -344,14 +344,16 @@ static int send_to_group(__u32 mask, const void *data, int data_type,
> > fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
> > group = mark->group;
> > marks_mask |= mark->mask;
> > - marks_ignored_mask |= mark->ignored_mask;
> > + if (!(mask & FS_ISDIR) ||
> > + (fsnotify_ignore_mask(mark) & FS_ISDIR))
> > + marks_ignore_mask |= mark->ignore_mask;
> > }
> >
> > - pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignored_mask=%x data=%p data_type=%d dir=%p cookie=%d\n",
> > - __func__, group, mask, marks_mask, marks_ignored_mask,
> > + pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignore_mask=%x data=%p data_type=%d dir=%p cookie=%d\n",
> > + __func__, group, mask, marks_mask, marks_ignore_mask,
> > data, data_type, dir, cookie);
> >
> > - if (!(test_mask & marks_mask & ~marks_ignored_mask))
> > + if (!(test_mask & marks_mask & ~marks_ignore_mask))
> > return 0;
>
> And I'm wondering about similar things here...
>
I can't remember if I left those cases on purpose.
I will check if it makes sense to use a macro here.
Amir.
