diff mbox series

[v2,07/16] fsnotify: replace inode pointer with tag

Message ID 20200217131455.31107-8-amir73il@gmail.com
State New, archived
Headers show
Series Fanotify event with name info | expand

Commit Message

Amir Goldstein Feb. 17, 2020, 1:14 p.m. UTC
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(-)

Comments

Jan Kara Feb. 26, 2020, 8:20 a.m. UTC | #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>

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
>
Jan Kara Feb. 26, 2020, 8:52 a.m. UTC | #2
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
Amir Goldstein Feb. 26, 2020, 9:34 a.m. UTC | #3
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 mbox series

Patch

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