Re: [PATCH 3/3] xfs_repair: clear the needsrepair flag

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

On Thu, Jan 14, 2021 at 04:39:42AM -0500, Brian Foster wrote:
> On Wed, Jan 13, 2021 at 05:05:48PM -0800, Darrick J. Wong wrote:
> > On Wed, Jan 13, 2021 at 01:17:49PM -0500, Brian Foster wrote:
> > > On Fri, Jan 08, 2021 at 10:28:16PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > Clear the needsrepair flag, since it's used to prevent mounting of an
> > > > inconsistent filesystem.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > >  repair/agheader.c |   11 +++++++++++
> > > >  1 file changed, 11 insertions(+)
> > > > 
> > > > 
> > > > diff --git a/repair/agheader.c b/repair/agheader.c
> > > > index 8bb99489..f6174dbf 100644
> > > > --- a/repair/agheader.c
> > > > +++ b/repair/agheader.c
> > > > @@ -452,6 +452,17 @@ secondary_sb_whack(
> > > >  			rval |= XR_AG_SB_SEC;
> > > >  	}
> > > >  
> > > > +	if (xfs_sb_version_needsrepair(sb)) {
> > > > +		if (!no_modify)
> > > > +			sb->sb_features_incompat &=
> > > > +					~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> > > > +		if (!do_bzero) {
> > > > +			rval |= XR_AG_SB;
> > > > +			do_warn(_("needsrepair flag set in sb %d\n"), i);
> > > > +		} else
> > > > +			rval |= XR_AG_SB_SEC;
> > > > +	}
> > > > +
> > > 
> > > Looks reasonable modulo the questions on the previous patch. When I give
> > > this a test, one thing that stands out is that the needsrepair state
> > > itself sort of presents as corruption. I.e.,
> > > 
> > > # ./db/xfs_db -x -c "version needsrepair" <dev>
> > > Upgrading V5 filesystem
> > > Upgraded V5 filesystem.  Please run xfs_repair.
> > > versionnum [0xb4a5+0x18a] =
> > > V5,NLINK,DIRV2,ALIGN,LOGV2,EXTFLG,MOREBITS,ATTR2,LAZYSBCOUNT,PROJID32BIT,CRC,FTYPE,FINOBT,SPARSE_INODES,REFLINK,NEEDSREPAIR
> > > # ./repair/xfs_repair <dev>
> > > Phase 1 - find and verify superblock...
> > > Phase 2 - using internal log
> > >         - zero log...
> > >         - scan filesystem freespace and inode maps...
> > > needsrepair flag set in sb 1
> > > reset bad sb for ag 1
> > > needsrepair flag set in sb 2
> > > reset bad sb for ag 2
> > > needsrepair flag set in sb 0
> > > reset bad sb for ag 0
> > > needsrepair flag set in sb 3
> > > reset bad sb for ag 3
> > >         - found root inode chunk
> > > Phase 3 - for each AG...
> > > ...
> > > 
> > > So nothing was ever done to this fs besides set and clear the bit. Not a
> > > huge deal, but I wonder if we should print something more user friendly
> > > to indicate that repair found and cleared the needsrepair state, or at
> > > least just avoid the "reset bad sb ..." message for the needsrepair
> > > case.
> > 
> > Hm.  For the backup supers I guess there's really not much point in
> > saying anything about the bit being set, because the only time they get
> > used is when repair tries to use one to fix a filesystem.
> > 
> > As for AG 0, I guess I could change that to say:
> > 
> > 	dbprintf(_("Thank you for running xfs_repair!"));
> > 
> > :D Or maybe there's no need to say anything at all.
> > 
> 
> Heh. FWIW, the "needsrepair flag set ..." messages didn't seem as
> alarming as the "reset bad sb ..." ones, but that's just me. I'd be fine
> with making this entirely silent too, or at least starting that way
> until some user complains with a good enough reason for a new message..
> ;)

I changed it so repair says "clearing needsrepair flag and regenerating
metadata" and omits the "reset bad sb" warnings.

--D

> Brian
> 
> > --D
> > 
> > > Brian
> > > 
> > > >  	return(rval);
> > > >  }
> > > >  
> > > > 
> > > 
> > 
>