* [PATCH 0/2] More fsnotify hook optimizations
@ 2025-07-07 17:07 Amir Goldstein
2025-07-07 17:07 ` [PATCH 1/2] fsnotify: merge file_set_fsnotify_mode_from_watchers() with open perm hook Amir Goldstein
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Amir Goldstein @ 2025-07-07 17:07 UTC (permalink / raw)
To: Jan Kara; +Cc: Christian Brauner, linux-fsdevel
Jan,
Even since we did the FMODE_NONOTIFY optimization, it really bothered me
that we do not optimize out FAN_ACCESS_PERM, which I consider to be
unused baggage of the legacy API.
I finally figured out a way to get rid of this unneeded overhead of
all the read APIs.
Along the way, also added a trivial optimization for non-applicable
FAN_ACCESS_PERM on readdir and prepared the code towards adding
pre-dir-content events.
This passes the LTP tests, but please take a good look to see if I
missed anything.
Thanks,
Amir.
Amir Goldstein (2):
fsnotify: merge file_set_fsnotify_mode_from_watchers() with open perm
hook
fsnotify: optimize FMODE_NONOTIFY_PERM for the common cases
fs/file_table.c | 2 +-
fs/notify/fsnotify.c | 97 +++++++++++++++++++++-----------
fs/open.c | 6 +-
include/linux/fs.h | 6 +-
include/linux/fsnotify.h | 35 ++----------
include/linux/fsnotify_backend.h | 6 +-
6 files changed, 80 insertions(+), 72 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] fsnotify: merge file_set_fsnotify_mode_from_watchers() with open perm hook
2025-07-07 17:07 [PATCH 0/2] More fsnotify hook optimizations Amir Goldstein
@ 2025-07-07 17:07 ` Amir Goldstein
2025-07-07 17:07 ` [PATCH 2/2] fsnotify: optimize FMODE_NONOTIFY_PERM for the common cases Amir Goldstein
2025-07-07 17:13 ` [PATCH 0/2] More fsnotify hook optimizations Amir Goldstein
2 siblings, 0 replies; 6+ messages in thread
From: Amir Goldstein @ 2025-07-07 17:07 UTC (permalink / raw)
To: Jan Kara; +Cc: Christian Brauner, linux-fsdevel
Create helper fsnotify_open_perm_and_set_mode() that moves the
fsnotify_open_perm() hook into file_set_fsnotify_mode_from_watchers().
This will allow some more optimizations.
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/file_table.c | 2 +-
fs/notify/fsnotify.c | 22 +++++++++++++---------
fs/open.c | 6 +++---
include/linux/fsnotify.h | 8 +++-----
4 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/fs/file_table.c b/fs/file_table.c
index f09d79a98111..81c72576e548 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -199,7 +199,7 @@ static int init_file(struct file *f, int flags, const struct cred *cred)
file_ref_init(&f->f_ref, 1);
/*
* Disable permission and pre-content events for all files by default.
- * They may be enabled later by file_set_fsnotify_mode_from_watchers().
+ * They may be enabled later by fsnotify_open_perm_and_set_mode().
*/
file_set_fsnotify_mode(f, FMODE_NONOTIFY_PERM);
return 0;
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index e2b4f17a48bb..de7e7425428b 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -656,12 +656,12 @@ EXPORT_SYMBOL_GPL(fsnotify);
#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
/*
- * At open time we check fsnotify_sb_has_priority_watchers() and set the
- * FMODE_NONOTIFY_ mode bits accordignly.
+ * At open time we check fsnotify_sb_has_priority_watchers(), call the open perm
+ * hook and set the FMODE_NONOTIFY_ mode bits accordignly.
* Later, fsnotify permission hooks do not check if there are permission event
* watches, but that there were permission event watches at open time.
*/
-void file_set_fsnotify_mode_from_watchers(struct file *file)
+int fsnotify_open_perm_and_set_mode(struct file *file)
{
struct dentry *dentry = file->f_path.dentry, *parent;
struct super_block *sb = dentry->d_sb;
@@ -669,7 +669,7 @@ void file_set_fsnotify_mode_from_watchers(struct file *file)
/* Is it a file opened by fanotify? */
if (FMODE_FSNOTIFY_NONE(file->f_mode))
- return;
+ return 0;
/*
* Permission events is a super set of pre-content events, so if there
@@ -679,7 +679,7 @@ void file_set_fsnotify_mode_from_watchers(struct file *file)
if (likely(!fsnotify_sb_has_priority_watchers(sb,
FSNOTIFY_PRIO_CONTENT))) {
file_set_fsnotify_mode(file, FMODE_NONOTIFY_PERM);
- return;
+ return 0;
}
/*
@@ -689,8 +689,9 @@ void file_set_fsnotify_mode_from_watchers(struct file *file)
if ((!d_is_dir(dentry) && !d_is_reg(dentry)) ||
likely(!fsnotify_sb_has_priority_watchers(sb,
FSNOTIFY_PRIO_PRE_CONTENT))) {
- file_set_fsnotify_mode(file, FMODE_NONOTIFY | FMODE_NONOTIFY_PERM);
- return;
+ file_set_fsnotify_mode(file, FMODE_NONOTIFY |
+ FMODE_NONOTIFY_PERM);
+ goto open_perm;
}
/*
@@ -702,7 +703,7 @@ void file_set_fsnotify_mode_from_watchers(struct file *file)
FSNOTIFY_PRE_CONTENT_EVENTS))) {
/* Enable pre-content events */
file_set_fsnotify_mode(file, 0);
- return;
+ goto open_perm;
}
/* Is parent watching for pre-content events on this file? */
@@ -713,11 +714,14 @@ void file_set_fsnotify_mode_from_watchers(struct file *file)
if (p_mask & FSNOTIFY_PRE_CONTENT_EVENTS) {
/* Enable pre-content events */
file_set_fsnotify_mode(file, 0);
- return;
+ goto open_perm;
}
}
/* Nobody watching for pre-content events from this file */
file_set_fsnotify_mode(file, FMODE_NONOTIFY | FMODE_NONOTIFY_PERM);
+
+open_perm:
+ return fsnotify_open_perm(file);
}
#endif
diff --git a/fs/open.c b/fs/open.c
index 7828234a7caa..f240b96ce586 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -943,12 +943,12 @@ static int do_dentry_open(struct file *f,
goto cleanup_all;
/*
- * Set FMODE_NONOTIFY_* bits according to existing permission watches.
+ * Call fsnotify open permission hook and set FMODE_NONOTIFY_* bits
+ * according to existing permission watches.
* If FMODE_NONOTIFY mode was already set for an fanotify fd or for a
* pseudo file, this call will not change the mode.
*/
- file_set_fsnotify_mode_from_watchers(f);
- error = fsnotify_open_perm(f);
+ error = fsnotify_open_perm_and_set_mode(f);
if (error)
goto cleanup_all;
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 454d8e466958..8c1fa617d375 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -129,7 +129,7 @@ static inline int fsnotify_file(struct file *file, __u32 mask)
#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
-void file_set_fsnotify_mode_from_watchers(struct file *file);
+int fsnotify_open_perm_and_set_mode(struct file *file);
/*
* fsnotify_file_area_perm - permission hook before access to file range
@@ -215,9 +215,6 @@ static inline int fsnotify_open_perm(struct file *file)
{
int ret;
- if (likely(!FMODE_FSNOTIFY_PERM(file->f_mode)))
- return 0;
-
if (file->f_flags & __FMODE_EXEC) {
ret = fsnotify_path(&file->f_path, FS_OPEN_EXEC_PERM);
if (ret)
@@ -228,8 +225,9 @@ static inline int fsnotify_open_perm(struct file *file)
}
#else
-static inline void file_set_fsnotify_mode_from_watchers(struct file *file)
+static inline int fsnotify_open_perm_and_set_mode(struct file *file)
{
+ return 0;
}
static inline int fsnotify_file_area_perm(struct file *file, int perm_mask,
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] fsnotify: optimize FMODE_NONOTIFY_PERM for the common cases
2025-07-07 17:07 [PATCH 0/2] More fsnotify hook optimizations Amir Goldstein
2025-07-07 17:07 ` [PATCH 1/2] fsnotify: merge file_set_fsnotify_mode_from_watchers() with open perm hook Amir Goldstein
@ 2025-07-07 17:07 ` Amir Goldstein
2025-07-08 11:25 ` Jan Kara
2025-07-07 17:13 ` [PATCH 0/2] More fsnotify hook optimizations Amir Goldstein
2 siblings, 1 reply; 6+ messages in thread
From: Amir Goldstein @ 2025-07-07 17:07 UTC (permalink / raw)
To: Jan Kara; +Cc: Christian Brauner, linux-fsdevel
The most unlikely watched permission event is FAN_ACCESS_PERM, because
at the time that it was introduced there were no evictable ignore mark,
so subscribing to FAN_ACCESS_PERM would have incured a very high
overhead.
Yet, when we set the fmode to FMODE_NOTIFY_HSM(), we never skip trying
to send FAN_ACCESS_PERM, which is almost always a waste of cycles.
We got to this logic because of bundling open permisson events and access
permission events in the same category and because FAN_OPEN_PERM is a
commonly used event.
By open coding fsnotify_open_perm() in fsnotify_open_perm_and_set_mode(),
we no longer need to regard FAN_OPEN*_PERM when calculating fmode.
This leaves the case of having pre-content events and not having access
permission events in the object masks a more likely case than the other
way around.
Rework the fmode macros and code so that their meaning now refers only
to hooks on an already open file:
- FMODE_NOTIFY_NONE() skip all events
- FMODE_NOTIFY_PERM() send all access permission events
- FMODE_NOTIFY_HSM() send pre-conent permission events
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/notify/fsnotify.c | 83 +++++++++++++++++++++-----------
include/linux/fs.h | 6 +--
include/linux/fsnotify.h | 27 +----------
include/linux/fsnotify_backend.h | 6 ++-
4 files changed, 64 insertions(+), 58 deletions(-)
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index de7e7425428b..0c109c50e0cb 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -199,8 +199,8 @@ static bool fsnotify_event_needs_parent(struct inode *inode, __u32 mnt_mask,
}
/* Are there any inode/mount/sb objects that watch for these events? */
-static inline bool fsnotify_object_watched(struct inode *inode, __u32 mnt_mask,
- __u32 mask)
+static inline __u32 fsnotify_object_watched(struct inode *inode, __u32 mnt_mask,
+ __u32 mask)
{
__u32 marks_mask = READ_ONCE(inode->i_fsnotify_mask) | mnt_mask |
READ_ONCE(inode->i_sb->s_fsnotify_mask);
@@ -665,7 +665,7 @@ int fsnotify_open_perm_and_set_mode(struct file *file)
{
struct dentry *dentry = file->f_path.dentry, *parent;
struct super_block *sb = dentry->d_sb;
- __u32 mnt_mask, p_mask;
+ __u32 mnt_mask, p_mask = 0;
/* Is it a file opened by fanotify? */
if (FMODE_FSNOTIFY_NONE(file->f_mode))
@@ -683,45 +683,70 @@ int fsnotify_open_perm_and_set_mode(struct file *file)
}
/*
- * If there are permission event watchers but no pre-content event
- * watchers, set FMODE_NONOTIFY | FMODE_NONOTIFY_PERM to indicate that.
+ * OK, there are some permission event watchers. Check if anybody is
+ * watching for permission events on *this* file.
*/
- if ((!d_is_dir(dentry) && !d_is_reg(dentry)) ||
- likely(!fsnotify_sb_has_priority_watchers(sb,
- FSNOTIFY_PRIO_PRE_CONTENT))) {
- file_set_fsnotify_mode(file, FMODE_NONOTIFY |
- FMODE_NONOTIFY_PERM);
+ mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask);
+ p_mask = fsnotify_object_watched(d_inode(dentry), mnt_mask,
+ ALL_FSNOTIFY_PERM_EVENTS);
+ if (dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) {
+ parent = dget_parent(dentry);
+ p_mask |= fsnotify_inode_watches_children(d_inode(parent));
+ dput(parent);
+ }
+
+ /*
+ * Without any access permission events, we only need to call the
+ * open perm hook and no further permission hooks on the open file.
+ * That is the common case with Anti-Malware protection service.
+ */
+ if (likely(!(p_mask & FSNOTIFY_ACCESS_PERM_EVENTS))) {
+ file_set_fsnotify_mode(file, FMODE_NONOTIFY_PERM);
goto open_perm;
}
/*
- * OK, there are some pre-content watchers. Check if anybody is
- * watching for pre-content events on *this* file.
+ * Legacy FAN_ACCESS_PERM events have very high performance overhead,
+ * so unlikely to be used in the wild. If they are used there will be
+ * no optimizations at all.
*/
- mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask);
- if (unlikely(fsnotify_object_watched(d_inode(dentry), mnt_mask,
- FSNOTIFY_PRE_CONTENT_EVENTS))) {
- /* Enable pre-content events */
+ if (unlikely(p_mask & FS_ACCESS_PERM)) {
+ /* Enable all permission and pre-content events */
file_set_fsnotify_mode(file, 0);
goto open_perm;
}
- /* Is parent watching for pre-content events on this file? */
- if (dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) {
- parent = dget_parent(dentry);
- p_mask = fsnotify_inode_watches_children(d_inode(parent));
- dput(parent);
- if (p_mask & FSNOTIFY_PRE_CONTENT_EVENTS) {
- /* Enable pre-content events */
- file_set_fsnotify_mode(file, 0);
- goto open_perm;
- }
+ /*
+ * Pre-content events are only supported on regular files.
+ * If there are pre-content event watchers and no permission access
+ * watchers, set FMODE_NONOTIFY | FMODE_NONOTIFY_PERM to indicate that.
+ * That is the common case with HSM service.
+ */
+ if (d_is_reg(dentry) && (p_mask & FSNOTIFY_PRE_CONTENT_EVENTS)) {
+ file_set_fsnotify_mode(file, FMODE_NONOTIFY |
+ FMODE_NONOTIFY_PERM);
+ goto open_perm;
}
- /* Nobody watching for pre-content events from this file */
- file_set_fsnotify_mode(file, FMODE_NONOTIFY | FMODE_NONOTIFY_PERM);
+
+ /* Nobody watching permission and pre-content events on this file */
+ file_set_fsnotify_mode(file, FMODE_NONOTIFY_PERM);
open_perm:
- return fsnotify_open_perm(file);
+ /*
+ * Send open perm events depending on object masks and regardless of
+ * FMODE_NONOTIFY_PERM.
+ */
+ if (file->f_flags & __FMODE_EXEC && p_mask & FS_OPEN_EXEC_PERM) {
+ int ret = fsnotify_path(&file->f_path, FS_OPEN_EXEC_PERM);
+
+ if (ret)
+ return ret;
+ }
+
+ if (p_mask & FS_OPEN_PERM)
+ return fsnotify_path(&file->f_path, FS_OPEN_PERM);
+
+ return 0;
}
#endif
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 45fe8f833284..1d54d323d9de 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -205,7 +205,7 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
*
* FMODE_NONOTIFY - suppress all (incl. non-permission) events.
* FMODE_NONOTIFY_PERM - suppress permission (incl. pre-content) events.
- * FMODE_NONOTIFY | FMODE_NONOTIFY_PERM - suppress only pre-content events.
+ * FMODE_NONOTIFY | FMODE_NONOTIFY_PERM - .. (excl. pre-content) events.
*/
#define FMODE_FSNOTIFY_MASK \
(FMODE_NONOTIFY | FMODE_NONOTIFY_PERM)
@@ -213,10 +213,10 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
#define FMODE_FSNOTIFY_NONE(mode) \
((mode & FMODE_FSNOTIFY_MASK) == FMODE_NONOTIFY)
#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
-#define FMODE_FSNOTIFY_PERM(mode) \
+#define FMODE_FSNOTIFY_HSM(mode) \
((mode & FMODE_FSNOTIFY_MASK) == 0 || \
(mode & FMODE_FSNOTIFY_MASK) == (FMODE_NONOTIFY | FMODE_NONOTIFY_PERM))
-#define FMODE_FSNOTIFY_HSM(mode) \
+#define FMODE_FSNOTIFY_PERM(mode) \
((mode & FMODE_FSNOTIFY_MASK) == 0)
#else
#define FMODE_FSNOTIFY_PERM(mode) 0
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 8c1fa617d375..6be0298701ae 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -147,9 +147,6 @@ static inline int fsnotify_file_area_perm(struct file *file, int perm_mask,
if (!(perm_mask & (MAY_READ | MAY_WRITE | MAY_ACCESS)))
return 0;
- if (likely(!FMODE_FSNOTIFY_PERM(file->f_mode)))
- return 0;
-
/*
* read()/write() and other types of access generate pre-content events.
*/
@@ -160,7 +157,8 @@ static inline int fsnotify_file_area_perm(struct file *file, int perm_mask,
return ret;
}
- if (!(perm_mask & MAY_READ))
+ if (!(perm_mask & MAY_READ) ||
+ likely(!FMODE_FSNOTIFY_PERM(file->f_mode)))
return 0;
/*
@@ -208,22 +206,6 @@ static inline int fsnotify_file_perm(struct file *file, int perm_mask)
return fsnotify_file_area_perm(file, perm_mask, NULL, 0);
}
-/*
- * fsnotify_open_perm - permission hook before file open
- */
-static inline int fsnotify_open_perm(struct file *file)
-{
- int ret;
-
- if (file->f_flags & __FMODE_EXEC) {
- ret = fsnotify_path(&file->f_path, FS_OPEN_EXEC_PERM);
- if (ret)
- return ret;
- }
-
- return fsnotify_path(&file->f_path, FS_OPEN_PERM);
-}
-
#else
static inline int fsnotify_open_perm_and_set_mode(struct file *file)
{
@@ -251,11 +233,6 @@ static inline int fsnotify_file_perm(struct file *file, int perm_mask)
{
return 0;
}
-
-static inline int fsnotify_open_perm(struct file *file)
-{
- return 0;
-}
#endif
/*
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 832d94d783d9..557f9b127960 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -87,14 +87,18 @@
/* Mount namespace events */
#define FSNOTIFY_MNT_EVENTS (FS_MNT_ATTACH | FS_MNT_DETACH)
+#define FSNOTIFY_OPEN_PERM_EVENTS (FS_OPEN_PERM | FS_OPEN_EXEC_PERM)
/* Content events can be used to inspect file content */
-#define FSNOTIFY_CONTENT_PERM_EVENTS (FS_OPEN_PERM | FS_OPEN_EXEC_PERM | \
+#define FSNOTIFY_CONTENT_PERM_EVENTS (FSNOTIFY_OPEN_PERM_EVENTS | \
FS_ACCESS_PERM)
/* Pre-content events can be used to fill file content */
#define FSNOTIFY_PRE_CONTENT_EVENTS (FS_PRE_ACCESS)
#define ALL_FSNOTIFY_PERM_EVENTS (FSNOTIFY_CONTENT_PERM_EVENTS | \
FSNOTIFY_PRE_CONTENT_EVENTS)
+/* Access permission events determine FMODE_NONOTIFY_PERM mode */
+#define FSNOTIFY_ACCESS_PERM_EVENTS (FS_ACCESS_PERM | \
+ FSNOTIFY_PRE_CONTENT_EVENTS)
/*
* This is a list of all events that may get sent to a parent that is watching
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] More fsnotify hook optimizations
2025-07-07 17:07 [PATCH 0/2] More fsnotify hook optimizations Amir Goldstein
2025-07-07 17:07 ` [PATCH 1/2] fsnotify: merge file_set_fsnotify_mode_from_watchers() with open perm hook Amir Goldstein
2025-07-07 17:07 ` [PATCH 2/2] fsnotify: optimize FMODE_NONOTIFY_PERM for the common cases Amir Goldstein
@ 2025-07-07 17:13 ` Amir Goldstein
2 siblings, 0 replies; 6+ messages in thread
From: Amir Goldstein @ 2025-07-07 17:13 UTC (permalink / raw)
To: Jan Kara; +Cc: Christian Brauner, linux-fsdevel
On Mon, Jul 7, 2025 at 7:07 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Jan,
>
> Even since we did the FMODE_NONOTIFY optimization, it really bothered me
> that we do not optimize out FAN_ACCESS_PERM, which I consider to be
> unused baggage of the legacy API.
>
> I finally figured out a way to get rid of this unneeded overhead of
> all the read APIs.
>
> Along the way, also added a trivial optimization for non-applicable
> FAN_ACCESS_PERM on readdir and prepared the code towards adding
> pre-dir-content events.
Urgh, I mean non-applicable FAN_PRE_ACCESS on readdir
referring to this finer tuned d_is_reg() check in patch 2:
/*
* Pre-content events are only supported on regular files.
...
*/
if (d_is_reg(dentry) && (p_mask & FSNOTIFY_PRE_CONTENT_EVENTS)) {
Thanks,
Amir.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] fsnotify: optimize FMODE_NONOTIFY_PERM for the common cases
2025-07-07 17:07 ` [PATCH 2/2] fsnotify: optimize FMODE_NONOTIFY_PERM for the common cases Amir Goldstein
@ 2025-07-08 11:25 ` Jan Kara
2025-07-08 13:32 ` Amir Goldstein
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2025-07-08 11:25 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Jan Kara, Christian Brauner, linux-fsdevel
On Mon 07-07-25 19:07:04, Amir Goldstein wrote:
> The most unlikely watched permission event is FAN_ACCESS_PERM, because
> at the time that it was introduced there were no evictable ignore mark,
> so subscribing to FAN_ACCESS_PERM would have incured a very high
> overhead.
>
> Yet, when we set the fmode to FMODE_NOTIFY_HSM(), we never skip trying
> to send FAN_ACCESS_PERM, which is almost always a waste of cycles.
>
> We got to this logic because of bundling open permisson events and access
> permission events in the same category and because FAN_OPEN_PERM is a
> commonly used event.
>
> By open coding fsnotify_open_perm() in fsnotify_open_perm_and_set_mode(),
> we no longer need to regard FAN_OPEN*_PERM when calculating fmode.
>
> This leaves the case of having pre-content events and not having access
> permission events in the object masks a more likely case than the other
> way around.
>
> Rework the fmode macros and code so that their meaning now refers only
> to hooks on an already open file:
>
> - FMODE_NOTIFY_NONE() skip all events
> - FMODE_NOTIFY_PERM() send all access permission events
I was a bit confused here but AFAIU you mean "send pre-content events and
FAN_ACCESS_PERM". And perhaps I'd call this macro
FMODE_NOTIFY_ACCESS_PERM() because that's the only place where it's going
to be used...
> - FMODE_NOTIFY_HSM() send pre-conent permission events
^^^ content
Otherwise neat trick, I like it. Some nitty comments below.
> @@ -683,45 +683,70 @@ int fsnotify_open_perm_and_set_mode(struct file *file)
> }
>
> /*
> - * If there are permission event watchers but no pre-content event
> - * watchers, set FMODE_NONOTIFY | FMODE_NONOTIFY_PERM to indicate that.
> + * OK, there are some permission event watchers. Check if anybody is
> + * watching for permission events on *this* file.
> */
> - if ((!d_is_dir(dentry) && !d_is_reg(dentry)) ||
> - likely(!fsnotify_sb_has_priority_watchers(sb,
> - FSNOTIFY_PRIO_PRE_CONTENT))) {
> - file_set_fsnotify_mode(file, FMODE_NONOTIFY |
> - FMODE_NONOTIFY_PERM);
> + mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask);
> + p_mask = fsnotify_object_watched(d_inode(dentry), mnt_mask,
> + ALL_FSNOTIFY_PERM_EVENTS);
> + if (dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) {
> + parent = dget_parent(dentry);
> + p_mask |= fsnotify_inode_watches_children(d_inode(parent));
> + dput(parent);
> + }
> +
> + /*
> + * Without any access permission events, we only need to call the
> + * open perm hook and no further permission hooks on the open file.
> + * That is the common case with Anti-Malware protection service.
> + */
> + if (likely(!(p_mask & FSNOTIFY_ACCESS_PERM_EVENTS))) {
> + file_set_fsnotify_mode(file, FMODE_NONOTIFY_PERM);
> goto open_perm;
> }
Why is the above if needed? It seems to me all the cases are properly
handled below already? And they are very cheap to check...
> /*
> - * OK, there are some pre-content watchers. Check if anybody is
> - * watching for pre-content events on *this* file.
> + * Legacy FAN_ACCESS_PERM events have very high performance overhead,
> + * so unlikely to be used in the wild. If they are used there will be
> + * no optimizations at all.
> */
> - mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask);
> - if (unlikely(fsnotify_object_watched(d_inode(dentry), mnt_mask,
> - FSNOTIFY_PRE_CONTENT_EVENTS))) {
> - /* Enable pre-content events */
> + if (unlikely(p_mask & FS_ACCESS_PERM)) {
> + /* Enable all permission and pre-content events */
> file_set_fsnotify_mode(file, 0);
> goto open_perm;
> }
>
> - /* Is parent watching for pre-content events on this file? */
> - if (dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) {
> - parent = dget_parent(dentry);
> - p_mask = fsnotify_inode_watches_children(d_inode(parent));
> - dput(parent);
> - if (p_mask & FSNOTIFY_PRE_CONTENT_EVENTS) {
> - /* Enable pre-content events */
> - file_set_fsnotify_mode(file, 0);
> - goto open_perm;
> - }
> + /*
> + * Pre-content events are only supported on regular files.
> + * If there are pre-content event watchers and no permission access
> + * watchers, set FMODE_NONOTIFY | FMODE_NONOTIFY_PERM to indicate that.
> + * That is the common case with HSM service.
> + */
> + if (d_is_reg(dentry) && (p_mask & FSNOTIFY_PRE_CONTENT_EVENTS)) {
> + file_set_fsnotify_mode(file, FMODE_NONOTIFY |
> + FMODE_NONOTIFY_PERM);
> + goto open_perm;
> }
> - /* Nobody watching for pre-content events from this file */
> - file_set_fsnotify_mode(file, FMODE_NONOTIFY | FMODE_NONOTIFY_PERM);
> +
> + /* Nobody watching permission and pre-content events on this file */
> + file_set_fsnotify_mode(file, FMODE_NONOTIFY_PERM);
<snip>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 45fe8f833284..1d54d323d9de 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -205,7 +205,7 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
> *
> * FMODE_NONOTIFY - suppress all (incl. non-permission) events.
> * FMODE_NONOTIFY_PERM - suppress permission (incl. pre-content) events.
> - * FMODE_NONOTIFY | FMODE_NONOTIFY_PERM - suppress only pre-content events.
> + * FMODE_NONOTIFY | FMODE_NONOTIFY_PERM - .. (excl. pre-content) events.
^^ I'd write here "suppress
FAN_ACCESS_PERM" to be explicit what this is about.
> @@ -213,10 +213,10 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
> #define FMODE_FSNOTIFY_NONE(mode) \
> ((mode & FMODE_FSNOTIFY_MASK) == FMODE_NONOTIFY)
> #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
> -#define FMODE_FSNOTIFY_PERM(mode) \
> +#define FMODE_FSNOTIFY_HSM(mode) \
> ((mode & FMODE_FSNOTIFY_MASK) == 0 || \
> (mode & FMODE_FSNOTIFY_MASK) == (FMODE_NONOTIFY | FMODE_NONOTIFY_PERM))
> -#define FMODE_FSNOTIFY_HSM(mode) \
> +#define FMODE_FSNOTIFY_PERM(mode) \
> ((mode & FMODE_FSNOTIFY_MASK) == 0)
As mentioned above I'd call this FMODE_FSNOTIFY_ACCESS_PERM().
> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 832d94d783d9..557f9b127960 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -87,14 +87,18 @@
> /* Mount namespace events */
> #define FSNOTIFY_MNT_EVENTS (FS_MNT_ATTACH | FS_MNT_DETACH)
>
> +#define FSNOTIFY_OPEN_PERM_EVENTS (FS_OPEN_PERM | FS_OPEN_EXEC_PERM)
> /* Content events can be used to inspect file content */
> -#define FSNOTIFY_CONTENT_PERM_EVENTS (FS_OPEN_PERM | FS_OPEN_EXEC_PERM | \
> +#define FSNOTIFY_CONTENT_PERM_EVENTS (FSNOTIFY_OPEN_PERM_EVENTS | \
> FS_ACCESS_PERM)
You don't use FSNOTIFY_OPEN_PERM_EVENTS anywhere. If anything I'd drop
FSNOTIFY_CONTENT_PERM_EVENTS completely as that has only single use in
ALL_FSNOTIFY_PERM_EVENTS instead of adding more practically unused defines.
> /* Pre-content events can be used to fill file content */
> #define FSNOTIFY_PRE_CONTENT_EVENTS (FS_PRE_ACCESS)
>
> #define ALL_FSNOTIFY_PERM_EVENTS (FSNOTIFY_CONTENT_PERM_EVENTS | \
> FSNOTIFY_PRE_CONTENT_EVENTS)
> +/* Access permission events determine FMODE_NONOTIFY_PERM mode */
> +#define FSNOTIFY_ACCESS_PERM_EVENTS (FS_ACCESS_PERM | \
> + FSNOTIFY_PRE_CONTENT_EVENTS)
I don't think this define is needed either so I'd drop it for now...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] fsnotify: optimize FMODE_NONOTIFY_PERM for the common cases
2025-07-08 11:25 ` Jan Kara
@ 2025-07-08 13:32 ` Amir Goldstein
0 siblings, 0 replies; 6+ messages in thread
From: Amir Goldstein @ 2025-07-08 13:32 UTC (permalink / raw)
To: Jan Kara; +Cc: Christian Brauner, linux-fsdevel
On Tue, Jul 8, 2025 at 1:26 PM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 07-07-25 19:07:04, Amir Goldstein wrote:
> > The most unlikely watched permission event is FAN_ACCESS_PERM, because
> > at the time that it was introduced there were no evictable ignore mark,
> > so subscribing to FAN_ACCESS_PERM would have incured a very high
> > overhead.
> >
> > Yet, when we set the fmode to FMODE_NOTIFY_HSM(), we never skip trying
> > to send FAN_ACCESS_PERM, which is almost always a waste of cycles.
> >
> > We got to this logic because of bundling open permisson events and access
> > permission events in the same category and because FAN_OPEN_PERM is a
> > commonly used event.
> >
> > By open coding fsnotify_open_perm() in fsnotify_open_perm_and_set_mode(),
> > we no longer need to regard FAN_OPEN*_PERM when calculating fmode.
> >
> > This leaves the case of having pre-content events and not having access
> > permission events in the object masks a more likely case than the other
> > way around.
> >
> > Rework the fmode macros and code so that their meaning now refers only
> > to hooks on an already open file:
> >
> > - FMODE_NOTIFY_NONE() skip all events
> > - FMODE_NOTIFY_PERM() send all access permission events
>
> I was a bit confused here but AFAIU you mean "send pre-content events and
> FAN_ACCESS_PERM". And perhaps I'd call this macro
> FMODE_NOTIFY_ACCESS_PERM() because that's the only place where it's going
> to be used...
Yes. agree.
>
> > - FMODE_NOTIFY_HSM() send pre-conent permission events
> ^^^ content
>
>
> Otherwise neat trick, I like it. Some nitty comments below.
Thanks.
>
> > @@ -683,45 +683,70 @@ int fsnotify_open_perm_and_set_mode(struct file *file)
> > }
> >
> > /*
> > - * If there are permission event watchers but no pre-content event
> > - * watchers, set FMODE_NONOTIFY | FMODE_NONOTIFY_PERM to indicate that.
> > + * OK, there are some permission event watchers. Check if anybody is
> > + * watching for permission events on *this* file.
> > */
> > - if ((!d_is_dir(dentry) && !d_is_reg(dentry)) ||
> > - likely(!fsnotify_sb_has_priority_watchers(sb,
> > - FSNOTIFY_PRIO_PRE_CONTENT))) {
> > - file_set_fsnotify_mode(file, FMODE_NONOTIFY |
> > - FMODE_NONOTIFY_PERM);
> > + mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask);
> > + p_mask = fsnotify_object_watched(d_inode(dentry), mnt_mask,
> > + ALL_FSNOTIFY_PERM_EVENTS);
> > + if (dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) {
> > + parent = dget_parent(dentry);
> > + p_mask |= fsnotify_inode_watches_children(d_inode(parent));
> > + dput(parent);
> > + }
> > +
> > + /*
> > + * Without any access permission events, we only need to call the
> > + * open perm hook and no further permission hooks on the open file.
> > + * That is the common case with Anti-Malware protection service.
> > + */
> > + if (likely(!(p_mask & FSNOTIFY_ACCESS_PERM_EVENTS))) {
> > + file_set_fsnotify_mode(file, FMODE_NONOTIFY_PERM);
> > goto open_perm;
> > }
>
> Why is the above if needed? It seems to me all the cases are properly
> handled below already? And they are very cheap to check...
>
> > /*
> > - * OK, there are some pre-content watchers. Check if anybody is
> > - * watching for pre-content events on *this* file.
> > + * Legacy FAN_ACCESS_PERM events have very high performance overhead,
> > + * so unlikely to be used in the wild. If they are used there will be
> > + * no optimizations at all.
> > */
> > - mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask);
> > - if (unlikely(fsnotify_object_watched(d_inode(dentry), mnt_mask,
> > - FSNOTIFY_PRE_CONTENT_EVENTS))) {
> > - /* Enable pre-content events */
> > + if (unlikely(p_mask & FS_ACCESS_PERM)) {
> > + /* Enable all permission and pre-content events */
> > file_set_fsnotify_mode(file, 0);
> > goto open_perm;
> > }
> >
> > - /* Is parent watching for pre-content events on this file? */
> > - if (dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) {
> > - parent = dget_parent(dentry);
> > - p_mask = fsnotify_inode_watches_children(d_inode(parent));
> > - dput(parent);
> > - if (p_mask & FSNOTIFY_PRE_CONTENT_EVENTS) {
> > - /* Enable pre-content events */
> > - file_set_fsnotify_mode(file, 0);
> > - goto open_perm;
> > - }
> > + /*
> > + * Pre-content events are only supported on regular files.
> > + * If there are pre-content event watchers and no permission access
> > + * watchers, set FMODE_NONOTIFY | FMODE_NONOTIFY_PERM to indicate that.
> > + * That is the common case with HSM service.
> > + */
> > + if (d_is_reg(dentry) && (p_mask & FSNOTIFY_PRE_CONTENT_EVENTS)) {
> > + file_set_fsnotify_mode(file, FMODE_NONOTIFY |
> > + FMODE_NONOTIFY_PERM);
> > + goto open_perm;
> > }
> > - /* Nobody watching for pre-content events from this file */
> > - file_set_fsnotify_mode(file, FMODE_NONOTIFY | FMODE_NONOTIFY_PERM);
> > +
> > + /* Nobody watching permission and pre-content events on this file */
> > + file_set_fsnotify_mode(file, FMODE_NONOTIFY_PERM);
>
> <snip>
>
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 45fe8f833284..1d54d323d9de 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -205,7 +205,7 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
> > *
> > * FMODE_NONOTIFY - suppress all (incl. non-permission) events.
> > * FMODE_NONOTIFY_PERM - suppress permission (incl. pre-content) events.
> > - * FMODE_NONOTIFY | FMODE_NONOTIFY_PERM - suppress only pre-content events.
> > + * FMODE_NONOTIFY | FMODE_NONOTIFY_PERM - .. (excl. pre-content) events.
> ^^ I'd write here "suppress
> FAN_ACCESS_PERM" to be explicit what this is about.
ok.
>
> > @@ -213,10 +213,10 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
> > #define FMODE_FSNOTIFY_NONE(mode) \
> > ((mode & FMODE_FSNOTIFY_MASK) == FMODE_NONOTIFY)
> > #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
> > -#define FMODE_FSNOTIFY_PERM(mode) \
> > +#define FMODE_FSNOTIFY_HSM(mode) \
> > ((mode & FMODE_FSNOTIFY_MASK) == 0 || \
> > (mode & FMODE_FSNOTIFY_MASK) == (FMODE_NONOTIFY | FMODE_NONOTIFY_PERM))
> > -#define FMODE_FSNOTIFY_HSM(mode) \
> > +#define FMODE_FSNOTIFY_PERM(mode) \
> > ((mode & FMODE_FSNOTIFY_MASK) == 0)
>
> As mentioned above I'd call this FMODE_FSNOTIFY_ACCESS_PERM().
ok.
>
> > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> > index 832d94d783d9..557f9b127960 100644
> > --- a/include/linux/fsnotify_backend.h
> > +++ b/include/linux/fsnotify_backend.h
> > @@ -87,14 +87,18 @@
> > /* Mount namespace events */
> > #define FSNOTIFY_MNT_EVENTS (FS_MNT_ATTACH | FS_MNT_DETACH)
> >
> > +#define FSNOTIFY_OPEN_PERM_EVENTS (FS_OPEN_PERM | FS_OPEN_EXEC_PERM)
> > /* Content events can be used to inspect file content */
> > -#define FSNOTIFY_CONTENT_PERM_EVENTS (FS_OPEN_PERM | FS_OPEN_EXEC_PERM | \
> > +#define FSNOTIFY_CONTENT_PERM_EVENTS (FSNOTIFY_OPEN_PERM_EVENTS | \
> > FS_ACCESS_PERM)
>
> You don't use FSNOTIFY_OPEN_PERM_EVENTS anywhere. If anything I'd drop
Right, I will drop it.
> FSNOTIFY_CONTENT_PERM_EVENTS completely as that has only single use in
> ALL_FSNOTIFY_PERM_EVENTS instead of adding more practically unused defines.
>
It is going to be used down the road.
In my followup fa_pre_dir_access patches, I split the dir/file macros:
#define FSNOTIFY_PRE_CONTENT_EVENTS \
(FSNOTIFY_PRE_FILE_CONTENT_EVENTS | \
FSNOTIFY_PRE_DIR_CONTENT_EVENTS)
Because the pre-dir-content events are not applicable ON_CHILD:
#define FS_EVENTS_POSS_ON_CHILD (FSNOTIFY_CONTENT_PERM_EVENTS | \
FSNOTIFY_PRE_FILE_CONTENT_EVENTS | \
> > /* Pre-content events can be used to fill file content */
> > #define FSNOTIFY_PRE_CONTENT_EVENTS (FS_PRE_ACCESS)
> >
> > #define ALL_FSNOTIFY_PERM_EVENTS (FSNOTIFY_CONTENT_PERM_EVENTS | \
> > FSNOTIFY_PRE_CONTENT_EVENTS)
> > +/* Access permission events determine FMODE_NONOTIFY_PERM mode */
> > +#define FSNOTIFY_ACCESS_PERM_EVENTS (FS_ACCESS_PERM | \
> > + FSNOTIFY_PRE_CONTENT_EVENTS)
>
> I don't think this define is needed either so I'd drop it for now...
ok. v2 soon...
Thanks,
Amir.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-07-08 13:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-07 17:07 [PATCH 0/2] More fsnotify hook optimizations Amir Goldstein
2025-07-07 17:07 ` [PATCH 1/2] fsnotify: merge file_set_fsnotify_mode_from_watchers() with open perm hook Amir Goldstein
2025-07-07 17:07 ` [PATCH 2/2] fsnotify: optimize FMODE_NONOTIFY_PERM for the common cases Amir Goldstein
2025-07-08 11:25 ` Jan Kara
2025-07-08 13:32 ` Amir Goldstein
2025-07-07 17:13 ` [PATCH 0/2] More fsnotify hook optimizations Amir Goldstein
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).