Re: [PATCH 2/2] fanotify: introduce FAN_MARK_IGNORE

[Amir Goldstein <amir73il@gmail.com>]

On Thu, Jun 23, 2022 at 1:14 PM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 20-06-22 16:45:51, Amir Goldstein wrote:
> > This flag is a new way to configure ignore mask which allows adding and
> > removing the event flags FAN_ONDIR and FAN_EVENT_ON_CHILD in ignore mask.
> >
> > The legacy FAN_MARK_IGNORED_MASK flag would always ignore events on
> > directories and would ignore events on children depending on whether
> > the FAN_EVENT_ON_CHILD flag was set in the (non ignored) mask.
> >
> > FAN_MARK_IGNORE can be used to ignore events on children without setting
> > FAN_EVENT_ON_CHILD in the mark's mask and will not ignore events on
> > directories unconditionally, only when FAN_ONDIR is set in ignore mask.
> >
> > The new behavior is sticky.  After calling fanotify_mark() with
> > FAN_MARK_IGNORE once, calling fanotify_mark() with FAN_MARK_IGNORED_MASK
> > will update the ignore mask, but will not change the event flags in
> > ignore mask nor how these flags are treated.
>
> IMHO this stickyness is not very obvious. Wouldn't it be less error-prone
> for users to say that once FAN_MARK_IGNORE is used for a mark, all
> subsequent modifications of ignore mask have to use FAN_MARK_IGNORE? I mean
> if some program bothers with FAN_MARK_IGNORE, I'd expect it to use it for
> all its calls as otherwise the mixup is kind of difficult to reason
> about...

I like that.

>
> Also it follows the behavior we have picked for FAN_MARK_EVICTABLE AFAIR
> but that's not really important to me.

It's kind of the opposite in the case of FAN_MARK_EVICTABLE.
FAN_MARK_EVICTABLE can be "upgraded" no non-evictable
but not the other way around.
And also with FAN_MARK_EVICTABLE we do not deprecate the
old API...

See man page draft:
https://github.com/amir73il/man-pages/commit/58851140bbc08b9ab9c7edd8830f37cf883d8d2a#diff-7a4387558a34e18ed6fb13d31933b2e4506496f8b3dd55df700f62b258e6f004R165

>
> > @@ -1591,10 +1601,20 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
> >
> >       /*
> >        * Event flags (FAN_ONDIR, FAN_EVENT_ON_CHILD) have no effect with
> > -      * FAN_MARK_IGNORED_MASK.
> > +      * FAN_MARK_IGNORED_MASK.  They can be updated in ignore mask with
> > +      * FAN_MARK_IGNORE and then they do take effect.
> >        */
> > -     if (ignore)
> > +     switch (ignore) {
> > +     case 0:
> > +     case FAN_MARK_IGNORE:
> > +             break;
> > +     case FAN_MARK_IGNORED_MASK:
> >               mask &= ~FANOTIFY_EVENT_FLAGS;
> > +             umask = FANOTIFY_EVENT_FLAGS;
> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
>
> I think this would be easier to follow as two ifs:
>
>         /* We don't allow FAN_MARK_IGNORE & FAN_MARK_IGNORED_MASK together */
>         if (ignore == FAN_MARK_IGNORE | FAN_MARK_IGNORED_MASK)
>                 return -EINVAL;
>         /*
>          * Event flags (FAN_ONDIR, FAN_EVENT_ON_CHILD) have no effect with
>          * FAN_MARK_IGNORED_MASK.
>          */
>         if (ignore == FAN_MARK_IGNORED_MASK) {
>                 mask &= ~FANOTIFY_EVENT_FLAGS;
>                 umask = FANOTIFY_EVENT_FLAGS;
>         }
>

Yeh that looks better.

Thanks,
Amir.