Re: [RFC][PATCH v2] fsnotify: optimize the case of no content event watchers

From: Jan Kara <jack@suse.cz>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Jan Kara <jack@suse.cz>, Jens Axboe <axboe@kernel.dk>,
	Christian Brauner <brauner@kernel.org>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [RFC][PATCH v2] fsnotify: optimize the case of no content event watchers
Date: Fri, 12 Jan 2024 12:09:36 +0100	[thread overview]
Message-ID: <20240112110936.ibz4s42x75mjzhlv@quack3> (raw)
In-Reply-To: <20240111152233.352912-1-amir73il@gmail.com>

On Thu 11-01-24 17:22:33, Amir Goldstein wrote:
> Commit e43de7f0862b ("fsnotify: optimize the case of no marks of any type")
> optimized the case where there are no fsnotify watchers on any of the
> filesystem's objects.
> 
> It is quite common for a system to have a single local filesystem and
> it is quite common for the system to have some inotify watches on some
> config files or directories, so the optimization of no marks at all is
> often not in effect.
> 
> Content event (i.e. access,modify) watchers on sb/mount more rare, so
> optimizing the case of no sb/mount marks with content events can improve
> performance for more systems, especially for performance sensitive io
> workloads.
> 
> Set a per-sb flag SB_I_CONTENT_WATCHED if sb/mount marks with content
> events in their mask exist and use that flag to optimize out the call to
> __fsnotify_parent() and fsnotify() in fsnotify access/modify hooks.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

...

> -static inline int fsnotify_file(struct file *file, __u32 mask)
> +static inline int fsnotify_path(const struct path *path, __u32 mask)
>  {
> -	const struct path *path;
> +	struct dentry *dentry = path->dentry;
>  
> -	if (file->f_mode & FMODE_NONOTIFY)
> +	if (!fsnotify_sb_has_watchers(dentry->d_sb))
>  		return 0;
>  
> -	path = &file->f_path;
> +	/* Optimize the likely case of sb/mount/parent not watching content */
> +	if (mask & FSNOTIFY_CONTENT_EVENTS &&
> +	    likely(!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED)) &&
> +	    likely(!(dentry->d_sb->s_iflags & SB_I_CONTENT_WATCHED))) {
> +		/*
> +		 * XXX: if SB_I_CONTENT_WATCHED is not set, checking for content
> +		 * events in s_fsnotify_mask is redundant, but it will be needed
> +		 * if we use the flag FS_MNT_CONTENT_WATCHED to indicate the
> +		 * existence of only mount content event watchers.
> +		 */
> +		__u32 marks_mask = d_inode(dentry)->i_fsnotify_mask |
> +				   dentry->d_sb->s_fsnotify_mask;
> +
> +		if (!(mask & marks_mask))
> +			return 0;
> +	}

So I'm probably missing something but how is all this patch different from:

	if (likely(!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED))) {
		__u32 marks_mask = d_inode(dentry)->i_fsnotify_mask |
			path->mnt->mnt_fsnotify_mask |
			dentry->d_sb->s_fsnotify_mask;
		if (!(mask & marks_mask))
			return 0;
	}

I mean (mask & FSNOTIFY_CONTENT_EVENTS) is true for the frequent events
(read & write) we care about. In Jens' case

	!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) &&
	!(dentry->d_sb->s_iflags & SB_I_CONTENT_WATCHED)

is true as otherwise we'd go right to fsnotify_parent() and so Jens
wouldn't see the performance benefit. But then with your patch you fetch
i_fsnotify_mask and s_fsnotify_mask anyway for the test so the only
difference to what I suggest above is the path->mnt->mnt_fsnotify_mask
fetch but that is equivalent to sb->s_iflags (or wherever we store that
bit) fetch?

So that would confirm that the parent handling costs in fsnotify_parent()
is what's really making the difference and just avoiding that by checking
masks early should be enough?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR