| Message ID | 20210810151220.285179-4-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Performance optimization for no fsnotify marks | expand |
On Tue, Aug 10, 2021 at 06:12:19PM +0300, Amir Goldstein wrote: > Rename s_fsnotify_inode_refs to s_fsnotify_connectors and count all > objects with attached connectors, not only inodes with attached > connectors. > > This will be used to optimize fsnotify() calls on sb without any > type of marks. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> LGTM. Reviewed-by: Matthew Bobrowski <repnop@google.com> > --- > fs/notify/fsnotify.c | 6 +++--- > fs/notify/fsnotify.h | 15 +++++++++++++++ > fs/notify/mark.c | 24 +++++++++++++++++++++--- > include/linux/fs.h | 4 ++-- > 4 files changed, 41 insertions(+), 8 deletions(-) > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > index 30d422b8c0fc..963e6ce75b96 100644 > --- a/fs/notify/fsnotify.c > +++ b/fs/notify/fsnotify.c > @@ -87,15 +87,15 @@ static void fsnotify_unmount_inodes(struct super_block *sb) > > if (iput_inode) > iput(iput_inode); > - /* Wait for outstanding inode references from connectors */ > - wait_var_event(&sb->s_fsnotify_inode_refs, > - !atomic_long_read(&sb->s_fsnotify_inode_refs)); > } > > void fsnotify_sb_delete(struct super_block *sb) > { > fsnotify_unmount_inodes(sb); > fsnotify_clear_marks_by_sb(sb); > + /* Wait for outstanding object references from connectors */ > + wait_var_event(&sb->s_fsnotify_connectors, > + !atomic_long_read(&sb->s_fsnotify_connectors)); > } > > /* > diff --git a/fs/notify/fsnotify.h b/fs/notify/fsnotify.h > index ff2063ec6b0f..87d8a50ee803 100644 > --- a/fs/notify/fsnotify.h > +++ b/fs/notify/fsnotify.h > @@ -27,6 +27,21 @@ static inline struct super_block *fsnotify_conn_sb( > return container_of(conn->obj, struct super_block, s_fsnotify_marks); > } > > +static inline struct super_block *fsnotify_connector_sb( > + struct fsnotify_mark_connector *conn) > +{ > + switch (conn->type) { > + case FSNOTIFY_OBJ_TYPE_INODE: > + return fsnotify_conn_inode(conn)->i_sb; > + case FSNOTIFY_OBJ_TYPE_VFSMOUNT: > + return fsnotify_conn_mount(conn)->mnt.mnt_sb; > + case FSNOTIFY_OBJ_TYPE_SB: > + return fsnotify_conn_sb(conn); > + default: > + return NULL; > + } > +} > + > /* destroy all events sitting in this groups notification queue */ > extern void fsnotify_flush_notify(struct fsnotify_group *group); > > diff --git a/fs/notify/mark.c b/fs/notify/mark.c > index 2d8c46e1167d..95006d1d29ab 100644 > --- a/fs/notify/mark.c > +++ b/fs/notify/mark.c > @@ -172,7 +172,7 @@ static void fsnotify_connector_destroy_workfn(struct work_struct *work) > static void fsnotify_get_inode_ref(struct inode *inode) > { > ihold(inode); > - atomic_long_inc(&inode->i_sb->s_fsnotify_inode_refs); > + atomic_long_inc(&inode->i_sb->s_fsnotify_connectors); > } > > static void fsnotify_put_inode_ref(struct inode *inode) > @@ -180,8 +180,24 @@ static void fsnotify_put_inode_ref(struct inode *inode) > struct super_block *sb = inode->i_sb; > > iput(inode); > - if (atomic_long_dec_and_test(&sb->s_fsnotify_inode_refs)) > - wake_up_var(&sb->s_fsnotify_inode_refs); > + if (atomic_long_dec_and_test(&sb->s_fsnotify_connectors)) > + wake_up_var(&sb->s_fsnotify_connectors); > +} > + > +static void fsnotify_get_sb_connectors(struct fsnotify_mark_connector *conn) > +{ > + struct super_block *sb = fsnotify_connector_sb(conn); > + > + if (sb) > + atomic_long_inc(&sb->s_fsnotify_connectors); > +} > + > +static void fsnotify_put_sb_connectors(struct fsnotify_mark_connector *conn) > +{ > + struct super_block *sb = fsnotify_connector_sb(conn); > + > + if (sb && atomic_long_dec_and_test(&sb->s_fsnotify_connectors)) > + wake_up_var(&sb->s_fsnotify_connectors); > } > > static void *fsnotify_detach_connector_from_object( > @@ -203,6 +219,7 @@ static void *fsnotify_detach_connector_from_object( > fsnotify_conn_sb(conn)->s_fsnotify_mask = 0; > } > > + fsnotify_put_sb_connectors(conn); > rcu_assign_pointer(*(conn->obj), NULL); > conn->obj = NULL; > conn->type = FSNOTIFY_OBJ_TYPE_DETACHED; > @@ -504,6 +521,7 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp, > inode = fsnotify_conn_inode(conn); > fsnotify_get_inode_ref(inode); > } > + fsnotify_get_sb_connectors(conn); > > /* > * cmpxchg() provides the barrier so that readers of *connp can see > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 640574294216..d48d2018dfa4 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1507,8 +1507,8 @@ struct super_block { > /* Number of inodes with nlink == 0 but still referenced */ > atomic_long_t s_remove_count; > > - /* Pending fsnotify inode refs */ > - atomic_long_t s_fsnotify_inode_refs; > + /* Number of inode/mount/sb objects that are being watched */ > + atomic_long_t s_fsnotify_connectors; > > /* Being remounted read-only */ > int s_readonly_remount; > -- > 2.32.0 > /M
On Tue 10-08-21 18:12:19, Amir Goldstein wrote: > Rename s_fsnotify_inode_refs to s_fsnotify_connectors and count all > objects with attached connectors, not only inodes with attached > connectors. > > This will be used to optimize fsnotify() calls on sb without any > type of marks. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> ... just a minor nit below ... > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 640574294216..d48d2018dfa4 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1507,8 +1507,8 @@ struct super_block { > /* Number of inodes with nlink == 0 but still referenced */ > atomic_long_t s_remove_count; > > - /* Pending fsnotify inode refs */ > - atomic_long_t s_fsnotify_inode_refs; > + /* Number of inode/mount/sb objects that are being watched */ > + atomic_long_t s_fsnotify_connectors; I've realized inode watches will be double-accounted because we increment s_fsnotify_connectors both when attaching a connector and when grabbing inode reference. It doesn't really matter and avoiding this would require special treatment of inode connectors (we need to decrement s_fsnotify_connectors only after dropping inode reference). So I'll just reflect this in the comment here so that we don't forget. Honza
On Wed, Aug 11, 2021 at 2:37 PM Jan Kara <jack@suse.cz> wrote: > > On Tue 10-08-21 18:12:19, Amir Goldstein wrote: > > Rename s_fsnotify_inode_refs to s_fsnotify_connectors and count all > > objects with attached connectors, not only inodes with attached > > connectors. > > > > This will be used to optimize fsnotify() calls on sb without any > > type of marks. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > ... just a minor nit below ... > > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > index 640574294216..d48d2018dfa4 100644 > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -1507,8 +1507,8 @@ struct super_block { > > /* Number of inodes with nlink == 0 but still referenced */ > > atomic_long_t s_remove_count; > > > > - /* Pending fsnotify inode refs */ > > - atomic_long_t s_fsnotify_inode_refs; > > + /* Number of inode/mount/sb objects that are being watched */ > > + atomic_long_t s_fsnotify_connectors; > > I've realized inode watches will be double-accounted because we increment > s_fsnotify_connectors both when attaching a connector and when grabbing > inode reference. It doesn't really matter and avoiding this would require > special treatment of inode connectors (we need to decrement > s_fsnotify_connectors only after dropping inode reference). So I'll just > reflect this in the comment here so that we don't forget. Great! I actually have a patch to make the inode reference optional per backend/mark [1], so keeping double account makes things a lot easier for that code as well. Thanks, Amir. [1] https://github.com/amir73il/linux/commits/fsnotify-volatile
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c index 30d422b8c0fc..963e6ce75b96 100644 --- a/fs/notify/fsnotify.c +++ b/fs/notify/fsnotify.c @@ -87,15 +87,15 @@ static void fsnotify_unmount_inodes(struct super_block *sb) if (iput_inode) iput(iput_inode); - /* Wait for outstanding inode references from connectors */ - wait_var_event(&sb->s_fsnotify_inode_refs, - !atomic_long_read(&sb->s_fsnotify_inode_refs)); } void fsnotify_sb_delete(struct super_block *sb) { fsnotify_unmount_inodes(sb); fsnotify_clear_marks_by_sb(sb); + /* Wait for outstanding object references from connectors */ + wait_var_event(&sb->s_fsnotify_connectors, + !atomic_long_read(&sb->s_fsnotify_connectors)); } /* diff --git a/fs/notify/fsnotify.h b/fs/notify/fsnotify.h index ff2063ec6b0f..87d8a50ee803 100644 --- a/fs/notify/fsnotify.h +++ b/fs/notify/fsnotify.h @@ -27,6 +27,21 @@ static inline struct super_block *fsnotify_conn_sb( return container_of(conn->obj, struct super_block, s_fsnotify_marks); } +static inline struct super_block *fsnotify_connector_sb( + struct fsnotify_mark_connector *conn) +{ + switch (conn->type) { + case FSNOTIFY_OBJ_TYPE_INODE: + return fsnotify_conn_inode(conn)->i_sb; + case FSNOTIFY_OBJ_TYPE_VFSMOUNT: + return fsnotify_conn_mount(conn)->mnt.mnt_sb; + case FSNOTIFY_OBJ_TYPE_SB: + return fsnotify_conn_sb(conn); + default: + return NULL; + } +} + /* destroy all events sitting in this groups notification queue */ extern void fsnotify_flush_notify(struct fsnotify_group *group); diff --git a/fs/notify/mark.c b/fs/notify/mark.c index 2d8c46e1167d..95006d1d29ab 100644 --- a/fs/notify/mark.c +++ b/fs/notify/mark.c @@ -172,7 +172,7 @@ static void fsnotify_connector_destroy_workfn(struct work_struct *work) static void fsnotify_get_inode_ref(struct inode *inode) { ihold(inode); - atomic_long_inc(&inode->i_sb->s_fsnotify_inode_refs); + atomic_long_inc(&inode->i_sb->s_fsnotify_connectors); } static void fsnotify_put_inode_ref(struct inode *inode) @@ -180,8 +180,24 @@ static void fsnotify_put_inode_ref(struct inode *inode) struct super_block *sb = inode->i_sb; iput(inode); - if (atomic_long_dec_and_test(&sb->s_fsnotify_inode_refs)) - wake_up_var(&sb->s_fsnotify_inode_refs); + if (atomic_long_dec_and_test(&sb->s_fsnotify_connectors)) + wake_up_var(&sb->s_fsnotify_connectors); +} + +static void fsnotify_get_sb_connectors(struct fsnotify_mark_connector *conn) +{ + struct super_block *sb = fsnotify_connector_sb(conn); + + if (sb) + atomic_long_inc(&sb->s_fsnotify_connectors); +} + +static void fsnotify_put_sb_connectors(struct fsnotify_mark_connector *conn) +{ + struct super_block *sb = fsnotify_connector_sb(conn); + + if (sb && atomic_long_dec_and_test(&sb->s_fsnotify_connectors)) + wake_up_var(&sb->s_fsnotify_connectors); } static void *fsnotify_detach_connector_from_object( @@ -203,6 +219,7 @@ static void *fsnotify_detach_connector_from_object( fsnotify_conn_sb(conn)->s_fsnotify_mask = 0; } + fsnotify_put_sb_connectors(conn); rcu_assign_pointer(*(conn->obj), NULL); conn->obj = NULL; conn->type = FSNOTIFY_OBJ_TYPE_DETACHED; @@ -504,6 +521,7 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp, inode = fsnotify_conn_inode(conn); fsnotify_get_inode_ref(inode); } + fsnotify_get_sb_connectors(conn); /* * cmpxchg() provides the barrier so that readers of *connp can see diff --git a/include/linux/fs.h b/include/linux/fs.h index 640574294216..d48d2018dfa4 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1507,8 +1507,8 @@ struct super_block { /* Number of inodes with nlink == 0 but still referenced */ atomic_long_t s_remove_count; - /* Pending fsnotify inode refs */ - atomic_long_t s_fsnotify_inode_refs; + /* Number of inode/mount/sb objects that are being watched */ + atomic_long_t s_fsnotify_connectors; /* Being remounted read-only */ int s_readonly_remount;
Rename s_fsnotify_inode_refs to s_fsnotify_connectors and count all objects with attached connectors, not only inodes with attached connectors. This will be used to optimize fsnotify() calls on sb without any type of marks. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/notify/fsnotify.c | 6 +++--- fs/notify/fsnotify.h | 15 +++++++++++++++ fs/notify/mark.c | 24 +++++++++++++++++++++--- include/linux/fs.h | 4 ++-- 4 files changed, 41 insertions(+), 8 deletions(-)