Message-ID: <CAOQ4uxj4pwH2hfmNL0N=q8-rOF6d=-Z_yWLEwHQ671t1EvRn6A@mail.gmail.com>
Date: Wed, 20 Nov 2024 17:42:18 +0100
From: Amir Goldstein <amir73il@...il.com>
To: Jan Kara <jack@...e.cz>
Cc: Josef Bacik <josef@...icpanda.com>, kernel-team@...com, linux-fsdevel@...r.kernel.org, 
	brauner@...nel.org, torvalds@...ux-foundation.org, viro@...iv.linux.org.uk, 
	linux-xfs@...r.kernel.org, linux-btrfs@...r.kernel.org, linux-mm@...ck.org, 
	linux-ext4@...r.kernel.org
Subject: Re: [PATCH v8 03/19] fsnotify: add helper to check if file is
 actually being watched

On Wed, Nov 20, 2024 at 5:02 PM Jan Kara <jack@...e.cz> wrote:
>
> On Fri 15-11-24 10:30:16, Josef Bacik wrote:
> > From: Amir Goldstein <amir73il@...il.com>
> >
> > So far, we set FMODE_NONOTIFY_ flags at open time if we know that there
> > are no permission event watchers at all on the filesystem, but lack of
> > FMODE_NONOTIFY_ flags does not mean that the file is actually watched.
> >
> > To make the flags more accurate we add a helper that checks if the
> > file's inode, mount, sb or parent are being watched for a set of events.
> >
> > This is going to be used for setting FMODE_NONOTIFY_HSM only when the
> > specific file is actually watched for pre-content events.
> >
> > Signed-off-by: Amir Goldstein <amir73il@...il.com>
>
> I did some changes here as well. See below:
>
> > -/* Are there any inode/mount/sb objects that are interested in this event? */
> > -static inline bool fsnotify_object_watched(struct inode *inode, __u32 mnt_mask,
> > -                                        __u32 mask)
> > +/* Are there any inode/mount/sb objects that watch for these events? */
> > +static inline __u32 fsnotify_object_watched(struct inode *inode, __u32 mnt_mask,
> > +                                         __u32 events_mask)
> >  {
> >       __u32 marks_mask = READ_ONCE(inode->i_fsnotify_mask) | mnt_mask |
> >                          READ_ONCE(inode->i_sb->s_fsnotify_mask);
> >
> > -     return mask & marks_mask & ALL_FSNOTIFY_EVENTS;
> > +     return events_mask & marks_mask;
> >  }
> >
> > +/* Are there any inode/mount/sb/parent objects that watch for these events? */
> > +__u32 fsnotify_file_object_watched(struct file *file, __u32 events_mask)
> > +{
> > +     struct dentry *dentry = file->f_path.dentry;
> > +     struct dentry *parent;
> > +     __u32 marks_mask, mnt_mask =
> > +             READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask);
> > +
> > +     marks_mask = fsnotify_object_watched(d_inode(dentry), mnt_mask,
> > +                                          events_mask);
> > +
> > +     if (likely(!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED)))
> > +             return marks_mask;
> > +
> > +     parent = dget_parent(dentry);
> > +     marks_mask |= fsnotify_inode_watches_children(d_inode(parent));
> > +     dput(parent);
> > +
> > +     return marks_mask & events_mask;
> > +}
> > +EXPORT_SYMBOL_GPL(fsnotify_file_object_watched);
>
> I find it confusing that fsnotify_object_watched() does not take parent
> into account while fsnotify_file_object_watched() does. Furthermore the
> naming doesn't very well reflect the fact we are actually returning a mask
> of events. I've ended up dropping this helper (it's used in a single place
> anyway) and instead doing the same directly in file_set_fsnotify_mode().
>
> @@ -658,6 +660,27 @@ void file_set_fsnotify_mode(struct file *file)
>                 file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM;
>                 return;
>         }
> +
> +       /*
> +        * OK, there are some pre-content watchers. Check if anybody can be
> +        * watching for pre-content events on *this* file.
> +        */
> +       mnt_mask = READ_ONCE(real_mount(file->f_path.mnt)->mnt_fsnotify_mask);
> +       if (likely(!(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED) &&
> +           !fsnotify_object_watched(d_inode(dentry), mnt_mask,
> +                                    FSNOTIFY_PRE_CONTENT_EVENTS))) {
> +               file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM;
> +               return;
> +       }
> +
> +       /* Even parent is not watching for pre-content events on this file? */
> +       parent = dget_parent(dentry);
> +       p_mask = fsnotify_inode_watches_children(d_inode(parent));
> +       dput(parent);
> +       if (!(p_mask & FSNOTIFY_PRE_CONTENT_EVENTS)) {
> +               file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM;
> +               return;
> +       }
>  }
>

Nice!

Note that I had a "hidden motive" for future optimization when I changed
return value of fsnotify_object_watched() to a mask -

I figured that while we are doing the checks above, we can check for the
same price the mask ALL_FSNOTIFY_PERM_EVENTS
then we get several answers for the same price:
1. Is the specific file watched by HSM?
2. Is the specific file watched by open permission events?
3. Is the specific file watched by post-open FAN_ACCESS_PERM?

If the answers are No, No, No, we get some extra optimization
in the (uncommon) use case that there are permission event watchers
on some random inodes in the filesystem.

If the answers are Yes, Yes, No, or No, Yes, No we can return a special
value from file_set_fsnotify_mode() to indicate that permission events
are needed ONLY for fsnotify_open_perm() hook, but not thereafter.

This would implement the semantic change of "respect FAN_ACCESS_PERM
only if it existed at open time" that can save a lot of unneeded cycles in
the very hot read/write path, for example, when watcher only cares about
FAN_OPEN_EXEC_PERM.

I wasn't sure that any of this was worth the effort at this time, but
just in case
this gives you ideas of other useful optimizations we can do with the
object combined marks_mask if we get it for free.

Thanks,
Amir.

Powered by blists - more mailing lists