Re: [PATCH 1/3] xfs_repair: detect v5 featureset mismatches in secondary supers

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, May 16, 2022 at 06:11:53PM +1000, Dave Chinner wrote:
> On Thu, May 12, 2022 at 12:13:29PM -0700, Darrick J. Wong wrote:
> > On Thu, May 12, 2022 at 02:02:33PM -0500, Eric Sandeen wrote:
> > > On 5/5/22 11:05 AM, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > Make sure we detect and correct mismatches between the V5 features
> > > > described in the primary and the secondary superblocks.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > 
> > > 
> > > > +	if ((mp->m_sb.sb_features_incompat ^ sb->sb_features_incompat) &
> > > > +			~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR) {
> > > 
> > > I'd like to add a comment about why XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR is special.
> > > (Why is XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR special? Just because userspace doesn't
> > > bother to set it on all superblocks in the upgrade paths, right?)
> > 
> > Right -- we only set it on the primary super to force users to run
> > xfs_repair.  Repair will clear NEEDSREPAIR on the primary and all
> > secondaries before it exits
> 
> Oh no it doesn't! :)
> 
> I just debugged the persistent xfs/158 failure down to repair only
> writing the secondary superblocks with "features_changed" is true.
> 
> xfs/158 trashes the repair process after setting the inobtcnt and
> needrepair feature bits in the primary superblock. That's the only
> code that sets the internal "features_changed" flag that triggers
> secondary superblock writeback. If repair then dies after this it
> won't get set on the next run without the upgrade options set on the
> command line. Hence we re-run repair without the upgrade feature
> being enabled to check that it still recovers correctly.
> 
> The result is that repair does the right thing in completing the
> feature upgrade because it sees the feature flag in the primary
> superblock, but it *never sets "features_changed"* and so the
> secondary superblocks are not updated when it is done. It then goes
> on to check NEEDSREPAIR in the primary superblock and clears it,
> allowing the fileystem to mount again.
> 
> This results in secondary superblocks with in-progress set and
> mis-matched feature bits, resulting in xfs_scrub reporting corrupt
> secondary superblocks and so failing the test.
> 
> This change will probably mask that specific bug - it'll still be
> lying their waiting to pounce on some unsuspecting bystander, but it
> will be masked by other code detecting the feature mismatch that it
> causes and correcting it that way...

Ah, ok.  You're pointing out that repair needs to set features_changed
right after printing "clearing needsrepair flag and regenerating
metadata", and that this patch sort of papers over the underlying issue.

The genesis of this patch wasn't xfs/158 failing, it was the secondary
sb fuzz testing noticing that xfs_scrub reported a failure whereas
xfs_repair didn't.

So, I'll go add an extra patch to fix the upgrade case, and I think we
can let Eric add this one to his tree.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com