Re: [PATCH v5 4/5] xfs: support shrinking unused space in the last AG

[Gao Xiang <hsiangkao@redhat.com>]

Hi Darrick,

On Wed, Jan 20, 2021 at 11:25:06AM -0800, Darrick J. Wong wrote:
> On Mon, Jan 18, 2021 at 04:36:59PM +0800, Gao Xiang wrote:

...

> >  
> > +int
> > +xfs_ag_shrink_space(
> > +	struct xfs_mount	*mp,
> > +	struct xfs_trans	*tp,
> > +	struct aghdr_init_data	*id,
> > +	xfs_extlen_t		len)
> > +{
> > +	struct xfs_alloc_arg	args = {
> > +		.tp	= tp,
> > +		.mp	= mp,
> > +		.type	= XFS_ALLOCTYPE_THIS_BNO,
> > +		.minlen = len,
> > +		.maxlen = len,
> > +		.oinfo	= XFS_RMAP_OINFO_SKIP_UPDATE,
> > +		.resv	= XFS_AG_RESV_NONE,
> > +		.prod	= 1
> > +	};
> > +	struct xfs_buf		*agibp, *agfbp;
> > +	struct xfs_agi		*agi;
> > +	struct xfs_agf		*agf;
> > +	int			error, err2;
> > +
> > +	ASSERT(id->agno == mp->m_sb.sb_agcount - 1);
> > +	error = xfs_ialloc_read_agi(mp, tp, id->agno, &agibp);
> > +	if (error)
> > +		return error;
> > +
> > +	agi = agibp->b_addr;
> > +
> > +	error = xfs_alloc_read_agf(mp, tp, id->agno, 0, &agfbp);
> > +	if (error)
> > +		return error;
> > +
> > +	agf = agfbp->b_addr;
> > +	if (XFS_IS_CORRUPT(mp, agf->agf_length != agi->agi_length))
> > +		return -EFSCORRUPTED;
> > +
> > +	args.fsbno = XFS_AGB_TO_FSB(mp, id->agno,
> > +				    be32_to_cpu(agi->agi_length) - len);
> > +
> > +	/* remove the preallocations before allocation and re-establish then */
> > +	error = xfs_ag_resv_free(agibp->b_pag);
> > +	if (error)
> > +		return error;
> > +
> > +	/* internal log shouldn't also show up in the free space btrees */
> > +	error = xfs_alloc_vextent(&args);
> > +	if (!error && args.agbno == NULLAGBLOCK)
> > +		error = -ENOSPC;
> > +
> > +	if (error) {
> 
> Aha, now I see why this bit:
> 
> 	if (!extend && ((tp->t_flags & XFS_TRANS_DIRTY) ||
> 			!list_empty(&tp->t_dfops)))
> 		xfs_trans_commit(tp);
> 
> is needed below -- we could have refilled the AGFL here but failed the
> allocation.  At this point we have a dirty transaction /and/ an error
> code.  We need to commit the AGFL refill changes and return to userspace
> with that error code, but calling xfs_trans_cancel on the dirty
> transaction causes an (unnecessary) shutdown.
> 
> What if you rolled the transaction here and passed the new tp and the
> error code back to the caller?  The new transaction is clean so it will
> cancel without any side effects, and then you can send the ENOSPC up to
> userspace.
> 
> Granted, you could just as easily commit the transaction here and make
> the caller smart enough to know that it no longer has a transaction.  I
> wonder if the transaction allocation and disposal ought to be part of
> the _ag_grow_space and _ag_shrink_space functions.
> 
> Also fwiw I would make sure the transaction is clean before I tried to
> re-initialize the per-ag reservation.

Thanks for your review!

Okay, will look into roll transaction way instead (at least for
the new _ag_shrink_space() since I don't touch _grow_space and
not sure if it has AGFL refill issue as well...)

> 
> > +		err2 = xfs_ag_resv_init(agibp->b_pag, tp);
> > +		if (err2)
> > +			goto resv_err;
> > +		return error;
> > +	}
> > +
> > +	/*
> > +	 * if successfully deleted from freespace btrees, need to confirm
> > +	 * per-AG reservation works as expected.
> > +	 */
> > +	be32_add_cpu(&agi->agi_length, -len);
> > +	be32_add_cpu(&agf->agf_length, -len);
> > +
> > +	err2 = xfs_ag_resv_init(agibp->b_pag, tp);
> > +	if (err2) {
> > +		be32_add_cpu(&agi->agi_length, len);
> > +		be32_add_cpu(&agf->agf_length, len);
> > +		if (err2 != -ENOSPC)
> > +			goto resv_err;
> 
> If we've just undone reducing ag[if]_length, don't we need to call
> xfs_ag_resv_init here to (try to) recreate the former per-ag
> reservations?

If my understanding is correct, xfs_fs_reserve_ag_blocks() in
xfs_growfs_data_private() would do that for all AGs... Do we
need to xfs_ag_resv_init() in advance here?

I thought xfs_ag_resv_init() here is mainly used to guarantee the
per-AG reservation for resized size is fine... if ag{i,f}_length
don't change, leave such normal reservation to
xfs_fs_reserve_ag_blocks() would be okay?

> 
> Also, the comment above about cleaning the transaction before trying to
> reinit the per-ag reservation and returning ENOSPC applies here.

ok. that'd be here if rolling a new transaction is needed.
Thanks for the reminder!

Thanks,
Gao Xiang

> 
> > +
> > +		__xfs_bmap_add_free(tp, args.fsbno, len,
> > +				    &XFS_RMAP_OINFO_SKIP_UPDATE, true);
> > +		return err2;
> > +	}
> > +	xfs_ialloc_log_agi(tp, agibp, XFS_AGI_LENGTH);
> > +	xfs_alloc_log_agf(tp, agfbp, XFS_AGF_LENGTH);
> > +	return 0;
> > +
> > +resv_err:
> > +	xfs_warn(mp,
> > +"Error %d reserving per-AG metadata reserve pool.", err2);
> > +		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> > +	return err2;
> > +}