On Thu 21-11-24 17:01:13, Amir Goldstein wrote: > On Thu, Nov 21, 2024 at 12:22 PM Jan Kara <jack@xxxxxxx> 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. > > > > For pre-content events, it is possible to optimize things so that we > > don't bother trying to send pre-content events if file was not watched > > (through sb, mnt, parent or inode itself) on open. Set FMODE_NONOTIFY_ > > flags according to that. > > > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> > > Signed-off-by: Jan Kara <jack@xxxxxxx> > > Link: https://patch.msgid.link/2ddcc9f8d1fde48d085318a6b5a889289d8871d8.1731684329.git.josef@xxxxxxxxxxxxxx > > --- > > fs/notify/fsnotify.c | 27 +++++++++++++++++++++++++-- > > include/linux/fsnotify_backend.h | 3 +++ > > 2 files changed, 28 insertions(+), 2 deletions(-) > > > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > > index 569ec356e4ce..dd1dffd89fd6 100644 > > --- a/fs/notify/fsnotify.c > > +++ b/fs/notify/fsnotify.c > > @@ -193,7 +193,7 @@ 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? */ > > +/* Are there any inode/mount/sb objects that watch for these events? */ > > static inline bool fsnotify_object_watched(struct inode *inode, __u32 mnt_mask, > > __u32 mask) > > { > > @@ -632,7 +632,9 @@ EXPORT_SYMBOL_GPL(fsnotify); > > */ > > void file_set_fsnotify_mode(struct file *file) > > { > > - struct super_block *sb = file->f_path.dentry->d_sb; > > + struct dentry *dentry = file->f_path.dentry, *parent; > > + struct super_block *sb = dentry->d_sb; > > + __u32 mnt_mask, p_mask; > > > > /* Is it a file opened by fanotify? */ > > if (FMODE_FSNOTIFY_NONE(file->f_mode)) > > @@ -658,6 +660,27 @@ void file_set_fsnotify_mode(struct file *file) > > file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; > > return; > > } > > This was lost in translation: > > @@ -672,8 +672,10 @@ void file_set_fsnotify_mode(struct file *file) > /* > * If there are permission event watchers but no pre-content event > * watchers, set FMODE_NONOTIFY | FMODE_NONOTIFY_PERM to indicate that. > + * Pre-content events are only reported for regular files and dirs. > */ > - if (likely(!fsnotify_sb_has_priority_watchers(sb, > + if ((!d_is_dir(dentry) && !d_is_reg(dentry)) || > + likely(!fsnotify_sb_has_priority_watchers(sb, > FSNOTIFY_PRIO_PRE_CONTENT))) { > file->f_mode |= FMODE_NONOTIFY | FMODE_NONOTIFY_PERM; > return; Right. Specifically the (!d_is_dir(dentry) && !d_is_reg(dentry)) got lost, likely when I moved stuff between different commits and then was resolving patch conflicts. I'll add it back. Thanks for finding this! > > + > > + /* > > + * 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; > > + } > > inlining broke the logic and your branch fails the new PRE_ACCESS > test cases of fanotify03 LTP test (now pushed to branch fan_hsm). > > Specifically in the test case that fails, parent is not watching and > inode is watching pre-content and your code gets to the p_mask > test and marks this file as no-pre-content watchers. > > This passes the test: > > @@ -684,17 +686,18 @@ void file_set_fsnotify_mode(struct file *file) > * 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; > + if (unlikely(fsnotify_object_watched(d_inode(dentry), mnt_mask, > + FSNOTIFY_PRE_CONTENT_EVENTS))) { > 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); > + p_mask = 0; > + if (unlikely(dentry->d_flags & DCACHE_FSNOTIFY_PARENT_WATCHED)) { > + 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; Right. I've fixed that up. Thanks! LTP from your fan_hsm branch is now passing so I've re-pushed the current state to my fsnotify_hsm branch. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
