Re: [PATCH v6 6/7] xfs: support shrinking unused space in the last AG

On Thu, Feb 04, 2021 at 05:18:35PM +0800, Gao Xiang wrote:

On Wed, Feb 03, 2021 at 01:01:26PM -0500, Brian Foster wrote:

quoted

On Wed, Feb 03, 2021 at 10:51:46PM +0800, Gao Xiang wrote:

...

quoted

quoted

quoted

quoted

 
-	/* If there are new blocks in the old last AG, extend it. */
+	/* If there are some blocks in the last AG, resize it. */
 	if (delta) {

This patch added a (nb == mp->m_sb.sb_dblocks) shortcut check at the top
of the function. Should we ever get to this point with delta == 0? (If
not, maybe convert it to an assert just to be safe.)

delta would be changed after xfs_resizefs_init_new_ags() (the original
growfs design is that, I don't want to touch the original logic). that
is why `delta' reflects the last AG delta now...

Oh, I see. Hmm... that's a bit obfuscated and easy to miss. Perhaps the
new helper should also include the extend_space() call below to do all
of the AG updates in one place. It's not clear to me if we need to keep
the growfs perag reservation code where it is. If so, the new helper
could take a boolean pointer (instead of delta) that it can set to true
if it had to extend the size of the old last AG because the perag res
bits don't actually use the delta value. IOW, I think this hunk could
look something like the following:

	bool	resetagres = false;

	if (extend)
		error = xfs_resizefs_init_new_ags(..., delta, &resetagres);
	else
		error = xfs_ag_shrink_space(... -delta);
	...

	if (resetagres) {
		<do perag res fixups>
	}
	...

Hm?

Not quite sure got your point since xfs_resizefs_init_new_ags() is not
part of the transaction (and no need to). If you mean that the current
codebase needs some refactor to make the whole growfs operation as a
new helper, I could do in the next version, but one thing out there is
there are too many local variables, if we introduce some new helper,
a new struct argument might be needed.

That seems fine either way. I think it's just a matter of passing the
transaction to the function or not. I've appended a diff based on the
previous refactoring patch to demonstrate what I mean (compile tested
only).

And I have no idea why growfs perag reservation stays at the end of
the function. My own understanding is that if growfs perag reservation
here is somewhat racy since no AGI/AGF lock protection it seems.

Ok. It's probably best to leave it alone until we figure that out and
then address it in a separate patch, if desired.

Brian

--- 8< ---

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 6c4ab5e31054..707c9379d6c1 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c

@@ -34,19 +34,20 @@
  */
 static int
 xfs_resizefs_init_new_ags(
-	struct xfs_mount	*mp,
+	struct xfs_trans	*tp,
 	struct aghdr_init_data	*id,
 	xfs_agnumber_t		oagcount,
 	xfs_agnumber_t		nagcount,
-	xfs_rfsblock_t		*delta)
+	xfs_rfsblock_t		delta)
 {
-	xfs_rfsblock_t		nb = mp->m_sb.sb_dblocks + *delta;
+	struct xfs_mount	*mp = tp->t_mountp;
+	xfs_rfsblock_t		nb = mp->m_sb.sb_dblocks + delta;
 	int			error;
 
 	INIT_LIST_HEAD(&id->buffer_list);
 	for (id->agno = nagcount - 1;
 	     id->agno >= oagcount;
-	     id->agno--, *delta -= id->agsize) {
+	     id->agno--, delta -= id->agsize) {
 
 		if (id->agno == nagcount - 1)
 			id->agsize = nb - (id->agno *

@@ -60,7 +61,16 @@ xfs_resizefs_init_new_ags(
 			return error;
 		}
 	}
-	return xfs_buf_delwri_submit(&id->buffer_list);
+
+	error = xfs_buf_delwri_submit(&id->buffer_list);
+	if (error)
+		return error;
+
+	xfs_trans_agblocks_delta(tp, id->nfree);
+
+	if (delta)
+		error = xfs_ag_extend_space(mp, tp, id, delta);
+	return error;
 }
 
 /*

@@ -117,19 +127,10 @@ xfs_growfs_data_private(
 	if (error)
 		return error;
 
-	error = xfs_resizefs_init_new_ags(mp, &id, oagcount, nagcount, &delta);
+	error = xfs_resizefs_init_new_ags(tp, &id, oagcount, nagcount, delta);
 	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