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

On Fri, Nov 12, 2021 at 6:39 PM Jan Kara [off-list ref] wrote:

Hi Amir!

On Sat 06-11-21 18:29:39, Amir Goldstein wrote:

quoted

On Fri, Oct 29, 2021 at 2:40 PM Amir Goldstein [off-list ref] wrote:

quoted

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 ;)

quoted

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.