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

[Brian Foster <bfoster@redhat.com>]

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
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).

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"),
>