On Fri, Jan 12, 2024 at 1:09 PM Jan Kara <jack@xxxxxxx> wrote:
>
> 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@xxxxxxxxx>
>
> ...
>
> > -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 |
It's actually:
real_mount(path->mnt)->mnt_fsnotify_mask
and this requires including "internal/mount.h" in all the call sites.
> 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?
Can't the benefit be also related to saving a function call?
Only one way to find out...
Jens,
Can you please test attached v3 with a non-inlined fsnotify_path() helper?
Thanks,
Amir.
From 7fa21951647ae24296cd0602f1291d00a8fa84d0 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@xxxxxxxxx>
Date: Fri, 9 Dec 2022 11:50:26 +0200
Subject: [PATCH v3] fsnotify: optimize the case of no content event watchers
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.
Unless a parent inode is watching, check for content events in masks of
sb/mount/inode masks early to optimize out the code in __fsnotify_parent()
and fsnotify() for fsnotify access/modify hooks.
Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
---
fs/notify/fsnotify.c | 26 ++++++++++++++++++++++++++
fs/notify/mark.c | 26 +++++++++++++++++++++++++-
include/linux/fs.h | 1 +
include/linux/fsnotify.h | 21 ++++++++++++---------
include/linux/fsnotify_backend.h | 13 +++++++++++++
5 files changed, 77 insertions(+), 10 deletions(-)
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 7974e91ffe13..b96251cdd165 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -237,6 +237,32 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
}
EXPORT_SYMBOL_GPL(__fsnotify_parent);
+/* Notify sb/mount/inode/parent watchers of this path */
+int fsnotify_path(const struct path *path, __u32 mask)
+{
+ struct dentry *dentry = path->dentry;
+
+ if (!fsnotify_sb_has_watchers(dentry->d_sb))
+ return 0;
+
+ /* Optimize the likely case of sb/mount/inode not watching content */
+ if (mask & FSNOTIFY_CONTENT_EVENTS &&
+ likely(!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED))) {
+ __u32 marks_mask = d_inode(dentry)->i_fsnotify_mask |
+ real_mount(path->mnt)->mnt_fsnotify_mask |
+ dentry->d_sb->s_fsnotify_mask;
+
+ if (!(mask & marks_mask))
+ return 0;
+ }
+
+ /*
+ * fsnotify_parent() checks the event masks of sb/mount/inode/parent.
+ */
+ return fsnotify_parent(path->dentry, mask, path, FSNOTIFY_EVENT_PATH);
+}
+EXPORT_SYMBOL_GPL(fsnotify_path);
+
static int fsnotify_handle_inode_event(struct fsnotify_group *group,
struct fsnotify_mark *inode_mark,
u32 mask, const void *data, int data_type,
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index d6944ff86ffa..d0e208913881 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -153,9 +153,30 @@ static struct inode *fsnotify_update_iref(struct fsnotify_mark_connector *conn,
return inode;
}
+/*
+ * To avoid the performance penalty of rare case of sb/mount content event
+ * watchers in the hot io path, taint sb if such watchers are added.
+ */
+static void fsnotify_update_sb_watchers(struct fsnotify_mark_connector *conn,
+ u32 old_mask, u32 new_mask)
+{
+ struct super_block *sb = fsnotify_connector_sb(conn);
+ u32 new_watchers = new_mask & ~old_mask & FSNOTIFY_CONTENT_EVENTS;
+
+ if (conn->type == FSNOTIFY_OBJ_TYPE_INODE || !sb || !new_watchers)
+ return;
+
+ /*
+ * TODO: We need to take sb conn->lock to set FS_MNT_CONTENT_WATCHED
+ * in sb->s_fsnotify_mask, but if this is a recalc of mount mark mask,
+ * it is not sure that we have an sb connector attached yet.
+ */
+ sb->s_iflags |= SB_I_CONTENT_WATCHED;
+}
+
static void *__fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
{
- u32 new_mask = 0;
+ u32 old_mask, new_mask = 0;
bool want_iref = false;
struct fsnotify_mark *mark;
@@ -163,6 +184,7 @@ static void *__fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
/* We can get detached connector here when inode is getting unlinked. */
if (!fsnotify_valid_obj_type(conn->type))
return NULL;
+ old_mask = fsnotify_conn_mask(conn);
hlist_for_each_entry(mark, &conn->list, obj_list) {
if (!(mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED))
continue;
@@ -173,6 +195,8 @@ static void *__fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
}
*fsnotify_conn_mask_p(conn) = new_mask;
+ fsnotify_update_sb_watchers(conn, old_mask, new_mask);
+
return fsnotify_update_iref(conn, want_iref);
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e6ba0cc6f2ee..dac36fe139e1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1173,6 +1173,7 @@ extern int send_sigurg(struct fown_struct *fown);
#define SB_I_TS_EXPIRY_WARNED 0x00000400 /* warned about timestamp range expiry */
#define SB_I_RETIRED 0x00000800 /* superblock shouldn't be reused */
#define SB_I_NOUMASK 0x00001000 /* VFS does not apply umask */
+#define SB_I_CONTENT_WATCHED 0x00002000 /* fsnotify file content monitor */
/* Possible states of 'frozen' field */
enum {
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 11e6434b8e71..2e0f47648a8b 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -17,6 +17,12 @@
#include <linux/slab.h>
#include <linux/bug.h>
+/* Are there any inode/mount/sb objects that are being watched? */
+static inline bool fsnotify_sb_has_watchers(struct super_block *sb)
+{
+ return atomic_long_read(&sb->s_fsnotify_connectors);
+}
+
/*
* Notify this @dir inode about a change in a child directory entry.
* The directory entry may have turned positive or negative or its inode may
@@ -30,7 +36,7 @@ static inline int fsnotify_name(__u32 mask, const void *data, int data_type,
struct inode *dir, const struct qstr *name,
u32 cookie)
{
- if (atomic_long_read(&dir->i_sb->s_fsnotify_connectors) == 0)
+ if (!fsnotify_sb_has_watchers(dir->i_sb))
return 0;
return fsnotify(mask, data, data_type, dir, name, NULL, cookie);
@@ -44,7 +50,7 @@ static inline void fsnotify_dirent(struct inode *dir, struct dentry *dentry,
static inline void fsnotify_inode(struct inode *inode, __u32 mask)
{
- if (atomic_long_read(&inode->i_sb->s_fsnotify_connectors) == 0)
+ if (!fsnotify_sb_has_watchers(inode->i_sb))
return;
if (S_ISDIR(inode->i_mode))
@@ -59,9 +65,6 @@ static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,
{
struct inode *inode = d_inode(dentry);
- if (atomic_long_read(&inode->i_sb->s_fsnotify_connectors) == 0)
- return 0;
-
if (S_ISDIR(inode->i_mode)) {
mask |= FS_ISDIR;
@@ -86,18 +89,18 @@ static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,
*/
static inline void fsnotify_dentry(struct dentry *dentry, __u32 mask)
{
+ if (!fsnotify_sb_has_watchers(dentry->d_sb))
+ return;
+
fsnotify_parent(dentry, mask, dentry, FSNOTIFY_EVENT_DENTRY);
}
static inline int fsnotify_file(struct file *file, __u32 mask)
{
- const struct path *path;
-
if (file->f_mode & FMODE_NONOTIFY)
return 0;
- path = &file->f_path;
- return fsnotify_parent(path->dentry, mask, path, FSNOTIFY_EVENT_PATH);
+ return fsnotify_path(&file->f_path, mask);
}
/*
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 7f63be5ca0f1..4e098bdeb3ca 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -66,6 +66,11 @@
#define FS_RENAME 0x10000000 /* File was renamed */
#define FS_DN_MULTISHOT 0x20000000 /* dnotify multishot */
#define FS_ISDIR 0x40000000 /* event occurred against dir */
+/*
+ * This flag is set in the object interest mask of sb to indicate that
+ * some mount mark is interested to get content events.
+ */
+#define FS_MNT_CONTENT_WATCHED 0x80000000
#define FS_MOVE (FS_MOVED_FROM | FS_MOVED_TO)
@@ -77,6 +82,13 @@
*/
#define ALL_FSNOTIFY_DIRENT_EVENTS (FS_CREATE | FS_DELETE | FS_MOVE | FS_RENAME)
+/* Content events can be used to inspect file content */
+#define FSNOTIFY_CONTENT_PERM_EVENTS (FS_ACCESS_PERM)
+
+/* Content events can be used to monitor file content */
+#define FSNOTIFY_CONTENT_EVENTS (FS_ACCESS | FS_MODIFY | \
+ FSNOTIFY_CONTENT_PERM_EVENTS)
+
#define ALL_FSNOTIFY_PERM_EVENTS (FS_OPEN_PERM | FS_ACCESS_PERM | \
FS_OPEN_EXEC_PERM)
@@ -541,6 +553,7 @@ struct fsnotify_mark {
extern int fsnotify(__u32 mask, const void *data, int data_type,
struct inode *dir, const struct qstr *name,
struct inode *inode, u32 cookie);
+extern int fsnotify_path(const struct path *path, __u32 mask);
extern int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
int data_type);
extern void __fsnotify_inode_delete(struct inode *inode);
--
2.34.1
