| Message ID | 20181003212539.2384-9-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | New fanotify event info API | expand |
On Thu 04-10-18 00:25:39, Amir Goldstein wrote: > inotify and dnotify are always interested in events on directories. > fanotify is interested in events on directories only if user requested > them by flag FAN_ONDIR. > > For the fanotify supported events on directories (open/access/close), > if there is no inotify/dnotify associated marks and if none of the > associated fanotify marks have the FS_ISDIR flag, we can skip iterating > the marks and we avoid taking the srcu_read_lock() for all events on > directories. > > In order to enable this check in fsnotify() we set the FS_ISDIR flag > explicitly on all dnotify/inotify marks. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/notify/dnotify/dnotify.c | 5 +++-- > fs/notify/fsnotify.c | 9 ++++++++- > fs/notify/inotify/inotify_user.c | 4 ++-- > 3 files changed, 13 insertions(+), 5 deletions(-) Looks good, except I think you forgot to convert kernel/audit_watch.c and kernel/audit_fsnotify.c which also need to set FS_ISDIR... Honza
On Thu, Oct 4, 2018 at 12:09 PM Jan Kara <jack@suse.cz> wrote: > > On Thu 04-10-18 00:25:39, Amir Goldstein wrote: > > inotify and dnotify are always interested in events on directories. > > fanotify is interested in events on directories only if user requested > > them by flag FAN_ONDIR. > > > > For the fanotify supported events on directories (open/access/close), > > if there is no inotify/dnotify associated marks and if none of the > > associated fanotify marks have the FS_ISDIR flag, we can skip iterating > > the marks and we avoid taking the srcu_read_lock() for all events on > > directories. > > > > In order to enable this check in fsnotify() we set the FS_ISDIR flag > > explicitly on all dnotify/inotify marks. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > fs/notify/dnotify/dnotify.c | 5 +++-- > > fs/notify/fsnotify.c | 9 ++++++++- > > fs/notify/inotify/inotify_user.c | 4 ++-- > > 3 files changed, 13 insertions(+), 5 deletions(-) > > Looks good, except I think you forgot to convert kernel/audit_watch.c and > kernel/audit_fsnotify.c which also need to set FS_ISDIR... > Oops. Right. So would you rather I come back with a fixed version after I ran some performance tests? Or will you fix and test it yourself? Thanks, Amir.
On Thu 04-10-18 13:30:38, Amir Goldstein wrote: > On Thu, Oct 4, 2018 at 12:09 PM Jan Kara <jack@suse.cz> wrote: > > > > On Thu 04-10-18 00:25:39, Amir Goldstein wrote: > > > inotify and dnotify are always interested in events on directories. > > > fanotify is interested in events on directories only if user requested > > > them by flag FAN_ONDIR. > > > > > > For the fanotify supported events on directories (open/access/close), > > > if there is no inotify/dnotify associated marks and if none of the > > > associated fanotify marks have the FS_ISDIR flag, we can skip iterating > > > the marks and we avoid taking the srcu_read_lock() for all events on > > > directories. > > > > > > In order to enable this check in fsnotify() we set the FS_ISDIR flag > > > explicitly on all dnotify/inotify marks. > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > fs/notify/dnotify/dnotify.c | 5 +++-- > > > fs/notify/fsnotify.c | 9 ++++++++- > > > fs/notify/inotify/inotify_user.c | 4 ++-- > > > 3 files changed, 13 insertions(+), 5 deletions(-) > > > > Looks good, except I think you forgot to convert kernel/audit_watch.c and > > kernel/audit_fsnotify.c which also need to set FS_ISDIR... > > > > Oops. Right. > So would you rather I come back with a fixed version after I ran > some performance tests? > Or will you fix and test it yourself? Let's postpone this patch for a bit later. I'll merge the remaining seven patches to my tree for this merge window. Honza
On Thu, Oct 4, 2018 at 2:26 PM Jan Kara <jack@suse.cz> wrote: > > On Thu 04-10-18 13:30:38, Amir Goldstein wrote: > > On Thu, Oct 4, 2018 at 12:09 PM Jan Kara <jack@suse.cz> wrote: > > > > > > On Thu 04-10-18 00:25:39, Amir Goldstein wrote: > > > > inotify and dnotify are always interested in events on directories. > > > > fanotify is interested in events on directories only if user requested > > > > them by flag FAN_ONDIR. > > > > > > > > For the fanotify supported events on directories (open/access/close), > > > > if there is no inotify/dnotify associated marks and if none of the > > > > associated fanotify marks have the FS_ISDIR flag, we can skip iterating > > > > the marks and we avoid taking the srcu_read_lock() for all events on > > > > directories. > > > > > > > > In order to enable this check in fsnotify() we set the FS_ISDIR flag > > > > explicitly on all dnotify/inotify marks. > > > > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > --- > > > > fs/notify/dnotify/dnotify.c | 5 +++-- > > > > fs/notify/fsnotify.c | 9 ++++++++- > > > > fs/notify/inotify/inotify_user.c | 4 ++-- > > > > 3 files changed, 13 insertions(+), 5 deletions(-) > > > > > > Looks good, except I think you forgot to convert kernel/audit_watch.c and > > > kernel/audit_fsnotify.c which also need to set FS_ISDIR... > > > > > > > Oops. Right. > > So would you rather I come back with a fixed version after I ran > > some performance tests? > > Or will you fix and test it yourself? > > Let's postpone this patch for a bit later. I'll merge the remaining seven > patches to my tree for this merge window. > For the records, On my small test system, I wasn't able to measure a difference in performance of parallel opendir/readdir/closedir workload (filebench listdirs) with and without a mount mark regardless of the optimization patch. Benchmark reports 40K~42K ops/sec for the compound op of "listdir". Perhaps more complex workloads that actually have SRCU sleeps would benefit from this optimization, but it doesn't look like avoiding the memory barrier alone moves the niddle. Not very surprising considering that readdir takes a shared lock anyway. So dropping the patch until further notice. Thanks, Amir.
diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c index 58d77dc696eb..55f0ae5fe0ba 100644 --- a/fs/notify/dnotify/dnotify.c +++ b/fs/notify/dnotify/dnotify.c @@ -89,7 +89,7 @@ static int dnotify_handle_event(struct fsnotify_group *group, struct dnotify_struct *dn; struct dnotify_struct **prev; struct fown_struct *fown; - __u32 test_mask = mask & ~FS_EVENT_ON_CHILD; + __u32 test_mask = mask & ALL_FSNOTIFY_EVENTS; /* not a dir, dnotify doesn't care */ if (!S_ISDIR(inode->i_mode)) @@ -197,7 +197,8 @@ void dnotify_flush(struct file *filp, fl_owner_t id) /* this conversion is done only at watch creation */ static __u32 convert_arg(unsigned long arg) { - __u32 new_mask = FS_EVENT_ON_CHILD; + /* Interested in events on children including subdirs */ + __u32 new_mask = FS_EVENT_ON_CHILD | FS_ISDIR; if (arg & DN_MULTISHOT) new_mask |= FS_DN_MULTISHOT; diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c index 875975504409..a0ad06285450 100644 --- a/fs/notify/fsnotify.c +++ b/fs/notify/fsnotify.c @@ -332,9 +332,16 @@ int fsnotify(struct inode *to_tell, __u32 mask, const void *data, int data_is, __u32 test_mask = (mask & ALL_FSNOTIFY_EVENTS); if (data_is == FSNOTIFY_EVENT_PATH) { - mnt = real_mount(((const struct path *)data)->mnt); + const struct path *path = data; + + mnt = real_mount(path->mnt); sb = mnt->mnt.mnt_sb; mnt_or_sb_mask = mnt->mnt_fsnotify_mask | sb->s_fsnotify_mask; + + /* Is anybody is interested in events on directories? */ + if (d_is_dir(path->dentry) && + !(FS_ISDIR & (to_tell->i_fsnotify_mask | mnt_or_sb_mask))) + return 0; } /* diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c index 105576daca4a..8d163b96a86a 100644 --- a/fs/notify/inotify/inotify_user.c +++ b/fs/notify/inotify/inotify_user.c @@ -91,9 +91,9 @@ static inline __u32 inotify_arg_to_mask(u32 arg) /* * everything should accept their own ignored, cares about children, - * and should receive events when the inode is unmounted + * directories, and should receive events when the inode is unmounted. */ - mask = (FS_IN_IGNORED | FS_EVENT_ON_CHILD | FS_UNMOUNT); + mask = (FS_IN_IGNORED | FS_EVENT_ON_CHILD | FS_ISDIR | FS_UNMOUNT); /* mask off the flags used to open the fd */ mask |= (arg & (IN_ALL_EVENTS | IN_ONESHOT | IN_EXCL_UNLINK));
inotify and dnotify are always interested in events on directories. fanotify is interested in events on directories only if user requested them by flag FAN_ONDIR. For the fanotify supported events on directories (open/access/close), if there is no inotify/dnotify associated marks and if none of the associated fanotify marks have the FS_ISDIR flag, we can skip iterating the marks and we avoid taking the srcu_read_lock() for all events on directories. In order to enable this check in fsnotify() we set the FS_ISDIR flag explicitly on all dnotify/inotify marks. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/notify/dnotify/dnotify.c | 5 +++-- fs/notify/fsnotify.c | 9 ++++++++- fs/notify/inotify/inotify_user.c | 4 ++-- 3 files changed, 13 insertions(+), 5 deletions(-)