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

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

On Wed, Jan 20, 2021 at 12:38:20PM -0500, Brian Foster wrote:
> On Tue, Jan 19, 2021 at 08:31:28PM -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>
> > ---
> > v2.1: only remove needsrepair at the end of the xfs_repair run
> > ---
> >  include/xfs_mount.h |    1 +
> >  libxfs/init.c       |    2 +-
> >  repair/agheader.c   |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  repair/agheader.h   |    2 ++
> >  repair/xfs_repair.c |    4 +++-
> >  5 files changed, 62 insertions(+), 2 deletions(-)
> > 
> ...
> > diff --git a/repair/agheader.c b/repair/agheader.c
> > index 8bb99489..56a7f45c 100644
> > --- a/repair/agheader.c
> > +++ b/repair/agheader.c
> > @@ -220,6 +220,40 @@ compare_sb(xfs_mount_t *mp, xfs_sb_t *sb)
> >  	return(XR_OK);
> >  }
> >  
> > +/* Clear needsrepair after a successful repair run. */
> > +int
> > +clear_needsrepair(
> > +	struct xfs_mount	*mp)
> > +{
> > +	struct xfs_buf		*bp;
> > +	int			error;
> > +
> > +	if (!xfs_sb_version_needsrepair(&mp->m_sb) || no_modify)
> > +		return 0;
> > +
> > +	/* We must succeed at flushing all dirty buffers to disk. */
> > +	error = -libxfs_flush_mount(mp);
> > +	if (error)
> > +		return error;
> > +
> 
> Do we really need a new helper and buf get/relse cycle just to
> incorporate the above flush? ISTM we could either lift the

Yes.  If quotacheck thinks the dquots are bad, we always want to try to
clear the CHKD flags in the primary superblock, even if other disk
writes fail due to IO errors or write verifiers failing, etc.  Note that
libxfs_bcache_flush only pwrite()s the dirty buffers to disk, it doesn't
force the disks to persist to stable media.

However, if NEEDSREPAIR was set and /any/ part of persisting the dirty
buffers fails (write verifier trips, media errors, etc.) then we don't
even want to try to clear NEEDSREPAIR.

Because of that requirement, the two writes have to be separate steps,
separated by a big flush.

--D

> libxfs_bcache_flush() call above the superblock update in the caller,
> insert the libxfs_flush_mount() right after that, and just do:
> 
> 	dsb->sb_features_incompat &= ~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> 
> ... in the hunk that also updates the quota flags.
> 
> Though perhaps cleaner would be to keep the helper, but genericize it a
> bit to something like final_sb_update() and fold in the qflags update
> and whatever flush/ordering is required for the feature bit.
> 
> > +	/* Clear needsrepair from the superblock. */
> > +	bp = libxfs_getsb(mp);
> > +	if (!bp)
> > +		return ENOMEM;
> > +	if (bp->b_error) {
> > +		error = bp->b_error;
> > +		libxfs_buf_relse(bp);
> > +		return -error;
> > +	}
> > +
> > +	mp->m_sb.sb_features_incompat &= ~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> > +
> > +	libxfs_sb_to_disk(bp->b_addr, &mp->m_sb);
> > +	libxfs_buf_mark_dirty(bp);
> > +	libxfs_buf_relse(bp);
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Possible fields that may have been set at mkfs time,
> >   * sb_inoalignmt, sb_unit, sb_width and sb_dirblklog.
> > @@ -452,6 +486,27 @@ 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;
> 
> I suspect this should be folded into the check below so we don't modify
> the primary sb by accident (should some other check dirty it).

Yup.  Fixed.

--D

> 
> Brian
> 
> > +		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"));
> > +		} else {
> > +			/*
> > +			 * Quietly clear needsrepair on the secondary supers as
> > +			 * part of ensuring them.  If needsrepair is set on the
> > +			 * primary, it will be done at the very end of repair.
> > +			 */
> > +			rval |= XR_AG_SB_SEC;
> > +		}
> > +	}
> > +
> >  	return(rval);
> >  }
> >  
> > diff --git a/repair/agheader.h b/repair/agheader.h
> > index a63827c8..552c1f70 100644
> > --- a/repair/agheader.h
> > +++ b/repair/agheader.h
> > @@ -82,3 +82,5 @@ typedef struct fs_geo_list  {
> >  #define XR_AG_AGF	0x2
> >  #define XR_AG_AGI	0x4
> >  #define XR_AG_SB_SEC	0x8
> > +
> > +int clear_needsrepair(struct xfs_mount *mp);
> > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > index 9409f0d8..d36c5a21 100644
> > --- a/repair/xfs_repair.c
> > +++ b/repair/xfs_repair.c
> > @@ -1133,7 +1133,9 @@ _("Note - stripe unit (%d) and width (%d) were copied from a backup superblock.\
> >  	format_log_max_lsn(mp);
> >  
> >  	/* Report failure if anything failed to get written to our fs. */
> > -	error = -libxfs_umount(mp);
> > +	error = clear_needsrepair(mp);
> > +	if (!error)
> > +		error = -libxfs_umount(mp);
> >  	if (error)
> >  		do_error(
> >  	_("File system metadata writeout failed, err=%d.  Re-run xfs_repair.\n"),
> > 
>