Re: [RFC PATCH 2/4] xfs: check ag is empty

[Gao Xiang <hsiangkao@redhat.com>]

Hi Dave,

On Thu, Apr 15, 2021 at 01:52:52PM +1000, Dave Chinner wrote:
> On Thu, Apr 15, 2021 at 03:52:38AM +0800, Gao Xiang wrote:
> > After a perag is stableized as inactive, we could check if such ag
> > is empty. In order to achieve that, AGFL is also needed to be
> > emptified in advance to make sure that only one freespace extent
> > will exist here.
> > 
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_alloc.c | 97 +++++++++++++++++++++++++++++++++++++++
> >  fs/xfs/libxfs/xfs_alloc.h |  4 ++
> >  2 files changed, 101 insertions(+)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index 01d4e4d4c1d6..60a8c134c00e 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -2474,6 +2474,103 @@ xfs_defer_agfl_block(
> >  	xfs_defer_add(tp, XFS_DEFER_OPS_TYPE_AGFL_FREE, &new->xefi_list);
> >  }
> >  
> > +int
> > +xfs_ag_emptify_agfl(
> > +	struct xfs_buf		*agfbp)
> > +{
> > +	struct xfs_mount	*mp = agfbp->b_mount;
> > +	struct xfs_perag	*pag = agfbp->b_pag;
> > +	struct xfs_trans	*tp;
> > +	int			error;
> > +	struct xfs_owner_info	oinfo = XFS_RMAP_OINFO_AG;
> > +
> > +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata, 0, 0,
> > +				XFS_TRANS_RESERVE, &tp);
> > +	if (error)
> > +		return error;
> > +
> > +	/* attach to the transaction and keep it from unlocked */
> > +	xfs_trans_bjoin(tp, agfbp);
> > +	xfs_trans_bhold(tp, agfbp);
> > +
> > +	while (pag->pagf_flcount) {
> > +		xfs_agblock_t	bno;
> > +		int		error;
> > +
> > +		error = xfs_alloc_get_freelist(tp, agfbp, &bno, 0);
> > +		if (error)
> > +			break;
> > +
> > +		ASSERT(bno != NULLAGBLOCK);
> > +		xfs_defer_agfl_block(tp, pag->pag_agno, bno, &oinfo);
> > +	}
> > +	xfs_trans_set_sync(tp);
> > +	xfs_trans_commit(tp);
> > +	return error;
> > +}
> 
> Why do we need to empty the agfl to determine the AG is empty?
> 
> > +int
> > +xfs_ag_is_empty(
> > +	struct xfs_buf		*agfbp)
> > +{
> > +	struct xfs_mount	*mp = agfbp->b_mount;
> > +	struct xfs_perag	*pag = agfbp->b_pag;
> > +	struct xfs_agf		*agf = agfbp->b_addr;
> > +	struct xfs_btree_cur	*cnt_cur;
> > +	xfs_agblock_t		nfbno;
> > +	xfs_extlen_t		nflen;
> > +	int			error, i;
> > +
> > +	if (!pag->pag_inactive)
> > +		return -EINVAL;
> > +
> > +	if (pag->pagf_freeblks + pag->pagf_flcount !=
> > +	    be32_to_cpu(agf->agf_length) - mp->m_ag_prealloc_blocks)
> > +		return -ENOTEMPTY;
> 
> This is the empty check right here, yes?
> 
> Hmmm - this has to fail if the log is in this AG, right? Because we
> can't move the log (yet), so we should explicitly be checking that
> the log is in this AG before check the amount of free space...

Ok.

> 
> > +
> > +	if (pag->pagf_flcount) {
> > +		error = xfs_ag_emptify_agfl(agfbp);
> > +		if (error)
> > +			return error;
> > +
> > +		if (pag->pagf_freeblks !=
> > +		    be32_to_cpu(agf->agf_length) - mp->m_ag_prealloc_blocks)
> > +			return -ENOTEMPTY;
> > +	}
> > +
> > +	if (pag->pagi_count > 0 || pag->pagi_freecount > 0)
> > +		return -ENOTEMPTY;
> > +
> > +	if (be32_to_cpu(agf->agf_levels[XFS_BTNUM_BNO]) > 1 ||
> > +	    be32_to_cpu(agf->agf_levels[XFS_BTNUM_CNT]) > 1)
> > +		return -ENOTEMPTY;
> > +
> > +	cnt_cur = xfs_allocbt_init_cursor(mp, NULL, agfbp,
> > +					  pag->pag_agno, XFS_BTNUM_CNT);
> > +	ASSERT(cnt_cur->bc_nlevels == 1);
> > +	error = xfs_alloc_lookup_ge(cnt_cur, 0,
> > +				    be32_to_cpu(agf->agf_longest), &i);
> > +	if (error || !i)
> > +		goto out;
> > +
> > +	error = xfs_alloc_get_rec(cnt_cur, &nfbno, &nflen, &i);
> > +	if (error)
> > +		goto out;
> > +
> > +	if (XFS_IS_CORRUPT(mp, i != 1)) {
> > +		error = -EFSCORRUPTED;
> > +		goto out;
> > +	}
> > +
> > +	error = -ENOTEMPTY;
> > +	if (nfbno == mp->m_ag_prealloc_blocks &&
> > +	    nflen == pag->pagf_freeblks)
> > +		error = 0;
> 
> Ah, that's why you are trying to empty the AGFL.
> 
> This won't work because the AG btree roots can be anywhere in the AG
> once the tree has grown beyond a single block. Hence when the AG is
> fully empty, the btree root blocks can still break up free space
> into multiple extents that are each less than a maximally sized
> single extent. e.g. from my workstation:
> 
> $ xfs_db -r -c "agf 0" -c "p" /dev/mapper/base-main 
> magicnum = 0x58414746
> versionnum = 1
> seqno = 0
> length = 13732864
> bnoroot = 29039
> cntroot = 922363
> rmaproot = 8461704
> refcntroot = 6
> bnolevel = 2
> cntlevel = 2
> rmaplevel = 3
> ....
> 
> none of the root blocks are inside the m_ag_prealloc_blocks region
> of the AG. m_ag_prealloc_blocks is just for space accounting and
> does not imply physical location of the btree root blocks...
> 
> Hence I think the only checks that need to be done here are whether
> the number of free blocks equals the maximal number of blocks
> available in the given AG.

Yeah, I forgot about it, thanks for reminder. Yet I vaguely remembered
you mentioned to check the freespace btree integrity before shrinking
months ago. If that is completely needed, I tend to kill such check
and leave the following check only,

	if (pag->pagf_freeblks + pag->pagf_flcount !=
	    be32_to_cpu(agf->agf_length) - mp->m_ag_prealloc_blocks)
		return -ENOTEMPTY;

Thanks,
Gao Xiang

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>