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@xxxxxxxxxxxxx> 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@xxxxxxxxx/ > > > > > > 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. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
