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

[Gao Xiang <hsiangkao@redhat.com>]

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:

...

> > > 
> > > >  
> > > > -	/* 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