[PATCH v2 0/2] Fsnotify ignored mask related fixes

[PATCH v2 0/2] Fsnotify ignored mask related fixes

[Amir Goldstein]

Jan,

The two patches are functionally unrelated, but they both incorporate
information from the ignored mask into the object's interest mask.

The 1st patch is a fix for a minor functional bug.
LTP test fanotify10 has a workaround for this bug, see comment:
  /* XXX: temporary hack may be removed in the future */
The test is to remove the hack and see that fanotify10 passes.

The 2nd patch is a performance optimization that you suggested.
Last time I posted it, you asked for performance numbers [1], so I ran
some micro benchmarks (see results below [3]).

Note that the other patch from that first posting ("optimize merging of
marks with no ignored masks") did not demonstrate any visible
improvements with the benchmarks that I ran so I left it out.

The micro benchmark is a simple program [2] that writes 1 byte at a time
in a loop. I ran it on tmpfs once without any mark and once with a mark
with a mask for DELETE_SELF event.

On upstream kernel, runtime with a mark is always ~25% longer.
With the optimization patch applied, runtime with a mark is most of the
time (but not always) very close to the runtime without a mark.

[1] https://lore.kernel.org/linux-fsdevel/20201203145220.GH11854@quack2.suse.cz/
[2] https://github.com/amir73il/fsnotify-utils/blob/master/src/test/ioloop.c
[3] Performance test results:

$ time ./ioloop /tmp/foo 10000000 w
$ inotifywait -e delete_self /tmp/foo &
$ time ./ioloop /tmp/foo 10000000 w
$ rm /tmp/foo

5.17.0-rc2 #1
-------------
ioloop count=10000000 op=write

real	0m24.264s
user	0m3.977s
sys	0m20.278s

Setting up watches.
Watches established.
ioloop count=10000000 op=write

real	0m29.914s
user	0m3.929s
sys	0m25.974s

/tmp/foo DELETE_SELF

5.17.0-rc2 #2
-------------
ioloop count=10000000 op=write

real	0m26.836s
user	0m4.212s
sys	0m22.615s

Setting up watches.
Watches established.
ioloop count=10000000 op=write

real	0m30.206s
user	0m4.110s
sys	0m26.090s

/tmp/foo DELETE_SELF

5.17.0-rc2 #3
-------------
ioloop count=10000000 op=write

real	0m25.359s
user	0m4.386s
sys	0m20.945s

Setting up watches.
Watches established.
ioloop count=10000000 op=write

real	0m30.213s
user	0m4.187s
sys	0m26.020s

/tmp/foo DELETE_SELF

fsnotify-ignored #1
-------------------
ioloop count=10000000 op=write

real	0m25.020s
user	0m3.982s
sys	0m21.028s

Setting up watches.
Watches established.
ioloop count=10000000 op=write

real	0m26.084s
user	0m4.266s
sys	0m21.812s

/tmp/foo DELETE_SELF

fsnotify-ignored #2
-------------------
ioloop count=10000000 op=write

real	0m24.642s
user	0m3.945s
sys	0m20.677s

Setting up watches.
Watches established.
ioloop count=10000000 op=write

real	0m25.790s
user	0m4.209s
sys	0m21.572s

/tmp/foo DELETE_SELF

fsnotify-ignored #3
-------------------
ioloop count=10000000 op=write

real	0m25.233s
user	0m4.315s
sys	0m20.906s

Setting up watches.
Watches established.
ioloop count=10000000 op=write

real	0m28.800s
user	0m4.329s
sys	0m24.462s

/tmp/foo DELETE_SELF

Amir Goldstein (2):
  fsnotify: fix merge with parent's ignored mask
  fsnotify: optimize FS_MODIFY events with no ignored masks

 fs/notify/fanotify/fanotify_user.c | 47 +++++++++++++++++++++---------
 fs/notify/fsnotify.c               |  8 +++--
 fs/notify/mark.c                   |  4 +--
 include/linux/fsnotify_backend.h   | 19 ++++++++++++
 4 files changed, 59 insertions(+), 19 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] fsnotify: fix merge with parent's ignored mask

[Amir Goldstein]

fsnotify_parent() does not consider the parent's mark at all unless
the parent inode shows interest in events on children and in the
specific event.

So unless parent added an event to both its mark mask and ignored mask,
the event will not be ignored.

Fix this by declaring the interest of an object in an event when the
event is in either a mark mask or ignored mask.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify_user.c | 17 +++++++++--------
 fs/notify/mark.c                   |  4 ++--
 include/linux/fsnotify_backend.h   | 15 +++++++++++++++
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 2ff6bd85ba8f..bd99430a128d 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1003,17 +1003,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
@@ -1023,7 +1024,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,
@@ -1081,23 +1082,23 @@ 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 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)
 			fsn_mark->flags |= FSNOTIFY_MARK_FLAG_IGNORED_SURV_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,
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 9007d6affff3..4853184f7dde 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;
 }
@@ -692,7 +692,7 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
 	if (ret)
 		goto err;
 
-	if (mark->mask)
+	if (mark->mask || mark->ignored_mask)
 		fsnotify_recalc_mask(mark->connector);
 
 	return ret;
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 790c31844db5..5f9c960049b0 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -601,6 +601,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;
+
+	/*
+	 * If mark is interested in ignoring events on children, the object must
+	 * show interest in those events for fsnotify_parent() to notice it.
+	 */
+	return mask | (mark->ignored_mask & ALL_FSNOTIFY_EVENTS);
+}
+
 /* 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 v2 2/2] 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 | 32 +++++++++++++++++++++++-------
 fs/notify/fsnotify.c               |  8 +++++---
 include/linux/fsnotify_backend.h   |  4 ++++
 3 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index bd99430a128d..9b32b76a9c30 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1081,8 +1081,28 @@ static int fanotify_remove_inode_mark(struct fsnotify_group *group,
 				    flags, umask);
 }
 
+static void fanotify_mark_add_ignored_mask(struct fsnotify_mark *fsn_mark,
+					   __u32 mask, unsigned int flags,
+					   __u32 *removed)
+{
+	fsn_mark->ignored_mask |= mask;
+
+	/*
+	 * Setting FAN_MARK_IGNORED_SURV_MODIFY for the first time may lead to
+	 * the removal of the FS_MODIFY bit in calculated mask if it was set
+	 * because of an ignored mask that is now 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;
+	}
+}
+
 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, newmask;
 
@@ -1091,9 +1111,7 @@ static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
 	if (!(flags & FAN_MARK_IGNORED_MASK)) {
 		fsn_mark->mask |= mask;
 	} else {
-		fsn_mark->ignored_mask |= mask;
-		if (flags & FAN_MARK_IGNORED_SURV_MODIFY)
-			fsn_mark->flags |= FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY;
+		fanotify_mark_add_ignored_mask(fsn_mark, mask, flags, removed);
 	}
 	newmask = fsnotify_calc_mask(fsn_mark);
 	spin_unlock(&fsn_mark->lock);
@@ -1156,7 +1174,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;
 	int ret = 0;
 
 	mutex_lock(&group->mark_mutex);
@@ -1179,8 +1197,8 @@ static int fanotify_add_mark(struct fsnotify_group *group,
 			goto out;
 	}
 
-	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);
 
 out:
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index ab81a0776ece..494f653efbc6 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -531,11 +531,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/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 5f9c960049b0..0805b74cae44 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -609,6 +609,10 @@ static inline __u32 fsnotify_calc_mask(struct fsnotify_mark *mark)
 	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;
+
 	/*
 	 * If mark is interested in ignoring events on children, the object must
 	 * show interest in those events for fsnotify_parent() to notice it.
-- 
2.25.1

Re: [PATCH v2 0/2] Fsnotify ignored mask related fixes

[Jan Kara]

Hi Amir!

On Wed 23-02-22 17:14:36, Amir Goldstein wrote:
> The two patches are functionally unrelated, but they both incorporate
> information from the ignored mask into the object's interest mask.
> 
> The 1st patch is a fix for a minor functional bug.
> LTP test fanotify10 has a workaround for this bug, see comment:
>   /* XXX: temporary hack may be removed in the future */
> The test is to remove the hack and see that fanotify10 passes.
> 
> The 2nd patch is a performance optimization that you suggested.
> Last time I posted it, you asked for performance numbers [1], so I ran
> some micro benchmarks (see results below [3]).
> 
> Note that the other patch from that first posting ("optimize merging of
> marks with no ignored masks") did not demonstrate any visible
> improvements with the benchmarks that I ran so I left it out.
> 
> The micro benchmark is a simple program [2] that writes 1 byte at a time
> in a loop. I ran it on tmpfs once without any mark and once with a mark
> with a mask for DELETE_SELF event.
> 
> On upstream kernel, runtime with a mark is always ~25% longer.
> With the optimization patch applied, runtime with a mark is most of the
> time (but not always) very close to the runtime without a mark.
> 
> [1] https://lore.kernel.org/linux-fsdevel/20201203145220.GH11854@quack2.suse.cz/
> [2] https://github.com/amir73il/fsnotify-utils/blob/master/src/test/ioloop.c
> [3] Performance test results:

Thanks! Both patches look good to me, I have added them to my tree (just
included shortened performance results into the changelog of the second
patch).

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