[PATCH 0/2] xfs stats fixes - V2

[PATCH 0/2] xfs stats fixes - V2

[Carlos Maiolino]

Hi,

this is a new version of my fixes to xfsstats code, the V1 is here:
https://marc.info/?l=linux-xfs&m=153857014823217&w=2

This patchset main goal, is to fix a bug which makes /proc/fs/xfs/xqmstat to
display garbage data (patch 1), and a re-work of the offset marks in __xfsstats,
replacing the defines, by usage of offsetof(), as suggested by Eric and Dave.

Patch 2 isn't tagged as V2, because it's essentially different from the patch 2
of the first version. It still prevents offset mistakes in future updates of
__xfsstats, but in a different way than the V1 patchset

I'm not tagging stable here, because patch 2 only applies cleanly since 4.18


Carlos Maiolino (2):
  xfs: Fix xqmstats offsets in /proc/fs/xfs/xqmstat
  xfs: use offsetof() in place of offset macros for __xfsstats

 fs/xfs/xfs_stats.c | 52 +++++++++++++++++++++++++---------------------
 fs/xfs/xfs_stats.h | 28 +++----------------------
 2 files changed, 31 insertions(+), 49 deletions(-)

-- 
2.17.1

[PATCH 1/2] xfs: Fix xqmstats offsets in /proc/fs/xfs/xqmstat

[Carlos Maiolino]

The addition of FIBT, RMAP and REFCOUNT changed the offsets into
__xfssats structure.

This caused xqmstat_proc_show() to display garbage data via
/proc/fs/xfs/xqmstat, once it relies on the offsets marked via macros.

Fix it.

Fixes: 00f4e4f9 xfs: add rmap btree stats infrastructure
Fixes: aafc3c24 xfs: support the XFS_BTNUM_FINOBT free inode btree type
Fixes: 46eeb521 xfs: introduce refcount btree definitions
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_stats.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
index 4e4423153071..740ac9674848 100644
--- a/fs/xfs/xfs_stats.c
+++ b/fs/xfs/xfs_stats.c
@@ -119,7 +119,7 @@ static int xqmstat_proc_show(struct seq_file *m, void *v)
 	int j;
 
 	seq_printf(m, "qm");
-	for (j = XFSSTAT_END_IBT_V2; j < XFSSTAT_END_XQMSTAT; j++)
+	for (j = XFSSTAT_END_REFCOUNT; j < XFSSTAT_END_XQMSTAT; j++)
 		seq_printf(m, " %u", counter_val(xfsstats.xs_stats, j));
 	seq_putc(m, '\n');
 	return 0;
-- 
2.17.1

[PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats

[Carlos Maiolino]

Most offset macro mess is used in xfs_stats_format() only, and we can
simply get the right offsets using offsetof(), instead of several macros
to mark the offsets inside __xfsstats structure.

Replace all XFSSTAT_END_* macros by a single helper macro to get the
right offset into __xfsstats, and use this helper in xfs_stats_format()
directly.

The quota stats code, still looks a bit cleaner when using XFSSTAT_*
macros, so, this patch also defines XFSSTAT_START_XQMSTAT and
XFSSTAT_END_XQMSTAT locally to that code. This also should prevent
offset mistakes when updates are done into __xfsstats.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 fs/xfs/xfs_stats.c | 52 +++++++++++++++++++++++++---------------------
 fs/xfs/xfs_stats.h | 28 +++----------------------
 2 files changed, 31 insertions(+), 49 deletions(-)

diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
index 740ac9674848..cc509743facd 100644
--- a/fs/xfs/xfs_stats.c
+++ b/fs/xfs/xfs_stats.c
@@ -29,30 +29,30 @@ int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
 		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		},
-		{ "rmapbt",		XFSSTAT_END_RMAP_V2		},
-		{ "refcntbt",		XFSSTAT_END_REFCOUNT		},
+		{ "extent_alloc",	xfsstats_offset(xs_abt_lookup)	},
+		{ "abt",		xfsstats_offset(xs_blk_mapr)	},
+		{ "blk_map",		xfsstats_offset(xs_bmbt_lookup)	},
+		{ "bmbt",		xfsstats_offset(xs_dir_lookup)	},
+		{ "dir",		xfsstats_offset(xs_trans_sync)	},
+		{ "trans",		xfsstats_offset(xs_ig_attempts)	},
+		{ "ig",			xfsstats_offset(xs_log_writes)	},
+		{ "log",		xfsstats_offset(xs_try_logspace)},
+		{ "push_ail",		xfsstats_offset(xs_xstrat_quick)},
+		{ "xstrat",		xfsstats_offset(xs_write_calls)	},
+		{ "rw",			xfsstats_offset(xs_attr_get)	},
+		{ "attr",		xfsstats_offset(xs_iflush_count)},
+		{ "icluster",		xfsstats_offset(vn_active)	},
+		{ "vnodes",		xfsstats_offset(xb_get)		},
+		{ "buf",		xfsstats_offset(xs_abtb_2)	},
+		{ "abtb2",		xfsstats_offset(xs_abtc_2)	},
+		{ "abtc2",		xfsstats_offset(xs_bmbt_2)	},
+		{ "bmbt2",		xfsstats_offset(xs_ibt_2)	},
+		{ "ibt2",		xfsstats_offset(xs_fibt_2)	},
+		{ "fibt2",		xfsstats_offset(xs_rmap_2)	},
+		{ "rmapbt",		xfsstats_offset(xs_refcbt_2)	},
+		{ "refcntbt",		xfsstats_offset(xs_qm_dqreclaims)},
 		/* we print both series of quota information together */
-		{ "qm",			XFSSTAT_END_QM			},
+		{ "qm",			xfsstats_offset(xs_xstrat_bytes)},
 	};
 
 	/* Loop over all stats groups */
@@ -104,6 +104,10 @@ void xfs_stats_clearall(struct xfsstats __percpu *stats)
 #ifdef CONFIG_PROC_FS
 /* legacy quota interfaces */
 #ifdef CONFIG_XFS_QUOTA
+
+#define XFSSTAT_START_XQMSTAT xfsstats_offset(xs_qm_dqreclaims)
+#define XFSSTAT_END_XQMSTAT xfsstats_offset(xs_qm_dquot)
+
 static int xqm_proc_show(struct seq_file *m, void *v)
 {
 	/* maximum; incore; ratio free to inuse; freelist */
@@ -119,7 +123,7 @@ static int xqmstat_proc_show(struct seq_file *m, void *v)
 	int j;
 
 	seq_printf(m, "qm");
-	for (j = XFSSTAT_END_REFCOUNT; j < XFSSTAT_END_XQMSTAT; j++)
+	for (j = XFSSTAT_START_XQMSTAT; j < XFSSTAT_END_XQMSTAT; j++)
 		seq_printf(m, " %u", counter_val(xfsstats.xs_stats, j));
 	seq_putc(m, '\n');
 	return 0;
diff --git a/fs/xfs/xfs_stats.h b/fs/xfs/xfs_stats.h
index 130db070e4d8..34d704f703d2 100644
--- a/fs/xfs/xfs_stats.h
+++ b/fs/xfs/xfs_stats.h
@@ -41,17 +41,14 @@ enum {
  * XFS global statistics
  */
 struct __xfsstats {
-# define XFSSTAT_END_EXTENT_ALLOC	4
 	uint32_t		xs_allocx;
 	uint32_t		xs_allocb;
 	uint32_t		xs_freex;
 	uint32_t		xs_freeb;
-# define XFSSTAT_END_ALLOC_BTREE	(XFSSTAT_END_EXTENT_ALLOC+4)
 	uint32_t		xs_abt_lookup;
 	uint32_t		xs_abt_compare;
 	uint32_t		xs_abt_insrec;
 	uint32_t		xs_abt_delrec;
-# define XFSSTAT_END_BLOCK_MAPPING	(XFSSTAT_END_ALLOC_BTREE+7)
 	uint32_t		xs_blk_mapr;
 	uint32_t		xs_blk_mapw;
 	uint32_t		xs_blk_unmap;
@@ -59,21 +56,17 @@ struct __xfsstats {
 	uint32_t		xs_del_exlist;
 	uint32_t		xs_look_exlist;
 	uint32_t		xs_cmp_exlist;
-# define XFSSTAT_END_BLOCK_MAP_BTREE	(XFSSTAT_END_BLOCK_MAPPING+4)
 	uint32_t		xs_bmbt_lookup;
 	uint32_t		xs_bmbt_compare;
 	uint32_t		xs_bmbt_insrec;
 	uint32_t		xs_bmbt_delrec;
-# define XFSSTAT_END_DIRECTORY_OPS	(XFSSTAT_END_BLOCK_MAP_BTREE+4)
 	uint32_t		xs_dir_lookup;
 	uint32_t		xs_dir_create;
 	uint32_t		xs_dir_remove;
 	uint32_t		xs_dir_getdents;
-# define XFSSTAT_END_TRANSACTIONS	(XFSSTAT_END_DIRECTORY_OPS+3)
 	uint32_t		xs_trans_sync;
 	uint32_t		xs_trans_async;
 	uint32_t		xs_trans_empty;
-# define XFSSTAT_END_INODE_OPS		(XFSSTAT_END_TRANSACTIONS+7)
 	uint32_t		xs_ig_attempts;
 	uint32_t		xs_ig_found;
 	uint32_t		xs_ig_frecycle;
@@ -81,13 +74,11 @@ struct __xfsstats {
 	uint32_t		xs_ig_dup;
 	uint32_t		xs_ig_reclaims;
 	uint32_t		xs_ig_attrchg;
-# define XFSSTAT_END_LOG_OPS		(XFSSTAT_END_INODE_OPS+5)
 	uint32_t		xs_log_writes;
 	uint32_t		xs_log_blocks;
 	uint32_t		xs_log_noiclogs;
 	uint32_t		xs_log_force;
 	uint32_t		xs_log_force_sleep;
-# define XFSSTAT_END_TAIL_PUSHING	(XFSSTAT_END_LOG_OPS+10)
 	uint32_t		xs_try_logspace;
 	uint32_t		xs_sleep_logspace;
 	uint32_t		xs_push_ail;
@@ -98,22 +89,17 @@ struct __xfsstats {
 	uint32_t		xs_push_ail_flushing;
 	uint32_t		xs_push_ail_restarts;
 	uint32_t		xs_push_ail_flush;
-# define XFSSTAT_END_WRITE_CONVERT	(XFSSTAT_END_TAIL_PUSHING+2)
 	uint32_t		xs_xstrat_quick;
 	uint32_t		xs_xstrat_split;
-# define XFSSTAT_END_READ_WRITE_OPS	(XFSSTAT_END_WRITE_CONVERT+2)
 	uint32_t		xs_write_calls;
 	uint32_t		xs_read_calls;
-# define XFSSTAT_END_ATTRIBUTE_OPS	(XFSSTAT_END_READ_WRITE_OPS+4)
 	uint32_t		xs_attr_get;
 	uint32_t		xs_attr_set;
 	uint32_t		xs_attr_remove;
 	uint32_t		xs_attr_list;
-# define XFSSTAT_END_INODE_CLUSTER	(XFSSTAT_END_ATTRIBUTE_OPS+3)
 	uint32_t		xs_iflush_count;
 	uint32_t		xs_icluster_flushcnt;
 	uint32_t		xs_icluster_flushinode;
-# define XFSSTAT_END_VNODE_OPS		(XFSSTAT_END_INODE_CLUSTER+8)
 	uint32_t		vn_active;	/* # vnodes not on free lists */
 	uint32_t		vn_alloc;	/* # times vn_alloc called */
 	uint32_t		vn_get;		/* # times vn_get called */
@@ -122,7 +108,6 @@ struct __xfsstats {
 	uint32_t		vn_reclaim;	/* # times vn_reclaim called */
 	uint32_t		vn_remove;	/* # times vn_remove called */
 	uint32_t		vn_free;	/* # times vn_free called */
-#define XFSSTAT_END_BUF			(XFSSTAT_END_VNODE_OPS+9)
 	uint32_t		xb_get;
 	uint32_t		xb_create;
 	uint32_t		xb_get_locked;
@@ -133,28 +118,19 @@ struct __xfsstats {
 	uint32_t		xb_page_found;
 	uint32_t		xb_get_read;
 /* Version 2 btree counters */
-#define XFSSTAT_END_ABTB_V2		(XFSSTAT_END_BUF + __XBTS_MAX)
 	uint32_t		xs_abtb_2[__XBTS_MAX];
-#define XFSSTAT_END_ABTC_V2		(XFSSTAT_END_ABTB_V2 + __XBTS_MAX)
 	uint32_t		xs_abtc_2[__XBTS_MAX];
-#define XFSSTAT_END_BMBT_V2		(XFSSTAT_END_ABTC_V2 + __XBTS_MAX)
 	uint32_t		xs_bmbt_2[__XBTS_MAX];
-#define XFSSTAT_END_IBT_V2		(XFSSTAT_END_BMBT_V2 + __XBTS_MAX)
 	uint32_t		xs_ibt_2[__XBTS_MAX];
-#define XFSSTAT_END_FIBT_V2		(XFSSTAT_END_IBT_V2 + __XBTS_MAX)
 	uint32_t		xs_fibt_2[__XBTS_MAX];
-#define XFSSTAT_END_RMAP_V2		(XFSSTAT_END_FIBT_V2 + __XBTS_MAX)
 	uint32_t		xs_rmap_2[__XBTS_MAX];
-#define XFSSTAT_END_REFCOUNT		(XFSSTAT_END_RMAP_V2 + __XBTS_MAX)
 	uint32_t		xs_refcbt_2[__XBTS_MAX];
-#define XFSSTAT_END_XQMSTAT		(XFSSTAT_END_REFCOUNT + 6)
 	uint32_t		xs_qm_dqreclaims;
 	uint32_t		xs_qm_dqreclaim_misses;
 	uint32_t		xs_qm_dquot_dups;
 	uint32_t		xs_qm_dqcachemisses;
 	uint32_t		xs_qm_dqcachehits;
 	uint32_t		xs_qm_dqwants;
-#define XFSSTAT_END_QM			(XFSSTAT_END_XQMSTAT+2)
 	uint32_t		xs_qm_dquot;
 	uint32_t		xs_qm_dquot_unused;
 /* Extra precision counters */
@@ -163,10 +139,12 @@ struct __xfsstats {
 	uint64_t		xs_read_bytes;
 };
 
+#define	xfsstats_offset(f)	(offsetof(struct __xfsstats, f)/sizeof(uint32_t))
+
 struct xfsstats {
 	union {
 		struct __xfsstats	s;
-		uint32_t		a[XFSSTAT_END_XQMSTAT];
+		uint32_t		a[xfsstats_offset(xs_qm_dquot)];
 	};
 };
 
-- 
2.17.1

Re: [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats

[Darrick J. Wong]

On Wed, Oct 10, 2018 at 02:37:08PM +0200, Carlos Maiolino wrote:
> Most offset macro mess is used in xfs_stats_format() only, and we can
> simply get the right offsets using offsetof(), instead of several macros
> to mark the offsets inside __xfsstats structure.
> 
> Replace all XFSSTAT_END_* macros by a single helper macro to get the
> right offset into __xfsstats, and use this helper in xfs_stats_format()
> directly.
> 
> The quota stats code, still looks a bit cleaner when using XFSSTAT_*
> macros, so, this patch also defines XFSSTAT_START_XQMSTAT and
> XFSSTAT_END_XQMSTAT locally to that code. This also should prevent
> offset mistakes when updates are done into __xfsstats.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_stats.c | 52 +++++++++++++++++++++++++---------------------
>  fs/xfs/xfs_stats.h | 28 +++----------------------
>  2 files changed, 31 insertions(+), 49 deletions(-)
> 
> diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
> index 740ac9674848..cc509743facd 100644
> --- a/fs/xfs/xfs_stats.c
> +++ b/fs/xfs/xfs_stats.c
> @@ -29,30 +29,30 @@ int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
>  		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		},
> -		{ "rmapbt",		XFSSTAT_END_RMAP_V2		},
> -		{ "refcntbt",		XFSSTAT_END_REFCOUNT		},
> +		{ "extent_alloc",	xfsstats_offset(xs_abt_lookup)	},
> +		{ "abt",		xfsstats_offset(xs_blk_mapr)	},
> +		{ "blk_map",		xfsstats_offset(xs_bmbt_lookup)	},
> +		{ "bmbt",		xfsstats_offset(xs_dir_lookup)	},
> +		{ "dir",		xfsstats_offset(xs_trans_sync)	},
> +		{ "trans",		xfsstats_offset(xs_ig_attempts)	},
> +		{ "ig",			xfsstats_offset(xs_log_writes)	},
> +		{ "log",		xfsstats_offset(xs_try_logspace)},
> +		{ "push_ail",		xfsstats_offset(xs_xstrat_quick)},
> +		{ "xstrat",		xfsstats_offset(xs_write_calls)	},
> +		{ "rw",			xfsstats_offset(xs_attr_get)	},
> +		{ "attr",		xfsstats_offset(xs_iflush_count)},
> +		{ "icluster",		xfsstats_offset(vn_active)	},
> +		{ "vnodes",		xfsstats_offset(xb_get)		},
> +		{ "buf",		xfsstats_offset(xs_abtb_2)	},
> +		{ "abtb2",		xfsstats_offset(xs_abtc_2)	},
> +		{ "abtc2",		xfsstats_offset(xs_bmbt_2)	},
> +		{ "bmbt2",		xfsstats_offset(xs_ibt_2)	},
> +		{ "ibt2",		xfsstats_offset(xs_fibt_2)	},
> +		{ "fibt2",		xfsstats_offset(xs_rmap_2)	},
> +		{ "rmapbt",		xfsstats_offset(xs_refcbt_2)	},
> +		{ "refcntbt",		xfsstats_offset(xs_qm_dqreclaims)},
>  		/* we print both series of quota information together */
> -		{ "qm",			XFSSTAT_END_QM			},
> +		{ "qm",			xfsstats_offset(xs_xstrat_bytes)},
>  	};
>  
>  	/* Loop over all stats groups */
> @@ -104,6 +104,10 @@ void xfs_stats_clearall(struct xfsstats __percpu *stats)
>  #ifdef CONFIG_PROC_FS
>  /* legacy quota interfaces */
>  #ifdef CONFIG_XFS_QUOTA
> +
> +#define XFSSTAT_START_XQMSTAT xfsstats_offset(xs_qm_dqreclaims)
> +#define XFSSTAT_END_XQMSTAT xfsstats_offset(xs_qm_dquot)
> +
>  static int xqm_proc_show(struct seq_file *m, void *v)
>  {
>  	/* maximum; incore; ratio free to inuse; freelist */
> @@ -119,7 +123,7 @@ static int xqmstat_proc_show(struct seq_file *m, void *v)
>  	int j;
>  
>  	seq_printf(m, "qm");
> -	for (j = XFSSTAT_END_REFCOUNT; j < XFSSTAT_END_XQMSTAT; j++)
> +	for (j = XFSSTAT_START_XQMSTAT; j < XFSSTAT_END_XQMSTAT; j++)
>  		seq_printf(m, " %u", counter_val(xfsstats.xs_stats, j));
>  	seq_putc(m, '\n');
>  	return 0;
> diff --git a/fs/xfs/xfs_stats.h b/fs/xfs/xfs_stats.h
> index 130db070e4d8..34d704f703d2 100644
> --- a/fs/xfs/xfs_stats.h
> +++ b/fs/xfs/xfs_stats.h
> @@ -41,17 +41,14 @@ enum {
>   * XFS global statistics
>   */
>  struct __xfsstats {
> -# define XFSSTAT_END_EXTENT_ALLOC	4
>  	uint32_t		xs_allocx;
>  	uint32_t		xs_allocb;
>  	uint32_t		xs_freex;
>  	uint32_t		xs_freeb;
> -# define XFSSTAT_END_ALLOC_BTREE	(XFSSTAT_END_EXTENT_ALLOC+4)
>  	uint32_t		xs_abt_lookup;
>  	uint32_t		xs_abt_compare;
>  	uint32_t		xs_abt_insrec;
>  	uint32_t		xs_abt_delrec;
> -# define XFSSTAT_END_BLOCK_MAPPING	(XFSSTAT_END_ALLOC_BTREE+7)
>  	uint32_t		xs_blk_mapr;
>  	uint32_t		xs_blk_mapw;
>  	uint32_t		xs_blk_unmap;
> @@ -59,21 +56,17 @@ struct __xfsstats {
>  	uint32_t		xs_del_exlist;
>  	uint32_t		xs_look_exlist;
>  	uint32_t		xs_cmp_exlist;
> -# define XFSSTAT_END_BLOCK_MAP_BTREE	(XFSSTAT_END_BLOCK_MAPPING+4)
>  	uint32_t		xs_bmbt_lookup;
>  	uint32_t		xs_bmbt_compare;
>  	uint32_t		xs_bmbt_insrec;
>  	uint32_t		xs_bmbt_delrec;
> -# define XFSSTAT_END_DIRECTORY_OPS	(XFSSTAT_END_BLOCK_MAP_BTREE+4)
>  	uint32_t		xs_dir_lookup;
>  	uint32_t		xs_dir_create;
>  	uint32_t		xs_dir_remove;
>  	uint32_t		xs_dir_getdents;
> -# define XFSSTAT_END_TRANSACTIONS	(XFSSTAT_END_DIRECTORY_OPS+3)
>  	uint32_t		xs_trans_sync;
>  	uint32_t		xs_trans_async;
>  	uint32_t		xs_trans_empty;
> -# define XFSSTAT_END_INODE_OPS		(XFSSTAT_END_TRANSACTIONS+7)
>  	uint32_t		xs_ig_attempts;
>  	uint32_t		xs_ig_found;
>  	uint32_t		xs_ig_frecycle;
> @@ -81,13 +74,11 @@ struct __xfsstats {
>  	uint32_t		xs_ig_dup;
>  	uint32_t		xs_ig_reclaims;
>  	uint32_t		xs_ig_attrchg;
> -# define XFSSTAT_END_LOG_OPS		(XFSSTAT_END_INODE_OPS+5)
>  	uint32_t		xs_log_writes;
>  	uint32_t		xs_log_blocks;
>  	uint32_t		xs_log_noiclogs;
>  	uint32_t		xs_log_force;
>  	uint32_t		xs_log_force_sleep;
> -# define XFSSTAT_END_TAIL_PUSHING	(XFSSTAT_END_LOG_OPS+10)
>  	uint32_t		xs_try_logspace;
>  	uint32_t		xs_sleep_logspace;
>  	uint32_t		xs_push_ail;
> @@ -98,22 +89,17 @@ struct __xfsstats {
>  	uint32_t		xs_push_ail_flushing;
>  	uint32_t		xs_push_ail_restarts;
>  	uint32_t		xs_push_ail_flush;
> -# define XFSSTAT_END_WRITE_CONVERT	(XFSSTAT_END_TAIL_PUSHING+2)
>  	uint32_t		xs_xstrat_quick;
>  	uint32_t		xs_xstrat_split;
> -# define XFSSTAT_END_READ_WRITE_OPS	(XFSSTAT_END_WRITE_CONVERT+2)
>  	uint32_t		xs_write_calls;
>  	uint32_t		xs_read_calls;
> -# define XFSSTAT_END_ATTRIBUTE_OPS	(XFSSTAT_END_READ_WRITE_OPS+4)
>  	uint32_t		xs_attr_get;
>  	uint32_t		xs_attr_set;
>  	uint32_t		xs_attr_remove;
>  	uint32_t		xs_attr_list;
> -# define XFSSTAT_END_INODE_CLUSTER	(XFSSTAT_END_ATTRIBUTE_OPS+3)
>  	uint32_t		xs_iflush_count;
>  	uint32_t		xs_icluster_flushcnt;
>  	uint32_t		xs_icluster_flushinode;
> -# define XFSSTAT_END_VNODE_OPS		(XFSSTAT_END_INODE_CLUSTER+8)
>  	uint32_t		vn_active;	/* # vnodes not on free lists */
>  	uint32_t		vn_alloc;	/* # times vn_alloc called */
>  	uint32_t		vn_get;		/* # times vn_get called */
> @@ -122,7 +108,6 @@ struct __xfsstats {
>  	uint32_t		vn_reclaim;	/* # times vn_reclaim called */
>  	uint32_t		vn_remove;	/* # times vn_remove called */
>  	uint32_t		vn_free;	/* # times vn_free called */
> -#define XFSSTAT_END_BUF			(XFSSTAT_END_VNODE_OPS+9)
>  	uint32_t		xb_get;
>  	uint32_t		xb_create;
>  	uint32_t		xb_get_locked;
> @@ -133,28 +118,19 @@ struct __xfsstats {
>  	uint32_t		xb_page_found;
>  	uint32_t		xb_get_read;
>  /* Version 2 btree counters */
> -#define XFSSTAT_END_ABTB_V2		(XFSSTAT_END_BUF + __XBTS_MAX)
>  	uint32_t		xs_abtb_2[__XBTS_MAX];
> -#define XFSSTAT_END_ABTC_V2		(XFSSTAT_END_ABTB_V2 + __XBTS_MAX)
>  	uint32_t		xs_abtc_2[__XBTS_MAX];
> -#define XFSSTAT_END_BMBT_V2		(XFSSTAT_END_ABTC_V2 + __XBTS_MAX)
>  	uint32_t		xs_bmbt_2[__XBTS_MAX];
> -#define XFSSTAT_END_IBT_V2		(XFSSTAT_END_BMBT_V2 + __XBTS_MAX)
>  	uint32_t		xs_ibt_2[__XBTS_MAX];
> -#define XFSSTAT_END_FIBT_V2		(XFSSTAT_END_IBT_V2 + __XBTS_MAX)
>  	uint32_t		xs_fibt_2[__XBTS_MAX];
> -#define XFSSTAT_END_RMAP_V2		(XFSSTAT_END_FIBT_V2 + __XBTS_MAX)
>  	uint32_t		xs_rmap_2[__XBTS_MAX];
> -#define XFSSTAT_END_REFCOUNT		(XFSSTAT_END_RMAP_V2 + __XBTS_MAX)
>  	uint32_t		xs_refcbt_2[__XBTS_MAX];
> -#define XFSSTAT_END_XQMSTAT		(XFSSTAT_END_REFCOUNT + 6)
>  	uint32_t		xs_qm_dqreclaims;
>  	uint32_t		xs_qm_dqreclaim_misses;
>  	uint32_t		xs_qm_dquot_dups;
>  	uint32_t		xs_qm_dqcachemisses;
>  	uint32_t		xs_qm_dqcachehits;
>  	uint32_t		xs_qm_dqwants;
> -#define XFSSTAT_END_QM			(XFSSTAT_END_XQMSTAT+2)
>  	uint32_t		xs_qm_dquot;
>  	uint32_t		xs_qm_dquot_unused;
>  /* Extra precision counters */
> @@ -163,10 +139,12 @@ struct __xfsstats {
>  	uint64_t		xs_read_bytes;
>  };
>  
> +#define	xfsstats_offset(f)	(offsetof(struct __xfsstats, f)/sizeof(uint32_t))

Goes past 80 columns, but otherwise looks ok,

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D


> +
>  struct xfsstats {
>  	union {
>  		struct __xfsstats	s;
> -		uint32_t		a[XFSSTAT_END_XQMSTAT];
> +		uint32_t		a[xfsstats_offset(xs_qm_dquot)];
>  	};
>  };
>  
> -- 
> 2.17.1
> 

Re: [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats

[Carlos Maiolino]

> >  	uint32_t		xs_rmap_2[__XBTS_MAX];
> > -#define XFSSTAT_END_REFCOUNT		(XFSSTAT_END_RMAP_V2 + __XBTS_MAX)
> >  	uint32_t		xs_refcbt_2[__XBTS_MAX];
> > -#define XFSSTAT_END_XQMSTAT		(XFSSTAT_END_REFCOUNT + 6)
> >  	uint32_t		xs_qm_dqreclaims;
> >  	uint32_t		xs_qm_dqreclaim_misses;
> >  	uint32_t		xs_qm_dquot_dups;
> >  	uint32_t		xs_qm_dqcachemisses;
> >  	uint32_t		xs_qm_dqcachehits;
> >  	uint32_t		xs_qm_dqwants;
> > -#define XFSSTAT_END_QM			(XFSSTAT_END_XQMSTAT+2)
> >  	uint32_t		xs_qm_dquot;
> >  	uint32_t		xs_qm_dquot_unused;
> >  /* Extra precision counters */
> > @@ -163,10 +139,12 @@ struct __xfsstats {
> >  	uint64_t		xs_read_bytes;
> >  };
> >  
> > +#define	xfsstats_offset(f)	(offsetof(struct __xfsstats, f)/sizeof(uint32_t))
> 
> Goes past 80 columns, but otherwise looks ok,
> 
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

Ops, sorry, I traded readability+tabs X 80 columns, I think changing the tabs
for spaces is enough to fix it.

Do you want me to send it again?

> 
> --D
> 
> 
> > +
> >  struct xfsstats {
> >  	union {
> >  		struct __xfsstats	s;
> > -		uint32_t		a[XFSSTAT_END_XQMSTAT];
> > +		uint32_t		a[xfsstats_offset(xs_qm_dquot)];
> >  	};
> >  };
> >  
> > -- 
> > 2.17.1
> > 

-- 
Carlos

Re: [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats

[Darrick J. Wong]

On Wed, Oct 10, 2018 at 04:58:53PM +0200, Carlos Maiolino wrote:
> > >  	uint32_t		xs_rmap_2[__XBTS_MAX];
> > > -#define XFSSTAT_END_REFCOUNT		(XFSSTAT_END_RMAP_V2 + __XBTS_MAX)
> > >  	uint32_t		xs_refcbt_2[__XBTS_MAX];
> > > -#define XFSSTAT_END_XQMSTAT		(XFSSTAT_END_REFCOUNT + 6)
> > >  	uint32_t		xs_qm_dqreclaims;
> > >  	uint32_t		xs_qm_dqreclaim_misses;
> > >  	uint32_t		xs_qm_dquot_dups;
> > >  	uint32_t		xs_qm_dqcachemisses;
> > >  	uint32_t		xs_qm_dqcachehits;
> > >  	uint32_t		xs_qm_dqwants;
> > > -#define XFSSTAT_END_QM			(XFSSTAT_END_XQMSTAT+2)
> > >  	uint32_t		xs_qm_dquot;
> > >  	uint32_t		xs_qm_dquot_unused;
> > >  /* Extra precision counters */
> > > @@ -163,10 +139,12 @@ struct __xfsstats {
> > >  	uint64_t		xs_read_bytes;
> > >  };
> > >  
> > > +#define	xfsstats_offset(f)	(offsetof(struct __xfsstats, f)/sizeof(uint32_t))
> > 
> > Goes past 80 columns, but otherwise looks ok,
> > 
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Ops, sorry, I traded readability+tabs X 80 columns, I think changing the tabs
> for spaces is enough to fix it.
> 
> Do you want me to send it again?

<shrug> If Dave elects to fix it on the way in that's fine with me.

Though I guess so long as I'm being pedantic about things that 'f' ought
to be parentheses-wrapped too, e.g.

#define xfsstats_offset(f) (offsetof(struct __xfsstats, (f)) / sizeof(uint32_t))

--D

> > 
> > --D
> > 
> > 
> > > +
> > >  struct xfsstats {
> > >  	union {
> > >  		struct __xfsstats	s;
> > > -		uint32_t		a[XFSSTAT_END_XQMSTAT];
> > > +		uint32_t		a[xfsstats_offset(xs_qm_dquot)];
> > >  	};
> > >  };
> > >  
> > > -- 
> > > 2.17.1
> > > 
> 
> -- 
> Carlos

Re: [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats

[Dave Chinner]

On Wed, Oct 10, 2018 at 08:02:10AM -0700, Darrick J. Wong wrote:
> On Wed, Oct 10, 2018 at 04:58:53PM +0200, Carlos Maiolino wrote:
> > > >  	uint32_t		xs_rmap_2[__XBTS_MAX];
> > > > -#define XFSSTAT_END_REFCOUNT		(XFSSTAT_END_RMAP_V2 + __XBTS_MAX)
> > > >  	uint32_t		xs_refcbt_2[__XBTS_MAX];
> > > > -#define XFSSTAT_END_XQMSTAT		(XFSSTAT_END_REFCOUNT + 6)
> > > >  	uint32_t		xs_qm_dqreclaims;
> > > >  	uint32_t		xs_qm_dqreclaim_misses;
> > > >  	uint32_t		xs_qm_dquot_dups;
> > > >  	uint32_t		xs_qm_dqcachemisses;
> > > >  	uint32_t		xs_qm_dqcachehits;
> > > >  	uint32_t		xs_qm_dqwants;
> > > > -#define XFSSTAT_END_QM			(XFSSTAT_END_XQMSTAT+2)
> > > >  	uint32_t		xs_qm_dquot;
> > > >  	uint32_t		xs_qm_dquot_unused;
> > > >  /* Extra precision counters */
> > > > @@ -163,10 +139,12 @@ struct __xfsstats {
> > > >  	uint64_t		xs_read_bytes;
> > > >  };
> > > >  
> > > > +#define	xfsstats_offset(f)	(offsetof(struct __xfsstats, f)/sizeof(uint32_t))
> > > 
> > > Goes past 80 columns, but otherwise looks ok,
> > > 
> > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Ops, sorry, I traded readability+tabs X 80 columns, I think changing the tabs
> > for spaces is enough to fix it.
> > 
> > Do you want me to send it again?
> 
> <shrug> If Dave elects to fix it on the way in that's fine with me.
> 
> Though I guess so long as I'm being pedantic about things that 'f' ought
> to be parentheses-wrapped too, e.g.
> 
> #define xfsstats_offset(f) (offsetof(struct __xfsstats, (f)) / sizeof(uint32_t))

I'll clean it up on merge.

Thanks,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats

[Dave Chinner]

On Wed, Oct 10, 2018 at 02:37:08PM +0200, Carlos Maiolino wrote:
> Most offset macro mess is used in xfs_stats_format() only, and we can
> simply get the right offsets using offsetof(), instead of several macros
> to mark the offsets inside __xfsstats structure.
> 
> Replace all XFSSTAT_END_* macros by a single helper macro to get the
> right offset into __xfsstats, and use this helper in xfs_stats_format()
> directly.
> 
> The quota stats code, still looks a bit cleaner when using XFSSTAT_*
> macros, so, this patch also defines XFSSTAT_START_XQMSTAT and
> XFSSTAT_END_XQMSTAT locally to that code. This also should prevent
> offset mistakes when updates are done into __xfsstats.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_stats.c | 52 +++++++++++++++++++++++++---------------------
>  fs/xfs/xfs_stats.h | 28 +++----------------------
>  2 files changed, 31 insertions(+), 49 deletions(-)
> 
> diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
> index 740ac9674848..cc509743facd 100644
> --- a/fs/xfs/xfs_stats.c
> +++ b/fs/xfs/xfs_stats.c
> @@ -29,30 +29,30 @@ int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
>  		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		},
> -		{ "rmapbt",		XFSSTAT_END_RMAP_V2		},
> -		{ "refcntbt",		XFSSTAT_END_REFCOUNT		},
> +		{ "extent_alloc",	xfsstats_offset(xs_abt_lookup)	},
> +		{ "abt",		xfsstats_offset(xs_blk_mapr)	},
> +		{ "blk_map",		xfsstats_offset(xs_bmbt_lookup)	},
> +		{ "bmbt",		xfsstats_offset(xs_dir_lookup)	},
> +		{ "dir",		xfsstats_offset(xs_trans_sync)	},
> +		{ "trans",		xfsstats_offset(xs_ig_attempts)	},
> +		{ "ig",			xfsstats_offset(xs_log_writes)	},
> +		{ "log",		xfsstats_offset(xs_try_logspace)},
> +		{ "push_ail",		xfsstats_offset(xs_xstrat_quick)},
> +		{ "xstrat",		xfsstats_offset(xs_write_calls)	},
> +		{ "rw",			xfsstats_offset(xs_attr_get)	},
> +		{ "attr",		xfsstats_offset(xs_iflush_count)},
> +		{ "icluster",		xfsstats_offset(vn_active)	},
> +		{ "vnodes",		xfsstats_offset(xb_get)		},
> +		{ "buf",		xfsstats_offset(xs_abtb_2)	},
> +		{ "abtb2",		xfsstats_offset(xs_abtc_2)	},
> +		{ "abtc2",		xfsstats_offset(xs_bmbt_2)	},
> +		{ "bmbt2",		xfsstats_offset(xs_ibt_2)	},
> +		{ "ibt2",		xfsstats_offset(xs_fibt_2)	},
> +		{ "fibt2",		xfsstats_offset(xs_rmap_2)	},
> +		{ "rmapbt",		xfsstats_offset(xs_refcbt_2)	},
> +		{ "refcntbt",		xfsstats_offset(xs_qm_dqreclaims)},
>  		/* we print both series of quota information together */
> -		{ "qm",			XFSSTAT_END_QM			},
> +		{ "qm",			xfsstats_offset(xs_xstrat_bytes)},
>  	};

Now that I look at this more closely , it's a little bit ... odd.
i.e. the endpoint offset points to the first variable in the next
line, not the last variable of the stats line described by the text.
While it works, it just doesn't seem quite right to me.

Can we change it so that the index in the table is the last variable
of that line? This may require changing the inner loop to a <= check
rather than a < check, but the table would make more sense this way.
ie.

	{ "extent_alloc",	xfsstats_index(xs_freeb) },
	{ "abt",		xfsstats_index(xs_abt_delrec) },
.....


It may require a different macro for the expanded btree stats (the
array based ones) because they are defined as an array rather than
individual variables, but I think that's ok given those already have
special macros for changing them.

> +#define	xfsstats_offset(f)	(offsetof(struct __xfsstats, f)/sizeof(uint32_t))

I also think this should be named xfsstats_index(), not _offset. i.e.
it's an index into a structure we treat as an array of ints, not a byte
offset from the start of a u8 buffer....

> +
>  struct xfsstats {
>  	union {
>  		struct __xfsstats	s;
> -		uint32_t		a[XFSSTAT_END_XQMSTAT];
> +		uint32_t		a[xfsstats_offset(xs_qm_dquot)];

I think this should remain XFSSTAT_END_XQMSTAT, as we still define
that and require the quota stats to be at the end of the structure.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats

[Eric Sandeen]

On 10/11/18 12:29 AM, Dave Chinner wrote:
> On Wed, Oct 10, 2018 at 02:37:08PM +0200, Carlos Maiolino wrote:
>> Most offset macro mess is used in xfs_stats_format() only, and we can
>> simply get the right offsets using offsetof(), instead of several macros
>> to mark the offsets inside __xfsstats structure.
>>
>> Replace all XFSSTAT_END_* macros by a single helper macro to get the
>> right offset into __xfsstats, and use this helper in xfs_stats_format()
>> directly.
>>
>> The quota stats code, still looks a bit cleaner when using XFSSTAT_*
>> macros, so, this patch also defines XFSSTAT_START_XQMSTAT and
>> XFSSTAT_END_XQMSTAT locally to that code. This also should prevent
>> offset mistakes when updates are done into __xfsstats.
>>
>> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
>> ---
>>  fs/xfs/xfs_stats.c | 52 +++++++++++++++++++++++++---------------------
>>  fs/xfs/xfs_stats.h | 28 +++----------------------
>>  2 files changed, 31 insertions(+), 49 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
>> index 740ac9674848..cc509743facd 100644
>> --- a/fs/xfs/xfs_stats.c
>> +++ b/fs/xfs/xfs_stats.c
>> @@ -29,30 +29,30 @@ int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
>>  		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		},
>> -		{ "rmapbt",		XFSSTAT_END_RMAP_V2		},
>> -		{ "refcntbt",		XFSSTAT_END_REFCOUNT		},
>> +		{ "extent_alloc",	xfsstats_offset(xs_abt_lookup)	},
>> +		{ "abt",		xfsstats_offset(xs_blk_mapr)	},
>> +		{ "blk_map",		xfsstats_offset(xs_bmbt_lookup)	},
>> +		{ "bmbt",		xfsstats_offset(xs_dir_lookup)	},
>> +		{ "dir",		xfsstats_offset(xs_trans_sync)	},
>> +		{ "trans",		xfsstats_offset(xs_ig_attempts)	},
>> +		{ "ig",			xfsstats_offset(xs_log_writes)	},
>> +		{ "log",		xfsstats_offset(xs_try_logspace)},
>> +		{ "push_ail",		xfsstats_offset(xs_xstrat_quick)},
>> +		{ "xstrat",		xfsstats_offset(xs_write_calls)	},
>> +		{ "rw",			xfsstats_offset(xs_attr_get)	},
>> +		{ "attr",		xfsstats_offset(xs_iflush_count)},
>> +		{ "icluster",		xfsstats_offset(vn_active)	},
>> +		{ "vnodes",		xfsstats_offset(xb_get)		},
>> +		{ "buf",		xfsstats_offset(xs_abtb_2)	},
>> +		{ "abtb2",		xfsstats_offset(xs_abtc_2)	},
>> +		{ "abtc2",		xfsstats_offset(xs_bmbt_2)	},
>> +		{ "bmbt2",		xfsstats_offset(xs_ibt_2)	},
>> +		{ "ibt2",		xfsstats_offset(xs_fibt_2)	},
>> +		{ "fibt2",		xfsstats_offset(xs_rmap_2)	},
>> +		{ "rmapbt",		xfsstats_offset(xs_refcbt_2)	},
>> +		{ "refcntbt",		xfsstats_offset(xs_qm_dqreclaims)},
>>  		/* we print both series of quota information together */
>> -		{ "qm",			XFSSTAT_END_QM			},
>> +		{ "qm",			xfsstats_offset(xs_xstrat_bytes)},
>>  	};
> 
> Now that I look at this more closely , it's a little bit ... odd.
> i.e. the endpoint offset points to the first variable in the next
> line, not the last variable of the stats line described by the text.
> While it works, it just doesn't seem quite right to me.

I had the same thought initially, but talked myself out of it.
And I started to reply here with why, then un-convinced myself again,
and now I'm re-convincing myself.  Bear with me.  ;)

> Can we change it so that the index in the table is the last variable
> of that line? This may require changing the inner loop to a <= check
> rather than a < check, but the table would make more sense this way.
> ie.
> 
> 	{ "extent_alloc",	xfsstats_index(xs_freeb) },
> 	{ "abt",		xfsstats_index(xs_abt_delrec) },
> .....

First off, any change here would definitely require good comments.  :)

So let's remember the the overarching goal here is to make things
more foolproof when the stats structure changes.

struct __xfsstats {
# define XFSSTAT_END_EXTENT_ALLOC       4
        uint32_t                xs_allocx;
        uint32_t                xs_allocb;
        uint32_t                xs_freex;
        uint32_t                xs_freeb;
# define XFSSTAT_END_ALLOC_BTREE        (XFSSTAT_END_EXTENT_ALLOC+4)
        uint32_t                xs_abt_lookup;
        uint32_t                xs_abt_compare;
        uint32_t                xs_abt_insrec;
        uint32_t                xs_abt_delrec;
...


and "what can go wrong" is somebody extends one of the groups without
changing the end, or they insert a group without changing where the xqm
stats start.

The most foolproof thing would be to define the boundaries at the start
of each group, because adding new stats at the end of the line is allowed,
but would require changing the END macros.  i.e. something like:

/*
 * XFS global statistics
 */
struct __xfsstats {
# define XFSSTAT_START_EXTENT_ALLOC     XFS_STATS_CALC_INDEX(xs_allocx)
        uint32_t                xs_allocx;
        uint32_t                xs_allocb;
        uint32_t                xs_freex;
        uint32_t                xs_freeb;
# define XFSSTAT_START_ALLOC_BTREE      XFS_STATS_CALC_INDEX(xs_abt_lookup)
        uint32_t                xs_abt_lookup;
...

and these START points would never change.

Then, the printing loop needs to be adjusted to look forward to the next
group to know when to stop, something like this where we change "endpoint"
to "start" and add a NULL guard at the end,

        } xstats[] = {
                { "extent_alloc",       XFSSTAT_START_EXTENT_ALLOC      },
                { "abt",                XFSSTAT_START_ALLOC_BTRE        },
...
                { "qm",                 XFSSTAT_START_QM                },
                { NULL,                 XFSSTAT_END                     },
        };

then look ahead while we print:

        for (i = j = 0; i < ARRAY_SIZE(xstats) && xstats[i].desc; i++) {
                len += snprintf(buf + len, PATH_MAX - len, "%s",
                                xstats[i].desc);
                /* inner loop does each group */
                for (; j < xstats[i+1].start; j++)
                        len += snprintf(buf + len, PATH_MAX - len, " %u",
                                        counter_val(stats, j));
                len += snprintf(buf + len, PATH_MAX - len, "\n");
        }

testing xstats[i].desc in the outer loop should stop at the last NULL line,
and looking ahead to xstats[i+1].start in the innner loop checks where the
next group will start.

XFSSTAT_END could be defined in terms of ARRAY_SIZE so it's automatic, too.

> 
> It may require a different macro for the expanded btree stats (the
> array based ones) because they are defined as an array rather than
> individual variables, but I think that's ok given those already have
> special macros for changing them.
> 
>> +#define	xfsstats_offset(f)	(offsetof(struct __xfsstats, f)/sizeof(uint32_t))

So, another thing worth pointing out is that we already have "this" macro:

/*
 * simple wrapper for getting the array index of s struct member offset
 */
#define XFS_STATS_CALC_INDEX(member)    \
        (offsetof(struct __xfsstats, member) / (int)sizeof(uint32_t))


so probably no need to define it again w/ another name, probably worth
consolidating.

> I also think this should be named xfsstats_index(), not _offset. i.e.
> it's an index into a structure we treat as an array of ints, not a byte
> offset from the start of a u8 buffer....

agreed, see above :D

>> +
>>  struct xfsstats {
>>  	union {
>>  		struct __xfsstats	s;
>> -		uint32_t		a[XFSSTAT_END_XQMSTAT];
>> +		uint32_t		a[xfsstats_offset(xs_qm_dquot)];
> 
> I think this should remain XFSSTAT_END_XQMSTAT, as we still define
> that and require the quota stats to be at the end of the structure.
> 
> Cheers,
> 
> Dave.
> 

Re: [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats

[Dave Chinner]

On Thu, Oct 11, 2018 at 09:04:14AM -0500, Eric Sandeen wrote:
> On 10/11/18 12:29 AM, Dave Chinner wrote:
> > On Wed, Oct 10, 2018 at 02:37:08PM +0200, Carlos Maiolino wrote:
> >> Most offset macro mess is used in xfs_stats_format() only, and we can
> >> simply get the right offsets using offsetof(), instead of several macros
> >> to mark the offsets inside __xfsstats structure.
> >>
> >> Replace all XFSSTAT_END_* macros by a single helper macro to get the
> >> right offset into __xfsstats, and use this helper in xfs_stats_format()
> >> directly.
> >>
> >> The quota stats code, still looks a bit cleaner when using XFSSTAT_*
> >> macros, so, this patch also defines XFSSTAT_START_XQMSTAT and
> >> XFSSTAT_END_XQMSTAT locally to that code. This also should prevent
> >> offset mistakes when updates are done into __xfsstats.
> >>
> >> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
.....
> > Now that I look at this more closely , it's a little bit ... odd.
> > i.e. the endpoint offset points to the first variable in the next
> > line, not the last variable of the stats line described by the text.
> > While it works, it just doesn't seem quite right to me.
....
> and "what can go wrong" is somebody extends one of the groups without
> changing the end, or they insert a group without changing where the xqm
> stats start.

<snip>

I don't really care that much - if was easy to change it would be
nice, but wasting more than a couple of minutes on it isn't worth
anyone's time. It's not complex, nor is it critical code so we do
not need to bikeshed this to death. I'll just take the original
patch as it's just a replacement of existing code and so is are
acceptable.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats

[Eric Sandeen]

On 10/12/18 2:01 AM, Dave Chinner wrote:
> On Thu, Oct 11, 2018 at 09:04:14AM -0500, Eric Sandeen wrote:
>> On 10/11/18 12:29 AM, Dave Chinner wrote:
>>> On Wed, Oct 10, 2018 at 02:37:08PM +0200, Carlos Maiolino wrote:
>>>> Most offset macro mess is used in xfs_stats_format() only, and we can
>>>> simply get the right offsets using offsetof(), instead of several macros
>>>> to mark the offsets inside __xfsstats structure.
>>>>
>>>> Replace all XFSSTAT_END_* macros by a single helper macro to get the
>>>> right offset into __xfsstats, and use this helper in xfs_stats_format()
>>>> directly.
>>>>
>>>> The quota stats code, still looks a bit cleaner when using XFSSTAT_*
>>>> macros, so, this patch also defines XFSSTAT_START_XQMSTAT and
>>>> XFSSTAT_END_XQMSTAT locally to that code. This also should prevent
>>>> offset mistakes when updates are done into __xfsstats.
>>>>
>>>> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> .....
>>> Now that I look at this more closely , it's a little bit ... odd.
>>> i.e. the endpoint offset points to the first variable in the next
>>> line, not the last variable of the stats line described by the text.
>>> While it works, it just doesn't seem quite right to me.
> ....
>> and "what can go wrong" is somebody extends one of the groups without
>> changing the end, or they insert a group without changing where the xqm
>> stats start.
> 
> <snip>
> 
> I don't really care that much - if was easy to change it would be
> nice, but wasting more than a couple of minutes on it isn't worth
> anyone's time. It's not complex, nor is it critical code so we do
> not need to bikeshed this to death. I'll just take the original
> patch as it's just a replacement of existing code and so is are
> acceptable.

I didn't think I was bikeshedding, just trying to actually achieve the
stated goal of making this whole stats structure foolproof when
new statistics get added.  It's not critical code, but it is a user
interface that we inadvertently broke, and we may break again.

Nothing /wrong/ with the original patch, I'm just not sure it achieves
the goal as well as it could.  I'll test & send a 2nd patch on top of it,
since I've already thought it through, and I do think it would be an
improvement.

-Eric