| Message ID | 20211029114028.569755-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Report more information in fanotify dirent events | expand |
On Fri, Oct 29, 2021 at 2:40 PM Amir Goldstein <amir73il@gmail.com> wrote: > > Jan, > > 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... 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. Thanks, Amir.
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. Honza
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.
On Sat, Nov 13, 2021 at 11:49 AM Amir Goldstein <amir73il@gmail.com> wrote: > > 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 > 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.
On Sat 13-11-21 21:31:25, Amir Goldstein wrote: > On Sat, Nov 13, 2021 at 11:49 AM Amir Goldstein <amir73il@gmail.com> wrote: > > > > 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 > > Correct, that's what I meant. > 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 Well, semantics are clear but in principle user does not have access to target dir either so the permission problems are the same as with option 2, aren't they? > 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. Good spotting. That can indeed be a problem. > 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? So from API POV I like most keeping FAN_RENAME separate from FAN_MOVED_TO & FAN_MOVED_FROM. It would be generated whenever source or target is tagged with FAN_RENAME, source info is provided if source is tagged, target info is provided when target is tagged (both are provides when both are tagged). So it is kind of like FAN_MOVED_FROM | FAN_MOVED_TO but with guaranteed merging. This looks like a clean enough and simple to explain API. Sure it duplicates FAN_MOVED_FROM & FAN_MOVED_TO a lot but I think the simplicity of the API outweights the duplication. Basically FAN_MOVED_FROM & FAN_MOVED_TO could be deprecated with this semantics of FAN_RENAME although I don't think we want to do it for compatibility reasons. Implementation-wise we have couple of options. Currently the simplest I can see is that fsnotify() would iterate marks on both source & target dirs (like we already do for inode & parent) when it handles FS_RENAME event. In fanotify_handle_event() we will decide which info to report with FAN_RENAME event based on which marks in iter_info have FS_RENAME set (luckily mount marks are out of question for rename events so it will be relatively simple). What do you think? Honza
> > > > > 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 > > > > > Correct, that's what I meant. > > > 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 > > Well, semantics are clear but in principle user does not have access to > target dir either so the permission problems are the same as with option 2, > aren't they? Correct. > > > 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. > > Good spotting. That can indeed be a problem. > > > 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? > > So from API POV I like most keeping FAN_RENAME separate from FAN_MOVED_TO & > FAN_MOVED_FROM. It would be generated whenever source or target is tagged > with FAN_RENAME, source info is provided if source is tagged, target info > is provided when target is tagged (both are provides when both are tagged). > So it is kind of like FAN_MOVED_FROM | FAN_MOVED_TO but with guaranteed > merging. This looks like a clean enough and simple to explain API. Sure it > duplicates FAN_MOVED_FROM & FAN_MOVED_TO a lot but I think the simplicity > of the API outweights the duplication. Basically FAN_MOVED_FROM & > FAN_MOVED_TO could be deprecated with this semantics of FAN_RENAME although > I don't think we want to do it for compatibility reasons. Well, not only for compatibility. The ability to request events for files moved into directory ~/inbox/ and files moved out of directory ~/outbox/ cannot be expressed with FAN_RENAME alone... > > Implementation-wise we have couple of options. Currently the simplest I can > see is that fsnotify() would iterate marks on both source & target dirs > (like we already do for inode & parent) when it handles FS_RENAME event. In Yes. I already have a WIP branch (fan_reanme) using FSNOTIFY_OBJ_TYPE_PARENT for the target dir mark. Heads up: I intend to repurpose FS_DN_RENAME, by sending FS_RENAME to ->handle_inode_event() backends only if (parent_mark == inode_mark). Duplicating FS_MOVED_FROM I can cope with, but wasting 3 flags for the same event is too much for me to bare ;-) > fanotify_handle_event() we will decide which info to report with FAN_RENAME > event based on which marks in iter_info have FS_RENAME set (luckily mount > marks are out of question for rename events so it will be relatively > simple). What do you think? I like it. However, If FAN_RENAME can have any combination of old,new,old+new info we cannot get any with a single new into type FAN_EVENT_INFO_TYPE_DFID_NAME2 (as in this posting) We can go with: #define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME 6 #define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME 7 #define FAN_EVENT_INFO_TYPE_OLD_DFID 8 #define FAN_EVENT_INFO_TYPE_NEW_DFID 9 Or we can go with: /* Sub-types common to all three fid info types */ #define FAN_EVENT_INFO_FID_OF_OLD_DIR 1 #define FAN_EVENT_INFO_FID_OF_NEW_DIR 2 struct fanotify_event_info_header { __u8 info_type; __u8 sub_type; __u16 len; }; (as in my wip branch fanotify_fid_of) We could also have FAN_RENAME require FAN_REPORT_NAME that would limit the number of info types, but I cannot find a good justification for this requirement. Any preference? Thanks, Amir.
On Mon 15-11-21 14:22:49, Amir Goldstein wrote: > > > 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? > > > > So from API POV I like most keeping FAN_RENAME separate from FAN_MOVED_TO & > > FAN_MOVED_FROM. It would be generated whenever source or target is tagged > > with FAN_RENAME, source info is provided if source is tagged, target info > > is provided when target is tagged (both are provides when both are tagged). > > So it is kind of like FAN_MOVED_FROM | FAN_MOVED_TO but with guaranteed > > merging. This looks like a clean enough and simple to explain API. Sure it > > duplicates FAN_MOVED_FROM & FAN_MOVED_TO a lot but I think the simplicity > > of the API outweights the duplication. Basically FAN_MOVED_FROM & > > FAN_MOVED_TO could be deprecated with this semantics of FAN_RENAME although > > I don't think we want to do it for compatibility reasons. > > Well, not only for compatibility. > The ability to request events for files moved into directory ~/inbox/ and files > moved out of directory ~/outbox/ cannot be expressed with FAN_RENAME > alone... If you ask for FAN_RENAME on ~/inbox, you can then filter out the "move out" events based on the information coming with the event to userspace. But I agree it requires more work in userspace to simulate FAN_MOVED_FROM. > > Implementation-wise we have couple of options. Currently the simplest I can > > see is that fsnotify() would iterate marks on both source & target dirs > > (like we already do for inode & parent) when it handles FS_RENAME event. In > > Yes. I already have a WIP branch (fan_reanme) using > FSNOTIFY_OBJ_TYPE_PARENT for the target dir mark. > > Heads up: I intend to repurpose FS_DN_RENAME, by sending FS_RENAME > to ->handle_inode_event() backends only if (parent_mark == inode_mark). > Duplicating FS_MOVED_FROM I can cope with, but wasting 3 flags for > the same event is too much for me to bare ;-) :) > > fanotify_handle_event() we will decide which info to report with FAN_RENAME > > event based on which marks in iter_info have FS_RENAME set (luckily mount > > marks are out of question for rename events so it will be relatively > > simple). What do you think? > > I like it. However, > If FAN_RENAME can have any combination of old,new,old+new info > we cannot get any with a single new into type > FAN_EVENT_INFO_TYPE_DFID_NAME2 > > (as in this posting) We could define only DFID2 and DFID_NAME2 but I agree it would be somewhat weird to have DFID_NAME2 in an event and not DFID_NAME. > We can go with: > #define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME 6 > #define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME 7 > #define FAN_EVENT_INFO_TYPE_OLD_DFID 8 > #define FAN_EVENT_INFO_TYPE_NEW_DFID 9 > > Or we can go with: > /* Sub-types common to all three fid info types */ > #define FAN_EVENT_INFO_FID_OF_OLD_DIR 1 > #define FAN_EVENT_INFO_FID_OF_NEW_DIR 2 > > struct fanotify_event_info_header { > __u8 info_type; > __u8 sub_type; > __u16 len; > }; > > (as in my wip branch fanotify_fid_of) When we went the way of having different types for FID and DFID, I'd continue with OLD_DFID_NAME, NEW_DFID_NAME, ... and keep the padding byte free for now (just in case there's some extension which would urgently need it). > We could also have FAN_RENAME require FAN_REPORT_NAME > that would limit the number of info types, but I cannot find a good > justification for this requirement. Yeah, I would not force that. Honza
> > I like it. However, > > If FAN_RENAME can have any combination of old,new,old+new info > > we cannot get any with a single new into type > > FAN_EVENT_INFO_TYPE_DFID_NAME2 > > > > (as in this posting) > > We could define only DFID2 and DFID_NAME2 but I agree it would be somewhat > weird to have DFID_NAME2 in an event and not DFID_NAME. > > > We can go with: > > #define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME 6 > > #define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME 7 > > #define FAN_EVENT_INFO_TYPE_OLD_DFID 8 > > #define FAN_EVENT_INFO_TYPE_NEW_DFID 9 > > > > Or we can go with: > > /* Sub-types common to all three fid info types */ > > #define FAN_EVENT_INFO_FID_OF_OLD_DIR 1 > > #define FAN_EVENT_INFO_FID_OF_NEW_DIR 2 > > > > struct fanotify_event_info_header { > > __u8 info_type; > > __u8 sub_type; > > __u16 len; > > }; > > > > (as in my wip branch fanotify_fid_of) > > When we went the way of having different types for FID and DFID, I'd > continue with OLD_DFID_NAME, NEW_DFID_NAME, ... and keep the padding byte > free for now (just in case there's some extension which would urgently need > it). > > > We could also have FAN_RENAME require FAN_REPORT_NAME > > that would limit the number of info types, but I cannot find a good > > justification for this requirement. > > Yeah, I would not force that. > On second thought and after trying to write a mental man page and realizing how ugly it gets, I feel strongly in favor of requiring FAN_REPORT_NAME for the FAN_RENAME event. My arguments are: 1. What is the benefit of FAN_RENAME without names? Is the knowledge that *something* was moved from dir A to dir B that important that it qualifies for the extra man page noise and application developer headache? 2. My declared motivation for this patch set was to close the last (?) functional gap between inotify and fanotify, that is, being able to reliably join MOVED_FROM and MOVED_TO events. Requiring FAN_REPORT_NAME still meets that goal. 3. In this patch set, FAN_REPORT_NAME is required (for now) for FAN_REPORT_TARGET_FID to reduce implementation and test matrix complexity (you did not object, so I wasn't planning on changing this requirement). The same argument holds for FAN_RENAME So let's say this - we can add support for OLD_DFID, NEW_DFID types later if we think they may serve a purpose, but at this time, I see no reason to complicate the UAPI anymore than it already is and I would rather implement only: /* Info types for FAN_RENAME */ #define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME 10 /* Reserved for FAN_EVENT_INFO_TYPE_OLD_DFID 11 */ #define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME 12 /* Reserved for FAN_EVENT_INFO_TYPE_NEW_DFID 13 */ Do you agree? Thanks, Amir.
On Tue 16-11-21 08:59:29, Amir Goldstein wrote: > > > I like it. However, > > > If FAN_RENAME can have any combination of old,new,old+new info > > > we cannot get any with a single new into type > > > FAN_EVENT_INFO_TYPE_DFID_NAME2 > > > > > > (as in this posting) > > > > We could define only DFID2 and DFID_NAME2 but I agree it would be somewhat > > weird to have DFID_NAME2 in an event and not DFID_NAME. > > > > > We can go with: > > > #define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME 6 > > > #define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME 7 > > > #define FAN_EVENT_INFO_TYPE_OLD_DFID 8 > > > #define FAN_EVENT_INFO_TYPE_NEW_DFID 9 > > > > > > Or we can go with: > > > /* Sub-types common to all three fid info types */ > > > #define FAN_EVENT_INFO_FID_OF_OLD_DIR 1 > > > #define FAN_EVENT_INFO_FID_OF_NEW_DIR 2 > > > > > > struct fanotify_event_info_header { > > > __u8 info_type; > > > __u8 sub_type; > > > __u16 len; > > > }; > > > > > > (as in my wip branch fanotify_fid_of) > > > > When we went the way of having different types for FID and DFID, I'd > > continue with OLD_DFID_NAME, NEW_DFID_NAME, ... and keep the padding byte > > free for now (just in case there's some extension which would urgently need > > it). > > > > > We could also have FAN_RENAME require FAN_REPORT_NAME > > > that would limit the number of info types, but I cannot find a good > > > justification for this requirement. > > > > Yeah, I would not force that. > > > > On second thought and after trying to write a mental man page > and realizing how ugly it gets, I feel strongly in favor of requiring > FAN_REPORT_NAME for the FAN_RENAME event. > > My arguments are: > 1. What is the benefit of FAN_RENAME without names? > Is the knowledge that *something* was moved from dir A to dir B > that important that it qualifies for the extra man page noise and > application developer headache? > 2. My declared motivation for this patch set was to close the last (?) > functional gap between inotify and fanotify, that is, being able to > reliably join MOVED_FROM and MOVED_TO events. > Requiring FAN_REPORT_NAME still meets that goal. > 3. In this patch set, FAN_REPORT_NAME is required (for now) for > FAN_REPORT_TARGET_FID to reduce implementation and test > matrix complexity (you did not object, so I wasn't planning on > changing this requirement). > The same argument holds for FAN_RENAME > > So let's say this - we can add support for OLD_DFID, NEW_DFID types > later if we think they may serve a purpose, but at this time, I see no > reason to complicate the UAPI anymore than it already is and I would > rather implement only: > > /* Info types for FAN_RENAME */ > #define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME 10 > /* Reserved for FAN_EVENT_INFO_TYPE_OLD_DFID 11 */ > #define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME 12 > /* Reserved for FAN_EVENT_INFO_TYPE_NEW_DFID 13 */ > > Do you agree? I agree the utility of FAN_RENAME without FAN_REPORT_NAME is very limited so I'm OK with not implementing that at least for now. Honza
> > So let's say this - we can add support for OLD_DFID, NEW_DFID types > > later if we think they may serve a purpose, but at this time, I see no > > reason to complicate the UAPI anymore than it already is and I would > > rather implement only: > > > > /* Info types for FAN_RENAME */ > > #define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME 10 > > /* Reserved for FAN_EVENT_INFO_TYPE_OLD_DFID 11 */ > > #define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME 12 > > /* Reserved for FAN_EVENT_INFO_TYPE_NEW_DFID 13 */ > > > > Do you agree? > > I agree the utility of FAN_RENAME without FAN_REPORT_NAME is very limited > so I'm OK with not implementing that at least for now. OK. The patches are staged at [1], but I ran into one more UAPI question that I wanted to run by you before posting the patches. The question may be best described by the last commit on the tests branch [2]: syscalls/fanotify16: Test FAN_RENAME with ignored mask When a file is moved between two directories and only one of them is watching for FAN_RENAME events, the FAN_RENAME event will include only the information about the entry in the watched directory. When one of the directories is watching FAN_RENAME, but the other is ignoring FAN_RENAME events, the FAN_RENAME event will not be reported at all. This is not the same behavior as MOVED_FROM/TO events. User cannot request to ignore MOVED_FROM events according to destination directory nor MOVED_TO events according to source directory. I chose this behavior because I found it to be useful and consistent with other behaviors involving ignored masks. Maybe "chose" is a strong word here - I did not do anything to implement this specific behavior - it is just how the code in fanotify_group_event_mask() works for merging masks and ignored masks of different marks. Let me know if you approve of those ignored FAN_RENAME semantics and I will post the patches for review. Thanks, Amir. [1] https://github.com/amir73il/linux/commits/fan_rename [2] https://github.com/amir73il/ltp/commits/fan_rename
On Thu 18-11-21 14:47:18, Amir Goldstein wrote: > > > So let's say this - we can add support for OLD_DFID, NEW_DFID types > > > later if we think they may serve a purpose, but at this time, I see no > > > reason to complicate the UAPI anymore than it already is and I would > > > rather implement only: > > > > > > /* Info types for FAN_RENAME */ > > > #define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME 10 > > > /* Reserved for FAN_EVENT_INFO_TYPE_OLD_DFID 11 */ > > > #define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME 12 > > > /* Reserved for FAN_EVENT_INFO_TYPE_NEW_DFID 13 */ > > > > > > Do you agree? > > > > I agree the utility of FAN_RENAME without FAN_REPORT_NAME is very limited > > so I'm OK with not implementing that at least for now. > > OK. The patches are staged at [1], but I ran into one more UAPI question > that I wanted to run by you before posting the patches. > > The question may be best described by the last commit on the tests branch [2]: > > syscalls/fanotify16: Test FAN_RENAME with ignored mask > > When a file is moved between two directories and only one of them is > watching for FAN_RENAME events, the FAN_RENAME event will include only > the information about the entry in the watched directory. > > When one of the directories is watching FAN_RENAME, but the other is > ignoring FAN_RENAME events, the FAN_RENAME event will not be reported > at all. > > This is not the same behavior as MOVED_FROM/TO events. User cannot > request to ignore MOVED_FROM events according to destination directory > nor MOVED_TO events according to source directory. > > I chose this behavior because I found it to be useful and consistent with > other behaviors involving ignored masks. Maybe "chose" is a strong word > here - I did not do anything to implement this specific behavior - it is just > how the code in fanotify_group_event_mask() works for merging masks > and ignored masks of different marks. > > Let me know if you approve of those ignored FAN_RENAME semantics > and I will post the patches for review. Yeah, I guess ignore masks with FAN_RENAME are going to be a bit non-intuitive either way and what you suggest makes sense. I suppose when SB has FAN_RENAME mark and either source or target have FAN_RENAME in the ignore mask, nothing will get reported as well, do it? If we are consistent like this, I guess fine by me. Honza
On Thu, Nov 18, 2021 at 6:29 PM Jan Kara <jack@suse.cz> wrote: > > On Thu 18-11-21 14:47:18, Amir Goldstein wrote: > > > > So let's say this - we can add support for OLD_DFID, NEW_DFID types > > > > later if we think they may serve a purpose, but at this time, I see no > > > > reason to complicate the UAPI anymore than it already is and I would > > > > rather implement only: > > > > > > > > /* Info types for FAN_RENAME */ > > > > #define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME 10 > > > > /* Reserved for FAN_EVENT_INFO_TYPE_OLD_DFID 11 */ > > > > #define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME 12 > > > > /* Reserved for FAN_EVENT_INFO_TYPE_NEW_DFID 13 */ > > > > > > > > Do you agree? > > > > > > I agree the utility of FAN_RENAME without FAN_REPORT_NAME is very limited > > > so I'm OK with not implementing that at least for now. > > > > OK. The patches are staged at [1], but I ran into one more UAPI question > > that I wanted to run by you before posting the patches. > > > > The question may be best described by the last commit on the tests branch [2]: > > > > syscalls/fanotify16: Test FAN_RENAME with ignored mask > > > > When a file is moved between two directories and only one of them is > > watching for FAN_RENAME events, the FAN_RENAME event will include only > > the information about the entry in the watched directory. > > > > When one of the directories is watching FAN_RENAME, but the other is > > ignoring FAN_RENAME events, the FAN_RENAME event will not be reported > > at all. > > > > This is not the same behavior as MOVED_FROM/TO events. User cannot > > request to ignore MOVED_FROM events according to destination directory > > nor MOVED_TO events according to source directory. > > > > I chose this behavior because I found it to be useful and consistent with > > other behaviors involving ignored masks. Maybe "chose" is a strong word > > here - I did not do anything to implement this specific behavior - it is just > > how the code in fanotify_group_event_mask() works for merging masks > > and ignored masks of different marks. > > > > Let me know if you approve of those ignored FAN_RENAME semantics > > and I will post the patches for review. > > Yeah, I guess ignore masks with FAN_RENAME are going to be a bit > non-intuitive either way and what you suggest makes sense. I suppose when > SB has FAN_RENAME mark and either source or target have FAN_RENAME in the > ignore mask, nothing will get reported as well, do it? If we are consistent > like this, I guess fine by me. Yes, that is correct, because the join of combined masks and combined ignored_masks is done at the very end, if an event is in ignored mask of any related mark, it will cause the drop of the event. I will post patches soon. Thanks, Amir.