Skip to contentSkip to conversation

New fanotify event info API

PATCH In review Amir Goldstein 3 participants 20 messages Oct 4, 2018 – Oct 16, 2018

Subsystems (5)
VFS Fsnotify: filesystem notification infrastructure Fanotify Inotify +1 more subsystems
lore
Share & tools
lore

Amir Goldstein

author reviewer
#0 lore raw

v3 cover New fanotify event info API

Jan,

Following is the fanotify cleanup for internal constants followed by the FAN_EVENT_INFO_TID API change.

The work is available on branch fanotify_api-v3 [1]. Work was tested against the inotify/fanotify LTP tests including my new fanotify10 test and FAN_EVENT_INFO_TID test fanotify11 by nixiaoming, both available on my LTP tree [2].

Per your request, the series is based on commit 96a71f21ef1f ("fanotify: store fanotify_init() flags in group's fanotify_data") from your 'fsnotify' branch.

As you can see, the cleanup grew a few more patches since v2:
- Patch 1 is the separate fix you requested for FAN_MARK_ONDIR
- Patches 2-3 then get rid of FAN_MARK_ONDIR
- Patch 4 convert FAN_ALL_* users to use FANOTIFY_* constants This convention in inline with some existing FANOTIFY_* internal constants and with INOTIFY_ and FSNOTIFY_ internal constants
- Patches 5-6 add the bits weight validators you requested
- Patch 7 (FAN_EVENT_INFO_TID) is mostly unmodified from v2
- Patch 8 is a bonus low hanging optimization

I suppose you can leave that last patch out, because I have not done any performance tests with in, but if you find it is "probably an improvement" maybe put it in next and let the LKP robots let us know what they think.

Thanks,
Amir.

[1] https://github.com/amir73il/linux/commits/fanotify_api-v3
[2] https://github.com/amir73il/ltp/commits/fanotify_unpriv

Amir Goldstein (8):
fanotify: fix collision of internal and uapi mark flags
fsnotify: generalize handling of extra event flags
fanotify: simplify handling of FAN_ONDIR
fanotify: deprecate uapi FAN_ALL_* constants
fsnotify: convert runtime BUG_ON() to BUILD_BUG_ON()
fanotify: add BUILD_BUG_ON() to count the bits of fanotify constants
fanotify: support reporting thread id instead of process id
fsnotify: optimize away srcu_read_lock() for events on directories

 fs/notify/dnotify/dnotify.c        |  5 ++-
 fs/notify/fanotify/fanotify.c      | 17 ++++++---
 fs/notify/fanotify/fanotify.h      |  4 +-
 fs/notify/fanotify/fanotify_user.c | 57 ++++++++++------------------
 fs/notify/fsnotify.c               | 16 +++++---
 fs/notify/inotify/inotify_user.c   |  6 +--
 include/linux/fanotify.h           | 61 ++++++++++++++++++++++++++++--
 include/linux/fsnotify_backend.h   |  9 ++++-
 include/uapi/linux/fanotify.h      | 21 +++++-----
 9 files changed, 126 insertions(+), 70 deletions(-)
Signature · 1 line
2.17.1
author reviewer
#1 lore raw

PATCH v3 1/8 fanotify: fix collision of internal and uapi mark flags

The new mark flag FAN_MARK_FILESYSTEMS collides with existing internal flag FAN_MARK_ONDIR. Change internal flag value to avoid the collision.

Fixes d54f4fba889b ("fanotify: add API to attach/detach super block mark")
Signed-off-by Amir Goldstein <[email protected]>
---
 include/linux/fanotify.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/fanotify.h +1 -1
index 9c5ea3bdfaa0..e70fccc3757e 100644
@@ -5,7 +5,7 @@
 #include <uapi/linux/fanotify.h>
 
 /* not valid from userspace, only kernel internal */
-#define FAN_MARK_ONDIR		0x00000100
+#define FAN_MARK_ONDIR		0x80000000
 
 #define FAN_GROUP_FLAG(group, flag) \
 	((group)->fanotify_data.flags & (flag))
Signature · 1 line
2.17.1
author reviewer
#2 lore raw

PATCH v3 2/8 fsnotify: generalize handling of extra event flags

In reply to Amir Goldstein

FS_EVENT_ON_CHILD gets a special treatment in fsnotify() because it is not a flag specifying an event type, but rather an extra flags that may be reported along with another event and control the handling of the event by the backend.

FS_ISDIR is also an "extra flag" and not an "event type" and therefore desrves the same treatment. With inotify/dnotify backends it was never possible to set FS_ISDIR in mark masks, so it did not matter. With fanotify backend, mark adding code jumps through hoops to avoid setting the FS_ISDIR in the commulative object mask.

Separate the constant ALL_FSNOTIFY_EVENTS to ALL_FSNOTIFY_FLAGS and ALL_FSNOTIFY_EVENTS, so the latter can be used to test for specific event types.

Signed-off-by Amir Goldstein <[email protected]>
---
 fs/notify/fsnotify.c             | 7 +++----
 include/linux/fsnotify_backend.h | 9 +++++++--
 2 files changed, 10 insertions(+), 6 deletions(-)
fs/notify/fsnotify.c +3 -4
index 422fbc6dffde..7391a02bf723 100644
@@ -196,7 +196,7 @@ static int send_to_group(struct inode *to_tell,
 			 struct fsnotify_iter_info *iter_info)
 {
 	struct fsnotify_group *group = NULL;
-	__u32 test_mask = (mask & ~FS_EVENT_ON_CHILD);
+	__u32 test_mask = (mask & ALL_FSNOTIFY_EVENTS);
 	__u32 marks_mask = 0;
 	__u32 marks_ignored_mask = 0;
 	struct fsnotify_mark *mark;

@@ -329,8 +329,7 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 	struct mount *mnt = NULL;
 	__u32 mnt_or_sb_mask = 0;
 	int ret = 0;
-	/* global tests shouldn't care about events on child only the specific event */
-	__u32 test_mask = (mask & ~FS_EVENT_ON_CHILD);
+	__u32 test_mask = (mask & ALL_FSNOTIFY_EVENTS);
 
 	if (data_is == FSNOTIFY_EVENT_PATH) {
 		mnt = real_mount(((const struct path *)data)->mnt);

@@ -396,7 +395,7 @@ static __init int fsnotify_init(void)
 {
 	int ret;
 
-	BUG_ON(hweight32(ALL_FSNOTIFY_EVENTS) != 23);
+	BUG_ON(hweight32(ALL_FSNOTIFY_BITS) != 23);
 
 	ret = init_srcu_struct(&fsnotify_mark_srcu);
 	if (ret)
include/linux/fsnotify_backend.h +7 -2
index 8e91341cbd8a..135b973e44d1 100644
@@ -68,15 +68,20 @@
 
 #define ALL_FSNOTIFY_PERM_EVENTS (FS_OPEN_PERM | FS_ACCESS_PERM)
 
+/* Events that can be reported to backends */
 #define ALL_FSNOTIFY_EVENTS (FS_ACCESS | FS_MODIFY | FS_ATTRIB | \
 			     FS_CLOSE_WRITE | FS_CLOSE_NOWRITE | FS_OPEN | \
 			     FS_MOVED_FROM | FS_MOVED_TO | FS_CREATE | \
 			     FS_DELETE | FS_DELETE_SELF | FS_MOVE_SELF | \
 			     FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED | \
-			     FS_OPEN_PERM | FS_ACCESS_PERM | FS_EXCL_UNLINK | \
-			     FS_ISDIR | FS_IN_ONESHOT | FS_DN_RENAME | \
+			     FS_OPEN_PERM | FS_ACCESS_PERM | FS_DN_RENAME)
+
+/* Extra flags that may be reported with event or control handling of events */
+#define ALL_FSNOTIFY_FLAGS  (FS_EXCL_UNLINK | FS_ISDIR | FS_IN_ONESHOT | \
 			     FS_DN_MULTISHOT | FS_EVENT_ON_CHILD)
 
+#define ALL_FSNOTIFY_BITS   (ALL_FSNOTIFY_EVENTS | ALL_FSNOTIFY_FLAGS)
+
 struct fsnotify_group;
 struct fsnotify_event;
 struct fsnotify_mark;
Signature · 1 line
2.17.1
author reviewer
#3 lore raw

PATCH v3 3/8 fanotify: simplify handling of FAN_ONDIR

In reply to Amir Goldstein

fanotify mark add/remove code jumps through hoops to avoid setting the FS_ISDIR in the commulative object mask.

That was just papering over a bug in fsnotify() handling of the FS_ISDIR extra flag. This bug is now fixed, so all the hoops can be removed along with the unneeded internal flag FAN_MARK_ONDIR.

Signed-off-by Amir Goldstein <[email protected]>
---
 fs/notify/fanotify/fanotify_user.c | 32 +++++-------------------------
 include/linux/fanotify.h           |  3 ---
 2 files changed, 5 insertions(+), 30 deletions(-)
fs/notify/fanotify/fanotify_user.c +5 -27
index 15719d4aa4b5..34b511407035 100644
@@ -506,18 +506,10 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
 
 	spin_lock(&fsn_mark->lock);
 	if (!(flags & FAN_MARK_IGNORED_MASK)) {
-		__u32 tmask = fsn_mark->mask & ~mask;
-
-		if (flags & FAN_MARK_ONDIR)
-			tmask &= ~FAN_ONDIR;
-
 		oldmask = fsn_mark->mask;
-		fsn_mark->mask = tmask;
+		fsn_mark->mask &= ~mask;
 	} else {
-		__u32 tmask = fsn_mark->ignored_mask & ~mask;
-		if (flags & FAN_MARK_ONDIR)
-			tmask &= ~FAN_ONDIR;
-		fsn_mark->ignored_mask = tmask;
+		fsn_mark->ignored_mask &= ~mask;
 	}
 	*destroy = !(fsn_mark->mask | fsn_mark->ignored_mask);
 	spin_unlock(&fsn_mark->lock);

@@ -586,19 +578,10 @@ static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
 
 	spin_lock(&fsn_mark->lock);
 	if (!(flags & FAN_MARK_IGNORED_MASK)) {
-		__u32 tmask = fsn_mark->mask | mask;
-
-		if (flags & FAN_MARK_ONDIR)
-			tmask |= FAN_ONDIR;
-
 		oldmask = fsn_mark->mask;
-		fsn_mark->mask = tmask;
+		fsn_mark->mask |= mask;
 	} else {
-		__u32 tmask = fsn_mark->ignored_mask | mask;
-		if (flags & FAN_MARK_ONDIR)
-			tmask |= FAN_ONDIR;
-
-		fsn_mark->ignored_mask = tmask;
+		fsn_mark->ignored_mask |= mask;
 		if (flags & FAN_MARK_IGNORED_SURV_MODIFY)
 			fsn_mark->flags |= FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY;
 	}

@@ -820,7 +803,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	struct fsnotify_group *group;
 	struct fd f;
 	struct path path;
-	u32 valid_mask = FAN_ALL_EVENTS | FAN_EVENT_ON_CHILD;
+	u32 valid_mask = FAN_ALL_EVENTS | FAN_EVENT_ON_CHILD | FAN_ONDIR;
 	unsigned int mark_type = flags & FAN_MARK_TYPE_MASK;
 	int ret;
 

@@ -857,11 +840,6 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 		return -EINVAL;
 	}
 
-	if (mask & FAN_ONDIR) {
-		flags |= FAN_MARK_ONDIR;
-		mask &= ~FAN_ONDIR;
-	}
-
 	if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
 		valid_mask |= FAN_ALL_PERM_EVENTS;
include/linux/fanotify.h +0 -3
index e70fccc3757e..a8c3fc54276d 100644
@@ -4,9 +4,6 @@
 
 #include <uapi/linux/fanotify.h>
 
-/* not valid from userspace, only kernel internal */
-#define FAN_MARK_ONDIR		0x80000000
-
 #define FAN_GROUP_FLAG(group, flag) \
 	((group)->fanotify_data.flags & (flag))
Signature · 1 line
2.17.1
author reviewer
#4 lore raw

PATCH v3 4/8 fanotify: deprecate uapi FAN_ALL_* constants

In reply to Amir Goldstein

We do not want to add new bits to the FAN_ALL_* uapi constants because they have been exposed to userspace. If there are programs out there using these constants, those programs could break if re-compiled with modified FAN_ALL_* constants and run on an old kernel.

We deprecate the uapi constants FAN_ALL_* and define new FANOTIFY_* constants for internal use to replace them. New feature bits will be added only to the new constants.

Cc <[email protected]>
Signed-off-by Amir Goldstein <[email protected]>
---
 fs/notify/fanotify/fanotify.c      |  6 ++--
 fs/notify/fanotify/fanotify.h      |  2 +-
 fs/notify/fanotify/fanotify_user.c | 22 +++++++-------
 include/linux/fanotify.h           | 47 ++++++++++++++++++++++++++++++
 include/uapi/linux/fanotify.h      | 18 ++++++------
 5 files changed, 71 insertions(+), 24 deletions(-)
fs/notify/fanotify/fanotify.c +3 -3
index 94b52157bf8d..03498eb995be 100644
@@ -131,8 +131,8 @@ static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info,
 	    !(marks_mask & FS_ISDIR & ~marks_ignored_mask))
 		return false;
 
-	if (event_mask & FAN_ALL_OUTGOING_EVENTS & marks_mask &
-				 ~marks_ignored_mask)
+	if (event_mask & FANOTIFY_OUTGOING_EVENTS &
+	    marks_mask & ~marks_ignored_mask)
 		return true;
 
 	return false;

@@ -236,7 +236,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 	ret = fsnotify_add_event(group, fsn_event, fanotify_merge);
 	if (ret) {
 		/* Permission events shouldn't be merged */
-		BUG_ON(ret == 1 && mask & FAN_ALL_PERM_EVENTS);
+		BUG_ON(ret == 1 && mask & FANOTIFY_PERM_EVENTS);
 		/* Our event wasn't used in the end. Free it. */
 		fsnotify_destroy_event(group, fsn_event);
fs/notify/fanotify/fanotify.h +1 -1
index 8609ba06f474..88a8290a61cb 100644
@@ -44,7 +44,7 @@ FANOTIFY_PE(struct fsnotify_event *fse)
 static inline bool fanotify_is_perm_event(u32 mask)
 {
 	return IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS) &&
-		mask & FAN_ALL_PERM_EVENTS;
+		mask & FANOTIFY_PERM_EVENTS;
 }
 
 static inline struct fanotify_event_info *FANOTIFY_E(struct fsnotify_event *fse)
fs/notify/fanotify/fanotify_user.c +11 -11
index 34b511407035..530e5e486105 100644
@@ -131,7 +131,7 @@ static int fill_event_metadata(struct fsnotify_group *group,
 	metadata->metadata_len = FAN_EVENT_METADATA_LEN;
 	metadata->vers = FANOTIFY_METADATA_VERSION;
 	metadata->reserved = 0;
-	metadata->mask = fsn_event->mask & FAN_ALL_OUTGOING_EVENTS;
+	metadata->mask = fsn_event->mask & FANOTIFY_OUTGOING_EVENTS;
 	metadata->pid = pid_vnr(event->tgid);
 	if (unlikely(fsn_event->mask & FAN_Q_OVERFLOW))
 		metadata->fd = FAN_NOFD;

@@ -395,7 +395,7 @@ static int fanotify_release(struct inode *ignored, struct file *file)
 	 */
 	while (!fsnotify_notify_queue_is_empty(group)) {
 		fsn_event = fsnotify_remove_first_event(group);
-		if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS)) {
+		if (!(fsn_event->mask & FANOTIFY_PERM_EVENTS)) {
 			spin_unlock(&group->notification_lock);
 			fsnotify_destroy_event(group, fsn_event);
 			spin_lock(&group->notification_lock);

@@ -691,9 +691,9 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 		return -EPERM;
 
 #ifdef CONFIG_AUDITSYSCALL
-	if (flags & ~(FAN_ALL_INIT_FLAGS | FAN_ENABLE_AUDIT))
+	if (flags & ~(FANOTIFY_INIT_FLAGS | FAN_ENABLE_AUDIT))
 #else
-	if (flags & ~FAN_ALL_INIT_FLAGS)
+	if (flags & ~FANOTIFY_INIT_FLAGS)
 #endif
 		return -EINVAL;
 

@@ -745,7 +745,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 	group->fanotify_data.f_flags = event_f_flags;
 	init_waitqueue_head(&group->fanotify_data.access_waitq);
 	INIT_LIST_HEAD(&group->fanotify_data.access_list);
-	switch (flags & FAN_ALL_CLASS_BITS) {
+	switch (flags & FANOTIFY_CLASS_BITS) {
 	case FAN_CLASS_NOTIF:
 		group->priority = FS_PRIO_0;
 		break;

@@ -803,8 +803,8 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	struct fsnotify_group *group;
 	struct fd f;
 	struct path path;
-	u32 valid_mask = FAN_ALL_EVENTS | FAN_EVENT_ON_CHILD | FAN_ONDIR;
-	unsigned int mark_type = flags & FAN_MARK_TYPE_MASK;
+	u32 valid_mask = FANOTIFY_EVENTS | FAN_EVENT_ON_CHILD | FAN_ONDIR;
+	unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
 	int ret;
 
 	pr_debug("%s: fanotify_fd=%d flags=%x dfd=%d pathname=%p mask=%llx\n",

@@ -814,7 +814,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	if (mask & ((__u64)0xffffffff << 32))
 		return -EINVAL;
 
-	if (flags & ~FAN_ALL_MARK_FLAGS)
+	if (flags & ~FANOTIFY_MARK_FLAGS)
 		return -EINVAL;
 
 	switch (mark_type) {

@@ -833,7 +833,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 			return -EINVAL;
 		break;
 	case FAN_MARK_FLUSH:
-		if (flags & ~(FAN_MARK_TYPE_MASK | FAN_MARK_FLUSH))
+		if (flags & ~(FANOTIFY_MARK_TYPE_BITS | FAN_MARK_FLUSH))
 			return -EINVAL;
 		break;
 	default:

@@ -841,7 +841,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	}
 
 	if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
-		valid_mask |= FAN_ALL_PERM_EVENTS;
+		valid_mask |= FANOTIFY_PERM_EVENTS;
 
 	if (mask & ~valid_mask)
 		return -EINVAL;

@@ -861,7 +861,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	 * allowed to set permissions events.
 	 */
 	ret = -EINVAL;
-	if (mask & FAN_ALL_PERM_EVENTS &&
+	if (mask & FANOTIFY_PERM_EVENTS &&
 	    group->priority == FS_PRIO_0)
 		goto fput_and_out;
include/linux/fanotify.h +47 -0

51 lines not on this page · raw message

include/uapi/linux/fanotify.h +9 -9
index ad81234d1919..d0c05de670ef 100644
@@ -31,6 +31,8 @@
 #define FAN_CLASS_NOTIF		0x00000000
 #define FAN_CLASS_CONTENT	0x00000004
 #define FAN_CLASS_PRE_CONTENT	0x00000008
+
+/* Deprecated - do not use this in programs and do not add new flags here! */
 #define FAN_ALL_CLASS_BITS	(FAN_CLASS_NOTIF | FAN_CLASS_CONTENT | \
 				 FAN_CLASS_PRE_CONTENT)
 

@@ -38,6 +40,7 @@
 #define FAN_UNLIMITED_MARKS	0x00000020
 #define FAN_ENABLE_AUDIT	0x00000040
 
+/* Deprecated - do not use this in programs and do not add new flags here! */
 #define FAN_ALL_INIT_FLAGS	(FAN_CLOEXEC | FAN_NONBLOCK | \
 				 FAN_ALL_CLASS_BITS | FAN_UNLIMITED_QUEUE |\
 				 FAN_UNLIMITED_MARKS)

@@ -57,23 +60,18 @@
 #define FAN_MARK_INODE		0x00000000
 #define FAN_MARK_MOUNT		0x00000010
 #define FAN_MARK_FILESYSTEM	0x00000100
-#define FAN_MARK_TYPE_MASK	(FAN_MARK_INODE | FAN_MARK_MOUNT | \
-				 FAN_MARK_FILESYSTEM)
 
+/* Deprecated - do not use this in programs and do not add new flags here! */
 #define FAN_ALL_MARK_FLAGS	(FAN_MARK_ADD |\
 				 FAN_MARK_REMOVE |\
 				 FAN_MARK_DONT_FOLLOW |\
 				 FAN_MARK_ONLYDIR |\
+				 FAN_MARK_MOUNT |\
 				 FAN_MARK_IGNORED_MASK |\
 				 FAN_MARK_IGNORED_SURV_MODIFY |\
-				 FAN_MARK_FLUSH|\
-				 FAN_MARK_TYPE_MASK)
+				 FAN_MARK_FLUSH)
 
-/*
- * All of the events - we build the list by hand so that we can add flags in
- * the future and not break backward compatibility.  Apps will get only the
- * events that they originally wanted.  Be sure to add new events here!
- */
+/* Deprecated - do not use this in programs and do not add new flags here! */
 #define FAN_ALL_EVENTS (FAN_ACCESS |\
 			FAN_MODIFY |\
 			FAN_CLOSE |\

@@ -82,9 +80,11 @@
 /*
  * All events which require a permission response from userspace
  */
+/* Deprecated - do not use this in programs and do not add new flags here! */
 #define FAN_ALL_PERM_EVENTS (FAN_OPEN_PERM |\
 			     FAN_ACCESS_PERM)
 
+/* Deprecated - do not use this in programs and do not add new flags here! */
 #define FAN_ALL_OUTGOING_EVENTS	(FAN_ALL_EVENTS |\
 				 FAN_ALL_PERM_EVENTS |\
 				 FAN_Q_OVERFLOW)
Signature · 1 line
2.17.1
author reviewer
#5 lore raw

PATCH v3 5/8 fsnotify: convert runtime BUG_ON() to BUILD_BUG_ON()

In reply to Amir Goldstein

The BUG_ON() statements to verify number of bits in ALL_FSNOTIFY_BITS and ALL_INOTIFY_BITS are converted to build time check of the constant.

Signed-off-by Amir Goldstein <[email protected]>
---
 fs/notify/fsnotify.c             | 2 +-
 fs/notify/inotify/inotify_user.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
fs/notify/fsnotify.c +1 -1
index 7391a02bf723..875975504409 100644
@@ -395,7 +395,7 @@ static __init int fsnotify_init(void)
 {
 	int ret;
 
-	BUG_ON(hweight32(ALL_FSNOTIFY_BITS) != 23);
+	BUILD_BUG_ON(HWEIGHT32(ALL_FSNOTIFY_BITS) != 23);
 
 	ret = init_srcu_struct(&fsnotify_mark_srcu);
 	if (ret)
fs/notify/inotify/inotify_user.c +1 -1
index ac6978d3208c..105576daca4a 100644
@@ -815,7 +815,7 @@ static int __init inotify_user_setup(void)
 	BUILD_BUG_ON(IN_ISDIR != FS_ISDIR);
 	BUILD_BUG_ON(IN_ONESHOT != FS_IN_ONESHOT);
 
-	BUG_ON(hweight32(ALL_INOTIFY_BITS) != 22);
+	BUILD_BUG_ON(HWEIGHT32(ALL_INOTIFY_BITS) != 22);
 
 	inotify_inode_mark_cachep = KMEM_CACHE(inotify_inode_mark,
 					       SLAB_PANIC|SLAB_ACCOUNT);
Signature · 1 line
2.17.1
author reviewer
#6 lore raw

PATCH v3 6/8 fanotify: add BUILD_BUG_ON() to count the bits of fanotify constants

In reply to Amir Goldstein

Also define the FANOTIFY_EVENT_FLAGS consisting of the extra flags FAN_ONDIR and FAN_ON_CHILD.

Signed-off-by Amir Goldstein <[email protected]>
---
 fs/notify/fanotify/fanotify.c      | 2 ++
 fs/notify/fanotify/fanotify_user.c | 5 ++++-
 include/linux/fanotify.h           | 8 ++++++++
 3 files changed, 14 insertions(+), 1 deletion(-)
fs/notify/fanotify/fanotify.c +2 -0
index 03498eb995be..361e3a0a445c 100644
@@ -205,6 +205,8 @@ static int fanotify_handle_event(struct fsnotify_group *group,
 	BUILD_BUG_ON(FAN_ACCESS_PERM != FS_ACCESS_PERM);
 	BUILD_BUG_ON(FAN_ONDIR != FS_ISDIR);
 
+	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 10);
+
 	if (!fanotify_should_send_event(iter_info, mask, data, data_type))
 		return 0;
fs/notify/fanotify/fanotify_user.c +4 -1
index 530e5e486105..14594e491d2b 100644
@@ -803,7 +803,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
 	struct fsnotify_group *group;
 	struct fd f;
 	struct path path;
-	u32 valid_mask = FANOTIFY_EVENTS | FAN_EVENT_ON_CHILD | FAN_ONDIR;
+	u32 valid_mask = FANOTIFY_EVENTS | FANOTIFY_EVENT_FLAGS;
 	unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
 	int ret;
 

@@ -944,6 +944,9 @@ COMPAT_SYSCALL_DEFINE6(fanotify_mark,
  */
 static int __init fanotify_user_setup(void)
 {
+	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 6);
+	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_MARK_FLAGS) != 9);
+
 	fanotify_mark_cache = KMEM_CACHE(fsnotify_mark,
 					 SLAB_PANIC|SLAB_ACCOUNT);
 	fanotify_event_cachep = KMEM_CACHE(fanotify_event_info, SLAB_PANIC);
include/linux/fanotify.h +8 -0
index 4519b0988afe..afddd7e0d5a1 100644
@@ -41,11 +41,19 @@
 /* Events that require a permission response from user */
 #define FANOTIFY_PERM_EVENTS	(FAN_OPEN_PERM | FAN_ACCESS_PERM)
 
+/* Extra flags that may be reported with event or control handling of events */
+#define FANOTIFY_EVENT_FLAGS	(FAN_EVENT_ON_CHILD | FAN_ONDIR)
+
 /* Events that may be reported to user */
 #define FANOTIFY_OUTGOING_EVENTS	(FANOTIFY_EVENTS | \
 					 FANOTIFY_PERM_EVENTS | \
 					 FAN_Q_OVERFLOW)
 
+#define ALL_FANOTIFY_EVENT_BITS		(FANOTIFY_EVENTS | \
+					 FANOTIFY_PERM_EVENTS | \
+					 FANOTIFY_EVENT_FLAGS | \
+					 FANOTIFY_OUTGOING_EVENTS)
+
 /* Do not use these old uapi constants internally */
 #undef FAN_ALL_CLASS_BITS
 #undef FAN_ALL_INIT_FLAGS
Signature · 1 line
2.17.1
maintainer
#7 lore raw

Re: [PATCH v3 6/8] fanotify: add BUILD_BUG_ON() to count the bits of fanotify constants

Quoted 4 lines · Amir Goldstein

Also define the FANOTIFY_EVENT_FLAGS consisting of the extra flags FAN_ONDIR and FAN_ON_CHILD.

Signed-off-by Amir Goldstein <[email protected]>

...

include/linux/fanotify.h line 56

Quoted 10 lines

/* Events that may be reported to user */
#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENTS | \

include/linux/fanotify.h +5 -0
 					 FANOTIFY_PERM_EVENTS | \
 					 FAN_Q_OVERFLOW)
 
+#define ALL_FANOTIFY_EVENT_BITS		(FANOTIFY_EVENTS | \
+					 FANOTIFY_PERM_EVENTS | \
+					 FANOTIFY_EVENT_FLAGS | \
+					 FANOTIFY_OUTGOING_EVENTS)
+

No point in repeating FANOTIFY_EVENTS | FANOTIFY_PERM_EVENTS here when they are already included on FANOTIFY_OUTGOING_EVENTS, right? I can fix that up myself, just wanted to point it out.

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
author reviewer
#8 lore raw

Re: [PATCH v3 6/8] fanotify: add BUILD_BUG_ON() to count the bits of fanotify constants

Quoted 12 lines · Jan Kara
Quoted 4 lines · Amir Goldstein

Also define the FANOTIFY_EVENT_FLAGS consisting of the extra flags FAN_ONDIR and FAN_ON_CHILD.

Signed-off-by Amir Goldstein <[email protected]>

...

Quoted 5 lines

/* Events that may be reported to user */

 #define FANOTIFY_OUTGOING_EVENTS     (FANOTIFY_EVENTS | \
                                       FANOTIFY_PERM_EVENTS | \
                                       FAN_Q_OVERFLOW)
Quoted 9 lines

include/linux/fanotify.h line 56

Quoted 5 lines
include/linux/fanotify.h +5 -0
+#define ALL_FANOTIFY_EVENT_BITS              (FANOTIFY_EVENTS | \
+                                      FANOTIFY_PERM_EVENTS | \
+                                      FANOTIFY_EVENT_FLAGS | \
+                                      FANOTIFY_OUTGOING_EVENTS)
+

No point in repeating FANOTIFY_EVENTS | FANOTIFY_PERM_EVENTS here when they are already included on FANOTIFY_OUTGOING_EVENTS, right? I can fix that up myself, just wanted to point it out.

It's fine to me.

Thanks,
Amir.

author reviewer
#9 lore raw

PATCH v3 7/8 fanotify: support reporting thread id instead of process id

In reply to Amir Goldstein

In order to identify which thread triggered the event in a multi-threaded program, add the FAN_EVENT_INFO_TID flag in fanotify_init to opt-in for reporting the event creator's thread id information.

Signed-off-by nixiaoming <[email protected]>
Signed-off-by Amir Goldstein <[email protected]>
---
 fs/notify/fanotify/fanotify.c      | 9 ++++++---
 fs/notify/fanotify/fanotify.h      | 2 +-
 fs/notify/fanotify/fanotify_user.c | 4 ++--
 include/linux/fanotify.h           | 3 +++
 include/uapi/linux/fanotify.h      | 3 +++
 5 files changed, 15 insertions(+), 6 deletions(-)
fs/notify/fanotify/fanotify.c +6 -3
index 361e3a0a445c..2c57186caa2e 100644
@@ -25,7 +25,7 @@ static bool should_merge(struct fsnotify_event *old_fsn,
 	old = FANOTIFY_E(old_fsn);
 	new = FANOTIFY_E(new_fsn);
 
-	if (old_fsn->inode == new_fsn->inode && old->tgid == new->tgid &&
+	if (old_fsn->inode == new_fsn->inode && old->pid == new->pid &&
 	    old->path.mnt == new->path.mnt &&
 	    old->path.dentry == new->path.dentry)
 		return true;

@@ -171,7 +171,10 @@ struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group,
 		goto out;
 init: __maybe_unused
 	fsnotify_init_event(&event->fse, inode, mask);
-	event->tgid = get_pid(task_tgid(current));
+	if (FAN_GROUP_FLAG(group, FAN_EVENT_INFO_TID))
+		event->pid = get_pid(task_pid(current));
+	else
+		event->pid = get_pid(task_tgid(current));
 	if (path) {
 		event->path = *path;
 		path_get(&event->path);

@@ -270,7 +273,7 @@ static void fanotify_free_event(struct fsnotify_event *fsn_event)
 
 	event = FANOTIFY_E(fsn_event);
 	path_put(&event->path);
-	put_pid(event->tgid);
+	put_pid(event->pid);
 	if (fanotify_is_perm_event(fsn_event->mask)) {
 		kmem_cache_free(fanotify_perm_event_cachep,
 				FANOTIFY_PE(fsn_event));
fs/notify/fanotify/fanotify.h +1 -1
index 88a8290a61cb..ea05b8a401e7 100644
@@ -19,7 +19,7 @@ struct fanotify_event_info {
 	 * during this object's lifetime
 	 */
 	struct path path;
-	struct pid *tgid;
+	struct pid *pid;
 };
 
 /*
fs/notify/fanotify/fanotify_user.c +2 -2
index 14594e491d2b..e03be5071362 100644
@@ -132,7 +132,7 @@ static int fill_event_metadata(struct fsnotify_group *group,
 	metadata->vers = FANOTIFY_METADATA_VERSION;
 	metadata->reserved = 0;
 	metadata->mask = fsn_event->mask & FANOTIFY_OUTGOING_EVENTS;
-	metadata->pid = pid_vnr(event->tgid);
+	metadata->pid = pid_vnr(event->pid);
 	if (unlikely(fsn_event->mask & FAN_Q_OVERFLOW))
 		metadata->fd = FAN_NOFD;
 	else {

@@ -944,7 +944,7 @@ COMPAT_SYSCALL_DEFINE6(fanotify_mark,
  */
 static int __init fanotify_user_setup(void)
 {
-	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 6);
+	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_INIT_FLAGS) != 7);
 	BUILD_BUG_ON(HWEIGHT32(FANOTIFY_MARK_FLAGS) != 9);
 
 	fanotify_mark_cache = KMEM_CACHE(fsnotify_mark,
include/linux/fanotify.h +3 -0
index afddd7e0d5a1..05b696b4856b 100644
@@ -18,7 +18,10 @@
 #define FANOTIFY_CLASS_BITS	(FAN_CLASS_NOTIF | FAN_CLASS_CONTENT | \
 				 FAN_CLASS_PRE_CONTENT)
 
+#define FANOTIFY_EVENT_INFO_FLAGS	(FAN_EVENT_INFO_TID)
+
 #define FANOTIFY_INIT_FLAGS	(FANOTIFY_CLASS_BITS | \
+				 FANOTIFY_EVENT_INFO_FLAGS | \
 				 FAN_CLOEXEC | FAN_NONBLOCK | \
 				 FAN_UNLIMITED_QUEUE | FAN_UNLIMITED_MARKS)
include/uapi/linux/fanotify.h +3 -0
index d0c05de670ef..00b2304ed124 100644
@@ -40,6 +40,9 @@
 #define FAN_UNLIMITED_MARKS	0x00000020
 #define FAN_ENABLE_AUDIT	0x00000040
 
+/* Flags to determine fanotify event format */
+#define FAN_EVENT_INFO_TID	0x00000100	/* event->pid is thread id */
+
 /* Deprecated - do not use this in programs and do not add new flags here! */
 #define FAN_ALL_INIT_FLAGS	(FAN_CLOEXEC | FAN_NONBLOCK | \
 				 FAN_ALL_CLASS_BITS | FAN_UNLIMITED_QUEUE |\
Signature · 1 line
2.17.1
maintainer
#10 lore raw

Re: [PATCH v3 7/8] fanotify: support reporting thread id instead of process id

Quoted 6 lines · Amir Goldstein

In order to identify which thread triggered the event in a multi-threaded program, add the FAN_EVENT_INFO_TID flag in fanotify_init to opt-in for reporting the event creator's thread id information.

Signed-off-by nixiaoming <[email protected]>
Signed-off-by Amir Goldstein <[email protected]>

...

Quoted 5 lines
include/linux/fanotify.h +0 -0
index afddd7e0d5a1..05b696b4856b 100644

include/linux/fanotify.h line 26

Quoted 9 lines
include/linux/fanotify.h +3 -0
@@ -18,7 +18,10 @@
 #define FANOTIFY_CLASS_BITS	(FAN_CLASS_NOTIF | FAN_CLASS_CONTENT | \
 				 FAN_CLASS_PRE_CONTENT)
 
+#define FANOTIFY_EVENT_INFO_FLAGS	(FAN_EVENT_INFO_TID)
+
 #define FANOTIFY_INIT_FLAGS	(FANOTIFY_CLASS_BITS | \
+				 FANOTIFY_EVENT_INFO_FLAGS | \
 				 FAN_CLOEXEC | FAN_NONBLOCK | \
 				 FAN_UNLIMITED_QUEUE | FAN_UNLIMITED_MARKS)

Is there reason to define FANOTIFY_EVENT_INFO_FLAGS? But I guess you prepare with this for further changes you want to do? Just that at this point it looks a bit silly and I cannot really think of a reason why we'd like to distinguish flags in FANOTIFY_EVENT_INFO_FLAGS from other flags in FANOTIFY_INIT_FLAGS...

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
author reviewer
#11 lore raw

Re: [PATCH v3 7/8] fanotify: support reporting thread id instead of process id

Quoted 17 lines · Jan Kara
Quoted 6 lines · Amir Goldstein

In order to identify which thread triggered the event in a multi-threaded program, add the FAN_EVENT_INFO_TID flag in fanotify_init to opt-in for reporting the event creator's thread id information.

Signed-off-by nixiaoming <[email protected]>
Signed-off-by Amir Goldstein <[email protected]>

...

Quoted 8 lines
include/linux/fanotify.h +0 -0
index afddd7e0d5a1..05b696b4856b 100644
@@ -18,7 +18,10 @@
 #define FANOTIFY_CLASS_BITS  (FAN_CLASS_NOTIF | FAN_CLASS_CONTENT | \
                               FAN_CLASS_PRE_CONTENT)
Quoted 13 lines

include/linux/fanotify.h line 26

Quoted 6 lines
include/linux/fanotify.h +3 -0
+#define FANOTIFY_EVENT_INFO_FLAGS    (FAN_EVENT_INFO_TID)
+
 #define FANOTIFY_INIT_FLAGS  (FANOTIFY_CLASS_BITS | \
+                              FANOTIFY_EVENT_INFO_FLAGS | \
                               FAN_CLOEXEC | FAN_NONBLOCK | \
                               FAN_UNLIMITED_QUEUE | FAN_UNLIMITED_MARKS)

Is there reason to define FANOTIFY_EVENT_INFO_FLAGS? But I guess you prepare with this for further changes you want to do? Just that at this point it looks a bit silly and I cannot really think of a reason why we'd like to distinguish flags in FANOTIFY_EVENT_INFO_FLAGS from other flags in FANOTIFY_INIT_FLAGS...

No functional reason. Feel free to drop FANOTIFY_EVENT_INFO_FLAGS.

Thanks,
Amir.

maintainer
#12 lore raw

Re: [PATCH v3 7/8] fanotify: support reporting thread id instead of process id

In reply to Amir Goldstein
Quoted 6 lines · Amir Goldstein

In order to identify which thread triggered the event in a multi-threaded program, add the FAN_EVENT_INFO_TID flag in fanotify_init to opt-in for reporting the event creator's thread id information.

Signed-off-by nixiaoming <[email protected]>
Signed-off-by Amir Goldstein <[email protected]>

Just one question occurred to me (after discussion with a colleague about this feature): Ming, why do you actually need to know thread ID and thread-group ID is not enough? Also note that standard glibc threading functions are *not* going to be compatible with the ID returned from fanotify (e.g. it will be different from POSIX thread ID as returned by pthread_self()). So the feature is somewhat difficult to use from userspace... (at least you could use gettid() systemcall to get the ID of current thread but there's not glibc wrapper for it).

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
#13 lore raw

RE: [PATCH v3 7/8] fanotify: support reporting thread id instead of process id

Quoted 8 lines · Jan Kara
Quoted 6 lines · Amir Goldstein

In order to identify which thread triggered the event in a multi-threaded program, add the FAN_EVENT_INFO_TID flag in fanotify_init to opt-in for reporting the event creator's thread id information.

Signed-off-by nixiaoming <[email protected]>
Signed-off-by Amir Goldstein <[email protected]>
Quoted 9 lines

Just one question occurred to me (after discussion with a colleague about this feature): Ming, why do you actually need to know thread ID and thread-group ID is not enough? Also note that standard glibc threading functions are *not* going to be compatible with the ID returned from fanotify (e.g. it will be different from POSIX thread ID as returned by pthread_self()). So the feature is somewhat difficult to use from userspace... (at least you could use gettid() systemcall to get the ID of current thread but there's not glibc wrapper for it).

When using fanotify to monitor system critical directories or files,
the monitor and event trigger are not the same process.
The monitoring task wants to know which task triggers the event. If the event is triggered by multiple threads,
I hope to know which thread is triggered by the task,
can't see the pthread id here, but I can see the tid.

Thanks

maintainer
#14 lore raw

Re: [PATCH v3 7/8] fanotify: support reporting thread id instead of process id

Quoted 19 lines · Nixiaoming
Quoted 8 lines · Jan Kara
Quoted 6 lines · Amir Goldstein

In order to identify which thread triggered the event in a multi-threaded program, add the FAN_EVENT_INFO_TID flag in fanotify_init to opt-in for reporting the event creator's thread id information.

Signed-off-by nixiaoming <[email protected]>
Signed-off-by Amir Goldstein <[email protected]>
Quoted 9 lines

Just one question occurred to me (after discussion with a colleague about this feature): Ming, why do you actually need to know thread ID and thread-group ID is not enough? Also note that standard glibc threading functions are *not* going to be compatible with the ID returned from fanotify (e.g. it will be different from POSIX thread ID as returned by pthread_self()). So the feature is somewhat difficult to use from userspace... (at least you could use gettid() systemcall to get the ID of current thread but there's not glibc wrapper for it).

Quoted 6 lines

When using fanotify to monitor system critical directories or files,
the monitor and event trigger are not the same process.
The monitoring task wants to know which task triggers the event. If the event is triggered by multiple threads,
I hope to know which thread is triggered by the task,
can't see the pthread id here, but I can see the tid.

OK, thanks for clarification.

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
author reviewer
#15 lore raw

PATCH v3 8/8 fsnotify: optimize away srcu_read_lock() for events on directories

In reply to Amir Goldstein

inotify and dnotify are always interested in events on directories. fanotify is interested in events on directories only if user requested them by flag FAN_ONDIR.

For the fanotify supported events on directories (open/access/close), if there is no inotify/dnotify associated marks and if none of the associated fanotify marks have the FS_ISDIR flag, we can skip iterating the marks and we avoid taking the srcu_read_lock() for all events on directories.

In order to enable this check in fsnotify() we set the FS_ISDIR flag explicitly on all dnotify/inotify marks.

Signed-off-by Amir Goldstein <[email protected]>
---
 fs/notify/dnotify/dnotify.c      | 5 +++--
 fs/notify/fsnotify.c             | 9 ++++++++-
 fs/notify/inotify/inotify_user.c | 4 ++--
 3 files changed, 13 insertions(+), 5 deletions(-)
fs/notify/dnotify/dnotify.c +3 -2
index 58d77dc696eb..55f0ae5fe0ba 100644
@@ -89,7 +89,7 @@ static int dnotify_handle_event(struct fsnotify_group *group,
 	struct dnotify_struct *dn;
 	struct dnotify_struct **prev;
 	struct fown_struct *fown;
-	__u32 test_mask = mask & ~FS_EVENT_ON_CHILD;
+	__u32 test_mask = mask & ALL_FSNOTIFY_EVENTS;
 
 	/* not a dir, dnotify doesn't care */
 	if (!S_ISDIR(inode->i_mode))

@@ -197,7 +197,8 @@ void dnotify_flush(struct file *filp, fl_owner_t id)
 /* this conversion is done only at watch creation */
 static __u32 convert_arg(unsigned long arg)
 {
-	__u32 new_mask = FS_EVENT_ON_CHILD;
+	/* Interested in events on children including subdirs */
+	__u32 new_mask = FS_EVENT_ON_CHILD | FS_ISDIR;
 
 	if (arg & DN_MULTISHOT)
 		new_mask |= FS_DN_MULTISHOT;
fs/notify/fsnotify.c +8 -1
index 875975504409..a0ad06285450 100644
@@ -332,9 +332,16 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is,
 	__u32 test_mask = (mask & ALL_FSNOTIFY_EVENTS);
 
 	if (data_is == FSNOTIFY_EVENT_PATH) {
-		mnt = real_mount(((const struct path *)data)->mnt);
+		const struct path *path = data;
+
+		mnt = real_mount(path->mnt);
 		sb = mnt->mnt.mnt_sb;
 		mnt_or_sb_mask = mnt->mnt_fsnotify_mask | sb->s_fsnotify_mask;
+
+		/* Is anybody is interested in events on directories? */
+		if (d_is_dir(path->dentry) &&
+		    !(FS_ISDIR & (to_tell->i_fsnotify_mask | mnt_or_sb_mask)))
+			return 0;
 	}
 
 	/*
fs/notify/inotify/inotify_user.c +2 -2
index 105576daca4a..8d163b96a86a 100644
@@ -91,9 +91,9 @@ static inline __u32 inotify_arg_to_mask(u32 arg)
 
 	/*
 	 * everything should accept their own ignored, cares about children,
-	 * and should receive events when the inode is unmounted
+	 * directories, and should receive events when the inode is unmounted.
 	 */
-	mask = (FS_IN_IGNORED | FS_EVENT_ON_CHILD | FS_UNMOUNT);
+	mask = (FS_IN_IGNORED | FS_EVENT_ON_CHILD | FS_ISDIR | FS_UNMOUNT);
 
 	/* mask off the flags used to open the fd */
 	mask |= (arg & (IN_ALL_EVENTS | IN_ONESHOT | IN_EXCL_UNLINK));
Signature · 1 line
2.17.1
maintainer
#16 lore raw

Re: [PATCH v3 8/8] fsnotify: optimize away srcu_read_lock() for events on directories

Quoted 13 lines · Amir Goldstein

inotify and dnotify are always interested in events on directories. fanotify is interested in events on directories only if user requested them by flag FAN_ONDIR.

For the fanotify supported events on directories (open/access/close), if there is no inotify/dnotify associated marks and if none of the associated fanotify marks have the FS_ISDIR flag, we can skip iterating the marks and we avoid taking the srcu_read_lock() for all events on directories.

In order to enable this check in fsnotify() we set the FS_ISDIR flag explicitly on all dnotify/inotify marks.

Quoted 6 lines
Signed-off-by Amir Goldstein <[email protected]>
---
 fs/notify/dnotify/dnotify.c      | 5 +++--
 fs/notify/fsnotify.c             | 9 ++++++++-
 fs/notify/inotify/inotify_user.c | 4 ++--
 3 files changed, 13 insertions(+), 5 deletions(-)

Looks good, except I think you forgot to convert kernel/audit_watch.c and kernel/audit_fsnotify.c which also need to set FS_ISDIR...

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
author reviewer
#17 lore raw

Re: [PATCH v3 8/8] fsnotify: optimize away srcu_read_lock() for events on directories

Quoted 22 lines · Jan Kara
Quoted 13 lines · Amir Goldstein

inotify and dnotify are always interested in events on directories. fanotify is interested in events on directories only if user requested them by flag FAN_ONDIR.

For the fanotify supported events on directories (open/access/close), if there is no inotify/dnotify associated marks and if none of the associated fanotify marks have the FS_ISDIR flag, we can skip iterating the marks and we avoid taking the srcu_read_lock() for all events on directories.

In order to enable this check in fsnotify() we set the FS_ISDIR flag explicitly on all dnotify/inotify marks.

Quoted 6 lines
Signed-off-by Amir Goldstein <[email protected]>
---
 fs/notify/dnotify/dnotify.c      | 5 +++--
 fs/notify/fsnotify.c             | 9 ++++++++-
 fs/notify/inotify/inotify_user.c | 4 ++--
 3 files changed, 13 insertions(+), 5 deletions(-)
Quoted 3 lines

Looks good, except I think you forgot to convert kernel/audit_watch.c and kernel/audit_fsnotify.c which also need to set FS_ISDIR...

Oops. Right.
So would you rather I come back with a fixed version after I ran some performance tests?
Or will you fix and test it yourself?

Thanks,
Amir.

maintainer
#18 lore raw

Re: [PATCH v3 8/8] fsnotify: optimize away srcu_read_lock() for events on directories

Quoted 23 lines · Amir Goldstein
Quoted 22 lines · Jan Kara
Quoted 19 lines · Amir Goldstein

inotify and dnotify are always interested in events on directories. fanotify is interested in events on directories only if user requested them by flag FAN_ONDIR.

For the fanotify supported events on directories (open/access/close), if there is no inotify/dnotify associated marks and if none of the associated fanotify marks have the FS_ISDIR flag, we can skip iterating the marks and we avoid taking the srcu_read_lock() for all events on directories.

In order to enable this check in fsnotify() we set the FS_ISDIR flag explicitly on all dnotify/inotify marks.

Signed-off-by Amir Goldstein <[email protected]>
---
 fs/notify/dnotify/dnotify.c      | 5 +++--
 fs/notify/fsnotify.c             | 9 ++++++++-
 fs/notify/inotify/inotify_user.c | 4 ++--
 3 files changed, 13 insertions(+), 5 deletions(-)
Quoted 8 lines
Quoted 3 lines

Looks good, except I think you forgot to convert kernel/audit_watch.c and kernel/audit_fsnotify.c which also need to set FS_ISDIR...

Oops. Right.
So would you rather I come back with a fixed version after I ran some performance tests?
Or will you fix and test it yourself?

Let's postpone this patch for a bit later. I'll merge the remaining seven patches to my tree for this merge window.

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
author reviewer
#19 lore raw

Re: [PATCH v3 8/8] fsnotify: optimize away srcu_read_lock() for events on directories

Quoted 29 lines · Jan Kara
Quoted 27 lines · Amir Goldstein
Quoted 25 lines · Jan Kara
Quoted 13 lines · Amir Goldstein

inotify and dnotify are always interested in events on directories. fanotify is interested in events on directories only if user requested them by flag FAN_ONDIR.

For the fanotify supported events on directories (open/access/close), if there is no inotify/dnotify associated marks and if none of the associated fanotify marks have the FS_ISDIR flag, we can skip iterating the marks and we avoid taking the srcu_read_lock() for all events on directories.

In order to enable this check in fsnotify() we set the FS_ISDIR flag explicitly on all dnotify/inotify marks.

Quoted 6 lines
Signed-off-by Amir Goldstein <[email protected]>
---
 fs/notify/dnotify/dnotify.c      | 5 +++--
 fs/notify/fsnotify.c             | 9 ++++++++-
 fs/notify/inotify/inotify_user.c | 4 ++--
 3 files changed, 13 insertions(+), 5 deletions(-)

Looks good, except I think you forgot to convert kernel/audit_watch.c and kernel/audit_fsnotify.c which also need to set FS_ISDIR...

Quoted 8 lines
Quoted 4 lines

Oops. Right.
So would you rather I come back with a fixed version after I ran some performance tests?
Or will you fix and test it yourself?

Let's postpone this patch for a bit later. I'll merge the remaining seven patches to my tree for this merge window.

For the records, On my small test system, I wasn't able to measure a difference in performance of parallel opendir/readdir/closedir workload (filebench listdirs) with and without a mount mark regardless of the optimization patch.

Benchmark reports 40K~42K ops/sec for the compound op of "listdir".

Perhaps more complex workloads that actually have SRCU sleeps would benefit from this optimization, but it doesn't look like avoiding the memory barrier alone moves the niddle. Not very surprising considering that readdir takes a shared lock anyway.

So dropping the patch until further notice.

Thanks,
Amir.