[PATCH v4 0/9] Generic per-sb io stats

[PATCH v4 0/9] Generic per-sb io stats

[Amir Goldstein]

Miklos,

I ran some micro benchmarks on v3 patch [1] which demonstrated up to
20% slowdown for some workloads (many small reads/writes in a small VM).
This revision adds the "relaxed" percpu counter helpers to mitigate
the iostats counters overhead.

With the relaxed counters, the micro benchmarks that I ran did not
demonstrate any measurable overhead on xfs, on overlayfs over xfs
and overlayfs over tmpfs.

Dave Chinner asked why the io stats should not be enabled for all
filesystems.  That change seems too bold for me so instead, I included
an extra patch to auto-enable per-sb io stats for blockdev filesystems.

Should you decide to take the patches for enabling io stats for
overlayfs and/or fuse through your tree, it is up to you to whether you
want to take this patch as well or leave it out until more people have
a chance to test it and run more performance tests on their setups.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/20220301184221.371853-1-amir73il@gmail.com/

Changes since v3:
- Use "relaxed" counters to reduce performance overhead
- Opt-in to per-sb io stats via fs_flags (dchinner)
- Add patch to auto-enable io stats for all blockdev fs (dchinner)

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

Amir Goldstein (9):
  lib/percpu_counter: add helpers for "relaxed" counters
  lib/percpu_counter: add helpers for arrays of counters
  fs: tidy up fs_flags definitions
  fs: add optional iostats counters to struct super_block
  fs: collect per-sb io stats
  fs: report per-sb io stats
  ovl: opt-in for per-sb io stats
  fuse: opt-in for per-sb io stats
  fs: enable per-sb io stats for all blockdev filesystems

 fs/Kconfig                     |   8 ++
 fs/fuse/inode.c                |   3 +-
 fs/nfsd/export.c               |  10 ++-
 fs/nfsd/nfscache.c             |   5 +-
 fs/nfsd/stats.c                |  37 +---------
 fs/nfsd/stats.h                |   3 -
 fs/overlayfs/super.c           |   3 +-
 fs/proc_namespace.c            |  16 ++++
 fs/read_write.c                |  88 ++++++++++++++++------
 fs/super.c                     |  11 +++
 include/linux/fs.h             |  25 ++++---
 include/linux/fs_iostats.h     | 130 +++++++++++++++++++++++++++++++++
 include/linux/percpu_counter.h |  48 ++++++++++++
 lib/percpu_counter.c           |  27 +++++++
 14 files changed, 337 insertions(+), 77 deletions(-)
 create mode 100644 include/linux/fs_iostats.h

-- 
2.25.1

[PATCH v4 1/9] lib/percpu_counter: add helpers for "relaxed" counters

[Amir Goldstein]

Counter that are only read with percpu_counter_sum() can use an
arbitrary large batch size.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 include/linux/percpu_counter.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h
index 01861eebed79..7f01f2e41304 100644
--- a/include/linux/percpu_counter.h
+++ b/include/linux/percpu_counter.h
@@ -193,4 +193,33 @@ static inline void percpu_counter_sub(struct percpu_counter *fbc, s64 amount)
 	percpu_counter_add(fbc, -amount);
 }
 
+/*
+ * Helpers for percpu counters for which per-cpu drift doesn't matter.
+ * This is typically the case for statistics counters that are read with
+ * percpu_counter_sum{,_positive}().
+ */
+#define PERCPU_COUNTER_LARGE_BATCH	(INT_MAX / 2)
+
+static inline void percpu_counter_add_relaxed(struct percpu_counter *fbc,
+					      s64 amount)
+{
+	percpu_counter_add_batch(fbc, amount, PERCPU_COUNTER_LARGE_BATCH);
+}
+
+static inline void percpu_counter_sub_relaxed(struct percpu_counter *fbc,
+					      s64 amount)
+{
+	percpu_counter_add_relaxed(fbc, amount);
+}
+
+static inline void percpu_counter_inc_relaxed(struct percpu_counter *fbc)
+{
+	percpu_counter_add_relaxed(fbc, 1);
+}
+
+static inline void percpu_counter_dec_relaxed(struct percpu_counter *fbc)
+{
+	percpu_counter_add_relaxed(fbc, -1);
+}
+
 #endif /* _LINUX_PERCPU_COUNTER_H */
-- 
2.25.1

[PATCH v4 2/9] lib/percpu_counter: add helpers for arrays of counters

[Amir Goldstein]

Hoist the helpers to init/destroy an array of counters from
nfsd_stats to percpu_counter library.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/nfsd/export.c               | 10 ++++++---
 fs/nfsd/nfscache.c             |  5 +++--
 fs/nfsd/stats.c                | 37 +++-------------------------------
 fs/nfsd/stats.h                |  3 ---
 include/linux/percpu_counter.h | 19 +++++++++++++++++
 lib/percpu_counter.c           | 27 +++++++++++++++++++++++++
 6 files changed, 59 insertions(+), 42 deletions(-)

diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index 668c7527b17e..ec97a086077a 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -334,17 +334,21 @@ static void nfsd4_fslocs_free(struct nfsd4_fs_locations *fsloc)
 static int export_stats_init(struct export_stats *stats)
 {
 	stats->start_time = ktime_get_seconds();
-	return nfsd_percpu_counters_init(stats->counter, EXP_STATS_COUNTERS_NUM);
+	return percpu_counters_init(stats->counter, EXP_STATS_COUNTERS_NUM, 0,
+				    GFP_KERNEL);
 }
 
 static void export_stats_reset(struct export_stats *stats)
 {
-	nfsd_percpu_counters_reset(stats->counter, EXP_STATS_COUNTERS_NUM);
+	int i;
+
+	for (i = 0; i < EXP_STATS_COUNTERS_NUM; i++)
+		percpu_counter_set(&stats->counter[i], 0);
 }
 
 static void export_stats_destroy(struct export_stats *stats)
 {
-	nfsd_percpu_counters_destroy(stats->counter, EXP_STATS_COUNTERS_NUM);
+	percpu_counters_destroy(stats->counter, EXP_STATS_COUNTERS_NUM);
 }
 
 static void svc_export_put(struct kref *ref)
diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
index 0b3f12aa37ff..d93bb4866d07 100644
--- a/fs/nfsd/nfscache.c
+++ b/fs/nfsd/nfscache.c
@@ -150,12 +150,13 @@ void nfsd_drc_slab_free(void)
 
 static int nfsd_reply_cache_stats_init(struct nfsd_net *nn)
 {
-	return nfsd_percpu_counters_init(nn->counter, NFSD_NET_COUNTERS_NUM);
+	return percpu_counters_init(nn->counter, NFSD_NET_COUNTERS_NUM, 0,
+				    GFP_KERNEL);
 }
 
 static void nfsd_reply_cache_stats_destroy(struct nfsd_net *nn)
 {
-	nfsd_percpu_counters_destroy(nn->counter, NFSD_NET_COUNTERS_NUM);
+	percpu_counters_destroy(nn->counter, NFSD_NET_COUNTERS_NUM);
 }
 
 int nfsd_reply_cache_init(struct nfsd_net *nn)
diff --git a/fs/nfsd/stats.c b/fs/nfsd/stats.c
index a8c5a02a84f0..933e703cbb3b 100644
--- a/fs/nfsd/stats.c
+++ b/fs/nfsd/stats.c
@@ -84,46 +84,15 @@ static const struct proc_ops nfsd_proc_ops = {
 	.proc_release	= single_release,
 };
 
-int nfsd_percpu_counters_init(struct percpu_counter counters[], int num)
-{
-	int i, err = 0;
-
-	for (i = 0; !err && i < num; i++)
-		err = percpu_counter_init(&counters[i], 0, GFP_KERNEL);
-
-	if (!err)
-		return 0;
-
-	for (; i > 0; i--)
-		percpu_counter_destroy(&counters[i-1]);
-
-	return err;
-}
-
-void nfsd_percpu_counters_reset(struct percpu_counter counters[], int num)
-{
-	int i;
-
-	for (i = 0; i < num; i++)
-		percpu_counter_set(&counters[i], 0);
-}
-
-void nfsd_percpu_counters_destroy(struct percpu_counter counters[], int num)
-{
-	int i;
-
-	for (i = 0; i < num; i++)
-		percpu_counter_destroy(&counters[i]);
-}
-
 static int nfsd_stat_counters_init(void)
 {
-	return nfsd_percpu_counters_init(nfsdstats.counter, NFSD_STATS_COUNTERS_NUM);
+	return percpu_counters_init(nfsdstats.counter, NFSD_STATS_COUNTERS_NUM,
+				    0, GFP_KERNEL);
 }
 
 static void nfsd_stat_counters_destroy(void)
 {
-	nfsd_percpu_counters_destroy(nfsdstats.counter, NFSD_STATS_COUNTERS_NUM);
+	percpu_counters_destroy(nfsdstats.counter, NFSD_STATS_COUNTERS_NUM);
 }
 
 int nfsd_stat_init(void)
diff --git a/fs/nfsd/stats.h b/fs/nfsd/stats.h
index 9b43dc3d9991..61840f9035a9 100644
--- a/fs/nfsd/stats.h
+++ b/fs/nfsd/stats.h
@@ -36,9 +36,6 @@ extern struct nfsd_stats	nfsdstats;
 
 extern struct svc_stat		nfsd_svcstats;
 
-int nfsd_percpu_counters_init(struct percpu_counter counters[], int num);
-void nfsd_percpu_counters_reset(struct percpu_counter counters[], int num);
-void nfsd_percpu_counters_destroy(struct percpu_counter counters[], int num);
 int nfsd_stat_init(void);
 void nfsd_stat_shutdown(void);
 
diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h
index 7f01f2e41304..37dd81c85411 100644
--- a/include/linux/percpu_counter.h
+++ b/include/linux/percpu_counter.h
@@ -46,6 +46,10 @@ s64 __percpu_counter_sum(struct percpu_counter *fbc);
 int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch);
 void percpu_counter_sync(struct percpu_counter *fbc);
 
+int percpu_counters_init(struct percpu_counter counters[], int num, s64 amount,
+			 gfp_t gfp);
+void percpu_counters_destroy(struct percpu_counter counters[], int num);
+
 static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs)
 {
 	return __percpu_counter_compare(fbc, rhs, percpu_counter_batch);
@@ -109,6 +113,21 @@ static inline void percpu_counter_destroy(struct percpu_counter *fbc)
 {
 }
 
+static inline int percpu_counters_init(struct percpu_counter counters[],
+				       int num, s64 amount, gfp_t gfp)
+{
+	int i;
+
+	for (i = 0; i < num; i++)
+		counters[i] = amount;
+	return 0;
+}
+
+static inline void percpu_counters_destroy(struct percpu_counter counters[],
+					   int num)
+{
+}
+
 static inline void percpu_counter_set(struct percpu_counter *fbc, s64 amount)
 {
 	fbc->count = amount;
diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
index ed610b75dc32..f75a45c63c18 100644
--- a/lib/percpu_counter.c
+++ b/lib/percpu_counter.c
@@ -181,6 +181,33 @@ void percpu_counter_destroy(struct percpu_counter *fbc)
 }
 EXPORT_SYMBOL(percpu_counter_destroy);
 
+int percpu_counters_init(struct percpu_counter counters[], int num, s64 amount,
+			 gfp_t gfp)
+{
+	int i, err = 0;
+
+	for (i = 0; !err && i < num; i++)
+		err = percpu_counter_init(&counters[i], amount, gfp);
+
+	if (!err)
+		return 0;
+
+	for (; i > 0; i--)
+		percpu_counter_destroy(&counters[i-1]);
+
+	return err;
+}
+EXPORT_SYMBOL(percpu_counters_init);
+
+void percpu_counters_destroy(struct percpu_counter counters[], int num)
+{
+	int i;
+
+	for (i = 0; i < num; i++)
+		percpu_counter_destroy(&counters[i]);
+}
+EXPORT_SYMBOL(percpu_counters_destroy);
+
 int percpu_counter_batch __read_mostly = 32;
 EXPORT_SYMBOL(percpu_counter_batch);
 
-- 
2.25.1

[PATCH v4 3/9] fs: tidy up fs_flags definitions

[Amir Goldstein]

Use bit shift for flag constants and abbreviate comments.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 include/linux/fs.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 831b20430d6e..ecb64997c390 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2429,13 +2429,13 @@ int sync_inode_metadata(struct inode *inode, int wait);
 struct file_system_type {
 	const char *name;
 	int fs_flags;
-#define FS_REQUIRES_DEV		1 
-#define FS_BINARY_MOUNTDATA	2
-#define FS_HAS_SUBTYPE		4
-#define FS_USERNS_MOUNT		8	/* Can be mounted by userns root */
-#define FS_DISALLOW_NOTIFY_PERM	16	/* Disable fanotify permission events */
-#define FS_ALLOW_IDMAP         32      /* FS has been updated to handle vfs idmappings. */
-#define FS_RENAME_DOES_D_MOVE	32768	/* FS will handle d_move() during rename() internally. */
+#define FS_REQUIRES_DEV		(1<<0)
+#define FS_BINARY_MOUNTDATA	(1<<1)
+#define FS_HAS_SUBTYPE		(1<<2)
+#define FS_USERNS_MOUNT		(1<<3)	/* Can be mounted by userns root */
+#define FS_DISALLOW_NOTIFY_PERM	(1<<4)	/* Disable fanotify permission events */
+#define FS_ALLOW_IDMAP		(1<<5)	/* FS can handle vfs idmappings */
+#define FS_RENAME_DOES_D_MOVE	(1<<15)	/* FS will handle d_move() internally */
 	int (*init_fs_context)(struct fs_context *);
 	const struct fs_parameter_spec *parameters;
 	struct dentry *(*mount) (struct file_system_type *, int,
-- 
2.25.1

[PATCH v4 4/9] fs: add optional iostats counters to struct super_block

[Amir Goldstein]

With CONFIG_FS_IOSTATS, filesystems can opt-in to generic per-sb I/O
statistics by setting the FS_SB_IOSTATS fstype flag.

These counters will be used to collect per-sb I/O statistics and display
them in /proc/<pid>/mountstats.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/Kconfig                 |   8 +++
 fs/super.c                 |   6 ++
 include/linux/fs.h         |  11 +++-
 include/linux/fs_iostats.h | 130 +++++++++++++++++++++++++++++++++++++
 4 files changed, 153 insertions(+), 2 deletions(-)
 create mode 100644 include/linux/fs_iostats.h

diff --git a/fs/Kconfig b/fs/Kconfig
index 6c7dc1387beb..394d9da6bda9 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -15,6 +15,14 @@ config VALIDATE_FS_PARSER
 	  Enable this to perform validation of the parameter description for a
 	  filesystem when it is registered.
 
+config FS_IOSTATS
+	bool "Enable generic filesystem I/O statistics"
+	help
+	  Enable this to allow collecting filesystem I/O statistics and display
+	  them in /proc/<pid>/mountstats.
+
+	  Say N if unsure.
+
 config FS_IOMAP
 	bool
 
diff --git a/fs/super.c b/fs/super.c
index f1d4a193602d..a18930693e54 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -36,6 +36,7 @@
 #include <linux/lockdep.h>
 #include <linux/user_namespace.h>
 #include <linux/fs_context.h>
+#include <linux/fs_iostats.h>
 #include <uapi/linux/mount.h>
 #include "internal.h"
 
@@ -179,6 +180,7 @@ static void destroy_unused_super(struct super_block *s)
 	up_write(&s->s_umount);
 	list_lru_destroy(&s->s_dentry_lru);
 	list_lru_destroy(&s->s_inode_lru);
+	sb_iostats_destroy(s);
 	security_sb_free(s);
 	put_user_ns(s->s_user_ns);
 	kfree(s->s_subtype);
@@ -230,6 +232,9 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
 	if (security_sb_alloc(s))
 		goto fail;
 
+	if (type->fs_flags & FS_SB_IOSTATS && sb_iostats_init(s))
+		goto fail;
+
 	for (i = 0; i < SB_FREEZE_LEVELS; i++) {
 		if (__percpu_init_rwsem(&s->s_writers.rw_sem[i],
 					sb_writers_name[i],
@@ -290,6 +295,7 @@ static void __put_super(struct super_block *s)
 		WARN_ON(s->s_dentry_lru.node);
 		WARN_ON(s->s_inode_lru.node);
 		WARN_ON(!list_empty(&s->s_mounts));
+		sb_iostats_destroy(s);
 		security_sb_free(s);
 		fscrypt_sb_free(s);
 		put_user_ns(s->s_user_ns);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ecb64997c390..f8e7ec81ae0b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1455,6 +1455,8 @@ struct sb_writers {
 	struct percpu_rw_semaphore	rw_sem[SB_FREEZE_LEVELS];
 };
 
+struct sb_iostats;
+
 struct super_block {
 	struct list_head	s_list;		/* Keep this first */
 	dev_t			s_dev;		/* search index; _not_ kdev_t */
@@ -1509,8 +1511,12 @@ struct super_block {
 	/* Granularity of c/m/atime in ns (cannot be worse than a second) */
 	u32			s_time_gran;
 	/* Time limits for c/m/atime in seconds */
-	time64_t		   s_time_min;
-	time64_t		   s_time_max;
+	time64_t		s_time_min;
+	time64_t		s_time_max;
+#ifdef CONFIG_FS_IOSTATS
+	/* Optional per-sb I/O stats */
+	struct sb_iostats	*s_iostats;
+#endif
 #ifdef CONFIG_FSNOTIFY
 	__u32			s_fsnotify_mask;
 	struct fsnotify_mark_connector __rcu	*s_fsnotify_marks;
@@ -2435,6 +2441,7 @@ struct file_system_type {
 #define FS_USERNS_MOUNT		(1<<3)	/* Can be mounted by userns root */
 #define FS_DISALLOW_NOTIFY_PERM	(1<<4)	/* Disable fanotify permission events */
 #define FS_ALLOW_IDMAP		(1<<5)	/* FS can handle vfs idmappings */
+#define FS_SB_IOSTATS		(1<<6)	/* FS has generic per-sb I/O stats */
 #define FS_RENAME_DOES_D_MOVE	(1<<15)	/* FS will handle d_move() internally */
 	int (*init_fs_context)(struct fs_context *);
 	const struct fs_parameter_spec *parameters;
diff --git a/include/linux/fs_iostats.h b/include/linux/fs_iostats.h
new file mode 100644
index 000000000000..2db13e9e17fc
--- /dev/null
+++ b/include/linux/fs_iostats.h
@@ -0,0 +1,130 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_FS_IOSTATS_H
+#define _LINUX_FS_IOSTATS_H
+
+#include <linux/fs.h>
+#include <linux/percpu_counter.h>
+#include <linux/slab.h>
+
+/* Similar to task_io_accounting members */
+enum {
+	SB_IOSTATS_CHARS_RD,	/* bytes read via syscalls */
+	SB_IOSTATS_CHARS_WR,	/* bytes written via syscalls */
+	SB_IOSTATS_SYSCALLS_RD,	/* # of read syscalls */
+	SB_IOSTATS_SYSCALLS_WR,	/* # of write syscalls */
+	SB_IOSTATS_COUNTERS_NUM
+};
+
+struct sb_iostats {
+	time64_t		start_time;
+	struct percpu_counter	counter[SB_IOSTATS_COUNTERS_NUM];
+};
+
+#ifdef CONFIG_FS_IOSTATS
+static inline struct sb_iostats *sb_iostats(struct super_block *sb)
+{
+	return sb->s_iostats;
+}
+
+static inline bool sb_has_iostats(struct super_block *sb)
+{
+	return !!sb->s_iostats;
+}
+
+/* Initialize per-sb I/O stats */
+static inline int sb_iostats_init(struct super_block *sb)
+{
+	int err;
+
+	if (sb->s_iostats)
+		return 0;
+
+	sb->s_iostats = kmalloc(sizeof(struct sb_iostats), GFP_KERNEL);
+	if (!sb->s_iostats)
+		return -ENOMEM;
+
+	err = percpu_counters_init(sb->s_iostats->counter,
+				   SB_IOSTATS_COUNTERS_NUM, 0, GFP_KERNEL);
+	if (err) {
+		kfree(sb->s_iostats);
+		sb->s_iostats = NULL;
+		return err;
+	}
+
+	sb->s_iostats->start_time = ktime_get_seconds();
+	return 0;
+}
+
+static inline void sb_iostats_destroy(struct super_block *sb)
+{
+	if (!sb->s_iostats)
+		return;
+
+	percpu_counters_destroy(sb->s_iostats->counter,
+				SB_IOSTATS_COUNTERS_NUM);
+	kfree(sb->s_iostats);
+	sb->s_iostats = NULL;
+}
+
+static inline void sb_iostats_counter_inc(struct super_block *sb, int id)
+{
+	if (!sb->s_iostats)
+		return;
+
+	percpu_counter_inc_relaxed(&sb->s_iostats->counter[id]);
+}
+
+static inline void sb_iostats_counter_add(struct super_block *sb, int id,
+					  s64 amt)
+{
+	if (!sb->s_iostats)
+		return;
+
+	percpu_counter_add_relaxed(&sb->s_iostats->counter[id], amt);
+}
+
+static inline s64 sb_iostats_counter_read(struct super_block *sb, int id)
+{
+	if (!sb->s_iostats)
+		return 0;
+
+	return percpu_counter_sum_positive(&sb->s_iostats->counter[id]);
+}
+
+#else /* !CONFIG_FS_IOSTATS */
+
+static inline struct sb_iostats *sb_iostats(struct super_block *sb)
+{
+	return NULL;
+}
+
+static inline bool sb_has_iostats(struct super_block *sb)
+{
+	return false;
+}
+
+static inline int sb_iostats_init(struct super_block *sb)
+{
+	return 0;
+}
+
+static inline void sb_iostats_destroy(struct super_block *sb)
+{
+}
+
+static inline void sb_iostats_counter_inc(struct super_block *sb, int id)
+{
+}
+
+static inline void sb_iostats_counter_add(struct super_block *sb, int id,
+					  s64 amt)
+{
+}
+
+static inline s64 sb_iostats_counter_read(struct super_block *sb, int id)
+{
+	return 0;
+}
+#endif
+
+#endif /* _LINUX_FS_IOSTATS_H */
-- 
2.25.1

[PATCH v4 5/9] fs: collect per-sb io stats

[Amir Goldstein]

Replace task io account helpers with wrappers that may also collect
per-sb io stats for filesystems that have per-sb io stats enabled.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/read_write.c | 88 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 64 insertions(+), 24 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 0074afa7ecb3..8c599bf2dd78 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -20,6 +20,7 @@
 #include <linux/compat.h>
 #include <linux/mount.h>
 #include <linux/fs.h>
+#include <linux/fs_iostats.h>
 #include "internal.h"
 
 #include <linux/uaccess.h>
@@ -34,6 +35,45 @@ const struct file_operations generic_ro_fops = {
 
 EXPORT_SYMBOL(generic_ro_fops);
 
+static inline void file_iostats_counter_inc(struct file *file, int id)
+{
+	if (file)
+		sb_iostats_counter_inc(file->f_path.mnt->mnt_sb, id);
+}
+
+static inline void file_iostats_counter_add(struct file *file, int id,
+					    ssize_t amt)
+{
+	if (file)
+		sb_iostats_counter_add(file->f_path.mnt->mnt_sb, id, amt);
+}
+
+static void file_add_rchar(struct file *file, struct task_struct *tsk,
+			   ssize_t amt)
+{
+	file_iostats_counter_add(file, SB_IOSTATS_CHARS_RD, amt);
+	add_rchar(tsk, amt);
+}
+
+static void file_add_wchar(struct file *file, struct task_struct *tsk,
+			   ssize_t amt)
+{
+	file_iostats_counter_add(file, SB_IOSTATS_CHARS_WR, amt);
+	add_wchar(tsk, amt);
+}
+
+static void file_inc_syscr(struct file *file, struct task_struct *tsk)
+{
+	file_iostats_counter_inc(file, SB_IOSTATS_SYSCALLS_RD);
+	inc_syscr(current);
+}
+
+static void file_inc_syscw(struct file *file, struct task_struct *tsk)
+{
+	file_iostats_counter_inc(file, SB_IOSTATS_SYSCALLS_WR);
+	inc_syscw(current);
+}
+
 static inline bool unsigned_offsets(struct file *file)
 {
 	return file->f_mode & FMODE_UNSIGNED_OFFSET;
@@ -441,9 +481,9 @@ ssize_t __kernel_read(struct file *file, void *buf, size_t count, loff_t *pos)
 		if (pos)
 			*pos = kiocb.ki_pos;
 		fsnotify_access(file);
-		add_rchar(current, ret);
+		file_add_rchar(file, current, ret);
 	}
-	inc_syscr(current);
+	file_inc_syscr(file, current);
 	return ret;
 }
 
@@ -483,9 +523,9 @@ ssize_t vfs_read(struct file *file, char __user *buf, size_t count, loff_t *pos)
 		ret = -EINVAL;
 	if (ret > 0) {
 		fsnotify_access(file);
-		add_rchar(current, ret);
+		file_add_rchar(file, current, ret);
 	}
-	inc_syscr(current);
+	file_inc_syscr(file, current);
 	return ret;
 }
 
@@ -537,9 +577,9 @@ ssize_t __kernel_write(struct file *file, const void *buf, size_t count, loff_t
 		if (pos)
 			*pos = kiocb.ki_pos;
 		fsnotify_modify(file);
-		add_wchar(current, ret);
+		file_add_wchar(file, current, ret);
 	}
-	inc_syscw(current);
+	file_inc_syscw(file, current);
 	return ret;
 }
 /*
@@ -592,9 +632,9 @@ ssize_t vfs_write(struct file *file, const char __user *buf, size_t count, loff_
 		ret = -EINVAL;
 	if (ret > 0) {
 		fsnotify_modify(file);
-		add_wchar(current, ret);
+		file_add_wchar(file, current, ret);
 	}
-	inc_syscw(current);
+	file_inc_syscw(file, current);
 	file_end_write(file);
 	return ret;
 }
@@ -947,8 +987,8 @@ static ssize_t do_readv(unsigned long fd, const struct iovec __user *vec,
 	}
 
 	if (ret > 0)
-		add_rchar(current, ret);
-	inc_syscr(current);
+		file_add_rchar(f.file, current, ret);
+	file_inc_syscr(f.file, current);
 	return ret;
 }
 
@@ -971,8 +1011,8 @@ static ssize_t do_writev(unsigned long fd, const struct iovec __user *vec,
 	}
 
 	if (ret > 0)
-		add_wchar(current, ret);
-	inc_syscw(current);
+		file_add_wchar(f.file, current, ret);
+	file_inc_syscw(f.file, current);
 	return ret;
 }
 
@@ -1000,8 +1040,8 @@ static ssize_t do_preadv(unsigned long fd, const struct iovec __user *vec,
 	}
 
 	if (ret > 0)
-		add_rchar(current, ret);
-	inc_syscr(current);
+		file_add_rchar(f.file, current, ret);
+	file_inc_syscr(f.file, current);
 	return ret;
 }
 
@@ -1023,8 +1063,8 @@ static ssize_t do_pwritev(unsigned long fd, const struct iovec __user *vec,
 	}
 
 	if (ret > 0)
-		add_wchar(current, ret);
-	inc_syscw(current);
+		file_add_wchar(f.file, current, ret);
+	file_inc_syscw(f.file, current);
 	return ret;
 }
 
@@ -1250,8 +1290,8 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
 	}
 
 	if (retval > 0) {
-		add_rchar(current, retval);
-		add_wchar(current, retval);
+		file_add_rchar(in.file, current, retval);
+		file_add_wchar(out.file, current, retval);
 		fsnotify_access(in.file);
 		fsnotify_modify(out.file);
 		out.file->f_pos = out_pos;
@@ -1261,8 +1301,8 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
 			in.file->f_pos = pos;
 	}
 
-	inc_syscr(current);
-	inc_syscw(current);
+	file_inc_syscr(in.file, current);
+	file_inc_syscw(out.file, current);
 	if (pos > max)
 		retval = -EOVERFLOW;
 
@@ -1511,13 +1551,13 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 done:
 	if (ret > 0) {
 		fsnotify_access(file_in);
-		add_rchar(current, ret);
+		file_add_rchar(file_in, current, ret);
 		fsnotify_modify(file_out);
-		add_wchar(current, ret);
+		file_add_wchar(file_out, current, ret);
 	}
 
-	inc_syscr(current);
-	inc_syscw(current);
+	file_inc_syscr(file_in, current);
+	file_inc_syscw(file_out, current);
 
 	file_end_write(file_out);
 
-- 
2.25.1

[PATCH v4 6/9] fs: report per-sb io stats

[Amir Goldstein]

Show optional collected per-sb io stats in /proc/<pid>/mountstats
for filesystems that do not implement their own show_stats() method
and have generic per-sb stats enabled.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/proc_namespace.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/fs/proc_namespace.c b/fs/proc_namespace.c
index 49650e54d2f8..9054a909e031 100644
--- a/fs/proc_namespace.c
+++ b/fs/proc_namespace.c
@@ -11,6 +11,7 @@
 #include <linux/nsproxy.h>
 #include <linux/security.h>
 #include <linux/fs_struct.h>
+#include <linux/fs_iostats.h>
 #include <linux/sched/task.h>
 
 #include "proc/internal.h" /* only for get_proc_task() in ->open() */
@@ -232,6 +233,21 @@ 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 (sb_has_iostats(sb)) {
+		struct sb_iostats *iostats = sb_iostats(sb);
+
+		/* 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",
+			   iostats->start_time, ktime_get_seconds(),
+			   sb_iostats_counter_read(sb, SB_IOSTATS_CHARS_RD),
+			   sb_iostats_counter_read(sb, SB_IOSTATS_CHARS_WR),
+			   sb_iostats_counter_read(sb, SB_IOSTATS_SYSCALLS_RD),
+			   sb_iostats_counter_read(sb, SB_IOSTATS_SYSCALLS_WR));
 	}
 
 	seq_putc(m, '\n');
-- 
2.25.1

[PATCH v4 7/9] ovl: opt-in for per-sb io stats

[Amir Goldstein]

Traditionally, system administrators have used the iostat utility
to track the amount of io performed to a local disk filesystem.

Similar functionality is provided for NFS mounts via the nfsstat
utility that reads the NFS client's stats from /proc/pid/mountstats.

There is currently no good way for a system administrator or a
monitoring application inside a container to track the amount of io
performed via overlayfs.

Opt-in for generic io stats via /proc/pid/mountstats to provide
that functionality.

This feature depends on CONFIG_FS_IOSTATS.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 7bb0a47cb615..4a5847bca1a6 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -6,6 +6,7 @@
 
 #include <uapi/linux/magic.h>
 #include <linux/fs.h>
+#include <linux/fs_iostats.h>
 #include <linux/namei.h>
 #include <linux/xattr.h>
 #include <linux/mount.h>
@@ -2165,7 +2166,7 @@ static struct dentry *ovl_mount(struct file_system_type *fs_type, int flags,
 static struct file_system_type ovl_fs_type = {
 	.owner		= THIS_MODULE,
 	.name		= "overlay",
-	.fs_flags	= FS_USERNS_MOUNT,
+	.fs_flags	= FS_USERNS_MOUNT | FS_SB_IOSTATS,
 	.mount		= ovl_mount,
 	.kill_sb	= kill_anon_super,
 };
-- 
2.25.1

[PATCH v4 8/9] fuse: opt-in for per-sb io stats

[Amir Goldstein]

Traditionally, system administrators have used the iostat utility
to track the amount of io performed to a local disk filesystem.

Similar functionality is provided for NFS mounts via the nfsstat
utility that reads the NFS client's stats from /proc/pid/mountstats.

There is currently no good way for a system administrator or a
monitoring application to track the amount of io performed via fuse
filesystems.

Opt-in for generic io stats via /proc/pid/mountstats to provide
that functionality.

It is possible to collect io stats on the server side inside libfuse,
but those io stats will not cover cached writes and reads.  Therefore,
implementing the server side io stats would be complementary to these
client side io stats.  Also, this feature provides the io stats for
existing fuse filesystem/lib release binaries.

This feature depends on CONFIG_FS_IOSTATS.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/fuse/inode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 9ee36aa73251..a2cd90e059f8 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -16,6 +16,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/fs_context.h>
+#include <linux/fs_iostats.h>
 #include <linux/fs_parser.h>
 #include <linux/statfs.h>
 #include <linux/random.h>
@@ -1806,7 +1807,7 @@ static void fuse_kill_sb_anon(struct super_block *sb)
 static struct file_system_type fuse_fs_type = {
 	.owner		= THIS_MODULE,
 	.name		= "fuse",
-	.fs_flags	= FS_HAS_SUBTYPE | FS_USERNS_MOUNT,
+	.fs_flags	= FS_HAS_SUBTYPE | FS_USERNS_MOUNT | FS_SB_IOSTATS,
 	.init_fs_context = fuse_init_fs_context,
 	.parameters	= fuse_fs_parameters,
 	.kill_sb	= fuse_kill_sb_anon,
-- 
2.25.1

[PATCH v4 9/9] fs: enable per-sb io stats for all blockdev filesystems

[Amir Goldstein]

In addition to filesystems that opt-in with FS_SB_IOSTATS, auto-enable
per-sb I/O stats for all blockdev filesystems.

This can be used by tools like iotop to display the total I/O stats
via sb along side the submitted I/O stats to block device to get a
more complete view that also includes the cached I/O stats.

Link: https://lore.kernel.org/linux-fsdevel/20220302211226.GG3927073@dread.disaster.area/
Suggested-by: Dave Chinner <david@fromorbit.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/super.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/super.c b/fs/super.c
index a18930693e54..e1bee46dfb5a 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -232,7 +232,12 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
 	if (security_sb_alloc(s))
 		goto fail;
 
-	if (type->fs_flags & FS_SB_IOSTATS && sb_iostats_init(s))
+	/*
+	 * Account per-sb I/O stats for all blockdev filesystems and for
+	 * filesystems that opt-in with FS_SB_IOSTATS.
+	 */
+	if (type->fs_flags & (FS_SB_IOSTATS | FS_REQUIRES_DEV) &&
+	    sb_iostats_init(s))
 		goto fail;
 
 	for (i = 0; i < SB_FREEZE_LEVELS; i++) {
-- 
2.25.1

Re: [PATCH v4 0/9] Generic per-sb io stats

[Theodore Ts'o]

On Sat, Mar 05, 2022 at 06:04:15PM +0200, Amir Goldstein wrote:
> 
> Dave Chinner asked why the io stats should not be enabled for all
> filesystems.  That change seems too bold for me so instead, I included
> an extra patch to auto-enable per-sb io stats for blockdev filesystems.

Perhaps something to consider is allowing users to be able to enable
or disable I/O stats on per mount basis?

Consider if a potential future user of this feature has servers with
one or two 256-core AMD Epyc chip, and suppose that they have a
several thousand iSCSI mounted file systems containing various
software packages for use by Kubernetes jobs.  (Or even several
thousand mounted overlay file systems.....)

The size of the percpu counter is going to be *big* on a large CPU
count machine, and the iostats structure has 5 of these per-cpu
counters, so if you have one for every single mounted file system,
even if the CPU slowdown isn't significant, the non-swappable kernel
memory overhead might be quite large.

So maybe a VFS-level mount option, say, "iostats" and "noiostats", and
some kind of global option indicating whether the default should be
iostats being enabled or disabled?  Bonus points if iostats can be
enabled or disabled after the initial mount via remount operation.

I could imagine some people only being interested to enable iostats on
certain file systems, or certain classes of block devices --- so they
might want it enabled on some ext4 file systems which are attached to
physical devices, but not on the N thousand iSCSI or nbd mounts that
are also happen to be using ext4.

Cheers,

						- Ted

Re: [PATCH v4 0/9] Generic per-sb io stats

[Amir Goldstein]

On Sun, Mar 6, 2022 at 6:18 AM Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Sat, Mar 05, 2022 at 06:04:15PM +0200, Amir Goldstein wrote:
> >
> > Dave Chinner asked why the io stats should not be enabled for all
> > filesystems.  That change seems too bold for me so instead, I included
> > an extra patch to auto-enable per-sb io stats for blockdev filesystems.
>
> Perhaps something to consider is allowing users to be able to enable
> or disable I/O stats on per mount basis?
>
> Consider if a potential future user of this feature has servers with
> one or two 256-core AMD Epyc chip, and suppose that they have a
> several thousand iSCSI mounted file systems containing various
> software packages for use by Kubernetes jobs.  (Or even several
> thousand mounted overlay file systems.....)
>
> The size of the percpu counter is going to be *big* on a large CPU
> count machine, and the iostats structure has 5 of these per-cpu
> counters, so if you have one for every single mounted file system,
> even if the CPU slowdown isn't significant, the non-swappable kernel
> memory overhead might be quite large.
>
> So maybe a VFS-level mount option, say, "iostats" and "noiostats", and
> some kind of global option indicating whether the default should be
> iostats being enabled or disabled?  Bonus points if iostats can be
> enabled or disabled after the initial mount via remount operation.
>
> I could imagine some people only being interested to enable iostats on
> certain file systems, or certain classes of block devices --- so they
> might want it enabled on some ext4 file systems which are attached to
> physical devices, but not on the N thousand iSCSI or nbd mounts that
> are also happen to be using ext4.
>

Those were my thoughts as well.

As a matter of fact, I started to have a go at implementing
"iostats"/"noiostats"
and then I realized I have no clue how the designers of the new mount option
parser API intended that new generic mount options like these would be added,
so I ended up reusing SB_MAND_LOCK for the test patch.

Was I supposed to extend struct fs_context fields sb_flags/sb_flags_mask to
unsigned long and add new common SB_ flags to high 32 bits, which can only
be set via fsopen()/fsconfig() on a 64bit arch?

Or did the designers have something completely different in mind?

Perhaps the scope of the new mount API was never to deal with running out of
space for common SB_ flags?

Thanks,
Amir.

Re: [PATCH v4 0/9] Generic per-sb io stats

[Dave Chinner]

On Sat, Mar 05, 2022 at 11:18:34PM -0500, Theodore Ts'o wrote:
> On Sat, Mar 05, 2022 at 06:04:15PM +0200, Amir Goldstein wrote:
> > 
> > Dave Chinner asked why the io stats should not be enabled for all
> > filesystems.  That change seems too bold for me so instead, I included
> > an extra patch to auto-enable per-sb io stats for blockdev filesystems.
> 
> Perhaps something to consider is allowing users to be able to enable
> or disable I/O stats on per mount basis?
> 
> Consider if a potential future user of this feature has servers with
> one or two 256-core AMD Epyc chip, and suppose that they have a
> several thousand iSCSI mounted file systems containing various
> software packages for use by Kubernetes jobs.  (Or even several
> thousand mounted overlay file systems.....)
> 
> The size of the percpu counter is going to be *big* on a large CPU
> count machine, and the iostats structure has 5 of these per-cpu
> counters, so if you have one for every single mounted file system,
> even if the CPU slowdown isn't significant, the non-swappable kernel
> memory overhead might be quite large.

A percpu counter on a 256 core machine is ~1kB. Adding 5kB to the
struct superblock isn't a bit deal for a machine of this size, even
if you have thousands of superblocks - we're talking a few
*megabytes* of extra memory in a machine that would typically have
hundreds of GB of RAM. Seriously, the memory overhead of the per-cpu
counters is noise compared to the memory footprint of, say, the
stacks needing to be allocated for every background worker thread
the filesystem needs.

Yeah, I know, we have ~175 per-cpu stats counters per XFS superblock
(we already cover the 4 counters Amir is proposing to add as generic
SB counters), and we have half a dozen dedicated worker threads per
mount. Yet systems still function just fine when there are thousands
of XFS filesystems and thousands of CPUs.

Seriously, a small handful of per-cpu counters that capture
information for all superblocks is not a big deal. Small systems
will have relatively litte overhead, large systems have the memory
to handle it.

> So maybe a VFS-level mount option, say, "iostats" and "noiostats", and
> some kind of global option indicating whether the default should be
> iostats being enabled or disabled?  Bonus points if iostats can be
> enabled or disabled after the initial mount via remount operation.

Can we please just avoid mount options for stuff like this? It'll
just never get tested unless it defaults to on, and then almost
no-one will ever turn it off because why would you bother tweaking
something that has not noticable impact but can give useful insights
the workload that is running?

I don't care one way or another here because this is essentially
duplicating something we've had in XFS for 20+ years. What I want to
avoid is blowing out the test matrix even further. Adding optional
features has a cost in terms of testing time, so if it's a feature
that is only rarely going to be turned on then we shouldn't add it
at all. If it's only rearely going to be turned off, OTOH, then we
should just make it ubiquitous and available for everything so it's
always tested.

Hence, AFAICT, the only real option for yes/no support is the
Kconfig option. If the kernel builder turns it on, it is on for
everything, otherwise it is off for everything.

> I could imagine some people only being interested to enable iostats on
> certain file systems, or certain classes of block devices --- so they
> might want it enabled on some ext4 file systems which are attached to
> physical devices, but not on the N thousand iSCSI or nbd mounts that
> are also happen to be using ext4.

That seems ... fairly contrived. Block device IO stats are not turned
on and off based on the block device type - they are generic.
Network device stats are not turned on and off based on teh network
device - they are generic. Why should per-filesystem IO stats be
special and different to everything else?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v4 0/9] Generic per-sb io stats

[Amir Goldstein]

On Mon, Mar 7, 2022 at 2:14 AM Dave Chinner <david@fromorbit.com> wrote:
>
> On Sat, Mar 05, 2022 at 11:18:34PM -0500, Theodore Ts'o wrote:
> > On Sat, Mar 05, 2022 at 06:04:15PM +0200, Amir Goldstein wrote:
> > >
> > > Dave Chinner asked why the io stats should not be enabled for all
> > > filesystems.  That change seems too bold for me so instead, I included
> > > an extra patch to auto-enable per-sb io stats for blockdev filesystems.
> >
> > Perhaps something to consider is allowing users to be able to enable
> > or disable I/O stats on per mount basis?
> >
> > Consider if a potential future user of this feature has servers with
> > one or two 256-core AMD Epyc chip, and suppose that they have a
> > several thousand iSCSI mounted file systems containing various
> > software packages for use by Kubernetes jobs.  (Or even several
> > thousand mounted overlay file systems.....)
> >
> > The size of the percpu counter is going to be *big* on a large CPU
> > count machine, and the iostats structure has 5 of these per-cpu
> > counters, so if you have one for every single mounted file system,
> > even if the CPU slowdown isn't significant, the non-swappable kernel
> > memory overhead might be quite large.
>
> A percpu counter on a 256 core machine is ~1kB. Adding 5kB to the
> struct superblock isn't a bit deal for a machine of this size, even
> if you have thousands of superblocks - we're talking a few
> *megabytes* of extra memory in a machine that would typically have
> hundreds of GB of RAM. Seriously, the memory overhead of the per-cpu
> counters is noise compared to the memory footprint of, say, the
> stacks needing to be allocated for every background worker thread
> the filesystem needs.
>
> Yeah, I know, we have ~175 per-cpu stats counters per XFS superblock
> (we already cover the 4 counters Amir is proposing to add as generic
> SB counters), and we have half a dozen dedicated worker threads per
> mount. Yet systems still function just fine when there are thousands
> of XFS filesystems and thousands of CPUs.
>
> Seriously, a small handful of per-cpu counters that capture
> information for all superblocks is not a big deal. Small systems
> will have relatively litte overhead, large systems have the memory
> to handle it.
>
> > So maybe a VFS-level mount option, say, "iostats" and "noiostats", and
> > some kind of global option indicating whether the default should be
> > iostats being enabled or disabled?  Bonus points if iostats can be
> > enabled or disabled after the initial mount via remount operation.
>
> Can we please just avoid mount options for stuff like this? It'll
> just never get tested unless it defaults to on, and then almost
> no-one will ever turn it off because why would you bother tweaking
> something that has not noticable impact but can give useful insights
> the workload that is running?
>
> I don't care one way or another here because this is essentially
> duplicating something we've had in XFS for 20+ years. What I want to
> avoid is blowing out the test matrix even further. Adding optional
> features has a cost in terms of testing time, so if it's a feature
> that is only rarely going to be turned on then we shouldn't add it
> at all. If it's only rearely going to be turned off, OTOH, then we
> should just make it ubiquitous and available for everything so it's
> always tested.
>
> Hence, AFAICT, the only real option for yes/no support is the
> Kconfig option. If the kernel builder turns it on, it is on for
> everything, otherwise it is off for everything.
>

I agree with this sentiment and I also share Ted's concerns
that we may be overlooking some aspect, so my preference would
be that Miklos takes the sb_iostats infra patches through his tree
to enable iostats for fuse/overlayfs (I argued in the commit message
why I think they deserve a special treatment).

Regarding the last patch -
Ted, would you be more comfortable if it came with yet another
Kconfig (e.g. FS_IOSTATS_DEFAULT)? Or perhaps with a /proc/sys/fs/
fail safety off switch (like protected_symlinks)?
That gives more options to distros.

Thanks,
Amir.

Re: [PATCH v4 2/9] lib/percpu_counter: add helpers for arrays of counters

[Amir Goldstein]

On Sat, Mar 5, 2022 at 6:04 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Hoist the helpers to init/destroy an array of counters from
> nfsd_stats to percpu_counter library.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/nfsd/export.c               | 10 ++++++---
>  fs/nfsd/nfscache.c             |  5 +++--
>  fs/nfsd/stats.c                | 37 +++-------------------------------
>  fs/nfsd/stats.h                |  3 ---
>  include/linux/percpu_counter.h | 19 +++++++++++++++++
>  lib/percpu_counter.c           | 27 +++++++++++++++++++++++++
>  6 files changed, 59 insertions(+), 42 deletions(-)
>
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index 668c7527b17e..ec97a086077a 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -334,17 +334,21 @@ static void nfsd4_fslocs_free(struct nfsd4_fs_locations *fsloc)
>  static int export_stats_init(struct export_stats *stats)
>  {
>         stats->start_time = ktime_get_seconds();
> -       return nfsd_percpu_counters_init(stats->counter, EXP_STATS_COUNTERS_NUM);
> +       return percpu_counters_init(stats->counter, EXP_STATS_COUNTERS_NUM, 0,
> +                                   GFP_KERNEL);
>  }
>
>  static void export_stats_reset(struct export_stats *stats)
>  {
> -       nfsd_percpu_counters_reset(stats->counter, EXP_STATS_COUNTERS_NUM);
> +       int i;
> +
> +       for (i = 0; i < EXP_STATS_COUNTERS_NUM; i++)
> +               percpu_counter_set(&stats->counter[i], 0);
>  }
>
>  static void export_stats_destroy(struct export_stats *stats)
>  {
> -       nfsd_percpu_counters_destroy(stats->counter, EXP_STATS_COUNTERS_NUM);
> +       percpu_counters_destroy(stats->counter, EXP_STATS_COUNTERS_NUM);
>  }
>
>  static void svc_export_put(struct kref *ref)
> diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
> index 0b3f12aa37ff..d93bb4866d07 100644
> --- a/fs/nfsd/nfscache.c
> +++ b/fs/nfsd/nfscache.c
> @@ -150,12 +150,13 @@ void nfsd_drc_slab_free(void)
>
>  static int nfsd_reply_cache_stats_init(struct nfsd_net *nn)
>  {
> -       return nfsd_percpu_counters_init(nn->counter, NFSD_NET_COUNTERS_NUM);
> +       return percpu_counters_init(nn->counter, NFSD_NET_COUNTERS_NUM, 0,
> +                                   GFP_KERNEL);
>  }
>
>  static void nfsd_reply_cache_stats_destroy(struct nfsd_net *nn)
>  {
> -       nfsd_percpu_counters_destroy(nn->counter, NFSD_NET_COUNTERS_NUM);
> +       percpu_counters_destroy(nn->counter, NFSD_NET_COUNTERS_NUM);
>  }
>
>  int nfsd_reply_cache_init(struct nfsd_net *nn)
> diff --git a/fs/nfsd/stats.c b/fs/nfsd/stats.c
> index a8c5a02a84f0..933e703cbb3b 100644
> --- a/fs/nfsd/stats.c
> +++ b/fs/nfsd/stats.c
> @@ -84,46 +84,15 @@ static const struct proc_ops nfsd_proc_ops = {
>         .proc_release   = single_release,
>  };
>
> -int nfsd_percpu_counters_init(struct percpu_counter counters[], int num)
> -{
> -       int i, err = 0;
> -
> -       for (i = 0; !err && i < num; i++)
> -               err = percpu_counter_init(&counters[i], 0, GFP_KERNEL);
> -
> -       if (!err)
> -               return 0;
> -
> -       for (; i > 0; i--)
> -               percpu_counter_destroy(&counters[i-1]);
> -
> -       return err;
> -}
> -
> -void nfsd_percpu_counters_reset(struct percpu_counter counters[], int num)
> -{
> -       int i;
> -
> -       for (i = 0; i < num; i++)
> -               percpu_counter_set(&counters[i], 0);
> -}
> -
> -void nfsd_percpu_counters_destroy(struct percpu_counter counters[], int num)
> -{
> -       int i;
> -
> -       for (i = 0; i < num; i++)
> -               percpu_counter_destroy(&counters[i]);
> -}
> -
>  static int nfsd_stat_counters_init(void)
>  {
> -       return nfsd_percpu_counters_init(nfsdstats.counter, NFSD_STATS_COUNTERS_NUM);
> +       return percpu_counters_init(nfsdstats.counter, NFSD_STATS_COUNTERS_NUM,
> +                                   0, GFP_KERNEL);
>  }
>
>  static void nfsd_stat_counters_destroy(void)
>  {
> -       nfsd_percpu_counters_destroy(nfsdstats.counter, NFSD_STATS_COUNTERS_NUM);
> +       percpu_counters_destroy(nfsdstats.counter, NFSD_STATS_COUNTERS_NUM);
>  }
>
>  int nfsd_stat_init(void)
> diff --git a/fs/nfsd/stats.h b/fs/nfsd/stats.h
> index 9b43dc3d9991..61840f9035a9 100644
> --- a/fs/nfsd/stats.h
> +++ b/fs/nfsd/stats.h
> @@ -36,9 +36,6 @@ extern struct nfsd_stats      nfsdstats;
>
>  extern struct svc_stat         nfsd_svcstats;
>
> -int nfsd_percpu_counters_init(struct percpu_counter counters[], int num);
> -void nfsd_percpu_counters_reset(struct percpu_counter counters[], int num);
> -void nfsd_percpu_counters_destroy(struct percpu_counter counters[], int num);
>  int nfsd_stat_init(void);
>  void nfsd_stat_shutdown(void);
>
> diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h
> index 7f01f2e41304..37dd81c85411 100644
> --- a/include/linux/percpu_counter.h
> +++ b/include/linux/percpu_counter.h
> @@ -46,6 +46,10 @@ s64 __percpu_counter_sum(struct percpu_counter *fbc);
>  int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch);
>  void percpu_counter_sync(struct percpu_counter *fbc);
>
> +int percpu_counters_init(struct percpu_counter counters[], int num, s64 amount,
> +                        gfp_t gfp);
> +void percpu_counters_destroy(struct percpu_counter counters[], int num);
> +
>  static inline int percpu_counter_compare(struct percpu_counter *fbc, s64 rhs)
>  {
>         return __percpu_counter_compare(fbc, rhs, percpu_counter_batch);
> @@ -109,6 +113,21 @@ static inline void percpu_counter_destroy(struct percpu_counter *fbc)
>  {
>  }
>
> +static inline int percpu_counters_init(struct percpu_counter counters[],
> +                                      int num, s64 amount, gfp_t gfp)
> +{
> +       int i;
> +
> +       for (i = 0; i < num; i++)
> +               counters[i] = amount;

OOPS:

+               counters[i].count = amount;

> +       return 0;
> +}
> +
> +static inline void percpu_counters_destroy(struct percpu_counter counters[],
> +                                          int num)
> +{
> +}
> +
>  static inline void percpu_counter_set(struct percpu_counter *fbc, s64 amount)
>  {
>         fbc->count = amount;
> diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
> index ed610b75dc32..f75a45c63c18 100644
> --- a/lib/percpu_counter.c
> +++ b/lib/percpu_counter.c
> @@ -181,6 +181,33 @@ void percpu_counter_destroy(struct percpu_counter *fbc)
>  }
>  EXPORT_SYMBOL(percpu_counter_destroy);
>
> +int percpu_counters_init(struct percpu_counter counters[], int num, s64 amount,
> +                        gfp_t gfp)
> +{
> +       int i, err = 0;
> +
> +       for (i = 0; !err && i < num; i++)
> +               err = percpu_counter_init(&counters[i], amount, gfp);
> +
> +       if (!err)
> +               return 0;
> +
> +       for (; i > 0; i--)
> +               percpu_counter_destroy(&counters[i-1]);
> +
> +       return err;
> +}
> +EXPORT_SYMBOL(percpu_counters_init);
> +
> +void percpu_counters_destroy(struct percpu_counter counters[], int num)
> +{
> +       int i;
> +
> +       for (i = 0; i < num; i++)
> +               percpu_counter_destroy(&counters[i]);
> +}
> +EXPORT_SYMBOL(percpu_counters_destroy);
> +
>  int percpu_counter_batch __read_mostly = 32;
>  EXPORT_SYMBOL(percpu_counter_batch);
>
> --
> 2.25.1
>

Re: [PATCH v4 6/9] fs: report per-sb io stats

[Miklos Szeredi]

On Sat, 5 Mar 2022 at 17:04, Amir Goldstein <amir73il@gmail.com> wrote:
>
> Show optional collected per-sb io stats in /proc/<pid>/mountstats
> for filesystems that do not implement their own show_stats() method
> and have generic per-sb stats enabled.

I still think it's wrong to extend the /proc/*/mount* family of
interfaces.  The big issue is that the kernel has to generate this
info for *all* mounts, even though the user may be just looking for
information on a specific mount.  Your workaround is to enable the
info generation for only a subset of fs types, but this doesn't solve
the fundamental issue.

So let's please implement a per-mount interface.  Yes, it's a much
bigger project, but one which needs to be done at one point, and which
would be generally useful.   There was tons of discussion around this
when dhowells tried to create one, and there was a suggestion by Linus
which I think all parties found acceptable:

 - return basic info in a binary format (similar to e.g. statx)
 - after the fix binary structure allow misc info to be added using an
ascii format

And since generating the info may be expensive in some cases, the
interface would need to allow selective queries (which statx does for
binary fields, but for ascii ones this is a new challenge).

I think allowing the user to query the list of supported fields should
also be possible (again, statx supports this).

So there are a number of requirements for this interface, and I'm not
quite sure what the best solution is.   I can try to put something
together if there are no objections...

Thanks,
Miklos

Re: [PATCH v4 6/9] fs: report per-sb io stats

[Amir Goldstein]

On Thu, Mar 10, 2022 at 11:54 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sat, 5 Mar 2022 at 17:04, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Show optional collected per-sb io stats in /proc/<pid>/mountstats
> > for filesystems that do not implement their own show_stats() method
> > and have generic per-sb stats enabled.
>
> I still think it's wrong to extend the /proc/*/mount* family of
> interfaces.  The big issue is that the kernel has to generate this
> info for *all* mounts, even though the user may be just looking for
> information on a specific mount.  Your workaround is to enable the
> info generation for only a subset of fs types, but this doesn't solve
> the fundamental issue.
>
> So let's please implement a per-mount interface.  Yes, it's a much
> bigger project, but one which needs to be done at one point, and which
> would be generally useful.   There was tons of discussion around this
> when dhowells tried to create one, and there was a suggestion by Linus
> which I think all parties found acceptable:
>
>  - return basic info in a binary format (similar to e.g. statx)
>  - after the fix binary structure allow misc info to be added using an
> ascii format
>
> And since generating the info may be expensive in some cases, the
> interface would need to allow selective queries (which statx does for
> binary fields, but for ascii ones this is a new challenge).
>
> I think allowing the user to query the list of supported fields should
> also be possible (again, statx supports this).
>
> So there are a number of requirements for this interface, and I'm not
> quite sure what the best solution is.   I can try to put something
> together if there are no objections...

No objections on my part.
Feel free to use the iostat info as a case study for how common vfs info
could be exported using this interface.

Thanks,
Amir.