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

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

[Gao Xiang]

Hi folks,

v6: https://lore.kernel.org/r/20210302024816.2525095-1-hsiangkao@redhat.com

This patchset attempts to support shrinking free space in the last AG.
This version mainly addresses previous review of v7. Hope I don't miss
previous comments...

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

changes since v7:
 - [3/5] rename `len' to `delta' (Darrick);
 - [3/5] add agi->length vs `delta' check (Darrick);
 - [4/5] drop an necessary blank line (Darrick).
 - Also xfs_errortag_random_default has been fixed in [5/5].

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       | 111 ++++++++++++++++++++
 fs/xfs/libxfs/xfs_ag.h       |   4 +-
 fs/xfs/libxfs/xfs_ag_resv.c  |   6 +-
 fs/xfs/libxfs/xfs_errortag.h |   4 +-
 fs/xfs/xfs_error.c           |   3 +
 fs/xfs/xfs_fsops.c           | 196 ++++++++++++++++++++++-------------
 fs/xfs/xfs_trans.c           |   1 -
 7 files changed, 247 insertions(+), 78 deletions(-)

-- 
2.27.0

[PATCH v8 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.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>
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 v8 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 | 107 +++++++++++++++++++++++++++------------------
 1 file changed, 64 insertions(+), 43 deletions(-)

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 9f9ba8bd0213..fc9e799b2ae3 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
  */
@@ -34,6 +92,7 @@ xfs_growfs_data_private(
 	xfs_agnumber_t		nagimax = 0;
 	xfs_rfsblock_t		nb, nb_div, nb_mod;
 	xfs_rfsblock_t		delta;
+	bool			lastag_resetagres;
 	xfs_agnumber_t		oagcount;
 	struct xfs_trans	*tp;
 	struct aghdr_init_data	id = {};
@@ -74,48 +133,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 +145,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 +173,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 v8 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 | 111 +++++++++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_ag.h |   4 +-
 2 files changed, 114 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
index 9331f3516afa..1f6f9e70e1cb 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,112 @@ 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		delta)
+{
+	struct xfs_alloc_arg	args = {
+		.tp	= *tpp,
+		.mp	= mp,
+		.type	= XFS_ALLOCTYPE_THIS_BNO,
+		.minlen = delta,
+		.maxlen = delta,
+		.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;
+
+	if (delta >= agi->agi_length)
+		return -EINVAL;
+
+	args.fsbno = XFS_AGB_TO_FSB(mp, agno,
+				    be32_to_cpu(agi->agi_length) - delta);
+
+	/* 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, -delta);
+	be32_add_cpu(&agf->agf_length, -delta);
+
+	err2 = xfs_ag_resv_init(agibp->b_pag, *tpp);
+	if (err2) {
+		be32_add_cpu(&agi->agi_length, delta);
+		be32_add_cpu(&agf->agf_length, delta);
+		if (err2 != -ENOSPC)
+			goto resv_err;
+
+		__xfs_bmap_add_free(*tpp, args.fsbno, delta, 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..41293ebde8da 100644
--- a/fs/xfs/libxfs/xfs_ag.h
+++ b/fs/xfs/libxfs/xfs_ag.h
@@ -24,8 +24,10 @@ 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);
+			struct aghdr_init_data *id, xfs_extlen_t delta);
 int xfs_ag_get_geometry(struct xfs_mount *mp, xfs_agnumber_t agno,
 			struct xfs_ag_geometry *ageo);
 
-- 
2.27.0

[PATCH v8 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.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 fs/xfs/xfs_fsops.c | 88 ++++++++++++++++++++++++++++------------------
 fs/xfs/xfs_trans.c |  1 -
 2 files changed, 53 insertions(+), 36 deletions(-)

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index fc9e799b2ae3..71cba61a451c 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -91,23 +91,28 @@ xfs_growfs_data_private(
 	xfs_agnumber_t		nagcount;
 	xfs_agnumber_t		nagimax = 0;
 	xfs_rfsblock_t		nb, nb_div, nb_mod;
-	xfs_rfsblock_t		delta;
+	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);
@@ -115,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 */
@@ -126,15 +136,22 @@ 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;
 
@@ -145,7 +162,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);
@@ -169,28 +186,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 v8 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           | 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 v8 5/5] xfs: add error injection for per-AG resv failure

[Darrick J. Wong]

On Fri, Mar 05, 2021 at 10:57:03AM +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           | 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,
>  };

Could you please add the BUILD_BUG_ON somewhere in xfs_error.c to make
sure that the length of xfs_errortag_random_default matches
XFS_ERRTAG_MAX?  I inquired about that in the v7 series review, but that
seems not to have been addressed?

--D

>  
>  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 v8 3/5] xfs: introduce xfs_ag_shrink_space()

[Darrick J. Wong]

On Fri, Mar 05, 2021 at 10:57:01AM +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>

Looks fine, moving on to examining the fstests...

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

--D

> ---
>  fs/xfs/libxfs/xfs_ag.c | 111 +++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/libxfs/xfs_ag.h |   4 +-
>  2 files changed, 114 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> index 9331f3516afa..1f6f9e70e1cb 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,112 @@ 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		delta)
> +{
> +	struct xfs_alloc_arg	args = {
> +		.tp	= *tpp,
> +		.mp	= mp,
> +		.type	= XFS_ALLOCTYPE_THIS_BNO,
> +		.minlen = delta,
> +		.maxlen = delta,
> +		.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;
> +
> +	if (delta >= agi->agi_length)
> +		return -EINVAL;
> +
> +	args.fsbno = XFS_AGB_TO_FSB(mp, agno,
> +				    be32_to_cpu(agi->agi_length) - delta);
> +
> +	/* 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, -delta);
> +	be32_add_cpu(&agf->agf_length, -delta);
> +
> +	err2 = xfs_ag_resv_init(agibp->b_pag, *tpp);
> +	if (err2) {
> +		be32_add_cpu(&agi->agi_length, delta);
> +		be32_add_cpu(&agf->agf_length, delta);
> +		if (err2 != -ENOSPC)
> +			goto resv_err;
> +
> +		__xfs_bmap_add_free(*tpp, args.fsbno, delta, 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..41293ebde8da 100644
> --- a/fs/xfs/libxfs/xfs_ag.h
> +++ b/fs/xfs/libxfs/xfs_ag.h
> @@ -24,8 +24,10 @@ 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);
> +			struct aghdr_init_data *id, xfs_extlen_t delta);
>  int xfs_ag_get_geometry(struct xfs_mount *mp, xfs_agnumber_t agno,
>  			struct xfs_ag_geometry *ageo);
>  
> -- 
> 2.27.0
> 

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

[Gao Xiang]

On Tue, Mar 09, 2021 at 10:05:03AM -0800, Darrick J. Wong wrote:
> On Fri, Mar 05, 2021 at 10:57:03AM +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           | 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,
> >  };
> 
> Could you please add the BUILD_BUG_ON somewhere in xfs_error.c to make
> sure that the length of xfs_errortag_random_default matches
> XFS_ERRTAG_MAX?  I inquired about that in the v7 series review, but that
> seems not to have been addressed?

Ah, sorry, I didn't get the point. I've sent out a seperate patch
to address this, please kindly check:
https://lore.kernel.org/r/20210309184205.18675-1-hsiangkao@aol.com

Thanks,
Gao Xiang

> 
> --D
> 
> >  
> >  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 v8 1/5] xfs: update lazy sb counters immediately for resizefs

[Brian Foster]

On Fri, Mar 05, 2021 at 10:56:59AM +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.
> 
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@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
> 

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

[Brian Foster]

On Fri, Mar 05, 2021 at 10:57:00AM +0800, Gao Xiang wrote:
> 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 | 107 +++++++++++++++++++++++++++------------------
>  1 file changed, 64 insertions(+), 43 deletions(-)
> 
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index 9f9ba8bd0213..fc9e799b2ae3 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)

Nit: I'd just call this lastag_extended or something that otherwise
indicates what this function reports (as opposed to trying to tell the
caller what to do).

> +{
> +	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
>   */
...
> @@ -123,9 +145,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);

Hm.. isn't delta still unsigned as of this patch?

Brian

>  	if (id.nfree)
>  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree);
>  
> @@ -152,7 +173,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
> 

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

[Brian Foster]

On Fri, Mar 05, 2021 at 10:57:01AM +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>
> ---

Looks mostly good to me. Some nits..

>  fs/xfs/libxfs/xfs_ag.c | 111 +++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/libxfs/xfs_ag.h |   4 +-
>  2 files changed, 114 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> index 9331f3516afa..1f6f9e70e1cb 100644
> --- a/fs/xfs/libxfs/xfs_ag.c
> +++ b/fs/xfs/libxfs/xfs_ag.c
...
> @@ -485,6 +490,112 @@ 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		delta)
> +{
> +	struct xfs_alloc_arg	args = {
> +		.tp	= *tpp,
> +		.mp	= mp,
> +		.type	= XFS_ALLOCTYPE_THIS_BNO,
> +		.minlen = delta,
> +		.maxlen = delta,
> +		.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;

Is this check here for a reason? It seems a bit random, so I wonder if
we should just leave the extra verification to buffer verifiers.

> +
> +	if (delta >= agi->agi_length)
> +		return -EINVAL;
> +
> +	args.fsbno = XFS_AGB_TO_FSB(mp, agno,
> +				    be32_to_cpu(agi->agi_length) - delta);
> +
> +	/* remove the preallocations before allocation and re-establish then */

The comment is a little confusing. Perhaps something like the following,
if accurate..?

/*
 * Disable perag reservations so it doesn't cause the allocation request
 * to fail. We'll reestablish reservation before we return.
 */

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

So if this fails and the transaction rolls, do we still hold the agi/agf
buffers here? If not, there might be a window of time where it's
possible for some other task to come in and alloc out of the AG without
the perag res being active.

> +	}
> +
> +	/*
> +	 * if successfully deleted from freespace btrees, need to confirm
> +	 * per-AG reservation works as expected.
> +	 */
> +	be32_add_cpu(&agi->agi_length, -delta);
> +	be32_add_cpu(&agf->agf_length, -delta);
> +
> +	err2 = xfs_ag_resv_init(agibp->b_pag, *tpp);
> +	if (err2) {
> +		be32_add_cpu(&agi->agi_length, delta);
> +		be32_add_cpu(&agf->agf_length, delta);
> +		if (err2 != -ENOSPC)
> +			goto resv_err;
> +
> +		__xfs_bmap_add_free(*tpp, args.fsbno, delta, 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..41293ebde8da 100644
> --- a/fs/xfs/libxfs/xfs_ag.h
> +++ b/fs/xfs/libxfs/xfs_ag.h
> @@ -24,8 +24,10 @@ 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);
> +			struct aghdr_init_data *id, xfs_extlen_t delta);

This looks misplaced..?

Or maybe this is trying to make the APIs consistent, but the function
definition still uses len as well as the declaration for
_ag_shrink_space() (while the definition of that function uses delta).

FWIW, the name delta tends to suggest a signed value to me based on our
pattern of usage, whereas here it seems like these helpers always want a
positive value (i.e. a length).

Brian

>  int xfs_ag_get_geometry(struct xfs_mount *mp, xfs_agnumber_t agno,
>  			struct xfs_ag_geometry *ageo);
>  
> -- 
> 2.27.0
> 

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

[Brian Foster]

On Fri, Mar 05, 2021 at 10:57:02AM +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.
> 
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
>  fs/xfs/xfs_fsops.c | 88 ++++++++++++++++++++++++++++------------------
>  fs/xfs/xfs_trans.c |  1 -
>  2 files changed, 53 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index fc9e799b2ae3..71cba61a451c 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -91,23 +91,28 @@ xfs_growfs_data_private(
>  	xfs_agnumber_t		nagcount;
>  	xfs_agnumber_t		nagimax = 0;
>  	xfs_rfsblock_t		nb, nb_div, nb_mod;
> -	xfs_rfsblock_t		delta;
> +	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;

It looks like the caller already does this check.

> +
> +	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);
> @@ -115,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;
> +

What if the filesystem is already single AG? Unless I'm missing
something, we already have a check a bit further down that prevents
removal of AGs in the first place.

Otherwise looks reasonable..

Brian

>  	oagcount = mp->m_sb.sb_agcount;
>  
>  	/* allocate the new per-ag structures */
> @@ -126,15 +136,22 @@ 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;
>  
> @@ -145,7 +162,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);
> @@ -169,28 +186,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 v8 2/5] xfs: hoist out xfs_resizefs_init_new_ags()

[Gao Xiang]

On Mon, Mar 22, 2021 at 07:28:07AM -0400, Brian Foster wrote:
> On Fri, Mar 05, 2021 at 10:57:00AM +0800, Gao Xiang wrote:
> > 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 | 107 +++++++++++++++++++++++++++------------------
> >  1 file changed, 64 insertions(+), 43 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > index 9f9ba8bd0213..fc9e799b2ae3 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)
> 
> Nit: I'd just call this lastag_extended or something that otherwise
> indicates what this function reports (as opposed to trying to tell the
> caller what to do).

ok, if other people don't oppose of it.

> 
> > +{
> > +	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
> >   */
> ...
> > @@ -123,9 +145,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);
> 
> Hm.. isn't delta still unsigned as of this patch?

Not sure if some difference exists, I could update it as "if (delta)",
therefore it seems [PATCH v8 4/5] won't modify this anymore.

Thanks,
Gao Xiang

> 
> Brian
> 
> >  	if (id.nfree)
> >  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree);
> >  
> > @@ -152,7 +173,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
> > 
> 

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

[Gao Xiang]

Hi Brian,

On Mon, Mar 22, 2021 at 07:30:03AM -0400, Brian Foster wrote:
> On Fri, Mar 05, 2021 at 10:57:01AM +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>
> > ---
> 
> Looks mostly good to me. Some nits..
> 
> >  fs/xfs/libxfs/xfs_ag.c | 111 +++++++++++++++++++++++++++++++++++++++++
> >  fs/xfs/libxfs/xfs_ag.h |   4 +-
> >  2 files changed, 114 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> > index 9331f3516afa..1f6f9e70e1cb 100644
> > --- a/fs/xfs/libxfs/xfs_ag.c
> > +++ b/fs/xfs/libxfs/xfs_ag.c
> ...
> > @@ -485,6 +490,112 @@ 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		delta)
> > +{
> > +	struct xfs_alloc_arg	args = {
> > +		.tp	= *tpp,
> > +		.mp	= mp,
> > +		.type	= XFS_ALLOCTYPE_THIS_BNO,
> > +		.minlen = delta,
> > +		.maxlen = delta,
> > +		.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;
> 
> Is this check here for a reason? It seems a bit random, so I wonder if
> we should just leave the extra verification to buffer verifiers.

It came from Darrick's thought. I'm fine with either way, but I feel
confused if different conflict opinions here:
https://lore.kernel.org/linux-xfs/20210303181931.GB3419940@magnolia/

> 
> > +
> > +	if (delta >= agi->agi_length)
> > +		return -EINVAL;
> > +
> > +	args.fsbno = XFS_AGB_TO_FSB(mp, agno,
> > +				    be32_to_cpu(agi->agi_length) - delta);
> > +
> > +	/* remove the preallocations before allocation and re-establish then */
> 
> The comment is a little confusing. Perhaps something like the following,
> if accurate..?
> 
> /*
>  * Disable perag reservations so it doesn't cause the allocation request
>  * to fail. We'll reestablish reservation before we return.
>  */

ok, will update in the next version.

> 
> > +	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;
> 
> So if this fails and the transaction rolls, do we still hold the agi/agf
> buffers here? If not, there might be a window of time where it's
> possible for some other task to come in and alloc out of the AG without
> the perag res being active.

Yeah, I might need to bhold this, will update.

> 
> > +	}
> > +
> > +	/*
> > +	 * if successfully deleted from freespace btrees, need to confirm
> > +	 * per-AG reservation works as expected.
> > +	 */
> > +	be32_add_cpu(&agi->agi_length, -delta);
> > +	be32_add_cpu(&agf->agf_length, -delta);
> > +
> > +	err2 = xfs_ag_resv_init(agibp->b_pag, *tpp);
> > +	if (err2) {
> > +		be32_add_cpu(&agi->agi_length, delta);
> > +		be32_add_cpu(&agf->agf_length, delta);
> > +		if (err2 != -ENOSPC)
> > +			goto resv_err;
> > +
> > +		__xfs_bmap_add_free(*tpp, args.fsbno, delta, 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..41293ebde8da 100644
> > --- a/fs/xfs/libxfs/xfs_ag.h
> > +++ b/fs/xfs/libxfs/xfs_ag.h
> > @@ -24,8 +24,10 @@ 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);
> > +			struct aghdr_init_data *id, xfs_extlen_t delta);
> 
> This looks misplaced..?
> 
> Or maybe this is trying to make the APIs consistent, but the function
> definition still uses len as well as the declaration for
> _ag_shrink_space() (while the definition of that function uses delta).
> 
> FWIW, the name delta tends to suggest a signed value to me based on our
> pattern of usage, whereas here it seems like these helpers always want a
> positive value (i.e. a length).

Yeah, it's just misplaced, thanks for pointing out, sorry about that.
`delta' name came from, `len' is confusing to Darrick.
https://lore.kernel.org/r/20210303182527.GC3419940@magnolia/

Thanks,
Gao Xiang

> 
> Brian
> 
> >  int xfs_ag_get_geometry(struct xfs_mount *mp, xfs_agnumber_t agno,
> >  			struct xfs_ag_geometry *ageo);
> >  
> > -- 
> > 2.27.0
> > 
> 

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

[Gao Xiang]

On Mon, Mar 22, 2021 at 07:30:40AM -0400, Brian Foster wrote:
> On Fri, Mar 05, 2021 at 10:57:02AM +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.
> > 
> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > ---
> >  fs/xfs/xfs_fsops.c | 88 ++++++++++++++++++++++++++++------------------
> >  fs/xfs/xfs_trans.c |  1 -
> >  2 files changed, 53 insertions(+), 36 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > index fc9e799b2ae3..71cba61a451c 100644
> > --- a/fs/xfs/xfs_fsops.c
> > +++ b/fs/xfs/xfs_fsops.c
> > @@ -91,23 +91,28 @@ xfs_growfs_data_private(
> >  	xfs_agnumber_t		nagcount;
> >  	xfs_agnumber_t		nagimax = 0;
> >  	xfs_rfsblock_t		nb, nb_div, nb_mod;
> > -	xfs_rfsblock_t		delta;
> > +	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;
> 
> It looks like the caller already does this check.

yeah, will remove it. Thanks for pointing out.

> 
> > +
> > +	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);
> > @@ -115,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;
> > +
> 
> What if the filesystem is already single AG? Unless I'm missing
> something, we already have a check a bit further down that prevents
> removal of AGs in the first place.

I think it tends to forbid (return -EINVAL) shrinking the filesystem with
a single AG only? Am I missing something?

Thanks,
Gao Xiang

> 
> Otherwise looks reasonable..
> 
> Brian
> 
> >  	oagcount = mp->m_sb.sb_agcount;
> >  
> >  	/* allocate the new per-ag structures */
> > @@ -126,15 +136,22 @@ 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;
> >  
> > @@ -145,7 +162,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);
> > @@ -169,28 +186,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 v8 2/5] xfs: hoist out xfs_resizefs_init_new_ags()

[Brian Foster]

On Mon, Mar 22, 2021 at 07:53:49PM +0800, Gao Xiang wrote:
> On Mon, Mar 22, 2021 at 07:28:07AM -0400, Brian Foster wrote:
> > On Fri, Mar 05, 2021 at 10:57:00AM +0800, Gao Xiang wrote:
> > > 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 | 107 +++++++++++++++++++++++++++------------------
> > >  1 file changed, 64 insertions(+), 43 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > > index 9f9ba8bd0213..fc9e799b2ae3 100644
> > > --- a/fs/xfs/xfs_fsops.c
> > > +++ b/fs/xfs/xfs_fsops.c
...
> > > @@ -123,9 +145,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);
> > 
> > Hm.. isn't delta still unsigned as of this patch?
> 
> Not sure if some difference exists, I could update it as "if (delta)",
> therefore it seems [PATCH v8 4/5] won't modify this anymore.
> 

Yeah, not sure it's a problem, it just stands out a bit with it being in
a separate patch from the change to the variable type.

Brian

> Thanks,
> Gao Xiang
> 
> > 
> > Brian
> > 
> > >  	if (id.nfree)
> > >  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree);
> > >  
> > > @@ -152,7 +173,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
> > > 
> > 
> 

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

[Brian Foster]

On Mon, Mar 22, 2021 at 08:03:10PM +0800, Gao Xiang wrote:
> Hi Brian,
> 
> On Mon, Mar 22, 2021 at 07:30:03AM -0400, Brian Foster wrote:
> > On Fri, Mar 05, 2021 at 10:57:01AM +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>
> > > ---
> > 
> > Looks mostly good to me. Some nits..
> > 
> > >  fs/xfs/libxfs/xfs_ag.c | 111 +++++++++++++++++++++++++++++++++++++++++
> > >  fs/xfs/libxfs/xfs_ag.h |   4 +-
> > >  2 files changed, 114 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> > > index 9331f3516afa..1f6f9e70e1cb 100644
> > > --- a/fs/xfs/libxfs/xfs_ag.c
> > > +++ b/fs/xfs/libxfs/xfs_ag.c
> > ...
> > > @@ -485,6 +490,112 @@ 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		delta)
> > > +{
> > > +	struct xfs_alloc_arg	args = {
> > > +		.tp	= *tpp,
> > > +		.mp	= mp,
> > > +		.type	= XFS_ALLOCTYPE_THIS_BNO,
> > > +		.minlen = delta,
> > > +		.maxlen = delta,
> > > +		.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;
> > 
> > Is this check here for a reason? It seems a bit random, so I wonder if
> > we should just leave the extra verification to buffer verifiers.
> 
> It came from Darrick's thought. I'm fine with either way, but I feel
> confused if different conflict opinions here:
> https://lore.kernel.org/linux-xfs/20210303181931.GB3419940@magnolia/
> 

Darrick's comment seems to refer to the check below. I'm referring to
the check above that agi_length and agf_length match. Are they intended
to go together? The check above seems to preexist the one below.

Anyways, if so, maybe just bunch them together and add a comment:

	/* some extra paranoid checks before we shrink the ag */
	if (XFS_IS_CORRUPT(...))
		return -EFSCORRUPTED;
	if (delta >= agf->agf_length)
		return -EVINAL; 

> > 
> > > +
> > > +	if (delta >= agi->agi_length)
> > > +		return -EINVAL;
> > > +
> > > +	args.fsbno = XFS_AGB_TO_FSB(mp, agno,
> > > +				    be32_to_cpu(agi->agi_length) - delta);
> > > +
> > > +	/* remove the preallocations before allocation and re-establish then */
...
> > > diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
> > > index 5166322807e7..41293ebde8da 100644
> > > --- a/fs/xfs/libxfs/xfs_ag.h
> > > +++ b/fs/xfs/libxfs/xfs_ag.h
> > > @@ -24,8 +24,10 @@ 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);
> > > +			struct aghdr_init_data *id, xfs_extlen_t delta);
> > 
> > This looks misplaced..?
> > 
> > Or maybe this is trying to make the APIs consistent, but the function
> > definition still uses len as well as the declaration for
> > _ag_shrink_space() (while the definition of that function uses delta).
> > 
> > FWIW, the name delta tends to suggest a signed value to me based on our
> > pattern of usage, whereas here it seems like these helpers always want a
> > positive value (i.e. a length).
> 
> Yeah, it's just misplaced, thanks for pointing out, sorry about that.
> `delta' name came from, `len' is confusing to Darrick.
> https://lore.kernel.org/r/20210303182527.GC3419940@magnolia/
> 

Fair enough. I'm not worried about the name, just pointing out some
potential inconsistencies.

Brian

> Thanks,
> Gao Xiang
> 
> > 
> > Brian
> > 
> > >  int xfs_ag_get_geometry(struct xfs_mount *mp, xfs_agnumber_t agno,
> > >  			struct xfs_ag_geometry *ageo);
> > >  
> > > -- 
> > > 2.27.0
> > > 
> > 
> 

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

[Brian Foster]

On Mon, Mar 22, 2021 at 08:07:22PM +0800, Gao Xiang wrote:
> On Mon, Mar 22, 2021 at 07:30:40AM -0400, Brian Foster wrote:
> > On Fri, Mar 05, 2021 at 10:57:02AM +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.
> > > 
> > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > > ---
> > >  fs/xfs/xfs_fsops.c | 88 ++++++++++++++++++++++++++++------------------
> > >  fs/xfs/xfs_trans.c |  1 -
> > >  2 files changed, 53 insertions(+), 36 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > > index fc9e799b2ae3..71cba61a451c 100644
> > > --- a/fs/xfs/xfs_fsops.c
> > > +++ b/fs/xfs/xfs_fsops.c
...
> > > @@ -115,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;
> > > +
> > 
> > What if the filesystem is already single AG? Unless I'm missing
> > something, we already have a check a bit further down that prevents
> > removal of AGs in the first place.
> 
> I think it tends to forbid (return -EINVAL) shrinking the filesystem with
> a single AG only? Am I missing something?
> 

My assumption was this check means one can't shrink a filesystem that is
already agcount == 1. The comment refers to preventing shrink from
causing an agcount == 1 fs. What is the intent?

Brian

> Thanks,
> Gao Xiang
> 
> > 
> > Otherwise looks reasonable..
> > 
> > Brian
> > 
> > >  	oagcount = mp->m_sb.sb_agcount;
> > >  
> > >  	/* allocate the new per-ag structures */
> > > @@ -126,15 +136,22 @@ 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;
> > >  
> > > @@ -145,7 +162,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);
> > > @@ -169,28 +186,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 v8 3/5] xfs: introduce xfs_ag_shrink_space()

[Gao Xiang]

On Mon, Mar 22, 2021 at 08:25:55AM -0400, Brian Foster wrote:
> On Mon, Mar 22, 2021 at 08:03:10PM +0800, Gao Xiang wrote:
> > Hi Brian,
> > 
> > On Mon, Mar 22, 2021 at 07:30:03AM -0400, Brian Foster wrote:
> > > On Fri, Mar 05, 2021 at 10:57:01AM +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>
> > > > ---
> > > 
> > > Looks mostly good to me. Some nits..
> > > 
> > > >  fs/xfs/libxfs/xfs_ag.c | 111 +++++++++++++++++++++++++++++++++++++++++
> > > >  fs/xfs/libxfs/xfs_ag.h |   4 +-
> > > >  2 files changed, 114 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> > > > index 9331f3516afa..1f6f9e70e1cb 100644
> > > > --- a/fs/xfs/libxfs/xfs_ag.c
> > > > +++ b/fs/xfs/libxfs/xfs_ag.c
> > > ...
> > > > @@ -485,6 +490,112 @@ 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		delta)
> > > > +{
> > > > +	struct xfs_alloc_arg	args = {
> > > > +		.tp	= *tpp,
> > > > +		.mp	= mp,
> > > > +		.type	= XFS_ALLOCTYPE_THIS_BNO,
> > > > +		.minlen = delta,
> > > > +		.maxlen = delta,
> > > > +		.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;
> > > 
> > > Is this check here for a reason? It seems a bit random, so I wonder if
> > > we should just leave the extra verification to buffer verifiers.
> > 
> > It came from Darrick's thought. I'm fine with either way, but I feel
> > confused if different conflict opinions here:
> > https://lore.kernel.org/linux-xfs/20210303181931.GB3419940@magnolia/
> > 
> 
> Darrick's comment seems to refer to the check below. I'm referring to
> the check above that agi_length and agf_length match. Are they intended
> to go together? The check above seems to preexist the one below.

Sorry, update link of this:
https://lore.kernel.org/r/20210111181753.GC1164246@magnolia/

> 
> Anyways, if so, maybe just bunch them together and add a comment:
> 
> 	/* some extra paranoid checks before we shrink the ag */
> 	if (XFS_IS_CORRUPT(...))
> 		return -EFSCORRUPTED;
> 	if (delta >= agf->agf_length)
> 		return -EVINAL; 
> 

ok, will update this.

> > > 
> > > > +
> > > > +	if (delta >= agi->agi_length)
> > > > +		return -EINVAL;
> > > > +
> > > > +	args.fsbno = XFS_AGB_TO_FSB(mp, agno,
> > > > +				    be32_to_cpu(agi->agi_length) - delta);
> > > > +
> > > > +	/* remove the preallocations before allocation and re-establish then */
> ...
> > > > diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
> > > > index 5166322807e7..41293ebde8da 100644
> > > > --- a/fs/xfs/libxfs/xfs_ag.h
> > > > +++ b/fs/xfs/libxfs/xfs_ag.h
> > > > @@ -24,8 +24,10 @@ 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);
> > > > +			struct aghdr_init_data *id, xfs_extlen_t delta);
> > > 
> > > This looks misplaced..?
> > > 
> > > Or maybe this is trying to make the APIs consistent, but the function
> > > definition still uses len as well as the declaration for
> > > _ag_shrink_space() (while the definition of that function uses delta).
> > > 
> > > FWIW, the name delta tends to suggest a signed value to me based on our
> > > pattern of usage, whereas here it seems like these helpers always want a
> > > positive value (i.e. a length).
> > 
> > Yeah, it's just misplaced, thanks for pointing out, sorry about that.
> > `delta' name came from, `len' is confusing to Darrick.
> > https://lore.kernel.org/r/20210303182527.GC3419940@magnolia/
> > 
> 
> Fair enough. I'm not worried about the name, just pointing out some
> potential inconsistencies.

Thanks for pointing out!

Thanks,
Gao Xiang

> 
> Brian
> 
> > Thanks,
> > Gao Xiang
> > 
> > > 
> > > Brian
> > > 
> > > >  int xfs_ag_get_geometry(struct xfs_mount *mp, xfs_agnumber_t agno,
> > > >  			struct xfs_ag_geometry *ageo);
> > > >  
> > > > -- 
> > > > 2.27.0
> > > > 
> > > 
> > 
> 

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

[Gao Xiang]

On Mon, Mar 22, 2021 at 08:26:41AM -0400, Brian Foster wrote:
> On Mon, Mar 22, 2021 at 08:07:22PM +0800, Gao Xiang wrote:
> > On Mon, Mar 22, 2021 at 07:30:40AM -0400, Brian Foster wrote:
> > > On Fri, Mar 05, 2021 at 10:57:02AM +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.
> > > > 
> > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > > > ---
> > > >  fs/xfs/xfs_fsops.c | 88 ++++++++++++++++++++++++++++------------------
> > > >  fs/xfs/xfs_trans.c |  1 -
> > > >  2 files changed, 53 insertions(+), 36 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > > > index fc9e799b2ae3..71cba61a451c 100644
> > > > --- a/fs/xfs/xfs_fsops.c
> > > > +++ b/fs/xfs/xfs_fsops.c
> ...
> > > > @@ -115,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;
> > > > +
> > > 
> > > What if the filesystem is already single AG? Unless I'm missing
> > > something, we already have a check a bit further down that prevents
> > > removal of AGs in the first place.
> > 
> > I think it tends to forbid (return -EINVAL) shrinking the filesystem with
> > a single AG only? Am I missing something?
> > 
> 
> My assumption was this check means one can't shrink a filesystem that is
> already agcount == 1. The comment refers to preventing shrink from
> causing an agcount == 1 fs. What is the intent?

I think it means the latter -- preventing shrink from causing an agcount == 1
fs. since nagcount (new agcount) <= 1?

Actually, I'm not good at English, if comments need to be improved, please
kindly point out. Thank you very much!

Thanks,
Gao Xiang

> 
> Brian
> 
> > Thanks,
> > Gao Xiang
> > 
> > > 
> > > Otherwise looks reasonable..
> > > 
> > > Brian
> > > 
> > > >  	oagcount = mp->m_sb.sb_agcount;
> > > >  
> > > >  	/* allocate the new per-ag structures */
> > > > @@ -126,15 +136,22 @@ 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;
> > > >  
> > > > @@ -145,7 +162,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);
> > > > @@ -169,28 +186,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 v8 4/5] xfs: support shrinking unused space in the last AG

[Brian Foster]

On Mon, Mar 22, 2021 at 08:36:52PM +0800, Gao Xiang wrote:
> On Mon, Mar 22, 2021 at 08:26:41AM -0400, Brian Foster wrote:
> > On Mon, Mar 22, 2021 at 08:07:22PM +0800, Gao Xiang wrote:
> > > On Mon, Mar 22, 2021 at 07:30:40AM -0400, Brian Foster wrote:
> > > > On Fri, Mar 05, 2021 at 10:57:02AM +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.
> > > > > 
> > > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > > > > ---
> > > > >  fs/xfs/xfs_fsops.c | 88 ++++++++++++++++++++++++++++------------------
> > > > >  fs/xfs/xfs_trans.c |  1 -
> > > > >  2 files changed, 53 insertions(+), 36 deletions(-)
> > > > > 
> > > > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > > > > index fc9e799b2ae3..71cba61a451c 100644
> > > > > --- a/fs/xfs/xfs_fsops.c
> > > > > +++ b/fs/xfs/xfs_fsops.c
> > ...
> > > > > @@ -115,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;
> > > > > +
> > > > 
> > > > What if the filesystem is already single AG? Unless I'm missing
> > > > something, we already have a check a bit further down that prevents
> > > > removal of AGs in the first place.
> > > 
> > > I think it tends to forbid (return -EINVAL) shrinking the filesystem with
> > > a single AG only? Am I missing something?
> > > 
> > 
> > My assumption was this check means one can't shrink a filesystem that is
> > already agcount == 1. The comment refers to preventing shrink from
> > causing an agcount == 1 fs. What is the intent?
> 
> I think it means the latter -- preventing shrink from causing an agcount == 1
> fs. since nagcount (new agcount) <= 1?
> 

Right, so that leads to my question... does this check also fail a
shrink on an fs that is already agcount == 1? If so, why? I know
technically it's not a supported configuration, but mkfs allows it.

Brian

> Actually, I'm not good at English, if comments need to be improved, please
> kindly point out. Thank you very much!
> 
> Thanks,
> Gao Xiang
> 
> > 
> > Brian
> > 
> > > Thanks,
> > > Gao Xiang
> > > 
> > > > 
> > > > Otherwise looks reasonable..
> > > > 
> > > > Brian
> > > > 
> > > > >  	oagcount = mp->m_sb.sb_agcount;
> > > > >  
> > > > >  	/* allocate the new per-ag structures */
> > > > > @@ -126,15 +136,22 @@ 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;
> > > > >  
> > > > > @@ -145,7 +162,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);
> > > > > @@ -169,28 +186,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 v8 4/5] xfs: support shrinking unused space in the last AG

[Gao Xiang]

On Mon, Mar 22, 2021 at 08:43:19AM -0400, Brian Foster wrote:
> On Mon, Mar 22, 2021 at 08:36:52PM +0800, Gao Xiang wrote:
> > On Mon, Mar 22, 2021 at 08:26:41AM -0400, Brian Foster wrote:
> > > On Mon, Mar 22, 2021 at 08:07:22PM +0800, Gao Xiang wrote:
> > > > On Mon, Mar 22, 2021 at 07:30:40AM -0400, Brian Foster wrote:
> > > > > On Fri, Mar 05, 2021 at 10:57:02AM +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.
> > > > > > 
> > > > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > > > > > ---
> > > > > >  fs/xfs/xfs_fsops.c | 88 ++++++++++++++++++++++++++++------------------
> > > > > >  fs/xfs/xfs_trans.c |  1 -
> > > > > >  2 files changed, 53 insertions(+), 36 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > > > > > index fc9e799b2ae3..71cba61a451c 100644
> > > > > > --- a/fs/xfs/xfs_fsops.c
> > > > > > +++ b/fs/xfs/xfs_fsops.c
> > > ...
> > > > > > @@ -115,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;
> > > > > > +
> > > > > 
> > > > > What if the filesystem is already single AG? Unless I'm missing
> > > > > something, we already have a check a bit further down that prevents
> > > > > removal of AGs in the first place.
> > > > 
> > > > I think it tends to forbid (return -EINVAL) shrinking the filesystem with
> > > > a single AG only? Am I missing something?
> > > > 
> > > 
> > > My assumption was this check means one can't shrink a filesystem that is
> > > already agcount == 1. The comment refers to preventing shrink from
> > > causing an agcount == 1 fs. What is the intent?
> > 
> > I think it means the latter -- preventing shrink from causing an agcount == 1
> > fs. since nagcount (new agcount) <= 1?
> > 
> 
> Right, so that leads to my question... does this check also fail a
> shrink on an fs that is already agcount == 1? If so, why? I know
> technically it's not a supported configuration, but mkfs allows it.

Ah, I'm not sure if Darrick would like to forbid agcount == 1 shrinking
functionitity completely, see the previous comment:
https://lore.kernel.org/r/20201014160633.GD9832@magnolia/

(please ignore the modification at that time, since it was buggy...)

Thanks,
Gao Xiang

> 
> Brian
> 
> > Actually, I'm not good at English, if comments need to be improved, please
> > kindly point out. Thank you very much!
> > 
> > Thanks,
> > Gao Xiang
> > 
> > > 
> > > Brian
> > > 
> > > > Thanks,
> > > > Gao Xiang
> > > > 
> > > > > 
> > > > > Otherwise looks reasonable..
> > > > > 
> > > > > Brian
> > > > > 
> > > > > >  	oagcount = mp->m_sb.sb_agcount;
> > > > > >  
> > > > > >  	/* allocate the new per-ag structures */
> > > > > > @@ -126,15 +136,22 @@ 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;
> > > > > >  
> > > > > > @@ -145,7 +162,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);
> > > > > > @@ -169,28 +186,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 v8 3/5] xfs: introduce xfs_ag_shrink_space()

[Darrick J. Wong]

On Mon, Mar 22, 2021 at 08:33:28PM +0800, Gao Xiang wrote:
> On Mon, Mar 22, 2021 at 08:25:55AM -0400, Brian Foster wrote:
> > On Mon, Mar 22, 2021 at 08:03:10PM +0800, Gao Xiang wrote:
> > > Hi Brian,
> > > 
> > > On Mon, Mar 22, 2021 at 07:30:03AM -0400, Brian Foster wrote:
> > > > On Fri, Mar 05, 2021 at 10:57:01AM +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>
> > > > > ---
> > > > 
> > > > Looks mostly good to me. Some nits..
> > > > 
> > > > >  fs/xfs/libxfs/xfs_ag.c | 111 +++++++++++++++++++++++++++++++++++++++++
> > > > >  fs/xfs/libxfs/xfs_ag.h |   4 +-
> > > > >  2 files changed, 114 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> > > > > index 9331f3516afa..1f6f9e70e1cb 100644
> > > > > --- a/fs/xfs/libxfs/xfs_ag.c
> > > > > +++ b/fs/xfs/libxfs/xfs_ag.c
> > > > ...
> > > > > @@ -485,6 +490,112 @@ 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		delta)
> > > > > +{
> > > > > +	struct xfs_alloc_arg	args = {
> > > > > +		.tp	= *tpp,
> > > > > +		.mp	= mp,
> > > > > +		.type	= XFS_ALLOCTYPE_THIS_BNO,
> > > > > +		.minlen = delta,
> > > > > +		.maxlen = delta,
> > > > > +		.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;
> > > > 
> > > > Is this check here for a reason? It seems a bit random, so I wonder if
> > > > we should just leave the extra verification to buffer verifiers.
> > > 
> > > It came from Darrick's thought. I'm fine with either way, but I feel
> > > confused if different conflict opinions here:
> > > https://lore.kernel.org/linux-xfs/20210303181931.GB3419940@magnolia/
> > > 
> > 
> > Darrick's comment seems to refer to the check below. I'm referring to
> > the check above that agi_length and agf_length match. Are they intended
> > to go together? The check above seems to preexist the one below.
> 
> Sorry, update link of this:
> https://lore.kernel.org/r/20210111181753.GC1164246@magnolia/
> 
> > 
> > Anyways, if so, maybe just bunch them together and add a comment:
> > 
> > 	/* some extra paranoid checks before we shrink the ag */

Yes, all this is born out of paranoia checks on my part because I feel
that shrink is likely to have Real Bad Dataloss Consequences(tm) if we
don't check everything a second time before proceeding.

--D

> > 	if (XFS_IS_CORRUPT(...))
> > 		return -EFSCORRUPTED;
> > 	if (delta >= agf->agf_length)
> > 		return -EVINAL; 
> > 
> 
> ok, will update this.
> 
> > > > 
> > > > > +
> > > > > +	if (delta >= agi->agi_length)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	args.fsbno = XFS_AGB_TO_FSB(mp, agno,
> > > > > +				    be32_to_cpu(agi->agi_length) - delta);
> > > > > +
> > > > > +	/* remove the preallocations before allocation and re-establish then */
> > ...
> > > > > diff --git a/fs/xfs/libxfs/xfs_ag.h b/fs/xfs/libxfs/xfs_ag.h
> > > > > index 5166322807e7..41293ebde8da 100644
> > > > > --- a/fs/xfs/libxfs/xfs_ag.h
> > > > > +++ b/fs/xfs/libxfs/xfs_ag.h
> > > > > @@ -24,8 +24,10 @@ 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);
> > > > > +			struct aghdr_init_data *id, xfs_extlen_t delta);
> > > > 
> > > > This looks misplaced..?
> > > > 
> > > > Or maybe this is trying to make the APIs consistent, but the function
> > > > definition still uses len as well as the declaration for
> > > > _ag_shrink_space() (while the definition of that function uses delta).
> > > > 
> > > > FWIW, the name delta tends to suggest a signed value to me based on our
> > > > pattern of usage, whereas here it seems like these helpers always want a
> > > > positive value (i.e. a length).
> > > 
> > > Yeah, it's just misplaced, thanks for pointing out, sorry about that.
> > > `delta' name came from, `len' is confusing to Darrick.
> > > https://lore.kernel.org/r/20210303182527.GC3419940@magnolia/
> > > 
> > 
> > Fair enough. I'm not worried about the name, just pointing out some
> > potential inconsistencies.
> 
> Thanks for pointing out!
> 
> Thanks,
> Gao Xiang
> 
> > 
> > Brian
> > 
> > > Thanks,
> > > Gao Xiang
> > > 
> > > > 
> > > > Brian
> > > > 
> > > > >  int xfs_ag_get_geometry(struct xfs_mount *mp, xfs_agnumber_t agno,
> > > > >  			struct xfs_ag_geometry *ageo);
> > > > >  
> > > > > -- 
> > > > > 2.27.0
> > > > > 
> > > > 
> > > 
> > 
> 

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

[Darrick J. Wong]

On Mon, Mar 22, 2021 at 08:50:28PM +0800, Gao Xiang wrote:
> On Mon, Mar 22, 2021 at 08:43:19AM -0400, Brian Foster wrote:
> > On Mon, Mar 22, 2021 at 08:36:52PM +0800, Gao Xiang wrote:
> > > On Mon, Mar 22, 2021 at 08:26:41AM -0400, Brian Foster wrote:
> > > > On Mon, Mar 22, 2021 at 08:07:22PM +0800, Gao Xiang wrote:
> > > > > On Mon, Mar 22, 2021 at 07:30:40AM -0400, Brian Foster wrote:
> > > > > > On Fri, Mar 05, 2021 at 10:57:02AM +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.
> > > > > > > 
> > > > > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > > > > > > ---
> > > > > > >  fs/xfs/xfs_fsops.c | 88 ++++++++++++++++++++++++++++------------------
> > > > > > >  fs/xfs/xfs_trans.c |  1 -
> > > > > > >  2 files changed, 53 insertions(+), 36 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > > > > > > index fc9e799b2ae3..71cba61a451c 100644
> > > > > > > --- a/fs/xfs/xfs_fsops.c
> > > > > > > +++ b/fs/xfs/xfs_fsops.c
> > > > ...
> > > > > > > @@ -115,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;
> > > > > > > +
> > > > > > 
> > > > > > What if the filesystem is already single AG? Unless I'm missing
> > > > > > something, we already have a check a bit further down that prevents
> > > > > > removal of AGs in the first place.
> > > > > 
> > > > > I think it tends to forbid (return -EINVAL) shrinking the filesystem with
> > > > > a single AG only? Am I missing something?
> > > > > 
> > > > 
> > > > My assumption was this check means one can't shrink a filesystem that is
> > > > already agcount == 1. The comment refers to preventing shrink from
> > > > causing an agcount == 1 fs. What is the intent?

Both of those things.

> > > 
> > > I think it means the latter -- preventing shrink from causing an agcount == 1
> > > fs. since nagcount (new agcount) <= 1?
> > > 
> > 
> > Right, so that leads to my question... does this check also fail a
> > shrink on an fs that is already agcount == 1? If so, why? I know
> > technically it's not a supported configuration, but mkfs allows it.
> 
> Ah, I'm not sure if Darrick would like to forbid agcount == 1 shrinking
> functionitity completely, see the previous comment:
> https://lore.kernel.org/r/20201014160633.GD9832@magnolia/
> 
> (please ignore the modification at that time, since it was buggy...)

Given the confusion I propose a new comment:

	/*
	 * Reject filesystems with a single AG because they are not
	 * supported, and reject a shrink operation that would cause a
	 * filesystem to become unsupported.
	 */
	if (delta < 0 && nagcount < 2)
		return -EINVAL;

--D

> 
> Thanks,
> Gao Xiang
> 
> > 
> > Brian
> > 
> > > Actually, I'm not good at English, if comments need to be improved, please
> > > kindly point out. Thank you very much!
> > > 
> > > Thanks,
> > > Gao Xiang
> > > 
> > > > 
> > > > Brian
> > > > 
> > > > > Thanks,
> > > > > Gao Xiang
> > > > > 
> > > > > > 
> > > > > > Otherwise looks reasonable..
> > > > > > 
> > > > > > Brian
> > > > > > 
> > > > > > >  	oagcount = mp->m_sb.sb_agcount;
> > > > > > >  
> > > > > > >  	/* allocate the new per-ag structures */
> > > > > > > @@ -126,15 +136,22 @@ 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;
> > > > > > >  
> > > > > > > @@ -145,7 +162,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);
> > > > > > > @@ -169,28 +186,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 v8 4/5] xfs: support shrinking unused space in the last AG

[Gao Xiang]

On Mon, Mar 22, 2021 at 09:42:55AM -0700, Darrick J. Wong wrote:
> On Mon, Mar 22, 2021 at 08:50:28PM +0800, Gao Xiang wrote:
> > On Mon, Mar 22, 2021 at 08:43:19AM -0400, Brian Foster wrote:
> > > On Mon, Mar 22, 2021 at 08:36:52PM +0800, Gao Xiang wrote:
> > > > On Mon, Mar 22, 2021 at 08:26:41AM -0400, Brian Foster wrote:
> > > > > On Mon, Mar 22, 2021 at 08:07:22PM +0800, Gao Xiang wrote:
> > > > > > On Mon, Mar 22, 2021 at 07:30:40AM -0400, Brian Foster wrote:
> > > > > > > On Fri, Mar 05, 2021 at 10:57:02AM +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.
> > > > > > > > 
> > > > > > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > > > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > > > > > > > ---
> > > > > > > >  fs/xfs/xfs_fsops.c | 88 ++++++++++++++++++++++++++++------------------
> > > > > > > >  fs/xfs/xfs_trans.c |  1 -
> > > > > > > >  2 files changed, 53 insertions(+), 36 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > > > > > > > index fc9e799b2ae3..71cba61a451c 100644
> > > > > > > > --- a/fs/xfs/xfs_fsops.c
> > > > > > > > +++ b/fs/xfs/xfs_fsops.c
> > > > > ...
> > > > > > > > @@ -115,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;
> > > > > > > > +
> > > > > > > 
> > > > > > > What if the filesystem is already single AG? Unless I'm missing
> > > > > > > something, we already have a check a bit further down that prevents
> > > > > > > removal of AGs in the first place.
> > > > > > 
> > > > > > I think it tends to forbid (return -EINVAL) shrinking the filesystem with
> > > > > > a single AG only? Am I missing something?
> > > > > > 
> > > > > 
> > > > > My assumption was this check means one can't shrink a filesystem that is
> > > > > already agcount == 1. The comment refers to preventing shrink from
> > > > > causing an agcount == 1 fs. What is the intent?
> 
> Both of those things.
> 
> > > > 
> > > > I think it means the latter -- preventing shrink from causing an agcount == 1
> > > > fs. since nagcount (new agcount) <= 1?
> > > > 
> > > 
> > > Right, so that leads to my question... does this check also fail a
> > > shrink on an fs that is already agcount == 1? If so, why? I know
> > > technically it's not a supported configuration, but mkfs allows it.
> > 
> > Ah, I'm not sure if Darrick would like to forbid agcount == 1 shrinking
> > functionitity completely, see the previous comment:
> > https://lore.kernel.org/r/20201014160633.GD9832@magnolia/
> > 
> > (please ignore the modification at that time, since it was buggy...)
> 
> Given the confusion I propose a new comment:
> 
> 	/*
> 	 * Reject filesystems with a single AG because they are not
> 	 * supported, and reject a shrink operation that would cause a
> 	 * filesystem to become unsupported.
> 	 */
> 	if (delta < 0 && nagcount < 2)
> 		return -EINVAL;
> 

ok, will update this comment. thanks for your suggestion!

Thanks,
Gao Xiang

> --D