Re: [PATCH 5/6] xfs: snapshot old AGFL before rewriting it

[Carlos Maiolino <cem@kernel.org>]

On Thu, Sep 03, 2026 at 11:27:04PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> LOLLM complains that we can't undo an attempt at fixing the AGFL if
> anything goes wrong during the rewrite, so take a snapshot of the whole
> buffer so that we can restore it.  Move the xrep_agfl_update_agf call so
> that we only update the AGF if the AGFL update is 100% successful.
> 
> Cc: <stable@vger.kernel.org> # v4.19
> Fixes: 0e93d3f43ec7d3 ("xfs: repair the AGFL")
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> Assisted-by: LOLLM # finding obvious bugs
> ---
>  fs/xfs/scrub/agheader_repair.c |   37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> 
> 
> diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
> index 2104512f1ee19a..46c95354ca6438 100644
> --- a/fs/xfs/scrub/agheader_repair.c
> +++ b/fs/xfs/scrub/agheader_repair.c
> @@ -668,14 +668,16 @@ xrep_agfl_init_header(
>  	struct xfs_scrub	*sc,
>  	struct xfs_buf		*agfl_bp,
>  	struct xagb_bitmap	*agfl_extents,
> -	xfs_agblock_t		flcount)
> +	xfs_agblock_t		flcount,
> +	struct xfs_agfl		*old_agfl)
>  {
>  	struct xrep_agfl_fill	af = {
>  		.sc		= sc,
>  		.flcount	= flcount,
>  	};
>  	struct xfs_mount	*mp = sc->mp;
> -	struct xfs_agfl		*agfl;
> +	struct xfs_agfl		*agfl = XFS_BUF_TO_AGFL(agfl_bp);
> +	const size_t		agfl_sz = BBTOB(agfl_bp->b_length);
>  	int			error;
>  
>  	ASSERT(flcount <= xfs_agfl_size(mp));
> @@ -684,8 +686,8 @@ xrep_agfl_init_header(
>  	 * Start rewriting the header by setting the bno[] array to
>  	 * NULLAGBLOCK, then setting AGFL header fields.
>  	 */
> -	agfl = XFS_BUF_TO_AGFL(agfl_bp);
> -	memset(agfl, 0xFF, BBTOB(agfl_bp->b_length));
> +	memcpy(old_agfl, agfl, agfl_sz);
> +	memset(agfl, 0xFF, agfl_sz);
>  	agfl->agfl_magicnum = cpu_to_be32(XFS_AGFL_MAGIC);
>  	agfl->agfl_seqno = cpu_to_be32(pag_agno(sc->sa.pag));
>  	uuid_copy(&agfl->agfl_uuid, &mp->m_sb.sb_meta_uuid);
> @@ -700,13 +702,18 @@ xrep_agfl_init_header(
>  	xagb_bitmap_walk(agfl_extents, xrep_agfl_fill, &af);
>  	error = xagb_bitmap_disunion(agfl_extents, &af.used_extents);
>  	if (error)
> -		return error;
> +		goto err_undo;
>  
>  	/* Write new AGFL to disk. */
>  	xfs_trans_buf_set_type(sc->tp, agfl_bp, XFS_BLFT_AGFL_BUF);
> -	xfs_trans_log_buf(sc->tp, agfl_bp, 0, BBTOB(agfl_bp->b_length) - 1);
> +	xfs_trans_log_buf(sc->tp, agfl_bp, 0, agfl_sz - 1);
>  	xagb_bitmap_destroy(&af.used_extents);
>  	return 0;
> +
> +err_undo:
> +	xagb_bitmap_destroy(&af.used_extents);
> +	memcpy(agfl, old_agfl, agfl_sz);
> +	return error;
>  }
>  
>  /* Repair the AGFL. */
> @@ -718,6 +725,7 @@ xrep_agfl(
>  	struct xfs_mount	*mp = sc->mp;
>  	struct xfs_buf		*agf_bp;
>  	struct xfs_buf		*agfl_bp;
> +	struct xfs_agfl		*old_agfl;
>  	xfs_agblock_t		flcount;
>  	int			error;
>  
> @@ -725,6 +733,10 @@ xrep_agfl(
>  	if (!xfs_has_rmapbt(mp))
>  		return -EOPNOTSUPP;
>  
> +	old_agfl = kzalloc(BBTOB(XFS_FSS_TO_BB(mp, 1)), XCHK_GFP_FLAGS);
> +	if (!old_agfl)
> +		return -ENOMEM;
> +
>  	xagb_bitmap_init(&agfl_extents);
>  
>  	/*
> @@ -734,7 +746,7 @@ xrep_agfl(
>  	 */
>  	error = xfs_alloc_read_agf(sc->sa.pag, sc->tp, 0, &agf_bp);
>  	if (error)
> -		return error;
> +		goto err_old_agfl;
>  
>  	/*
>  	 * Make sure we have the AGFL buffer, as scrub might have decided it
> @@ -745,7 +757,7 @@ xrep_agfl(
>  						XFS_AGFL_DADDR(mp)),
>  			XFS_FSS_TO_BB(mp, 1), 0, &agfl_bp, NULL);
>  	if (error)
> -		return error;
> +		goto err_old_agfl;
>  	agfl_bp->b_ops = &xfs_agfl_buf_ops;
>  
>  	/* Gather all the extents we're going to put on the new AGFL. */
> @@ -762,10 +774,11 @@ xrep_agfl(
>  	 * we adjust the AGF flcount (which can fail) so avoid updating any
>  	 * buffers until we know that part works.
>  	 */
> +	error = xrep_agfl_init_header(sc, agfl_bp, &agfl_extents, flcount,
> +			old_agfl);
> +	if (error)
> +		goto err;
>  	xrep_agfl_update_agf(sc, agf_bp, flcount);
> -	error = xrep_agfl_init_header(sc, agfl_bp, &agfl_extents, flcount);
> -	if (error)
> -		goto err;
>  
>  	/*
>  	 * Ok, the AGFL should be ready to go now.  Roll the transaction to
> @@ -785,6 +798,8 @@ xrep_agfl(
>  
>  err:
>  	xagb_bitmap_destroy(&agfl_extents);
> +err_old_agfl:
> +	kfree(old_agfl);
>  	return error;
>  }
>  
> 
>