Re: [PATCH 0/7] Report more information in fanotify dirent events

[Amir Goldstein <amir73il@gmail.com>]

On Fri, Nov 12, 2021 at 6:39 PM Jan Kara <jack@suse.cz> wrote:
>
> Hi Amir!
>
> On Sat 06-11-21 18:29:39, Amir Goldstein wrote:
> > On Fri, Oct 29, 2021 at 2:40 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > This patch set follows up on the discussion on FAN_REPORT_TARGET_FID [1]
> > > from 3 months ago.
> > >
> > > With FAN_REPORT_PIDFD in 5.15 and FAN_FS_ERROR on its way to 5.16,
> > > I figured we could get an early (re)start of the discussion on
> > > FAN_REPORT_TARGET_FID towards 5.17.
> > >
> > > The added information in dirent events solves problems for my use case -
> > > It helps getting the following information in a race free manner:
> > > 1. fid of a created directory on mkdir
> > > 2. from/to path information on rename of non-dir
> > >
> > > I realize those are two different API traits, but they are close enough
> > > so I preferred not to clutter the REPORT flags space any further than it
> > > already is. The single added flag FAN_REPORT_TARGET_FID adds:
> > > 1. child fid info to CREATE/DELETE/MOVED_* events
> > > 2. new parent+name info to MOVED_FROM event
> > >
> > > Instead of going the "inotify way" and trying to join the MOVED_FROM/
> > > MOVED_TO events using a cookie, I chose to incorporate the new
> > > parent+name intomation only in the MOVED_FROM event.
> > > I made this choice for several reasons:
> > > 1. Availability of the moved dentry in the hook and event data
> > > 2. First info record is the old parent+name, like FAN_REPORT_DFID_NAME
> > > 3. Unlike, MOVED_TO, MOVED_FROM was useless for applications that use
> > >    DFID_NAME info to statat(2) the object as we suggested
> > >
> > > I chose to reduce testing complexity and require all other FID
> > > flags with FAN_REPORT_TARGET_FID and there is a convenience
> > > macro FAN_REPORT_ALL_FIDS that application can use.
> >
> > Self comment - Don't use ALL_ for macro names in uapi...
> > There are 3 comment of "Deprecated ..."  for ALL flags in fanotify.h alone...
>
> Yeah, probably the ALL_FIDS is not worth the possible confusion when we add
> another FID flag later ;)
>
> > BTW, I did not mention the FAN_RENAME event alternative proposal in this posting
> > not because I object to FAN_RENAME, just because it was simpler to implement
> > the MOVED_FROM alternative, so I thought I'll start with this proposal
> > and see how
> > it goes.
>
> I've read through all the patches and I didn't find anything wrong.
> Thinking about FAN_RENAME proposal - essentially fsnotify_move() would call
> fsnotify_name() once more with FS_RENAME event and we'd gate addition of
> second dir+name info just by FS_RENAME instead of FS_MOVED_FROM &&
> FAN_REPORT_TARGET_FID. Otherwise everything would be the same as in the
> current patch set, wouldn't it? IMHO it looks like a bit cleaner API so I'd
> lean a bit more towards that.

I grew to like FAN_RENAME better myself as well.
To make sure we are talking about the same thing:
1. FAN_RENAME always reports 2*(dirfid+name)
2. FAN_REPORT_TARGET_FID adds optional child fid record to
    CREATE/DELETE/RENAME/MOVED_TO/FROM

Thanks,
Amir.