From: Amir Goldstein <amir73il@gmail.com> Date: 2021-10-29 11:40:38
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.
This restriction could be relaxed in the future if we have a good reason
to do so.
Since the POC branch 3 months ago, I dropped the 'sub_type' field of
the info header, because it did not add much value IMO.
Patches [2] and LTP test [3] are available on my github.
Thanks,
Amir.
[1] https://lore.kernel.org/linux-fsdevel/CAOQ4uxjYDDk00VPdWtRB1_tf+gCoPFgSQ9O0p0fGaW_JiFUUKA@mail.gmail.com/
[2] https://github.com/amir73il/linux/commits/fanotify_target_fid
[3] https://github.com/amir73il/ltp/commits/fanotify_target_fid
Amir Goldstein (7):
fsnotify: pass dentry instead of inode data for move events
fanotify: introduce group flag FAN_REPORT_TARGET_FID
fanotify: use macros to get the offset to fanotify_info buffer
fanotify: support secondary dir fh and name in fanotify_info
fanotify: record new parent and name in MOVED_FROM event
fanotify: report new parent and name in MOVED_FROM event
fanotify: enable the FAN_REPORT_TARGET_FID flag
fs/notify/fanotify/fanotify.c | 108 ++++++++++++++++++++++-----
fs/notify/fanotify/fanotify.h | 113 +++++++++++++++++++++++++----
fs/notify/fanotify/fanotify_user.c | 43 +++++++++--
include/linux/fanotify.h | 2 +-
include/linux/fsnotify.h | 7 +-
include/uapi/linux/fanotify.h | 5 ++
6 files changed, 235 insertions(+), 43 deletions(-)
--
2.33.1
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-11-06 16:29:52
On Fri, Oct 29, 2021 at 2:40 PM Amir Goldstein [off-list ref] 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.
From: Jan Kara <jack@suse.cz> Date: 2021-11-12 16:39:59
Hi Amir!
On Sat 06-11-21 18:29:39, Amir Goldstein wrote:
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 ;)
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
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-11-13 09:50:14
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.
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-11-13 19:31:39
On Sat, Nov 13, 2021 at 11:49 AM Amir Goldstein [off-list ref] wrote:
On Fri, Nov 12, 2021 at 6:39 PM Jan Kara [off-list ref] wrote:
quoted
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
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.
From: Jan Kara <jack@suse.cz> Date: 2021-11-15 10:23:43
On Sat 13-11-21 21:31:25, Amir Goldstein wrote:
On Sat, Nov 13, 2021 at 11:49 AM Amir Goldstein [off-list ref] wrote:
quoted
On Fri, Nov 12, 2021 at 6:39 PM Jan Kara [off-list ref] wrote:
quoted
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
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
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-11-15 12:23:11
quoted
quoted
quoted
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
Correct, that's what I meant.
quoted
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.
quoted
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.
quoted
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.
From: Jan Kara <jack@suse.cz> Date: 2021-11-15 14:39:04
On Mon 15-11-21 14:22:49, Amir Goldstein wrote:
quoted
quoted
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.
quoted
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 ;-)
:)
quoted
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
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-11-16 06:59:45
quoted
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.
quoted
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).
quoted
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.
From: Jan Kara <jack@suse.cz> Date: 2021-11-16 10:13:17
On Tue 16-11-21 08:59:29, Amir Goldstein wrote:
quoted
quoted
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.
quoted
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).
quoted
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
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-11-18 12:48:00
quoted
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
From: Jan Kara <jack@suse.cz> Date: 2021-11-18 16:30:01
On Thu 18-11-21 14:47:18, Amir Goldstein wrote:
quoted
quoted
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
--
Jan Kara [off-list ref]
SUSE Labs, CR
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-11-18 16:43:46
On Thu, Nov 18, 2021 at 6:29 PM Jan Kara [off-list ref] wrote:
On Thu 18-11-21 14:47:18, Amir Goldstein wrote:
quoted
quoted
quoted
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.