| Message ID | 20200716084230.30611-13-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fanotify events with name info | expand |
On Thu 16-07-20 11:42:20, Amir Goldstein wrote: > fsnotify usually calls inotify_handle_event() once for watching parent > to report event with child's name and once for watching child to report > event without child's name. > > Do the same thing with a single callback instead of two callbacks when > marks iterator contains both inode and child entries. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> Another idea for possible future cleanup here: Everybody except for fanotify cares only about inode marks and reporting both parent and child only complicates things for them (and I can imagine bugs being created by in-kernel fsnotify users because they misunderstand inode-vs-child mark types etc.). So maybe we can create another fsnotify_group operation similar to ->handle_event but with simpler signature for these simple notification handlers and send_to_group() will take care of translating the complex fsnotify() call into a sequence of these simple callbacks. Honza
On Thu, Jul 16, 2020 at 3:52 PM Jan Kara <jack@suse.cz> wrote: > > On Thu 16-07-20 11:42:20, Amir Goldstein wrote: > > fsnotify usually calls inotify_handle_event() once for watching parent > > to report event with child's name and once for watching child to report > > event without child's name. > > > > Do the same thing with a single callback instead of two callbacks when > > marks iterator contains both inode and child entries. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > Another idea for possible future cleanup here: Everybody except for > fanotify cares only about inode marks and reporting both parent and child > only complicates things for them (and I can imagine bugs being created by > in-kernel fsnotify users because they misunderstand inode-vs-child mark > types etc.). So maybe we can create another fsnotify_group operation > similar to ->handle_event but with simpler signature for these simple > notification handlers and send_to_group() will take care of translating > the complex fsnotify() call into a sequence of these simple callbacks. > Yeh we could do that. But then it's not every day that a new in-kernel fsnotify_group is added... Thanks, Amir.
On Thu 16-07-20 17:25:27, Amir Goldstein wrote: > On Thu, Jul 16, 2020 at 3:52 PM Jan Kara <jack@suse.cz> wrote: > > > > On Thu 16-07-20 11:42:20, Amir Goldstein wrote: > > > fsnotify usually calls inotify_handle_event() once for watching parent > > > to report event with child's name and once for watching child to report > > > event without child's name. > > > > > > Do the same thing with a single callback instead of two callbacks when > > > marks iterator contains both inode and child entries. > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > Another idea for possible future cleanup here: Everybody except for > > fanotify cares only about inode marks and reporting both parent and child > > only complicates things for them (and I can imagine bugs being created by > > in-kernel fsnotify users because they misunderstand inode-vs-child mark > > types etc.). So maybe we can create another fsnotify_group operation > > similar to ->handle_event but with simpler signature for these simple > > notification handlers and send_to_group() will take care of translating > > the complex fsnotify() call into a sequence of these simple callbacks. > > > > Yeh we could do that. > But then it's not every day that a new in-kernel fsnotify_group is added... Definitely. But then we often do not notice when it is added (to review the usage) or when e.g. audit decides to tweak its event mask and things suddently subtly break for it... Honza
diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c index dfd455798a1b..a65cf8c9f600 100644 --- a/fs/notify/inotify/inotify_fsnotify.c +++ b/fs/notify/inotify/inotify_fsnotify.c @@ -55,13 +55,11 @@ static int inotify_merge(struct list_head *list, return event_compare(last_event, event); } -int inotify_handle_event(struct fsnotify_group *group, u32 mask, - const void *data, int data_type, struct inode *dir, - const struct qstr *file_name, u32 cookie, - struct fsnotify_iter_info *iter_info) +static int inotify_one_event(struct fsnotify_group *group, u32 mask, + struct fsnotify_mark *inode_mark, + const struct path *path, + const struct qstr *file_name, u32 cookie) { - const struct path *path = fsnotify_data_path(data, data_type); - struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info); struct inotify_inode_mark *i_mark; struct inotify_event_info *event; struct fsnotify_event *fsn_event; @@ -69,9 +67,6 @@ int inotify_handle_event(struct fsnotify_group *group, u32 mask, int len = 0; int alloc_len = sizeof(struct inotify_event_info); - if (WARN_ON(fsnotify_iter_vfsmount_mark(iter_info))) - return 0; - if ((inode_mark->mask & FS_EXCL_UNLINK) && path && d_unlinked(path->dentry)) return 0; @@ -135,6 +130,37 @@ int inotify_handle_event(struct fsnotify_group *group, u32 mask, return 0; } +int inotify_handle_event(struct fsnotify_group *group, u32 mask, + const void *data, int data_type, struct inode *dir, + const struct qstr *file_name, u32 cookie, + struct fsnotify_iter_info *iter_info) +{ + const struct path *path = fsnotify_data_path(data, data_type); + struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info); + struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info); + int ret = 0; + + if (WARN_ON(fsnotify_iter_vfsmount_mark(iter_info))) + return 0; + + /* + * Some events cannot be sent on both parent and child marks + * (e.g. IN_CREATE). Those events are always sent on inode_mark. + * For events that are possible on both parent and child (e.g. IN_OPEN), + * event is sent on inode_mark with name if the parent is watching and + * is sent on child_mark without name if child is watching. + * If both parent and child are watching, report the event with child's + * name here and report another event without child's name below. + */ + if (inode_mark) + ret = inotify_one_event(group, mask, inode_mark, path, + file_name, cookie); + if (ret || !child_mark) + return ret; + + return inotify_one_event(group, mask, child_mark, path, NULL, 0); +} + static void inotify_freeing_mark(struct fsnotify_mark *fsn_mark, struct fsnotify_group *group) { inotify_ignored_and_remove_idr(fsn_mark, group);
fsnotify usually calls inotify_handle_event() once for watching parent to report event with child's name and once for watching child to report event without child's name. Do the same thing with a single callback instead of two callbacks when marks iterator contains both inode and child entries. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/notify/inotify/inotify_fsnotify.c | 44 ++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 9 deletions(-)