Re: [PATCH 1/2] xfs: stats expose padding value at end of qm line

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Sep 30, 2020 at 04:35:31PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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@redhat.com>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  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
>