On Tue, Jan 19, 2021 at 08:31:28PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> Clear the needsrepair flag, since it's used to prevent mounting of an
> inconsistent filesystem.
>
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
> 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"),
>
