Re: [PATCH RFC] xfs: change agfl perag res into an rmapbt-based reservation

[Bill O'Donnell <billodo@redhat.com>]

On Wed, Jan 31, 2018 at 09:59:01AM -0500, Brian Foster wrote:
> The agfl perag metadata reservation reserves blocks for the reverse
> mapping btree (rmapbt). Since the rmapbt uses blocks from the agfl
> and perag accounting is updated as blocks are allocated from the
> allocation btrees, the reservation actually accounts blocks as they
> are allocated to (or freed from) the agfl rather than the rmapbt
> itself.
> 
> While this works for blocks that are eventually used for the rmapbt,
> not all agfl blocks are destined for the rmapbt. Blocks that are
> allocated to the agfl (and thus "reserved" for the rmapbt) but then
> used by another structure leads to a growing inconsistency over time
> between the runtime tracking of rmapbt usage vs. actual rmapbt
> usage. Since the runtime tracking thinks all agfl blocks are rmapbt
> blocks, it essentially believes that less future reservation is
> required to satisfy the rmapbt than what is actually necessary.
> 
> The inconsistency can be rectified across mount cycles because the
> perag reservation is initialized based on the actual rmapbt usage at
> mount time. The problem, however, is that the excessive drain of the
> reservation at runtime opens a window to allocate blocks for other
> purposes that might be expected to be reserved for the rmapbt on a
> subsequent mount. This problem can be demonstrated by a simple test
> that runs an allocation workload to consume agfl blocks over time
> and then observe the difference in the agfl reservation requirement
> across an unmount/mount cycle:
> 
>   mount ...: xfs_ag_resv_init: ... resv 3193 ask 3194 len 3194
>   ...
>   ...      : xfs_ag_resv_alloc_extent: ... resv 2957 ask 3194 len 1
>   umount...: xfs_ag_resv_free: ... resv 2956 ask 3194 len 0
>   mount ...: xfs_ag_resv_init: ... resv 3052 ask 3194 len 3194
> 
> As the above tracepoints show, the agfl reservation requirement
> reduces from 3194 blocks to 2956 blocks as the workload runs.
> Without any other changes in the filesystem, the same reservation
> requirement jumps to 3052 blocks over a umount/mount cycle.
> 
> To address this inconsistency, update the AGFL reservation to
> account blocks used for the rmapbt only rather than all blocks
> filled into the agfl. This patch makes several high-level changes
> toward that end:
> 
> 1.) Rename ->pag_agfl_resv to ->pag_rmapbt_resv and ssociate a new
>     XFS_AG_RESV_RMAPBT type with the perag reservation.
> 2.) Invoke XFS_AG_RESV_RMAPBT accounting from actual rmapbt block
>     allocations.
> 3.) Repurpose XFS_AG_RESV_AGFL to act as a no-op for the perag res.
>     accounting code to correctly handle agfl allocations.

Since there are 3 high-level changes, perhaps it would make sense to
split this into 3 separate patches? If that's feasible, it would
definitely help me to grok it. ;)

> 
> This last change is required because agfl blocks are tracked as
> "free" blocks throughout their lifetime. The agfl fixup code
> therefore needs a way to tell the btree-based allocator to not make
> free space accounting changes for blocks that are being allocated to
> fill into the agfl.
> 
> Altogether, these changes ensure that the runtime rmapbt reservation
> accounting remains consistent with actual rmapbt block usage and
> reduce the risk of leaving the rmapbt reservation underfilled.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> 
> This is RFC for the moment because I have reproduced a one-off
> sb_fdblocks inconsistency during xfstests. Unfortunately, I have not
> been able to reproduce that problem after several rmapbt/reflink enabled
> cycles so far.
> 
> It's not yet clear to me if this is a bug in this patch or not, so more
> testing is required at minimum. I'm sending this out for thoughts in the
> meantime.
> 
> Brian
> 
>  fs/xfs/libxfs/xfs_ag_resv.c    | 39 ++++++++++++++++++++++-----------------
>  fs/xfs/libxfs/xfs_ag_resv.h    | 31 +++++++++++++++++++++++++++++++
>  fs/xfs/libxfs/xfs_alloc.c      | 14 +++-----------
>  fs/xfs/libxfs/xfs_rmap_btree.c |  4 ++++
>  fs/xfs/xfs_mount.h             |  9 +++++----
>  fs/xfs/xfs_reflink.c           |  2 +-
>  6 files changed, 66 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
> index 2291f4224e24..1945202426c4 100644
> --- a/fs/xfs/libxfs/xfs_ag_resv.c
> +++ b/fs/xfs/libxfs/xfs_ag_resv.c
> @@ -95,13 +95,13 @@ xfs_ag_resv_critical(
>  
>  	switch (type) {
>  	case XFS_AG_RESV_METADATA:
> -		avail = pag->pagf_freeblks - pag->pag_agfl_resv.ar_reserved;
> +		avail = pag->pagf_freeblks - pag->pag_rmapbt_resv.ar_reserved;
>  		orig = pag->pag_meta_resv.ar_asked;
>  		break;
> -	case XFS_AG_RESV_AGFL:
> +	case XFS_AG_RESV_RMAPBT:
>  		avail = pag->pagf_freeblks + pag->pagf_flcount -
>  			pag->pag_meta_resv.ar_reserved;
> -		orig = pag->pag_agfl_resv.ar_asked;
> +		orig = pag->pag_rmapbt_resv.ar_asked;
>  		break;
>  	default:
>  		ASSERT(0);
> @@ -126,10 +126,10 @@ xfs_ag_resv_needed(
>  {
>  	xfs_extlen_t			len;
>  
> -	len = pag->pag_meta_resv.ar_reserved + pag->pag_agfl_resv.ar_reserved;
> +	len = pag->pag_meta_resv.ar_reserved + pag->pag_rmapbt_resv.ar_reserved;
>  	switch (type) {
>  	case XFS_AG_RESV_METADATA:
> -	case XFS_AG_RESV_AGFL:
> +	case XFS_AG_RESV_RMAPBT:
>  		len -= xfs_perag_resv(pag, type)->ar_reserved;
>  		break;
>  	case XFS_AG_RESV_NONE:
> @@ -160,10 +160,11 @@ __xfs_ag_resv_free(
>  	if (pag->pag_agno == 0)
>  		pag->pag_mount->m_ag_max_usable += resv->ar_asked;
>  	/*
> -	 * AGFL blocks are always considered "free", so whatever
> -	 * was reserved at mount time must be given back at umount.
> +	 * RMAPBT blocks come from the AGFL and AGFL blocks are always
> +	 * considered "free", so whatever was reserved at mount time must be
> +	 * given back at umount.
>  	 */
> -	if (type == XFS_AG_RESV_AGFL)
> +	if (type == XFS_AG_RESV_RMAPBT)
>  		oldresv = resv->ar_orig_reserved;
>  	else
>  		oldresv = resv->ar_reserved;
> @@ -185,7 +186,7 @@ xfs_ag_resv_free(
>  	int				error;
>  	int				err2;
>  
> -	error = __xfs_ag_resv_free(pag, XFS_AG_RESV_AGFL);
> +	error = __xfs_ag_resv_free(pag, XFS_AG_RESV_RMAPBT);
>  	err2 = __xfs_ag_resv_free(pag, XFS_AG_RESV_METADATA);
>  	if (err2 && !error)
>  		error = err2;
> @@ -284,15 +285,15 @@ xfs_ag_resv_init(
>  		}
>  	}
>  
> -	/* Create the AGFL metadata reservation */
> -	if (pag->pag_agfl_resv.ar_asked == 0) {
> +	/* Create the RMAPBT metadata reservation */
> +	if (pag->pag_rmapbt_resv.ar_asked == 0) {
>  		ask = used = 0;
>  
>  		error = xfs_rmapbt_calc_reserves(mp, agno, &ask, &used);
>  		if (error)
>  			goto out;
>  
> -		error = __xfs_ag_resv_init(pag, XFS_AG_RESV_AGFL, ask, used);
> +		error = __xfs_ag_resv_init(pag, XFS_AG_RESV_RMAPBT, ask, used);
>  		if (error)
>  			goto out;
>  	}
> @@ -304,7 +305,7 @@ xfs_ag_resv_init(
>  		return error;
>  
>  	ASSERT(xfs_perag_resv(pag, XFS_AG_RESV_METADATA)->ar_reserved +
> -	       xfs_perag_resv(pag, XFS_AG_RESV_AGFL)->ar_reserved <=
> +	       xfs_perag_resv(pag, XFS_AG_RESV_RMAPBT)->ar_reserved <=
>  	       pag->pagf_freeblks + pag->pagf_flcount);
>  #endif
>  out:
> @@ -325,8 +326,10 @@ xfs_ag_resv_alloc_extent(
>  	trace_xfs_ag_resv_alloc_extent(pag, type, args->len);
>  
>  	switch (type) {
> -	case XFS_AG_RESV_METADATA:
>  	case XFS_AG_RESV_AGFL:
> +		return;
> +	case XFS_AG_RESV_RMAPBT:
> +	case XFS_AG_RESV_METADATA:
>  		resv = xfs_perag_resv(pag, type);
>  		break;
>  	default:
> @@ -341,7 +344,7 @@ xfs_ag_resv_alloc_extent(
>  
>  	len = min_t(xfs_extlen_t, args->len, resv->ar_reserved);
>  	resv->ar_reserved -= len;
> -	if (type == XFS_AG_RESV_AGFL)
> +	if (type == XFS_AG_RESV_RMAPBT)
>  		return;
>  	/* Allocations of reserved blocks only need on-disk sb updates... */
>  	xfs_trans_mod_sb(args->tp, XFS_TRANS_SB_RES_FDBLOCKS, -(int64_t)len);
> @@ -365,8 +368,10 @@ xfs_ag_resv_free_extent(
>  	trace_xfs_ag_resv_free_extent(pag, type, len);
>  
>  	switch (type) {
> -	case XFS_AG_RESV_METADATA:
>  	case XFS_AG_RESV_AGFL:
> +		return;
> +	case XFS_AG_RESV_RMAPBT:
> +	case XFS_AG_RESV_METADATA:
>  		resv = xfs_perag_resv(pag, type);
>  		break;
>  	default:
> @@ -379,7 +384,7 @@ xfs_ag_resv_free_extent(
>  
>  	leftover = min_t(xfs_extlen_t, len, resv->ar_asked - resv->ar_reserved);
>  	resv->ar_reserved += leftover;
> -	if (type == XFS_AG_RESV_AGFL)
> +	if (type == XFS_AG_RESV_RMAPBT)
>  		return;
>  	/* Freeing into the reserved pool only requires on-disk update... */
>  	xfs_trans_mod_sb(tp, XFS_TRANS_SB_RES_FDBLOCKS, len);
> diff --git a/fs/xfs/libxfs/xfs_ag_resv.h b/fs/xfs/libxfs/xfs_ag_resv.h
> index 8d6c687deef3..938f2f96c5e8 100644
> --- a/fs/xfs/libxfs/xfs_ag_resv.h
> +++ b/fs/xfs/libxfs/xfs_ag_resv.h
> @@ -32,4 +32,35 @@ void xfs_ag_resv_alloc_extent(struct xfs_perag *pag, enum xfs_ag_resv_type type,
>  void xfs_ag_resv_free_extent(struct xfs_perag *pag, enum xfs_ag_resv_type type,
>  		struct xfs_trans *tp, xfs_extlen_t len);
>  
> +/*
> + * RMAPBT reservation accounting wrappers. Since rmapbt blocks are sourced from
> + * the AGFL, they are allocated one at a time and the reservation updates don't
> + * require a transaction.
> + */
> +static inline void
> +xfs_ag_resv_rmapbt_alloc(
> +	struct xfs_mount	*mp,
> +	xfs_agnumber_t		agno)
> +{
> +	struct xfs_alloc_arg	args = {0};
> +	struct xfs_perag	*pag;
> +
> +	args.len = 1;
> +	pag = xfs_perag_get(mp, agno);
> +	xfs_ag_resv_alloc_extent(pag, XFS_AG_RESV_RMAPBT, &args);
> +	xfs_perag_put(pag);
> +}
> +
> +static inline void
> +xfs_ag_resv_rmapbt_free(
> +	struct xfs_mount	*mp,
> +	xfs_agnumber_t		agno)
> +{
> +	struct xfs_perag	*pag;
> +
> +	pag = xfs_perag_get(mp, agno);
> +	xfs_ag_resv_free_extent(pag, XFS_AG_RESV_RMAPBT, NULL, 1);
> +	xfs_perag_put(pag);
> +}
> +
>  #endif	/* __XFS_AG_RESV_H__ */
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index c02781a4c091..8c401efb2d74 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -1564,7 +1564,6 @@ xfs_alloc_ag_vextent_small(
>  	int		*stat)	/* status: 0-freelist, 1-normal/none */
>  {
>  	struct xfs_owner_info	oinfo;
> -	struct xfs_perag	*pag;
>  	int		error;
>  	xfs_agblock_t	fbno;
>  	xfs_extlen_t	flen;
> @@ -1616,18 +1615,13 @@ xfs_alloc_ag_vextent_small(
>  			/*
>  			 * If we're feeding an AGFL block to something that
>  			 * doesn't live in the free space, we need to clear
> -			 * out the OWN_AG rmap and add the block back to
> -			 * the AGFL per-AG reservation.
> +			 * out the OWN_AG rmap.
>  			 */
>  			xfs_rmap_ag_owner(&oinfo, XFS_RMAP_OWN_AG);
>  			error = xfs_rmap_free(args->tp, args->agbp, args->agno,
>  					fbno, 1, &oinfo);
>  			if (error)
>  				goto error0;
> -			pag = xfs_perag_get(args->mp, args->agno);
> -			xfs_ag_resv_free_extent(pag, XFS_AG_RESV_AGFL,
> -					args->tp, 1);
> -			xfs_perag_put(pag);
>  
>  			*stat = 0;
>  			return 0;
> @@ -1911,14 +1905,12 @@ xfs_free_ag_extent(
>  	XFS_STATS_INC(mp, xs_freex);
>  	XFS_STATS_ADD(mp, xs_freeb, len);
>  
> -	trace_xfs_free_extent(mp, agno, bno, len, type == XFS_AG_RESV_AGFL,
> -			haveleft, haveright);
> +	trace_xfs_free_extent(mp, agno, bno, len, type, haveleft, haveright);
>  
>  	return 0;
>  
>   error0:
> -	trace_xfs_free_extent(mp, agno, bno, len, type == XFS_AG_RESV_AGFL,
> -			-1, -1);
> +	trace_xfs_free_extent(mp, agno, bno, len, type, -1, -1);
>  	if (bno_cur)
>  		xfs_btree_del_cursor(bno_cur, XFS_BTREE_ERROR);
>  	if (cnt_cur)
> diff --git a/fs/xfs/libxfs/xfs_rmap_btree.c b/fs/xfs/libxfs/xfs_rmap_btree.c
> index e829c3e489ea..8560091554e0 100644
> --- a/fs/xfs/libxfs/xfs_rmap_btree.c
> +++ b/fs/xfs/libxfs/xfs_rmap_btree.c
> @@ -130,6 +130,8 @@ xfs_rmapbt_alloc_block(
>  	be32_add_cpu(&agf->agf_rmap_blocks, 1);
>  	xfs_alloc_log_agf(cur->bc_tp, agbp, XFS_AGF_RMAP_BLOCKS);
>  
> +	xfs_ag_resv_rmapbt_alloc(cur->bc_mp, cur->bc_private.a.agno);
> +
>  	XFS_BTREE_TRACE_CURSOR(cur, XBT_EXIT);
>  	*stat = 1;
>  	return 0;
> @@ -158,6 +160,8 @@ xfs_rmapbt_free_block(
>  			      XFS_EXTENT_BUSY_SKIP_DISCARD);
>  	xfs_trans_agbtree_delta(cur->bc_tp, -1);
>  
> +	xfs_ag_resv_rmapbt_free(cur->bc_mp, cur->bc_private.a.agno);
> +
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index e0792d036be2..f659045507fb 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -327,6 +327,7 @@ xfs_daddr_to_agbno(struct xfs_mount *mp, xfs_daddr_t d)
>  enum xfs_ag_resv_type {
>  	XFS_AG_RESV_NONE = 0,
>  	XFS_AG_RESV_METADATA,
> +	XFS_AG_RESV_RMAPBT,
>  	XFS_AG_RESV_AGFL,
>  };
>  
> @@ -391,8 +392,8 @@ typedef struct xfs_perag {
>  
>  	/* Blocks reserved for all kinds of metadata. */
>  	struct xfs_ag_resv	pag_meta_resv;
> -	/* Blocks reserved for just AGFL-based metadata. */
> -	struct xfs_ag_resv	pag_agfl_resv;
> +	/* Blocks reserved for the reverse mapping btree. */
> +	struct xfs_ag_resv	pag_rmapbt_resv;
>  
>  	/* reference count */
>  	uint8_t			pagf_refcount_level;
> @@ -406,8 +407,8 @@ xfs_perag_resv(
>  	switch (type) {
>  	case XFS_AG_RESV_METADATA:
>  		return &pag->pag_meta_resv;
> -	case XFS_AG_RESV_AGFL:
> -		return &pag->pag_agfl_resv;
> +	case XFS_AG_RESV_RMAPBT:
> +		return &pag->pag_rmapbt_resv;
>  	default:
>  		return NULL;
>  	}
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 270246943a06..832df6f49ba1 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -1061,7 +1061,7 @@ xfs_reflink_ag_has_free_space(
>  		return 0;
>  
>  	pag = xfs_perag_get(mp, agno);
> -	if (xfs_ag_resv_critical(pag, XFS_AG_RESV_AGFL) ||
> +	if (xfs_ag_resv_critical(pag, XFS_AG_RESV_RMAPBT) ||
>  	    xfs_ag_resv_critical(pag, XFS_AG_RESV_METADATA))
>  		error = -ENOSPC;
>  	xfs_perag_put(pag);
> -- 
> 2.13.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html