On Wed 20-11-24 17:42:18, Amir Goldstein wrote:
> On Wed, Nov 20, 2024 at 5:02 PM Jan Kara <jack@xxxxxxx> wrote:
> >
> > On Fri 15-11-24 10:30:16, Josef Bacik wrote:
> > > From: Amir Goldstein <amir73il@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> >
> > 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.
OK, I'm not opposed to returning the combined mask in principle. Just I'd
pick somewhat different function name and it didn't quite make sense to me
in the context of this series. If we decide to implement the optimizations
you describe above, then I have no problem with tweaking the helpers.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
