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

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

...

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.

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.

Thanks,
Gao Xiang

Brian