[PATCH v3 0/8] New fanotify event info API

[PATCH v3 0/8] New fanotify event info API

[Amir Goldstein]

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(-)

-- 
2.17.1

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

[Amir Goldstein]

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 <amir73il@gmail.com>
---
 include/linux/fanotify.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index 9c5ea3bdfaa0..e70fccc3757e 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -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))
-- 
2.17.1

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

[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 <amir73il@gmail.com>
---
 fs/notify/fsnotify.c             | 7 +++----
 include/linux/fsnotify_backend.h | 9 +++++++--
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 422fbc6dffde..7391a02bf723 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -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)
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 8e91341cbd8a..135b973e44d1 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -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;
-- 
2.17.1

[PATCH v3 3/8] fanotify: simplify handling of FAN_ONDIR

[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 <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify_user.c | 32 +++++-------------------------
 include/linux/fanotify.h           |  3 ---
 2 files changed, 5 insertions(+), 30 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 15719d4aa4b5..34b511407035 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -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;
 
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index e70fccc3757e..a8c3fc54276d 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -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))
 
-- 
2.17.1

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

[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: <linux-api@vger.kernel.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 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(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 94b52157bf8d..03498eb995be 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -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);
 
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 8609ba06f474..88a8290a61cb 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -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)
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 34b511407035..530e5e486105 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -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;
 
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index a8c3fc54276d..4519b0988afe 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -7,4 +7,51 @@
 #define FAN_GROUP_FLAG(group, flag) \
 	((group)->fanotify_data.flags & (flag))
 
+/*
+ * Flags allowed to be passed from/to userspace.
+ *
+ * We intentionally do not add new bits to the old FAN_ALL_* constants, because
+ * they are uapi exposed constants. If there are programs out there using
+ * these constant, the programs may break if re-compiled with new uapi headers
+ * and then run on an old kernel.
+ */
+#define FANOTIFY_CLASS_BITS	(FAN_CLASS_NOTIF | FAN_CLASS_CONTENT | \
+				 FAN_CLASS_PRE_CONTENT)
+
+#define FANOTIFY_INIT_FLAGS	(FANOTIFY_CLASS_BITS | \
+				 FAN_CLOEXEC | FAN_NONBLOCK | \
+				 FAN_UNLIMITED_QUEUE | FAN_UNLIMITED_MARKS)
+
+#define FANOTIFY_MARK_TYPE_BITS	(FAN_MARK_INODE | FAN_MARK_MOUNT | \
+				 FAN_MARK_FILESYSTEM)
+
+#define FANOTIFY_MARK_FLAGS	(FANOTIFY_MARK_TYPE_BITS | \
+				 FAN_MARK_ADD | \
+				 FAN_MARK_REMOVE | \
+				 FAN_MARK_DONT_FOLLOW | \
+				 FAN_MARK_ONLYDIR | \
+				 FAN_MARK_IGNORED_MASK | \
+				 FAN_MARK_IGNORED_SURV_MODIFY | \
+				 FAN_MARK_FLUSH)
+
+/* Events that user can request to be notified on */
+#define FANOTIFY_EVENTS		(FAN_ACCESS | FAN_MODIFY | \
+				 FAN_CLOSE | FAN_OPEN)
+
+/* Events that require a permission response from user */
+#define FANOTIFY_PERM_EVENTS	(FAN_OPEN_PERM | FAN_ACCESS_PERM)
+
+/* Events that may be reported to user */
+#define FANOTIFY_OUTGOING_EVENTS	(FANOTIFY_EVENTS | \
+					 FANOTIFY_PERM_EVENTS | \
+					 FAN_Q_OVERFLOW)
+
+/* Do not use these old uapi constants internally */
+#undef FAN_ALL_CLASS_BITS
+#undef FAN_ALL_INIT_FLAGS
+#undef FAN_ALL_MARK_FLAGS
+#undef FAN_ALL_EVENTS
+#undef FAN_ALL_PERM_EVENTS
+#undef FAN_ALL_OUTGOING_EVENTS
+
 #endif /* _LINUX_FANOTIFY_H */
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index ad81234d1919..d0c05de670ef 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -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)
-- 
2.17.1

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

[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 <amir73il@gmail.com>
---
 fs/notify/fsnotify.c             | 2 +-
 fs/notify/inotify/inotify_user.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 7391a02bf723..875975504409 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -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)
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index ac6978d3208c..105576daca4a 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -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);
-- 
2.17.1

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

[Amir Goldstein]

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

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 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(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 03498eb995be..361e3a0a445c 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -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;
 
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 530e5e486105..14594e491d2b 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -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);
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index 4519b0988afe..afddd7e0d5a1 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -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
-- 
2.17.1

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

[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 <nixiaoming@huawei.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 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(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 361e3a0a445c..2c57186caa2e 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -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));
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 88a8290a61cb..ea05b8a401e7 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -19,7 +19,7 @@ struct fanotify_event_info {
 	 * during this object's lifetime
 	 */
 	struct path path;
-	struct pid *tgid;
+	struct pid *pid;
 };
 
 /*
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 14594e491d2b..e03be5071362 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -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,
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index afddd7e0d5a1..05b696b4856b 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -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)
 
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index d0c05de670ef..00b2304ed124 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -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 |\
-- 
2.17.1

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

[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 <amir73il@gmail.com>
---
 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(-)

diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index 58d77dc696eb..55f0ae5fe0ba 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -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;
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 875975504409..a0ad06285450 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -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;
 	}
 
 	/*
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 105576daca4a..8d163b96a86a 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -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));
-- 
2.17.1

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

[Jan Kara]

On Thu 04-10-18 00:25:37, Amir Goldstein wrote:
> Also define the FANOTIFY_EVENT_FLAGS consisting of the extra flags
> FAN_ONDIR and FAN_ON_CHILD.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
...
>  /* 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)
> +

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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Jan Kara]

On Thu 04-10-18 00:25:38, Amir Goldstein wrote:
> 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 <nixiaoming@huawei.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
...
> diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> index afddd7e0d5a1..05b696b4856b 100644
> --- a/include/linux/fanotify.h
> +++ b/include/linux/fanotify.h
> @@ -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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Jan Kara]

On Thu 04-10-18 00:25:39, Amir Goldstein wrote:
> 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 <amir73il@gmail.com>
> ---
>  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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Amir Goldstein]

On Thu, Oct 4, 2018 at 11:41 AM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 04-10-18 00:25:37, Amir Goldstein wrote:
> > Also define the FANOTIFY_EVENT_FLAGS consisting of the extra flags
> > FAN_ONDIR and FAN_ON_CHILD.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ...
> >  /* 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)
> > +
>
> 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.

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

[Amir Goldstein]

On Thu, Oct 4, 2018 at 11:46 AM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 04-10-18 00:25:38, Amir Goldstein wrote:
> > 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 <nixiaoming@huawei.com>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ...
> > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> > index afddd7e0d5a1..05b696b4856b 100644
> > --- a/include/linux/fanotify.h
> > +++ b/include/linux/fanotify.h
> > @@ -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...
>

No functional reason. Feel free to drop FANOTIFY_EVENT_INFO_FLAGS.

Thanks,
Amir.

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

[Amir Goldstein]

On Thu, Oct 4, 2018 at 12:09 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 04-10-18 00:25:39, Amir Goldstein wrote:
> > 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 <amir73il@gmail.com>
> > ---
> >  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...
>

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.

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

[Jan Kara]

On Thu 04-10-18 13:30:38, Amir Goldstein wrote:
> On Thu, Oct 4, 2018 at 12:09 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Thu 04-10-18 00:25:39, Amir Goldstein wrote:
> > > 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 <amir73il@gmail.com>
> > > ---
> > >  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...
> >
> 
> 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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Amir Goldstein]

On Thu, Oct 4, 2018 at 2:26 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 04-10-18 13:30:38, Amir Goldstein wrote:
> > On Thu, Oct 4, 2018 at 12:09 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Thu 04-10-18 00:25:39, Amir Goldstein wrote:
> > > > 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 <amir73il@gmail.com>
> > > > ---
> > > >  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...
> > >
> >
> > 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.

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

[Jan Kara]

On Thu 04-10-18 00:25:38, Amir Goldstein wrote:
> 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 <nixiaoming@huawei.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

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

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

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

[Nixiaoming]

On Thu, Oct 11, 2018 at 6:16 PM Jan Kara <mailto:jack@suse.cz> wrote:
>On Thu 04-10-18 00:25:38, Amir Goldstein wrote:
>> 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 <nixiaoming@huawei.com>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
>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

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

[Jan Kara]

On Fri 12-10-18 02:43:02, Nixiaoming wrote:
> On Thu, Oct 11, 2018 at 6:16 PM Jan Kara <mailto:jack@suse.cz> wrote:
> >On Thu 04-10-18 00:25:38, Amir Goldstein wrote:
> >> 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 <nixiaoming@huawei.com>
> >> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> >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.

OK, thanks for clarification.

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