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: I looked at the code and the only two cases I found were the two cases that you pointed out that needed to use fsnotify_ignored_events(). > > > @@ -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... fixed. > > > @@ -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'? You are right. This can end up clearing FAN_ONDIR and then we won't report it. However, take a look at this: commit 0badfa029e5fd6d5462adb767937319335637c83 Author: Amir Goldstein <amir73il@xxxxxxxxx> Date: Thu Jul 16 11:42:09 2020 +0300 fanotify: generalize the handling of extra event flags In fanotify_group_event_mask() there is logic in place to make sure we are not going to handle an event with no type and just FAN_ONDIR flag. Generalize this logic to any FANOTIFY_EVENT_FLAGS. There is only one more flag in this group at the moment - FAN_EVENT_ON_CHILD. We never report it to user, but we do pass it in to fanotify_alloc_event() when group is reporting fid as indication that event happened on child. We will have use for this indication later on. What the hell did I mean by "We will have use for this indication later on"? fanotify_alloc_event() does not look at the FAN_EVENT_ON_CHILD flag. I think I had the idea that events reported in a group with FAN_REPORT_NAME on an inode mark should not report its parent fid+name to be compatible with inotify behavior and I think you shot this idea down, but it is only a guess. > > > @@ -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... Fixed here too, but here there is no meaning to the event flags, because test_mask masks them out. Thanks, Amir.