[PATCH 0/5 v7] xfs: stats in sysfs

[PATCH 0/5 v7] xfs: stats in sysfs

[Bill O'Donnell]

Hello-

Following is the next iteration of the series to add xfs stats to
sysfs. This iteration adds patch 5/5 to incorporate sysfs/kobject
into xfsstats.

----------history---------------
v7:
-add patch 5/5: incorporate sysfs/kobject in xfsstats: handlers
take kobjects. Allocate & deallocate per-fs stats structures
and set up the sysfs entries for them. Add kobject and a pointer
to a per-cpu struct xfsstats. Modify the macros that manipulate
the stats accordingly.

v6:
-add patch 4/4: move to_xlog(kobject) to the relevant show/store
operations. This keeps the xfs_sysfs_object_show/store functions
generic. Also, with the change, there can be some cleanup of the
show/store function arguments.

v5:
-optimization of sysfs_ops function.
-style fixups

v4:
-whitespace fixup of patch 1
-add patch 4 (sysfs ops consolidation - dbg, stats, log)

v3:
-style fixups and removal of extraneous printk.

v2:
-style fixups.

v1:
--------------------------------

The series provides infrastructure for per-fs stats (in addition
to global accumulative stats).

We already have per-fs information in /sys, so it makes sense to
have per-fs stats there too.  As a first step, moving existing
global stats infrastructure to /sys will allow us to re-use that
sysfs code for per-fs stats as well.

Patch 1 handles the bring-up and tear down of xfs/stats directory
structure in sysfs when an fs is mounted. The directory contains
the stats file and the stats_clear file. The stats file contents mimic
those of /proc/fs/xfs/stat. The stats_clear file is empty, and much
like the current stat_clear command, handles the zeroing of the stats
file when a "1" is echoed to the stats_clear file.

Patch 2 creates the symlink for stats from procfs to sysfs.

Patch 3 removes the now unused portions of procfs for stat.

Patch 4 consolidates the sysfs ops for dbg, stats, log.

Patch 5 allocates and deallocates per-fs stats structures and
sets up the sysfs entries for them. Add kobject and a pointer
to a per-cpu struct xfsstats. Modify the macros that manipulate
the stats accordingly.

Again, comments and questions are welcome.

Thanks-
Bill

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/5] xfs: create global stats and stats_clear in sysfs

[Bill O'Donnell]

Currently, xfs global stats are in procfs. This patch introduces
(replicates) the global stats in sysfs. Additionally a stats_clear file
is introduced in sysfs.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
 fs/xfs/xfs_stats.c  | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_stats.h  |  2 ++
 fs/xfs/xfs_super.c  | 20 +++++++++----
 fs/xfs/xfs_sysctl.c | 15 ++--------
 fs/xfs/xfs_sysfs.c  | 75 +++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_sysfs.h  |  1 +
 6 files changed, 179 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
index f224038..6008e25 100644
--- a/fs/xfs/xfs_stats.c
+++ b/fs/xfs/xfs_stats.c
@@ -29,6 +29,89 @@ static int counter_val(int idx)
 	return val;
 }
 
+int xfs_stats_format(char *buf)
+{
+	int		i, j;
+	int		len = 0;
+	__uint64_t	xs_xstrat_bytes = 0;
+	__uint64_t	xs_write_bytes = 0;
+	__uint64_t	xs_read_bytes = 0;
+
+	static const struct xstats_entry {
+		char	*desc;
+		int	endpoint;
+	} xstats[] = {
+		{ "extent_alloc",	XFSSTAT_END_EXTENT_ALLOC	},
+		{ "abt",		XFSSTAT_END_ALLOC_BTREE		},
+		{ "blk_map",		XFSSTAT_END_BLOCK_MAPPING	},
+		{ "bmbt",		XFSSTAT_END_BLOCK_MAP_BTREE	},
+		{ "dir",		XFSSTAT_END_DIRECTORY_OPS	},
+		{ "trans",		XFSSTAT_END_TRANSACTIONS	},
+		{ "ig",			XFSSTAT_END_INODE_OPS		},
+		{ "log",		XFSSTAT_END_LOG_OPS		},
+		{ "push_ail",		XFSSTAT_END_TAIL_PUSHING	},
+		{ "xstrat",		XFSSTAT_END_WRITE_CONVERT	},
+		{ "rw",			XFSSTAT_END_READ_WRITE_OPS	},
+		{ "attr",		XFSSTAT_END_ATTRIBUTE_OPS	},
+		{ "icluster",		XFSSTAT_END_INODE_CLUSTER	},
+		{ "vnodes",		XFSSTAT_END_VNODE_OPS		},
+		{ "buf",		XFSSTAT_END_BUF			},
+		{ "abtb2",		XFSSTAT_END_ABTB_V2		},
+		{ "abtc2",		XFSSTAT_END_ABTC_V2		},
+		{ "bmbt2",		XFSSTAT_END_BMBT_V2		},
+		{ "ibt2",		XFSSTAT_END_IBT_V2		},
+		{ "fibt2",		XFSSTAT_END_FIBT_V2		},
+		/* we print both series of quota information together */
+		{ "qm",			XFSSTAT_END_QM			},
+	};
+
+	/* Loop over all stats groups */
+
+	for (i = j = 0; i < ARRAY_SIZE(xstats); i++) {
+		len += snprintf(buf + len, PATH_MAX - len, "%s",
+				xstats[i].desc);
+		/* inner loop does each group */
+		for (; j < xstats[i].endpoint; j++)
+			len += snprintf(buf + len, PATH_MAX - len, " %u",
+					counter_val(j));
+		len += snprintf(buf + len, PATH_MAX - len, "\n");
+	}
+	/* extra precision counters */
+	for_each_possible_cpu(i) {
+		xs_xstrat_bytes += per_cpu(xfsstats, i).xs_xstrat_bytes;
+		xs_write_bytes += per_cpu(xfsstats, i).xs_write_bytes;
+		xs_read_bytes += per_cpu(xfsstats, i).xs_read_bytes;
+	}
+
+	len += snprintf(buf+len, PATH_MAX-len, "xpc %Lu %Lu %Lu\n",
+			xs_xstrat_bytes, xs_write_bytes, xs_read_bytes);
+	len += snprintf(buf+len, PATH_MAX-len, "debug %u\n",
+#if defined(DEBUG)
+		1);
+#else
+		0);
+#endif
+
+return len;
+}
+
+void xfs_stats_clearall(void)
+{
+	int		c;
+	__uint32_t	vn_active;
+
+	xfs_notice(NULL, "Clearing xfsstats");
+	for_each_possible_cpu(c) {
+		preempt_disable();
+		/* save vn_active, it's a universal truth! */
+		vn_active = per_cpu(xfsstats, c).vn_active;
+		memset(&per_cpu(xfsstats, c), 0,
+		       sizeof(struct xfsstats));
+		per_cpu(xfsstats, c).vn_active = vn_active;
+		preempt_enable();
+	}
+}
+
 static int xfs_stat_proc_show(struct seq_file *m, void *v)
 {
 	int		i, j;
diff --git a/fs/xfs/xfs_stats.h b/fs/xfs/xfs_stats.h
index c8f238b..18807b5 100644
--- a/fs/xfs/xfs_stats.h
+++ b/fs/xfs/xfs_stats.h
@@ -18,6 +18,8 @@
 #ifndef __XFS_STATS_H__
 #define __XFS_STATS_H__
 
+int xfs_stats_format(char *buf);
+void xfs_stats_clearall(void);
 
 #if defined(CONFIG_PROC_FS) && !defined(XFS_STATS_OFF)
 
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 3bf503a..31ad281 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -61,6 +61,7 @@ static kmem_zone_t *xfs_ioend_zone;
 mempool_t *xfs_ioend_pool;
 
 static struct kset *xfs_kset;		/* top-level xfs sysfs dir */
+static struct xfs_kobj xfs_stats_kobj;	/* global stats sysfs attrs */
 #ifdef DEBUG
 static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs attrs */
 #endif
@@ -1838,19 +1839,25 @@ init_xfs_fs(void)
 	xfs_kset = kset_create_and_add("xfs", NULL, fs_kobj);
 	if (!xfs_kset) {
 		error = -ENOMEM;
-		goto out_sysctl_unregister;;
+		goto out_sysctl_unregister;
 	}
 
+	xfs_stats_kobj.kobject.kset = xfs_kset;
+	error = xfs_sysfs_init(&xfs_stats_kobj, &xfs_stats_ktype, NULL,
+			       "stats");
+	if (error)
+		goto out_kset_unregister;
+
 #ifdef DEBUG
 	xfs_dbg_kobj.kobject.kset = xfs_kset;
 	error = xfs_sysfs_init(&xfs_dbg_kobj, &xfs_dbg_ktype, NULL, "debug");
 	if (error)
-		goto out_kset_unregister;
+		goto out_remove_stats_kobj;
 #endif
 
 	error = xfs_qm_init();
 	if (error)
-		goto out_remove_kobj;
+		goto out_remove_dbg_kobj;
 
 	error = register_filesystem(&xfs_fs_type);
 	if (error)
@@ -1859,11 +1866,13 @@ init_xfs_fs(void)
 
  out_qm_exit:
 	xfs_qm_exit();
- out_remove_kobj:
+ out_remove_dbg_kobj:
 #ifdef DEBUG
 	xfs_sysfs_del(&xfs_dbg_kobj);
- out_kset_unregister:
+ out_remove_stats_kobj:
 #endif
+	xfs_sysfs_del(&xfs_stats_kobj);
+ out_kset_unregister:
 	kset_unregister(xfs_kset);
  out_sysctl_unregister:
 	xfs_sysctl_unregister();
@@ -1889,6 +1898,7 @@ exit_xfs_fs(void)
 #ifdef DEBUG
 	xfs_sysfs_del(&xfs_dbg_kobj);
 #endif
+	xfs_sysfs_del(&xfs_stats_kobj);
 	kset_unregister(xfs_kset);
 	xfs_sysctl_unregister();
 	xfs_cleanup_procfs();
diff --git a/fs/xfs/xfs_sysctl.c b/fs/xfs/xfs_sysctl.c
index a0c8067..5defabb 100644
--- a/fs/xfs/xfs_sysctl.c
+++ b/fs/xfs/xfs_sysctl.c
@@ -19,6 +19,7 @@
 #include <linux/sysctl.h>
 #include <linux/proc_fs.h>
 #include "xfs_error.h"
+#include "xfs_stats.h"
 
 static struct ctl_table_header *xfs_table_header;
 
@@ -31,22 +32,12 @@ xfs_stats_clear_proc_handler(
 	size_t			*lenp,
 	loff_t			*ppos)
 {
-	int		c, ret, *valp = ctl->data;
-	__uint32_t	vn_active;
+	int		ret, *valp = ctl->data;
 
 	ret = proc_dointvec_minmax(ctl, write, buffer, lenp, ppos);
 
 	if (!ret && write && *valp) {
-		xfs_notice(NULL, "Clearing xfsstats");
-		for_each_possible_cpu(c) {
-			preempt_disable();
-			/* save vn_active, it's a universal truth! */
-			vn_active = per_cpu(xfsstats, c).vn_active;
-			memset(&per_cpu(xfsstats, c), 0,
-			       sizeof(struct xfsstats));
-			per_cpu(xfsstats, c).vn_active = vn_active;
-			preempt_enable();
-		}
+		xfs_stats_clearall();
 		xfs_stats_clear = 0;
 	}
 
diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index aa03670..a094e20 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -21,6 +21,7 @@
 #include "xfs_log_format.h"
 #include "xfs_log.h"
 #include "xfs_log_priv.h"
+#include "xfs_stats.h"
 
 struct xfs_sysfs_attr {
 	struct attribute attr;
@@ -38,6 +39,8 @@ to_attr(struct attribute *attr)
 	static struct xfs_sysfs_attr xfs_sysfs_attr_##name = __ATTR_RW(name)
 #define XFS_SYSFS_ATTR_RO(name) \
 	static struct xfs_sysfs_attr xfs_sysfs_attr_##name = __ATTR_RO(name)
+#define XFS_SYSFS_ATTR_WO(name) \
+	static struct xfs_sysfs_attr xfs_sysfs_attr_##name = __ATTR_WO(name)
 
 #define ATTR_LIST(name) &xfs_sysfs_attr_##name.attr
 
@@ -125,6 +128,78 @@ struct kobj_type xfs_dbg_ktype = {
 
 #endif /* DEBUG */
 
+
+/* stats */
+
+STATIC ssize_t
+stats_show(
+	char	*buf,
+	void	*data)
+{
+	return xfs_stats_format(buf);
+}
+XFS_SYSFS_ATTR_RO(stats);
+
+STATIC ssize_t
+stats_clear_store(
+	const char	*buf,
+	size_t		count,
+	void		*data)
+{
+	int		ret;
+	int		val;
+
+	ret = kstrtoint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	if (val != 1)
+		return -EINVAL;
+	xfs_stats_clearall();
+	return count;
+}
+XFS_SYSFS_ATTR_WO(stats_clear);
+
+static struct attribute *xfs_stats_attrs[] = {
+	ATTR_LIST(stats),
+	ATTR_LIST(stats_clear),
+	NULL,
+};
+
+STATIC ssize_t
+xfs_stats_show(
+	struct kobject		*kobject,
+	struct attribute	*attr,
+	char			*buf)
+{
+	struct xfs_sysfs_attr *xfs_attr = to_attr(attr);
+
+	return xfs_attr->show ? xfs_attr->show(buf, NULL) : 0;
+}
+
+STATIC ssize_t
+xfs_stats_store(
+	struct kobject		*kobject,
+	struct attribute	*attr,
+	const char		*buf,
+	size_t			count)
+{
+	struct xfs_sysfs_attr *xfs_attr = to_attr(attr);
+
+	return xfs_attr->store ? xfs_attr->store(buf, count, NULL) : 0;
+}
+
+static struct sysfs_ops xfs_stats_ops = {
+	.show = xfs_stats_show,
+	.store = xfs_stats_store,
+};
+
+struct kobj_type xfs_stats_ktype = {
+	.release = xfs_sysfs_release,
+	.sysfs_ops = &xfs_stats_ops,
+	.default_attrs = xfs_stats_attrs,
+};
+
 /* xlog */
 
 STATIC ssize_t
diff --git a/fs/xfs/xfs_sysfs.h b/fs/xfs/xfs_sysfs.h
index 240eee3..be692e5 100644
--- a/fs/xfs/xfs_sysfs.h
+++ b/fs/xfs/xfs_sysfs.h
@@ -22,6 +22,7 @@
 extern struct kobj_type xfs_mp_ktype;	/* xfs_mount */
 extern struct kobj_type xfs_dbg_ktype;	/* debug */
 extern struct kobj_type xfs_log_ktype;	/* xlog */
+extern struct kobj_type xfs_stats_ktype;	/* stats */
 
 static inline struct xfs_kobj *
 to_kobj(struct kobject *kobject)
-- 
2.4.3

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/5] xfs: create symlink proc/fs/xfs/stat to sys/fs/xfs/stats

[Bill O'Donnell]

As a part of the work to move xfs global stats from procfs to sysfs,
this patch creates the symlink from proc/fs/xfs/stat to sys/fs/xfs/stats.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
 fs/xfs/xfs_stats.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
index 6008e25..a9f05de 100644
--- a/fs/xfs/xfs_stats.c
+++ b/fs/xfs/xfs_stats.c
@@ -244,9 +244,10 @@ xfs_init_procfs(void)
 	if (!proc_mkdir("fs/xfs", NULL))
 		goto out;
 
-	if (!proc_create("fs/xfs/stat", 0, NULL,
-			 &xfs_stat_proc_fops))
+	if (!proc_symlink("fs/xfs/stat", NULL,
+			  "/sys/fs/xfs/stats/stats"))
 		goto out_remove_xfs_dir;
+
 #ifdef CONFIG_XFS_QUOTA
 	if (!proc_create("fs/xfs/xqmstat", 0, NULL,
 			 &xqmstat_proc_fops))
-- 
2.4.3

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/5] xfs: remove unused procfs code

[Bill O'Donnell]

As a part of the work to move xfs global stats from procfs to sysfs,
this patch removes the now unused procfs code that was xfs stat specific.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
 fs/xfs/xfs_stats.c | 74 ------------------------------------------------------
 1 file changed, 74 deletions(-)

diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
index a9f05de..05d5227 100644
--- a/fs/xfs/xfs_stats.c
+++ b/fs/xfs/xfs_stats.c
@@ -112,80 +112,6 @@ void xfs_stats_clearall(void)
 	}
 }
 
-static int xfs_stat_proc_show(struct seq_file *m, void *v)
-{
-	int		i, j;
-	__uint64_t	xs_xstrat_bytes = 0;
-	__uint64_t	xs_write_bytes = 0;
-	__uint64_t	xs_read_bytes = 0;
-
-	static const struct xstats_entry {
-		char	*desc;
-		int	endpoint;
-	} xstats[] = {
-		{ "extent_alloc",	XFSSTAT_END_EXTENT_ALLOC	},
-		{ "abt",		XFSSTAT_END_ALLOC_BTREE		},
-		{ "blk_map",		XFSSTAT_END_BLOCK_MAPPING	},
-		{ "bmbt",		XFSSTAT_END_BLOCK_MAP_BTREE	},
-		{ "dir",		XFSSTAT_END_DIRECTORY_OPS	},
-		{ "trans",		XFSSTAT_END_TRANSACTIONS	},
-		{ "ig",			XFSSTAT_END_INODE_OPS		},
-		{ "log",		XFSSTAT_END_LOG_OPS		},
-		{ "push_ail",		XFSSTAT_END_TAIL_PUSHING	},
-		{ "xstrat",		XFSSTAT_END_WRITE_CONVERT	},
-		{ "rw",			XFSSTAT_END_READ_WRITE_OPS	},
-		{ "attr",		XFSSTAT_END_ATTRIBUTE_OPS	},
-		{ "icluster",		XFSSTAT_END_INODE_CLUSTER	},
-		{ "vnodes",		XFSSTAT_END_VNODE_OPS		},
-		{ "buf",		XFSSTAT_END_BUF			},
-		{ "abtb2",		XFSSTAT_END_ABTB_V2		},
-		{ "abtc2",		XFSSTAT_END_ABTC_V2		},
-		{ "bmbt2",		XFSSTAT_END_BMBT_V2		},
-		{ "ibt2",		XFSSTAT_END_IBT_V2		},
-		{ "fibt2",		XFSSTAT_END_FIBT_V2		},
-		/* we print both series of quota information together */
-		{ "qm",			XFSSTAT_END_QM			},
-	};
-
-	/* Loop over all stats groups */
-	for (i = j = 0; i < ARRAY_SIZE(xstats); i++) {
-		seq_printf(m, "%s", xstats[i].desc);
-		/* inner loop does each group */
-		for (; j < xstats[i].endpoint; j++)
-			seq_printf(m, " %u", counter_val(j));
-		seq_putc(m, '\n');
-	}
-	/* extra precision counters */
-	for_each_possible_cpu(i) {
-		xs_xstrat_bytes += per_cpu(xfsstats, i).xs_xstrat_bytes;
-		xs_write_bytes += per_cpu(xfsstats, i).xs_write_bytes;
-		xs_read_bytes += per_cpu(xfsstats, i).xs_read_bytes;
-	}
-
-	seq_printf(m, "xpc %Lu %Lu %Lu\n",
-			xs_xstrat_bytes, xs_write_bytes, xs_read_bytes);
-	seq_printf(m, "debug %u\n",
-#if defined(DEBUG)
-		1);
-#else
-		0);
-#endif
-	return 0;
-}
-
-static int xfs_stat_proc_open(struct inode *inode, struct file *file)
-{
-	return single_open(file, xfs_stat_proc_show, NULL);
-}
-
-static const struct file_operations xfs_stat_proc_fops = {
-	.owner		= THIS_MODULE,
-	.open		= xfs_stat_proc_open,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= single_release,
-};
-
 /* legacy quota interfaces */
 #ifdef CONFIG_XFS_QUOTA
 static int xqm_proc_show(struct seq_file *m, void *v)
-- 
2.4.3

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 4/5] xfs: consolidate sysfs ops (dbg, stats, log)

[Bill O'Donnell]

As a part of the series to move xfs global stats from procfs to sysfs,
this patch consolidates the sysfs ops functions and removes redundancy.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
 fs/xfs/xfs_stats.c |   2 +-
 fs/xfs/xfs_sysfs.c | 182 +++++++++++++++++++----------------------------------
 2 files changed, 64 insertions(+), 120 deletions(-)

diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
index 05d5227..dc6ca67 100644
--- a/fs/xfs/xfs_stats.c
+++ b/fs/xfs/xfs_stats.c
@@ -92,7 +92,7 @@ int xfs_stats_format(char *buf)
 		0);
 #endif
 
-return len;
+	return len;
 }
 
 void xfs_stats_clearall(void)
diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index a094e20..ab4850b 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -25,8 +25,9 @@
 
 struct xfs_sysfs_attr {
 	struct attribute attr;
-	ssize_t (*show)(char *buf, void *data);
-	ssize_t (*store)(const char *buf, size_t count, void *data);
+	ssize_t (*show)(struct kobject *kobject, char *buf);
+	ssize_t (*store)(struct kobject *kobject, const char *buf,
+			 size_t count);
 };
 
 static inline struct xfs_sysfs_attr *
@@ -54,14 +55,42 @@ struct kobj_type xfs_mp_ktype = {
 	.release = xfs_sysfs_release,
 };
 
+STATIC ssize_t
+xfs_sysfs_object_show(
+	struct kobject		*kobject,
+	struct attribute	*attr,
+	char			*buf)
+{
+	struct xfs_sysfs_attr *xfs_attr = to_attr(attr);
+
+	return xfs_attr->show ? xfs_attr->show(kobject, buf) : 0;
+}
+
+STATIC ssize_t
+xfs_sysfs_object_store(
+	struct kobject		*kobject,
+	struct attribute	*attr,
+	const char		*buf,
+	size_t			count)
+{
+	struct xfs_sysfs_attr *xfs_attr = to_attr(attr);
+
+	return xfs_attr->store ? xfs_attr->store(kobject, buf, count) : 0;
+}
+
+static const struct sysfs_ops xfs_sysfs_ops = {
+	.show = xfs_sysfs_object_show,
+	.store = xfs_sysfs_object_store,
+};
+
 #ifdef DEBUG
 /* debug */
 
 STATIC ssize_t
 log_recovery_delay_store(
+	struct kobject	*kobject,
 	const char	*buf,
-	size_t		count,
-	void		*data)
+	size_t		count)
 {
 	int		ret;
 	int		val;
@@ -80,8 +109,8 @@ log_recovery_delay_store(
 
 STATIC ssize_t
 log_recovery_delay_show(
-	char	*buf,
-	void	*data)
+	struct kobject	*kobject,
+	char	*buf)
 {
 	return snprintf(buf, PAGE_SIZE, "%d\n", xfs_globals.log_recovery_delay);
 }
@@ -92,49 +121,20 @@ static struct attribute *xfs_dbg_attrs[] = {
 	NULL,
 };
 
-STATIC ssize_t
-xfs_dbg_show(
-	struct kobject		*kobject,
-	struct attribute	*attr,
-	char			*buf)
-{
-	struct xfs_sysfs_attr *xfs_attr = to_attr(attr);
-
-	return xfs_attr->show ? xfs_attr->show(buf, NULL) : 0;
-}
-
-STATIC ssize_t
-xfs_dbg_store(
-	struct kobject		*kobject,
-	struct attribute	*attr,
-	const char		*buf,
-	size_t			count)
-{
-	struct xfs_sysfs_attr *xfs_attr = to_attr(attr);
-
-	return xfs_attr->store ? xfs_attr->store(buf, count, NULL) : 0;
-}
-
-static struct sysfs_ops xfs_dbg_ops = {
-	.show = xfs_dbg_show,
-	.store = xfs_dbg_store,
-};
-
 struct kobj_type xfs_dbg_ktype = {
 	.release = xfs_sysfs_release,
-	.sysfs_ops = &xfs_dbg_ops,
+	.sysfs_ops = &xfs_sysfs_ops,
 	.default_attrs = xfs_dbg_attrs,
 };
 
 #endif /* DEBUG */
 
-
 /* stats */
 
 STATIC ssize_t
 stats_show(
-	char	*buf,
-	void	*data)
+	struct kobject	*kobject,
+	char	*buf)
 {
 	return xfs_stats_format(buf);
 }
@@ -142,9 +142,9 @@ XFS_SYSFS_ATTR_RO(stats);
 
 STATIC ssize_t
 stats_clear_store(
+	struct kobject	*kobject,
 	const char	*buf,
-	size_t		count,
-	void		*data)
+	size_t		count)
 {
 	int		ret;
 	int		val;
@@ -166,50 +166,30 @@ static struct attribute *xfs_stats_attrs[] = {
 	NULL,
 };
 
-STATIC ssize_t
-xfs_stats_show(
-	struct kobject		*kobject,
-	struct attribute	*attr,
-	char			*buf)
-{
-	struct xfs_sysfs_attr *xfs_attr = to_attr(attr);
-
-	return xfs_attr->show ? xfs_attr->show(buf, NULL) : 0;
-}
-
-STATIC ssize_t
-xfs_stats_store(
-	struct kobject		*kobject,
-	struct attribute	*attr,
-	const char		*buf,
-	size_t			count)
-{
-	struct xfs_sysfs_attr *xfs_attr = to_attr(attr);
-
-	return xfs_attr->store ? xfs_attr->store(buf, count, NULL) : 0;
-}
-
-static struct sysfs_ops xfs_stats_ops = {
-	.show = xfs_stats_show,
-	.store = xfs_stats_store,
-};
-
 struct kobj_type xfs_stats_ktype = {
 	.release = xfs_sysfs_release,
-	.sysfs_ops = &xfs_stats_ops,
+	.sysfs_ops = &xfs_sysfs_ops,
 	.default_attrs = xfs_stats_attrs,
 };
 
 /* xlog */
 
+static inline struct xlog *
+to_xlog(struct kobject *kobject)
+{
+	struct xfs_kobj *kobj = to_kobj(kobject);
+
+	return container_of(kobj, struct xlog, l_kobj);
+}
+
 STATIC ssize_t
 log_head_lsn_show(
-	char	*buf,
-	void	*data)
+	struct kobject	*kobject,
+	char	*buf)
 {
-	struct xlog *log = data;
 	int cycle;
 	int block;
+	struct xlog *log = to_xlog(kobject);
 
 	spin_lock(&log->l_icloglock);
 	cycle = log->l_curr_cycle;
@@ -222,12 +202,12 @@ XFS_SYSFS_ATTR_RO(log_head_lsn);
 
 STATIC ssize_t
 log_tail_lsn_show(
-	char	*buf,
-	void	*data)
+	struct kobject	*kobject,
+	char	*buf)
 {
-	struct xlog *log = data;
 	int cycle;
 	int block;
+	struct xlog *log = to_xlog(kobject);
 
 	xlog_crack_atomic_lsn(&log->l_tail_lsn, &cycle, &block);
 	return snprintf(buf, PAGE_SIZE, "%d:%d\n", cycle, block);
@@ -236,12 +216,13 @@ XFS_SYSFS_ATTR_RO(log_tail_lsn);
 
 STATIC ssize_t
 reserve_grant_head_show(
-	char	*buf,
-	void	*data)
+	struct kobject	*kobject,
+	char	*buf)
+
 {
-	struct xlog *log = data;
 	int cycle;
 	int bytes;
+	struct xlog *log = to_xlog(kobject);
 
 	xlog_crack_grant_head(&log->l_reserve_head.grant, &cycle, &bytes);
 	return snprintf(buf, PAGE_SIZE, "%d:%d\n", cycle, bytes);
@@ -250,12 +231,12 @@ XFS_SYSFS_ATTR_RO(reserve_grant_head);
 
 STATIC ssize_t
 write_grant_head_show(
-	char	*buf,
-	void	*data)
+	struct kobject	*kobject,
+	char	*buf)
 {
-	struct xlog *log = data;
 	int cycle;
 	int bytes;
+	struct xlog *log = to_xlog(kobject);
 
 	xlog_crack_grant_head(&log->l_write_head.grant, &cycle, &bytes);
 	return snprintf(buf, PAGE_SIZE, "%d:%d\n", cycle, bytes);
@@ -270,45 +251,8 @@ static struct attribute *xfs_log_attrs[] = {
 	NULL,
 };
 
-static inline struct xlog *
-to_xlog(struct kobject *kobject)
-{
-	struct xfs_kobj *kobj = to_kobj(kobject);
-	return container_of(kobj, struct xlog, l_kobj);
-}
-
-STATIC ssize_t
-xfs_log_show(
-	struct kobject		*kobject,
-	struct attribute	*attr,
-	char			*buf)
-{
-	struct xlog *log = to_xlog(kobject);
-	struct xfs_sysfs_attr *xfs_attr = to_attr(attr);
-
-	return xfs_attr->show ? xfs_attr->show(buf, log) : 0;
-}
-
-STATIC ssize_t
-xfs_log_store(
-	struct kobject		*kobject,
-	struct attribute	*attr,
-	const char		*buf,
-	size_t			count)
-{
-	struct xlog *log = to_xlog(kobject);
-	struct xfs_sysfs_attr *xfs_attr = to_attr(attr);
-
-	return xfs_attr->store ? xfs_attr->store(buf, count, log) : 0;
-}
-
-static struct sysfs_ops xfs_log_ops = {
-	.show = xfs_log_show,
-	.store = xfs_log_store,
-};
-
 struct kobj_type xfs_log_ktype = {
 	.release = xfs_sysfs_release,
-	.sysfs_ops = &xfs_log_ops,
+	.sysfs_ops = &xfs_sysfs_ops,
 	.default_attrs = xfs_log_attrs,
 };
-- 
2.4.3

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 5/5] xfs: incorporate sysfs/kobject in xfsstats: handlers take kobjects.

[Bill O'Donnell]

This patch is the next step toward per-fs xfs stats. Allocate &
deallocate per-fs stats structures and set up the sysfs entries for them.

Instead of a single global xfsstats structure, add kobject and a pointer
to a per-cpu struct xfsstats. Modify the macros that manipulate the stats
accordingly: XFS_STATS_INC, XFS_STATS_DEC, and XFS_STATS_ADD now access
xfsstats->xs_stats.

The sysfs functions need to get from the kobject back to the xfsstats
structure which contains it, and pass the pointer to the ->xs_stats
percpu structure into the show & clear routines.


Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
 fs/xfs/xfs_linux.h  |  7 +++++++
 fs/xfs/xfs_stats.c  | 47 ++++++++++++++++++++++++-----------------------
 fs/xfs/xfs_stats.h  | 13 ++++++-------
 fs/xfs/xfs_super.c  | 15 ++++++++-------
 fs/xfs/xfs_sysctl.c |  2 +-
 fs/xfs/xfs_sysfs.c  | 17 +++++++++++++++--
 6 files changed, 61 insertions(+), 40 deletions(-)

diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index 85f883d..f1a8505 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -171,6 +171,13 @@ struct xfs_kobj {
 	struct completion	complete;
 };
 
+struct xstats {
+	struct xfsstats __percpu *xs_stats;
+	struct xfs_kobj         xs_kobj;
+};
+
+extern struct xstats xfsstats;
+
 /* Kernel uid/gid conversion. These are used to convert to/from the on disk
  * uid_t/gid_t types to the kuid_t/kgid_t types that the kernel uses internally.
  * The conversion here is type only, the value will remain the same since we
diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
index dc6ca67..ab79973 100644
--- a/fs/xfs/xfs_stats.c
+++ b/fs/xfs/xfs_stats.c
@@ -18,18 +18,18 @@
 #include "xfs.h"
 #include <linux/proc_fs.h>
 
-DEFINE_PER_CPU(struct xfsstats, xfsstats);
+struct xstats xfsstats;
 
-static int counter_val(int idx)
+static int counter_val(struct xfsstats __percpu *stats, int idx)
 {
 	int val = 0, cpu;
 
 	for_each_possible_cpu(cpu)
-		val += *(((__u32 *)&per_cpu(xfsstats, cpu) + idx));
+		val += *(((__u32 *)&per_cpu(*stats, cpu) + idx));
 	return val;
 }
 
-int xfs_stats_format(char *buf)
+int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
 {
 	int		i, j;
 	int		len = 0;
@@ -73,14 +73,14 @@ int xfs_stats_format(char *buf)
 		/* inner loop does each group */
 		for (; j < xstats[i].endpoint; j++)
 			len += snprintf(buf + len, PATH_MAX - len, " %u",
-					counter_val(j));
+					counter_val(stats, j));
 		len += snprintf(buf + len, PATH_MAX - len, "\n");
 	}
 	/* extra precision counters */
 	for_each_possible_cpu(i) {
-		xs_xstrat_bytes += per_cpu(xfsstats, i).xs_xstrat_bytes;
-		xs_write_bytes += per_cpu(xfsstats, i).xs_write_bytes;
-		xs_read_bytes += per_cpu(xfsstats, i).xs_read_bytes;
+		xs_xstrat_bytes += per_cpu(*stats, i).xs_xstrat_bytes;
+		xs_write_bytes += per_cpu(*stats, i).xs_write_bytes;
+		xs_read_bytes += per_cpu(*stats, i).xs_read_bytes;
 	}
 
 	len += snprintf(buf+len, PATH_MAX-len, "xpc %Lu %Lu %Lu\n",
@@ -95,7 +95,7 @@ int xfs_stats_format(char *buf)
 	return len;
 }
 
-void xfs_stats_clearall(void)
+void xfs_stats_clearall(struct xfsstats __percpu *stats)
 {
 	int		c;
 	__uint32_t	vn_active;
@@ -104,10 +104,10 @@ void xfs_stats_clearall(void)
 	for_each_possible_cpu(c) {
 		preempt_disable();
 		/* save vn_active, it's a universal truth! */
-		vn_active = per_cpu(xfsstats, c).vn_active;
-		memset(&per_cpu(xfsstats, c), 0,
-		       sizeof(struct xfsstats));
-		per_cpu(xfsstats, c).vn_active = vn_active;
+		vn_active = per_cpu(*stats, c).vn_active;
+		memset(&per_cpu(*stats, c), 0,
+		       sizeof(*stats));
+		per_cpu(*stats, c).vn_active = vn_active;
 		preempt_enable();
 	}
 }
@@ -119,9 +119,10 @@ static int xqm_proc_show(struct seq_file *m, void *v)
 	/* maximum; incore; ratio free to inuse; freelist */
 	seq_printf(m, "%d\t%d\t%d\t%u\n",
 			0,
-			counter_val(XFSSTAT_END_XQMSTAT),
+			counter_val(xfsstats.xs_stats, XFSSTAT_END_XQMSTAT),
 			0,
-			counter_val(XFSSTAT_END_XQMSTAT + 1));
+			counter_val(xfsstats.xs_stats,
+				XFSSTAT_END_XQMSTAT + 1));
 	return 0;
 }
 
@@ -145,7 +146,7 @@ static int xqmstat_proc_show(struct seq_file *m, void *v)
 
 	seq_printf(m, "qm");
 	for (j = XFSSTAT_END_IBT_V2; j < XFSSTAT_END_XQMSTAT; j++)
-		seq_printf(m, " %u", counter_val(j));
+		seq_printf(m, " %u", counter_val(xfsstats.xs_stats, j));
 	seq_putc(m, '\n');
 	return 0;
 }
@@ -171,28 +172,28 @@ xfs_init_procfs(void)
 		goto out;
 
 	if (!proc_symlink("fs/xfs/stat", NULL,
-			  "/sys/fs/xfs/stats/stats"))
+			"/sys/fs/xfs/stats/stats"))
 		goto out_remove_xfs_dir;
 
 #ifdef CONFIG_XFS_QUOTA
 	if (!proc_create("fs/xfs/xqmstat", 0, NULL,
-			 &xqmstat_proc_fops))
+			&xqmstat_proc_fops))
 		goto out_remove_stat_file;
 	if (!proc_create("fs/xfs/xqm", 0, NULL,
-			 &xqm_proc_fops))
+			&xqm_proc_fops))
 		goto out_remove_xqmstat_file;
 #endif
 	return 0;
 
 #ifdef CONFIG_XFS_QUOTA
- out_remove_xqmstat_file:
+out_remove_xqmstat_file:
 	remove_proc_entry("fs/xfs/xqmstat", NULL);
- out_remove_stat_file:
+out_remove_stat_file:
 	remove_proc_entry("fs/xfs/stat", NULL);
 #endif
- out_remove_xfs_dir:
+out_remove_xfs_dir:
 	remove_proc_entry("fs/xfs", NULL);
- out:
+out:
 	return -ENOMEM;
 }
 
diff --git a/fs/xfs/xfs_stats.h b/fs/xfs/xfs_stats.h
index 18807b5..672fe83 100644
--- a/fs/xfs/xfs_stats.h
+++ b/fs/xfs/xfs_stats.h
@@ -18,9 +18,6 @@
 #ifndef __XFS_STATS_H__
 #define __XFS_STATS_H__
 
-int xfs_stats_format(char *buf);
-void xfs_stats_clearall(void);
-
 #if defined(CONFIG_PROC_FS) && !defined(XFS_STATS_OFF)
 
 #include <linux/percpu.h>
@@ -217,15 +214,17 @@ struct xfsstats {
 	__uint64_t		xs_read_bytes;
 };
 
-DECLARE_PER_CPU(struct xfsstats, xfsstats);
+int xfs_stats_format(struct xfsstats __percpu *stats, char *buf);
+void xfs_stats_clearall(struct xfsstats __percpu *stats);
+extern struct xstats xfsstats;
 
 /*
  * We don't disable preempt, not too worried about poking the
  * wrong CPU's stat for now (also aggregated before reporting).
  */
-#define XFS_STATS_INC(v)	(per_cpu(xfsstats, current_cpu()).v++)
-#define XFS_STATS_DEC(v)	(per_cpu(xfsstats, current_cpu()).v--)
-#define XFS_STATS_ADD(v, inc)	(per_cpu(xfsstats, current_cpu()).v += (inc))
+#define XFS_STATS_INC(v)	(per_cpu(*xfsstats.xs_stats, current_cpu()).v++)
+#define XFS_STATS_DEC(v)	(per_cpu(*xfsstats.xs_stats, current_cpu()).v--)
+#define XFS_STATS_ADD(v, inc)	(per_cpu(*xfsstats.xs_stats, current_cpu()).v += (inc))
 
 extern int xfs_init_procfs(void);
 extern void xfs_cleanup_procfs(void);
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 31ad281..f1ea1c9 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -61,7 +61,6 @@ static kmem_zone_t *xfs_ioend_zone;
 mempool_t *xfs_ioend_pool;
 
 static struct kset *xfs_kset;		/* top-level xfs sysfs dir */
-static struct xfs_kobj xfs_stats_kobj;	/* global stats sysfs attrs */
 #ifdef DEBUG
 static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs attrs */
 #endif
@@ -1802,6 +1801,7 @@ xfs_destroy_workqueues(void)
 	destroy_workqueue(xfs_alloc_wq);
 }
 
+
 STATIC int __init
 init_xfs_fs(void)
 {
@@ -1842,8 +1842,9 @@ init_xfs_fs(void)
 		goto out_sysctl_unregister;
 	}
 
-	xfs_stats_kobj.kobject.kset = xfs_kset;
-	error = xfs_sysfs_init(&xfs_stats_kobj, &xfs_stats_ktype, NULL,
+	xfsstats.xs_stats = alloc_percpu(struct xfsstats);
+	xfsstats.xs_kobj.kobject.kset = xfs_kset;
+	error = xfs_sysfs_init(&xfsstats.xs_kobj, &xfs_stats_ktype, NULL,
 			       "stats");
 	if (error)
 		goto out_kset_unregister;
@@ -1852,7 +1853,7 @@ init_xfs_fs(void)
 	xfs_dbg_kobj.kobject.kset = xfs_kset;
 	error = xfs_sysfs_init(&xfs_dbg_kobj, &xfs_dbg_ktype, NULL, "debug");
 	if (error)
-		goto out_remove_stats_kobj;
+		goto out_remove_stats;
 #endif
 
 	error = xfs_qm_init();
@@ -1869,9 +1870,9 @@ init_xfs_fs(void)
  out_remove_dbg_kobj:
 #ifdef DEBUG
 	xfs_sysfs_del(&xfs_dbg_kobj);
- out_remove_stats_kobj:
+ out_remove_stats:
 #endif
-	xfs_sysfs_del(&xfs_stats_kobj);
+	free_percpu(xfsstats.xs_stats);
  out_kset_unregister:
 	kset_unregister(xfs_kset);
  out_sysctl_unregister:
@@ -1898,7 +1899,7 @@ exit_xfs_fs(void)
 #ifdef DEBUG
 	xfs_sysfs_del(&xfs_dbg_kobj);
 #endif
-	xfs_sysfs_del(&xfs_stats_kobj);
+	free_percpu(xfsstats.xs_stats);
 	kset_unregister(xfs_kset);
 	xfs_sysctl_unregister();
 	xfs_cleanup_procfs();
diff --git a/fs/xfs/xfs_sysctl.c b/fs/xfs/xfs_sysctl.c
index 5defabb..aed74d3 100644
--- a/fs/xfs/xfs_sysctl.c
+++ b/fs/xfs/xfs_sysctl.c
@@ -37,7 +37,7 @@ xfs_stats_clear_proc_handler(
 	ret = proc_dointvec_minmax(ctl, write, buffer, lenp, ppos);
 
 	if (!ret && write && *valp) {
-		xfs_stats_clearall();
+		xfs_stats_clearall(xfsstats.xs_stats);
 		xfs_stats_clear = 0;
 	}
 
diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
index ab4850b..3275408 100644
--- a/fs/xfs/xfs_sysfs.c
+++ b/fs/xfs/xfs_sysfs.c
@@ -131,12 +131,23 @@ struct kobj_type xfs_dbg_ktype = {
 
 /* stats */
 
+static inline struct xstats *
+to_xstats(struct kobject *kobject)
+{
+	struct xfs_kobj *kobj = to_kobj(kobject);
+
+	return container_of(kobj, struct xstats, xs_kobj);
+}
+
+
 STATIC ssize_t
 stats_show(
 	struct kobject	*kobject,
 	char	*buf)
 {
-	return xfs_stats_format(buf);
+	struct xstats *stats = to_xstats(kobject);
+
+	return xfs_stats_format(stats->xs_stats, buf);
 }
 XFS_SYSFS_ATTR_RO(stats);
 
@@ -148,6 +159,7 @@ stats_clear_store(
 {
 	int		ret;
 	int		val;
+	struct xstats *stats = to_xstats(kobject);
 
 	ret = kstrtoint(buf, 0, &val);
 	if (ret)
@@ -155,7 +167,8 @@ stats_clear_store(
 
 	if (val != 1)
 		return -EINVAL;
-	xfs_stats_clearall();
+
+	xfs_stats_clearall(stats->xs_stats);
 	return count;
 }
 XFS_SYSFS_ATTR_WO(stats_clear);
-- 
2.4.3

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/5] xfs: incorporate sysfs/kobject in xfsstats: handlers take kobjects.

[Eric Sandeen]

On 9/22/15 7:07 AM, Bill O'Donnell wrote:
> This patch is the next step toward per-fs xfs stats. Allocate &
> deallocate per-fs stats structures and set up the sysfs entries for them.

Ok, the patch doesn't actually do that last sentence yet, so
I'd leave it out of the description.  :)

You could mumble about how this enables the next patch, though, i.e.
this makes the show & clear routines able to handle any stats structure
associated with a kobject.

> Instead of a single global xfsstats structure, add kobject and a pointer
> to a per-cpu struct xfsstats. Modify the macros that manipulate the stats
> accordingly: XFS_STATS_INC, XFS_STATS_DEC, and XFS_STATS_ADD now access
> xfsstats->xs_stats.
> 
> The sysfs functions need to get from the kobject back to the xfsstats
> structure which contains it, and pass the pointer to the ->xs_stats
> percpu structure into the show & clear routines.
> 
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
>  fs/xfs/xfs_linux.h  |  7 +++++++
>  fs/xfs/xfs_stats.c  | 47 ++++++++++++++++++++++++-----------------------
>  fs/xfs/xfs_stats.h  | 13 ++++++-------
>  fs/xfs/xfs_super.c  | 15 ++++++++-------
>  fs/xfs/xfs_sysctl.c |  2 +-
>  fs/xfs/xfs_sysfs.c  | 17 +++++++++++++++--
>  6 files changed, 61 insertions(+), 40 deletions(-)
> 
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index 85f883d..f1a8505 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -171,6 +171,13 @@ struct xfs_kobj {
>  	struct completion	complete;
>  };
>  
> +struct xstats {
> +	struct xfsstats __percpu *xs_stats;
> +	struct xfs_kobj         xs_kobj;

nitpick, I'd fiddle w/ whitespace a little to make the 2 members line
up nicely.

> +};
> +
> +extern struct xstats xfsstats;
> +
>  /* Kernel uid/gid conversion. These are used to convert to/from the on disk
>   * uid_t/gid_t types to the kuid_t/kgid_t types that the kernel uses internally.
>   * The conversion here is type only, the value will remain the same since we
> diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
> index dc6ca67..ab79973 100644
> --- a/fs/xfs/xfs_stats.c
> +++ b/fs/xfs/xfs_stats.c
> @@ -18,18 +18,18 @@
>  #include "xfs.h"
>  #include <linux/proc_fs.h>
>  
> -DEFINE_PER_CPU(struct xfsstats, xfsstats);
> +struct xstats xfsstats;
>  
> -static int counter_val(int idx)
> +static int counter_val(struct xfsstats __percpu *stats, int idx)
>  {
>  	int val = 0, cpu;
>  
>  	for_each_possible_cpu(cpu)
> -		val += *(((__u32 *)&per_cpu(xfsstats, cpu) + idx));
> +		val += *(((__u32 *)&per_cpu(*stats, cpu) + idx));

Now that we are referring to a pointer to a percpu structure
(instead of the old global xfsstats structure itself), I think it's
a lot cleaner & more legible to do:

+		val += *(((__u32 *)per_cpu_ptr(stats, cpu) + idx));

(ok the cast gyrations may not qualify as "clean and legible" but it
helps - see also below)

>  	return val;
>  }
>  
> -int xfs_stats_format(char *buf)
> +int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
>  {
>  	int		i, j;
>  	int		len = 0;
> @@ -73,14 +73,14 @@ int xfs_stats_format(char *buf)
>  		/* inner loop does each group */
>  		for (; j < xstats[i].endpoint; j++)
>  			len += snprintf(buf + len, PATH_MAX - len, " %u",
> -					counter_val(j));
> +					counter_val(stats, j));
>  		len += snprintf(buf + len, PATH_MAX - len, "\n");
>  	}
>  	/* extra precision counters */
>  	for_each_possible_cpu(i) {
> -		xs_xstrat_bytes += per_cpu(xfsstats, i).xs_xstrat_bytes;
> -		xs_write_bytes += per_cpu(xfsstats, i).xs_write_bytes;
> -		xs_read_bytes += per_cpu(xfsstats, i).xs_read_bytes;
> +		xs_xstrat_bytes += per_cpu(*stats, i).xs_xstrat_bytes;
> +		xs_write_bytes += per_cpu(*stats, i).xs_write_bytes;
> +		xs_read_bytes += per_cpu(*stats, i).xs_read_bytes;

ditto here,

+		xs_xstrat_bytes += per_cpu_ptr(stats, i)->xs_xstrat_bytes;
+		xs_write_bytes += per_cpu_ptr(stats, i)->xs_write_bytes;
+		xs_read_bytes += per_cpu_ptr(stats, i)->xs_read_bytes;

>  	}
>  
>  	len += snprintf(buf+len, PATH_MAX-len, "xpc %Lu %Lu %Lu\n",
> @@ -95,7 +95,7 @@ int xfs_stats_format(char *buf)
>  	return len;
>  }
>  
> -void xfs_stats_clearall(void)
> +void xfs_stats_clearall(struct xfsstats __percpu *stats)
>  {
>  	int		c;
>  	__uint32_t	vn_active;
> @@ -104,10 +104,10 @@ void xfs_stats_clearall(void)
>  	for_each_possible_cpu(c) {
>  		preempt_disable();
>  		/* save vn_active, it's a universal truth! */
> -		vn_active = per_cpu(xfsstats, c).vn_active;
> -		memset(&per_cpu(xfsstats, c), 0,
> -		       sizeof(struct xfsstats));
> -		per_cpu(xfsstats, c).vn_active = vn_active;
> +		vn_active = per_cpu(*stats, c).vn_active;
> +		memset(&per_cpu(*stats, c), 0,
> +		       sizeof(*stats));
> +		per_cpu(*stats, c).vn_active = vn_active;
>  		preempt_enable();
>  	}
>  }


+		vn_active = per_cpu_ptr(stats, c)->vn_active;
+		memset(per_cpu_ptr(stats, c), 0, sizeof(*stats));
+		per_cpu_ptr(stats, c)->vn_active = vn_active;


> @@ -119,9 +119,10 @@ static int xqm_proc_show(struct seq_file *m, void *v)
>  	/* maximum; incore; ratio free to inuse; freelist */
>  	seq_printf(m, "%d\t%d\t%d\t%u\n",
>  			0,
> -			counter_val(XFSSTAT_END_XQMSTAT),
> +			counter_val(xfsstats.xs_stats, XFSSTAT_END_XQMSTAT),
>  			0,
> -			counter_val(XFSSTAT_END_XQMSTAT + 1));
> +			counter_val(xfsstats.xs_stats,
> +				XFSSTAT_END_XQMSTAT + 1));

might be able to make this fit on one line by lining up the "0"
with the "%d" but *shrug* now i'm really being stupidly nitpicky.

>  	return 0;
>  }
>  
> @@ -145,7 +146,7 @@ static int xqmstat_proc_show(struct seq_file *m, void *v)
>  
>  	seq_printf(m, "qm");
>  	for (j = XFSSTAT_END_IBT_V2; j < XFSSTAT_END_XQMSTAT; j++)
> -		seq_printf(m, " %u", counter_val(j));
> +		seq_printf(m, " %u", counter_val(xfsstats.xs_stats, j));
>  	seq_putc(m, '\n');
>  	return 0;
>  }
> @@ -171,28 +172,28 @@ xfs_init_procfs(void)
>  		goto out;
>  
>  	if (!proc_symlink("fs/xfs/stat", NULL,
> -			  "/sys/fs/xfs/stats/stats"))
> +			"/sys/fs/xfs/stats/stats"))

intentional whitespace changes?

>  		goto out_remove_xfs_dir;
>  
>  #ifdef CONFIG_XFS_QUOTA
>  	if (!proc_create("fs/xfs/xqmstat", 0, NULL,
> -			 &xqmstat_proc_fops))
> +			&xqmstat_proc_fops))
>  		goto out_remove_stat_file;
>  	if (!proc_create("fs/xfs/xqm", 0, NULL,
> -			 &xqm_proc_fops))
> +			&xqm_proc_fops))

Same question.  (did checkpatch complain or something?)

>  		goto out_remove_xqmstat_file;
>  #endif
>  	return 0;
>  
>  #ifdef CONFIG_XFS_QUOTA
> - out_remove_xqmstat_file:
> +out_remove_xqmstat_file:
>  	remove_proc_entry("fs/xfs/xqmstat", NULL);
> - out_remove_stat_file:
> +out_remove_stat_file:
>  	remove_proc_entry("fs/xfs/stat", NULL);
>  #endif
> - out_remove_xfs_dir:
> +out_remove_xfs_dir:
>  	remove_proc_entry("fs/xfs", NULL);
> - out:
> +out:
>  	return -ENOMEM;
>  }

same whitespace question.  unless there's a reason, I'd not include
these nonfunctional changes in this patch.

> diff --git a/fs/xfs/xfs_stats.h b/fs/xfs/xfs_stats.h
> index 18807b5..672fe83 100644
> --- a/fs/xfs/xfs_stats.h
> +++ b/fs/xfs/xfs_stats.h
> @@ -18,9 +18,6 @@
>  #ifndef __XFS_STATS_H__
>  #define __XFS_STATS_H__
>  
> -int xfs_stats_format(char *buf);
> -void xfs_stats_clearall(void);
> -
>  #if defined(CONFIG_PROC_FS) && !defined(XFS_STATS_OFF)
>  
>  #include <linux/percpu.h>
> @@ -217,15 +214,17 @@ struct xfsstats {
>  	__uint64_t		xs_read_bytes;
>  };
>  
> -DECLARE_PER_CPU(struct xfsstats, xfsstats);
> +int xfs_stats_format(struct xfsstats __percpu *stats, char *buf);
> +void xfs_stats_clearall(struct xfsstats __percpu *stats);
> +extern struct xstats xfsstats;
>  
>  /*
>   * We don't disable preempt, not too worried about poking the
>   * wrong CPU's stat for now (also aggregated before reporting).
>   */
> -#define XFS_STATS_INC(v)	(per_cpu(xfsstats, current_cpu()).v++)
> -#define XFS_STATS_DEC(v)	(per_cpu(xfsstats, current_cpu()).v--)
> -#define XFS_STATS_ADD(v, inc)	(per_cpu(xfsstats, current_cpu()).v += (inc))
> +#define XFS_STATS_INC(v)	(per_cpu(*xfsstats.xs_stats, current_cpu()).v++)
> +#define XFS_STATS_DEC(v)	(per_cpu(*xfsstats.xs_stats, current_cpu()).v--)
> +#define XFS_STATS_ADD(v, inc)	(per_cpu(*xfsstats.xs_stats, current_cpu()).v += (inc))

Line > 80 chars

>  
>  extern int xfs_init_procfs(void);
>  extern void xfs_cleanup_procfs(void);
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 31ad281..f1ea1c9 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -61,7 +61,6 @@ static kmem_zone_t *xfs_ioend_zone;
>  mempool_t *xfs_ioend_pool;
>  
>  static struct kset *xfs_kset;		/* top-level xfs sysfs dir */
> -static struct xfs_kobj xfs_stats_kobj;	/* global stats sysfs attrs */
>  #ifdef DEBUG
>  static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs attrs */
>  #endif
> @@ -1802,6 +1801,7 @@ xfs_destroy_workqueues(void)
>  	destroy_workqueue(xfs_alloc_wq);
>  }
>  
> +

no need for a new newline here

>  STATIC int __init
>  init_xfs_fs(void)
>  {
> @@ -1842,8 +1842,9 @@ init_xfs_fs(void)
>  		goto out_sysctl_unregister;
>  	}
>  
> -	xfs_stats_kobj.kobject.kset = xfs_kset;
> -	error = xfs_sysfs_init(&xfs_stats_kobj, &xfs_stats_ktype, NULL,
> +	xfsstats.xs_stats = alloc_percpu(struct xfsstats);


need to check for allocation failure here, and fix up the goto/cleanup
targets as a result.

> +	xfsstats.xs_kobj.kobject.kset = xfs_kset;
> +	error = xfs_sysfs_init(&xfsstats.xs_kobj, &xfs_stats_ktype, NULL,
>  			       "stats");
>  	if (error)
>  		goto out_kset_unregister;
> @@ -1852,7 +1853,7 @@ init_xfs_fs(void)
>  	xfs_dbg_kobj.kobject.kset = xfs_kset;
>  	error = xfs_sysfs_init(&xfs_dbg_kobj, &xfs_dbg_ktype, NULL, "debug");
>  	if (error)
> -		goto out_remove_stats_kobj;
> +		goto out_remove_stats;
>  #endif
>  
>  	error = xfs_qm_init();
> @@ -1869,9 +1870,9 @@ init_xfs_fs(void)
>   out_remove_dbg_kobj:
>  #ifdef DEBUG
>  	xfs_sysfs_del(&xfs_dbg_kobj);
> - out_remove_stats_kobj:
> + out_remove_stats:
>  #endif
> -	xfs_sysfs_del(&xfs_stats_kobj);
> +	free_percpu(xfsstats.xs_stats);
>   out_kset_unregister:
>  	kset_unregister(xfs_kset);
>   out_sysctl_unregister:
> @@ -1898,7 +1899,7 @@ exit_xfs_fs(void)
>  #ifdef DEBUG
>  	xfs_sysfs_del(&xfs_dbg_kobj);
>  #endif
> -	xfs_sysfs_del(&xfs_stats_kobj);
> +	free_percpu(xfsstats.xs_stats);

still need to delete the sysfs entry, even though it's no longer
the xfs_stats_kobj - now it's the xfsstats.xs_kobj right?

>  	kset_unregister(xfs_kset);
>  	xfs_sysctl_unregister();
>  	xfs_cleanup_procfs();
> diff --git a/fs/xfs/xfs_sysctl.c b/fs/xfs/xfs_sysctl.c
> index 5defabb..aed74d3 100644
> --- a/fs/xfs/xfs_sysctl.c
> +++ b/fs/xfs/xfs_sysctl.c
> @@ -37,7 +37,7 @@ xfs_stats_clear_proc_handler(
>  	ret = proc_dointvec_minmax(ctl, write, buffer, lenp, ppos);
>  
>  	if (!ret && write && *valp) {
> -		xfs_stats_clearall();
> +		xfs_stats_clearall(xfsstats.xs_stats);
>  		xfs_stats_clear = 0;
>  	}
>  
> diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
> index ab4850b..3275408 100644
> --- a/fs/xfs/xfs_sysfs.c
> +++ b/fs/xfs/xfs_sysfs.c
> @@ -131,12 +131,23 @@ struct kobj_type xfs_dbg_ktype = {
>  
>  /* stats */
>  
> +static inline struct xstats *
> +to_xstats(struct kobject *kobject)
> +{
> +	struct xfs_kobj *kobj = to_kobj(kobject);
> +
> +	return container_of(kobj, struct xstats, xs_kobj);
> +}
> +
> +
>  STATIC ssize_t
>  stats_show(
>  	struct kobject	*kobject,
>  	char	*buf)
>  {
> -	return xfs_stats_format(buf);
> +	struct xstats *stats = to_xstats(kobject);

nitpick, tab before *stats to line it all up.

> +
> +	return xfs_stats_format(stats->xs_stats, buf);
>  }
>  XFS_SYSFS_ATTR_RO(stats);
>  
> @@ -148,6 +159,7 @@ stats_clear_store(
>  {
>  	int		ret;
>  	int		val;
> +	struct xstats *stats = to_xstats(kobject);

nitpick, tab before *stats to line it all up.

Thanks,
-Eric

>  
>  	ret = kstrtoint(buf, 0, &val);
>  	if (ret)
> @@ -155,7 +167,8 @@ stats_clear_store(
>  
>  	if (val != 1)
>  		return -EINVAL;
> -	xfs_stats_clearall();
> +
> +	xfs_stats_clearall(stats->xs_stats);
>  	return count;
>  }
>  XFS_SYSFS_ATTR_WO(stats_clear);
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/5] xfs: incorporate sysfs/kobject in xfsstats: handlers take kobjects.

[Bill O'Donnell]

Thanks for the review, Eric.


On Tue, Sep 22, 2015 at 09:47:58AM -0500, Eric Sandeen wrote:
> On 9/22/15 7:07 AM, Bill O'Donnell wrote:
> > This patch is the next step toward per-fs xfs stats. Allocate &
> > deallocate per-fs stats structures and set up the sysfs entries for them.
> 
> Ok, the patch doesn't actually do that last sentence yet, so
> I'd leave it out of the description.  :)

Agreed.

> 
> You could mumble about how this enables the next patch, though, i.e.
> this makes the show & clear routines able to handle any stats structure
> associated with a kobject.

Yep.

> 
> > Instead of a single global xfsstats structure, add kobject and a pointer
> > to a per-cpu struct xfsstats. Modify the macros that manipulate the stats
> > accordingly: XFS_STATS_INC, XFS_STATS_DEC, and XFS_STATS_ADD now access
> > xfsstats->xs_stats.
> > 
> > The sysfs functions need to get from the kobject back to the xfsstats
> > structure which contains it, and pass the pointer to the ->xs_stats
> > percpu structure into the show & clear routines.
> > 
> > 
> > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > ---
> >  fs/xfs/xfs_linux.h  |  7 +++++++
> >  fs/xfs/xfs_stats.c  | 47 ++++++++++++++++++++++++-----------------------
> >  fs/xfs/xfs_stats.h  | 13 ++++++-------
> >  fs/xfs/xfs_super.c  | 15 ++++++++-------
> >  fs/xfs/xfs_sysctl.c |  2 +-
> >  fs/xfs/xfs_sysfs.c  | 17 +++++++++++++++--
> >  6 files changed, 61 insertions(+), 40 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> > index 85f883d..f1a8505 100644
> > --- a/fs/xfs/xfs_linux.h
> > +++ b/fs/xfs/xfs_linux.h
> > @@ -171,6 +171,13 @@ struct xfs_kobj {
> >  	struct completion	complete;
> >  };
> >  
> > +struct xstats {
> > +	struct xfsstats __percpu *xs_stats;
> > +	struct xfs_kobj         xs_kobj;
> 
> nitpick, I'd fiddle w/ whitespace a little to make the 2 members line
> up nicely.

Ok

> 
> > +};
> > +
> > +extern struct xstats xfsstats;
> > +
> >  /* Kernel uid/gid conversion. These are used to convert to/from the on disk
> >   * uid_t/gid_t types to the kuid_t/kgid_t types that the kernel uses internally.
> >   * The conversion here is type only, the value will remain the same since we
> > diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
> > index dc6ca67..ab79973 100644
> > --- a/fs/xfs/xfs_stats.c
> > +++ b/fs/xfs/xfs_stats.c
> > @@ -18,18 +18,18 @@
> >  #include "xfs.h"
> >  #include <linux/proc_fs.h>
> >  
> > -DEFINE_PER_CPU(struct xfsstats, xfsstats);
> > +struct xstats xfsstats;
> >  
> > -static int counter_val(int idx)
> > +static int counter_val(struct xfsstats __percpu *stats, int idx)
> >  {
> >  	int val = 0, cpu;
> >  
> >  	for_each_possible_cpu(cpu)
> > -		val += *(((__u32 *)&per_cpu(xfsstats, cpu) + idx));
> > +		val += *(((__u32 *)&per_cpu(*stats, cpu) + idx));
> 
> Now that we are referring to a pointer to a percpu structure
> (instead of the old global xfsstats structure itself), I think it's
> a lot cleaner & more legible to do:
> 
> +		val += *(((__u32 *)per_cpu_ptr(stats, cpu) + idx));
> 
> (ok the cast gyrations may not qualify as "clean and legible" but it
> helps - see also below)

Agreed.

> 
> >  	return val;
> >  }
> >  
> > -int xfs_stats_format(char *buf)
> > +int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
> >  {
> >  	int		i, j;
> >  	int		len = 0;
> > @@ -73,14 +73,14 @@ int xfs_stats_format(char *buf)
> >  		/* inner loop does each group */
> >  		for (; j < xstats[i].endpoint; j++)
> >  			len += snprintf(buf + len, PATH_MAX - len, " %u",
> > -					counter_val(j));
> > +					counter_val(stats, j));
> >  		len += snprintf(buf + len, PATH_MAX - len, "\n");
> >  	}
> >  	/* extra precision counters */
> >  	for_each_possible_cpu(i) {
> > -		xs_xstrat_bytes += per_cpu(xfsstats, i).xs_xstrat_bytes;
> > -		xs_write_bytes += per_cpu(xfsstats, i).xs_write_bytes;
> > -		xs_read_bytes += per_cpu(xfsstats, i).xs_read_bytes;
> > +		xs_xstrat_bytes += per_cpu(*stats, i).xs_xstrat_bytes;
> > +		xs_write_bytes += per_cpu(*stats, i).xs_write_bytes;
> > +		xs_read_bytes += per_cpu(*stats, i).xs_read_bytes;
> 
> ditto here,
> 
> +		xs_xstrat_bytes += per_cpu_ptr(stats, i)->xs_xstrat_bytes;
> +		xs_write_bytes += per_cpu_ptr(stats, i)->xs_write_bytes;
> +		xs_read_bytes += per_cpu_ptr(stats, i)->xs_read_bytes;
> 
> >  	}
> >  
> >  	len += snprintf(buf+len, PATH_MAX-len, "xpc %Lu %Lu %Lu\n",
> > @@ -95,7 +95,7 @@ int xfs_stats_format(char *buf)
> >  	return len;
> >  }
> >  
> > -void xfs_stats_clearall(void)
> > +void xfs_stats_clearall(struct xfsstats __percpu *stats)
> >  {
> >  	int		c;
> >  	__uint32_t	vn_active;
> > @@ -104,10 +104,10 @@ void xfs_stats_clearall(void)
> >  	for_each_possible_cpu(c) {
> >  		preempt_disable();
> >  		/* save vn_active, it's a universal truth! */
> > -		vn_active = per_cpu(xfsstats, c).vn_active;
> > -		memset(&per_cpu(xfsstats, c), 0,
> > -		       sizeof(struct xfsstats));
> > -		per_cpu(xfsstats, c).vn_active = vn_active;
> > +		vn_active = per_cpu(*stats, c).vn_active;
> > +		memset(&per_cpu(*stats, c), 0,
> > +		       sizeof(*stats));
> > +		per_cpu(*stats, c).vn_active = vn_active;
> >  		preempt_enable();
> >  	}
> >  }
> 
> 
> +		vn_active = per_cpu_ptr(stats, c)->vn_active;
> +		memset(per_cpu_ptr(stats, c), 0, sizeof(*stats));
> +		per_cpu_ptr(stats, c)->vn_active = vn_active;
> 
> 
> > @@ -119,9 +119,10 @@ static int xqm_proc_show(struct seq_file *m, void *v)
> >  	/* maximum; incore; ratio free to inuse; freelist */
> >  	seq_printf(m, "%d\t%d\t%d\t%u\n",
> >  			0,
> > -			counter_val(XFSSTAT_END_XQMSTAT),
> > +			counter_val(xfsstats.xs_stats, XFSSTAT_END_XQMSTAT),
> >  			0,
> > -			counter_val(XFSSTAT_END_XQMSTAT + 1));
> > +			counter_val(xfsstats.xs_stats,
> > +				XFSSTAT_END_XQMSTAT + 1));
> 
> might be able to make this fit on one line by lining up the "0"
> with the "%d" but *shrug* now i'm really being stupidly nitpicky.

;)

> 
> >  	return 0;
> >  }
> >  
> > @@ -145,7 +146,7 @@ static int xqmstat_proc_show(struct seq_file *m, void *v)
> >  
> >  	seq_printf(m, "qm");
> >  	for (j = XFSSTAT_END_IBT_V2; j < XFSSTAT_END_XQMSTAT; j++)
> > -		seq_printf(m, " %u", counter_val(j));
> > +		seq_printf(m, " %u", counter_val(xfsstats.xs_stats, j));
> >  	seq_putc(m, '\n');
> >  	return 0;
> >  }
> > @@ -171,28 +172,28 @@ xfs_init_procfs(void)
> >  		goto out;
> >  
> >  	if (!proc_symlink("fs/xfs/stat", NULL,
> > -			  "/sys/fs/xfs/stats/stats"))
> > +			"/sys/fs/xfs/stats/stats"))
> 
> intentional whitespace changes?
> 
> >  		goto out_remove_xfs_dir;
> >  
> >  #ifdef CONFIG_XFS_QUOTA
> >  	if (!proc_create("fs/xfs/xqmstat", 0, NULL,
> > -			 &xqmstat_proc_fops))
> > +			&xqmstat_proc_fops))
> >  		goto out_remove_stat_file;
> >  	if (!proc_create("fs/xfs/xqm", 0, NULL,
> > -			 &xqm_proc_fops))
> > +			&xqm_proc_fops))
> 
> Same question.  (did checkpatch complain or something?)

Yes. Checkpatch didn't like any spaces, so I turned em into tabs.

> 
> >  		goto out_remove_xqmstat_file;
> >  #endif
> >  	return 0;
> >  
> >  #ifdef CONFIG_XFS_QUOTA
> > - out_remove_xqmstat_file:
> > +out_remove_xqmstat_file:
> >  	remove_proc_entry("fs/xfs/xqmstat", NULL);
> > - out_remove_stat_file:
> > +out_remove_stat_file:
> >  	remove_proc_entry("fs/xfs/stat", NULL);
> >  #endif
> > - out_remove_xfs_dir:
> > +out_remove_xfs_dir:
> >  	remove_proc_entry("fs/xfs", NULL);
> > - out:
> > +out:
> >  	return -ENOMEM;
> >  }
> 
> same whitespace question.  unless there's a reason, I'd not include
> these nonfunctional changes in this patch.

Again, checkpatch didn't care for the preceding space on out_blah...

> 
> > diff --git a/fs/xfs/xfs_stats.h b/fs/xfs/xfs_stats.h
> > index 18807b5..672fe83 100644
> > --- a/fs/xfs/xfs_stats.h
> > +++ b/fs/xfs/xfs_stats.h
> > @@ -18,9 +18,6 @@
> >  #ifndef __XFS_STATS_H__
> >  #define __XFS_STATS_H__
> >  
> > -int xfs_stats_format(char *buf);
> > -void xfs_stats_clearall(void);
> > -
> >  #if defined(CONFIG_PROC_FS) && !defined(XFS_STATS_OFF)
> >  
> >  #include <linux/percpu.h>
> > @@ -217,15 +214,17 @@ struct xfsstats {
> >  	__uint64_t		xs_read_bytes;
> >  };
> >  
> > -DECLARE_PER_CPU(struct xfsstats, xfsstats);
> > +int xfs_stats_format(struct xfsstats __percpu *stats, char *buf);
> > +void xfs_stats_clearall(struct xfsstats __percpu *stats);
> > +extern struct xstats xfsstats;
> >  
> >  /*
> >   * We don't disable preempt, not too worried about poking the
> >   * wrong CPU's stat for now (also aggregated before reporting).
> >   */
> > -#define XFS_STATS_INC(v)	(per_cpu(xfsstats, current_cpu()).v++)
> > -#define XFS_STATS_DEC(v)	(per_cpu(xfsstats, current_cpu()).v--)
> > -#define XFS_STATS_ADD(v, inc)	(per_cpu(xfsstats, current_cpu()).v += (inc))
> > +#define XFS_STATS_INC(v)	(per_cpu(*xfsstats.xs_stats, current_cpu()).v++)
> > +#define XFS_STATS_DEC(v)	(per_cpu(*xfsstats.xs_stats, current_cpu()).v--)
> > +#define XFS_STATS_ADD(v, inc)	(per_cpu(*xfsstats.xs_stats, current_cpu()).v += (inc))
> 
> Line > 80 chars

I'll fix it.

> 
> >  
> >  extern int xfs_init_procfs(void);
> >  extern void xfs_cleanup_procfs(void);
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 31ad281..f1ea1c9 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -61,7 +61,6 @@ static kmem_zone_t *xfs_ioend_zone;
> >  mempool_t *xfs_ioend_pool;
> >  
> >  static struct kset *xfs_kset;		/* top-level xfs sysfs dir */
> > -static struct xfs_kobj xfs_stats_kobj;	/* global stats sysfs attrs */
> >  #ifdef DEBUG
> >  static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs attrs */
> >  #endif
> > @@ -1802,6 +1801,7 @@ xfs_destroy_workqueues(void)
> >  	destroy_workqueue(xfs_alloc_wq);
> >  }
> >  
> > +
> 
> no need for a new newline here
> 
> >  STATIC int __init
> >  init_xfs_fs(void)
> >  {
> > @@ -1842,8 +1842,9 @@ init_xfs_fs(void)
> >  		goto out_sysctl_unregister;
> >  	}
> >  
> > -	xfs_stats_kobj.kobject.kset = xfs_kset;
> > -	error = xfs_sysfs_init(&xfs_stats_kobj, &xfs_stats_ktype, NULL,
> > +	xfsstats.xs_stats = alloc_percpu(struct xfsstats);
> 
> 
> need to check for allocation failure here, and fix up the goto/cleanup
> targets as a result.

Will do.

> 
> > +	xfsstats.xs_kobj.kobject.kset = xfs_kset;
> > +	error = xfs_sysfs_init(&xfsstats.xs_kobj, &xfs_stats_ktype, NULL,
> >  			       "stats");
> >  	if (error)
> >  		goto out_kset_unregister;
> > @@ -1852,7 +1853,7 @@ init_xfs_fs(void)
> >  	xfs_dbg_kobj.kobject.kset = xfs_kset;
> >  	error = xfs_sysfs_init(&xfs_dbg_kobj, &xfs_dbg_ktype, NULL, "debug");
> >  	if (error)
> > -		goto out_remove_stats_kobj;
> > +		goto out_remove_stats;
> >  #endif
> >  
> >  	error = xfs_qm_init();
> > @@ -1869,9 +1870,9 @@ init_xfs_fs(void)
> >   out_remove_dbg_kobj:
> >  #ifdef DEBUG
> >  	xfs_sysfs_del(&xfs_dbg_kobj);
> > - out_remove_stats_kobj:
> > + out_remove_stats:
> >  #endif
> > -	xfs_sysfs_del(&xfs_stats_kobj);
> > +	free_percpu(xfsstats.xs_stats);
> >   out_kset_unregister:
> >  	kset_unregister(xfs_kset);
> >   out_sysctl_unregister:
> > @@ -1898,7 +1899,7 @@ exit_xfs_fs(void)
> >  #ifdef DEBUG
> >  	xfs_sysfs_del(&xfs_dbg_kobj);
> >  #endif
> > -	xfs_sysfs_del(&xfs_stats_kobj);
> > +	free_percpu(xfsstats.xs_stats);
> 
> still need to delete the sysfs entry, even though it's no longer
> the xfs_stats_kobj - now it's the xfsstats.xs_kobj right?

Correct.

> 
> >  	kset_unregister(xfs_kset);
> >  	xfs_sysctl_unregister();
> >  	xfs_cleanup_procfs();
> > diff --git a/fs/xfs/xfs_sysctl.c b/fs/xfs/xfs_sysctl.c
> > index 5defabb..aed74d3 100644
> > --- a/fs/xfs/xfs_sysctl.c
> > +++ b/fs/xfs/xfs_sysctl.c
> > @@ -37,7 +37,7 @@ xfs_stats_clear_proc_handler(
> >  	ret = proc_dointvec_minmax(ctl, write, buffer, lenp, ppos);
> >  
> >  	if (!ret && write && *valp) {
> > -		xfs_stats_clearall();
> > +		xfs_stats_clearall(xfsstats.xs_stats);
> >  		xfs_stats_clear = 0;
> >  	}
> >  
> > diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
> > index ab4850b..3275408 100644
> > --- a/fs/xfs/xfs_sysfs.c
> > +++ b/fs/xfs/xfs_sysfs.c
> > @@ -131,12 +131,23 @@ struct kobj_type xfs_dbg_ktype = {
> >  
> >  /* stats */
> >  
> > +static inline struct xstats *
> > +to_xstats(struct kobject *kobject)
> > +{
> > +	struct xfs_kobj *kobj = to_kobj(kobject);
> > +
> > +	return container_of(kobj, struct xstats, xs_kobj);
> > +}
> > +
> > +
> >  STATIC ssize_t
> >  stats_show(
> >  	struct kobject	*kobject,
> >  	char	*buf)
> >  {
> > -	return xfs_stats_format(buf);
> > +	struct xstats *stats = to_xstats(kobject);
> 
> nitpick, tab before *stats to line it all up.
> 
> > +
> > +	return xfs_stats_format(stats->xs_stats, buf);
> >  }
> >  XFS_SYSFS_ATTR_RO(stats);
> >  
> > @@ -148,6 +159,7 @@ stats_clear_store(
> >  {
> >  	int		ret;
> >  	int		val;
> > +	struct xstats *stats = to_xstats(kobject);
> 
> nitpick, tab before *stats to line it all up.
> 
> Thanks,
> -Eric
> 
> >  
> >  	ret = kstrtoint(buf, 0, &val);
> >  	if (ret)
> > @@ -155,7 +167,8 @@ stats_clear_store(
> >  
> >  	if (val != 1)
> >  		return -EINVAL;
> > -	xfs_stats_clearall();
> > +
> > +	xfs_stats_clearall(stats->xs_stats);
> >  	return count;
> >  }
> >  XFS_SYSFS_ATTR_WO(stats_clear);
> > 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/5] xfs: incorporate sysfs/kobject in xfsstats: handlers take kobjects.

[Dave Chinner]

On Tue, Sep 22, 2015 at 10:16:05AM -0500, Bill O'Donnell wrote:
> > >  		goto out;
> > >  
> > >  	if (!proc_symlink("fs/xfs/stat", NULL,
> > > -			  "/sys/fs/xfs/stats/stats"))
> > > +			"/sys/fs/xfs/stats/stats"))
> > 
> > intentional whitespace changes?
> > 
> > >  		goto out_remove_xfs_dir;
> > >  
> > >  #ifdef CONFIG_XFS_QUOTA
> > >  	if (!proc_create("fs/xfs/xqmstat", 0, NULL,
> > > -			 &xqmstat_proc_fops))
> > > +			&xqmstat_proc_fops))
> > >  		goto out_remove_stat_file;
> > >  	if (!proc_create("fs/xfs/xqm", 0, NULL,
> > > -			 &xqm_proc_fops))
> > > +			&xqm_proc_fops))
> > 
> > Same question.  (did checkpatch complain or something?)
> 
> Yes. Checkpatch didn't like any spaces, so I turned em into tabs.

Checkpatch should be considered harmful.

It's a good starting point, but it ends up doing more harm than good
because it doesn't understand the subtle differences in code across
different subsystems.

In this case, the standard we use for XFS is that multi-line
parameters should be lined up with the first parameter, unless the
new parameters don't fit on a single line, and then we back up the
indent by a tab at a time. i.e. This is correct:

	if (!proc_create("fs/xfs/xqm", 0, NULL,
			 &xqm_proc_fops))

regardless of what checkpatch says. Indeed, if checkpatch had any
brains, it would have noticed that a line break is not necessary
as this:

	if (!proc_create("fs/xfs/xqm", 0, NULL, &xqm_proc_fops))

fits in 80 columns just fine.

Remember that Documentation/CodingStyle is a set of guidelines, not
a strict, enforcable set of rules.  The basic guidelines canbe
summarised as:

	1. Use the same style as the existing code in the file,
	   unless reviewers/maintainers ask otherwise.
	2. new files should follow the format used in other files in
	   the subsystem, unless reviewers/maintainer asks otherwise.
	3. Do not reformat code that you don't need to directly
	   modify in the patch.
	4. modify your editor to highlight common style things that
	   you need reminders to get right
	5. Immolate checkpatch before the plague spreads further

As to #4, there's plenty of simple things you can do. e.g to
identify stray whitespace in files, add this to your .vimrc file
(you can do similar with emacs, google it):

	" highlight whitespace damage
	highlight RedundantSpaces ctermbg=red guibg=red
	match RedundantSpaces /\s\+$\| \+\ze\t/

This will catch things like "tab-space-tab" and it will also find
trailing whitespace at the end of lines. They will appear in red.

> > > -#define XFS_STATS_INC(v)	(per_cpu(xfsstats, current_cpu()).v++)
> > > -#define XFS_STATS_DEC(v)	(per_cpu(xfsstats, current_cpu()).v--)
> > > -#define XFS_STATS_ADD(v, inc)	(per_cpu(xfsstats, current_cpu()).v += (inc))
> > > +#define XFS_STATS_INC(v)	(per_cpu(*xfsstats.xs_stats, current_cpu()).v++)
> > > +#define XFS_STATS_DEC(v)	(per_cpu(*xfsstats.xs_stats, current_cpu()).v--)
> > > +#define XFS_STATS_ADD(v, inc)	(per_cpu(*xfsstats.xs_stats, current_cpu()).v += (inc))
> > 
> > Line > 80 chars
> 
> I'll fix it.

Checkpatch got changed not to warn about that, because too many
people complained about it when modifying files with >80 column
formatting.

To that end, I also use custom column width highlights based on file
names so that line wrapping occurs automatically at the correct
spot, and when looking at code I can clearly see long lines:

	" set the textwidth to 80 characters by default
	set tw=80

	" set the textwidth to 68 characters for guilt commit messages
	au BufNewFile,BufRead guilt.msg.*,.gitsendemail*,.git* set tw=68

	" set the textwidth to 68 characters for replies (email&usenet)
	au BufNewFile,BufRead .letter,mutt*,nn.*,snd.* set tw=68

	" highlight textwidth
	set cc=+1

If you do little things like this, the need for checkpatch goes
away. With a default tw=80 and a highlight, I only have to glance
at a patch to know it has lines longer than 80 columns in it.

As such, I haven't used checkpatch for years, and I recommend that
you develop the right habits and automation such that you don't need
to use it after a couple of months, either...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/5] xfs: incorporate sysfs/kobject in xfsstats: handlers take kobjects.

[Bill O'Donnell]

On Wed, Sep 23, 2015 at 08:26:36AM +1000, Dave Chinner wrote:
> On Tue, Sep 22, 2015 at 10:16:05AM -0500, Bill O'Donnell wrote:
> > > >  		goto out;
> > > >  
> > > >  	if (!proc_symlink("fs/xfs/stat", NULL,
> > > > -			  "/sys/fs/xfs/stats/stats"))
> > > > +			"/sys/fs/xfs/stats/stats"))
> > > 
> > > intentional whitespace changes?
> > > 
> > > >  		goto out_remove_xfs_dir;
> > > >  
> > > >  #ifdef CONFIG_XFS_QUOTA
> > > >  	if (!proc_create("fs/xfs/xqmstat", 0, NULL,
> > > > -			 &xqmstat_proc_fops))
> > > > +			&xqmstat_proc_fops))
> > > >  		goto out_remove_stat_file;
> > > >  	if (!proc_create("fs/xfs/xqm", 0, NULL,
> > > > -			 &xqm_proc_fops))
> > > > +			&xqm_proc_fops))
> > > 
> > > Same question.  (did checkpatch complain or something?)
> > 
> > Yes. Checkpatch didn't like any spaces, so I turned em into tabs.
> 
> Checkpatch should be considered harmful.
> 
> It's a good starting point, but it ends up doing more harm than good
> because it doesn't understand the subtle differences in code across
> different subsystems.
> 
> In this case, the standard we use for XFS is that multi-line
> parameters should be lined up with the first parameter, unless the
> new parameters don't fit on a single line, and then we back up the
> indent by a tab at a time. i.e. This is correct:
> 
> 	if (!proc_create("fs/xfs/xqm", 0, NULL,
> 			 &xqm_proc_fops))
> 
> regardless of what checkpatch says. Indeed, if checkpatch had any
> brains, it would have noticed that a line break is not necessary
> as this:
> 
> 	if (!proc_create("fs/xfs/xqm", 0, NULL, &xqm_proc_fops))
> 
> fits in 80 columns just fine.
> 
> Remember that Documentation/CodingStyle is a set of guidelines, not
> a strict, enforcable set of rules.  The basic guidelines canbe
> summarised as:
> 
> 	1. Use the same style as the existing code in the file,
> 	   unless reviewers/maintainers ask otherwise.
> 	2. new files should follow the format used in other files in
> 	   the subsystem, unless reviewers/maintainer asks otherwise.
> 	3. Do not reformat code that you don't need to directly
> 	   modify in the patch.
> 	4. modify your editor to highlight common style things that
> 	   you need reminders to get right
> 	5. Immolate checkpatch before the plague spreads further
> 
> As to #4, there's plenty of simple things you can do. e.g to
> identify stray whitespace in files, add this to your .vimrc file
> (you can do similar with emacs, google it):
> 
> 	" highlight whitespace damage
> 	highlight RedundantSpaces ctermbg=red guibg=red
> 	match RedundantSpaces /\s\+$\| \+\ze\t/
> 
> This will catch things like "tab-space-tab" and it will also find
> trailing whitespace at the end of lines. They will appear in red.
> 
> > > > -#define XFS_STATS_INC(v)	(per_cpu(xfsstats, current_cpu()).v++)
> > > > -#define XFS_STATS_DEC(v)	(per_cpu(xfsstats, current_cpu()).v--)
> > > > -#define XFS_STATS_ADD(v, inc)	(per_cpu(xfsstats, current_cpu()).v += (inc))
> > > > +#define XFS_STATS_INC(v)	(per_cpu(*xfsstats.xs_stats, current_cpu()).v++)
> > > > +#define XFS_STATS_DEC(v)	(per_cpu(*xfsstats.xs_stats, current_cpu()).v--)
> > > > +#define XFS_STATS_ADD(v, inc)	(per_cpu(*xfsstats.xs_stats, current_cpu()).v += (inc))
> > > 
> > > Line > 80 chars
> > 
> > I'll fix it.
> 
> Checkpatch got changed not to warn about that, because too many
> people complained about it when modifying files with >80 column
> formatting.
> 
> To that end, I also use custom column width highlights based on file
> names so that line wrapping occurs automatically at the correct
> spot, and when looking at code I can clearly see long lines:
> 
> 	" set the textwidth to 80 characters by default
> 	set tw=80
> 
> 	" set the textwidth to 68 characters for guilt commit messages
> 	au BufNewFile,BufRead guilt.msg.*,.gitsendemail*,.git* set tw=68
> 
> 	" set the textwidth to 68 characters for replies (email&usenet)
> 	au BufNewFile,BufRead .letter,mutt*,nn.*,snd.* set tw=68
> 
> 	" highlight textwidth
> 	set cc=+1
> 
> If you do little things like this, the need for checkpatch goes
> away. With a default tw=80 and a highlight, I only have to glance
> at a patch to know it has lines longer than 80 columns in it.
> 
> As such, I haven't used checkpatch for years, and I recommend that
> you develop the right habits and automation such that you don't need
> to use it after a couple of months, either...

All points well taken. I'm fixing it all up in the next patch!

Thanks!
Bill

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs