On Wed, Sep 30, 2020 at 04:35:31PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> There are 8 quota stats exposed, but:
>
> $ grep qm /proc/fs/xfs/stat
> qm 0 0 0 1889 308893 0 0 0 0
> $
>
> There are 9 values exposed. Code inspection reveals that the struct
> xfsstats has a hole in the structure where the values change from 32
> bit counters to 64 bit counters. pahole output:
>
> ....
> uint32_t xs_qm_dquot; /* 748 4 */
> uint32_t xs_qm_dquot_unused; /* 752 4 */
>
> /* XXX 4 bytes hole, try to pack */
>
> uint64_t xs_xstrat_bytes; /* 760 8 */
Any chance we could use BUILD_BUG_ON(offsetof(xs_xstrat_bytes) % 8 == 0)
to catch this kind of thing on 64-bit machines in the future? Or maybe
we shift the u64 values to the start of the structure to avoid padding
holes?
Also, does 32-bit XFS have this 9th value?
--D
> ....
>
> Fix this by defining an "end of 32 bit variables" variable that
> we then use to define the end of the quota line. This will then
> ensure that we print the correct number of values regardless of
> structure layout.
>
> However, ABI requirements for userspace parsers mean we cannot
> remove the output that results from this hole, so we also need to
> explicitly define this unused value until such time that we actually
> add a new stat that makes the output meaningful.
>
> And now we have a defined end of 32bit variables, update the stats
> union to be sized to match all the 32 bit variables correctly.
>
> Output with this patch:
>
> $ grep qm /proc/fs/xfs/stat
> qm 0 0 0 326 1802 0 6 3 0
> $
>
> Reported-by: Nathan Scott <nathans@xxxxxxxxxx>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/xfs_stats.c | 2 +-
> fs/xfs/xfs_stats.h | 15 +++++++++++++--
> 2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/fs/xfs/xfs_stats.c b/fs/xfs/xfs_stats.c
> index f70f1255220b..3409b273f00a 100644
> --- a/fs/xfs/xfs_stats.c
> +++ b/fs/xfs/xfs_stats.c
> @@ -51,7 +51,7 @@ int xfs_stats_format(struct xfsstats __percpu *stats, char *buf)
> { "rmapbt", xfsstats_offset(xs_refcbt_2) },
> { "refcntbt", xfsstats_offset(xs_qm_dqreclaims)},
> /* we print both series of quota information together */
> - { "qm", xfsstats_offset(xs_xstrat_bytes)},
> + { "qm", xfsstats_offset(xs_end_of_32bit_counts)},
> };
>
> /* Loop over all stats groups */
> diff --git a/fs/xfs/xfs_stats.h b/fs/xfs/xfs_stats.h
> index 34d704f703d2..861acf84cb3c 100644
> --- a/fs/xfs/xfs_stats.h
> +++ b/fs/xfs/xfs_stats.h
> @@ -133,6 +133,17 @@ struct __xfsstats {
> uint32_t xs_qm_dqwants;
> uint32_t xs_qm_dquot;
> uint32_t xs_qm_dquot_unused;
> + uint32_t xs_qm_zero_until_next_stat_is_added;
> +
> +/*
> + * Define the end of 32 bit counters as a 32 bit variable so that we don't
> + * end up exposing an implicit structure padding hole due to the next counters
> + * being 64bit values. If the number of coutners is odd, this fills the hole. If
> + * the number of coutners is even the hole is after this variable and the stats
> + * line will terminate printing at this offset and not print the hole.
> + */
> + uint32_t xs_end_of_32bit_counts;
> +
> /* Extra precision counters */
> uint64_t xs_xstrat_bytes;
> uint64_t xs_write_bytes;
> @@ -143,8 +154,8 @@ struct __xfsstats {
>
> struct xfsstats {
> union {
> - struct __xfsstats s;
> - uint32_t a[xfsstats_offset(xs_qm_dquot)];
> + struct __xfsstats s;
> + uint32_t a[xfsstats_offset(xs_end_of_32bit_counts)];
> };
> };
>
> --
> 2.28.0
>
