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:

...

quoted

 
+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...)

quoted

+		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

quoted

+
+		__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;
+}