On 9/30/20 9:25 AM, Darrick J. Wong wrote:
> 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?
Hmm seems like another case of "add more nice things?" killed off the
bugfix.
> Also, does 32-bit XFS have this 9th value?
on my pi it does ;)
# grep qm /proc/fs/xfs/stat
qm 0 0 0 0 0 0 0 0 0
> --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
>>
>
