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@xxxxxx> 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..
