Re: [PATCH 6/6] xfs: update the pag for the last AG at recovery time

[Brian Foster <bfoster@redhat.com>]

On Mon, Oct 14, 2024 at 08:04:55AM +0200, Christoph Hellwig wrote:
> Currently log recovery never updates the in-core perag values for the
> last allocation group when they were grown by growfs.  This leads to
> btree record validation failures for the alloc, ialloc or finotbt
> trees if a transaction references this new space.
> 
> Found by Brian's new growfs recovery stress test.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Thanks for tracking this down. The test now passes here as well. I'll
try to get it polished up and posted soon.

>  fs/xfs/libxfs/xfs_ag.c        | 17 +++++++++++++++++
>  fs/xfs/libxfs/xfs_ag.h        |  1 +
>  fs/xfs/xfs_buf_item_recover.c | 19 ++++++++++++++++---
>  3 files changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> index 25cec9dc10c941..5ca8d01068273d 100644
> --- a/fs/xfs/libxfs/xfs_ag.c
> +++ b/fs/xfs/libxfs/xfs_ag.c
> @@ -273,6 +273,23 @@ xfs_agino_range(
>  	return __xfs_agino_range(mp, xfs_ag_block_count(mp, agno), first, last);
>  }
>  

Comment please. I.e.,

/*
 * Update the perag of the previous tail AG if it has been changed
 * during recovery (i.e. recovery of a growfs).
 */

> +int
> +xfs_update_last_ag_size(
> +	struct xfs_mount	*mp,
> +	xfs_agnumber_t		prev_agcount)
> +{
> +	struct xfs_perag	*pag = xfs_perag_grab(mp, prev_agcount - 1);
> +
> +	if (!pag)
> +		return -EFSCORRUPTED;
> +	pag->block_count = __xfs_ag_block_count(mp, prev_agcount - 1,
> +			mp->m_sb.sb_agcount, mp->m_sb.sb_dblocks);
> +	__xfs_agino_range(mp, pag->block_count, &pag->agino_min,
> +			&pag->agino_max);
> +	xfs_perag_rele(pag);
> +	return 0;
> +}
> +
>  int
>  xfs_initialize_perag(
>  	struct xfs_mount	*mp,
> diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
> index 6e68d6a3161a0f..9edfe0e9643964 100644
> --- a/fs/xfs/libxfs/xfs_ag.h
> +++ b/fs/xfs/libxfs/xfs_ag.h
> @@ -150,6 +150,7 @@ int xfs_initialize_perag(struct xfs_mount *mp, xfs_agnumber_t old_agcount,
>  void xfs_free_perag_range(struct xfs_mount *mp, xfs_agnumber_t first_agno,
>  		xfs_agnumber_t end_agno);
>  int xfs_initialize_perag_data(struct xfs_mount *mp, xfs_agnumber_t agno);
> +int xfs_update_last_ag_size(struct xfs_mount *mp, xfs_agnumber_t prev_agcount);
>  
>  /* Passive AG references */
>  struct xfs_perag *xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno);
> diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
> index a839ff5dcaa908..5180cbf5a90b4b 100644
> --- a/fs/xfs/xfs_buf_item_recover.c
> +++ b/fs/xfs/xfs_buf_item_recover.c
> @@ -708,6 +708,11 @@ xlog_recover_do_primary_sb_buffer(
>  
>  	xlog_recover_do_reg_buffer(mp, item, bp, buf_f, current_lsn);
>  
> +	if (orig_agcount == 0) {
> +		xfs_alert(mp, "Trying to grow file system without AGs");
> +		return -EFSCORRUPTED;
> +	}
> +
>  	/*
>  	 * Update the in-core super block from the freshly recovered on-disk one.
>  	 */
> @@ -718,15 +723,23 @@ xlog_recover_do_primary_sb_buffer(
>  		return -EFSCORRUPTED;
>  	}
>  
> +	/*
> +	 * Growfs can also grow the last existing AG.  In this case we also need

It can shrink the last AG as well, FWIW.

> +	 * to update the length in the in-core perag structure and values
> +	 * depending on it.
> +	 */
> +	error = xfs_update_last_ag_size(mp, orig_agcount);
> +	if (error)
> +		return error;
> +
>  	/*
>  	 * Initialize the new perags, and also update various block and inode
>  	 * allocator setting based off the number of AGs or total blocks.
>  	 * Because of the latter this also needs to happen if the agcount did
>  	 * not change.
>  	 */
> -	error = xfs_initialize_perag(mp, orig_agcount,
> -			mp->m_sb.sb_agcount, mp->m_sb.sb_dblocks,
> -			&mp->m_maxagi);
> +	error = xfs_initialize_perag(mp, orig_agcount, mp->m_sb.sb_agcount,
> +			mp->m_sb.sb_dblocks, &mp->m_maxagi);

Seems like this should be folded into an earlier patch?

With the nits addressed:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  	if (error) {
>  		xfs_warn(mp, "Failed recovery per-ag init: %d", error);
>  		return error;
> -- 
> 2.45.2
> 
>