| Message ID | 20201202120713.702387-3-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fsnotify fixes and cleanups | expand |
On Wed 02-12-20 14:07:08, Amir Goldstein wrote: > Convert inotify to use the simple handle_inode_event() interface to > get rid of the code duplication between the generic helper > fsnotify_handle_event() and the inotify_handle_event() callback, which > also happen to be buggy code. > > The bug will be fixed in the generic helper. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/notify/inotify/inotify.h | 9 +++--- > fs/notify/inotify/inotify_fsnotify.c | 47 ++++++---------------------- > fs/notify/inotify/inotify_user.c | 7 +---- > 3 files changed, 15 insertions(+), 48 deletions(-) > > diff --git a/fs/notify/inotify/inotify.h b/fs/notify/inotify/inotify.h > index 4327d0e9c364..7fc3782b2fb8 100644 > --- a/fs/notify/inotify/inotify.h > +++ b/fs/notify/inotify/inotify.h > @@ -24,11 +24,10 @@ static inline struct inotify_event_info *INOTIFY_E(struct fsnotify_event *fse) > > extern void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark, > struct fsnotify_group *group); > -extern 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); > +extern int inotify_handle_event(struct fsnotify_group *group, > + struct fsnotify_mark *inode_mark, u32 mask, > + struct inode *inode, struct inode *dir, > + const struct qstr *name, u32 cookie); > > extern const struct fsnotify_ops inotify_fsnotify_ops; > extern struct kmem_cache *inotify_inode_mark_cachep; > diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c > index 9ddcbadc98e2..f348c1d3b358 100644 > --- a/fs/notify/inotify/inotify_fsnotify.c > +++ b/fs/notify/inotify/inotify_fsnotify.c > @@ -55,10 +55,10 @@ static int inotify_merge(struct list_head *list, > return event_compare(last_event, event); > } > > -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) > +int inotify_handle_event(struct fsnotify_group *group, > + struct fsnotify_mark *inode_mark, u32 mask, > + struct inode *inode, struct inode *dir, > + const struct qstr *file_name, u32 cookie) > { > struct inotify_inode_mark *i_mark; > struct inotify_event_info *event; > @@ -68,10 +68,6 @@ static int inotify_one_event(struct fsnotify_group *group, u32 mask, > int alloc_len = sizeof(struct inotify_event_info); > struct mem_cgroup *old_memcg; > > - if ((inode_mark->mask & FS_EXCL_UNLINK) && > - path && d_unlinked(path->dentry)) > - return 0; > - > if (file_name) { > len = file_name->len; > alloc_len += len + 1; > @@ -131,35 +127,12 @@ static int inotify_one_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) > +static int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask, > + struct inode *inode, struct inode *dir, > + const struct qstr *name, u32 cookie) > { Looking at this I'd just fold inotify_handle_event() into inotify_handle_inode_event() as the only difference is the 'group' argument and that can be always fetched from inode_mark->group... Honza
On Thu, Dec 3, 2020 at 11:55 AM Jan Kara <jack@suse.cz> wrote: > > On Wed 02-12-20 14:07:08, Amir Goldstein wrote: > > Convert inotify to use the simple handle_inode_event() interface to > > get rid of the code duplication between the generic helper > > fsnotify_handle_event() and the inotify_handle_event() callback, which > > also happen to be buggy code. > > > > The bug will be fixed in the generic helper. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > fs/notify/inotify/inotify.h | 9 +++--- > > fs/notify/inotify/inotify_fsnotify.c | 47 ++++++---------------------- > > fs/notify/inotify/inotify_user.c | 7 +---- > > 3 files changed, 15 insertions(+), 48 deletions(-) > > > > diff --git a/fs/notify/inotify/inotify.h b/fs/notify/inotify/inotify.h > > index 4327d0e9c364..7fc3782b2fb8 100644 > > --- a/fs/notify/inotify/inotify.h > > +++ b/fs/notify/inotify/inotify.h > > @@ -24,11 +24,10 @@ static inline struct inotify_event_info *INOTIFY_E(struct fsnotify_event *fse) > > > > extern void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark, > > struct fsnotify_group *group); > > -extern 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); > > +extern int inotify_handle_event(struct fsnotify_group *group, > > + struct fsnotify_mark *inode_mark, u32 mask, > > + struct inode *inode, struct inode *dir, > > + const struct qstr *name, u32 cookie); > > > > extern const struct fsnotify_ops inotify_fsnotify_ops; > > extern struct kmem_cache *inotify_inode_mark_cachep; > > diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c > > index 9ddcbadc98e2..f348c1d3b358 100644 > > --- a/fs/notify/inotify/inotify_fsnotify.c > > +++ b/fs/notify/inotify/inotify_fsnotify.c > > @@ -55,10 +55,10 @@ static int inotify_merge(struct list_head *list, > > return event_compare(last_event, event); > > } > > > > -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) > > +int inotify_handle_event(struct fsnotify_group *group, > > + struct fsnotify_mark *inode_mark, u32 mask, > > + struct inode *inode, struct inode *dir, > > + const struct qstr *file_name, u32 cookie) > > { > > struct inotify_inode_mark *i_mark; > > struct inotify_event_info *event; > > @@ -68,10 +68,6 @@ static int inotify_one_event(struct fsnotify_group *group, u32 mask, > > int alloc_len = sizeof(struct inotify_event_info); > > struct mem_cgroup *old_memcg; > > > > - if ((inode_mark->mask & FS_EXCL_UNLINK) && > > - path && d_unlinked(path->dentry)) > > - return 0; > > - > > if (file_name) { > > len = file_name->len; > > alloc_len += len + 1; > > @@ -131,35 +127,12 @@ static int inotify_one_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) > > +static int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask, > > + struct inode *inode, struct inode *dir, > > + const struct qstr *name, u32 cookie) > > { > > Looking at this I'd just fold inotify_handle_event() into > inotify_handle_inode_event() as the only difference is the 'group' argument > and that can be always fetched from inode_mark->group... > Yes, that is what I though, but then I wasn't sure about the call coming from inotify_ignored_and_remove_idr(). It seemed to be on purpose that the group argument is separate from the freeing mark. Thanks, Amir.
On Thu 03-12-20 12:45:15, Amir Goldstein wrote: > On Thu, Dec 3, 2020 at 11:55 AM Jan Kara <jack@suse.cz> wrote: > > > > On Wed 02-12-20 14:07:08, Amir Goldstein wrote: > > > Convert inotify to use the simple handle_inode_event() interface to > > > get rid of the code duplication between the generic helper > > > fsnotify_handle_event() and the inotify_handle_event() callback, which > > > also happen to be buggy code. > > > > > > The bug will be fixed in the generic helper. > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > fs/notify/inotify/inotify.h | 9 +++--- > > > fs/notify/inotify/inotify_fsnotify.c | 47 ++++++---------------------- > > > fs/notify/inotify/inotify_user.c | 7 +---- > > > 3 files changed, 15 insertions(+), 48 deletions(-) > > > > > > diff --git a/fs/notify/inotify/inotify.h b/fs/notify/inotify/inotify.h > > > index 4327d0e9c364..7fc3782b2fb8 100644 > > > --- a/fs/notify/inotify/inotify.h > > > +++ b/fs/notify/inotify/inotify.h > > > @@ -24,11 +24,10 @@ static inline struct inotify_event_info *INOTIFY_E(struct fsnotify_event *fse) > > > > > > extern void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark, > > > struct fsnotify_group *group); > > > -extern 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); > > > +extern int inotify_handle_event(struct fsnotify_group *group, > > > + struct fsnotify_mark *inode_mark, u32 mask, > > > + struct inode *inode, struct inode *dir, > > > + const struct qstr *name, u32 cookie); > > > > > > extern const struct fsnotify_ops inotify_fsnotify_ops; > > > extern struct kmem_cache *inotify_inode_mark_cachep; > > > diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c > > > index 9ddcbadc98e2..f348c1d3b358 100644 > > > --- a/fs/notify/inotify/inotify_fsnotify.c > > > +++ b/fs/notify/inotify/inotify_fsnotify.c > > > @@ -55,10 +55,10 @@ static int inotify_merge(struct list_head *list, > > > return event_compare(last_event, event); > > > } > > > > > > -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) > > > +int inotify_handle_event(struct fsnotify_group *group, > > > + struct fsnotify_mark *inode_mark, u32 mask, > > > + struct inode *inode, struct inode *dir, > > > + const struct qstr *file_name, u32 cookie) > > > { > > > struct inotify_inode_mark *i_mark; > > > struct inotify_event_info *event; > > > @@ -68,10 +68,6 @@ static int inotify_one_event(struct fsnotify_group *group, u32 mask, > > > int alloc_len = sizeof(struct inotify_event_info); > > > struct mem_cgroup *old_memcg; > > > > > > - if ((inode_mark->mask & FS_EXCL_UNLINK) && > > > - path && d_unlinked(path->dentry)) > > > - return 0; > > > - > > > if (file_name) { > > > len = file_name->len; > > > alloc_len += len + 1; > > > @@ -131,35 +127,12 @@ static int inotify_one_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) > > > +static int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask, > > > + struct inode *inode, struct inode *dir, > > > + const struct qstr *name, u32 cookie) > > > { > > > > Looking at this I'd just fold inotify_handle_event() into > > inotify_handle_inode_event() as the only difference is the 'group' argument > > and that can be always fetched from inode_mark->group... > > > > Yes, that is what I though, but then I wasn't sure about the call coming from > inotify_ignored_and_remove_idr(). It seemed to be on purpose that the > group argument is separate from the freeing mark. Well, the 'group' argument is just fetched from mark->group in fsnotify_free_mark() so I rather think it is a relict from the past when the lifetime rules for marks were different. In fact fsnotify_final_mark_destroy() which is called just before really freeing the memory uses mark->group. So I'm pretty sure we are safe to grab mark->group in inotify_handle_event() as well. Honza
On Thu, Dec 3, 2020 at 2:37 PM Jan Kara <jack@suse.cz> wrote: > > On Thu 03-12-20 12:45:15, Amir Goldstein wrote: > > On Thu, Dec 3, 2020 at 11:55 AM Jan Kara <jack@suse.cz> wrote: > > > > > > On Wed 02-12-20 14:07:08, Amir Goldstein wrote: > > > > Convert inotify to use the simple handle_inode_event() interface to > > > > get rid of the code duplication between the generic helper > > > > fsnotify_handle_event() and the inotify_handle_event() callback, which > > > > also happen to be buggy code. > > > > > > > > The bug will be fixed in the generic helper. > > > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > --- > > > > fs/notify/inotify/inotify.h | 9 +++--- > > > > fs/notify/inotify/inotify_fsnotify.c | 47 ++++++---------------------- > > > > fs/notify/inotify/inotify_user.c | 7 +---- > > > > 3 files changed, 15 insertions(+), 48 deletions(-) > > > > > > > > diff --git a/fs/notify/inotify/inotify.h b/fs/notify/inotify/inotify.h > > > > index 4327d0e9c364..7fc3782b2fb8 100644 > > > > --- a/fs/notify/inotify/inotify.h > > > > +++ b/fs/notify/inotify/inotify.h > > > > @@ -24,11 +24,10 @@ static inline struct inotify_event_info *INOTIFY_E(struct fsnotify_event *fse) > > > > > > > > extern void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark, > > > > struct fsnotify_group *group); > > > > -extern 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); > > > > +extern int inotify_handle_event(struct fsnotify_group *group, > > > > + struct fsnotify_mark *inode_mark, u32 mask, > > > > + struct inode *inode, struct inode *dir, > > > > + const struct qstr *name, u32 cookie); > > > > > > > > extern const struct fsnotify_ops inotify_fsnotify_ops; > > > > extern struct kmem_cache *inotify_inode_mark_cachep; > > > > diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c > > > > index 9ddcbadc98e2..f348c1d3b358 100644 > > > > --- a/fs/notify/inotify/inotify_fsnotify.c > > > > +++ b/fs/notify/inotify/inotify_fsnotify.c > > > > @@ -55,10 +55,10 @@ static int inotify_merge(struct list_head *list, > > > > return event_compare(last_event, event); > > > > } > > > > > > > > -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) > > > > +int inotify_handle_event(struct fsnotify_group *group, > > > > + struct fsnotify_mark *inode_mark, u32 mask, > > > > + struct inode *inode, struct inode *dir, > > > > + const struct qstr *file_name, u32 cookie) > > > > { > > > > struct inotify_inode_mark *i_mark; > > > > struct inotify_event_info *event; > > > > @@ -68,10 +68,6 @@ static int inotify_one_event(struct fsnotify_group *group, u32 mask, > > > > int alloc_len = sizeof(struct inotify_event_info); > > > > struct mem_cgroup *old_memcg; > > > > > > > > - if ((inode_mark->mask & FS_EXCL_UNLINK) && > > > > - path && d_unlinked(path->dentry)) > > > > - return 0; > > > > - > > > > if (file_name) { > > > > len = file_name->len; > > > > alloc_len += len + 1; > > > > @@ -131,35 +127,12 @@ static int inotify_one_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) > > > > +static int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask, > > > > + struct inode *inode, struct inode *dir, > > > > + const struct qstr *name, u32 cookie) > > > > { > > > > > > Looking at this I'd just fold inotify_handle_event() into > > > inotify_handle_inode_event() as the only difference is the 'group' argument > > > and that can be always fetched from inode_mark->group... > > > > > > > Yes, that is what I though, but then I wasn't sure about the call coming from > > inotify_ignored_and_remove_idr(). It seemed to be on purpose that the > > group argument is separate from the freeing mark. > > Well, the 'group' argument is just fetched from mark->group in > fsnotify_free_mark() so I rather think it is a relict from the past when > the lifetime rules for marks were different. In fact Ha right! I should have checked that. > fsnotify_final_mark_destroy() which is called just before really freeing > the memory uses mark->group. So I'm pretty sure we are safe to grab > mark->group in inotify_handle_event() as well. > Unless I have other fixes to post, I'll assume you will be folding the inotify_handle_inode_event wrapper on commit. Thanks, Amir.
diff --git a/fs/notify/inotify/inotify.h b/fs/notify/inotify/inotify.h index 4327d0e9c364..7fc3782b2fb8 100644 --- a/fs/notify/inotify/inotify.h +++ b/fs/notify/inotify/inotify.h @@ -24,11 +24,10 @@ static inline struct inotify_event_info *INOTIFY_E(struct fsnotify_event *fse) extern void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark, struct fsnotify_group *group); -extern 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); +extern int inotify_handle_event(struct fsnotify_group *group, + struct fsnotify_mark *inode_mark, u32 mask, + struct inode *inode, struct inode *dir, + const struct qstr *name, u32 cookie); extern const struct fsnotify_ops inotify_fsnotify_ops; extern struct kmem_cache *inotify_inode_mark_cachep; diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c index 9ddcbadc98e2..f348c1d3b358 100644 --- a/fs/notify/inotify/inotify_fsnotify.c +++ b/fs/notify/inotify/inotify_fsnotify.c @@ -55,10 +55,10 @@ static int inotify_merge(struct list_head *list, return event_compare(last_event, event); } -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) +int inotify_handle_event(struct fsnotify_group *group, + struct fsnotify_mark *inode_mark, u32 mask, + struct inode *inode, struct inode *dir, + const struct qstr *file_name, u32 cookie) { struct inotify_inode_mark *i_mark; struct inotify_event_info *event; @@ -68,10 +68,6 @@ static int inotify_one_event(struct fsnotify_group *group, u32 mask, int alloc_len = sizeof(struct inotify_event_info); struct mem_cgroup *old_memcg; - if ((inode_mark->mask & FS_EXCL_UNLINK) && - path && d_unlinked(path->dentry)) - return 0; - if (file_name) { len = file_name->len; alloc_len += len + 1; @@ -131,35 +127,12 @@ static int inotify_one_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) +static int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask, + struct inode *inode, struct inode *dir, + const struct qstr *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 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); + return inotify_handle_event(inode_mark->group, inode_mark, mask, inode, + dir, name, cookie); } static void inotify_freeing_mark(struct fsnotify_mark *fsn_mark, struct fsnotify_group *group) @@ -227,7 +200,7 @@ static void inotify_free_mark(struct fsnotify_mark *fsn_mark) } const struct fsnotify_ops inotify_fsnotify_ops = { - .handle_event = inotify_handle_event, + .handle_inode_event = inotify_handle_inode_event, .free_group_priv = inotify_free_group_priv, .free_event = inotify_free_event, .freeing_mark = inotify_freeing_mark, diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c index 24d17028375e..b559f296d4cf 100644 --- a/fs/notify/inotify/inotify_user.c +++ b/fs/notify/inotify/inotify_user.c @@ -495,14 +495,9 @@ void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark, struct fsnotify_group *group) { struct inotify_inode_mark *i_mark; - struct fsnotify_iter_info iter_info = { }; - - fsnotify_iter_set_report_type_mark(&iter_info, FSNOTIFY_OBJ_TYPE_INODE, - fsn_mark); /* Queue ignore event for the watch */ - inotify_handle_event(group, FS_IN_IGNORED, NULL, FSNOTIFY_EVENT_NONE, - NULL, NULL, 0, &iter_info); + inotify_handle_event(group, fsn_mark, FS_IN_IGNORED, NULL, NULL, NULL, 0); i_mark = container_of(fsn_mark, struct inotify_inode_mark, fsn_mark); /* remove this mark from the idr */
Convert inotify to use the simple handle_inode_event() interface to get rid of the code duplication between the generic helper fsnotify_handle_event() and the inotify_handle_event() callback, which also happen to be buggy code. The bug will be fixed in the generic helper. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/notify/inotify/inotify.h | 9 +++--- fs/notify/inotify/inotify_fsnotify.c | 47 ++++++---------------------- fs/notify/inotify/inotify_user.c | 7 +---- 3 files changed, 15 insertions(+), 48 deletions(-)