Re: [PATCH 2/5] xfs: roll the scrub transaction after completing a repair

Christoph Hellwig
2023-11-25T06:05:38Z

On Fri, Nov 24, 2023 at 03:50:17PM -0800, Darrick J. Wong wrote:
> Going forward, repair functions should commit the transaction if they're
> going to return success. Usually the space reaping functions that run
> after a successful atomic commit of the new metadata will take care of
> that for us.

Generally looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

A random comment on a pre-existing function from reading the code, and
a nitpick on the patch itself below:

> +++ b/fs/xfs/scrub/agheader_repair.c
> @@ -73,7 +73,7 @@ xrep_superblock(
> /* Write this to disk. */
> xfs_trans_buf_set_type(sc->tp, bp, XFS_BLFT_SB_BUF);
> xfs_trans_log_buf(sc->tp, bp, 0, BBTOB(bp->b_length) - 1);
> - return error;
> + return 0;

After looking through the code this is obviously fine, error must
be 0 here because the last patch touching it is xchk_should_terminate,
which only sets the error if it returns true.

But the calling conventions for xchk_should_terminate really make me
scratch my head as they are so hard to reason about. I did quick
look over must caller and most of them get there with error always
set to 0. So just making xchk_should_terminate return the error
would seem a lot better to me - any caller with a previous error
would need a second error2, but that seems better than what we have
there right now.

> /* Repair the AGF. v5 filesystems only. */
> @@ -789,6 +789,9 @@ xrep_agfl(
> /* Dump any AGFL overflow. */
> error = xrep_reap_agblocks(sc, &agfl_extents, &XFS_RMAP_OINFO_AG,
> XFS_AG_RESV_AGFL);
> + if (error)
> + goto err;
> +
> err:

This seems rather pointless and doesn't change anything..