On Thu, May 12, 2022 at 8:50 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Thu, May 12, 2022 at 8:20 PM Jan Kara <jack@xxxxxxx> wrote:
> >
> > 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.
>
> Yes, you are right about that. But,
> With the 2 patches applied, fanotify09 test gets into livelock here.
> Fixing the livelock requires that fsnotify_iter_should_report_type()
> condition is removed.
>
> I am assuming that it is the next patch that introduces the livelock,
> but I cannot figure out why the comment I have written above is not true
> in the code in master.
Nevermind. I got it.
>
> Could it be that the livelock already exists but we do not have a test case
> to trigger it and the test case in fanotify09 which checks the next patch
> is just a private case?
>
No. This is because of the change of behavior in the next patch.
> In any case, even if the logic change here is not needed before the next
> patch, it would make no sense to convert this function to
> fsnotify_foreach_iter_mark_type() and change it back.
> The most that could make sense is to add this comment and remove
> fsnotify_iter_should_report_type() together with the next patch.
Yes. I will do that and better explain in the comment.
Thanks,
Amir.
