Re: [PATCH 1/2] fsnotify: introduce mark type iterator

[Amir Goldstein <amir73il@gmail.com>]

On Wed, May 11, 2022 at 3:54 PM Jan Kara <jack@suse.cz> 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@suse.com>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> 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.
         */

>
> > @@ -438,6 +438,9 @@ FSNOTIFY_ITER_FUNCS(sb, SB)
> >
> >  #define fsnotify_foreach_iter_type(type) \
> >       for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++)
> > +#define fsnotify_foreach_iter_mark_type(iter, mark, type) \
> > +     for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++) \
> > +             if (!(mark = fsnotify_iter_mark(iter, type))) {} else
>
> Hum, you're really inventive here ;) I'd rather go for something a bit more
> conservative and readable like:

It's good that you are here to restrain me ;-)

>
> static inline int fsnotify_iter_step(struct fsnotify_iter_info *iter, int type,
>                                      struct fsnotify_mark **markp)
> {
>         while (type < FSNOTIFY_ITER_TYPE_COUNT) {
>                 *markp = fsnotify_iter_mark(iter, type);
>                 if (*markp)
>                         break;
>                 type++;
>         }
>         return type;
> }
>
> #define fsnotify_foreach_iter_mark_type(iter, mark, type) \
>         for (type = 0; \
>              (type = fsnotify_iter_step(iter, type, &mark)) < FSNOTIFY_ITER_TYPE_COUNT; \
>              type++)
>
>

That looks nicer.

Thanks,
Amir.