| Message ID | 20210803180344.2398374-4-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Performance optimization for no fsnotify marks | expand |
On Tue, Aug 03, 2021 at 09:03:43PM +0300, Amir Goldstein wrote: > Rename s_fsnotify_inode_refs to s_fsnotify_conectors and count all s/s_fsnotify_conectors/s_fsnotify_connectors ;) > 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> Have one question below. > --- > fs/notify/fsnotify.c | 6 +++--- > fs/notify/mark.c | 45 +++++++++++++++++++++++++++++++++++++++++--- > include/linux/fs.h | 4 ++-- > 3 files changed, 47 insertions(+), 8 deletions(-) > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > index 30d422b8c0fc..a5de7f32c493 100644 > --- a/fs/notify/fsnotify.c > +++ b/fs/notify/fsnotify.c > @@ -87,9 +87,9 @@ 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)); > + /* Wait for outstanding object references from connectors */ > + wait_var_event(&sb->s_fsnotify_connectors, > + !atomic_long_read(&sb->s_fsnotify_connectors)); > } > > void fsnotify_sb_delete(struct super_block *sb) > diff --git a/fs/notify/mark.c b/fs/notify/mark.c > index 2d8c46e1167d..622bcbface4f 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,45 @@ 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; > + > + if (conn->type == FSNOTIFY_OBJ_TYPE_DETACHED) > + return; > + > + if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) > + sb = fsnotify_conn_inode(conn)->i_sb; > + else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) > + sb = fsnotify_conn_mount(conn)->mnt.mnt_sb; > + else if (conn->type == FSNOTIFY_OBJ_TYPE_SB) > + sb = fsnotify_conn_sb(conn); I noticed that you haven't provided an explicit case when no conditions are matched, however this scenario appears to be handled in fsnotify_put_sb_connectors() below. Why is this the case here and not in fsnotify_put_sb_connectors()? Also, I'm wondering if these blocks of code would be better expressed in a switch statement. Alternatively, if these conditionals are shared across the two helpers, why not factor out the super_block retrieval into an inline helper just to simplify the callsite and not duplicate code? That's of course if there is commonality between the two helpers. > + > + atomic_long_inc(&sb->s_fsnotify_connectors); > +} > + > +static void fsnotify_put_sb_connectors(struct fsnotify_mark_connector *conn) > +{ > + struct super_block *sb; > + > + if (conn->type == FSNOTIFY_OBJ_TYPE_DETACHED) > + return; > + > + if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) > + sb = fsnotify_conn_inode(conn)->i_sb; > + else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) > + sb = fsnotify_conn_mount(conn)->mnt.mnt_sb; > + else if (conn->type == FSNOTIFY_OBJ_TYPE_SB) > + sb = fsnotify_conn_sb(conn); > + else > + return; > + > + if (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 +240,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 +542,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.25.1 /M
On Tue 03-08-21 21:03:43, Amir Goldstein wrote: > Rename s_fsnotify_inode_refs to s_fsnotify_conectors 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/mark.c | 45 +++++++++++++++++++++++++++++++++++++++++--- > include/linux/fs.h | 4 ++-- > 3 files changed, 47 insertions(+), 8 deletions(-) > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > index 30d422b8c0fc..a5de7f32c493 100644 > --- a/fs/notify/fsnotify.c > +++ b/fs/notify/fsnotify.c > @@ -87,9 +87,9 @@ 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)); > + /* Wait for outstanding object references from connectors */ > + wait_var_event(&sb->s_fsnotify_connectors, > + !atomic_long_read(&sb->s_fsnotify_connectors)); > } I think this is wrong and will deadlock unmount if there's pending sb mark because s_fsnotify_connectors won't drop to 0. I think you need to move this wait to fsnotify_sb_delete() after fsnotify_clear_marks_by_sb(). Otherwise the patch looks good to me. Honza > > void fsnotify_sb_delete(struct super_block *sb) > diff --git a/fs/notify/mark.c b/fs/notify/mark.c > index 2d8c46e1167d..622bcbface4f 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,45 @@ 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; > + > + if (conn->type == FSNOTIFY_OBJ_TYPE_DETACHED) > + return; > + > + if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) > + sb = fsnotify_conn_inode(conn)->i_sb; > + else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) > + sb = fsnotify_conn_mount(conn)->mnt.mnt_sb; > + else if (conn->type == FSNOTIFY_OBJ_TYPE_SB) > + sb = fsnotify_conn_sb(conn); > + > + atomic_long_inc(&sb->s_fsnotify_connectors); > +} > + > +static void fsnotify_put_sb_connectors(struct fsnotify_mark_connector *conn) > +{ > + struct super_block *sb; > + > + if (conn->type == FSNOTIFY_OBJ_TYPE_DETACHED) > + return; > + > + if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) > + sb = fsnotify_conn_inode(conn)->i_sb; > + else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) > + sb = fsnotify_conn_mount(conn)->mnt.mnt_sb; > + else if (conn->type == FSNOTIFY_OBJ_TYPE_SB) > + sb = fsnotify_conn_sb(conn); > + else > + return; > + > + if (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 +240,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 +542,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.25.1 >
On Tue, Aug 10, 2021 at 9:32 AM Matthew Bobrowski <repnop@google.com> wrote: > > On Tue, Aug 03, 2021 at 09:03:43PM +0300, Amir Goldstein wrote: > > Rename s_fsnotify_inode_refs to s_fsnotify_conectors and count all > > s/s_fsnotify_conectors/s_fsnotify_connectors ;) > > > 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> > > Have one question below. > > > --- > > fs/notify/fsnotify.c | 6 +++--- > > fs/notify/mark.c | 45 +++++++++++++++++++++++++++++++++++++++++--- > > include/linux/fs.h | 4 ++-- > > 3 files changed, 47 insertions(+), 8 deletions(-) > > > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > > index 30d422b8c0fc..a5de7f32c493 100644 > > --- a/fs/notify/fsnotify.c > > +++ b/fs/notify/fsnotify.c > > @@ -87,9 +87,9 @@ 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)); > > + /* Wait for outstanding object references from connectors */ > > + wait_var_event(&sb->s_fsnotify_connectors, > > + !atomic_long_read(&sb->s_fsnotify_connectors)); > > } > > > > void fsnotify_sb_delete(struct super_block *sb) > > diff --git a/fs/notify/mark.c b/fs/notify/mark.c > > index 2d8c46e1167d..622bcbface4f 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,45 @@ 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; > > + > > + if (conn->type == FSNOTIFY_OBJ_TYPE_DETACHED) > > + return; > > + > > + if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) > > + sb = fsnotify_conn_inode(conn)->i_sb; > > + else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) > > + sb = fsnotify_conn_mount(conn)->mnt.mnt_sb; > > + else if (conn->type == FSNOTIFY_OBJ_TYPE_SB) > > + sb = fsnotify_conn_sb(conn); > > I noticed that you haven't provided an explicit case when no conditions are > matched, however this scenario appears to be handled in > fsnotify_put_sb_connectors() below. Why is this the case here and not in > fsnotify_put_sb_connectors()? No reason. I fixed a warning reported by a static checker here and didn't notice the other one. > > Also, I'm wondering if these blocks of code would be better expressed in a > switch statement. Alternatively, if these conditionals are shared across > the two helpers, why not factor out the super_block retrieval into an > inline helper just to simplify the callsite and not duplicate code? That's > of course if there is commonality between the two helpers. > Makes sense. Will do. Thanks, Amir.
On Tue, Aug 10, 2021 at 1:47 PM Jan Kara <jack@suse.cz> wrote: > > On Tue 03-08-21 21:03:43, Amir Goldstein wrote: > > Rename s_fsnotify_inode_refs to s_fsnotify_conectors 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/mark.c | 45 +++++++++++++++++++++++++++++++++++++++++--- > > include/linux/fs.h | 4 ++-- > > 3 files changed, 47 insertions(+), 8 deletions(-) > > > > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c > > index 30d422b8c0fc..a5de7f32c493 100644 > > --- a/fs/notify/fsnotify.c > > +++ b/fs/notify/fsnotify.c > > @@ -87,9 +87,9 @@ 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)); > > + /* Wait for outstanding object references from connectors */ > > + wait_var_event(&sb->s_fsnotify_connectors, > > + !atomic_long_read(&sb->s_fsnotify_connectors)); > > } > > I think this is wrong and will deadlock unmount if there's pending sb mark > because s_fsnotify_connectors won't drop to 0. I think you need to move > this wait to fsnotify_sb_delete() after fsnotify_clear_marks_by_sb(). Oops. Thanks for catching this. Amir.
diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c index 30d422b8c0fc..a5de7f32c493 100644 --- a/fs/notify/fsnotify.c +++ b/fs/notify/fsnotify.c @@ -87,9 +87,9 @@ 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)); + /* Wait for outstanding object references from connectors */ + wait_var_event(&sb->s_fsnotify_connectors, + !atomic_long_read(&sb->s_fsnotify_connectors)); } void fsnotify_sb_delete(struct super_block *sb) diff --git a/fs/notify/mark.c b/fs/notify/mark.c index 2d8c46e1167d..622bcbface4f 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,45 @@ 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; + + if (conn->type == FSNOTIFY_OBJ_TYPE_DETACHED) + return; + + if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) + sb = fsnotify_conn_inode(conn)->i_sb; + else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) + sb = fsnotify_conn_mount(conn)->mnt.mnt_sb; + else if (conn->type == FSNOTIFY_OBJ_TYPE_SB) + sb = fsnotify_conn_sb(conn); + + atomic_long_inc(&sb->s_fsnotify_connectors); +} + +static void fsnotify_put_sb_connectors(struct fsnotify_mark_connector *conn) +{ + struct super_block *sb; + + if (conn->type == FSNOTIFY_OBJ_TYPE_DETACHED) + return; + + if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) + sb = fsnotify_conn_inode(conn)->i_sb; + else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) + sb = fsnotify_conn_mount(conn)->mnt.mnt_sb; + else if (conn->type == FSNOTIFY_OBJ_TYPE_SB) + sb = fsnotify_conn_sb(conn); + else + return; + + if (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 +240,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 +542,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_conectors 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/mark.c | 45 +++++++++++++++++++++++++++++++++++++++++--- include/linux/fs.h | 4 ++-- 3 files changed, 47 insertions(+), 8 deletions(-)