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

[Brian Foster <bfoster@redhat.com>]

On Tue, Jan 19, 2021 at 10:15:49AM -0800, Darrick J. Wong wrote:
> On Tue, Jan 19, 2021 at 09:37:54AM -0500, Brian Foster wrote:
> > On Fri, Jan 15, 2021 at 05:24:53PM -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>
> > > ---
> > 
> > Code/errors look much cleaner. Though looking at the repair code again,
> > I wonder... if we clear the needsrepair bit and dirty/write the sb in
> > phase 2 and then xfs_repair happens to crash, do we risk clearing the
> > bit and thus allowing a potential mount before whatever requisite
> > metadata updates have been made?
> 
> [Oh good, now mail.kernel.org is having problems...]
> 
> Yes, though I think that falls into the realm of "sysadmins should be
> sufficiently self-aware not to expect mount to work after repair
> fails/system crashes during an upgrade".
> 
> I've thought about how to solve the general problem of preventing people
> from mounting filesystems if repair doesn't run to completion.  I think
> xfs_repair could be modified so that once it finds the primary super, it
> writes it back out with NEEDSREPAIR set (V5) or inprogress set (V4).
> Once we've finished the buffer cache flush at the end of repair, we
> clear needsrepair/inprogress and write the primary super again.
> 

That's kind of what I was thinking.. set a global flag if/when we come
across the bit set on disk and clear it as a final step.

> An optimization on that would be to find a way to avoid that first super
> write until we flush the first dirty buffer.
> 
> Another way to make repair more "transactional" would be to do it would
> be to fiddle with the buffer manager so that writes are sent to a
> metadump file which could be mdrestore'd if repair completes
> successfully.  But that's a short-circuit around the even bigger project
> of porting the kernel logging code to userspace and use that in repair.
> 

Yeah, I wouldn't want to have clearing a feature bit depend on such a
significant rework of core functionality. The bit is not going to be set
in most cases, so I'd suspect an extra superblock write at the end of
repair wouldn't be much of a problem. In fact, it looks like main()
already has an unconditional sb write before the final libxfs_unmount()
call. Perhaps we could just hitch onto that for the primary super
update (and continue to clear the secondaries earlier as the current
patch does)..?

Brian

> --D
> 
> > Brian
> > 
> > >  repair/agheader.c |   15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > 
> > > diff --git a/repair/agheader.c b/repair/agheader.c
> > > index 8bb99489..d9b72d3a 100644
> > > --- a/repair/agheader.c
> > > +++ b/repair/agheader.c
> > > @@ -452,6 +452,21 @@ 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 (i == 0) {
> > > +			if (!no_modify)
> > > +				do_warn(
> > > +	_("clearing needsrepair flag and regenerating metadata\n"));
> > > +			else
> > > +				do_warn(
> > > +	_("would clear needsrepair flag and regenerate metadata\n"));
> > > +		}
> > > +		rval |= XR_AG_SB_SEC;
> > > +	}
> > > +
> > >  	return(rval);
> > >  }
> > >  
> > > 
> > 
>