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

[Gao Xiang <hsiangkao@redhat.com>]

Hi Brian,

On Thu, Feb 04, 2021 at 07:33:16AM -0500, Brian Foster wrote:
> 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:
> > > 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.
> > 
> 
> 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).

(forget to reply this email...)

Ok, will update in the next version.

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

Okay.

Thanks,
Gao Xiang

> 
> Brian