[PATCH 0/7] fsnotify fixes and cleanups

[PATCH 0/7] fsnotify fixes and cleanups

[Amir Goldstein]

Jan,

I was working on some non urgent cleanups and stumbled on a bug,
so flushing my patch queue as is.

Patches 1-2 are cleanups needed for the bug fix in patch 3.
This [1] LTP test demonstrates the bug.

Patches 4-5 are pretty simple cleanups that you may or may not like
to apply without the work that they build up to (I started this as
prep work for subtree marks iterator).

Patches 6-7 are optimizations related to ignored mask which we
discussed in the past.  I have written them a while back and had put
them aside because I have no means to run performance tests that will
demonstrate the benefit, which is probably not huge.

Since you suggested those optimizations (at least the first one),
I decided to post and let you choose what to do with them.

Thanks,
Amir.

[1] https://github.com/amir73il/ltp/commits/fsnotify-fixes

Amir Goldstein (7):
  fsnotify: generalize handle_inode_event()
  inotify: convert to handle_inode_event() interface
  fsnotify: fix events reported to watching parent and child
  fsnotify: clarify object type argument
  fsnotify: separate mark iterator type from object type enum
  fsnotify: optimize FS_MODIFY events with no ignored masks
  fsnotify: optimize merging of marks with no ignored masks

 fs/nfsd/filecache.c                  |   2 +-
 fs/notify/dnotify/dnotify.c          |   2 +-
 fs/notify/fanotify/fanotify.c        |  16 +--
 fs/notify/fanotify/fanotify_user.c   |  44 +++++---
 fs/notify/fsnotify.c                 | 147 +++++++++++++++++----------
 fs/notify/group.c                    |   2 +-
 fs/notify/inotify/inotify.h          |   9 +-
 fs/notify/inotify/inotify_fsnotify.c |  47 ++-------
 fs/notify/inotify/inotify_user.c     |   7 +-
 fs/notify/mark.c                     |  30 +++---
 include/linux/fsnotify_backend.h     |  92 +++++++++++------
 kernel/audit_fsnotify.c              |   2 +-
 kernel/audit_tree.c                  |   2 +-
 kernel/audit_watch.c                 |   2 +-
 14 files changed, 233 insertions(+), 171 deletions(-)

-- 
2.25.1

[PATCH 1/7] fsnotify: generalize handle_inode_event()

[Amir Goldstein]

The handle_inode_event() interface was added as (quoting comment):
"a simple variant of handle_event() for groups that only have inode
marks and don't have ignore mask".

In other words, all backends except fanotify.  The inotify backend
also falls under this category, but because it required extra arguments
it was left out of the initial pass of backends conversion to the
simple interface.

This results in code duplication between the generic helper
fsnotify_handle_event() and the inotify_handle_event() callback
which also happen to be buggy code.

Generalize the handle_inode_event() arguments and add the check for
FS_EXCL_UNLINK flag to the generic helper, so inotify backend could
be converted to use the simple interface.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/nfsd/filecache.c              |  2 +-
 fs/notify/dnotify/dnotify.c      |  2 +-
 fs/notify/fsnotify.c             | 31 ++++++++++++++++++++++++-------
 include/linux/fsnotify_backend.h |  3 ++-
 kernel/audit_fsnotify.c          |  2 +-
 kernel/audit_tree.c              |  2 +-
 kernel/audit_watch.c             |  2 +-
 7 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 3c6c2f7d1688..5849c1bd88f1 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -600,7 +600,7 @@ static struct notifier_block nfsd_file_lease_notifier = {
 static int
 nfsd_file_fsnotify_handle_event(struct fsnotify_mark *mark, u32 mask,
 				struct inode *inode, struct inode *dir,
-				const struct qstr *name)
+				const struct qstr *name, u32 cookie)
 {
 	trace_nfsd_file_fsnotify_handle_event(inode, mask);
 
diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index 5dcda8f20c04..e45ca6ecba95 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -72,7 +72,7 @@ static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
  */
 static int dnotify_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
 				struct inode *inode, struct inode *dir,
-				const struct qstr *name)
+				const struct qstr *name, u32 cookie)
 {
 	struct dnotify_mark *dn_mark;
 	struct dnotify_struct *dn;
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 8d3ad5ef2925..c5c68bcbaadf 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -232,6 +232,26 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
 }
 EXPORT_SYMBOL_GPL(__fsnotify_parent);
 
+static int fsnotify_handle_inode_event(struct fsnotify_group *group,
+				       struct fsnotify_mark *inode_mark,
+				       u32 mask, const void *data, int data_type,
+				       struct inode *dir, const struct qstr *name,
+				       u32 cookie)
+{
+	const struct path *path = fsnotify_data_path(data, data_type);
+	struct inode *inode = fsnotify_data_inode(data, data_type);
+	const struct fsnotify_ops *ops = group->ops;
+
+	if (WARN_ON_ONCE(!ops->handle_inode_event))
+		return 0;
+
+	if ((inode_mark->mask & FS_EXCL_UNLINK) &&
+	    path && d_unlinked(path->dentry))
+		return 0;
+
+	return ops->handle_inode_event(inode_mark, mask, inode, dir, name, cookie);
+}
+
 static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask,
 				 const void *data, int data_type,
 				 struct inode *dir, const struct qstr *name,
@@ -239,13 +259,8 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask,
 {
 	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
 	struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info);
-	struct inode *inode = fsnotify_data_inode(data, data_type);
-	const struct fsnotify_ops *ops = group->ops;
 	int ret;
 
-	if (WARN_ON_ONCE(!ops->handle_inode_event))
-		return 0;
-
 	if (WARN_ON_ONCE(fsnotify_iter_sb_mark(iter_info)) ||
 	    WARN_ON_ONCE(fsnotify_iter_vfsmount_mark(iter_info)))
 		return 0;
@@ -262,7 +277,8 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask,
 		name = NULL;
 	}
 
-	ret = ops->handle_inode_event(inode_mark, mask, inode, dir, name);
+	ret = fsnotify_handle_inode_event(group, inode_mark, mask, data, data_type,
+					  dir, name, cookie);
 	if (ret || !child_mark)
 		return ret;
 
@@ -272,7 +288,8 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask,
 	 * report the event once to parent dir with name and once to child
 	 * without name.
 	 */
-	return ops->handle_inode_event(child_mark, mask, inode, NULL, NULL);
+	return fsnotify_handle_inode_event(group, child_mark, mask, data, data_type,
+					   NULL, NULL, 0);
 }
 
 static int send_to_group(__u32 mask, const void *data, int data_type,
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index f8529a3a2923..4ee3044eedd0 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -137,6 +137,7 @@ struct mem_cgroup;
  *		if @file_name is not NULL, this is the directory that
  *		@file_name is relative to.
  * @file_name:	optional file name associated with event
+ * @cookie:	inotify rename cookie
  *
  * free_group_priv - called when a group refcnt hits 0 to clean up the private union
  * freeing_mark - called when a mark is being destroyed for some reason.  The group
@@ -151,7 +152,7 @@ struct fsnotify_ops {
 			    struct fsnotify_iter_info *iter_info);
 	int (*handle_inode_event)(struct fsnotify_mark *mark, u32 mask,
 			    struct inode *inode, struct inode *dir,
-			    const struct qstr *file_name);
+			    const struct qstr *file_name, u32 cookie);
 	void (*free_group_priv)(struct fsnotify_group *group);
 	void (*freeing_mark)(struct fsnotify_mark *mark, struct fsnotify_group *group);
 	void (*free_event)(struct fsnotify_event *event);
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index bfcfcd61adb6..5b3f01da172b 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -154,7 +154,7 @@ static void audit_autoremove_mark_rule(struct audit_fsnotify_mark *audit_mark)
 /* Update mark data in audit rules based on fsnotify events. */
 static int audit_mark_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
 				   struct inode *inode, struct inode *dir,
-				   const struct qstr *dname)
+				   const struct qstr *dname, u32 cookie)
 {
 	struct audit_fsnotify_mark *audit_mark;
 
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index 83e1c07fc99e..6c91902f4f45 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -1037,7 +1037,7 @@ static void evict_chunk(struct audit_chunk *chunk)
 
 static int audit_tree_handle_event(struct fsnotify_mark *mark, u32 mask,
 				   struct inode *inode, struct inode *dir,
-				   const struct qstr *file_name)
+				   const struct qstr *file_name, u32 cookie)
 {
 	return 0;
 }
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 246e5ba704c0..2acf7ca49154 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -466,7 +466,7 @@ void audit_remove_watch_rule(struct audit_krule *krule)
 /* Update watch data in audit rules based on fsnotify events. */
 static int audit_watch_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
 				    struct inode *inode, struct inode *dir,
-				    const struct qstr *dname)
+				    const struct qstr *dname, u32 cookie)
 {
 	struct audit_parent *parent;
 
-- 
2.25.1

[PATCH 2/7] inotify: convert to handle_inode_event() interface

[Amir Goldstein]

Convert inotify to use the simple handle_inode_event() interface to
get rid of the code duplication between the generic helper
fsnotify_handle_event() and the inotify_handle_event() callback, which
also happen to be buggy code.

The bug will be fixed in the generic helper.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/inotify/inotify.h          |  9 +++---
 fs/notify/inotify/inotify_fsnotify.c | 47 ++++++----------------------
 fs/notify/inotify/inotify_user.c     |  7 +----
 3 files changed, 15 insertions(+), 48 deletions(-)

diff --git a/fs/notify/inotify/inotify.h b/fs/notify/inotify/inotify.h
index 4327d0e9c364..7fc3782b2fb8 100644
--- a/fs/notify/inotify/inotify.h
+++ b/fs/notify/inotify/inotify.h
@@ -24,11 +24,10 @@ static inline struct inotify_event_info *INOTIFY_E(struct fsnotify_event *fse)
 
 extern void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark,
 					   struct fsnotify_group *group);
-extern int inotify_handle_event(struct fsnotify_group *group, u32 mask,
-				const void *data, int data_type,
-				struct inode *dir,
-				const struct qstr *file_name, u32 cookie,
-				struct fsnotify_iter_info *iter_info);
+extern int inotify_handle_event(struct fsnotify_group *group,
+				struct fsnotify_mark *inode_mark, u32 mask,
+				struct inode *inode, struct inode *dir,
+				const struct qstr *name, u32 cookie);
 
 extern const struct fsnotify_ops inotify_fsnotify_ops;
 extern struct kmem_cache *inotify_inode_mark_cachep;
diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
index 9ddcbadc98e2..f348c1d3b358 100644
--- a/fs/notify/inotify/inotify_fsnotify.c
+++ b/fs/notify/inotify/inotify_fsnotify.c
@@ -55,10 +55,10 @@ static int inotify_merge(struct list_head *list,
 	return event_compare(last_event, event);
 }
 
-static int inotify_one_event(struct fsnotify_group *group, u32 mask,
-			     struct fsnotify_mark *inode_mark,
-			     const struct path *path,
-			     const struct qstr *file_name, u32 cookie)
+int inotify_handle_event(struct fsnotify_group *group,
+			 struct fsnotify_mark *inode_mark, u32 mask,
+			 struct inode *inode, struct inode *dir,
+			 const struct qstr *file_name, u32 cookie)
 {
 	struct inotify_inode_mark *i_mark;
 	struct inotify_event_info *event;
@@ -68,10 +68,6 @@ static int inotify_one_event(struct fsnotify_group *group, u32 mask,
 	int alloc_len = sizeof(struct inotify_event_info);
 	struct mem_cgroup *old_memcg;
 
-	if ((inode_mark->mask & FS_EXCL_UNLINK) &&
-	    path && d_unlinked(path->dentry))
-		return 0;
-
 	if (file_name) {
 		len = file_name->len;
 		alloc_len += len + 1;
@@ -131,35 +127,12 @@ static int inotify_one_event(struct fsnotify_group *group, u32 mask,
 	return 0;
 }
 
-int inotify_handle_event(struct fsnotify_group *group, u32 mask,
-			 const void *data, int data_type, struct inode *dir,
-			 const struct qstr *file_name, u32 cookie,
-			 struct fsnotify_iter_info *iter_info)
+static int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask,
+				      struct inode *inode, struct inode *dir,
+				      const struct qstr *name, u32 cookie)
 {
-	const struct path *path = fsnotify_data_path(data, data_type);
-	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
-	struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info);
-	int ret = 0;
-
-	if (WARN_ON(fsnotify_iter_vfsmount_mark(iter_info)))
-		return 0;
-
-	/*
-	 * Some events cannot be sent on both parent and child marks
-	 * (e.g. IN_CREATE).  Those events are always sent on inode_mark.
-	 * For events that are possible on both parent and child (e.g. IN_OPEN),
-	 * event is sent on inode_mark with name if the parent is watching and
-	 * is sent on child_mark without name if child is watching.
-	 * If both parent and child are watching, report the event with child's
-	 * name here and report another event without child's name below.
-	 */
-	if (inode_mark)
-		ret = inotify_one_event(group, mask, inode_mark, path,
-					file_name, cookie);
-	if (ret || !child_mark)
-		return ret;
-
-	return inotify_one_event(group, mask, child_mark, path, NULL, 0);
+	return inotify_handle_event(inode_mark->group, inode_mark, mask, inode,
+				    dir, name, cookie);
 }
 
 static void inotify_freeing_mark(struct fsnotify_mark *fsn_mark, struct fsnotify_group *group)
@@ -227,7 +200,7 @@ static void inotify_free_mark(struct fsnotify_mark *fsn_mark)
 }
 
 const struct fsnotify_ops inotify_fsnotify_ops = {
-	.handle_event = inotify_handle_event,
+	.handle_inode_event = inotify_handle_inode_event,
 	.free_group_priv = inotify_free_group_priv,
 	.free_event = inotify_free_event,
 	.freeing_mark = inotify_freeing_mark,
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 24d17028375e..b559f296d4cf 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -495,14 +495,9 @@ void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark,
 				    struct fsnotify_group *group)
 {
 	struct inotify_inode_mark *i_mark;
-	struct fsnotify_iter_info iter_info = { };
-
-	fsnotify_iter_set_report_type_mark(&iter_info, FSNOTIFY_OBJ_TYPE_INODE,
-					   fsn_mark);
 
 	/* Queue ignore event for the watch */
-	inotify_handle_event(group, FS_IN_IGNORED, NULL, FSNOTIFY_EVENT_NONE,
-			     NULL, NULL, 0, &iter_info);
+	inotify_handle_event(group, fsn_mark, FS_IN_IGNORED, NULL, NULL, NULL, 0);
 
 	i_mark = container_of(fsn_mark, struct inotify_inode_mark, fsn_mark);
 	/* remove this mark from the idr */
-- 
2.25.1

[PATCH 3/7] fsnotify: fix events reported to watching parent and child

[Amir Goldstein]

fsnotify_parent() used to send two separate events to backends when a
parent inode is watcing children and the child inode is also watching.

In an attempt to avoid duplicate events in fanotify, we unified the two
backend callbacks to a single callback and handled the reporting of the
two separate events for the relevant backends (inotify and dnotify).

The unified event callback with two inode marks (parent and child) is
called when both parent and child inode are watched and interested in
the event, but they could each be watched by a different group.

So before reporting the parent or child event flavor to backend we need
to check that the group is really interested in that event flavor.

The semantics of INODE and CHILD marks were hard to follow and made the
logic more complicated than it should have been.  Replace it with INODE
and PARENT marks semantics to hopefully make the logic more clear.

Fixes: eca4784cbb18 ("fsnotify: send event to parent and child with single callback")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c    |  7 ++-
 fs/notify/fsnotify.c             | 78 ++++++++++++++++++--------------
 include/linux/fsnotify_backend.h |  6 +--
 3 files changed, 51 insertions(+), 40 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 9167884a61ec..1192c9953620 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -268,12 +268,11 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 			continue;
 
 		/*
-		 * If the event is for a child and this mark is on a parent not
+		 * If the event is on a child and this mark is on a parent not
 		 * watching children, don't send it!
 		 */
-		if (event_mask & FS_EVENT_ON_CHILD &&
-		    type == FSNOTIFY_OBJ_TYPE_INODE &&
-		     !(mark->mask & FS_EVENT_ON_CHILD))
+		if (type == FSNOTIFY_OBJ_TYPE_PARENT &&
+		    !(mark->mask & FS_EVENT_ON_CHILD))
 			continue;
 
 		marks_mask |= mark->mask;
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index c5c68bcbaadf..0676ce4d3352 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -152,6 +152,13 @@ static bool fsnotify_event_needs_parent(struct inode *inode, struct mount *mnt,
 	if (mask & FS_ISDIR)
 		return false;
 
+	/*
+	 * All events that are possible on child can also may be reported with
+	 * parent/name info to inode/sb/mount.  Otherwise, a watching parent
+	 * could result in events reported with unexpected name info to sb/mount.
+	 */
+	BUILD_BUG_ON(FS_EVENTS_POSS_ON_CHILD & ~FS_EVENTS_POSS_TO_PARENT);
+
 	/* Did either inode/sb/mount subscribe for events with parent/name? */
 	marks_mask |= fsnotify_parent_needed_mask(inode->i_fsnotify_mask);
 	marks_mask |= fsnotify_parent_needed_mask(inode->i_sb->s_fsnotify_mask);
@@ -249,6 +256,10 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group,
 	    path && d_unlinked(path->dentry))
 		return 0;
 
+	/* Check interest of this mark in case event was sent with two marks */
+	if (!(mask & inode_mark->mask & ALL_FSNOTIFY_EVENTS))
+		return 0;
+
 	return ops->handle_inode_event(inode_mark, mask, inode, dir, name, cookie);
 }
 
@@ -258,38 +269,40 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask,
 				 u32 cookie, struct fsnotify_iter_info *iter_info)
 {
 	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
-	struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info);
+	struct fsnotify_mark *parent_mark = fsnotify_iter_parent_mark(iter_info);
 	int ret;
 
 	if (WARN_ON_ONCE(fsnotify_iter_sb_mark(iter_info)) ||
 	    WARN_ON_ONCE(fsnotify_iter_vfsmount_mark(iter_info)))
 		return 0;
 
-	/*
-	 * An event can be sent on child mark iterator instead of inode mark
-	 * iterator because of other groups that have interest of this inode
-	 * and have marks on both parent and child.  We can simplify this case.
-	 */
-	if (!inode_mark) {
-		inode_mark = child_mark;
-		child_mark = NULL;
+	if (parent_mark) {
+		/*
+		 * parent_mark indicates that the parent inode is watching children
+		 * and interested in this event, which is an event possible on child.
+		 * But is this mark watching children and interested in this event?
+		 */
+		if (parent_mark->mask & FS_EVENT_ON_CHILD) {
+			ret = fsnotify_handle_inode_event(group, parent_mark, mask,
+							  data, data_type, dir, name, 0);
+			if (ret)
+				return ret;
+		}
+		if (!inode_mark)
+			return 0;
+
+		/*
+		 * Some events can be sent on both parent dir and child marks
+		 * (e.g. FS_ATTRIB).  If both parent dir and child are watching,
+		 * report the event once to parent dir with name (if interested)
+		 * and once to child without name (if interested).
+		 */
 		dir = NULL;
 		name = NULL;
 	}
 
-	ret = fsnotify_handle_inode_event(group, inode_mark, mask, data, data_type,
-					  dir, name, cookie);
-	if (ret || !child_mark)
-		return ret;
-
-	/*
-	 * Some events can be sent on both parent dir and child marks
-	 * (e.g. FS_ATTRIB).  If both parent dir and child are watching,
-	 * report the event once to parent dir with name and once to child
-	 * without name.
-	 */
-	return fsnotify_handle_inode_event(group, child_mark, mask, data, data_type,
-					   NULL, NULL, 0);
+	return fsnotify_handle_inode_event(group, inode_mark, mask, data, data_type,
+					   dir, name, cookie);
 }
 
 static int send_to_group(__u32 mask, const void *data, int data_type,
@@ -447,7 +460,7 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 	struct fsnotify_iter_info iter_info = {};
 	struct super_block *sb;
 	struct mount *mnt = NULL;
-	struct inode *child = NULL;
+	struct inode *parent = NULL;
 	int ret = 0;
 	__u32 test_mask, marks_mask;
 
@@ -459,11 +472,10 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 		inode = dir;
 	} else if (mask & FS_EVENT_ON_CHILD) {
 		/*
-		 * Event on child - report on TYPE_INODE to dir if it is
-		 * watching children and on TYPE_CHILD to child.
+		 * Event on child - report on TYPE_PARENT to dir if it is
+		 * watching children and on TYPE_INODE to child.
 		 */
-		child = inode;
-		inode = dir;
+		parent = dir;
 	}
 	sb = inode->i_sb;
 
@@ -477,7 +489,7 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 	if (!sb->s_fsnotify_marks &&
 	    (!mnt || !mnt->mnt_fsnotify_marks) &&
 	    (!inode || !inode->i_fsnotify_marks) &&
-	    (!child || !child->i_fsnotify_marks))
+	    (!parent || !parent->i_fsnotify_marks))
 		return 0;
 
 	marks_mask = sb->s_fsnotify_mask;
@@ -485,8 +497,8 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 		marks_mask |= mnt->mnt_fsnotify_mask;
 	if (inode)
 		marks_mask |= inode->i_fsnotify_mask;
-	if (child)
-		marks_mask |= child->i_fsnotify_mask;
+	if (parent)
+		marks_mask |= parent->i_fsnotify_mask;
 
 
 	/*
@@ -509,9 +521,9 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 		iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
 			fsnotify_first_mark(&inode->i_fsnotify_marks);
 	}
-	if (child) {
-		iter_info.marks[FSNOTIFY_OBJ_TYPE_CHILD] =
-			fsnotify_first_mark(&child->i_fsnotify_marks);
+	if (parent) {
+		iter_info.marks[FSNOTIFY_OBJ_TYPE_PARENT] =
+			fsnotify_first_mark(&parent->i_fsnotify_marks);
 	}
 
 	/*
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 4ee3044eedd0..a2e42d3cd87c 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -278,7 +278,7 @@ static inline const struct path *fsnotify_data_path(const void *data,
 
 enum fsnotify_obj_type {
 	FSNOTIFY_OBJ_TYPE_INODE,
-	FSNOTIFY_OBJ_TYPE_CHILD,
+	FSNOTIFY_OBJ_TYPE_PARENT,
 	FSNOTIFY_OBJ_TYPE_VFSMOUNT,
 	FSNOTIFY_OBJ_TYPE_SB,
 	FSNOTIFY_OBJ_TYPE_COUNT,
@@ -286,7 +286,7 @@ enum fsnotify_obj_type {
 };
 
 #define FSNOTIFY_OBJ_TYPE_INODE_FL	(1U << FSNOTIFY_OBJ_TYPE_INODE)
-#define FSNOTIFY_OBJ_TYPE_CHILD_FL	(1U << FSNOTIFY_OBJ_TYPE_CHILD)
+#define FSNOTIFY_OBJ_TYPE_PARENT_FL	(1U << FSNOTIFY_OBJ_TYPE_PARENT)
 #define FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL	(1U << FSNOTIFY_OBJ_TYPE_VFSMOUNT)
 #define FSNOTIFY_OBJ_TYPE_SB_FL		(1U << FSNOTIFY_OBJ_TYPE_SB)
 #define FSNOTIFY_OBJ_ALL_TYPES_MASK	((1U << FSNOTIFY_OBJ_TYPE_COUNT) - 1)
@@ -331,7 +331,7 @@ static inline struct fsnotify_mark *fsnotify_iter_##name##_mark( \
 }
 
 FSNOTIFY_ITER_FUNCS(inode, INODE)
-FSNOTIFY_ITER_FUNCS(child, CHILD)
+FSNOTIFY_ITER_FUNCS(parent, PARENT)
 FSNOTIFY_ITER_FUNCS(vfsmount, VFSMOUNT)
 FSNOTIFY_ITER_FUNCS(sb, SB)
 
-- 
2.25.1

[PATCH 4/7] fsnotify: clarify object type argument

[Amir Goldstein]

In preparation for separating object type from iterator type, rename
some 'type' arguments in functions to 'obj_type' and remove the unused
interface to clear marks by object type mask.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify_user.c |  8 ++++----
 fs/notify/group.c                  |  2 +-
 fs/notify/mark.c                   | 24 ++++++++++++------------
 include/linux/fsnotify_backend.h   | 25 ++++++++++---------------
 4 files changed, 27 insertions(+), 32 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 3e01d8f2ab90..f9c74fa82038 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -819,7 +819,7 @@ static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
 
 static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
 						   fsnotify_connp_t *connp,
-						   unsigned int type,
+						   unsigned int obj_type,
 						   __kernel_fsid_t *fsid)
 {
 	struct fsnotify_mark *mark;
@@ -833,7 +833,7 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
 		return ERR_PTR(-ENOMEM);
 
 	fsnotify_init_mark(mark, group);
-	ret = fsnotify_add_mark_locked(mark, connp, type, 0, fsid);
+	ret = fsnotify_add_mark_locked(mark, connp, obj_type, 0, fsid);
 	if (ret) {
 		fsnotify_put_mark(mark);
 		return ERR_PTR(ret);
@@ -844,7 +844,7 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
 
 
 static int fanotify_add_mark(struct fsnotify_group *group,
-			     fsnotify_connp_t *connp, unsigned int type,
+			     fsnotify_connp_t *connp, unsigned int obj_type,
 			     __u32 mask, unsigned int flags,
 			     __kernel_fsid_t *fsid)
 {
@@ -854,7 +854,7 @@ static int fanotify_add_mark(struct fsnotify_group *group,
 	mutex_lock(&group->mark_mutex);
 	fsn_mark = fsnotify_find_mark(connp, group);
 	if (!fsn_mark) {
-		fsn_mark = fanotify_add_new_mark(group, connp, type, fsid);
+		fsn_mark = fanotify_add_new_mark(group, connp, obj_type, fsid);
 		if (IS_ERR(fsn_mark)) {
 			mutex_unlock(&group->mark_mutex);
 			return PTR_ERR(fsn_mark);
diff --git a/fs/notify/group.c b/fs/notify/group.c
index a4a4b1c64d32..8255b4c45802 100644
--- a/fs/notify/group.c
+++ b/fs/notify/group.c
@@ -58,7 +58,7 @@ void fsnotify_destroy_group(struct fsnotify_group *group)
 	fsnotify_group_stop_queueing(group);
 
 	/* Clear all marks for this group and queue them for destruction */
-	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_ALL_TYPES_MASK);
+	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_TYPE_ANY);
 
 	/*
 	 * Some marks can still be pinned when waiting for response from
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 8387937b9d01..7792f5486d61 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -474,7 +474,7 @@ int fsnotify_compare_groups(struct fsnotify_group *a, struct fsnotify_group *b)
 }
 
 static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
-					       unsigned int type,
+					       unsigned int obj_type,
 					       __kernel_fsid_t *fsid)
 {
 	struct inode *inode = NULL;
@@ -485,7 +485,7 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
 		return -ENOMEM;
 	spin_lock_init(&conn->lock);
 	INIT_HLIST_HEAD(&conn->list);
-	conn->type = type;
+	conn->type = obj_type;
 	conn->obj = connp;
 	/* Cache fsid of filesystem containing the object */
 	if (fsid) {
@@ -545,7 +545,7 @@ static struct fsnotify_mark_connector *fsnotify_grab_connector(
  * priority, highest number first, and then by the group's location in memory.
  */
 static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
-				  fsnotify_connp_t *connp, unsigned int type,
+				  fsnotify_connp_t *connp, unsigned int obj_type,
 				  int allow_dups, __kernel_fsid_t *fsid)
 {
 	struct fsnotify_mark *lmark, *last = NULL;
@@ -553,7 +553,7 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
 	int cmp;
 	int err = 0;
 
-	if (WARN_ON(!fsnotify_valid_obj_type(type)))
+	if (WARN_ON(!fsnotify_valid_obj_type(obj_type)))
 		return -EINVAL;
 
 	/* Backend is expected to check for zero fsid (e.g. tmpfs) */
@@ -565,7 +565,7 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
 	conn = fsnotify_grab_connector(connp);
 	if (!conn) {
 		spin_unlock(&mark->lock);
-		err = fsnotify_attach_connector_to_object(connp, type, fsid);
+		err = fsnotify_attach_connector_to_object(connp, obj_type, fsid);
 		if (err)
 			return err;
 		goto restart;
@@ -638,7 +638,7 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
  * event types should be delivered to which group.
  */
 int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
-			     fsnotify_connp_t *connp, unsigned int type,
+			     fsnotify_connp_t *connp, unsigned int obj_type,
 			     int allow_dups, __kernel_fsid_t *fsid)
 {
 	struct fsnotify_group *group = mark->group;
@@ -660,7 +660,7 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
 	fsnotify_get_mark(mark); /* for g_list */
 	spin_unlock(&mark->lock);
 
-	ret = fsnotify_add_mark_list(mark, connp, type, allow_dups, fsid);
+	ret = fsnotify_add_mark_list(mark, connp, obj_type, allow_dups, fsid);
 	if (ret)
 		goto err;
 
@@ -681,13 +681,13 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
 }
 
 int fsnotify_add_mark(struct fsnotify_mark *mark, fsnotify_connp_t *connp,
-		      unsigned int type, int allow_dups, __kernel_fsid_t *fsid)
+		      unsigned int obj_type, int allow_dups, __kernel_fsid_t *fsid)
 {
 	int ret;
 	struct fsnotify_group *group = mark->group;
 
 	mutex_lock(&group->mark_mutex);
-	ret = fsnotify_add_mark_locked(mark, connp, type, allow_dups, fsid);
+	ret = fsnotify_add_mark_locked(mark, connp, obj_type, allow_dups, fsid);
 	mutex_unlock(&group->mark_mutex);
 	return ret;
 }
@@ -722,14 +722,14 @@ EXPORT_SYMBOL_GPL(fsnotify_find_mark);
 
 /* Clear any marks in a group with given type mask */
 void fsnotify_clear_marks_by_group(struct fsnotify_group *group,
-				   unsigned int type_mask)
+				   unsigned int obj_type)
 {
 	struct fsnotify_mark *lmark, *mark;
 	LIST_HEAD(to_free);
 	struct list_head *head = &to_free;
 
 	/* Skip selection step if we want to clear all marks. */
-	if (type_mask == FSNOTIFY_OBJ_ALL_TYPES_MASK) {
+	if (obj_type == FSNOTIFY_OBJ_TYPE_ANY) {
 		head = &group->marks_list;
 		goto clear;
 	}
@@ -744,7 +744,7 @@ void fsnotify_clear_marks_by_group(struct fsnotify_group *group,
 	 */
 	mutex_lock_nested(&group->mark_mutex, SINGLE_DEPTH_NESTING);
 	list_for_each_entry_safe(mark, lmark, &group->marks_list, g_list) {
-		if ((1U << mark->connector->type) & type_mask)
+		if (mark->connector->type == obj_type)
 			list_move(&mark->g_list, &to_free);
 	}
 	mutex_unlock(&group->mark_mutex);
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index a2e42d3cd87c..72bc120a65bc 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -277,6 +277,7 @@ static inline const struct path *fsnotify_data_path(const void *data,
 }
 
 enum fsnotify_obj_type {
+	FSNOTIFY_OBJ_TYPE_ANY = -1,
 	FSNOTIFY_OBJ_TYPE_INODE,
 	FSNOTIFY_OBJ_TYPE_PARENT,
 	FSNOTIFY_OBJ_TYPE_VFSMOUNT,
@@ -285,15 +286,9 @@ enum fsnotify_obj_type {
 	FSNOTIFY_OBJ_TYPE_DETACHED = FSNOTIFY_OBJ_TYPE_COUNT
 };
 
-#define FSNOTIFY_OBJ_TYPE_INODE_FL	(1U << FSNOTIFY_OBJ_TYPE_INODE)
-#define FSNOTIFY_OBJ_TYPE_PARENT_FL	(1U << FSNOTIFY_OBJ_TYPE_PARENT)
-#define FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL	(1U << FSNOTIFY_OBJ_TYPE_VFSMOUNT)
-#define FSNOTIFY_OBJ_TYPE_SB_FL		(1U << FSNOTIFY_OBJ_TYPE_SB)
-#define FSNOTIFY_OBJ_ALL_TYPES_MASK	((1U << FSNOTIFY_OBJ_TYPE_COUNT) - 1)
-
-static inline bool fsnotify_valid_obj_type(unsigned int type)
+static inline bool fsnotify_valid_obj_type(unsigned int obj_type)
 {
-	return (type < FSNOTIFY_OBJ_TYPE_COUNT);
+	return (obj_type < FSNOTIFY_OBJ_TYPE_COUNT);
 }
 
 struct fsnotify_iter_info {
@@ -326,7 +321,7 @@ static inline void fsnotify_iter_set_report_type_mark(
 static inline struct fsnotify_mark *fsnotify_iter_##name##_mark( \
 		struct fsnotify_iter_info *iter_info) \
 { \
-	return (iter_info->report_mask & FSNOTIFY_OBJ_TYPE_##NAME##_FL) ? \
+	return (iter_info->report_mask & (1U << FSNOTIFY_OBJ_TYPE_##NAME)) ? \
 		iter_info->marks[FSNOTIFY_OBJ_TYPE_##NAME] : NULL; \
 }
 
@@ -520,11 +515,11 @@ extern int fsnotify_get_conn_fsid(const struct fsnotify_mark_connector *conn,
 				  __kernel_fsid_t *fsid);
 /* attach the mark to the object */
 extern int fsnotify_add_mark(struct fsnotify_mark *mark,
-			     fsnotify_connp_t *connp, unsigned int type,
+			     fsnotify_connp_t *connp, unsigned int obj_type,
 			     int allow_dups, __kernel_fsid_t *fsid);
 extern int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
 				    fsnotify_connp_t *connp,
-				    unsigned int type, int allow_dups,
+				    unsigned int obj_type, int allow_dups,
 				    __kernel_fsid_t *fsid);
 
 /* attach the mark to the inode */
@@ -554,21 +549,21 @@ extern void fsnotify_free_mark(struct fsnotify_mark *mark);
 /* Wait until all marks queued for destruction are destroyed */
 extern void fsnotify_wait_marks_destroyed(void);
 /* run all the marks in a group, and clear all of the marks attached to given object type */
-extern void fsnotify_clear_marks_by_group(struct fsnotify_group *group, unsigned int type);
+extern void fsnotify_clear_marks_by_group(struct fsnotify_group *group, unsigned int obj_type);
 /* run all the marks in a group, and clear all of the vfsmount marks */
 static inline void fsnotify_clear_vfsmount_marks_by_group(struct fsnotify_group *group)
 {
-	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL);
+	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_TYPE_VFSMOUNT);
 }
 /* run all the marks in a group, and clear all of the inode marks */
 static inline void fsnotify_clear_inode_marks_by_group(struct fsnotify_group *group)
 {
-	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_TYPE_INODE_FL);
+	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_TYPE_INODE);
 }
 /* run all the marks in a group, and clear all of the sn marks */
 static inline void fsnotify_clear_sb_marks_by_group(struct fsnotify_group *group)
 {
-	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_TYPE_SB_FL);
+	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_TYPE_SB);
 }
 extern void fsnotify_get_mark(struct fsnotify_mark *mark);
 extern void fsnotify_put_mark(struct fsnotify_mark *mark);
-- 
2.25.1

[PATCH 5/7] fsnotify: separate mark iterator type from object type enum

[Amir Goldstein]

They are two different types that use the same enum, so this confusing.

Use the object type to indicate the type of object mark is attached to
and the iter type to indicate the type of watch.

A group can have two different watches of the same object type (parent
and child watches) that match the same event.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c    |  6 ++---
 fs/notify/fsnotify.c             | 26 +++++++++++++-------
 fs/notify/mark.c                 |  4 ++--
 include/linux/fsnotify_backend.h | 41 ++++++++++++++++++++++----------
 4 files changed, 50 insertions(+), 27 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 1192c9953620..8655a1e7c6a6 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -252,7 +252,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 			return 0;
 	}
 
-	fsnotify_foreach_obj_type(type) {
+	fsnotify_foreach_iter_type(type) {
 		if (!fsnotify_iter_should_report_type(iter_info, type))
 			continue;
 		mark = iter_info->marks[type];
@@ -271,7 +271,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
 		 * If the event is on a child and this mark is on a parent not
 		 * watching children, don't send it!
 		 */
-		if (type == FSNOTIFY_OBJ_TYPE_PARENT &&
+		if (type == FSNOTIFY_ITER_TYPE_PARENT &&
 		    !(mark->mask & FS_EVENT_ON_CHILD))
 			continue;
 
@@ -622,7 +622,7 @@ static __kernel_fsid_t fanotify_get_fsid(struct fsnotify_iter_info *iter_info)
 	int type;
 	__kernel_fsid_t fsid = {};
 
-	fsnotify_foreach_obj_type(type) {
+	fsnotify_foreach_iter_type(type) {
 		struct fsnotify_mark_connector *conn;
 
 		if (!fsnotify_iter_should_report_type(iter_info, type))
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 0676ce4d3352..bae3f306ed79 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -321,7 +321,7 @@ static int send_to_group(__u32 mask, const void *data, int data_type,
 
 	/* clear ignored on inode modification */
 	if (mask & FS_MODIFY) {
-		fsnotify_foreach_obj_type(type) {
+		fsnotify_foreach_iter_type(type) {
 			if (!fsnotify_iter_should_report_type(iter_info, type))
 				continue;
 			mark = iter_info->marks[type];
@@ -331,7 +331,7 @@ static int send_to_group(__u32 mask, const void *data, int data_type,
 		}
 	}
 
-	fsnotify_foreach_obj_type(type) {
+	fsnotify_foreach_iter_type(type) {
 		if (!fsnotify_iter_should_report_type(iter_info, type))
 			continue;
 		mark = iter_info->marks[type];
@@ -396,7 +396,7 @@ static unsigned int fsnotify_iter_select_report_types(
 	int type;
 
 	/* Choose max prio group among groups of all queue heads */
-	fsnotify_foreach_obj_type(type) {
+	fsnotify_foreach_iter_type(type) {
 		mark = iter_info->marks[type];
 		if (mark &&
 		    fsnotify_compare_groups(max_prio_group, mark->group) > 0)
@@ -408,7 +408,7 @@ static unsigned int fsnotify_iter_select_report_types(
 
 	/* Set the report mask for marks from same group as max prio group */
 	iter_info->report_mask = 0;
-	fsnotify_foreach_obj_type(type) {
+	fsnotify_foreach_iter_type(type) {
 		mark = iter_info->marks[type];
 		if (mark &&
 		    fsnotify_compare_groups(max_prio_group, mark->group) == 0)
@@ -426,7 +426,7 @@ static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
 {
 	int type;
 
-	fsnotify_foreach_obj_type(type) {
+	fsnotify_foreach_iter_type(type) {
 		if (fsnotify_iter_should_report_type(iter_info, type))
 			iter_info->marks[type] =
 				fsnotify_next_mark(iter_info->marks[type]);
@@ -511,18 +511,26 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 
 	iter_info.srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
 
-	iter_info.marks[FSNOTIFY_OBJ_TYPE_SB] =
+	/*
+	 * Just in case some backend still assumes that iterator type means
+	 * object type. We can relax this in the future.
+	 */
+	BUILD_BUG_ON(FSNOTIFY_OBJ_TYPE_INODE != (int)FSNOTIFY_ITER_TYPE_INODE);
+	BUILD_BUG_ON(FSNOTIFY_OBJ_TYPE_VFSMOUNT != (int)FSNOTIFY_ITER_TYPE_VFSMOUNT);
+	BUILD_BUG_ON(FSNOTIFY_OBJ_TYPE_SB != (int)FSNOTIFY_ITER_TYPE_SB);
+
+	iter_info.marks[FSNOTIFY_ITER_TYPE_SB] =
 		fsnotify_first_mark(&sb->s_fsnotify_marks);
 	if (mnt) {
-		iter_info.marks[FSNOTIFY_OBJ_TYPE_VFSMOUNT] =
+		iter_info.marks[FSNOTIFY_ITER_TYPE_VFSMOUNT] =
 			fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
 	}
 	if (inode) {
-		iter_info.marks[FSNOTIFY_OBJ_TYPE_INODE] =
+		iter_info.marks[FSNOTIFY_ITER_TYPE_INODE] =
 			fsnotify_first_mark(&inode->i_fsnotify_marks);
 	}
 	if (parent) {
-		iter_info.marks[FSNOTIFY_OBJ_TYPE_PARENT] =
+		iter_info.marks[FSNOTIFY_ITER_TYPE_PARENT] =
 			fsnotify_first_mark(&parent->i_fsnotify_marks);
 	}
 
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 7792f5486d61..ffa682cb747b 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -329,7 +329,7 @@ bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
 {
 	int type;
 
-	fsnotify_foreach_obj_type(type) {
+	fsnotify_foreach_iter_type(type) {
 		/* This can fail if mark is being removed */
 		if (!fsnotify_get_mark_safe(iter_info->marks[type])) {
 			__release(&fsnotify_mark_srcu);
@@ -358,7 +358,7 @@ void fsnotify_finish_user_wait(struct fsnotify_iter_info *iter_info)
 	int type;
 
 	iter_info->srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
-	fsnotify_foreach_obj_type(type)
+	fsnotify_foreach_iter_type(type)
 		fsnotify_put_mark_wake(iter_info->marks[type]);
 }
 
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 72bc120a65bc..9d03f031a41b 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -276,10 +276,25 @@ static inline const struct path *fsnotify_data_path(const void *data,
 	}
 }
 
+/*
+ * Index to merged marks iterator array that correlates to a type of watch.
+ * The type of watched object can be deduced from the iterator type, but not
+ * the other way around, because an event can match different watched objects
+ * of the same object type.
+ * For example, both parent and child are watching an object of type inode.
+ */
+enum fsnotify_iter_type {
+	FSNOTIFY_ITER_TYPE_INODE,
+	FSNOTIFY_ITER_TYPE_VFSMOUNT,
+	FSNOTIFY_ITER_TYPE_SB,
+	FSNOTIFY_ITER_TYPE_PARENT,
+	FSNOTIFY_ITER_TYPE_COUNT
+};
+
+/* The type of object that a mark is attached to */
 enum fsnotify_obj_type {
 	FSNOTIFY_OBJ_TYPE_ANY = -1,
 	FSNOTIFY_OBJ_TYPE_INODE,
-	FSNOTIFY_OBJ_TYPE_PARENT,
 	FSNOTIFY_OBJ_TYPE_VFSMOUNT,
 	FSNOTIFY_OBJ_TYPE_SB,
 	FSNOTIFY_OBJ_TYPE_COUNT,
@@ -292,37 +307,37 @@ static inline bool fsnotify_valid_obj_type(unsigned int obj_type)
 }
 
 struct fsnotify_iter_info {
-	struct fsnotify_mark *marks[FSNOTIFY_OBJ_TYPE_COUNT];
+	struct fsnotify_mark *marks[FSNOTIFY_ITER_TYPE_COUNT];
 	unsigned int report_mask;
 	int srcu_idx;
 };
 
 static inline bool fsnotify_iter_should_report_type(
-		struct fsnotify_iter_info *iter_info, int type)
+		struct fsnotify_iter_info *iter_info, int iter_type)
 {
-	return (iter_info->report_mask & (1U << type));
+	return (iter_info->report_mask & (1U << iter_type));
 }
 
 static inline void fsnotify_iter_set_report_type(
-		struct fsnotify_iter_info *iter_info, int type)
+		struct fsnotify_iter_info *iter_info, int iter_type)
 {
-	iter_info->report_mask |= (1U << type);
+	iter_info->report_mask |= (1U << iter_type);
 }
 
 static inline void fsnotify_iter_set_report_type_mark(
-		struct fsnotify_iter_info *iter_info, int type,
+		struct fsnotify_iter_info *iter_info, int iter_type,
 		struct fsnotify_mark *mark)
 {
-	iter_info->marks[type] = mark;
-	iter_info->report_mask |= (1U << type);
+	iter_info->marks[iter_type] = mark;
+	iter_info->report_mask |= (1U << iter_type);
 }
 
 #define FSNOTIFY_ITER_FUNCS(name, NAME) \
 static inline struct fsnotify_mark *fsnotify_iter_##name##_mark( \
 		struct fsnotify_iter_info *iter_info) \
 { \
-	return (iter_info->report_mask & (1U << FSNOTIFY_OBJ_TYPE_##NAME)) ? \
-		iter_info->marks[FSNOTIFY_OBJ_TYPE_##NAME] : NULL; \
+	return (iter_info->report_mask & (1U << FSNOTIFY_ITER_TYPE_##NAME)) ? \
+		iter_info->marks[FSNOTIFY_ITER_TYPE_##NAME] : NULL; \
 }
 
 FSNOTIFY_ITER_FUNCS(inode, INODE)
@@ -330,8 +345,8 @@ FSNOTIFY_ITER_FUNCS(parent, PARENT)
 FSNOTIFY_ITER_FUNCS(vfsmount, VFSMOUNT)
 FSNOTIFY_ITER_FUNCS(sb, SB)
 
-#define fsnotify_foreach_obj_type(type) \
-	for (type = 0; type < FSNOTIFY_OBJ_TYPE_COUNT; type++)
+#define fsnotify_foreach_iter_type(type) \
+	for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++)
 
 /*
  * fsnotify_connp_t is what we embed in objects which connector can be attached
-- 
2.25.1

[PATCH 6/7] fsnotify: optimize FS_MODIFY events with no ignored masks

[Amir Goldstein]

fsnotify() treats FS_MODIFY events specially - it does not skip them
even if the FS_MODIFY event does not apear in the object's fsnotify
mask.  This is because send_to_group() checks if FS_MODIFY needs to
clear ignored mask of marks.

The common case is that an object does not have any mark with ignored
mask and in particular, that it does not have a mark with ignored mask
and without the FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY flag.

Set FS_MODIFY in object's fsnotify mask during fsnotify_recalc_mask()
if object has a mark with an ignored mask and without the
FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY flag and remove the special
treatment of FS_MODIFY in fsnotify(), so that FS_MODIFY events could
be optimized in the common case.

Call fsnotify_recalc_mask() from fanotify after adding or removing an
ignored mask from a mark without FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY
or when adding the FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY flag to a mark
with ignored mask (the flag cannot be removed by fanotify uapi).

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify_user.c | 36 ++++++++++++++++++++----------
 fs/notify/fsnotify.c               |  8 ++++---
 fs/notify/mark.c                   |  2 +-
 include/linux/fsnotify_backend.h   | 15 +++++++++++++
 4 files changed, 45 insertions(+), 16 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index f9c74fa82038..80c36da037bb 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -720,17 +720,18 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
 					    __u32 mask, unsigned int flags,
 					    __u32 umask, int *destroy)
 {
-	__u32 oldmask = 0;
+	__u32 oldmask, newmask;
 
 	/* umask bits cannot be removed by user */
 	mask &= ~umask;
 	spin_lock(&fsn_mark->lock);
+	oldmask = fsnotify_calc_mask(fsn_mark);
 	if (!(flags & FAN_MARK_IGNORED_MASK)) {
-		oldmask = fsn_mark->mask;
 		fsn_mark->mask &= ~mask;
 	} else {
 		fsn_mark->ignored_mask &= ~mask;
 	}
+	newmask = fsnotify_calc_mask(fsn_mark);
 	/*
 	 * We need to keep the mark around even if remaining mask cannot
 	 * result in any events (e.g. mask == FAN_ONDIR) to support incremenal
@@ -740,7 +741,7 @@ static __u32 fanotify_mark_remove_from_mask(struct fsnotify_mark *fsn_mark,
 	*destroy = !((fsn_mark->mask | fsn_mark->ignored_mask) & ~umask);
 	spin_unlock(&fsn_mark->lock);
 
-	return mask & oldmask;
+	return oldmask & ~newmask;
 }
 
 static int fanotify_remove_mark(struct fsnotify_group *group,
@@ -798,23 +799,34 @@ static int fanotify_remove_inode_mark(struct fsnotify_group *group,
 }
 
 static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
-				       __u32 mask,
-				       unsigned int flags)
+				       __u32 mask, unsigned int flags,
+				       __u32 *removed)
 {
-	__u32 oldmask = -1;
+	__u32 oldmask, newmask;
 
 	spin_lock(&fsn_mark->lock);
+	oldmask = fsnotify_calc_mask(fsn_mark);
 	if (!(flags & FAN_MARK_IGNORED_MASK)) {
-		oldmask = fsn_mark->mask;
 		fsn_mark->mask |= mask;
 	} else {
 		fsn_mark->ignored_mask |= mask;
-		if (flags & FAN_MARK_IGNORED_SURV_MODIFY)
+		/*
+		 * Setting FAN_MARK_IGNORED_SURV_MODIFY for the first time
+		 * can lead to the removal of the FS_MODIFY bit in calculated
+		 * mask if it was set because of an ignored mask that from now
+		 * on is going to survive FS_MODIFY.
+		 */
+		if ((flags & FAN_MARK_IGNORED_SURV_MODIFY) &&
+		    !(fsn_mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY)) {
 			fsn_mark->flags |= FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY;
+			if (!(fsn_mark->mask & FS_MODIFY))
+				*removed = FS_MODIFY;
+		}
 	}
+	newmask = fsnotify_calc_mask(fsn_mark);
 	spin_unlock(&fsn_mark->lock);
 
-	return mask & ~oldmask;
+	return newmask & ~oldmask;
 }
 
 static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
@@ -849,7 +861,7 @@ static int fanotify_add_mark(struct fsnotify_group *group,
 			     __kernel_fsid_t *fsid)
 {
 	struct fsnotify_mark *fsn_mark;
-	__u32 added;
+	__u32 added, removed = 0;
 
 	mutex_lock(&group->mark_mutex);
 	fsn_mark = fsnotify_find_mark(connp, group);
@@ -860,8 +872,8 @@ static int fanotify_add_mark(struct fsnotify_group *group,
 			return PTR_ERR(fsn_mark);
 		}
 	}
-	added = fanotify_mark_add_to_mask(fsn_mark, mask, flags);
-	if (added & ~fsnotify_conn_mask(fsn_mark->connector))
+	added = fanotify_mark_add_to_mask(fsn_mark, mask, flags, &removed);
+	if (removed || (added & ~fsnotify_conn_mask(fsn_mark->connector)))
 		fsnotify_recalc_mask(fsn_mark->connector);
 	mutex_unlock(&group->mark_mutex);
 
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index bae3f306ed79..9a26207d1b5d 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -502,11 +502,13 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 
 
 	/*
-	 * if this is a modify event we may need to clear the ignored masks
-	 * otherwise return if none of the marks care about this type of event.
+	 * If this is a modify event we may need to clear some ignored masks.
+	 * In that case, the object with ignored masks will have the FS_MODIFY
+	 * event in its mask.
+	 * Otherwise, return if none of the marks care about this type of event.
 	 */
 	test_mask = (mask & ALL_FSNOTIFY_EVENTS);
-	if (!(mask & FS_MODIFY) && !(test_mask & marks_mask))
+	if (!(test_mask & marks_mask))
 		return 0;
 
 	iter_info.srcu_idx = srcu_read_lock(&fsnotify_mark_srcu);
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index ffa682cb747b..662963fb510f 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -127,7 +127,7 @@ static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 		return;
 	hlist_for_each_entry(mark, &conn->list, obj_list) {
 		if (mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)
-			new_mask |= mark->mask;
+			new_mask |= fsnotify_calc_mask(mark);
 	}
 	*fsnotify_conn_mask_p(conn) = new_mask;
 }
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 9d03f031a41b..046fcfb88492 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -516,6 +516,21 @@ extern void fsnotify_remove_queued_event(struct fsnotify_group *group,
 
 /* functions used to manipulate the marks attached to inodes */
 
+/* Get mask for calculating object interest taking ignored mask into account */
+static inline __u32 fsnotify_calc_mask(struct fsnotify_mark *mark)
+{
+	__u32 mask = mark->mask;
+
+	if (!mark->ignored_mask)
+		return mask;
+
+	/* Interest in FS_MODIFY may be needed for clearing ignored mask */
+	if (!(mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY))
+		mask |= FS_MODIFY;
+
+	return mask;
+}
+
 /* Get mask of events for a list of marks */
 extern __u32 fsnotify_conn_mask(struct fsnotify_mark_connector *conn);
 /* Calculate mask of events for a list of marks */
-- 
2.25.1

[PATCH 7/7] fsnotify: optimize merging of marks with no ignored masks

[Amir Goldstein]

fsnotify() tries to merge marks on all object types (sb, mount, inode)
even if the object's mask shows no interest in the specific event type.

This is done for the case that the object has marks with the event type
in their ignored mask, but the common case is that an object does not
have any marks with ignored mask.

Set a bit in object's fsnotify mask during fsnotify_recalc_mask() to
indicate the existence of any marks with ignored masks.

Instead of merging marks of all object types, only merge marks from
objects that either showed interest in the specific event type or have
any marks with ignored mask.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c    |  5 +++++
 fs/notify/fsnotify.c             | 18 ++++++++++++------
 include/linux/fsnotify_backend.h | 12 ++++++++++--
 3 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 8655a1e7c6a6..4441de2fba11 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -677,6 +677,11 @@ static int fanotify_handle_event(struct fsnotify_group *group, u32 mask,
 	BUILD_BUG_ON(FAN_OPEN_EXEC_PERM != FS_OPEN_EXEC_PERM);
 
 	BUILD_BUG_ON(HWEIGHT32(ALL_FANOTIFY_EVENT_BITS) != 19);
+	/*
+	 * FS_HAS_IGNORED_MASK bit is reserved for internal use so should
+	 * not be exposed to fanotify uapi.
+	 */
+	BUILD_BUG_ON(ALL_FANOTIFY_EVENT_BITS & FS_HAS_IGNORED_MASK);
 
 	mask = fanotify_group_event_mask(group, iter_info, mask, data,
 					 data_type, dir);
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 9a26207d1b5d..6b3a828db6aa 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -500,7 +500,6 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 	if (parent)
 		marks_mask |= parent->i_fsnotify_mask;
 
-
 	/*
 	 * If this is a modify event we may need to clear some ignored masks.
 	 * In that case, the object with ignored masks will have the FS_MODIFY
@@ -521,17 +520,24 @@ int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
 	BUILD_BUG_ON(FSNOTIFY_OBJ_TYPE_VFSMOUNT != (int)FSNOTIFY_ITER_TYPE_VFSMOUNT);
 	BUILD_BUG_ON(FSNOTIFY_OBJ_TYPE_SB != (int)FSNOTIFY_ITER_TYPE_SB);
 
-	iter_info.marks[FSNOTIFY_ITER_TYPE_SB] =
-		fsnotify_first_mark(&sb->s_fsnotify_marks);
-	if (mnt) {
+	/*
+	 * Consider only marks that care about this type of event and marks with
+	 * an ignored mask.
+	 */
+	test_mask |= FS_HAS_IGNORED_MASK;
+	if (test_mask && sb->s_fsnotify_mask) {
+		iter_info.marks[FSNOTIFY_ITER_TYPE_SB] =
+			fsnotify_first_mark(&sb->s_fsnotify_marks);
+	}
+	if (mnt && (test_mask & mnt->mnt_fsnotify_mask)) {
 		iter_info.marks[FSNOTIFY_ITER_TYPE_VFSMOUNT] =
 			fsnotify_first_mark(&mnt->mnt_fsnotify_marks);
 	}
-	if (inode) {
+	if (inode && (test_mask & inode->i_fsnotify_mask)) {
 		iter_info.marks[FSNOTIFY_ITER_TYPE_INODE] =
 			fsnotify_first_mark(&inode->i_fsnotify_marks);
 	}
-	if (parent) {
+	if (parent && (test_mask & parent->i_fsnotify_mask)) {
 		iter_info.marks[FSNOTIFY_ITER_TYPE_PARENT] =
 			fsnotify_first_mark(&parent->i_fsnotify_marks);
 	}
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 046fcfb88492..0615ca2fddf9 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -61,6 +61,14 @@
 #define FS_ISDIR		0x40000000	/* event occurred against dir */
 #define FS_IN_ONESHOT		0x80000000	/* only send event once */
 
+/*
+ * Overload FS_IN_ONESHOT is set only on inotify marks, which never set the
+ * ignored mask and is not relevant in the object's cumulative mask.
+ * Overload the flag to indicate the existence of marks on the object that
+ * have an ignored mask.
+ */
+#define FS_HAS_IGNORED_MASK	FS_IN_ONESHOT
+
 #define FS_MOVE			(FS_MOVED_FROM | FS_MOVED_TO)
 
 /*
@@ -522,13 +530,13 @@ static inline __u32 fsnotify_calc_mask(struct fsnotify_mark *mark)
 	__u32 mask = mark->mask;
 
 	if (!mark->ignored_mask)
-		return mask;
+		return mask & ~FS_HAS_IGNORED_MASK;
 
 	/* Interest in FS_MODIFY may be needed for clearing ignored mask */
 	if (!(mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY))
 		mask |= FS_MODIFY;
 
-	return mask;
+	return mask | FS_HAS_IGNORED_MASK;
 }
 
 /* Get mask of events for a list of marks */
-- 
2.25.1

Re: [PATCH 1/7] fsnotify: generalize handle_inode_event()

[Jan Kara]

On Wed 02-12-20 14:07:07, Amir Goldstein wrote:
> The handle_inode_event() interface was added as (quoting comment):
> "a simple variant of handle_event() for groups that only have inode
> marks and don't have ignore mask".
> 
> In other words, all backends except fanotify.  The inotify backend
> also falls under this category, but because it required extra arguments
> it was left out of the initial pass of backends conversion to the
> simple interface.
> 
> This results in code duplication between the generic helper
> fsnotify_handle_event() and the inotify_handle_event() callback
> which also happen to be buggy code.
> 
> Generalize the handle_inode_event() arguments and add the check for
> FS_EXCL_UNLINK flag to the generic helper, so inotify backend could
> be converted to use the simple interface.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

The patch looks good to me. Just one curious question below.

> +static int fsnotify_handle_inode_event(struct fsnotify_group *group,
> +				       struct fsnotify_mark *inode_mark,
> +				       u32 mask, const void *data, int data_type,
> +				       struct inode *dir, const struct qstr *name,
> +				       u32 cookie)
> +{
> +	const struct path *path = fsnotify_data_path(data, data_type);
> +	struct inode *inode = fsnotify_data_inode(data, data_type);
> +	const struct fsnotify_ops *ops = group->ops;
> +
> +	if (WARN_ON_ONCE(!ops->handle_inode_event))
> +		return 0;
> +
> +	if ((inode_mark->mask & FS_EXCL_UNLINK) &&
> +	    path && d_unlinked(path->dentry))
> +		return 0;

When I was looking at this condition I was wondering why do we check
d_unlinked() and not inode->i_nlink? When is there a difference?

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

Re: [PATCH 2/7] inotify: convert to handle_inode_event() interface

[Jan Kara]

On Wed 02-12-20 14:07:08, Amir Goldstein wrote:
> Convert inotify to use the simple handle_inode_event() interface to
> get rid of the code duplication between the generic helper
> fsnotify_handle_event() and the inotify_handle_event() callback, which
> also happen to be buggy code.
> 
> The bug will be fixed in the generic helper.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/notify/inotify/inotify.h          |  9 +++---
>  fs/notify/inotify/inotify_fsnotify.c | 47 ++++++----------------------
>  fs/notify/inotify/inotify_user.c     |  7 +----
>  3 files changed, 15 insertions(+), 48 deletions(-)
> 
> diff --git a/fs/notify/inotify/inotify.h b/fs/notify/inotify/inotify.h
> index 4327d0e9c364..7fc3782b2fb8 100644
> --- a/fs/notify/inotify/inotify.h
> +++ b/fs/notify/inotify/inotify.h
> @@ -24,11 +24,10 @@ static inline struct inotify_event_info *INOTIFY_E(struct fsnotify_event *fse)
>  
>  extern void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark,
>  					   struct fsnotify_group *group);
> -extern int inotify_handle_event(struct fsnotify_group *group, u32 mask,
> -				const void *data, int data_type,
> -				struct inode *dir,
> -				const struct qstr *file_name, u32 cookie,
> -				struct fsnotify_iter_info *iter_info);
> +extern int inotify_handle_event(struct fsnotify_group *group,
> +				struct fsnotify_mark *inode_mark, u32 mask,
> +				struct inode *inode, struct inode *dir,
> +				const struct qstr *name, u32 cookie);
>  
>  extern const struct fsnotify_ops inotify_fsnotify_ops;
>  extern struct kmem_cache *inotify_inode_mark_cachep;
> diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
> index 9ddcbadc98e2..f348c1d3b358 100644
> --- a/fs/notify/inotify/inotify_fsnotify.c
> +++ b/fs/notify/inotify/inotify_fsnotify.c
> @@ -55,10 +55,10 @@ static int inotify_merge(struct list_head *list,
>  	return event_compare(last_event, event);
>  }
>  
> -static int inotify_one_event(struct fsnotify_group *group, u32 mask,
> -			     struct fsnotify_mark *inode_mark,
> -			     const struct path *path,
> -			     const struct qstr *file_name, u32 cookie)
> +int inotify_handle_event(struct fsnotify_group *group,
> +			 struct fsnotify_mark *inode_mark, u32 mask,
> +			 struct inode *inode, struct inode *dir,
> +			 const struct qstr *file_name, u32 cookie)
>  {
>  	struct inotify_inode_mark *i_mark;
>  	struct inotify_event_info *event;
> @@ -68,10 +68,6 @@ static int inotify_one_event(struct fsnotify_group *group, u32 mask,
>  	int alloc_len = sizeof(struct inotify_event_info);
>  	struct mem_cgroup *old_memcg;
>  
> -	if ((inode_mark->mask & FS_EXCL_UNLINK) &&
> -	    path && d_unlinked(path->dentry))
> -		return 0;
> -
>  	if (file_name) {
>  		len = file_name->len;
>  		alloc_len += len + 1;
> @@ -131,35 +127,12 @@ static int inotify_one_event(struct fsnotify_group *group, u32 mask,
>  	return 0;
>  }
>  
> -int inotify_handle_event(struct fsnotify_group *group, u32 mask,
> -			 const void *data, int data_type, struct inode *dir,
> -			 const struct qstr *file_name, u32 cookie,
> -			 struct fsnotify_iter_info *iter_info)
> +static int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask,
> +				      struct inode *inode, struct inode *dir,
> +				      const struct qstr *name, u32 cookie)
>  {

Looking at this I'd just fold inotify_handle_event() into
inotify_handle_inode_event() as the only difference is the 'group' argument
and that can be always fetched from inode_mark->group...

								Honza

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

Re: [PATCH 7/7] fsnotify: optimize merging of marks with no ignored masks

[Jan Kara]

On Wed 02-12-20 14:07:13, Amir Goldstein wrote:
> fsnotify() tries to merge marks on all object types (sb, mount, inode)
> even if the object's mask shows no interest in the specific event type.
> 
> This is done for the case that the object has marks with the event type
> in their ignored mask, but the common case is that an object does not
> have any marks with ignored mask.
> 
> Set a bit in object's fsnotify mask during fsnotify_recalc_mask() to
> indicate the existence of any marks with ignored masks.
> 
> Instead of merging marks of all object types, only merge marks from
> objects that either showed interest in the specific event type or have
> any marks with ignored mask.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

The idea looks sound to me and the patch fine (besides one bug noted
below). I'd be interested if an actual performance impact of this and
previous change could be noticed. Maybe some careful microbenchmark could
reveal it...

> +	/*
> +	 * Consider only marks that care about this type of event and marks with
> +	 * an ignored mask.
> +	 */
> +	test_mask |= FS_HAS_IGNORED_MASK;
> +	if (test_mask && sb->s_fsnotify_mask) {
		      ^^ Just '&' here.


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

Re: [PATCH 1/7] fsnotify: generalize handle_inode_event()

[Amir Goldstein]

On Thu, Dec 3, 2020 at 11:51 AM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 02-12-20 14:07:07, Amir Goldstein wrote:
> > The handle_inode_event() interface was added as (quoting comment):
> > "a simple variant of handle_event() for groups that only have inode
> > marks and don't have ignore mask".
> >
> > In other words, all backends except fanotify.  The inotify backend
> > also falls under this category, but because it required extra arguments
> > it was left out of the initial pass of backends conversion to the
> > simple interface.
> >
> > This results in code duplication between the generic helper
> > fsnotify_handle_event() and the inotify_handle_event() callback
> > which also happen to be buggy code.
> >
> > Generalize the handle_inode_event() arguments and add the check for
> > FS_EXCL_UNLINK flag to the generic helper, so inotify backend could
> > be converted to use the simple interface.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> The patch looks good to me. Just one curious question below.
>
> > +static int fsnotify_handle_inode_event(struct fsnotify_group *group,
> > +                                    struct fsnotify_mark *inode_mark,
> > +                                    u32 mask, const void *data, int data_type,
> > +                                    struct inode *dir, const struct qstr *name,
> > +                                    u32 cookie)
> > +{
> > +     const struct path *path = fsnotify_data_path(data, data_type);
> > +     struct inode *inode = fsnotify_data_inode(data, data_type);
> > +     const struct fsnotify_ops *ops = group->ops;
> > +
> > +     if (WARN_ON_ONCE(!ops->handle_inode_event))
> > +             return 0;
> > +
> > +     if ((inode_mark->mask & FS_EXCL_UNLINK) &&
> > +         path && d_unlinked(path->dentry))
> > +             return 0;
>
> When I was looking at this condition I was wondering why do we check
> d_unlinked() and not inode->i_nlink? When is there a difference?

When a hardlink has been unlinked.
inotify gets the filename and it doesn't want to get events with unlinked
names (although another name could still be linked) I suppose...

Thanks,
Amir.

Re: [PATCH 2/7] inotify: convert to handle_inode_event() interface

[Amir Goldstein]

On Thu, Dec 3, 2020 at 11:55 AM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 02-12-20 14:07:08, Amir Goldstein wrote:
> > Convert inotify to use the simple handle_inode_event() interface to
> > get rid of the code duplication between the generic helper
> > fsnotify_handle_event() and the inotify_handle_event() callback, which
> > also happen to be buggy code.
> >
> > The bug will be fixed in the generic helper.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/notify/inotify/inotify.h          |  9 +++---
> >  fs/notify/inotify/inotify_fsnotify.c | 47 ++++++----------------------
> >  fs/notify/inotify/inotify_user.c     |  7 +----
> >  3 files changed, 15 insertions(+), 48 deletions(-)
> >
> > diff --git a/fs/notify/inotify/inotify.h b/fs/notify/inotify/inotify.h
> > index 4327d0e9c364..7fc3782b2fb8 100644
> > --- a/fs/notify/inotify/inotify.h
> > +++ b/fs/notify/inotify/inotify.h
> > @@ -24,11 +24,10 @@ static inline struct inotify_event_info *INOTIFY_E(struct fsnotify_event *fse)
> >
> >  extern void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark,
> >                                          struct fsnotify_group *group);
> > -extern int inotify_handle_event(struct fsnotify_group *group, u32 mask,
> > -                             const void *data, int data_type,
> > -                             struct inode *dir,
> > -                             const struct qstr *file_name, u32 cookie,
> > -                             struct fsnotify_iter_info *iter_info);
> > +extern int inotify_handle_event(struct fsnotify_group *group,
> > +                             struct fsnotify_mark *inode_mark, u32 mask,
> > +                             struct inode *inode, struct inode *dir,
> > +                             const struct qstr *name, u32 cookie);
> >
> >  extern const struct fsnotify_ops inotify_fsnotify_ops;
> >  extern struct kmem_cache *inotify_inode_mark_cachep;
> > diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
> > index 9ddcbadc98e2..f348c1d3b358 100644
> > --- a/fs/notify/inotify/inotify_fsnotify.c
> > +++ b/fs/notify/inotify/inotify_fsnotify.c
> > @@ -55,10 +55,10 @@ static int inotify_merge(struct list_head *list,
> >       return event_compare(last_event, event);
> >  }
> >
> > -static int inotify_one_event(struct fsnotify_group *group, u32 mask,
> > -                          struct fsnotify_mark *inode_mark,
> > -                          const struct path *path,
> > -                          const struct qstr *file_name, u32 cookie)
> > +int inotify_handle_event(struct fsnotify_group *group,
> > +                      struct fsnotify_mark *inode_mark, u32 mask,
> > +                      struct inode *inode, struct inode *dir,
> > +                      const struct qstr *file_name, u32 cookie)
> >  {
> >       struct inotify_inode_mark *i_mark;
> >       struct inotify_event_info *event;
> > @@ -68,10 +68,6 @@ static int inotify_one_event(struct fsnotify_group *group, u32 mask,
> >       int alloc_len = sizeof(struct inotify_event_info);
> >       struct mem_cgroup *old_memcg;
> >
> > -     if ((inode_mark->mask & FS_EXCL_UNLINK) &&
> > -         path && d_unlinked(path->dentry))
> > -             return 0;
> > -
> >       if (file_name) {
> >               len = file_name->len;
> >               alloc_len += len + 1;
> > @@ -131,35 +127,12 @@ static int inotify_one_event(struct fsnotify_group *group, u32 mask,
> >       return 0;
> >  }
> >
> > -int inotify_handle_event(struct fsnotify_group *group, u32 mask,
> > -                      const void *data, int data_type, struct inode *dir,
> > -                      const struct qstr *file_name, u32 cookie,
> > -                      struct fsnotify_iter_info *iter_info)
> > +static int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask,
> > +                                   struct inode *inode, struct inode *dir,
> > +                                   const struct qstr *name, u32 cookie)
> >  {
>
> Looking at this I'd just fold inotify_handle_event() into
> inotify_handle_inode_event() as the only difference is the 'group' argument
> and that can be always fetched from inode_mark->group...
>

Yes, that is what I though, but then I wasn't sure about the call coming from
inotify_ignored_and_remove_idr(). It seemed to be on purpose that the
group argument is separate from the freeing mark.

Thanks,
Amir.

Re: [PATCH 7/7] fsnotify: optimize merging of marks with no ignored masks

[Amir Goldstein]

On Thu, Dec 3, 2020 at 12:35 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 02-12-20 14:07:13, Amir Goldstein wrote:
> > fsnotify() tries to merge marks on all object types (sb, mount, inode)
> > even if the object's mask shows no interest in the specific event type.
> >
> > This is done for the case that the object has marks with the event type
> > in their ignored mask, but the common case is that an object does not
> > have any marks with ignored mask.
> >
> > Set a bit in object's fsnotify mask during fsnotify_recalc_mask() to
> > indicate the existence of any marks with ignored masks.
> >
> > Instead of merging marks of all object types, only merge marks from
> > objects that either showed interest in the specific event type or have
> > any marks with ignored mask.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> The idea looks sound to me and the patch fine (besides one bug noted
> below). I'd be interested if an actual performance impact of this and
> previous change could be noticed. Maybe some careful microbenchmark could
> reveal it...

Yeh, as I wrote in the cover letter. I don't know what to do with this.
By all accounts we should not merge unless we can prove benefit.

The reason I brought this back is I was thinking of a way to avoid
having to look up subtree marks on ancestors if sb had no subtree marks
that are interested in the event (or have an ignored mask).

So the benefit may become apparent when we add subtree marks
and then there is no harm to keep the same logic for other mark types.

>
> > +     /*
> > +      * Consider only marks that care about this type of event and marks with
> > +      * an ignored mask.
> > +      */
> > +     test_mask |= FS_HAS_IGNORED_MASK;
> > +     if (test_mask && sb->s_fsnotify_mask) {
>                       ^^ Just '&' here.
>

Heh! oops :)

Thanks,
Amir.

Re: [PATCH 3/7] fsnotify: fix events reported to watching parent and child

[Jan Kara]

On Wed 02-12-20 14:07:09, Amir Goldstein wrote:
> fsnotify_parent() used to send two separate events to backends when a
> parent inode is watcing children and the child inode is also watching.
                     ^^ watching
 
> In an attempt to avoid duplicate events in fanotify, we unified the two
> backend callbacks to a single callback and handled the reporting of the
> two separate events for the relevant backends (inotify and dnotify).
> 
> The unified event callback with two inode marks (parent and child) is
> called when both parent and child inode are watched and interested in
> the event, but they could each be watched by a different group.
> 
> So before reporting the parent or child event flavor to backend we need
> to check that the group is really interested in that event flavor.

So I'm not 100% sure what is the actual visible problem - is it that we
could deliver events a group didn't ask for?

Also I'm confused by a "different group" argument above. AFAICT
fsnotify_iter_select_report_types() makes sure we always select marks from
a single group and only after that we look at mark's masks.

That being said I agree that the loop in send_to_group() will 'or' parent
and child masks and then check test_mask & marks_mask & ~marks_ignored_mask
so if either parent *or* child was interested in the event, we'll deliver
it to both parent and the child. Fanotify is not prone to this since it
does its own checks. Dnotify also isn't prone to the problem because it
has only events on directories (so there are never two inodes to deliver
to). Inotify is prone to the problem although only because we have 'wd' in
the event. So an inotify group can receive event also with a wrong 'wd'.

After more pondering about your patch I think what I write above isn't
actually a problem you were concerned about :) I think you were concerned
about the situation when event mask gets FS_EVENT_ON_CHILD because some
group has a mark on the parent which is interested in watching children
(and so __fsnotify_parent() sets this flag). But then *another* group has
a mark without FS_EVENT_ON_CHILD on the parent but we'll send the event to
it regardless. This can actually result in completely spurious event on
directory inode for inotify & dnotify.

If I understood the problem correctly, I suggest modifying beginning of the
changelog like below because I was able to figure it out but some poor
distro guy deciding whether this could be fixing the problem his customer
is hitting or not has a small chance...

"fsnotify_parent() used to send two separate events to backends when a
parent inode is watching children and the child inode is also watching.
In an attempt to avoid duplicate events in fanotify, we unified the two
backend callbacks to a single callback and handled the reporting of the
two separate events for the relevant backends (inotify and dnotify).
However the handling is buggy and can result in inotify and dnotify listeners
receiving events of the type they never asked for or spurious events."

> The semantics of INODE and CHILD marks were hard to follow and made the
> logic more complicated than it should have been.  Replace it with INODE
> and PARENT marks semantics to hopefully make the logic more clear.

Heh, wasn't I complaining about this when I was initially reviewing the
changes? ;)

> Fixes: eca4784cbb18 ("fsnotify: send event to parent and child with single callback")
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/notify/fanotify/fanotify.c    |  7 ++-
>  fs/notify/fsnotify.c             | 78 ++++++++++++++++++--------------
>  include/linux/fsnotify_backend.h |  6 +--
>  3 files changed, 51 insertions(+), 40 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 9167884a61ec..1192c9953620 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -268,12 +268,11 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
>  			continue;
>  
>  		/*
> -		 * If the event is for a child and this mark is on a parent not
> +		 * If the event is on a child and this mark is on a parent not
>  		 * watching children, don't send it!
>  		 */
> -		if (event_mask & FS_EVENT_ON_CHILD &&
> -		    type == FSNOTIFY_OBJ_TYPE_INODE &&
> -		     !(mark->mask & FS_EVENT_ON_CHILD))
> +		if (type == FSNOTIFY_OBJ_TYPE_PARENT &&
> +		    !(mark->mask & FS_EVENT_ON_CHILD))
>  			continue;
>  
>  		marks_mask |= mark->mask;
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index c5c68bcbaadf..0676ce4d3352 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -152,6 +152,13 @@ static bool fsnotify_event_needs_parent(struct inode *inode, struct mount *mnt,
>  	if (mask & FS_ISDIR)
>  		return false;
>  
> +	/*
> +	 * All events that are possible on child can also may be reported with
> +	 * parent/name info to inode/sb/mount.  Otherwise, a watching parent
> +	 * could result in events reported with unexpected name info to sb/mount.
> +	 */
> +	BUILD_BUG_ON(FS_EVENTS_POSS_ON_CHILD & ~FS_EVENTS_POSS_TO_PARENT);
> +
>  	/* Did either inode/sb/mount subscribe for events with parent/name? */
>  	marks_mask |= fsnotify_parent_needed_mask(inode->i_fsnotify_mask);
>  	marks_mask |= fsnotify_parent_needed_mask(inode->i_sb->s_fsnotify_mask);
> @@ -249,6 +256,10 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group,
>  	    path && d_unlinked(path->dentry))
>  		return 0;
>  
> +	/* Check interest of this mark in case event was sent with two marks */
> +	if (!(mask & inode_mark->mask & ALL_FSNOTIFY_EVENTS))
> +		return 0;
> +
>  	return ops->handle_inode_event(inode_mark, mask, inode, dir, name, cookie);
>  }
>  
> @@ -258,38 +269,40 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask,
>  				 u32 cookie, struct fsnotify_iter_info *iter_info)
>  {
>  	struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
> -	struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info);
> +	struct fsnotify_mark *parent_mark = fsnotify_iter_parent_mark(iter_info);
>  	int ret;
>  
>  	if (WARN_ON_ONCE(fsnotify_iter_sb_mark(iter_info)) ||
>  	    WARN_ON_ONCE(fsnotify_iter_vfsmount_mark(iter_info)))
>  		return 0;
>  
> -	/*
> -	 * An event can be sent on child mark iterator instead of inode mark
> -	 * iterator because of other groups that have interest of this inode
> -	 * and have marks on both parent and child.  We can simplify this case.
> -	 */
> -	if (!inode_mark) {
> -		inode_mark = child_mark;
> -		child_mark = NULL;
> +	if (parent_mark) {
> +		/*
> +		 * parent_mark indicates that the parent inode is watching children
> +		 * and interested in this event, which is an event possible on child.
> +		 * But is this mark watching children and interested in this event?
> +		 */
> +		if (parent_mark->mask & FS_EVENT_ON_CHILD) {

Is this really enough? I'd expect us to also check (mask &
parent_mark->mask & ALL_FSNOTIFY_EVENTS) != 0...

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

Re: [PATCH 1/7] fsnotify: generalize handle_inode_event()

[Jan Kara]

On Thu 03-12-20 12:41:41, Amir Goldstein wrote:
> On Thu, Dec 3, 2020 at 11:51 AM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 02-12-20 14:07:07, Amir Goldstein wrote:
> > > The handle_inode_event() interface was added as (quoting comment):
> > > "a simple variant of handle_event() for groups that only have inode
> > > marks and don't have ignore mask".
> > >
> > > In other words, all backends except fanotify.  The inotify backend
> > > also falls under this category, but because it required extra arguments
> > > it was left out of the initial pass of backends conversion to the
> > > simple interface.
> > >
> > > This results in code duplication between the generic helper
> > > fsnotify_handle_event() and the inotify_handle_event() callback
> > > which also happen to be buggy code.
> > >
> > > Generalize the handle_inode_event() arguments and add the check for
> > > FS_EXCL_UNLINK flag to the generic helper, so inotify backend could
> > > be converted to use the simple interface.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > The patch looks good to me. Just one curious question below.
> >
> > > +static int fsnotify_handle_inode_event(struct fsnotify_group *group,
> > > +                                    struct fsnotify_mark *inode_mark,
> > > +                                    u32 mask, const void *data, int data_type,
> > > +                                    struct inode *dir, const struct qstr *name,
> > > +                                    u32 cookie)
> > > +{
> > > +     const struct path *path = fsnotify_data_path(data, data_type);
> > > +     struct inode *inode = fsnotify_data_inode(data, data_type);
> > > +     const struct fsnotify_ops *ops = group->ops;
> > > +
> > > +     if (WARN_ON_ONCE(!ops->handle_inode_event))
> > > +             return 0;
> > > +
> > > +     if ((inode_mark->mask & FS_EXCL_UNLINK) &&
> > > +         path && d_unlinked(path->dentry))
> > > +             return 0;
> >
> > When I was looking at this condition I was wondering why do we check
> > d_unlinked() and not inode->i_nlink? When is there a difference?
> 
> When a hardlink has been unlinked.
> inotify gets the filename and it doesn't want to get events with unlinked
> names (although another name could still be linked) I suppose...

OK, fair enough. I didn't think about this case.

								Honza

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

Re: [PATCH 2/7] inotify: convert to handle_inode_event() interface

[Jan Kara]

On Thu 03-12-20 12:45:15, Amir Goldstein wrote:
> On Thu, Dec 3, 2020 at 11:55 AM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 02-12-20 14:07:08, Amir Goldstein wrote:
> > > Convert inotify to use the simple handle_inode_event() interface to
> > > get rid of the code duplication between the generic helper
> > > fsnotify_handle_event() and the inotify_handle_event() callback, which
> > > also happen to be buggy code.
> > >
> > > The bug will be fixed in the generic helper.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >  fs/notify/inotify/inotify.h          |  9 +++---
> > >  fs/notify/inotify/inotify_fsnotify.c | 47 ++++++----------------------
> > >  fs/notify/inotify/inotify_user.c     |  7 +----
> > >  3 files changed, 15 insertions(+), 48 deletions(-)
> > >
> > > diff --git a/fs/notify/inotify/inotify.h b/fs/notify/inotify/inotify.h
> > > index 4327d0e9c364..7fc3782b2fb8 100644
> > > --- a/fs/notify/inotify/inotify.h
> > > +++ b/fs/notify/inotify/inotify.h
> > > @@ -24,11 +24,10 @@ static inline struct inotify_event_info *INOTIFY_E(struct fsnotify_event *fse)
> > >
> > >  extern void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark,
> > >                                          struct fsnotify_group *group);
> > > -extern int inotify_handle_event(struct fsnotify_group *group, u32 mask,
> > > -                             const void *data, int data_type,
> > > -                             struct inode *dir,
> > > -                             const struct qstr *file_name, u32 cookie,
> > > -                             struct fsnotify_iter_info *iter_info);
> > > +extern int inotify_handle_event(struct fsnotify_group *group,
> > > +                             struct fsnotify_mark *inode_mark, u32 mask,
> > > +                             struct inode *inode, struct inode *dir,
> > > +                             const struct qstr *name, u32 cookie);
> > >
> > >  extern const struct fsnotify_ops inotify_fsnotify_ops;
> > >  extern struct kmem_cache *inotify_inode_mark_cachep;
> > > diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
> > > index 9ddcbadc98e2..f348c1d3b358 100644
> > > --- a/fs/notify/inotify/inotify_fsnotify.c
> > > +++ b/fs/notify/inotify/inotify_fsnotify.c
> > > @@ -55,10 +55,10 @@ static int inotify_merge(struct list_head *list,
> > >       return event_compare(last_event, event);
> > >  }
> > >
> > > -static int inotify_one_event(struct fsnotify_group *group, u32 mask,
> > > -                          struct fsnotify_mark *inode_mark,
> > > -                          const struct path *path,
> > > -                          const struct qstr *file_name, u32 cookie)
> > > +int inotify_handle_event(struct fsnotify_group *group,
> > > +                      struct fsnotify_mark *inode_mark, u32 mask,
> > > +                      struct inode *inode, struct inode *dir,
> > > +                      const struct qstr *file_name, u32 cookie)
> > >  {
> > >       struct inotify_inode_mark *i_mark;
> > >       struct inotify_event_info *event;
> > > @@ -68,10 +68,6 @@ static int inotify_one_event(struct fsnotify_group *group, u32 mask,
> > >       int alloc_len = sizeof(struct inotify_event_info);
> > >       struct mem_cgroup *old_memcg;
> > >
> > > -     if ((inode_mark->mask & FS_EXCL_UNLINK) &&
> > > -         path && d_unlinked(path->dentry))
> > > -             return 0;
> > > -
> > >       if (file_name) {
> > >               len = file_name->len;
> > >               alloc_len += len + 1;
> > > @@ -131,35 +127,12 @@ static int inotify_one_event(struct fsnotify_group *group, u32 mask,
> > >       return 0;
> > >  }
> > >
> > > -int inotify_handle_event(struct fsnotify_group *group, u32 mask,
> > > -                      const void *data, int data_type, struct inode *dir,
> > > -                      const struct qstr *file_name, u32 cookie,
> > > -                      struct fsnotify_iter_info *iter_info)
> > > +static int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask,
> > > +                                   struct inode *inode, struct inode *dir,
> > > +                                   const struct qstr *name, u32 cookie)
> > >  {
> >
> > Looking at this I'd just fold inotify_handle_event() into
> > inotify_handle_inode_event() as the only difference is the 'group' argument
> > and that can be always fetched from inode_mark->group...
> >
> 
> Yes, that is what I though, but then I wasn't sure about the call coming from
> inotify_ignored_and_remove_idr(). It seemed to be on purpose that the
> group argument is separate from the freeing mark.

Well, the 'group' argument is just fetched from mark->group in
fsnotify_free_mark() so I rather think it is a relict from the past when
the lifetime rules for marks were different. In fact
fsnotify_final_mark_destroy() which is called just before really freeing
the memory uses mark->group. So I'm pretty sure we are safe to grab
mark->group in inotify_handle_event() as well.

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

Re: [PATCH 2/7] inotify: convert to handle_inode_event() interface

[Amir Goldstein]

On Thu, Dec 3, 2020 at 2:37 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 03-12-20 12:45:15, Amir Goldstein wrote:
> > On Thu, Dec 3, 2020 at 11:55 AM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Wed 02-12-20 14:07:08, Amir Goldstein wrote:
> > > > Convert inotify to use the simple handle_inode_event() interface to
> > > > get rid of the code duplication between the generic helper
> > > > fsnotify_handle_event() and the inotify_handle_event() callback, which
> > > > also happen to be buggy code.
> > > >
> > > > The bug will be fixed in the generic helper.
> > > >
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > ---
> > > >  fs/notify/inotify/inotify.h          |  9 +++---
> > > >  fs/notify/inotify/inotify_fsnotify.c | 47 ++++++----------------------
> > > >  fs/notify/inotify/inotify_user.c     |  7 +----
> > > >  3 files changed, 15 insertions(+), 48 deletions(-)
> > > >
> > > > diff --git a/fs/notify/inotify/inotify.h b/fs/notify/inotify/inotify.h
> > > > index 4327d0e9c364..7fc3782b2fb8 100644
> > > > --- a/fs/notify/inotify/inotify.h
> > > > +++ b/fs/notify/inotify/inotify.h
> > > > @@ -24,11 +24,10 @@ static inline struct inotify_event_info *INOTIFY_E(struct fsnotify_event *fse)
> > > >
> > > >  extern void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark,
> > > >                                          struct fsnotify_group *group);
> > > > -extern int inotify_handle_event(struct fsnotify_group *group, u32 mask,
> > > > -                             const void *data, int data_type,
> > > > -                             struct inode *dir,
> > > > -                             const struct qstr *file_name, u32 cookie,
> > > > -                             struct fsnotify_iter_info *iter_info);
> > > > +extern int inotify_handle_event(struct fsnotify_group *group,
> > > > +                             struct fsnotify_mark *inode_mark, u32 mask,
> > > > +                             struct inode *inode, struct inode *dir,
> > > > +                             const struct qstr *name, u32 cookie);
> > > >
> > > >  extern const struct fsnotify_ops inotify_fsnotify_ops;
> > > >  extern struct kmem_cache *inotify_inode_mark_cachep;
> > > > diff --git a/fs/notify/inotify/inotify_fsnotify.c b/fs/notify/inotify/inotify_fsnotify.c
> > > > index 9ddcbadc98e2..f348c1d3b358 100644
> > > > --- a/fs/notify/inotify/inotify_fsnotify.c
> > > > +++ b/fs/notify/inotify/inotify_fsnotify.c
> > > > @@ -55,10 +55,10 @@ static int inotify_merge(struct list_head *list,
> > > >       return event_compare(last_event, event);
> > > >  }
> > > >
> > > > -static int inotify_one_event(struct fsnotify_group *group, u32 mask,
> > > > -                          struct fsnotify_mark *inode_mark,
> > > > -                          const struct path *path,
> > > > -                          const struct qstr *file_name, u32 cookie)
> > > > +int inotify_handle_event(struct fsnotify_group *group,
> > > > +                      struct fsnotify_mark *inode_mark, u32 mask,
> > > > +                      struct inode *inode, struct inode *dir,
> > > > +                      const struct qstr *file_name, u32 cookie)
> > > >  {
> > > >       struct inotify_inode_mark *i_mark;
> > > >       struct inotify_event_info *event;
> > > > @@ -68,10 +68,6 @@ static int inotify_one_event(struct fsnotify_group *group, u32 mask,
> > > >       int alloc_len = sizeof(struct inotify_event_info);
> > > >       struct mem_cgroup *old_memcg;
> > > >
> > > > -     if ((inode_mark->mask & FS_EXCL_UNLINK) &&
> > > > -         path && d_unlinked(path->dentry))
> > > > -             return 0;
> > > > -
> > > >       if (file_name) {
> > > >               len = file_name->len;
> > > >               alloc_len += len + 1;
> > > > @@ -131,35 +127,12 @@ static int inotify_one_event(struct fsnotify_group *group, u32 mask,
> > > >       return 0;
> > > >  }
> > > >
> > > > -int inotify_handle_event(struct fsnotify_group *group, u32 mask,
> > > > -                      const void *data, int data_type, struct inode *dir,
> > > > -                      const struct qstr *file_name, u32 cookie,
> > > > -                      struct fsnotify_iter_info *iter_info)
> > > > +static int inotify_handle_inode_event(struct fsnotify_mark *inode_mark, u32 mask,
> > > > +                                   struct inode *inode, struct inode *dir,
> > > > +                                   const struct qstr *name, u32 cookie)
> > > >  {
> > >
> > > Looking at this I'd just fold inotify_handle_event() into
> > > inotify_handle_inode_event() as the only difference is the 'group' argument
> > > and that can be always fetched from inode_mark->group...
> > >
> >
> > Yes, that is what I though, but then I wasn't sure about the call coming from
> > inotify_ignored_and_remove_idr(). It seemed to be on purpose that the
> > group argument is separate from the freeing mark.
>
> Well, the 'group' argument is just fetched from mark->group in
> fsnotify_free_mark() so I rather think it is a relict from the past when
> the lifetime rules for marks were different. In fact

Ha right! I should have checked that.

> fsnotify_final_mark_destroy() which is called just before really freeing
> the memory uses mark->group. So I'm pretty sure we are safe to grab
> mark->group in inotify_handle_event() as well.
>

Unless I have other fixes to post, I'll assume you will be folding the
inotify_handle_inode_event wrapper on commit.

Thanks,
Amir.

Re: [PATCH 3/7] fsnotify: fix events reported to watching parent and child

[Amir Goldstein]

On Thu, Dec 3, 2020 at 1:53 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 02-12-20 14:07:09, Amir Goldstein wrote:
> > fsnotify_parent() used to send two separate events to backends when a
> > parent inode is watcing children and the child inode is also watching.
>                      ^^ watching
>
> > In an attempt to avoid duplicate events in fanotify, we unified the two
> > backend callbacks to a single callback and handled the reporting of the
> > two separate events for the relevant backends (inotify and dnotify).
> >
> > The unified event callback with two inode marks (parent and child) is
> > called when both parent and child inode are watched and interested in
> > the event, but they could each be watched by a different group.
> >
> > So before reporting the parent or child event flavor to backend we need
> > to check that the group is really interested in that event flavor.
>
> So I'm not 100% sure what is the actual visible problem - is it that we
> could deliver events a group didn't ask for?

Sort of yes. See:
https://github.com/amir73il/ltp/blob/fsnotify-fixes/testcases/kernel/syscalls/inotify/inotify11.c

>
> Also I'm confused by a "different group" argument above. AFAICT
> fsnotify_iter_select_report_types() makes sure we always select marks from
> a single group and only after that we look at mark's masks.

The group will get an event it has ask for but on the wrong object.

>
> That being said I agree that the loop in send_to_group() will 'or' parent
> and child masks and then check test_mask & marks_mask & ~marks_ignored_mask
> so if either parent *or* child was interested in the event, we'll deliver
> it to both parent and the child. Fanotify is not prone to this since it
> does its own checks. Dnotify also isn't prone to the problem because it
> has only events on directories (so there are never two inodes to deliver
> to).

FS_ATTRIB on dir will be delivered to parent watcher as well as child watcher
I think.

> Inotify is prone to the problem although only because we have 'wd' in
> the event. So an inotify group can receive event also with a wrong 'wd'.
>

True wrong wd and with unexpected name or unexpected missing name.
group1 could be watching a file foo and get events on file bar because
group1 is watching the parent (for another event) and group2 is watching
parent for this event.

> After more pondering about your patch I think what I write above isn't
> actually a problem you were concerned about :) I think you were concerned
> about the situation when event mask gets FS_EVENT_ON_CHILD because some
> group has a mark on the parent which is interested in watching children
> (and so __fsnotify_parent() sets this flag). But then *another* group has
> a mark without FS_EVENT_ON_CHILD on the parent but we'll send the event to
> it regardless. This can actually result in completely spurious event on
> directory inode for inotify & dnotify.
>

Haha lags in emails :)

> If I understood the problem correctly, I suggest modifying beginning of the
> changelog like below because I was able to figure it out but some poor
> distro guy deciding whether this could be fixing the problem his customer
> is hitting or not has a small chance...
>
> "fsnotify_parent() used to send two separate events to backends when a
> parent inode is watching children and the child inode is also watching.
> In an attempt to avoid duplicate events in fanotify, we unified the two
> backend callbacks to a single callback and handled the reporting of the
> two separate events for the relevant backends (inotify and dnotify).
> However the handling is buggy and can result in inotify and dnotify listeners
> receiving events of the type they never asked for or spurious events."
>

ACK

> > The semantics of INODE and CHILD marks were hard to follow and made the
> > logic more complicated than it should have been.  Replace it with INODE
> > and PARENT marks semantics to hopefully make the logic more clear.
>
> Heh, wasn't I complaining about this when I was initially reviewing the
> changes? ;)

You certainly did and rightfully so.
It took me a long time to untangle this knot, so I hope you like the result.

>
> > Fixes: eca4784cbb18 ("fsnotify: send event to parent and child with single callback")
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/notify/fanotify/fanotify.c    |  7 ++-
> >  fs/notify/fsnotify.c             | 78 ++++++++++++++++++--------------
> >  include/linux/fsnotify_backend.h |  6 +--
> >  3 files changed, 51 insertions(+), 40 deletions(-)
> >
> > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > index 9167884a61ec..1192c9953620 100644
> > --- a/fs/notify/fanotify/fanotify.c
> > +++ b/fs/notify/fanotify/fanotify.c
> > @@ -268,12 +268,11 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> >                       continue;
> >
> >               /*
> > -              * If the event is for a child and this mark is on a parent not
> > +              * If the event is on a child and this mark is on a parent not
> >                * watching children, don't send it!
> >                */
> > -             if (event_mask & FS_EVENT_ON_CHILD &&
> > -                 type == FSNOTIFY_OBJ_TYPE_INODE &&
> > -                  !(mark->mask & FS_EVENT_ON_CHILD))
> > +             if (type == FSNOTIFY_OBJ_TYPE_PARENT &&
> > +                 !(mark->mask & FS_EVENT_ON_CHILD))
> >                       continue;
> >
> >               marks_mask |= mark->mask;
> > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> > index c5c68bcbaadf..0676ce4d3352 100644
> > --- a/fs/notify/fsnotify.c
> > +++ b/fs/notify/fsnotify.c
> > @@ -152,6 +152,13 @@ static bool fsnotify_event_needs_parent(struct inode *inode, struct mount *mnt,
> >       if (mask & FS_ISDIR)
> >               return false;
> >
> > +     /*
> > +      * All events that are possible on child can also may be reported with
> > +      * parent/name info to inode/sb/mount.  Otherwise, a watching parent
> > +      * could result in events reported with unexpected name info to sb/mount.
> > +      */
> > +     BUILD_BUG_ON(FS_EVENTS_POSS_ON_CHILD & ~FS_EVENTS_POSS_TO_PARENT);
> > +
> >       /* Did either inode/sb/mount subscribe for events with parent/name? */
> >       marks_mask |= fsnotify_parent_needed_mask(inode->i_fsnotify_mask);
> >       marks_mask |= fsnotify_parent_needed_mask(inode->i_sb->s_fsnotify_mask);
> > @@ -249,6 +256,10 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group,
> >           path && d_unlinked(path->dentry))
> >               return 0;
> >
> > +     /* Check interest of this mark in case event was sent with two marks */
> > +     if (!(mask & inode_mark->mask & ALL_FSNOTIFY_EVENTS))
> > +             return 0;
> > +
> >       return ops->handle_inode_event(inode_mark, mask, inode, dir, name, cookie);
> >  }
> >
> > @@ -258,38 +269,40 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask,
> >                                u32 cookie, struct fsnotify_iter_info *iter_info)
> >  {
> >       struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
> > -     struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info);
> > +     struct fsnotify_mark *parent_mark = fsnotify_iter_parent_mark(iter_info);
> >       int ret;
> >
> >       if (WARN_ON_ONCE(fsnotify_iter_sb_mark(iter_info)) ||
> >           WARN_ON_ONCE(fsnotify_iter_vfsmount_mark(iter_info)))
> >               return 0;
> >
> > -     /*
> > -      * An event can be sent on child mark iterator instead of inode mark
> > -      * iterator because of other groups that have interest of this inode
> > -      * and have marks on both parent and child.  We can simplify this case.
> > -      */
> > -     if (!inode_mark) {
> > -             inode_mark = child_mark;
> > -             child_mark = NULL;
> > +     if (parent_mark) {
> > +             /*
> > +              * parent_mark indicates that the parent inode is watching children
> > +              * and interested in this event, which is an event possible on child.
> > +              * But is this mark watching children and interested in this event?
> > +              */
> > +             if (parent_mark->mask & FS_EVENT_ON_CHILD) {
>
> Is this really enough? I'd expect us to also check (mask &
> parent_mark->mask & ALL_FSNOTIFY_EVENTS) != 0...

I put it up in fsnotify_event_needs_parent() because this check is needed
for both parent and child.

BTW, at first I was thinking we needed to check EVENTS_POSS_ON_CHILD
here but we don't because if event is not EVENTS_POSS_ON_CHILD
(a.k.a. !parent_interested) then flag ON_CHILD is not set and parent_mark
is not iterated .

Thanks,
Amir.

Re: [PATCH 3/7] fsnotify: fix events reported to watching parent and child

[Jan Kara]

On Thu 03-12-20 14:58:41, Amir Goldstein wrote:
> On Thu, Dec 3, 2020 at 1:53 PM Jan Kara <jack@suse.cz> wrote:
> > > The semantics of INODE and CHILD marks were hard to follow and made the
> > > logic more complicated than it should have been.  Replace it with INODE
> > > and PARENT marks semantics to hopefully make the logic more clear.
> >
> > Heh, wasn't I complaining about this when I was initially reviewing the
> > changes? ;)
> 
> You certainly did and rightfully so.
> It took me a long time to untangle this knot, so I hope you like the result.

Yes, it is IMO more readable now.

> > > Fixes: eca4784cbb18 ("fsnotify: send event to parent and child with single callback")
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >  fs/notify/fanotify/fanotify.c    |  7 ++-
> > >  fs/notify/fsnotify.c             | 78 ++++++++++++++++++--------------
> > >  include/linux/fsnotify_backend.h |  6 +--
> > >  3 files changed, 51 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > > index 9167884a61ec..1192c9953620 100644
> > > --- a/fs/notify/fanotify/fanotify.c
> > > +++ b/fs/notify/fanotify/fanotify.c
> > > @@ -268,12 +268,11 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> > >                       continue;
> > >
> > >               /*
> > > -              * If the event is for a child and this mark is on a parent not
> > > +              * If the event is on a child and this mark is on a parent not
> > >                * watching children, don't send it!
> > >                */
> > > -             if (event_mask & FS_EVENT_ON_CHILD &&
> > > -                 type == FSNOTIFY_OBJ_TYPE_INODE &&
> > > -                  !(mark->mask & FS_EVENT_ON_CHILD))
> > > +             if (type == FSNOTIFY_OBJ_TYPE_PARENT &&
> > > +                 !(mark->mask & FS_EVENT_ON_CHILD))
> > >                       continue;
> > >
> > >               marks_mask |= mark->mask;
> > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> > > index c5c68bcbaadf..0676ce4d3352 100644
> > > --- a/fs/notify/fsnotify.c
> > > +++ b/fs/notify/fsnotify.c
> > > @@ -152,6 +152,13 @@ static bool fsnotify_event_needs_parent(struct inode *inode, struct mount *mnt,
> > >       if (mask & FS_ISDIR)
> > >               return false;
> > >
> > > +     /*
> > > +      * All events that are possible on child can also may be reported with
> > > +      * parent/name info to inode/sb/mount.  Otherwise, a watching parent
> > > +      * could result in events reported with unexpected name info to sb/mount.
> > > +      */
> > > +     BUILD_BUG_ON(FS_EVENTS_POSS_ON_CHILD & ~FS_EVENTS_POSS_TO_PARENT);
> > > +
> > >       /* Did either inode/sb/mount subscribe for events with parent/name? */
> > >       marks_mask |= fsnotify_parent_needed_mask(inode->i_fsnotify_mask);
> > >       marks_mask |= fsnotify_parent_needed_mask(inode->i_sb->s_fsnotify_mask);
> > > @@ -249,6 +256,10 @@ static int fsnotify_handle_inode_event(struct fsnotify_group *group,
> > >           path && d_unlinked(path->dentry))
> > >               return 0;
> > >
> > > +     /* Check interest of this mark in case event was sent with two marks */
> > > +     if (!(mask & inode_mark->mask & ALL_FSNOTIFY_EVENTS))
> > > +             return 0;
> > > +
> > >       return ops->handle_inode_event(inode_mark, mask, inode, dir, name, cookie);
> > >  }
> > >
> > > @@ -258,38 +269,40 @@ static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask,
> > >                                u32 cookie, struct fsnotify_iter_info *iter_info)
> > >  {
> > >       struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
> > > -     struct fsnotify_mark *child_mark = fsnotify_iter_child_mark(iter_info);
> > > +     struct fsnotify_mark *parent_mark = fsnotify_iter_parent_mark(iter_info);
> > >       int ret;
> > >
> > >       if (WARN_ON_ONCE(fsnotify_iter_sb_mark(iter_info)) ||
> > >           WARN_ON_ONCE(fsnotify_iter_vfsmount_mark(iter_info)))
> > >               return 0;
> > >
> > > -     /*
> > > -      * An event can be sent on child mark iterator instead of inode mark
> > > -      * iterator because of other groups that have interest of this inode
> > > -      * and have marks on both parent and child.  We can simplify this case.
> > > -      */
> > > -     if (!inode_mark) {
> > > -             inode_mark = child_mark;
> > > -             child_mark = NULL;
> > > +     if (parent_mark) {
> > > +             /*
> > > +              * parent_mark indicates that the parent inode is watching children
> > > +              * and interested in this event, which is an event possible on child.
> > > +              * But is this mark watching children and interested in this event?
> > > +              */
> > > +             if (parent_mark->mask & FS_EVENT_ON_CHILD) {
> >
> > Is this really enough? I'd expect us to also check (mask &
> > parent_mark->mask & ALL_FSNOTIFY_EVENTS) != 0...
> 
> I put it up in fsnotify_event_needs_parent() because this check is needed
> for both parent and child.

Right, I missed that. Thanks for explanation.

> BTW, at first I was thinking we needed to check EVENTS_POSS_ON_CHILD
> here but we don't because if event is not EVENTS_POSS_ON_CHILD
> (a.k.a. !parent_interested) then flag ON_CHILD is not set and parent_mark
> is not iterated .

OK :). I'll just improve the changelog and pick this patch up.

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

Re: [PATCH 0/7] fsnotify fixes and cleanups

[Jan Kara]

Hi Amir!

On Wed 02-12-20 14:07:06, Amir Goldstein wrote:
> I was working on some non urgent cleanups and stumbled on a bug,
> so flushing my patch queue as is.
> 
> Patches 1-2 are cleanups needed for the bug fix in patch 3.
> This [1] LTP test demonstrates the bug.
> 
> Patches 4-5 are pretty simple cleanups that you may or may not like
> to apply without the work that they build up to (I started this as
> prep work for subtree marks iterator).
> 
> Patches 6-7 are optimizations related to ignored mask which we
> discussed in the past.  I have written them a while back and had put
> them aside because I have no means to run performance tests that will
> demonstrate the benefit, which is probably not huge.
> 
> Since you suggested those optimizations (at least the first one),
> I decided to post and let you choose what to do with them.

Thanks for the fixes and optimizations. For now I've picked up patches 1-3
to my tree (with smaller fixups we talked about - plus I'm not sure where
you've got the "Fixes" tag commit ID from - I've fixed that as well). For
cleanups 4-5, I have no problem with them but also at this point don't see
a strong reason to merge them. I do like optimizations 6-7 but I'd like to
see some numbers on them so I didn't queue them just yet either.

								Honza

> [1] https://github.com/amir73il/ltp/commits/fsnotify-fixes
> 
> Amir Goldstein (7):
>   fsnotify: generalize handle_inode_event()
>   inotify: convert to handle_inode_event() interface
>   fsnotify: fix events reported to watching parent and child
>   fsnotify: clarify object type argument
>   fsnotify: separate mark iterator type from object type enum
>   fsnotify: optimize FS_MODIFY events with no ignored masks
>   fsnotify: optimize merging of marks with no ignored masks
> 
>  fs/nfsd/filecache.c                  |   2 +-
>  fs/notify/dnotify/dnotify.c          |   2 +-
>  fs/notify/fanotify/fanotify.c        |  16 +--
>  fs/notify/fanotify/fanotify_user.c   |  44 +++++---
>  fs/notify/fsnotify.c                 | 147 +++++++++++++++++----------
>  fs/notify/group.c                    |   2 +-
>  fs/notify/inotify/inotify.h          |   9 +-
>  fs/notify/inotify/inotify_fsnotify.c |  47 ++-------
>  fs/notify/inotify/inotify_user.c     |   7 +-
>  fs/notify/mark.c                     |  30 +++---
>  include/linux/fsnotify_backend.h     |  92 +++++++++++------
>  kernel/audit_fsnotify.c              |   2 +-
>  kernel/audit_tree.c                  |   2 +-
>  kernel/audit_watch.c                 |   2 +-
>  14 files changed, 233 insertions(+), 171 deletions(-)
> 
> -- 
> 2.25.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR