On Sat, Nov 13, 2021 at 11:49 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > On Fri, Nov 12, 2021 at 6:39 PM Jan Kara <jack@xxxxxxx> 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@xxxxxxxxx> 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 > Err, I tried the FAN_RENAME approach and hit a semantic issue: Users can watch a directory inode and get only MOVED_FROM when entries are moved from this directory. Same for MOVED_TO. How would FAN_RENAME behave when setting FAN_RENAME on a directory inode? Should listeners get events on files renamed in and out of that directory? I see several options: 1. Go back to FAN_MOVED_FROM as in this patch set, where semantics are clear 2. Report FAN_RENAME if either old or new dir is watched (or mount/sb) 3. Report FAN_RENAME only if both old and new dirs are watched (or mount/sb) For option 2, we may need to hide information records, For example, because an unprivileged listener may not have access to old or new directory. A variant of option 3, is that FAN_RENAME will be an event mask flag that can be added to FAN_MOVE events, to request that if both FROM/TO events are going to be reported, then a single joint event will be reported instead, e.g: #define FAN_MOVE (FAN_MOVED_FROM | FAN_MOVED_TO) #define FAN_RENAME (FAN_MOVE | __FAN_MOVE_JOIN) Instead of generating an extra FS_RENAME event in fsnotify_move(), fsnotify() will search for matching marks on the moved->d_parent->d_inode of MOVED_FROM event add the mark as the FSNOTIFY_OBJ_TYPE_PARENT mark iterator type and then fanotify_group_event_mask() will be able to tell if the event should be reported as FAN_MOVED_FROM, FAN_MOVED_TO or a joint FAN_RENAME. If a group has the FAN_RENAME mask on the new parent dir, then FS_MOVED_TO events can be dropped, because the event was already reported as FAN_MOVED_TO or FAN_RENAME with the FS_MOVED_FROM event. Am I over complicating this? Do you have a better and clearer semantics to propose? Thanks, Amir.
