* [PATCH 0/2] xfs stats fixes - V2 @ 2018-10-10 12:37 Carlos Maiolino 2018-10-10 12:37 ` [PATCH 1/2] xfs: Fix xqmstats offsets in /proc/fs/xfs/xqmstat Carlos Maiolino 2018-10-10 12:37 ` [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats Carlos Maiolino 0 siblings, 2 replies; 11+ messages in thread From: Carlos Maiolino @ 2018-10-10 12:37 UTC (permalink / raw) To: linux-xfs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] xfs: Fix xqmstats offsets in /proc/fs/xfs/xqmstat 2018-10-10 12:37 [PATCH 0/2] xfs stats fixes - V2 Carlos Maiolino @ 2018-10-10 12:37 ` Carlos Maiolino 2018-10-10 12:37 ` [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats Carlos Maiolino 1 sibling, 0 replies; 11+ messages in thread From: Carlos Maiolino @ 2018-10-10 12:37 UTC (permalink / raw) To: linux-xfs 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats 2018-10-10 12:37 [PATCH 0/2] xfs stats fixes - V2 Carlos Maiolino 2018-10-10 12:37 ` [PATCH 1/2] xfs: Fix xqmstats offsets in /proc/fs/xfs/xqmstat Carlos Maiolino @ 2018-10-10 12:37 ` Carlos Maiolino 2018-10-10 14:49 ` Darrick J. Wong 2018-10-11 5:29 ` Dave Chinner 1 sibling, 2 replies; 11+ messages in thread From: Carlos Maiolino @ 2018-10-10 12:37 UTC (permalink / raw) To: linux-xfs 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats 2018-10-10 12:37 ` [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats Carlos Maiolino @ 2018-10-10 14:49 ` Darrick J. Wong 2018-10-10 14:58 ` Carlos Maiolino 2018-10-11 5:29 ` Dave Chinner 1 sibling, 1 reply; 11+ messages in thread From: Darrick J. Wong @ 2018-10-10 14:49 UTC (permalink / raw) To: Carlos Maiolino; +Cc: linux-xfs 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats 2018-10-10 14:49 ` Darrick J. Wong @ 2018-10-10 14:58 ` Carlos Maiolino 2018-10-10 15:02 ` Darrick J. Wong 0 siblings, 1 reply; 11+ messages in thread From: Carlos Maiolino @ 2018-10-10 14:58 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs > > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats 2018-10-10 14:58 ` Carlos Maiolino @ 2018-10-10 15:02 ` Darrick J. Wong 2018-10-10 21:28 ` Dave Chinner 0 siblings, 1 reply; 11+ messages in thread From: Darrick J. Wong @ 2018-10-10 15:02 UTC (permalink / raw) To: linux-xfs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats 2018-10-10 15:02 ` Darrick J. Wong @ 2018-10-10 21:28 ` Dave Chinner 0 siblings, 0 replies; 11+ messages in thread From: Dave Chinner @ 2018-10-10 21:28 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats 2018-10-10 12:37 ` [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats Carlos Maiolino 2018-10-10 14:49 ` Darrick J. Wong @ 2018-10-11 5:29 ` Dave Chinner 2018-10-11 14:04 ` Eric Sandeen 1 sibling, 1 reply; 11+ messages in thread From: Dave Chinner @ 2018-10-11 5:29 UTC (permalink / raw) To: Carlos Maiolino; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats 2018-10-11 5:29 ` Dave Chinner @ 2018-10-11 14:04 ` Eric Sandeen 2018-10-12 7:01 ` Dave Chinner 0 siblings, 1 reply; 11+ messages in thread From: Eric Sandeen @ 2018-10-11 14:04 UTC (permalink / raw) To: Dave Chinner, Carlos Maiolino; +Cc: linux-xfs 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. > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats 2018-10-11 14:04 ` Eric Sandeen @ 2018-10-12 7:01 ` Dave Chinner 2018-10-12 15:00 ` Eric Sandeen 0 siblings, 1 reply; 11+ messages in thread From: Dave Chinner @ 2018-10-12 7:01 UTC (permalink / raw) To: Eric Sandeen; +Cc: Carlos Maiolino, linux-xfs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats 2018-10-12 7:01 ` Dave Chinner @ 2018-10-12 15:00 ` Eric Sandeen 0 siblings, 0 replies; 11+ messages in thread From: Eric Sandeen @ 2018-10-12 15:00 UTC (permalink / raw) To: Dave Chinner; +Cc: Carlos Maiolino, linux-xfs 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-10-12 22:33 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-10-10 12:37 [PATCH 0/2] xfs stats fixes - V2 Carlos Maiolino 2018-10-10 12:37 ` [PATCH 1/2] xfs: Fix xqmstats offsets in /proc/fs/xfs/xqmstat Carlos Maiolino 2018-10-10 12:37 ` [PATCH 2/2] xfs: use offsetof() in place of offset macros for __xfsstats Carlos Maiolino 2018-10-10 14:49 ` Darrick J. Wong 2018-10-10 14:58 ` Carlos Maiolino 2018-10-10 15:02 ` Darrick J. Wong 2018-10-10 21:28 ` Dave Chinner 2018-10-11 5:29 ` Dave Chinner 2018-10-11 14:04 ` Eric Sandeen 2018-10-12 7:01 ` Dave Chinner 2018-10-12 15:00 ` Eric Sandeen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).