[PATCH 0/2] xfs: a couple of small fixes

[PATCH 0/2] xfs: a couple of small fixes

[Dave Chinner]

Hi folks,

These are fixes for the problems that Nathan Scott brought to our
attention earlier this afternoon on #xfs. Many thanks to Nathan for
finding these issues.

Cheers,

Dave.

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

[Dave Chinner]

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 */
....

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

[PATCH 2/2] xfs: fix finobt btree block recovery ordering

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Nathan popped up on #xfs and pointed out that we fail to handle
finobt btree blocks in xlog_recover_get_buf_lsn(). This means they
always fall through the entire magic number matching code to "recover
immediately". Whilst most of the time this is the correct behaviour,
occasionally it will be incorrect and could potentially overwrite
more recent metadata because we don't check the LSN in the on disk
metadata at all.

This bug has been present since the finobt was first introduced, and
is a potential cause of the occasional xfs_iget_check_free_state()
failures we see that indicate that the inode btree state does not
match the on disk inode state.

Fixes: aafc3c246529 ("xfs: support the XFS_BTNUM_FINOBT free inode btree type")
Reported-by: Nathan Scott <nathans@redhat.com>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_buf_item_recover.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
index 24c7a8d11e1a..d44e8b4a3391 100644
--- a/fs/xfs/xfs_buf_item_recover.c
+++ b/fs/xfs/xfs_buf_item_recover.c
@@ -719,6 +719,8 @@ xlog_recover_get_buf_lsn(
 	case XFS_ABTC_MAGIC:
 	case XFS_RMAP_CRC_MAGIC:
 	case XFS_REFC_CRC_MAGIC:
+	case XFS_FIBT_CRC_MAGIC:
+	case XFS_FIBT_MAGIC:
 	case XFS_IBT_CRC_MAGIC:
 	case XFS_IBT_MAGIC: {
 		struct xfs_btree_block *btb = blk;
-- 
2.28.0

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

[Darrick J. Wong]

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
> 

Re: [PATCH 2/2] xfs: fix finobt btree block recovery ordering

[Darrick J. Wong]

On Wed, Sep 30, 2020 at 04:35:32PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Nathan popped up on #xfs and pointed out that we fail to handle
> finobt btree blocks in xlog_recover_get_buf_lsn(). This means they
> always fall through the entire magic number matching code to "recover
> immediately". Whilst most of the time this is the correct behaviour,
> occasionally it will be incorrect and could potentially overwrite
> more recent metadata because we don't check the LSN in the on disk
> metadata at all.
> 
> This bug has been present since the finobt was first introduced, and
> is a potential cause of the occasional xfs_iget_check_free_state()
> failures we see that indicate that the inode btree state does not
> match the on disk inode state.
> 
> Fixes: aafc3c246529 ("xfs: support the XFS_BTNUM_FINOBT free inode btree type")
> Reported-by: Nathan Scott <nathans@redhat.com>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Aha, I saw that once...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

Hmmm, zooming out a bit, what kinds of buffers get logged that don't
have magics?  Realtime bitmap and summary?  Can anyone else think of
something? ;)

--D

> ---
>  fs/xfs/xfs_buf_item_recover.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
> index 24c7a8d11e1a..d44e8b4a3391 100644
> --- a/fs/xfs/xfs_buf_item_recover.c
> +++ b/fs/xfs/xfs_buf_item_recover.c
> @@ -719,6 +719,8 @@ xlog_recover_get_buf_lsn(
>  	case XFS_ABTC_MAGIC:
>  	case XFS_RMAP_CRC_MAGIC:
>  	case XFS_REFC_CRC_MAGIC:
> +	case XFS_FIBT_CRC_MAGIC:
> +	case XFS_FIBT_MAGIC:
>  	case XFS_IBT_CRC_MAGIC:
>  	case XFS_IBT_MAGIC: {
>  		struct xfs_btree_block *btb = blk;
> -- 
> 2.28.0
> 

Re: [PATCH 2/2] xfs: fix finobt btree block recovery ordering

[Brian Foster]

On Wed, Sep 30, 2020 at 04:35:32PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Nathan popped up on #xfs and pointed out that we fail to handle
> finobt btree blocks in xlog_recover_get_buf_lsn(). This means they
> always fall through the entire magic number matching code to "recover
> immediately". Whilst most of the time this is the correct behaviour,
> occasionally it will be incorrect and could potentially overwrite
> more recent metadata because we don't check the LSN in the on disk
> metadata at all.
> 
> This bug has been present since the finobt was first introduced, and
> is a potential cause of the occasional xfs_iget_check_free_state()
> failures we see that indicate that the inode btree state does not
> match the on disk inode state.
> 
> Fixes: aafc3c246529 ("xfs: support the XFS_BTNUM_FINOBT free inode btree type")
> Reported-by: Nathan Scott <nathans@redhat.com>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_buf_item_recover.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
> index 24c7a8d11e1a..d44e8b4a3391 100644
> --- a/fs/xfs/xfs_buf_item_recover.c
> +++ b/fs/xfs/xfs_buf_item_recover.c
> @@ -719,6 +719,8 @@ xlog_recover_get_buf_lsn(
>  	case XFS_ABTC_MAGIC:
>  	case XFS_RMAP_CRC_MAGIC:
>  	case XFS_REFC_CRC_MAGIC:
> +	case XFS_FIBT_CRC_MAGIC:
> +	case XFS_FIBT_MAGIC:
>  	case XFS_IBT_CRC_MAGIC:
>  	case XFS_IBT_MAGIC: {
>  		struct xfs_btree_block *btb = blk;
> -- 
> 2.28.0
> 

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

[Eric Sandeen]

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@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?

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@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
>>
> 

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

[Eric Sandeen]

On 9/30/20 1:35 AM, 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 */
> ....
> 
> 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>

This makes sense, and fixes a real bug, so seems like it could
get merged:

Reviewed-by: Eric Sandeen <sandeen@redhat.com>


Now, I could bikeshed by suggesting that we avoid holes by doing something like:


        static const struct xstats_entry {
                char    *desc;
                int     last;
        } xstats[] = {
                { "extent_alloc",       xfsstats_last(xs_freeb)  },
                { "abt",                xfsstats_last(xs_abt_insrec)    },
...

#define xfsstats_last(f)      ((offsetof(struct __xfsstats, f)+sizeof(f))/sizeof(uint32_t))

etc, so that we point to the last group item, instead of pointing to the next
group's first item, possibly exposing a hole in between.

but maybe that's just rearranging deck chairs, and I don't have a very strong
opinion on this.

-Eric


> ---
>  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)];
>  	};
>  };
>  
>