On 9/25/15 6:23 PM, Bill O'Donnell wrote:
> This patch modifies the stats counting macros and the callers
> to those macros to properly increment, decrement, and add-to
> the xfs stats counts. The counts for global and per-fs stats
> are correctly advanced, and cleared by writing a "1" to the
> corresponding clear file.
>
> global counts: /sys/fs/xfs/stats/stats
> per-fs counts: /sys/fs/xfs/sda*/stats/stats
>
> global clear: /sys/fs/xfs/stats/stats_clear
> per-fs clear: /sys/fs/xfs/sda*/stats/stats_clear
>
> Signed-off-by: Bill O'Donnell <billodo@xxxxxxxxxx>
> ---
<big snip>
> diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
> index 8f18bab..e42d969 100644
> --- a/fs/xfs/libxfs/xfs_btree.h
> +++ b/fs/xfs/libxfs/xfs_btree.h
> @@ -84,22 +84,23 @@ union xfs_btree_rec {
> /*
> * Generic stats interface
> */
> -#define __XFS_BTREE_STATS_INC(type, stat) \
> - XFS_STATS_INC(xs_ ## type ## _2_ ## stat)
> -#define XFS_BTREE_STATS_INC(cur, stat) \
> +#define __XFS_BTREE_STATS_INC(mp, type, stat) \
> + XFS_STATS_INC(mp, xs_ ## type ## _2_ ## stat)
> +#define XFS_BTREE_STATS_INC(cur, stat) \
> do { \
> + struct xfs_mount *mp = cur->bc_mp; \
> switch (cur->bc_btnum) { \
> - case XFS_BTNUM_BNO: __XFS_BTREE_STATS_INC(abtb, stat); break; \
> - case XFS_BTNUM_CNT: __XFS_BTREE_STATS_INC(abtc, stat); break; \
> - case XFS_BTNUM_BMAP: __XFS_BTREE_STATS_INC(bmbt, stat); break; \
> - case XFS_BTNUM_INO: __XFS_BTREE_STATS_INC(ibt, stat); break; \
> - case XFS_BTNUM_FINO: __XFS_BTREE_STATS_INC(fibt, stat); break; \
> + case XFS_BTNUM_BNO: __XFS_BTREE_STATS_INC(mp, abtb, stat); break; \
> + case XFS_BTNUM_CNT: __XFS_BTREE_STATS_INC(mp, abtc, stat); break; \
> + case XFS_BTNUM_BMAP: __XFS_BTREE_STATS_INC(mp, bmbt, stat); break; \
> + case XFS_BTNUM_INO: __XFS_BTREE_STATS_INC(mp, ibt, stat); break; \
> + case XFS_BTNUM_FINO: __XFS_BTREE_STATS_INC(mp, fibt, stat); break; \
> case XFS_BTNUM_MAX: ASSERT(0); /* fucking gcc */ ; break; \
> } \
> } while (0)
>
> #define __XFS_BTREE_STATS_ADD(type, stat, val) \
> - XFS_STATS_ADD(xs_ ## type ## _2_ ## stat, val)
> + XFS_STATS_ADD(cur->bc_mp, xs_ ## type ## _2_ ## stat, val)
Where does "cur" come from? XFS_BTREE_STATS_ADD should grab mp from
cur & pass it to __XFS_BTREE_STATS_ADD like XFS_BTREE_STATS_INC did,
otherwise you're relying on the macro being called from somewhere with
a local variable named "cur" - macros referencing local vars @ the callpoint
are a bad idea.
> #define XFS_BTREE_STATS_ADD(cur, stat, val) \
> do { \
> switch (cur->bc_btnum) { \
> @@ -108,7 +109,7 @@ do { \
> case XFS_BTNUM_BMAP: __XFS_BTREE_STATS_ADD(bmbt, stat, val); break; \
> case XFS_BTNUM_INO: __XFS_BTREE_STATS_ADD(ibt, stat, val); break; \
> case XFS_BTNUM_FINO: __XFS_BTREE_STATS_ADD(fibt, stat, val); break; \
> - case XFS_BTNUM_MAX: ASSERT(0); /* fucking gcc */ ; break; \
> + case XFS_BTNUM_MAX: ASSERT(0); /* fucking gcc */ ; break; \
> } \
> } while (0)
>
<snip>
> diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
> index d5b3704..d3c8da3 100644
> --- a/fs/xfs/xfs_stats.c
> +++ b/fs/xfs/xfs_stats.c
> @@ -122,11 +122,9 @@ 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(xfsstats.xs_stats, XFSSTAT_END_XQMSTAT),
> - 0,
> - counter_val(xfsstats.xs_stats,
> - XFSSTAT_END_XQMSTAT + 1));
> + 0, counter_val(xfsstats.xs_stats, XFSSTAT_END_XQMSTAT),
> + 0, counter_val(xfsstats.xs_stats,
> + XFSSTAT_END_XQMSTAT + 1));
unrelated whitespace stuff; if it needs to be reformatted, do it in the
patch that introduced the formatting you don't like.
> return 0;
> }
>
> diff --git a/fs/xfs/xfs_stats.h b/fs/xfs/xfs_stats.h
> index 00afc13..7e84702 100644
> --- a/fs/xfs/xfs_stats.h
> +++ b/fs/xfs/xfs_stats.h
> @@ -224,10 +224,25 @@ struct xfs_mount;
> * 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.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))
> +#define XFS_STATS_INC(mp, v) { per_cpu_ptr(xfsstats.xs_stats, \
> + current_cpu())->v++; \
> + per_cpu_ptr( \
> + mp->m_stats.xs_stats, \
> + current_cpu())->v++; }
> +
kind of odd style here; I'd do:
+#define XFS_STATS_INC(mp, v) \
+do { \
+ per_cpu_ptr(xfsstats.xs_stats, current_cpu())->v++; \
+ per_cpu_ptr(mp->m_stats.xs_stats, current_cpu())->v++; \
+} while (0)
(that do { } while (0) idiom is odd, and I think it has fallen out of favor,
but it's how the other multiline stats macros work - see XFS_BTREE_STATS_ADD
for example)
But honestly, maybe this should turn into a static inline. I guess we have
plenty of multiline macros already...
and really, those per_cpu()'s should have been made per_cpu_ptr's in the patch
which introduced the xfsstats.xs_stats usage.
> +#define XFS_STATS_DEC(mp, v) { per_cpu_ptr(xfsstats.xs_stats, \
> + current_cpu())->v++; \
> + per_cpu_ptr( \
> + mp->m_stats.xs_stats, \
> + current_cpu())->v--; }
> +
> +#define XFS_STATS_ADD(mp, v, inc) { per_cpu_ptr(xfsstats.xs_stats, \
> + current_cpu())->v \
> + += (inc); \
> + per_cpu_ptr( \
> + mp->m_stats.xs_stats, \
> + current_cpu())->v \
> + += (inc); }
(here too of course)
-Eric
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
