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

From: Amir Goldstein <amir73il@gmail.com>
To: Jan Kara <jack@suse.cz>
Cc: Matthew Bobrowski <mbobrowski@mbobrowski.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH 1/2] fsnotify: introduce mark type iterator
Date: Fri, 13 May 2022 06:07:28 +0300	[thread overview]
Message-ID: <CAOQ4uxiXDjf4FJ3LA+7y6TV_RstZki3AVykv_zAbz-3Jf5khEA@mail.gmail.com> (raw)
In-Reply-To: <CAOQ4uxitDOdh4mvY7JXN1hDuN3BmLg2AS=SJ+rW6Ex-K76NeXg@mail.gmail.com>

On Thu, May 12, 2022 at 8:50 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, May 12, 2022 at 8:20 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 11-05-22 21:26:16, Amir Goldstein wrote:
> > > 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.
> > >          */
> >
> > 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.