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