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:

quoted

On Mon, Mar 22, 2021 at 07:30:40AM -0400, Brian Foster wrote:

quoted

On Fri, Mar 05, 2021 at 10:57:02AM +0800, Gao Xiang wrote:

quoted

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 <redacted>
---
 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

...

quoted

quoted

quoted

@@ -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

quoted

Thanks,
Gao Xiang

quoted

Otherwise looks reasonable..

Brian

quoted

 	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