| Message ID | 2ddcc9f8d1fde48d085318a6b5a889289d8871d8.1731684329.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | fanotify: add pre-content hooks | expand |
On Fri 15-11-24 10:30:16, Josef Bacik wrote: > From: Amir Goldstein <amir73il@gmail.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@gmail.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; + } } Honza
On Wed, Nov 20, 2024 at 5:02 PM Jan Kara <jack@suse.cz> wrote: > > On Fri 15-11-24 10:30:16, Josef Bacik wrote: > > From: Amir Goldstein <amir73il@gmail.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@gmail.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.
On Wed 20-11-24 17:42:18, Amir Goldstein wrote: > On Wed, Nov 20, 2024 at 5:02 PM Jan Kara <jack@suse.cz> wrote: > > > > On Fri 15-11-24 10:30:16, Josef Bacik wrote: > > > From: Amir Goldstein <amir73il@gmail.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@gmail.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. 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
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c index f976949d2634..33576a848a9f 100644 --- a/fs/notify/fsnotify.c +++ b/fs/notify/fsnotify.c @@ -193,16 +193,38 @@ static bool fsnotify_event_needs_parent(struct inode *inode, __u32 mnt_mask, return mask & marks_mask; } -/* 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); + /* * Notify this dentry's parent about a child's events with child name info * if parent is watching or if inode/sb/mount are interested in events with @@ -221,7 +243,7 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data, struct dentry *parent; bool parent_watched = dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED; bool parent_needed, parent_interested; - __u32 p_mask; + __u32 p_mask, test_mask = mask & ALL_FSNOTIFY_EVENTS; struct inode *p_inode = NULL; struct name_snapshot name; struct qstr *file_name = NULL; @@ -229,7 +251,7 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data, /* Optimize the likely case of nobody watching this path */ if (likely(!parent_watched && - !fsnotify_object_watched(inode, mnt_mask, mask))) + !fsnotify_object_watched(inode, mnt_mask, test_mask))) return 0; parent = NULL; @@ -248,7 +270,7 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data, * Include parent/name in notification either if some notification * groups require parent info or the parent is interested in this event. */ - parent_interested = mask & p_mask & ALL_FSNOTIFY_EVENTS; + parent_interested = p_mask & test_mask; if (parent_needed || parent_interested) { /* When notifying parent, child should be passed as data */ WARN_ON_ONCE(inode != fsnotify_data_inode(data, data_type)); diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h index 3ecf7768e577..99d81c3c11d7 100644 --- a/include/linux/fsnotify_backend.h +++ b/include/linux/fsnotify_backend.h @@ -855,8 +855,15 @@ static inline void fsnotify_init_event(struct fsnotify_event *event) INIT_LIST_HEAD(&event->list); } +__u32 fsnotify_file_object_watched(struct file *file, __u32 mask); + #else +static inline __u32 fsnotify_file_object_watched(struct file *file, __u32 mask) +{ + return 0; +} + static inline int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir, const struct qstr *name, struct inode *inode, u32 cookie)