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
