Skip to contentSkip to conversation

fanotify: limit number of event merge attempts

PATCH In review Amir Goldstein 2 participants 2 messages Sep 15, 2021 – Sep 16, 2021

Subsystems (2)
Stable branch VFS
lore
Share & tools
lore

Amir Goldstein

author
#0 lore raw

PATCH 1/1 fanotify: limit number of event merge attempts

commit b8cd0ee8cda68a888a317991c1e918a8cba1a568 upstream.

Event merges are expensive when event queue size is large, so limit the linear search to 128 merge tests.

[Stable backport notes] The following statement from upstream commit is irrelevant for backport:
-
-In combination with 128 size hash table, there is a potential to merge -with up to 16K events in the hashed queue.
-
[Stable backport notes] The problem is as old as fanotify and described in the linked cover letter "Performance improvement for fanotify merge". This backported patch fixes the performance issue at the cost of merging fewer potential events. Fixing the performance issue is more important than preserving the "event merge" behavior, which was not predictable in any way that applications could rely on.

Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Amir Goldstein <[email protected]>
Signed-off-by Jan Kara <[email protected]>
Cc <[email protected]>
Link https://lore.kernel.org/linux-fsdevel/[email protected]/ open here
Link https://lore.kernel.org/linux-fsdevel/[email protected]/ open here
Signed-off-by Amir Goldstein <[email protected]>
---
 fs/notify/fanotify/fanotify.c | 6 ++++++
 1 file changed, 6 insertions(+)
fs/notify/fanotify/fanotify.c +6 -0
index 1192c9953620..c3af99e94f1d 100644
@@ -129,11 +129,15 @@ static bool fanotify_should_merge(struct fsnotify_event *old_fsn,
 	return false;
 }
 
+/* Limit event merges to limit CPU overhead per event */
+#define FANOTIFY_MAX_MERGE_EVENTS 128
+
 /* and the list better be locked by something too! */
 static int fanotify_merge(struct list_head *list, struct fsnotify_event *event)
 {
 	struct fsnotify_event *test_event;
 	struct fanotify_event *new;
+	int i = 0;
 
 	pr_debug("%s: list=%p event=%p\n", __func__, list, event);
 	new = FANOTIFY_E(event);

@@ -147,6 +151,8 @@ static int fanotify_merge(struct list_head *list, struct fsnotify_event *event)
 		return 0;
 
 	list_for_each_entry_reverse(test_event, list, list) {
+		if (++i > FANOTIFY_MAX_MERGE_EVENTS)
+			break;
 		if (fanotify_should_merge(test_event, event)) {
 			FANOTIFY_E(test_event)->mask |= new->mask;
 			return 1;
Signature · 1 line
2.16.5
maintainer
#1 lore raw
Quoted 25 lines · Amir Goldstein

commit b8cd0ee8cda68a888a317991c1e918a8cba1a568 upstream.

Event merges are expensive when event queue size is large, so limit the linear search to 128 merge tests.

[Stable backport notes] The following statement from upstream commit is irrelevant for backport:

(quoted diff) +0 -4
-
-In combination with 128 size hash table, there is a potential to merge
-with up to 16K events in the hashed queue.
-

[Stable backport notes] The problem is as old as fanotify and described in the linked cover letter "Performance improvement for fanotify merge". This backported patch fixes the performance issue at the cost of merging fewer potential events. Fixing the performance issue is more important than preserving the "event merge" behavior, which was not predictable in any way that applications could rely on.

Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Amir Goldstein <[email protected]>
Signed-off-by Jan Kara <[email protected]>
Cc <[email protected]>
Link https://lore.kernel.org/linux-fsdevel/[email protected]/ open here
Link https://lore.kernel.org/linux-fsdevel/[email protected]/ open here
Signed-off-by Amir Goldstein <[email protected]>
Quoted 3 lines
---
 fs/notify/fanotify/fanotify.c | 6 ++++++
 1 file changed, 6 insertions(+)

Now queued up, thanks.

greg k-h