> > So at this time, we have patch v2 which looks like a clear win.
> > It uses a slightly hackish SB_I_CONTENT_WATCHED to work around
> > the fact that real_mount() is not accessible to the inlined call sites.
>
> But won't it be better to move mnt_fsnotify_mask and perhaps
> mnt_fsnotify_marks (to keep things in one place) into struct vfsmount in
> that case?
I am afraid to even bring this up to Al and Christian ;-)
> IMHO working around this visibility problem with extra flag (and
> the code maintaining it) is not a great tradeoff...
I agree.
>
> As I wrote in my email to Jens I think your v3 patch just makes the real
> cost of fsnotify checks visible in fsnotify_path() instead of being hidden
> in the cost of read / write.
There is a flaw in that argument -
Assuming that in Jens' test no parent/mnt/sb are watching,
all the reads/write on regular file would end up calling __fsnotify_parent()
in upstream code - there is nothing to optimize away this call.
But now that I am looking closer at __fsnotify_parent(), I see what you
were trying to say - if I move the checks from fsnotify_path() to the
beginning of __fsnotify_parent(), it should get most of the performance
from v3 patch, so this is all that should be needed is this simple patch
attached.
I cannot really understand why we did not do this sooner...
Jens, if you have time for another round please test.
My expectation would be to see something like
+1.46% [kernel.vmlinux] [k] __fsnotify_parent
Thanks,
Amir.
+
+ if (!(mask & marks_mask))
+ return 0;
+ }
From 43bbeff601ed510936ac51b572d24388107f033b Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@xxxxxxxxx>
Date: Fri, 9 Dec 2022 11:50:26 +0200
Subject: [PATCH] fsnotify: optimize the case of no parent watcher
If parent inode is not watching, check for the event in masks of
sb/mount/inode masks early to optimize out most of the code in
__fsnotify_parent() and fsnotify().
Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
---
fs/notify/fsnotify.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 8bfd690e9f10..ae74f6e60c64 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -190,13 +190,15 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data,
struct qstr *file_name = NULL;
int ret = 0;
- /*
- * Do inode/sb/mount care about parent and name info on non-dir?
- * Do they care about any event at all?
- */
- if (!inode->i_fsnotify_marks && !inode->i_sb->s_fsnotify_marks &&
- (!mnt || !mnt->mnt_fsnotify_marks) && !parent_watched)
- return 0;
+ /* Optimize the likely case of parent not watching */
+ if (likely(!parent_watched)) {
+ __u32 marks_mask = inode->i_fsnotify_mask |
+ inode->i_sb->s_fsnotify_mask |
+ (mnt ? mnt->mnt_fsnotify_mask : 0);
+
+ if (!(mask & marks_mask))
+ return 0;
+ }
parent = NULL;
parent_needed = fsnotify_event_needs_parent(inode, mnt, mask);
--
2.34.1
