by Jan Kara

[permalink] [raw]

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

On Wed 24-06-15 17:16:08, Dave Hansen wrote:
>
> From: Dave Hansen <[email protected]>
>
> 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 <[email protected]>
> Cc: Alexander Viro <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: Paul E. McKenney <[email protected]>
> Cc: Tim Chen <[email protected]>
> Cc: Andi Kleen <[email protected]>
> Signed-off-by: Dave Hansen <[email protected]>
> ---
>
> 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 <[email protected]>
SUSE Labs, CR