Re: [PATCH CANDIDATE 6.1] xfs: give xfs_extfree_intent its own perag reference

On Thu, Mar 20, 2025 at 06:01:12PM -0700, Leah Rumancik wrote:
> From: "Darrick J. Wong" <[email protected]>
> 
> [ Upstream commit f6b384631e1e3482c24e35b53adbd3da50e47e8f ]
> 
> Give the xfs_extfree_intent an passive reference to the perag structure
> data.  This reference will be used to enable scrub intent draining
> functionality in subsequent patches.  The space being freed must already
> be allocated, so we need to able to run even if the AG is being offlined
> or shrunk.
> 
> Signed-off-by: Darrick J. Wong <[email protected]>
> Reviewed-by: Dave Chinner <[email protected]>
> Signed-off-by: Leah Rumancik <[email protected]>
> ---
> 
> This is to fix build fialure noted here:
> https://lore.kernel.org/stable/[email protected]/
> 
> Tested on auto group x 9 configs with no regressions seen.

LGTM
Acked-by: "Darrick J. Wong" <[email protected]>

--D

> 
> - leah
> 
>  fs/xfs/libxfs/xfs_alloc.c |  7 +++--
>  fs/xfs/libxfs/xfs_alloc.h |  4 +++
>  fs/xfs/xfs_extfree_item.c | 58 +++++++++++++++++++++++++--------------
>  3 files changed, 47 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index e44f3f5c6d27..c08265f19136 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -2509,30 +2509,31 @@ xfs_defer_agfl_block(
>  	xefi->xefi_owner = oinfo->oi_owner;
>  	xefi->xefi_agresv = XFS_AG_RESV_AGFL;
>  
>  	trace_xfs_agfl_free_defer(mp, agno, 0, agbno, 1);
>  
> +	xfs_extent_free_get_group(mp, xefi);
>  	xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_AGFL_FREE, &xefi->xefi_list);
>  	return 0;
>  }
>  
>  /*
>   * Add the extent to the list of extents to be free at transaction end.
>   * The list is maintained sorted (by block number).
>   */
>  int
>  __xfs_free_extent_later(
>  	struct xfs_trans		*tp,
>  	xfs_fsblock_t			bno,
>  	xfs_filblks_t			len,
>  	const struct xfs_owner_info	*oinfo,
>  	enum xfs_ag_resv_type		type,
>  	bool				skip_discard)
>  {
>  	struct xfs_extent_free_item	*xefi;
> -#ifdef DEBUG
>  	struct xfs_mount		*mp = tp->t_mountp;
> +#ifdef DEBUG
>  	xfs_agnumber_t			agno;
>  	xfs_agblock_t			agbno;
>  
>  	ASSERT(bno != NULLFSBLOCK);
>  	ASSERT(len > 0);
> @@ -2567,13 +2568,15 @@ __xfs_free_extent_later(
>  			xefi->xefi_flags |= XFS_EFI_BMBT_BLOCK;
>  		xefi->xefi_owner = oinfo->oi_owner;
>  	} else {
>  		xefi->xefi_owner = XFS_RMAP_OWN_NULL;
>  	}
> -	trace_xfs_bmap_free_defer(tp->t_mountp,
> +	trace_xfs_bmap_free_defer(mp,
>  			XFS_FSB_TO_AGNO(tp->t_mountp, bno), 0,
>  			XFS_FSB_TO_AGBNO(tp->t_mountp, bno), len);
> +
> +	xfs_extent_free_get_group(mp, xefi);
>  	xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_FREE, &xefi->xefi_list);
>  	return 0;
>  }
>  
>  #ifdef DEBUG
> diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
> index bbedb18651de..2dd93d62150f 100644
> --- a/fs/xfs/libxfs/xfs_alloc.h
> +++ b/fs/xfs/libxfs/xfs_alloc.h
> @@ -224,14 +224,18 @@ int __xfs_free_extent_later(struct xfs_trans *tp, xfs_fsblock_t bno,
>  struct xfs_extent_free_item {
>  	struct list_head	xefi_list;
>  	uint64_t		xefi_owner;
>  	xfs_fsblock_t		xefi_startblock;/* starting fs block number */
>  	xfs_extlen_t		xefi_blockcount;/* number of blocks in extent */
> +	struct xfs_perag	*xefi_pag;
>  	unsigned int		xefi_flags;
>  	enum xfs_ag_resv_type	xefi_agresv;
>  };
>  
> +void xfs_extent_free_get_group(struct xfs_mount *mp,
> +		struct xfs_extent_free_item *xefi);
> +
>  #define XFS_EFI_SKIP_DISCARD	(1U << 0) /* don't issue discard */
>  #define XFS_EFI_ATTR_FORK	(1U << 1) /* freeing attr fork block */
>  #define XFS_EFI_BMBT_BLOCK	(1U << 2) /* freeing bmap btree block */
>  
>  static inline int
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 9c726f082285..ab9d0e8b77da 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -348,32 +348,27 @@ xfs_trans_free_extent(
>  	struct xfs_extent_free_item	*xefi)
>  {
>  	struct xfs_owner_info		oinfo = { };
>  	struct xfs_mount		*mp = tp->t_mountp;
>  	struct xfs_extent		*extp;
> -	struct xfs_perag		*pag;
>  	uint				next_extent;
> -	xfs_agnumber_t			agno = XFS_FSB_TO_AGNO(mp,
> -							xefi->xefi_startblock);
>  	xfs_agblock_t			agbno = XFS_FSB_TO_AGBNO(mp,
>  							xefi->xefi_startblock);
>  	int				error;
>  
>  	oinfo.oi_owner = xefi->xefi_owner;
>  	if (xefi->xefi_flags & XFS_EFI_ATTR_FORK)
>  		oinfo.oi_flags |= XFS_OWNER_INFO_ATTR_FORK;
>  	if (xefi->xefi_flags & XFS_EFI_BMBT_BLOCK)
>  		oinfo.oi_flags |= XFS_OWNER_INFO_BMBT_BLOCK;
>  
> -	trace_xfs_bmap_free_deferred(tp->t_mountp, agno, 0, agbno,
> -			xefi->xefi_blockcount);
> +	trace_xfs_bmap_free_deferred(tp->t_mountp, xefi->xefi_pag->pag_agno, 0,
> +			agbno, xefi->xefi_blockcount);
>  
> -	pag = xfs_perag_get(mp, agno);
> -	error = __xfs_free_extent(tp, pag, agbno, xefi->xefi_blockcount,
> -			&oinfo, xefi->xefi_agresv,
> +	error = __xfs_free_extent(tp, xefi->xefi_pag, agbno,
> +			xefi->xefi_blockcount, &oinfo, xefi->xefi_agresv,
>  			xefi->xefi_flags & XFS_EFI_SKIP_DISCARD);
> -	xfs_perag_put(pag);
>  
>  	/*
>  	 * Mark the transaction dirty, even on error. This ensures the
>  	 * transaction is aborted, which:
>  	 *
> @@ -398,18 +393,17 @@ static int
>  xfs_extent_free_diff_items(
>  	void				*priv,
>  	const struct list_head		*a,
>  	const struct list_head		*b)
>  {
> -	struct xfs_mount		*mp = priv;
>  	struct xfs_extent_free_item	*ra;
>  	struct xfs_extent_free_item	*rb;
>  
>  	ra = container_of(a, struct xfs_extent_free_item, xefi_list);
>  	rb = container_of(b, struct xfs_extent_free_item, xefi_list);
> -	return  XFS_FSB_TO_AGNO(mp, ra->xefi_startblock) -
> -		XFS_FSB_TO_AGNO(mp, rb->xefi_startblock);
> +
> +	return ra->xefi_pag->pag_agno - rb->xefi_pag->pag_agno;
>  }
>  
>  /* Log a free extent to the intent item. */
>  STATIC void
>  xfs_extent_free_log_item(
> @@ -464,44 +458,68 @@ xfs_extent_free_create_done(
>  	unsigned int			count)
>  {
>  	return &xfs_trans_get_efd(tp, EFI_ITEM(intent), count)->efd_item;
>  }
>  
> +/* Take a passive ref to the AG containing the space we're freeing. */
> +void
> +xfs_extent_free_get_group(
> +	struct xfs_mount		*mp,
> +	struct xfs_extent_free_item	*xefi)
> +{
> +	xfs_agnumber_t			agno;
> +
> +	agno = XFS_FSB_TO_AGNO(mp, xefi->xefi_startblock);
> +	xefi->xefi_pag = xfs_perag_get(mp, agno);
> +}
> +
> +/* Release a passive AG ref after some freeing work. */
> +static inline void
> +xfs_extent_free_put_group(
> +	struct xfs_extent_free_item	*xefi)
> +{
> +	xfs_perag_put(xefi->xefi_pag);
> +}
> +
>  /* Process a free extent. */
>  STATIC int
>  xfs_extent_free_finish_item(
>  	struct xfs_trans		*tp,
>  	struct xfs_log_item		*done,
>  	struct list_head		*item,
>  	struct xfs_btree_cur		**state)
>  {
>  	struct xfs_extent_free_item	*xefi;
>  	int				error;
>  
>  	xefi = container_of(item, struct xfs_extent_free_item, xefi_list);
>  
>  	error = xfs_trans_free_extent(tp, EFD_ITEM(done), xefi);
> +
> +	xfs_extent_free_put_group(xefi);
>  	kmem_cache_free(xfs_extfree_item_cache, xefi);
>  	return error;
>  }
>  
>  /* Abort all pending EFIs. */
>  STATIC void
>  xfs_extent_free_abort_intent(
>  	struct xfs_log_item		*intent)
>  {
>  	xfs_efi_release(EFI_ITEM(intent));
>  }
>  
>  /* Cancel a free extent. */
>  STATIC void
>  xfs_extent_free_cancel_item(
>  	struct list_head		*item)
>  {
>  	struct xfs_extent_free_item	*xefi;
>  
>  	xefi = container_of(item, struct xfs_extent_free_item, xefi_list);
> +
> +	xfs_extent_free_put_group(xefi);
>  	kmem_cache_free(xfs_extfree_item_cache, xefi);
>  }
>  
>  const struct xfs_defer_op_type xfs_extent_free_defer_type = {
>  	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
> @@ -528,46 +546,44 @@ xfs_agfl_free_finish_item(
>  	struct xfs_efd_log_item		*efdp = EFD_ITEM(done);
>  	struct xfs_extent_free_item	*xefi;
>  	struct xfs_extent		*extp;
>  	struct xfs_buf			*agbp;
>  	int				error;
> -	xfs_agnumber_t			agno;
>  	xfs_agblock_t			agbno;
>  	uint				next_extent;
> -	struct xfs_perag		*pag;
>  
>  	xefi = container_of(item, struct xfs_extent_free_item, xefi_list);
>  	ASSERT(xefi->xefi_blockcount == 1);
> -	agno = XFS_FSB_TO_AGNO(mp, xefi->xefi_startblock);
>  	agbno = XFS_FSB_TO_AGBNO(mp, xefi->xefi_startblock);
>  	oinfo.oi_owner = xefi->xefi_owner;
>  
> -	trace_xfs_agfl_free_deferred(mp, agno, 0, agbno, xefi->xefi_blockcount);
> +	trace_xfs_agfl_free_deferred(mp, xefi->xefi_pag->pag_agno, 0, agbno,
> +			xefi->xefi_blockcount);
>  
> -	pag = xfs_perag_get(mp, agno);
> -	error = xfs_alloc_read_agf(pag, tp, 0, &agbp);
> +	error = xfs_alloc_read_agf(xefi->xefi_pag, tp, 0, &agbp);
>  	if (!error)
> -		error = xfs_free_agfl_block(tp, agno, agbno, agbp, &oinfo);
> -	xfs_perag_put(pag);
> +		error = xfs_free_agfl_block(tp, xefi->xefi_pag->pag_agno,
> +				agbno, agbp, &oinfo);
>  
>  	/*
>  	 * Mark the transaction dirty, even on error. This ensures the
>  	 * transaction is aborted, which:
>  	 *
>  	 * 1.) releases the EFI and frees the EFD
>  	 * 2.) shuts down the filesystem
>  	 */
>  	tp->t_flags |= XFS_TRANS_DIRTY;
>  	set_bit(XFS_LI_DIRTY, &efdp->efd_item.li_flags);
>  
>  	next_extent = efdp->efd_next_extent;
>  	ASSERT(next_extent < efdp->efd_format.efd_nextents);
>  	extp = &(efdp->efd_format.efd_extents[next_extent]);
>  	extp->ext_start = xefi->xefi_startblock;
>  	extp->ext_len = xefi->xefi_blockcount;
>  	efdp->efd_next_extent++;
>  
> +	xfs_extent_free_put_group(xefi);
>  	kmem_cache_free(xfs_extfree_item_cache, xefi);
>  	return error;
>  }
>  
>  /* sub-type with special handling for AGFL deferred frees */
> @@ -640,11 +656,13 @@ xfs_efi_item_recover(
>  		extp = &efip->efi_format.efi_extents[i];
>  
>  		fake.xefi_startblock = extp->ext_start;
>  		fake.xefi_blockcount = extp->ext_len;
>  
> +		xfs_extent_free_get_group(mp, &fake);
>  		error = xfs_trans_free_extent(tp, efdp, &fake);
> +		xfs_extent_free_put_group(&fake);
>  		if (error == -EFSCORRUPTED)
>  			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
>  					extp, sizeof(*extp));
>  		if (error)
>  			goto abort_error;
> -- 
> 2.49.0.395.g12beb8f557-goog
> 
>