[PATCH 0/2] More fsnotify hook optimizations

[PATCH 0/2] More fsnotify hook optimizations

[Amir Goldstein]

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

[PATCH 1/2] fsnotify: merge file_set_fsnotify_mode_from_watchers() with open perm hook

[Amir Goldstein]

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

[PATCH 2/2] fsnotify: optimize FMODE_NONOTIFY_PERM for the common cases

[Amir Goldstein]

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

Re: [PATCH 2/2] fsnotify: optimize FMODE_NONOTIFY_PERM for the common cases

[Jan Kara]

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

Re: [PATCH 2/2] fsnotify: optimize FMODE_NONOTIFY_PERM for the common cases

[Amir Goldstein]

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.

Re: [PATCH 0/2] More fsnotify hook optimizations

[Amir Goldstein]

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.