Re: [PATCH v3 0/6] Generic per-sb io stats

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Mar 2, 2022 at 10:27 AM Dave Chinner <david@fromorbit.com> wrote:
>
> On Wed, Mar 02, 2022 at 09:43:50AM +0200, Amir Goldstein wrote:
> > On Wed, Mar 2, 2022 at 8:59 AM Dave Chinner <david@fromorbit.com> wrote:
> > >
> > > On Tue, Mar 01, 2022 at 08:42:15PM +0200, Amir Goldstein wrote:
> > > > Miklos,
> > > >
> > > > Following your feedback on v2 [1], I moved the iostats to per-sb.
> > > >
> > > > Thanks,
> > > > Amir.
> > > >
> > > > [1] https://lore.kernel.org/linux-unionfs/20220228113910.1727819-1-amir73il@gmail.com/
> > > >
> > > > Changes since v2:
> > > > - Change from per-mount to per-sb io stats (szeredi)
> > > > - Avoid percpu loop when reading mountstats (dchinner)
> > > >
> > > > Changes since v1:
> > > > - Opt-in for per-mount io stats for overlayfs and fuse
> > >
> > > Why make it optional only for specific filesystem types? Shouldn't
> > > every superblock capture these stats and export them in exactly the
> > > same place?
> > >
> >
> > I am not sure what you are asking.
> >
> > Any filesystem can opt-in to get those generic io stats.
>
> Yes, but why even make it opt-in? Why not just set these up
> unconditionally in alloc_super() for all filesystems? Either this is
> useful information for userspace montioring and diagnostics, or it's
> not useful at all. If it is useful, then all superblocks should
> export this stuff rather than just some special subset of
> filesystems where individual maintainers have noticed it and thought
> "that might be useful".
>
> Just enable it for every superblock....
>
> > This is exactly the same as any filesystem can already opt-in for
> > fs specific io stats using the s_op->show_stats() vfs op.
> >
> > All I did was to provide a generic implementation.
> > The generic io stats are collected and displayed for all filesystems the
> > same way.
> >
> > I only included patches for overlayfs and fuse to opt-in for generic io stats,
> > because I think those stats should be reported unconditionally (to
> > mount options)
> > for fuse/overlayfs and I hope that Miklos agrees with me.
>
> Yup, and I'm asking you why it should be optional - no filesystem
> ever sees this information - it's totally generic VFS level code
> except for the structure allocation. What's the point of *not*
> enabling it for every superblock unconditionally?
>
> > If there is wide consensus that all filesystems should have those stats
> > unconditionally (to mount options), then I can post another patch to make
> > the behavior not opt-in, but I have a feeling that this discussion
>
> That's exactly what I want you to do. We're already having this
> discussion, so let's get it over and done with right now.
>
> > How would you prefer the io stats behavior for xfs (or any fs) to be?
> > Unconditional to mount options?
> > Opt-in with mount option? (suggest name please)
> > Opt-in/out with mount options and default with Kconfig/sysfs tunable?
> > Anything else?
>
> Unconditional, for all filesystems, so they all display the same
> stats in exactly same place without any filesystem having to
> implement a single line of code anywhere. A single kconfig option
> like you already hav is just fine to turn it off for those that
> don't want to use it.
>

Very well then. I'll post this version with your Suggested-by ;-)

Thanks,
Amir.