Re: [RFCv2][PATCH 7/7] fsnotify: track when ignored mask clearing is needed

[Jan Kara <jack@suse.cz>]

On Wed 24-06-15 17:16:08, Dave Hansen wrote:
> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> According to Jan Kara:
> 
> 	You can have ignored mask set without any of the
> 	notification masks set and you are expected to clear the
> 	ignored mask on the first IN_MODIFY event.
> 
> But, the only way we currently have to go and find if we need to
> do this ignored-mask-clearing is to go through the mark lists
> and look for them.  That mark list iteration requires an
> srcu_read_lock() which has a memory barrier and can be expensive.
> 
> The calculation of 'has_ignore' is pretty cheap because we store
> it next to another value which we are updating and we do it
> inside of a loop we were already running.
> 
> This patch will really only matter when we have a workload where
> a file is being modified often _and_ there is an active fsnotify
> mark on it.  Otherwise the checks against *_fsnotify.marks.first
> will keep us out of the expensive srcu_read_lock() call.

So this is a nice optimization but definitely not worth growing struct
inode.  We could use one bit in the mask itself for this though.

								Honza
> 
> Cc: Jan Kara <jack@suse.cz>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: linux-fsdevel@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Tim Chen <tim.c.chen@linux.intel.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> ---
> 
>  b/fs/notify/fsnotify.c          |   44 ++++++++++++++++++++++++++++++++++------
>  b/fs/notify/mark.c              |    8 +++++--
>  b/include/linux/fsnotify_head.h |    1 
>  3 files changed, 45 insertions(+), 8 deletions(-)
> 
> diff -puN fs/notify/fsnotify.c~fsnotify-ignore-present fs/notify/fsnotify.c
> --- a/fs/notify/fsnotify.c~fsnotify-ignore-present	2015-06-24 17:14:37.187226743 -0700
> +++ b/fs/notify/fsnotify.c	2015-06-24 17:14:37.194227057 -0700
> @@ -183,6 +183,34 @@ static int send_to_group(struct inode *t
>  }
>  
>  /*
> + * The "logical or" of all of the marks' ->mask is kept in the
> + * i/mnt_fsnotify.mask.  We can check it instead of going
> + * through all of the marks.  fsnotify_recalc_mask() does the
> + * updates.
> + */
> +static int some_mark_is_interested(__u32 mask, struct inode *inode, struct mount *mnt)
> +{
> +	if (mask & inode->i_fsnotify.mask)
> +		return 1;
> +	if (mnt && (mask & mnt->mnt_fsnotify.mask))
> +		return 1;
> +	return 0;
> +}
> +
> +/*
> + * fsnotify_recalc_mask() recalculates "has_ignore" whenever any
> + * mark's flags change.
> + */
> +static int some_mark_needs_ignore_clear(struct inode *inode, struct mount *mnt)
> +{
> +	if (inode->i_fsnotify.has_ignore)
> +		return 1;
> +	if (mnt && mnt->mnt_fsnotify.has_ignore)
> +		return 1;
> +	return 0;
> +}
> +
> +/*
>   * This is the main call to fsnotify.  The VFS calls into hook specific functions
>   * in linux/fsnotify.h.  Those functions then in turn call here.  Here will call
>   * out to all of the registered fsnotify_group.  Those groups can then use the
> @@ -205,14 +233,18 @@ int fsnotify(struct inode *to_tell, __u3
>  		mnt = NULL;
>  
>  	/*
> -	 * if this is a modify event we may need to clear the ignored masks
> -	 * otherwise return if neither the inode nor the vfsmount care about
> -	 * this type of event.
> +	 * We must clear the (user-visible) ignored mask on the first IN_MODIFY
> +	 * event despite the 'mask' which is passed in here.  But we can safely
> +	 * skip that step if we know there are no marks which need this action.
> +	 *
> +	 * We can also skip looking at the list of marks if we know that none
> +	 * of the marks are interested in the events in our 'mask'.
>  	 */
> -	if (!(mask & FS_MODIFY) &&
> -	    !(test_mask & to_tell->i_fsnotify.mask) &&
> -	    !(mnt && test_mask & mnt->mnt_fsnotify.mask))
> +	if ((mask & FS_MODIFY) && !some_mark_needs_ignore_clear(to_tell, mnt))
> +		return 0;
> +	else if (!some_mark_is_interested(test_mask, to_tell, mnt))
>  		return 0;
> +
>  	/*
>  	 * Optimization: srcu_read_lock() has a memory barrier which can
>  	 * be expensive.  It protects walking the *_fsnotify_marks lists.
> diff -puN fs/notify/mark.c~fsnotify-ignore-present fs/notify/mark.c
> --- a/fs/notify/mark.c~fsnotify-ignore-present	2015-06-24 17:14:37.189226832 -0700
> +++ b/fs/notify/mark.c	2015-06-24 17:14:37.194227057 -0700
> @@ -116,10 +116,14 @@ void fsnotify_recalc_mask(struct fsnotif
>  {
>  	u32 new_mask = 0;
>  	struct fsnotify_mark *mark;
> +	u32 has_ignore = 0;
>  
> -	hlist_for_each_entry(mark, &fsn->marks, obj_list)
> +	hlist_for_each_entry(mark, &fsn->marks, obj_list) {
> +		if (mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY)
> +			has_ignore = 1;
>  		new_mask |= mark->mask;
> -
> +	}
> +	fsn->has_ignore = has_ignore;
>  	fsn->mask = new_mask;
>  }
>  
> diff -puN include/linux/fsnotify_head.h~fsnotify-ignore-present include/linux/fsnotify_head.h
> --- a/include/linux/fsnotify_head.h~fsnotify-ignore-present	2015-06-24 17:14:37.190226877 -0700
> +++ b/include/linux/fsnotify_head.h	2015-06-24 17:14:37.193227012 -0700
> @@ -11,6 +11,7 @@ struct fsnotify_head {
>  #ifdef CONFIG_FSNOTIFY
>  	__u32                   mask; /* all events this object cares about */
>  	struct hlist_head       marks;
> +	__u32                   has_ignore; /* any marks has ignore set */
>  #endif
>  };
>  
> _
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR