| Message ID | 20200217131455.31107-8-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fanotify event with name info | expand |
On Mon 17-02-20 15:14:46, Amir Goldstein wrote: > The event inode field is used only for comparison in queue merges and > cannot be dereferenced after handle_event(), because it does not hold a > refcount on the inode. > > Replace it with an abstract tag do to the same thing. We are going to > set this tag for values other than inode pointer in fanotify. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> I like this but can we call it say 'objectid' or something like that? 'tag' seems too generic to me and it isn't clear why we should merge or not merge events with different tags... Honza > --- > fs/notify/fanotify/fanotify.c | 2 +- > fs/notify/inotify/inotify_fsnotify.c | 2 +- > include/linux/fsnotify_backend.h | 8 +++----- > 3 files changed, 5 insertions(+), 7 deletions(-) > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 19ec7a4f4d50..98c3cbf29003 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -26,7 +26,7 @@ static bool should_merge(struct fsnotify_event *old_fsn, > old = FANOTIFY_E(old_fsn); > new = FANOTIFY_E(new_fsn); > > - if (old_fsn->inode != new_fsn->inode || old->pid != new->pid || > + if (old_fsn->tag != new_fsn->tag || old->pid != new->pid || > old->fh_type != new->fh_type || old->fh_len != new->fh_len) > return false; > > diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c > index 6bb98522bbfd..4f42ea7b7fdd 100644 > --- a/fs/notify/inotify/inotify_fsnotify.c > +++ b/fs/notify/inotify/inotify_fsnotify.c > @@ -39,7 +39,7 @@ static bool event_compare(struct fsnotify_event *old_fsn, > if (old->mask & FS_IN_IGNORED) > return false; > if ((old->mask == new->mask) && > - (old_fsn->inode == new_fsn->inode) && > + (old_fsn->tag == new_fsn->tag) && > (old->name_len == new->name_len) && > (!old->name_len || !strcmp(old->name, new->name))) > return true; > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h > index bd3f6114a7a9..cd106b5c87a4 100644 > --- a/include/linux/fsnotify_backend.h > +++ b/include/linux/fsnotify_backend.h > @@ -132,8 +132,7 @@ struct fsnotify_ops { > */ > struct fsnotify_event { > struct list_head list; > - /* inode may ONLY be dereferenced during handle_event(). */ > - struct inode *inode; /* either the inode the event happened to or its parent */ > + unsigned long tag; /* identifier for queue merges */ > }; > > /* > @@ -542,11 +541,10 @@ extern void fsnotify_put_mark(struct fsnotify_mark *mark); > extern void fsnotify_finish_user_wait(struct fsnotify_iter_info *iter_info); > extern bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info); > > -static inline void fsnotify_init_event(struct fsnotify_event *event, > - struct inode *inode) > +static inline void fsnotify_init_event(struct fsnotify_event *event, void *tag) > { > INIT_LIST_HEAD(&event->list); > - event->inode = inode; > + event->tag = (unsigned long)tag; > } > > #else > -- > 2.17.1 >
On Mon 17-02-20 15:14:46, Amir Goldstein wrote: > The event inode field is used only for comparison in queue merges and > cannot be dereferenced after handle_event(), because it does not hold a > refcount on the inode. > > Replace it with an abstract tag do to the same thing. We are going to > set this tag for values other than inode pointer in fanotify. ... > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > -static inline void fsnotify_init_event(struct fsnotify_event *event, > - struct inode *inode) > +static inline void fsnotify_init_event(struct fsnotify_event *event, void *tag) > { > INIT_LIST_HEAD(&event->list); > - event->inode = inode; > + event->tag = (unsigned long)tag; > } Oh, and why not make the argument to fsnotify_init_event() unsigned long from the start? It would be IMHO cleaner and using void * doesn't really save us many type casts... Honza
On Wed, Feb 26, 2020 at 10:20 AM Jan Kara <jack@suse.cz> wrote: > > On Mon 17-02-20 15:14:46, Amir Goldstein wrote: > > The event inode field is used only for comparison in queue merges and > > cannot be dereferenced after handle_event(), because it does not hold a > > refcount on the inode. > > > > Replace it with an abstract tag do to the same thing. We are going to > > set this tag for values other than inode pointer in fanotify. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > I like this but can we call it say 'objectid' or something like that? 'tag' > seems too generic to me and it isn't clear why we should merge or not merge > events with different tags... Sounds good to me. And I agree to the comment about fsnotify_init_event() Apropos event merging, I ran across a simple create/delete files workload where fanotify_merge() was hogging the CPU. I currently carry a small private patch to limit the merge depth to 128 recent events. I still didn't have time to think about the best way to deal with this properly. Thanks, Amir.
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 19ec7a4f4d50..98c3cbf29003 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -26,7 +26,7 @@ static bool should_merge(struct fsnotify_event *old_fsn, old = FANOTIFY_E(old_fsn); new = FANOTIFY_E(new_fsn); - if (old_fsn->inode != new_fsn->inode || old->pid != new->pid || + if (old_fsn->tag != new_fsn->tag || old->pid != new->pid || old->fh_type != new->fh_type || old->fh_len != new->fh_len) return false; diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c index 6bb98522bbfd..4f42ea7b7fdd 100644 --- a/fs/notify/inotify/inotify_fsnotify.c +++ b/fs/notify/inotify/inotify_fsnotify.c @@ -39,7 +39,7 @@ static bool event_compare(struct fsnotify_event *old_fsn, if (old->mask & FS_IN_IGNORED) return false; if ((old->mask == new->mask) && - (old_fsn->inode == new_fsn->inode) && + (old_fsn->tag == new_fsn->tag) && (old->name_len == new->name_len) && (!old->name_len || !strcmp(old->name, new->name))) return true; diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h index bd3f6114a7a9..cd106b5c87a4 100644 --- a/include/linux/fsnotify_backend.h +++ b/include/linux/fsnotify_backend.h @@ -132,8 +132,7 @@ struct fsnotify_ops { */ struct fsnotify_event { struct list_head list; - /* inode may ONLY be dereferenced during handle_event(). */ - struct inode *inode; /* either the inode the event happened to or its parent */ + unsigned long tag; /* identifier for queue merges */ }; /* @@ -542,11 +541,10 @@ extern void fsnotify_put_mark(struct fsnotify_mark *mark); extern void fsnotify_finish_user_wait(struct fsnotify_iter_info *iter_info); extern bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info); -static inline void fsnotify_init_event(struct fsnotify_event *event, - struct inode *inode) +static inline void fsnotify_init_event(struct fsnotify_event *event, void *tag) { INIT_LIST_HEAD(&event->list); - event->inode = inode; + event->tag = (unsigned long)tag; } #else
The event inode field is used only for comparison in queue merges and cannot be dereferenced after handle_event(), because it does not hold a refcount on the inode. Replace it with an abstract tag do to the same thing. We are going to set this tag for values other than inode pointer in fanotify. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/notify/fanotify/fanotify.c | 2 +- fs/notify/inotify/inotify_fsnotify.c | 2 +- include/linux/fsnotify_backend.h | 8 +++----- 3 files changed, 5 insertions(+), 7 deletions(-)