| Message ID | Yo02nmlajIuFqVez@magnolia (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs_repair: don't flag log_incompat inconsistencies as corruptions | expand |
On 5/24/22 2:48 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > While testing xfs/233 and xfs/127 with LARP mode enabled, I noticed > errors such as the following: > > xfs_growfs --BlockSize=4096 --Blocks=8192 > data blocks changed from 8192 to 2579968 > meta-data=/dev/sdf isize=512 agcount=630, agsize=4096 blks > = sectsz=512 attr=2, projid32bit=1 > = crc=1 finobt=1, sparse=1, rmapbt=1 > = reflink=1 bigtime=1 inobtcount=1 > data = bsize=4096 blocks=2579968, imaxpct=25 > = sunit=0 swidth=0 blks > naming =version 2 bsize=4096 ascii-ci=0, ftype=1 > log =internal log bsize=4096 blocks=3075, version=2 > = sectsz=512 sunit=0 blks, lazy-count=1 > realtime =none extsz=4096 blocks=0, rtextents=0 > _check_xfs_filesystem: filesystem on /dev/sdf is inconsistent (r) > *** xfs_repair -n output *** > Phase 1 - find and verify superblock... > - reporting progress in intervals of 15 minutes > Phase 2 - using internal log > - zero log... > - 23:03:47: zeroing log - 3075 of 3075 blocks done > - scan filesystem freespace and inode maps... > would fix log incompat feature mismatch in AG 30 super, 0x0 != 0x1 > would fix log incompat feature mismatch in AG 8 super, 0x0 != 0x1 > would fix log incompat feature mismatch in AG 12 super, 0x0 != 0x1 > would fix log incompat feature mismatch in AG 24 super, 0x0 != 0x1 > would fix log incompat feature mismatch in AG 18 super, 0x0 != 0x1 > <snip> > > 0x1 corresponds to XFS_SB_FEAT_INCOMPAT_LOG_XATTRS, which is the feature > bit used to indicate that the log contains extended attribute log intent > items. This is a mechanism to prevent older kernels from trying to > recover log items that they won't know how to recover. > > I thought about this a little bit more, and realized that log_incompat > features bits are set on the primary sb prior to writing certain types > of log records, and cleared once the log has written the committed > changes back to the filesystem. If the secondary superblocks are > updated at any point during that interval (due to things like growfs or > setting labels), the log_incompat field will now be set on the secondary > supers. > > Due to the ephemeral nature of the current log_incompat feature bits, > a discrepancy between the primary and secondary supers is not a > corruption. If we're in dry run mode, we should log the discrepancy, > but that's not a reason to end with EXIT_FAILURE. Interesting. This makes me wonder a few things. This approach differs from the just-added handling of XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR, where we /always/ ignore it. For now I think that's a little different, because that flag only gets set from userspace, but that could change in the future, maybe? So I wonder why we have this feature getting noted and cleared, but the other one always ignored. I also notice that scrub tries to avoid setting it in the first place: * Don't write out a secondary super with NEEDSREPAIR or log incompat * features set, since both are ignored when set on a secondary. ... should growfs avoid it as well? It feels like we're spreading this special handling around, copying (or not) and ignoring (or not) at various points. I kinda want to step back and think about this a little. It seems like the most consistent approach would be to always keep all supers in sync, though I suppose that has costs. The 2nd most consistent approach would be to never copy these ephemeral features to the secondary. Whatever the consistent future looks like, I guess we do have to deal with inconsistent stuff in the wild, already. Thoughts? -Eric > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > repair/agheader.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/repair/agheader.c b/repair/agheader.c > index 2c2a26d1..478ed7e5 100644 > --- a/repair/agheader.c > +++ b/repair/agheader.c > @@ -286,15 +286,24 @@ check_v5_feature_mismatch( > } > } > > + /* > + * Log incompat feature bits are set and cleared from the primary super > + * as needed to protect against log replay on old kernels finding log > + * records that they cannot handle. Secondary sb resyncs performed as > + * part of a geometry update to the primary sb (e.g. growfs, label/uuid > + * changes) will copy the log incompat feature bits, but it's not a > + * corruption for a secondary to have a bit set that is clear in the > + * primary super. > + */ > if (mp->m_sb.sb_features_log_incompat != sb->sb_features_log_incompat) { > if (no_modify) { > - do_warn( > - _("would fix log incompat feature mismatch in AG %u super, 0x%x != 0x%x\n"), > + do_log( > + _("would sync log incompat feature in AG %u super, 0x%x != 0x%x\n"), > agno, mp->m_sb.sb_features_log_incompat, > sb->sb_features_log_incompat); > } else { > do_warn( > - _("will fix log incompat feature mismatch in AG %u super, 0x%x != 0x%x\n"), > + _("will sync log incompat feature in AG %u super, 0x%x != 0x%x\n"), > agno, mp->m_sb.sb_features_log_incompat, > sb->sb_features_log_incompat); > dirty = true; >
On Thu, May 26, 2022 at 04:53:30PM -0500, Eric Sandeen wrote: > On 5/24/22 2:48 PM, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > While testing xfs/233 and xfs/127 with LARP mode enabled, I noticed > > errors such as the following: > > > > xfs_growfs --BlockSize=4096 --Blocks=8192 > > data blocks changed from 8192 to 2579968 > > meta-data=/dev/sdf isize=512 agcount=630, agsize=4096 blks > > = sectsz=512 attr=2, projid32bit=1 > > = crc=1 finobt=1, sparse=1, rmapbt=1 > > = reflink=1 bigtime=1 inobtcount=1 > > data = bsize=4096 blocks=2579968, imaxpct=25 > > = sunit=0 swidth=0 blks > > naming =version 2 bsize=4096 ascii-ci=0, ftype=1 > > log =internal log bsize=4096 blocks=3075, version=2 > > = sectsz=512 sunit=0 blks, lazy-count=1 > > realtime =none extsz=4096 blocks=0, rtextents=0 > > _check_xfs_filesystem: filesystem on /dev/sdf is inconsistent (r) > > *** xfs_repair -n output *** > > Phase 1 - find and verify superblock... > > - reporting progress in intervals of 15 minutes > > Phase 2 - using internal log > > - zero log... > > - 23:03:47: zeroing log - 3075 of 3075 blocks done > > - scan filesystem freespace and inode maps... > > would fix log incompat feature mismatch in AG 30 super, 0x0 != 0x1 > > would fix log incompat feature mismatch in AG 8 super, 0x0 != 0x1 > > would fix log incompat feature mismatch in AG 12 super, 0x0 != 0x1 > > would fix log incompat feature mismatch in AG 24 super, 0x0 != 0x1 > > would fix log incompat feature mismatch in AG 18 super, 0x0 != 0x1 > > <snip> > > > > 0x1 corresponds to XFS_SB_FEAT_INCOMPAT_LOG_XATTRS, which is the feature > > bit used to indicate that the log contains extended attribute log intent > > items. This is a mechanism to prevent older kernels from trying to > > recover log items that they won't know how to recover. > > > > I thought about this a little bit more, and realized that log_incompat > > features bits are set on the primary sb prior to writing certain types > > of log records, and cleared once the log has written the committed > > changes back to the filesystem. If the secondary superblocks are > > updated at any point during that interval (due to things like growfs or > > setting labels), the log_incompat field will now be set on the secondary > > supers. > > > > Due to the ephemeral nature of the current log_incompat feature bits, > > a discrepancy between the primary and secondary supers is not a > > corruption. If we're in dry run mode, we should log the discrepancy, > > but that's not a reason to end with EXIT_FAILURE. > > Interesting. This makes me wonder a few things. > > This approach differs from the just-added handling of > XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR, where we /always/ ignore it. For now I think > that's a little different, because that flag only gets set from userspace, but > that could change in the future, maybe? Yes, though I don't have any plans to change that. > So I wonder why we have this feature getting noted and cleared, but the other > one always ignored. NEEDSREPAIR is different from log_incompat -- if it's set on the primary sb, then mount will reject the filesystem. I don't think any code ever makes any decisions based on NEEDSREPAIR bit in the secondary supers. If xfs_repair recovers the primary sb from a secondary, it will always clear needsrepair on the primary sb when it's done. Adding a feature via xfs_repair causes it to sync the secondary supers, but we're careful to mask off the NEEDSREPAIR bit first. I guess right now check_v5_feature_mismatch can accidentally propagate needsrepair from a primary to a secondary sb. > I also notice that scrub tries to avoid setting it in the first place: > > * Don't write out a secondary super with NEEDSREPAIR or log incompat > * features set, since both are ignored when set on a secondary. > > ... should growfs avoid it as well? > > It feels like we're spreading this special handling around, copying (or not) > and ignoring (or not) at various points. I kinda want to step back and think > about this a little. > > It seems like the most consistent approach would be to always keep all supers > in sync, though I suppose that has costs. The 2nd most consistent approach would > be to never copy these ephemeral features to the secondary. > > Whatever the consistent future looks like, I guess we do have to deal with > inconsistent stuff in the wild, already. There aren't any filesystems in the wild with log_incompat bits set, so we still have time to sort this one out. I think it might actually be an error that online repair zeroes the log_incompat bits when resetting the primary superblock, and that we don't update the secondaries when we set or clear a bit on the primary. If the system goes down and the primary sb somehow gets wiped, the fs will be unmountable. xfs_repair will try to find a secondary, and if it does, it writes that onto the primary sb. If the primary had a log_incompat bit set but the secondary didn't, we've effectively cleared that bit in the primary super. If repair then finds that the log is dirty, it will tell the user to try to mount the fs to recover the log. If the kernel is too old to handle the log contents (say you're booting from a Knoppix CD) and those log items would have been protected by that log_incompat bit in the primary sb, then recovery starts up and will fail at some point midway through. Soooo ... I've changed my mind. :P Now I've convinced myself that the code that clears and sets the LARP log-incompat bit needs to update the secondaries and online fsck needs to make log-incompat discrepancies a real error (and a real fix). xfs_repair was actually DTRT and this patch should be ignored. More thoughts? :D --D > > Thoughts? > > -Eric > > > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > repair/agheader.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/repair/agheader.c b/repair/agheader.c > > index 2c2a26d1..478ed7e5 100644 > > --- a/repair/agheader.c > > +++ b/repair/agheader.c > > @@ -286,15 +286,24 @@ check_v5_feature_mismatch( > > } > > } > > > > + /* > > + * Log incompat feature bits are set and cleared from the primary super > > + * as needed to protect against log replay on old kernels finding log > > + * records that they cannot handle. Secondary sb resyncs performed as > > + * part of a geometry update to the primary sb (e.g. growfs, label/uuid > > + * changes) will copy the log incompat feature bits, but it's not a > > + * corruption for a secondary to have a bit set that is clear in the > > + * primary super. > > + */ > > if (mp->m_sb.sb_features_log_incompat != sb->sb_features_log_incompat) { > > if (no_modify) { > > - do_warn( > > - _("would fix log incompat feature mismatch in AG %u super, 0x%x != 0x%x\n"), > > + do_log( > > + _("would sync log incompat feature in AG %u super, 0x%x != 0x%x\n"), > > agno, mp->m_sb.sb_features_log_incompat, > > sb->sb_features_log_incompat); > > } else { > > do_warn( > > - _("will fix log incompat feature mismatch in AG %u super, 0x%x != 0x%x\n"), > > + _("will sync log incompat feature in AG %u super, 0x%x != 0x%x\n"), > > agno, mp->m_sb.sb_features_log_incompat, > > sb->sb_features_log_incompat); > > dirty = true; > > >
diff --git a/repair/agheader.c b/repair/agheader.c index 2c2a26d1..478ed7e5 100644 --- a/repair/agheader.c +++ b/repair/agheader.c @@ -286,15 +286,24 @@ check_v5_feature_mismatch( } } + /* + * Log incompat feature bits are set and cleared from the primary super + * as needed to protect against log replay on old kernels finding log + * records that they cannot handle. Secondary sb resyncs performed as + * part of a geometry update to the primary sb (e.g. growfs, label/uuid + * changes) will copy the log incompat feature bits, but it's not a + * corruption for a secondary to have a bit set that is clear in the + * primary super. + */ if (mp->m_sb.sb_features_log_incompat != sb->sb_features_log_incompat) { if (no_modify) { - do_warn( - _("would fix log incompat feature mismatch in AG %u super, 0x%x != 0x%x\n"), + do_log( + _("would sync log incompat feature in AG %u super, 0x%x != 0x%x\n"), agno, mp->m_sb.sb_features_log_incompat, sb->sb_features_log_incompat); } else { do_warn( - _("will fix log incompat feature mismatch in AG %u super, 0x%x != 0x%x\n"), + _("will sync log incompat feature in AG %u super, 0x%x != 0x%x\n"), agno, mp->m_sb.sb_features_log_incompat, sb->sb_features_log_incompat); dirty = true;