Re: [PATCH v2 4/6] fs: report per-mount io stats

On Mon, Feb 28, 2022 at 11:11 PM Dave Chinner <[email protected]> wrote:
>
> On Mon, Feb 28, 2022 at 01:39:08PM +0200, Amir Goldstein wrote:
> > Show optional collected per-mount io stats in /proc/<pid>/mountstats
> > for filesystems that do not implement their own show_stats() method
> > and opted-in to generic per-mount stats with FS_MOUNT_STATS flag.
> >
> > Signed-off-by: Amir Goldstein <[email protected]>
> > ---
> >  fs/mount.h          |  1 +
> >  fs/namespace.c      |  2 ++
> >  fs/proc_namespace.c | 13 +++++++++++++
> >  3 files changed, 16 insertions(+)
> >
> > diff --git a/fs/mount.h b/fs/mount.h
> > index f98bf4cd5b1a..2ab6308af78b 100644
> > --- a/fs/mount.h
> > +++ b/fs/mount.h
> > @@ -91,6 +91,7 @@ struct mount {
> >       int mnt_id;                     /* mount identifier */
> >       int mnt_group_id;               /* peer group identifier */
> >       int mnt_expiry_mark;            /* true if marked for expiry */
> > +     time64_t mnt_time;              /* time of mount */
> >       struct hlist_head mnt_pins;
> >       struct hlist_head mnt_stuck_children;
> >  } __randomize_layout;
> > diff --git a/fs/namespace.c b/fs/namespace.c
> > index 3fb8f11a42a1..546f07ed44c5 100644
> > --- a/fs/namespace.c
> > +++ b/fs/namespace.c
> > @@ -220,6 +220,8 @@ static struct mount *alloc_vfsmnt(const char *name)
> >               mnt->mnt_count = 1;
> >               mnt->mnt_writers = 0;
> >  #endif
> > +             /* For proc/<pid>/mountstats */
> > +             mnt->mnt_time = ktime_get_seconds();
> >
> >               INIT_HLIST_NODE(&mnt->mnt_hash);
> >               INIT_LIST_HEAD(&mnt->mnt_child);
> > diff --git a/fs/proc_namespace.c b/fs/proc_namespace.c
> > index 49650e54d2f8..d744fb8543f5 100644
> > --- a/fs/proc_namespace.c
> > +++ b/fs/proc_namespace.c
> > @@ -232,6 +232,19 @@ static int show_vfsstat(struct seq_file *m, struct vfsmount *mnt)
> >       if (sb->s_op->show_stats) {
> >               seq_putc(m, ' ');
> >               err = sb->s_op->show_stats(m, mnt_path.dentry);
> > +     } else if (mnt_has_stats(mnt)) {
> > +             /* Similar to /proc/<pid>/io */
> > +             seq_printf(m, "\n"
> > +                        "\ttimes: %lld %lld\n"
> > +                        "\trchar: %lld\n"
> > +                        "\twchar: %lld\n"
> > +                        "\tsyscr: %lld\n"
> > +                        "\tsyscw: %lld\n",
> > +                        r->mnt_time, ktime_get_seconds(),
> > +                        mnt_iostats_counter_read(r, MNTIOS_CHARS_RD),
> > +                        mnt_iostats_counter_read(r, MNTIOS_CHARS_WR),
> > +                        mnt_iostats_counter_read(r, MNTIOS_SYSCALLS_RD),
> > +                        mnt_iostats_counter_read(r, MNTIOS_SYSCALLS_WR));
>
> This doesn't scale as {cpus, mounts, counters, read frequency}
> matrix explodes.  Please iterate the per-mount per cpu counters
> once, adding up all counters in one pass to an array on stack, then
> print them all from the array.

I am planning to move to per-sb iostats and was thinking of using
an array of 4 struct percpu_counter. That will make this sort of iteration
more challenging.

Do you really think the read frequency of /proc/self/mountstats
warrants such performance optimization?

It's not like the case of the mighty struct xfsstats.
It is only going to fold 4 per cpu iterations into 1.
This doesn't look like a game changer to me.
Am I missing something?

Thanks,
Amir.