On Tue, Aug 10, 2021 at 9:32 AM Matthew Bobrowski <repnop@xxxxxxxxxx> 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@xxxxxxxxx>
>
> 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.
