On 1/12/24 6:00 AM, Amir Goldstein wrote:
> On Fri, Jan 12, 2024 at 1:09?PM Jan Kara <jack@xxxxxxx> wrote:
>>
>> On Thu 11-01-24 17:22:33, Amir Goldstein wrote:
>>> Commit e43de7f0862b ("fsnotify: optimize the case of no marks of any type")
>>> optimized the case where there are no fsnotify watchers on any of the
>>> filesystem's objects.
>>>
>>> It is quite common for a system to have a single local filesystem and
>>> it is quite common for the system to have some inotify watches on some
>>> config files or directories, so the optimization of no marks at all is
>>> often not in effect.
>>>
>>> Content event (i.e. access,modify) watchers on sb/mount more rare, so
>>> optimizing the case of no sb/mount marks with content events can improve
>>> performance for more systems, especially for performance sensitive io
>>> workloads.
>>>
>>> Set a per-sb flag SB_I_CONTENT_WATCHED if sb/mount marks with content
>>> events in their mask exist and use that flag to optimize out the call to
>>> __fsnotify_parent() and fsnotify() in fsnotify access/modify hooks.
>>>
>>> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
>>
>> ...
>>
>>> -static inline int fsnotify_file(struct file *file, __u32 mask)
>>> +static inline int fsnotify_path(const struct path *path, __u32 mask)
>>> {
>>> - const struct path *path;
>>> + struct dentry *dentry = path->dentry;
>>>
>>> - if (file->f_mode & FMODE_NONOTIFY)
>>> + if (!fsnotify_sb_has_watchers(dentry->d_sb))
>>> return 0;
>>>
>>> - path = &file->f_path;
>>> + /* Optimize the likely case of sb/mount/parent not watching content */
>>> + if (mask & FSNOTIFY_CONTENT_EVENTS &&
>>> + likely(!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED)) &&
>>> + likely(!(dentry->d_sb->s_iflags & SB_I_CONTENT_WATCHED))) {
>>> + /*
>>> + * XXX: if SB_I_CONTENT_WATCHED is not set, checking for content
>>> + * events in s_fsnotify_mask is redundant, but it will be needed
>>> + * if we use the flag FS_MNT_CONTENT_WATCHED to indicate the
>>> + * existence of only mount content event watchers.
>>> + */
>>> + __u32 marks_mask = d_inode(dentry)->i_fsnotify_mask |
>>> + dentry->d_sb->s_fsnotify_mask;
>>> +
>>> + if (!(mask & marks_mask))
>>> + return 0;
>>> + }
>>
>> So I'm probably missing something but how is all this patch different from:
>>
>> if (likely(!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED))) {
>> __u32 marks_mask = d_inode(dentry)->i_fsnotify_mask |
>> path->mnt->mnt_fsnotify_mask |
>
> It's actually:
>
> real_mount(path->mnt)->mnt_fsnotify_mask
>
> and this requires including "internal/mount.h" in all the call sites.
>
>> dentry->d_sb->s_fsnotify_mask;
>> if (!(mask & marks_mask))
>> return 0;
>> }
>>
>> I mean (mask & FSNOTIFY_CONTENT_EVENTS) is true for the frequent events
>> (read & write) we care about. In Jens' case
>>
>> !(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) &&
>> !(dentry->d_sb->s_iflags & SB_I_CONTENT_WATCHED)
>>
>> is true as otherwise we'd go right to fsnotify_parent() and so Jens
>> wouldn't see the performance benefit. But then with your patch you fetch
>> i_fsnotify_mask and s_fsnotify_mask anyway for the test so the only
>> difference to what I suggest above is the path->mnt->mnt_fsnotify_mask
>> fetch but that is equivalent to sb->s_iflags (or wherever we store that
>> bit) fetch?
>>
>> So that would confirm that the parent handling costs in fsnotify_parent()
>> is what's really making the difference and just avoiding that by checking
>> masks early should be enough?
>
> Can't the benefit be also related to saving a function call?
>
> Only one way to find out...
>
> Jens,
>
> Can you please test attached v3 with a non-inlined fsnotify_path() helper?
I can run it since it doesn't take much to do that, but there's no way
parallel universe where saving a function call would yield those kinds
of wins (or have that much cost).
--
Jens Axboe
