On Wed 11-05-22 21:26:16, Amir Goldstein wrote:
> On Wed, May 11, 2022 at 3:54 PM Jan Kara <jack@xxxxxxx> wrote:
> >
> > On Wed 11-05-22 12:29:13, Amir Goldstein wrote:
> > > fsnotify_foreach_iter_mark_type() is used to reduce boilerplate code
> > > of iteratating all marks of a specific group interested in an event
> > > by consulting the iterator report_mask.
> > >
> > > Use an open coded version of that iterator in fsnotify_iter_next()
> > > that collects all marks of the current iteration group without
> > > consulting the iterator report_mask.
> > >
> > > At the moment, the two iterator variants are the same, but this
> > > decoupling will allow us to exclude some of the group's marks from
> > > reporting the event, for example for event on child and inode marks
> > > on parent did not request to watch events on children.
> > >
> > > Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir")
> > > Reported-by: Jan Kara <jack@xxxxxxxx>
> > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> >
> > Mostly looks good. Two nits below.
> >
> > > /*
> > > - * Pop from iter_info multi head queue, the marks that were iterated in the
> > > + * Pop from iter_info multi head queue, the marks that belong to the group of
> > > * current iteration step.
> > > */
> > > static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
> > > {
> > > + struct fsnotify_mark *mark;
> > > int type;
> > >
> > > fsnotify_foreach_iter_type(type) {
> > > - if (fsnotify_iter_should_report_type(iter_info, type))
> > > + mark = iter_info->marks[type];
> > > + if (mark && mark->group == iter_info->current_group)
> > > iter_info->marks[type] =
> > > fsnotify_next_mark(iter_info->marks[type]);
> >
> > Wouldn't it be more natural here to use the new helper
> > fsnotify_foreach_iter_mark_type()? In principle we want to advance mark
> > types which were already reported...
>
> Took me an embarrassing amount of time to figure out why this would be wrong
> and I must have known this a few weeks ago when I wrote the patch, so
> a comment is in order:
>
> /*
> * We cannot use fsnotify_foreach_iter_mark_type() here because we
> * may need to check if next group has a mark of type X even if current
> * group did not have a mark of type X.
> */
Well, but this function is just advancing the lists for marks we have
already processed. And processed marks are exactly those set in report_mask.
So your code should be equivalent to the old one but using
fsnotify_foreach_iter_mark_type() should work as well AFAICT.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
