Re: [RFC][PATCH v2] fsnotify: optimize the case of no content event watchers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jan 12, 2024 at 4:11 PM Jens Axboe <axboe@xxxxxxxxx> wrote:
>
> On 1/12/24 6:58 AM, Jens Axboe wrote:
> > 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).
>
> Ran this patch, and it's better than mainline for sure, but it does have
> additional overhead that the previous version did not:
>
>                +1.46%  [kernel.vmlinux]  [k] fsnotify_path
>

That is what I suspected would happen.

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.

With a little more effort, we can move SB_I_CONTENT_WATCHED
into the s_fsnotify_mask, but TBH, I don't know if moving it is worth
the effort because right now, SB_I_CONTENT_WATCHED would
be really rare and with HSM events, an sb really does behave in
slightly different ways, so it might not be such a bad idea to mark
the sb and publish this state to all the subsystems with an s_iflags.

If folks agree to keeping SB_I_CONTENT_WATCHED, we would
need to protect from a rare concurrent update with
SB_I_TS_EXPIRY_WARNED flag.

Jan, what do you think?

Thanks,
Amir.





[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux