[PATCH v7 0/5] xfs: support shrinking free space in the last AG

[PATCH v7 0/5] xfs: support shrinking free space in the last AG

[Gao Xiang]

Hi folks,

v6: https://lore.kernel.org/linux-xfs/20210126125621.3846735-1-hsiangkao@redhat.com/

This patchset attempts to support shrinking free space in the last AG.
This version mainly addresses previous review of v6. hope I don't miss
any previous comments. Details in the changelog.

gitweb:
https://git.kernel.org/pub/scm/linux/kernel/git/xiang/linux.git/ tags/xfs/shrink_lastag_v7

changes since v6:
 - rebase on the latest for-next;
 - [1/5] refine the comment (Brian);
 - [2/5] keep `delta' constant as nb - mp->m_sb.sb_dblocks, avoid
         `extent' boolean but introduce `lastag_resetagres' for growfs
         per-AG reservation judgement (Brian, Darrick);
 - [3/5] pass in agno directly for xfs_ag_shrink_space() (Brian);
 - [4/5] fix growfs 1 agcount case (Brian);
 - [4/5] refine xfs_fs_reserve_ag_blocks() in the end only for growfs
         due to mp->m_finobt_nores == false (Brian);
 - [5/5] broadened error tag (Brian, although I think xfs_mod_fdblocks
         shouldn't be triggered then.)

Thanks for the time!

Thanks,
Gao Xiang

xfsprogs: https://lore.kernel.org/r/20201028114010.545331-1-hsiangkao@redhat.com
xfstests: https://lore.kernel.org/r/20201028230909.639698-1-hsiangkao@redhat.com

Gao Xiang (5):
  xfs: update lazy sb counters immediately for resizefs
  xfs: hoist out xfs_resizefs_init_new_ags()
  xfs: introduce xfs_ag_shrink_space()
  xfs: support shrinking unused space in the last AG
  xfs: add error injection for per-AG resv failure

 fs/xfs/libxfs/xfs_ag.c       | 108 +++++++++++++++++++
 fs/xfs/libxfs/xfs_ag.h       |   2 +
 fs/xfs/libxfs/xfs_ag_resv.c  |   6 +-
 fs/xfs/libxfs/xfs_errortag.h |   4 +-
 fs/xfs/xfs_error.c           |   2 +
 fs/xfs/xfs_fsops.c           | 195 ++++++++++++++++++++++-------------
 fs/xfs/xfs_trans.c           |   1 -
 7 files changed, 242 insertions(+), 76 deletions(-)

-- 
2.27.0

[PATCH v7 1/5] xfs: update lazy sb counters immediately for resizefs

[Gao Xiang]

sb_fdblocks will be updated lazily if lazysbcount is enabled,
therefore when shrinking the filesystem sb_fdblocks could be
larger than sb_dblocks and xfs_validate_sb_write() would fail.

Even for growfs case, it'd be better to update lazy sb counters
immediately to reflect the real sb counters.

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/xfs/xfs_fsops.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index a2a407039227..9f9ba8bd0213 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -128,6 +128,15 @@ xfs_growfs_data_private(
 				 nb - mp->m_sb.sb_dblocks);
 	if (id.nfree)
 		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree);
+
+	/*
+	 * Sync sb counters now to reflect the updated values. This is
+	 * particularly important for shrink because the write verifier
+	 * will fail if sb_fdblocks is ever larger than sb_dblocks.
+	 */
+	if (xfs_sb_version_haslazysbcount(&mp->m_sb))
+		xfs_log_sb(tp);
+
 	xfs_trans_set_sync(tp);
 	error = xfs_trans_commit(tp);
 	if (error)
-- 
2.27.0

[PATCH v7 2/5] xfs: hoist out xfs_resizefs_init_new_ags()

[Gao Xiang]

Move out related logic for initializing new added AGs to a new helper
in preparation for shrinking. No logic changes.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/xfs/xfs_fsops.c | 110 ++++++++++++++++++++++++++-------------------
 1 file changed, 65 insertions(+), 45 deletions(-)

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 9f9ba8bd0213..494f9354e3fb 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -20,6 +20,64 @@
 #include "xfs_ag.h"
 #include "xfs_ag_resv.h"
 
+/*
+ * Write new AG headers to disk. Non-transactional, but need to be
+ * written and completed prior to the growfs transaction being logged.
+ * To do this, we use a delayed write buffer list and wait for
+ * submission and IO completion of the list as a whole. This allows the
+ * IO subsystem to merge all the AG headers in a single AG into a single
+ * IO and hide most of the latency of the IO from us.
+ *
+ * This also means that if we get an error whilst building the buffer
+ * list to write, we can cancel the entire list without having written
+ * anything.
+ */
+static int
+xfs_resizefs_init_new_ags(
+	struct xfs_trans	*tp,
+	struct aghdr_init_data	*id,
+	xfs_agnumber_t		oagcount,
+	xfs_agnumber_t		nagcount,
+	xfs_rfsblock_t		delta,
+	bool			*lastag_resetagres)
+{
+	struct xfs_mount	*mp = tp->t_mountp;
+	xfs_rfsblock_t		nb = mp->m_sb.sb_dblocks + delta;
+	int			error;
+
+	*lastag_resetagres = false;
+
+	INIT_LIST_HEAD(&id->buffer_list);
+	for (id->agno = nagcount - 1;
+	     id->agno >= oagcount;
+	     id->agno--, delta -= id->agsize) {
+
+		if (id->agno == nagcount - 1)
+			id->agsize = nb - (id->agno *
+					(xfs_rfsblock_t)mp->m_sb.sb_agblocks);
+		else
+			id->agsize = mp->m_sb.sb_agblocks;
+
+		error = xfs_ag_init_headers(mp, id);
+		if (error) {
+			xfs_buf_delwri_cancel(&id->buffer_list);
+			return error;
+		}
+	}
+
+	error = xfs_buf_delwri_submit(&id->buffer_list);
+	if (error)
+		return error;
+
+	xfs_trans_agblocks_delta(tp, id->nfree);
+
+	if (delta) {
+		*lastag_resetagres = true;
+		error = xfs_ag_extend_space(mp, tp, id, delta);
+	}
+	return error;
+}
+
 /*
  * growfs operations
  */
@@ -32,8 +90,8 @@ xfs_growfs_data_private(
 	int			error;
 	xfs_agnumber_t		nagcount;
 	xfs_agnumber_t		nagimax = 0;
-	xfs_rfsblock_t		nb, nb_div, nb_mod;
-	xfs_rfsblock_t		delta;
+	xfs_rfsblock_t		nb, nb_div, nb_mod, delta;
+	bool			lastag_resetagres;
 	xfs_agnumber_t		oagcount;
 	struct xfs_trans	*tp;
 	struct aghdr_init_data	id = {};
@@ -74,48 +132,11 @@ xfs_growfs_data_private(
 	if (error)
 		return error;
 
-	/*
-	 * Write new AG headers to disk. Non-transactional, but need to be
-	 * written and completed prior to the growfs transaction being logged.
-	 * To do this, we use a delayed write buffer list and wait for
-	 * submission and IO completion of the list as a whole. This allows the
-	 * IO subsystem to merge all the AG headers in a single AG into a single
-	 * IO and hide most of the latency of the IO from us.
-	 *
-	 * This also means that if we get an error whilst building the buffer
-	 * list to write, we can cancel the entire list without having written
-	 * anything.
-	 */
-	INIT_LIST_HEAD(&id.buffer_list);
-	for (id.agno = nagcount - 1;
-	     id.agno >= oagcount;
-	     id.agno--, delta -= id.agsize) {
-
-		if (id.agno == nagcount - 1)
-			id.agsize = nb -
-				(id.agno * (xfs_rfsblock_t)mp->m_sb.sb_agblocks);
-		else
-			id.agsize = mp->m_sb.sb_agblocks;
-
-		error = xfs_ag_init_headers(mp, &id);
-		if (error) {
-			xfs_buf_delwri_cancel(&id.buffer_list);
-			goto out_trans_cancel;
-		}
-	}
-	error = xfs_buf_delwri_submit(&id.buffer_list);
+	error = xfs_resizefs_init_new_ags(tp, &id, oagcount, nagcount,
+					  delta, &lastag_resetagres);
 	if (error)
 		goto out_trans_cancel;
 
-	xfs_trans_agblocks_delta(tp, id.nfree);
-
-	/* If there are new blocks in the old last AG, extend it. */
-	if (delta) {
-		error = xfs_ag_extend_space(mp, tp, &id, delta);
-		if (error)
-			goto out_trans_cancel;
-	}
-
 	/*
 	 * Update changed superblock fields transactionally. These are not
 	 * seen by the rest of the world until the transaction commit applies
@@ -123,9 +144,8 @@ xfs_growfs_data_private(
 	 */
 	if (nagcount > oagcount)
 		xfs_trans_mod_sb(tp, XFS_TRANS_SB_AGCOUNT, nagcount - oagcount);
-	if (nb > mp->m_sb.sb_dblocks)
-		xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS,
-				 nb - mp->m_sb.sb_dblocks);
+	if (delta > 0)
+		xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS, delta);
 	if (id.nfree)
 		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree);
 
@@ -152,7 +172,7 @@ xfs_growfs_data_private(
 	 * If we expanded the last AG, free the per-AG reservation
 	 * so we can reinitialize it with the new size.
 	 */
-	if (delta) {
+	if (lastag_resetagres) {
 		struct xfs_perag	*pag;
 
 		pag = xfs_perag_get(mp, id.agno);
-- 
2.27.0

[PATCH v7 3/5] xfs: introduce xfs_ag_shrink_space()

[Gao Xiang]

This patch introduces a helper to shrink unused space in the last AG
by fixing up the freespace btree.

Also make sure that the per-AG reservation works under the new AG
size. If such per-AG reservation or extent allocation fails, roll
the transaction so the new transaction could cancel without any side
effects.

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/xfs/libxfs/xfs_ag.c | 108 +++++++++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_ag.h |   2 +
 2 files changed, 110 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
index 9331f3516afa..a1128814630a 100644
--- a/fs/xfs/libxfs/xfs_ag.c
+++ b/fs/xfs/libxfs/xfs_ag.c
@@ -22,6 +22,11 @@
 #include "xfs_ag.h"
 #include "xfs_ag_resv.h"
 #include "xfs_health.h"
+#include "xfs_error.h"
+#include "xfs_bmap.h"
+#include "xfs_defer.h"
+#include "xfs_log_format.h"
+#include "xfs_trans.h"
 
 static int
 xfs_get_aghdr_buf(
@@ -485,6 +490,109 @@ xfs_ag_init_headers(
 	return error;
 }
 
+int
+xfs_ag_shrink_space(
+	struct xfs_mount	*mp,
+	struct xfs_trans	**tpp,
+	xfs_agnumber_t		agno,
+	xfs_extlen_t		len)
+{
+	struct xfs_alloc_arg	args = {
+		.tp	= *tpp,
+		.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(agno == mp->m_sb.sb_agcount - 1);
+	error = xfs_ialloc_read_agi(mp, *tpp, agno, &agibp);
+	if (error)
+		return error;
+
+	agi = agibp->b_addr;
+
+	error = xfs_alloc_read_agf(mp, *tpp, 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, 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) {
+		/*
+		 * if extent allocation fails, need to roll the transaction to
+		 * ensure that the AGFL fixup has been committed anyway.
+		 */
+		err2 = xfs_trans_roll(tpp);
+		if (err2)
+			return err2;
+		goto resv_init_out;
+	}
+
+	/*
+	 * 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, *tpp);
+	if (err2) {
+		be32_add_cpu(&agi->agi_length, len);
+		be32_add_cpu(&agf->agf_length, len);
+		if (err2 != -ENOSPC)
+			goto resv_err;
+
+		__xfs_bmap_add_free(*tpp, args.fsbno, len, NULL, true);
+
+		/*
+		 * Roll the transaction before trying to re-init the per-ag
+		 * reservation. The new transaction is clean so it will cancel
+		 * without any side effects.
+		 */
+		error = xfs_defer_finish(tpp);
+		if (error)
+			return error;
+
+		error = -ENOSPC;
+		goto resv_init_out;
+	}
+	xfs_ialloc_log_agi(*tpp, agibp, XFS_AGI_LENGTH);
+	xfs_alloc_log_agf(*tpp, agfbp, XFS_AGF_LENGTH);
+	return 0;
+
+resv_init_out:
+	err2 = xfs_ag_resv_init(agibp->b_pag, *tpp);
+	if (!err2)
+		return error;
+resv_err:
+	xfs_warn(mp, "Error %d reserving per-AG metadata reserve pool.", err2);
+	xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
+	return err2;
+}
+
 /*
  * Extent the AG indicated by the @id by the length passed in
  */
diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
index 5166322807e7..f33388eb130a 100644
--- a/fs/xfs/libxfs/xfs_ag.h
+++ b/fs/xfs/libxfs/xfs_ag.h
@@ -24,6 +24,8 @@ struct aghdr_init_data {
 };
 
 int xfs_ag_init_headers(struct xfs_mount *mp, struct aghdr_init_data *id);
+int xfs_ag_shrink_space(struct xfs_mount *mp, struct xfs_trans **tpp,
+			xfs_agnumber_t agno, xfs_extlen_t len);
 int xfs_ag_extend_space(struct xfs_mount *mp, struct xfs_trans *tp,
 			struct aghdr_init_data *id, xfs_extlen_t len);
 int xfs_ag_get_geometry(struct xfs_mount *mp, xfs_agnumber_t agno,
-- 
2.27.0

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

[Gao Xiang]

As the first step of shrinking, this attempts to enable shrinking
unused space in the last allocation group by fixing up freespace
btree, agi, agf and adjusting super block and use a helper
xfs_ag_shrink_space() to fixup the last AG.

This can be all done in one transaction for now, so I think no
additional protection is needed.

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/xfs/xfs_fsops.c | 90 ++++++++++++++++++++++++++++------------------
 fs/xfs/xfs_trans.c |  1 -
 2 files changed, 55 insertions(+), 36 deletions(-)

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 494f9354e3fb..204c96d0010f 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -90,23 +90,29 @@ xfs_growfs_data_private(
 	int			error;
 	xfs_agnumber_t		nagcount;
 	xfs_agnumber_t		nagimax = 0;
-	xfs_rfsblock_t		nb, nb_div, nb_mod, delta;
+	xfs_rfsblock_t		nb, nb_div, nb_mod;
+	int64_t			delta;
 	bool			lastag_resetagres;
 	xfs_agnumber_t		oagcount;
 	struct xfs_trans	*tp;
 	struct aghdr_init_data	id = {};
 
 	nb = in->newblocks;
-	if (nb < mp->m_sb.sb_dblocks)
-		return -EINVAL;
-	if ((error = xfs_sb_validate_fsb_count(&mp->m_sb, nb)))
+	if (nb == mp->m_sb.sb_dblocks)
+		return 0;
+
+	error = xfs_sb_validate_fsb_count(&mp->m_sb, nb);
+	if (error)
 		return error;
-	error = xfs_buf_read_uncached(mp->m_ddev_targp,
+
+	if (nb > mp->m_sb.sb_dblocks) {
+		error = xfs_buf_read_uncached(mp->m_ddev_targp,
 				XFS_FSB_TO_BB(mp, nb) - XFS_FSS_TO_BB(mp, 1),
 				XFS_FSS_TO_BB(mp, 1), 0, &bp, NULL);
-	if (error)
-		return error;
-	xfs_buf_relse(bp);
+		if (error)
+			return error;
+		xfs_buf_relse(bp);
+	}
 
 	nb_div = nb;
 	nb_mod = do_div(nb_div, mp->m_sb.sb_agblocks);
@@ -114,10 +120,15 @@ xfs_growfs_data_private(
 	if (nb_mod && nb_mod < XFS_MIN_AG_BLOCKS) {
 		nagcount--;
 		nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks;
-		if (nb < mp->m_sb.sb_dblocks)
-			return -EINVAL;
 	}
 	delta = nb - mp->m_sb.sb_dblocks;
+	/*
+	 * XFS doesn't really support single-AG filesystems, so do not
+	 * permit callers to remove the filesystem's second and last AG.
+	 */
+	if (delta < 0 && nagcount < 2)
+		return -EINVAL;
+
 	oagcount = mp->m_sb.sb_agcount;
 
 	/* allocate the new per-ag structures */
@@ -125,15 +136,23 @@ xfs_growfs_data_private(
 		error = xfs_initialize_perag(mp, nagcount, &nagimax);
 		if (error)
 			return error;
+	} else if (nagcount < oagcount) {
+		/* TODO: shrinking the entire AGs hasn't yet completed */
+		return -EINVAL;
 	}
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata,
-			XFS_GROWFS_SPACE_RES(mp), 0, XFS_TRANS_RESERVE, &tp);
+			(delta > 0 ? XFS_GROWFS_SPACE_RES(mp) : -delta), 0,
+			XFS_TRANS_RESERVE, &tp);
 	if (error)
 		return error;
 
-	error = xfs_resizefs_init_new_ags(tp, &id, oagcount, nagcount,
-					  delta, &lastag_resetagres);
+	if (delta > 0)
+		error = xfs_resizefs_init_new_ags(tp, &id, oagcount, nagcount,
+						  delta, &lastag_resetagres);
+	else
+		error = xfs_ag_shrink_space(mp, &tp, nagcount - 1, -delta);
+
 	if (error)
 		goto out_trans_cancel;
 
@@ -144,7 +163,7 @@ xfs_growfs_data_private(
 	 */
 	if (nagcount > oagcount)
 		xfs_trans_mod_sb(tp, XFS_TRANS_SB_AGCOUNT, nagcount - oagcount);
-	if (delta > 0)
+	if (delta)
 		xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS, delta);
 	if (id.nfree)
 		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree);
@@ -168,28 +187,29 @@ xfs_growfs_data_private(
 	xfs_set_low_space_thresholds(mp);
 	mp->m_alloc_set_aside = xfs_alloc_set_aside(mp);
 
-	/*
-	 * If we expanded the last AG, free the per-AG reservation
-	 * so we can reinitialize it with the new size.
-	 */
-	if (lastag_resetagres) {
-		struct xfs_perag	*pag;
-
-		pag = xfs_perag_get(mp, id.agno);
-		error = xfs_ag_resv_free(pag);
-		xfs_perag_put(pag);
-		if (error)
-			return error;
+	if (delta > 0) {
+		/*
+		 * If we expanded the last AG, free the per-AG reservation
+		 * so we can reinitialize it with the new size.
+		 */
+		if (lastag_resetagres) {
+			struct xfs_perag	*pag;
+
+			pag = xfs_perag_get(mp, id.agno);
+			error = xfs_ag_resv_free(pag);
+			xfs_perag_put(pag);
+			if (error)
+				return error;
+		}
+		/*
+		 * Reserve AG metadata blocks. ENOSPC here does not mean there
+		 * was a growfs failure, just that there still isn't space for
+		 * new user data after the grow has been run.
+		 */
+		error = xfs_fs_reserve_ag_blocks(mp);
+		if (error == -ENOSPC)
+			error = 0;
 	}
-
-	/*
-	 * Reserve AG metadata blocks. ENOSPC here does not mean there was a
-	 * growfs failure, just that there still isn't space for new user data
-	 * after the grow has been run.
-	 */
-	error = xfs_fs_reserve_ag_blocks(mp);
-	if (error == -ENOSPC)
-		error = 0;
 	return error;
 
 out_trans_cancel:
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 44f72c09c203..d047f5f26cc0 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -434,7 +434,6 @@ xfs_trans_mod_sb(
 		tp->t_res_frextents_delta += delta;
 		break;
 	case XFS_TRANS_SB_DBLOCKS:
-		ASSERT(delta > 0);
 		tp->t_dblocks_delta += delta;
 		break;
 	case XFS_TRANS_SB_AGCOUNT:
-- 
2.27.0

[PATCH v7 5/5] xfs: add error injection for per-AG resv failure

[Gao Xiang]

per-AG resv failure after fixing up freespace is hard to test in an
effective way, so directly add an error injection path to observe
such error handling path works as expected.

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/xfs/libxfs/xfs_ag_resv.c  | 6 +++++-
 fs/xfs/libxfs/xfs_errortag.h | 4 +++-
 fs/xfs/xfs_error.c           | 2 ++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
index fdfe6dc0d307..6c5f8d10589c 100644
--- a/fs/xfs/libxfs/xfs_ag_resv.c
+++ b/fs/xfs/libxfs/xfs_ag_resv.c
@@ -211,7 +211,11 @@ __xfs_ag_resv_init(
 		ASSERT(0);
 		return -EINVAL;
 	}
-	error = xfs_mod_fdblocks(mp, -(int64_t)hidden_space, true);
+
+	if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_AG_RESV_FAIL))
+		error = -ENOSPC;
+	else
+		error = xfs_mod_fdblocks(mp, -(int64_t)hidden_space, true);
 	if (error) {
 		trace_xfs_ag_resv_init_error(pag->pag_mount, pag->pag_agno,
 				error, _RET_IP_);
diff --git a/fs/xfs/libxfs/xfs_errortag.h b/fs/xfs/libxfs/xfs_errortag.h
index 6ca9084b6934..b433ef735217 100644
--- a/fs/xfs/libxfs/xfs_errortag.h
+++ b/fs/xfs/libxfs/xfs_errortag.h
@@ -40,6 +40,7 @@
 #define XFS_ERRTAG_REFCOUNT_FINISH_ONE			25
 #define XFS_ERRTAG_BMAP_FINISH_ONE			26
 #define XFS_ERRTAG_AG_RESV_CRITICAL			27
+
 /*
  * DEBUG mode instrumentation to test and/or trigger delayed allocation
  * block killing in the event of failed writes. When enabled, all
@@ -58,7 +59,8 @@
 #define XFS_ERRTAG_BUF_IOERROR				35
 #define XFS_ERRTAG_REDUCE_MAX_IEXTENTS			36
 #define XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT		37
-#define XFS_ERRTAG_MAX					38
+#define XFS_ERRTAG_AG_RESV_FAIL				38
+#define XFS_ERRTAG_MAX					39
 
 /*
  * Random factors for above tags, 1 means always, 2 means 1/2 time, etc.
diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index 185b4915b7bf..5192a7063d95 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -168,6 +168,7 @@ XFS_ERRORTAG_ATTR_RW(iunlink_fallback,	XFS_ERRTAG_IUNLINK_FALLBACK);
 XFS_ERRORTAG_ATTR_RW(buf_ioerror,	XFS_ERRTAG_BUF_IOERROR);
 XFS_ERRORTAG_ATTR_RW(reduce_max_iextents,	XFS_ERRTAG_REDUCE_MAX_IEXTENTS);
 XFS_ERRORTAG_ATTR_RW(bmap_alloc_minlen_extent,	XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT);
+XFS_ERRORTAG_ATTR_RW(ag_resv_fail, XFS_ERRTAG_AG_RESV_FAIL);
 
 static struct attribute *xfs_errortag_attrs[] = {
 	XFS_ERRORTAG_ATTR_LIST(noerror),
@@ -208,6 +209,7 @@ static struct attribute *xfs_errortag_attrs[] = {
 	XFS_ERRORTAG_ATTR_LIST(buf_ioerror),
 	XFS_ERRORTAG_ATTR_LIST(reduce_max_iextents),
 	XFS_ERRORTAG_ATTR_LIST(bmap_alloc_minlen_extent),
+	XFS_ERRORTAG_ATTR_LIST(ag_resv_fail),
 	NULL,
 };
 
-- 
2.27.0

[PATCH v7.1 5/5] xfs: add error injection for per-AG resv failure

[Gao Xiang]

per-AG resv failure after fixing up freespace is hard to test in an
effective way, so directly add an error injection path to observe
such error handling path works as expected.

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
changes since v7:
 - add missing random errortag reported by 0day CI and smatch.

 fs/xfs/libxfs/xfs_ag_resv.c  | 6 +++++-
 fs/xfs/libxfs/xfs_errortag.h | 4 +++-
 fs/xfs/xfs_error.c           | 3 +++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
index fdfe6dc0d307..6c5f8d10589c 100644
--- a/fs/xfs/libxfs/xfs_ag_resv.c
+++ b/fs/xfs/libxfs/xfs_ag_resv.c
@@ -211,7 +211,11 @@ __xfs_ag_resv_init(
 		ASSERT(0);
 		return -EINVAL;
 	}
-	error = xfs_mod_fdblocks(mp, -(int64_t)hidden_space, true);
+
+	if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_AG_RESV_FAIL))
+		error = -ENOSPC;
+	else
+		error = xfs_mod_fdblocks(mp, -(int64_t)hidden_space, true);
 	if (error) {
 		trace_xfs_ag_resv_init_error(pag->pag_mount, pag->pag_agno,
 				error, _RET_IP_);
diff --git a/fs/xfs/libxfs/xfs_errortag.h b/fs/xfs/libxfs/xfs_errortag.h
index 6ca9084b6934..a23a52e643ad 100644
--- a/fs/xfs/libxfs/xfs_errortag.h
+++ b/fs/xfs/libxfs/xfs_errortag.h
@@ -58,7 +58,8 @@
 #define XFS_ERRTAG_BUF_IOERROR				35
 #define XFS_ERRTAG_REDUCE_MAX_IEXTENTS			36
 #define XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT		37
-#define XFS_ERRTAG_MAX					38
+#define XFS_ERRTAG_AG_RESV_FAIL				38
+#define XFS_ERRTAG_MAX					39
 
 /*
  * Random factors for above tags, 1 means always, 2 means 1/2 time, etc.
@@ -101,5 +102,6 @@
 #define XFS_RANDOM_BUF_IOERROR				XFS_RANDOM_DEFAULT
 #define XFS_RANDOM_REDUCE_MAX_IEXTENTS			1
 #define XFS_RANDOM_BMAP_ALLOC_MINLEN_EXTENT		1
+#define XFS_RANDOM_AG_RESV_FAIL				1
 
 #endif /* __XFS_ERRORTAG_H_ */
diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
index 185b4915b7bf..f70984f3174d 100644
--- a/fs/xfs/xfs_error.c
+++ b/fs/xfs/xfs_error.c
@@ -56,6 +56,7 @@ static unsigned int xfs_errortag_random_default[] = {
 	XFS_RANDOM_BUF_IOERROR,
 	XFS_RANDOM_REDUCE_MAX_IEXTENTS,
 	XFS_RANDOM_BMAP_ALLOC_MINLEN_EXTENT,
+	XFS_RANDOM_AG_RESV_FAIL,
 };
 
 struct xfs_errortag_attr {
@@ -168,6 +169,7 @@ XFS_ERRORTAG_ATTR_RW(iunlink_fallback,	XFS_ERRTAG_IUNLINK_FALLBACK);
 XFS_ERRORTAG_ATTR_RW(buf_ioerror,	XFS_ERRTAG_BUF_IOERROR);
 XFS_ERRORTAG_ATTR_RW(reduce_max_iextents,	XFS_ERRTAG_REDUCE_MAX_IEXTENTS);
 XFS_ERRORTAG_ATTR_RW(bmap_alloc_minlen_extent,	XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT);
+XFS_ERRORTAG_ATTR_RW(ag_resv_fail, XFS_ERRTAG_AG_RESV_FAIL);
 
 static struct attribute *xfs_errortag_attrs[] = {
 	XFS_ERRORTAG_ATTR_LIST(noerror),
@@ -208,6 +210,7 @@ static struct attribute *xfs_errortag_attrs[] = {
 	XFS_ERRORTAG_ATTR_LIST(buf_ioerror),
 	XFS_ERRORTAG_ATTR_LIST(reduce_max_iextents),
 	XFS_ERRORTAG_ATTR_LIST(bmap_alloc_minlen_extent),
+	XFS_ERRORTAG_ATTR_LIST(ag_resv_fail),
 	NULL,
 };
 
-- 
2.27.0

Re: [PATCH v7 1/5] xfs: update lazy sb counters immediately for resizefs

[Darrick J. Wong]

On Tue, Mar 02, 2021 at 10:48:12AM +0800, Gao Xiang wrote:
> sb_fdblocks will be updated lazily if lazysbcount is enabled,
> therefore when shrinking the filesystem sb_fdblocks could be
> larger than sb_dblocks and xfs_validate_sb_write() would fail.
> 
> Even for growfs case, it'd be better to update lazy sb counters
> immediately to reflect the real sb counters.
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

Looks good to me,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_fsops.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index a2a407039227..9f9ba8bd0213 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -128,6 +128,15 @@ xfs_growfs_data_private(
>  				 nb - mp->m_sb.sb_dblocks);
>  	if (id.nfree)
>  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree);
> +
> +	/*
> +	 * Sync sb counters now to reflect the updated values. This is
> +	 * particularly important for shrink because the write verifier
> +	 * will fail if sb_fdblocks is ever larger than sb_dblocks.
> +	 */
> +	if (xfs_sb_version_haslazysbcount(&mp->m_sb))
> +		xfs_log_sb(tp);
> +
>  	xfs_trans_set_sync(tp);
>  	error = xfs_trans_commit(tp);
>  	if (error)
> -- 
> 2.27.0
> 

Re: [PATCH v7 3/5] xfs: introduce xfs_ag_shrink_space()

[Darrick J. Wong]

On Tue, Mar 02, 2021 at 10:48:14AM +0800, Gao Xiang wrote:
> This patch introduces a helper to shrink unused space in the last AG
> by fixing up the freespace btree.
> 
> Also make sure that the per-AG reservation works under the new AG
> size. If such per-AG reservation or extent allocation fails, roll
> the transaction so the new transaction could cancel without any side
> effects.
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_ag.c | 108 +++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/libxfs/xfs_ag.h |   2 +
>  2 files changed, 110 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> index 9331f3516afa..a1128814630a 100644
> --- a/fs/xfs/libxfs/xfs_ag.c
> +++ b/fs/xfs/libxfs/xfs_ag.c
> @@ -22,6 +22,11 @@
>  #include "xfs_ag.h"
>  #include "xfs_ag_resv.h"
>  #include "xfs_health.h"
> +#include "xfs_error.h"
> +#include "xfs_bmap.h"
> +#include "xfs_defer.h"
> +#include "xfs_log_format.h"
> +#include "xfs_trans.h"
>  
>  static int
>  xfs_get_aghdr_buf(
> @@ -485,6 +490,109 @@ xfs_ag_init_headers(
>  	return error;
>  }
>  
> +int
> +xfs_ag_shrink_space(
> +	struct xfs_mount	*mp,
> +	struct xfs_trans	**tpp,
> +	xfs_agnumber_t		agno,
> +	xfs_extlen_t		len)
> +{
> +	struct xfs_alloc_arg	args = {
> +		.tp	= *tpp,
> +		.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(agno == mp->m_sb.sb_agcount - 1);
> +	error = xfs_ialloc_read_agi(mp, *tpp, agno, &agibp);
> +	if (error)
> +		return error;
> +
> +	agi = agibp->b_addr;
> +
> +	error = xfs_alloc_read_agf(mp, *tpp, 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, agno,
> +				    be32_to_cpu(agi->agi_length) - len);

Paranoia nit: Should we check that len < agi_length?

> +
> +	/* 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);

I forget, does xfs_alloc_vextent ever roll args.tp?

Other than those two things this looks good to me.

--D

> +	if (!error && args.agbno == NULLAGBLOCK)
> +		error = -ENOSPC;
> +
> +	if (error) {
> +		/*
> +		 * if extent allocation fails, need to roll the transaction to
> +		 * ensure that the AGFL fixup has been committed anyway.
> +		 */
> +		err2 = xfs_trans_roll(tpp);
> +		if (err2)
> +			return err2;
> +		goto resv_init_out;
> +	}
> +
> +	/*
> +	 * 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, *tpp);
> +	if (err2) {
> +		be32_add_cpu(&agi->agi_length, len);
> +		be32_add_cpu(&agf->agf_length, len);
> +		if (err2 != -ENOSPC)
> +			goto resv_err;
> +
> +		__xfs_bmap_add_free(*tpp, args.fsbno, len, NULL, true);
> +
> +		/*
> +		 * Roll the transaction before trying to re-init the per-ag
> +		 * reservation. The new transaction is clean so it will cancel
> +		 * without any side effects.
> +		 */
> +		error = xfs_defer_finish(tpp);
> +		if (error)
> +			return error;
> +
> +		error = -ENOSPC;
> +		goto resv_init_out;
> +	}
> +	xfs_ialloc_log_agi(*tpp, agibp, XFS_AGI_LENGTH);
> +	xfs_alloc_log_agf(*tpp, agfbp, XFS_AGF_LENGTH);
> +	return 0;
> +
> +resv_init_out:
> +	err2 = xfs_ag_resv_init(agibp->b_pag, *tpp);
> +	if (!err2)
> +		return error;
> +resv_err:
> +	xfs_warn(mp, "Error %d reserving per-AG metadata reserve pool.", err2);
> +	xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> +	return err2;
> +}
> +
>  /*
>   * Extent the AG indicated by the @id by the length passed in
>   */
> diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
> index 5166322807e7..f33388eb130a 100644
> --- a/fs/xfs/libxfs/xfs_ag.h
> +++ b/fs/xfs/libxfs/xfs_ag.h
> @@ -24,6 +24,8 @@ struct aghdr_init_data {
>  };
>  
>  int xfs_ag_init_headers(struct xfs_mount *mp, struct aghdr_init_data *id);
> +int xfs_ag_shrink_space(struct xfs_mount *mp, struct xfs_trans **tpp,
> +			xfs_agnumber_t agno, xfs_extlen_t len);
>  int xfs_ag_extend_space(struct xfs_mount *mp, struct xfs_trans *tp,
>  			struct aghdr_init_data *id, xfs_extlen_t len);
>  int xfs_ag_get_geometry(struct xfs_mount *mp, xfs_agnumber_t agno,
> -- 
> 2.27.0
> 

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

[Darrick J. Wong]

On Tue, Mar 02, 2021 at 10:48:15AM +0800, Gao Xiang wrote:
> As the first step of shrinking, this attempts to enable shrinking
> unused space in the last allocation group by fixing up freespace
> btree, agi, agf and adjusting super block and use a helper
> xfs_ag_shrink_space() to fixup the last AG.
> 
> This can be all done in one transaction for now, so I think no
> additional protection is needed.
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
>  fs/xfs/xfs_fsops.c | 90 ++++++++++++++++++++++++++++------------------
>  fs/xfs/xfs_trans.c |  1 -
>  2 files changed, 55 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index 494f9354e3fb..204c96d0010f 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -90,23 +90,29 @@ xfs_growfs_data_private(
>  	int			error;
>  	xfs_agnumber_t		nagcount;
>  	xfs_agnumber_t		nagimax = 0;
> -	xfs_rfsblock_t		nb, nb_div, nb_mod, delta;
> +	xfs_rfsblock_t		nb, nb_div, nb_mod;
> +	int64_t			delta;
>  	bool			lastag_resetagres;
>  	xfs_agnumber_t		oagcount;
>  	struct xfs_trans	*tp;
>  	struct aghdr_init_data	id = {};
>  
>  	nb = in->newblocks;
> -	if (nb < mp->m_sb.sb_dblocks)
> -		return -EINVAL;
> -	if ((error = xfs_sb_validate_fsb_count(&mp->m_sb, nb)))
> +	if (nb == mp->m_sb.sb_dblocks)
> +		return 0;
> +
> +	error = xfs_sb_validate_fsb_count(&mp->m_sb, nb);
> +	if (error)
>  		return error;
> -	error = xfs_buf_read_uncached(mp->m_ddev_targp,
> +
> +	if (nb > mp->m_sb.sb_dblocks) {
> +		error = xfs_buf_read_uncached(mp->m_ddev_targp,
>  				XFS_FSB_TO_BB(mp, nb) - XFS_FSS_TO_BB(mp, 1),
>  				XFS_FSS_TO_BB(mp, 1), 0, &bp, NULL);
> -	if (error)
> -		return error;
> -	xfs_buf_relse(bp);
> +		if (error)
> +			return error;
> +		xfs_buf_relse(bp);
> +	}
>  
>  	nb_div = nb;
>  	nb_mod = do_div(nb_div, mp->m_sb.sb_agblocks);
> @@ -114,10 +120,15 @@ xfs_growfs_data_private(
>  	if (nb_mod && nb_mod < XFS_MIN_AG_BLOCKS) {
>  		nagcount--;
>  		nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks;
> -		if (nb < mp->m_sb.sb_dblocks)
> -			return -EINVAL;
>  	}
>  	delta = nb - mp->m_sb.sb_dblocks;
> +	/*
> +	 * XFS doesn't really support single-AG filesystems, so do not
> +	 * permit callers to remove the filesystem's second and last AG.
> +	 */
> +	if (delta < 0 && nagcount < 2)
> +		return -EINVAL;
> +
>  	oagcount = mp->m_sb.sb_agcount;
>  
>  	/* allocate the new per-ag structures */
> @@ -125,15 +136,23 @@ xfs_growfs_data_private(
>  		error = xfs_initialize_perag(mp, nagcount, &nagimax);
>  		if (error)
>  			return error;
> +	} else if (nagcount < oagcount) {
> +		/* TODO: shrinking the entire AGs hasn't yet completed */
> +		return -EINVAL;
>  	}
>  
>  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata,
> -			XFS_GROWFS_SPACE_RES(mp), 0, XFS_TRANS_RESERVE, &tp);
> +			(delta > 0 ? XFS_GROWFS_SPACE_RES(mp) : -delta), 0,
> +			XFS_TRANS_RESERVE, &tp);
>  	if (error)
>  		return error;
>  
> -	error = xfs_resizefs_init_new_ags(tp, &id, oagcount, nagcount,
> -					  delta, &lastag_resetagres);
> +	if (delta > 0)
> +		error = xfs_resizefs_init_new_ags(tp, &id, oagcount, nagcount,
> +						  delta, &lastag_resetagres);
> +	else
> +		error = xfs_ag_shrink_space(mp, &tp, nagcount - 1, -delta);

Hm, looking back at the previous patch, I think the last argument to
this function should be named "shrink_len" (or maybe just delta?) so
that future readers don't confuse it for the new (shorter) AG length.

> +

Nit: no blank line needed here.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  	if (error)
>  		goto out_trans_cancel;
>  
> @@ -144,7 +163,7 @@ xfs_growfs_data_private(
>  	 */
>  	if (nagcount > oagcount)
>  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_AGCOUNT, nagcount - oagcount);
> -	if (delta > 0)
> +	if (delta)
>  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS, delta);
>  	if (id.nfree)
>  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree);
> @@ -168,28 +187,29 @@ xfs_growfs_data_private(
>  	xfs_set_low_space_thresholds(mp);
>  	mp->m_alloc_set_aside = xfs_alloc_set_aside(mp);
>  
> -	/*
> -	 * If we expanded the last AG, free the per-AG reservation
> -	 * so we can reinitialize it with the new size.
> -	 */
> -	if (lastag_resetagres) {
> -		struct xfs_perag	*pag;
> -
> -		pag = xfs_perag_get(mp, id.agno);
> -		error = xfs_ag_resv_free(pag);
> -		xfs_perag_put(pag);
> -		if (error)
> -			return error;
> +	if (delta > 0) {
> +		/*
> +		 * If we expanded the last AG, free the per-AG reservation
> +		 * so we can reinitialize it with the new size.
> +		 */
> +		if (lastag_resetagres) {
> +			struct xfs_perag	*pag;
> +
> +			pag = xfs_perag_get(mp, id.agno);
> +			error = xfs_ag_resv_free(pag);
> +			xfs_perag_put(pag);
> +			if (error)
> +				return error;
> +		}
> +		/*
> +		 * Reserve AG metadata blocks. ENOSPC here does not mean there
> +		 * was a growfs failure, just that there still isn't space for
> +		 * new user data after the grow has been run.
> +		 */
> +		error = xfs_fs_reserve_ag_blocks(mp);
> +		if (error == -ENOSPC)
> +			error = 0;
>  	}
> -
> -	/*
> -	 * Reserve AG metadata blocks. ENOSPC here does not mean there was a
> -	 * growfs failure, just that there still isn't space for new user data
> -	 * after the grow has been run.
> -	 */
> -	error = xfs_fs_reserve_ag_blocks(mp);
> -	if (error == -ENOSPC)
> -		error = 0;
>  	return error;
>  
>  out_trans_cancel:
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 44f72c09c203..d047f5f26cc0 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -434,7 +434,6 @@ xfs_trans_mod_sb(
>  		tp->t_res_frextents_delta += delta;
>  		break;
>  	case XFS_TRANS_SB_DBLOCKS:
> -		ASSERT(delta > 0);
>  		tp->t_dblocks_delta += delta;
>  		break;
>  	case XFS_TRANS_SB_AGCOUNT:
> -- 
> 2.27.0
> 

Re: [PATCH v7 5/5] xfs: add error injection for per-AG resv failure

[Darrick J. Wong]

On Tue, Mar 02, 2021 at 10:48:16AM +0800, Gao Xiang wrote:
> per-AG resv failure after fixing up freespace is hard to test in an
> effective way, so directly add an error injection path to observe
> such error handling path works as expected.
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_ag_resv.c  | 6 +++++-
>  fs/xfs/libxfs/xfs_errortag.h | 4 +++-
>  fs/xfs/xfs_error.c           | 2 ++
>  3 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
> index fdfe6dc0d307..6c5f8d10589c 100644
> --- a/fs/xfs/libxfs/xfs_ag_resv.c
> +++ b/fs/xfs/libxfs/xfs_ag_resv.c
> @@ -211,7 +211,11 @@ __xfs_ag_resv_init(
>  		ASSERT(0);
>  		return -EINVAL;
>  	}
> -	error = xfs_mod_fdblocks(mp, -(int64_t)hidden_space, true);
> +
> +	if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_AG_RESV_FAIL))
> +		error = -ENOSPC;
> +	else
> +		error = xfs_mod_fdblocks(mp, -(int64_t)hidden_space, true);
>  	if (error) {
>  		trace_xfs_ag_resv_init_error(pag->pag_mount, pag->pag_agno,
>  				error, _RET_IP_);
> diff --git a/fs/xfs/libxfs/xfs_errortag.h b/fs/xfs/libxfs/xfs_errortag.h
> index 6ca9084b6934..b433ef735217 100644
> --- a/fs/xfs/libxfs/xfs_errortag.h
> +++ b/fs/xfs/libxfs/xfs_errortag.h
> @@ -40,6 +40,7 @@
>  #define XFS_ERRTAG_REFCOUNT_FINISH_ONE			25
>  #define XFS_ERRTAG_BMAP_FINISH_ONE			26
>  #define XFS_ERRTAG_AG_RESV_CRITICAL			27
> +

Extra space?

>  /*
>   * DEBUG mode instrumentation to test and/or trigger delayed allocation
>   * block killing in the event of failed writes. When enabled, all
> @@ -58,7 +59,8 @@
>  #define XFS_ERRTAG_BUF_IOERROR				35
>  #define XFS_ERRTAG_REDUCE_MAX_IEXTENTS			36
>  #define XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT		37
> -#define XFS_ERRTAG_MAX					38
> +#define XFS_ERRTAG_AG_RESV_FAIL				38
> +#define XFS_ERRTAG_MAX					39
>  
>  /*
>   * Random factors for above tags, 1 means always, 2 means 1/2 time, etc.

This needs to define XFS_RANDOM_AG_RESV_FAIL and put it in
xfs_errortag_random_default in xfs_error.c to avoid running off the end
of the array.

Also... that _default array /really/ needs to have a BUILD_BUG_ON
somewhere to scream loudly if it isn't XFS_ERRTAG_MAX elements long.

--D

> diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> index 185b4915b7bf..5192a7063d95 100644
> --- a/fs/xfs/xfs_error.c
> +++ b/fs/xfs/xfs_error.c
> @@ -168,6 +168,7 @@ XFS_ERRORTAG_ATTR_RW(iunlink_fallback,	XFS_ERRTAG_IUNLINK_FALLBACK);
>  XFS_ERRORTAG_ATTR_RW(buf_ioerror,	XFS_ERRTAG_BUF_IOERROR);
>  XFS_ERRORTAG_ATTR_RW(reduce_max_iextents,	XFS_ERRTAG_REDUCE_MAX_IEXTENTS);
>  XFS_ERRORTAG_ATTR_RW(bmap_alloc_minlen_extent,	XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT);
> +XFS_ERRORTAG_ATTR_RW(ag_resv_fail, XFS_ERRTAG_AG_RESV_FAIL);
>  
>  static struct attribute *xfs_errortag_attrs[] = {
>  	XFS_ERRORTAG_ATTR_LIST(noerror),
> @@ -208,6 +209,7 @@ static struct attribute *xfs_errortag_attrs[] = {
>  	XFS_ERRORTAG_ATTR_LIST(buf_ioerror),
>  	XFS_ERRORTAG_ATTR_LIST(reduce_max_iextents),
>  	XFS_ERRORTAG_ATTR_LIST(bmap_alloc_minlen_extent),
> +	XFS_ERRORTAG_ATTR_LIST(ag_resv_fail),
>  	NULL,
>  };
>  
> -- 
> 2.27.0
> 

Re: [PATCH v7 5/5] xfs: add error injection for per-AG resv failure

[Gao Xiang]

Hi Darrick,

On Wed, Mar 03, 2021 at 10:30:42AM -0800, Darrick J. Wong wrote:
> On Tue, Mar 02, 2021 at 10:48:16AM +0800, Gao Xiang wrote:
> > per-AG resv failure after fixing up freespace is hard to test in an
> > effective way, so directly add an error injection path to observe
> > such error handling path works as expected.
> > 
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_ag_resv.c  | 6 +++++-
> >  fs/xfs/libxfs/xfs_errortag.h | 4 +++-
> >  fs/xfs/xfs_error.c           | 2 ++
> >  3 files changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_ag_resv.c b/fs/xfs/libxfs/xfs_ag_resv.c
> > index fdfe6dc0d307..6c5f8d10589c 100644
> > --- a/fs/xfs/libxfs/xfs_ag_resv.c
> > +++ b/fs/xfs/libxfs/xfs_ag_resv.c
> > @@ -211,7 +211,11 @@ __xfs_ag_resv_init(
> >  		ASSERT(0);
> >  		return -EINVAL;
> >  	}
> > -	error = xfs_mod_fdblocks(mp, -(int64_t)hidden_space, true);
> > +
> > +	if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_AG_RESV_FAIL))
> > +		error = -ENOSPC;
> > +	else
> > +		error = xfs_mod_fdblocks(mp, -(int64_t)hidden_space, true);
> >  	if (error) {
> >  		trace_xfs_ag_resv_init_error(pag->pag_mount, pag->pag_agno,
> >  				error, _RET_IP_);
> > diff --git a/fs/xfs/libxfs/xfs_errortag.h b/fs/xfs/libxfs/xfs_errortag.h
> > index 6ca9084b6934..b433ef735217 100644
> > --- a/fs/xfs/libxfs/xfs_errortag.h
> > +++ b/fs/xfs/libxfs/xfs_errortag.h
> > @@ -40,6 +40,7 @@
> >  #define XFS_ERRTAG_REFCOUNT_FINISH_ONE			25
> >  #define XFS_ERRTAG_BMAP_FINISH_ONE			26
> >  #define XFS_ERRTAG_AG_RESV_CRITICAL			27
> > +
> 
> Extra space?
> 
> >  /*
> >   * DEBUG mode instrumentation to test and/or trigger delayed allocation
> >   * block killing in the event of failed writes. When enabled, all
> > @@ -58,7 +59,8 @@
> >  #define XFS_ERRTAG_BUF_IOERROR				35
> >  #define XFS_ERRTAG_REDUCE_MAX_IEXTENTS			36
> >  #define XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT		37
> > -#define XFS_ERRTAG_MAX					38
> > +#define XFS_ERRTAG_AG_RESV_FAIL				38
> > +#define XFS_ERRTAG_MAX					39
> >  
> >  /*
> >   * Random factors for above tags, 1 means always, 2 means 1/2 time, etc.
> 
> This needs to define XFS_RANDOM_AG_RESV_FAIL and put it in
> xfs_errortag_random_default in xfs_error.c to avoid running off the end
> of the array.
> 
> Also... that _default array /really/ needs to have a BUILD_BUG_ON
> somewhere to scream loudly if it isn't XFS_ERRTAG_MAX elements long.

I've sent out v7.1 of this patch attached to this version,
https://lore.kernel.org/linux-xfs/20210303000202.2671220-1-hsiangkao@redhat.com/

Yes, I received a 0day CI report of this, would you mind
take a look at the fix above instead?

Thanks,
Ga oXiang

> 
> --D
> 
> > diff --git a/fs/xfs/xfs_error.c b/fs/xfs/xfs_error.c
> > index 185b4915b7bf..5192a7063d95 100644
> > --- a/fs/xfs/xfs_error.c
> > +++ b/fs/xfs/xfs_error.c
> > @@ -168,6 +168,7 @@ XFS_ERRORTAG_ATTR_RW(iunlink_fallback,	XFS_ERRTAG_IUNLINK_FALLBACK);
> >  XFS_ERRORTAG_ATTR_RW(buf_ioerror,	XFS_ERRTAG_BUF_IOERROR);
> >  XFS_ERRORTAG_ATTR_RW(reduce_max_iextents,	XFS_ERRTAG_REDUCE_MAX_IEXTENTS);
> >  XFS_ERRORTAG_ATTR_RW(bmap_alloc_minlen_extent,	XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT);
> > +XFS_ERRORTAG_ATTR_RW(ag_resv_fail, XFS_ERRTAG_AG_RESV_FAIL);
> >  
> >  static struct attribute *xfs_errortag_attrs[] = {
> >  	XFS_ERRORTAG_ATTR_LIST(noerror),
> > @@ -208,6 +209,7 @@ static struct attribute *xfs_errortag_attrs[] = {
> >  	XFS_ERRORTAG_ATTR_LIST(buf_ioerror),
> >  	XFS_ERRORTAG_ATTR_LIST(reduce_max_iextents),
> >  	XFS_ERRORTAG_ATTR_LIST(bmap_alloc_minlen_extent),
> > +	XFS_ERRORTAG_ATTR_LIST(ag_resv_fail),
> >  	NULL,
> >  };
> >  
> > -- 
> > 2.27.0
> > 
> 

Re: [PATCH v7 3/5] xfs: introduce xfs_ag_shrink_space()

[Gao Xiang]

On Wed, Mar 03, 2021 at 10:19:31AM -0800, Darrick J. Wong wrote:
> On Tue, Mar 02, 2021 at 10:48:14AM +0800, Gao Xiang wrote:
> > This patch introduces a helper to shrink unused space in the last AG
> > by fixing up the freespace btree.
> > 
> > Also make sure that the per-AG reservation works under the new AG
> > size. If such per-AG reservation or extent allocation fails, roll
> > the transaction so the new transaction could cancel without any side
> > effects.
> > 
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_ag.c | 108 +++++++++++++++++++++++++++++++++++++++++
> >  fs/xfs/libxfs/xfs_ag.h |   2 +
> >  2 files changed, 110 insertions(+)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> > index 9331f3516afa..a1128814630a 100644
> > --- a/fs/xfs/libxfs/xfs_ag.c
> > +++ b/fs/xfs/libxfs/xfs_ag.c
> > @@ -22,6 +22,11 @@
> >  #include "xfs_ag.h"
> >  #include "xfs_ag_resv.h"
> >  #include "xfs_health.h"
> > +#include "xfs_error.h"
> > +#include "xfs_bmap.h"
> > +#include "xfs_defer.h"
> > +#include "xfs_log_format.h"
> > +#include "xfs_trans.h"
> >  
> >  static int
> >  xfs_get_aghdr_buf(
> > @@ -485,6 +490,109 @@ xfs_ag_init_headers(
> >  	return error;
> >  }
> >  
> > +int
> > +xfs_ag_shrink_space(
> > +	struct xfs_mount	*mp,
> > +	struct xfs_trans	**tpp,
> > +	xfs_agnumber_t		agno,
> > +	xfs_extlen_t		len)
> > +{
> > +	struct xfs_alloc_arg	args = {
> > +		.tp	= *tpp,
> > +		.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(agno == mp->m_sb.sb_agcount - 1);
> > +	error = xfs_ialloc_read_agi(mp, *tpp, agno, &agibp);
> > +	if (error)
> > +		return error;
> > +
> > +	agi = agibp->b_addr;
> > +
> > +	error = xfs_alloc_read_agf(mp, *tpp, 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, agno,
> > +				    be32_to_cpu(agi->agi_length) - len);
> 
> Paranoia nit: Should we check that len < agi_length?

Ok, although (I think) that's impossible for the current only caller,
I could add some check in the next version.

> 
> > +
> > +	/* 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);
> 
> I forget, does xfs_alloc_vextent ever roll args.tp?

I think xfs_alloc_vextent will return a dirty transaction without
rolling instead.

Thanks,
Gao Xiang

> 
> Other than those two things this looks good to me.
> 
> --D
> 
> > +	if (!error && args.agbno == NULLAGBLOCK)
> > +		error = -ENOSPC;
> > +
> > +	if (error) {
> > +		/*
> > +		 * if extent allocation fails, need to roll the transaction to
> > +		 * ensure that the AGFL fixup has been committed anyway.
> > +		 */
> > +		err2 = xfs_trans_roll(tpp);
> > +		if (err2)
> > +			return err2;
> > +		goto resv_init_out;
> > +	}
> > +
> > +	/*
> > +	 * 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, *tpp);
> > +	if (err2) {
> > +		be32_add_cpu(&agi->agi_length, len);
> > +		be32_add_cpu(&agf->agf_length, len);
> > +		if (err2 != -ENOSPC)
> > +			goto resv_err;
> > +
> > +		__xfs_bmap_add_free(*tpp, args.fsbno, len, NULL, true);
> > +
> > +		/*
> > +		 * Roll the transaction before trying to re-init the per-ag
> > +		 * reservation. The new transaction is clean so it will cancel
> > +		 * without any side effects.
> > +		 */
> > +		error = xfs_defer_finish(tpp);
> > +		if (error)
> > +			return error;
> > +
> > +		error = -ENOSPC;
> > +		goto resv_init_out;
> > +	}
> > +	xfs_ialloc_log_agi(*tpp, agibp, XFS_AGI_LENGTH);
> > +	xfs_alloc_log_agf(*tpp, agfbp, XFS_AGF_LENGTH);
> > +	return 0;
> > +
> > +resv_init_out:
> > +	err2 = xfs_ag_resv_init(agibp->b_pag, *tpp);
> > +	if (!err2)
> > +		return error;
> > +resv_err:
> > +	xfs_warn(mp, "Error %d reserving per-AG metadata reserve pool.", err2);
> > +	xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> > +	return err2;
> > +}
> > +
> >  /*
> >   * Extent the AG indicated by the @id by the length passed in
> >   */
> > diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
> > index 5166322807e7..f33388eb130a 100644
> > --- a/fs/xfs/libxfs/xfs_ag.h
> > +++ b/fs/xfs/libxfs/xfs_ag.h
> > @@ -24,6 +24,8 @@ struct aghdr_init_data {
> >  };
> >  
> >  int xfs_ag_init_headers(struct xfs_mount *mp, struct aghdr_init_data *id);
> > +int xfs_ag_shrink_space(struct xfs_mount *mp, struct xfs_trans **tpp,
> > +			xfs_agnumber_t agno, xfs_extlen_t len);
> >  int xfs_ag_extend_space(struct xfs_mount *mp, struct xfs_trans *tp,
> >  			struct aghdr_init_data *id, xfs_extlen_t len);
> >  int xfs_ag_get_geometry(struct xfs_mount *mp, xfs_agnumber_t agno,
> > -- 
> > 2.27.0
> > 
> 

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

[Gao Xiang]

On Wed, Mar 03, 2021 at 10:25:27AM -0800, Darrick J. Wong wrote:
> On Tue, Mar 02, 2021 at 10:48:15AM +0800, Gao Xiang wrote:
> > As the first step of shrinking, this attempts to enable shrinking
> > unused space in the last allocation group by fixing up freespace
> > btree, agi, agf and adjusting super block and use a helper
> > xfs_ag_shrink_space() to fixup the last AG.
> > 
> > This can be all done in one transaction for now, so I think no
> > additional protection is needed.
> > 
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > ---
> >  fs/xfs/xfs_fsops.c | 90 ++++++++++++++++++++++++++++------------------
> >  fs/xfs/xfs_trans.c |  1 -
> >  2 files changed, 55 insertions(+), 36 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > index 494f9354e3fb..204c96d0010f 100644
> > --- a/fs/xfs/xfs_fsops.c
> > +++ b/fs/xfs/xfs_fsops.c
> > @@ -90,23 +90,29 @@ xfs_growfs_data_private(
> >  	int			error;
> >  	xfs_agnumber_t		nagcount;
> >  	xfs_agnumber_t		nagimax = 0;
> > -	xfs_rfsblock_t		nb, nb_div, nb_mod, delta;
> > +	xfs_rfsblock_t		nb, nb_div, nb_mod;
> > +	int64_t			delta;
> >  	bool			lastag_resetagres;
> >  	xfs_agnumber_t		oagcount;
> >  	struct xfs_trans	*tp;
> >  	struct aghdr_init_data	id = {};
> >  
> >  	nb = in->newblocks;
> > -	if (nb < mp->m_sb.sb_dblocks)
> > -		return -EINVAL;
> > -	if ((error = xfs_sb_validate_fsb_count(&mp->m_sb, nb)))
> > +	if (nb == mp->m_sb.sb_dblocks)
> > +		return 0;
> > +
> > +	error = xfs_sb_validate_fsb_count(&mp->m_sb, nb);
> > +	if (error)
> >  		return error;
> > -	error = xfs_buf_read_uncached(mp->m_ddev_targp,
> > +
> > +	if (nb > mp->m_sb.sb_dblocks) {
> > +		error = xfs_buf_read_uncached(mp->m_ddev_targp,
> >  				XFS_FSB_TO_BB(mp, nb) - XFS_FSS_TO_BB(mp, 1),
> >  				XFS_FSS_TO_BB(mp, 1), 0, &bp, NULL);
> > -	if (error)
> > -		return error;
> > -	xfs_buf_relse(bp);
> > +		if (error)
> > +			return error;
> > +		xfs_buf_relse(bp);
> > +	}
> >  
> >  	nb_div = nb;
> >  	nb_mod = do_div(nb_div, mp->m_sb.sb_agblocks);
> > @@ -114,10 +120,15 @@ xfs_growfs_data_private(
> >  	if (nb_mod && nb_mod < XFS_MIN_AG_BLOCKS) {
> >  		nagcount--;
> >  		nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks;
> > -		if (nb < mp->m_sb.sb_dblocks)
> > -			return -EINVAL;
> >  	}
> >  	delta = nb - mp->m_sb.sb_dblocks;
> > +	/*
> > +	 * XFS doesn't really support single-AG filesystems, so do not
> > +	 * permit callers to remove the filesystem's second and last AG.
> > +	 */
> > +	if (delta < 0 && nagcount < 2)
> > +		return -EINVAL;
> > +
> >  	oagcount = mp->m_sb.sb_agcount;
> >  
> >  	/* allocate the new per-ag structures */
> > @@ -125,15 +136,23 @@ xfs_growfs_data_private(
> >  		error = xfs_initialize_perag(mp, nagcount, &nagimax);
> >  		if (error)
> >  			return error;
> > +	} else if (nagcount < oagcount) {
> > +		/* TODO: shrinking the entire AGs hasn't yet completed */
> > +		return -EINVAL;
> >  	}
> >  
> >  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata,
> > -			XFS_GROWFS_SPACE_RES(mp), 0, XFS_TRANS_RESERVE, &tp);
> > +			(delta > 0 ? XFS_GROWFS_SPACE_RES(mp) : -delta), 0,
> > +			XFS_TRANS_RESERVE, &tp);
> >  	if (error)
> >  		return error;
> >  
> > -	error = xfs_resizefs_init_new_ags(tp, &id, oagcount, nagcount,
> > -					  delta, &lastag_resetagres);
> > +	if (delta > 0)
> > +		error = xfs_resizefs_init_new_ags(tp, &id, oagcount, nagcount,
> > +						  delta, &lastag_resetagres);
> > +	else
> > +		error = xfs_ag_shrink_space(mp, &tp, nagcount - 1, -delta);
> 
> Hm, looking back at the previous patch, I think the last argument to
> this function should be named "shrink_len" (or maybe just delta?) so
> that future readers don't confuse it for the new (shorter) AG length.
> 

Ok, will update in the next version.

> > +
> 
> Nit: no blank line needed here.
> 
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>

Thanks!

Thanks,
Gao Xiang

> 
> --D
> 
> >  	if (error)
> >  		goto out_trans_cancel;
> >  
> > @@ -144,7 +163,7 @@ xfs_growfs_data_private(
> >  	 */
> >  	if (nagcount > oagcount)
> >  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_AGCOUNT, nagcount - oagcount);
> > -	if (delta > 0)
> > +	if (delta)
> >  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS, delta);
> >  	if (id.nfree)
> >  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree);
> > @@ -168,28 +187,29 @@ xfs_growfs_data_private(
> >  	xfs_set_low_space_thresholds(mp);
> >  	mp->m_alloc_set_aside = xfs_alloc_set_aside(mp);
> >  
> > -	/*
> > -	 * If we expanded the last AG, free the per-AG reservation
> > -	 * so we can reinitialize it with the new size.
> > -	 */
> > -	if (lastag_resetagres) {
> > -		struct xfs_perag	*pag;
> > -
> > -		pag = xfs_perag_get(mp, id.agno);
> > -		error = xfs_ag_resv_free(pag);
> > -		xfs_perag_put(pag);
> > -		if (error)
> > -			return error;
> > +	if (delta > 0) {
> > +		/*
> > +		 * If we expanded the last AG, free the per-AG reservation
> > +		 * so we can reinitialize it with the new size.
> > +		 */
> > +		if (lastag_resetagres) {
> > +			struct xfs_perag	*pag;
> > +
> > +			pag = xfs_perag_get(mp, id.agno);
> > +			error = xfs_ag_resv_free(pag);
> > +			xfs_perag_put(pag);
> > +			if (error)
> > +				return error;
> > +		}
> > +		/*
> > +		 * Reserve AG metadata blocks. ENOSPC here does not mean there
> > +		 * was a growfs failure, just that there still isn't space for
> > +		 * new user data after the grow has been run.
> > +		 */
> > +		error = xfs_fs_reserve_ag_blocks(mp);
> > +		if (error == -ENOSPC)
> > +			error = 0;
> >  	}
> > -
> > -	/*
> > -	 * Reserve AG metadata blocks. ENOSPC here does not mean there was a
> > -	 * growfs failure, just that there still isn't space for new user data
> > -	 * after the grow has been run.
> > -	 */
> > -	error = xfs_fs_reserve_ag_blocks(mp);
> > -	if (error == -ENOSPC)
> > -		error = 0;
> >  	return error;
> >  
> >  out_trans_cancel:
> > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> > index 44f72c09c203..d047f5f26cc0 100644
> > --- a/fs/xfs/xfs_trans.c
> > +++ b/fs/xfs/xfs_trans.c
> > @@ -434,7 +434,6 @@ xfs_trans_mod_sb(
> >  		tp->t_res_frextents_delta += delta;
> >  		break;
> >  	case XFS_TRANS_SB_DBLOCKS:
> > -		ASSERT(delta > 0);
> >  		tp->t_dblocks_delta += delta;
> >  		break;
> >  	case XFS_TRANS_SB_AGCOUNT:
> > -- 
> > 2.27.0
> > 
>