Re: [PATCH v4 15/15] fanotify: report FAN_ONDIR to listener with FAN_REPORT_FID

[Matthew Bobrowski <mbobrowski@mbobrowski.org>]

On Fri, Jan 04, 2019 at 02:39:06PM +0200, Amir Goldstein wrote:
> On Fri, Jan 4, 2019 at 2:18 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Fri 04-01-19 13:42:33, Amir Goldstein wrote:
> > > On Fri, Jan 4, 2019 at 12:57 PM Jan Kara <jack@suse.cz> wrote:
> > > > The reporting of FAN_ONDIR when the event was mkdir / rmdir could be useful
> > > > I guess. E.g. when implementing recursive watching of a directory. Or what
> > > > is your intended usecase? It should be said explicitely in the changelog.
> > >
> > > Recursive watch of directory tree is certainly a use case that could benefit
> > > from "mkdir" events. I will add that to commit message.
> >
> > Hum, so it does not seem like you had any particular usecase in mind? :)
> 
> Our application does have special handling (scanning) for mkdir events.
> 
> > Before complicating the interface with reporting FAN_ONDIR in some cases
> > I'd prefer we considered whether the usecases are worth it. So let me start
> > that: Reporting FAN_ONDIR can distinguish between file/directory
> > creation/deletion. That can save userspace some rescans of the changed
> > directory if it is interested only in subdirectory creation / deletion (if
> > the application is interested only in file changes it just does not set
> > FAN_ONDIR and that's it).
> >
> > Another motivation is the compatibility with inotify and it's IN_ISDIR flag
> > I guess.
> >
> > Hum, OK, I guess the complication is worth it.
> >
> 
> Let's see.
> 
> The fact that FAN_ONDIR is an "out" flag IFF FAN_REPORT_FID
> is set, is something that needs to be clarified.
> 
> The fact that reported FID refers to the modified directory in dirent events
> but FAN_ONDIR flag refers to the created/removed/renamed child is
> another thing that needs to be clarified.
> 
> Sigh! "things that need to be clarified" are things we need to avoid.
> Therefore, I am going to make another suggestion.
> 
> How about if we introduced a new flag FAN_DIRENT_ISDIR?
> Like IN_ISDIR, it is an out-only flag that cannot be requested.
> As its name suggests, it is only applicable to dirent events, so will always
> be set when a sub directory entry is created/renamed/removed.
> 
> FAN_ONDIR has no effect on dirent events, because FAN_ONDIR
> refers to the "victim" inode and the "victim" is the modified directory.
> If user specified FAN_CREATE|FAN_DELETE|FAN_MOVE, user
> wants to get notified when *directories* are modified and specifying
> FAN_ONDIR explicitly is not needed.
> 
> How about that?

I think this could work.

However to me, and if I'm understanding this correctly, it looks like we're
going down the route of adding "bonus" flags to particular events, which I
thought was something that we wanted to try and avoid in future versions of the
API? This would be so that users don't get any sudden surprises when processing
their events. It would also go against the notion of only receiving events that
were explicitly requested for by the calling process, which was something that
we discussed and implemented in a previous patch series?
 
> > > > > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> > > > > index e9d45387089f..f5f86566c277 100644
> > > > > --- a/include/linux/fanotify.h
> > > > > +++ b/include/linux/fanotify.h
> > > > > @@ -61,13 +61,16 @@
> > > > >  #define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \
> > > > >                                FAN_OPEN_EXEC_PERM)
> > > > >
> > > > > +/* Events types that may be reported from vfs */
> > > > > +#define FANOTIFY_EVENT_TYPES (FANOTIFY_EVENTS | \
> > > > > +                              FANOTIFY_PERM_EVENTS)
> > > > > +
> > > > >  /* Extra flags that may be reported with event or control handling of events */
> > > > >  #define FANOTIFY_EVENT_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR)
> > > > >
> > > > >  /* Events that may be reported to user */
> > > > > -#define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENTS | \
> > > > > -                                      FANOTIFY_PERM_EVENTS | \
> > > > > -                                      FAN_Q_OVERFLOW)
> > > > > +#define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENT_TYPES | \
> > > > > +                                      FAN_Q_OVERFLOW | FAN_ONDIR)
> > > > >
> > > > >  #define ALL_FANOTIFY_EVENT_BITS              (FANOTIFY_OUTGOING_EVENTS | \
> > > > >                                        FANOTIFY_EVENT_FLAGS)
> > > >
> > > > I don't like this renaming. FAN_ONDIR essentially becomes the same type of
> > > > thing as FAN_EVENT_ON_CHILD - i.e., an event flag. So I'd just leave these
> > > > defines as is...
> > > >
> > >
> > > Sorry. I don't understand what you mean.
> > > FAN_EVENT_ON_CHILD is not in FANOTIFY_OUTGOING_EVENTS
> > > FAN_ONDIR is in FANOTIFY_OUTGOING_EVENTS after this change.
> > > copy_event_to_user() masks out with FANOTIFY_OUTGOING_EVENTS.
> > > Do you not like the new group definition FANOTIFY_EVENT_TYPES?
> >
> > Sorry, I've got confused and thought that FAN_EVENT_ON_CHILD gets reported
> > to userspace. I don't like the FANOTIFY_EVENT_TYPES name and
> > FANOTIFY_OUTGOING_EVENTS becomes somewhat a misnomer after adding FAN_ONDIR
> > there. So how about renaming FANOTIFY_OUTGOING_EVENTS to
> > FANOTIFY_OUTGOING_MASK and have FANOTIFY_OUTGOING_EVENTS what your
> > FANOTIFY_EVENT_TYPES is?
> >
> 
> Still a bit confusing to me.
> I think that with FAN_DIRENT_ISDIR being a different flag than FAN_ONDIR
> I can do without the FANOTIFY_EVENT_TYPES group.
> something like this:
> 
> #define FANOTIFY_EVENT_IN_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR)
> #define FANOTIFY_EVENT_OUT_FLAGS (FAN_Q_OVERFLOW | FAN_DIRENT_ISDIR)
> #define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENTS | \
>                                       FANOTIFY_PERM_EVENTS | \
>                                       FANOTIFY_EVENT_OUT_FLAGS)
> 
> ...
>        test_mask = event_mask & marks_mask & ~marks_ignored_mask;
> 
>         if ((event_mask & FS_ISDIR) &&
>            (test_mask & ALL_FSNOTIFY_DIRENT_EVENTS))
>               test_mask |= FAN_DIRENT_ISDIR;
> 
>         return test_mask & FANOTIFY_OUTGOING_EVENTS;

-- 
Matthew Bobrowski