* [PATCH 0/2] xfs: a couple of small fixes @ 2020-09-30 6:35 Dave Chinner 2020-09-30 6:35 ` [PATCH 1/2] xfs: stats expose padding value at end of qm line Dave Chinner 2020-09-30 6:35 ` [PATCH 2/2] xfs: fix finobt btree block recovery ordering Dave Chinner 0 siblings, 2 replies; 8+ messages in thread From: Dave Chinner @ 2020-09-30 6:35 UTC (permalink / raw) To: linux-xfs; +Cc: nathans 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] xfs: stats expose padding value at end of qm line 2020-09-30 6:35 [PATCH 0/2] xfs: a couple of small fixes Dave Chinner @ 2020-09-30 6:35 ` Dave Chinner 2020-09-30 14:25 ` Darrick J. Wong 2021-02-16 19:15 ` Eric Sandeen 2020-09-30 6:35 ` [PATCH 2/2] xfs: fix finobt btree block recovery ordering Dave Chinner 1 sibling, 2 replies; 8+ messages in thread From: Dave Chinner @ 2020-09-30 6:35 UTC (permalink / raw) To: linux-xfs; +Cc: nathans 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] xfs: stats expose padding value at end of qm line 2020-09-30 6:35 ` [PATCH 1/2] xfs: stats expose padding value at end of qm line Dave Chinner @ 2020-09-30 14:25 ` Darrick J. Wong 2021-02-16 19:08 ` Eric Sandeen 2021-02-16 19:15 ` Eric Sandeen 1 sibling, 1 reply; 8+ messages in thread From: Darrick J. Wong @ 2020-09-30 14:25 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs, nathans 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] xfs: stats expose padding value at end of qm line 2020-09-30 14:25 ` Darrick J. Wong @ 2021-02-16 19:08 ` Eric Sandeen 0 siblings, 0 replies; 8+ messages in thread From: Eric Sandeen @ 2021-02-16 19:08 UTC (permalink / raw) To: Darrick J. Wong, Dave Chinner; +Cc: linux-xfs, nathans 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 >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] xfs: stats expose padding value at end of qm line 2020-09-30 6:35 ` [PATCH 1/2] xfs: stats expose padding value at end of qm line Dave Chinner 2020-09-30 14:25 ` Darrick J. Wong @ 2021-02-16 19:15 ` Eric Sandeen 1 sibling, 0 replies; 8+ messages in thread From: Eric Sandeen @ 2021-02-16 19:15 UTC (permalink / raw) To: Dave Chinner, linux-xfs; +Cc: nathans 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)]; > }; > }; > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] xfs: fix finobt btree block recovery ordering 2020-09-30 6:35 [PATCH 0/2] xfs: a couple of small fixes Dave Chinner 2020-09-30 6:35 ` [PATCH 1/2] xfs: stats expose padding value at end of qm line Dave Chinner @ 2020-09-30 6:35 ` Dave Chinner 2020-09-30 14:28 ` Darrick J. Wong 2020-09-30 14:51 ` Brian Foster 1 sibling, 2 replies; 8+ messages in thread From: Dave Chinner @ 2020-09-30 6:35 UTC (permalink / raw) To: linux-xfs; +Cc: nathans 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] xfs: fix finobt btree block recovery ordering 2020-09-30 6:35 ` [PATCH 2/2] xfs: fix finobt btree block recovery ordering Dave Chinner @ 2020-09-30 14:28 ` Darrick J. Wong 2020-09-30 14:51 ` Brian Foster 1 sibling, 0 replies; 8+ messages in thread From: Darrick J. Wong @ 2020-09-30 14:28 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs, nathans 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] xfs: fix finobt btree block recovery ordering 2020-09-30 6:35 ` [PATCH 2/2] xfs: fix finobt btree block recovery ordering Dave Chinner 2020-09-30 14:28 ` Darrick J. Wong @ 2020-09-30 14:51 ` Brian Foster 1 sibling, 0 replies; 8+ messages in thread From: Brian Foster @ 2020-09-30 14:51 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs, nathans 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-02-16 19:16 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-30 6:35 [PATCH 0/2] xfs: a couple of small fixes Dave Chinner 2020-09-30 6:35 ` [PATCH 1/2] xfs: stats expose padding value at end of qm line Dave Chinner 2020-09-30 14:25 ` Darrick J. Wong 2021-02-16 19:08 ` Eric Sandeen 2021-02-16 19:15 ` Eric Sandeen 2020-09-30 6:35 ` [PATCH 2/2] xfs: fix finobt btree block recovery ordering Dave Chinner 2020-09-30 14:28 ` Darrick J. Wong 2020-09-30 14:51 ` Brian Foster
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.