| Message ID | 20210126125621.3846735-7-hsiangkao@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: support shrinking free space in the last AG | expand |
On Tue, Jan 26, 2021 at 08:56:20PM +0800, Gao Xiang wrote: > As the first step of shrinking, this attempts to enable shrinking > unused space in the last allocation group by fixing up freespace > btree, agi, agf and adjusting super block and use a helper > xfs_ag_shrink_space() to fixup the last AG. > > This can be all done in one transaction for now, so I think no > additional protection is needed. > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > --- > fs/xfs/xfs_fsops.c | 64 ++++++++++++++++++++++++++++++---------------- > fs/xfs/xfs_trans.c | 1 - > 2 files changed, 42 insertions(+), 23 deletions(-) > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c > index 6c4ab5e31054..4bcea22f7b3f 100644 > --- a/fs/xfs/xfs_fsops.c > +++ b/fs/xfs/xfs_fsops.c > @@ -38,7 +38,7 @@ xfs_resizefs_init_new_ags( > struct aghdr_init_data *id, > xfs_agnumber_t oagcount, > xfs_agnumber_t nagcount, > - xfs_rfsblock_t *delta) > + int64_t *delta) > { > xfs_rfsblock_t nb = mp->m_sb.sb_dblocks + *delta; > int error; > @@ -76,33 +76,41 @@ xfs_growfs_data_private( > xfs_agnumber_t nagcount; > xfs_agnumber_t nagimax = 0; > xfs_rfsblock_t nb, nb_div, nb_mod; > - xfs_rfsblock_t delta; > + int64_t delta; > xfs_agnumber_t oagcount; > struct xfs_trans *tp; > + bool extend; > struct aghdr_init_data id = {}; > > nb = in->newblocks; > - if (nb < mp->m_sb.sb_dblocks) > - return -EINVAL; > - if ((error = xfs_sb_validate_fsb_count(&mp->m_sb, nb))) > + if (nb == mp->m_sb.sb_dblocks) > + return 0; > + > + error = xfs_sb_validate_fsb_count(&mp->m_sb, nb); > + if (error) > return error; > - error = xfs_buf_read_uncached(mp->m_ddev_targp, > + > + if (nb > mp->m_sb.sb_dblocks) { > + error = xfs_buf_read_uncached(mp->m_ddev_targp, > XFS_FSB_TO_BB(mp, nb) - XFS_FSS_TO_BB(mp, 1), > XFS_FSS_TO_BB(mp, 1), 0, &bp, NULL); > - if (error) > - return error; > - xfs_buf_relse(bp); > + if (error) > + return error; > + xfs_buf_relse(bp); > + } > > nb_div = nb; > nb_mod = do_div(nb_div, mp->m_sb.sb_agblocks); > nagcount = nb_div + (nb_mod != 0); > if (nb_mod && nb_mod < XFS_MIN_AG_BLOCKS) { > nagcount--; > - nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks; > - if (nb < mp->m_sb.sb_dblocks) > + if (nagcount < 2) > return -EINVAL; What's the reason for the nagcount < 2 check? IIRC we warn about this configuration at mkfs time, but allow it to proceed. Is it just that we don't want to accidentally put the fs into an agcount == 1 state that was originally formatted with >1 AGs? What about the case where we attempt to grow an agcount == 1 fs but don't enlarge enough to add the second AG? Does this change error behavior in that case? > + nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks; > } > + > delta = nb - mp->m_sb.sb_dblocks; > + extend = (delta > 0); > oagcount = mp->m_sb.sb_agcount; > > /* allocate the new per-ag structures */ > @@ -110,22 +118,34 @@ xfs_growfs_data_private( > error = xfs_initialize_perag(mp, nagcount, &nagimax); > if (error) > return error; > + } else if (nagcount < oagcount) { > + /* TODO: shrinking the entire AGs hasn't yet completed */ > + return -EINVAL; > } > > error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata, > - XFS_GROWFS_SPACE_RES(mp), 0, XFS_TRANS_RESERVE, &tp); > + (extend ? XFS_GROWFS_SPACE_RES(mp) : -delta), 0, > + XFS_TRANS_RESERVE, &tp); > if (error) > return error; > > - error = xfs_resizefs_init_new_ags(mp, &id, oagcount, nagcount, &delta); > - if (error) > - goto out_trans_cancel; > - > + if (extend) { > + error = xfs_resizefs_init_new_ags(mp, &id, oagcount, > + nagcount, &delta); > + if (error) > + goto out_trans_cancel; > + } > xfs_trans_agblocks_delta(tp, id.nfree); It looks like id isn't used until the resize call above. Is this call relevant for the shrink case? > > - /* 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.) > - error = xfs_ag_extend_space(mp, tp, &id, delta); > + if (extend) { > + error = xfs_ag_extend_space(mp, tp, &id, delta); > + } else { > + id.agno = nagcount - 1; > + error = xfs_ag_shrink_space(mp, &tp, &id, -delta); xfs_ag_shrink_space() looks like it only accesses id->agno. Perhaps just pass in agno for now..? > + } > + > if (error) > goto out_trans_cancel; > } > @@ -137,15 +157,15 @@ xfs_growfs_data_private( > */ > if (nagcount > oagcount) > xfs_trans_mod_sb(tp, XFS_TRANS_SB_AGCOUNT, nagcount - oagcount); > - if (nb > mp->m_sb.sb_dblocks) > + if (nb != mp->m_sb.sb_dblocks) > xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS, > nb - mp->m_sb.sb_dblocks); Maybe use delta here? > if (id.nfree) > xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree); > id.nfree tracks newly added free space in the growfs space. Is it not used in the shrink case because the allocation handles this for us? > /* > - * update in-core counters now to reflect the real numbers > - * (especially sb_fdblocks) > + * update in-core counters now to reflect the real numbers (especially > + * sb_fdblocks). And xfs_validate_sb_write() can pass for shrinkfs. > */ > if (xfs_sb_version_haslazysbcount(&mp->m_sb)) > xfs_log_sb(tp); > @@ -165,7 +185,7 @@ xfs_growfs_data_private( > * If we expanded the last AG, free the per-AG reservation > * so we can reinitialize it with the new size. > */ > - if (delta) { > + if (extend && delta) { > struct xfs_perag *pag; > > pag = xfs_perag_get(mp, id.agno); We call xfs_fs_reserve_ag_blocks() a bit further down before we exit this function. xfs_ag_shrink_space() from the previous patch is intended to deal with perag reservation changes for shrink, but it looks like the reserve call further down could potentially reset mp->m_finobt_nores to false if it previously might have been set to true. Brian > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c > index e72730f85af1..fd2cbf414b80 100644 > --- a/fs/xfs/xfs_trans.c > +++ b/fs/xfs/xfs_trans.c > @@ -419,7 +419,6 @@ xfs_trans_mod_sb( > tp->t_res_frextents_delta += delta; > break; > case XFS_TRANS_SB_DBLOCKS: > - ASSERT(delta > 0); > tp->t_dblocks_delta += delta; > break; > case XFS_TRANS_SB_AGCOUNT: > -- > 2.27.0 >
Hi Brian, On Wed, Feb 03, 2021 at 09:23:37AM -0500, Brian Foster wrote: > On Tue, Jan 26, 2021 at 08:56:20PM +0800, Gao Xiang wrote: > > As the first step of shrinking, this attempts to enable shrinking > > unused space in the last allocation group by fixing up freespace > > btree, agi, agf and adjusting super block and use a helper > > xfs_ag_shrink_space() to fixup the last AG. > > > > This can be all done in one transaction for now, so I think no > > additional protection is needed. > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > > --- > > fs/xfs/xfs_fsops.c | 64 ++++++++++++++++++++++++++++++---------------- > > fs/xfs/xfs_trans.c | 1 - > > 2 files changed, 42 insertions(+), 23 deletions(-) > > > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c > > index 6c4ab5e31054..4bcea22f7b3f 100644 > > --- a/fs/xfs/xfs_fsops.c > > +++ b/fs/xfs/xfs_fsops.c > > @@ -38,7 +38,7 @@ xfs_resizefs_init_new_ags( > > struct aghdr_init_data *id, > > xfs_agnumber_t oagcount, > > xfs_agnumber_t nagcount, > > - xfs_rfsblock_t *delta) > > + int64_t *delta) > > { > > xfs_rfsblock_t nb = mp->m_sb.sb_dblocks + *delta; > > int error; > > @@ -76,33 +76,41 @@ xfs_growfs_data_private( > > xfs_agnumber_t nagcount; > > xfs_agnumber_t nagimax = 0; > > xfs_rfsblock_t nb, nb_div, nb_mod; > > - xfs_rfsblock_t delta; > > + int64_t delta; > > xfs_agnumber_t oagcount; > > struct xfs_trans *tp; > > + bool extend; > > struct aghdr_init_data id = {}; > > > > nb = in->newblocks; > > - if (nb < mp->m_sb.sb_dblocks) > > - return -EINVAL; > > - if ((error = xfs_sb_validate_fsb_count(&mp->m_sb, nb))) > > + if (nb == mp->m_sb.sb_dblocks) > > + return 0; > > + > > + error = xfs_sb_validate_fsb_count(&mp->m_sb, nb); > > + if (error) > > return error; > > - error = xfs_buf_read_uncached(mp->m_ddev_targp, > > + > > + if (nb > mp->m_sb.sb_dblocks) { > > + error = xfs_buf_read_uncached(mp->m_ddev_targp, > > XFS_FSB_TO_BB(mp, nb) - XFS_FSS_TO_BB(mp, 1), > > XFS_FSS_TO_BB(mp, 1), 0, &bp, NULL); > > - if (error) > > - return error; > > - xfs_buf_relse(bp); > > + if (error) > > + return error; > > + xfs_buf_relse(bp); > > + } > > > > nb_div = nb; > > nb_mod = do_div(nb_div, mp->m_sb.sb_agblocks); > > nagcount = nb_div + (nb_mod != 0); > > if (nb_mod && nb_mod < XFS_MIN_AG_BLOCKS) { > > nagcount--; > > - nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks; > > - if (nb < mp->m_sb.sb_dblocks) > > + if (nagcount < 2) > > return -EINVAL; > > What's the reason for the nagcount < 2 check? IIRC we warn about this > configuration at mkfs time, but allow it to proceed. Is it just that we > don't want to accidentally put the fs into an agcount == 1 state that > was originally formatted with >1 AGs? Darrick once asked for avoiding shrinking the filesystem which has only 1 AG. > > What about the case where we attempt to grow an agcount == 1 fs but > don't enlarge enough to add the second AG? Does this change error > behavior in that case? Yeah, thanks for catching this! If growfs allows 1 AG case before, I think it needs to be refined. Let me update this in the next version! > > > + nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks; > > } > > + > > delta = nb - mp->m_sb.sb_dblocks; > > + extend = (delta > 0); > > oagcount = mp->m_sb.sb_agcount; > > > > /* allocate the new per-ag structures */ > > @@ -110,22 +118,34 @@ xfs_growfs_data_private( > > error = xfs_initialize_perag(mp, nagcount, &nagimax); > > if (error) > > return error; > > + } else if (nagcount < oagcount) { > > + /* TODO: shrinking the entire AGs hasn't yet completed */ > > + return -EINVAL; > > } > > > > error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata, > > - XFS_GROWFS_SPACE_RES(mp), 0, XFS_TRANS_RESERVE, &tp); > > + (extend ? XFS_GROWFS_SPACE_RES(mp) : -delta), 0, > > + XFS_TRANS_RESERVE, &tp); > > if (error) > > return error; > > > > - error = xfs_resizefs_init_new_ags(mp, &id, oagcount, nagcount, &delta); > > - if (error) > > - goto out_trans_cancel; > > - > > + if (extend) { > > + error = xfs_resizefs_init_new_ags(mp, &id, oagcount, > > + nagcount, &delta); > > + if (error) > > + goto out_trans_cancel; > > + } > > xfs_trans_agblocks_delta(tp, id.nfree); > > It looks like id isn't used until the resize call above. Is this call > relevant for the shrink case? I think it has nothing to do for the shrink the last AG case as well (id.nfree == 0 here) but maybe use for the later shrinking the whole AGs patchset. I can move into if (extend) in the next version. > > > > > - /* 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... > > > - error = xfs_ag_extend_space(mp, tp, &id, delta); > > + if (extend) { > > + error = xfs_ag_extend_space(mp, tp, &id, delta); > > + } else { > > + id.agno = nagcount - 1; > > + error = xfs_ag_shrink_space(mp, &tp, &id, -delta); > > xfs_ag_shrink_space() looks like it only accesses id->agno. Perhaps just > pass in agno for now..? Both way are ok, yet in my incomplete shrink whole empty AGs patchset, it seems more natural to pass in &id rather than agno (since id.agno = nagcount - 1 will be stayed in some new helper e.g. xfs_shrink_ags()) > > > + } > > + > > if (error) > > goto out_trans_cancel; > > } > > @@ -137,15 +157,15 @@ xfs_growfs_data_private( > > */ > > if (nagcount > oagcount) > > xfs_trans_mod_sb(tp, XFS_TRANS_SB_AGCOUNT, nagcount - oagcount); > > - if (nb > mp->m_sb.sb_dblocks) > > + if (nb != mp->m_sb.sb_dblocks) > > xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS, > > nb - mp->m_sb.sb_dblocks); > > Maybe use delta here? The reason is the same as above, `delta' here was changed due to xfs_resizefs_init_new_ags(), which is not nb - mp->m_sb.sb_dblocks anymore. so `extend` boolean is used (rather than just use delta > 0) > > > if (id.nfree) > > xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree); > > > > id.nfree tracks newly added free space in the growfs space. Is it not > used in the shrink case because the allocation handles this for us? Yeah, I'm afraid so. This is some common code, and also used in my shrinking the whole AGs patchset. > > > /* > > - * update in-core counters now to reflect the real numbers > > - * (especially sb_fdblocks) > > + * update in-core counters now to reflect the real numbers (especially > > + * sb_fdblocks). And xfs_validate_sb_write() can pass for shrinkfs. > > */ > > if (xfs_sb_version_haslazysbcount(&mp->m_sb)) > > xfs_log_sb(tp); > > @@ -165,7 +185,7 @@ xfs_growfs_data_private( > > * If we expanded the last AG, free the per-AG reservation > > * so we can reinitialize it with the new size. > > */ > > - if (delta) { > > + if (extend && delta) { > > struct xfs_perag *pag; > > > > pag = xfs_perag_get(mp, id.agno); > > We call xfs_fs_reserve_ag_blocks() a bit further down before we exit > this function. xfs_ag_shrink_space() from the previous patch is intended > to deal with perag reservation changes for shrink, but it looks like the > reserve call further down could potentially reset mp->m_finobt_nores to > false if it previously might have been set to true. Yeah, if my understanding is correct, I might need to call xfs_fs_reserve_ag_blocks() only for growfs case as well for mp->m_finobt_nores = true case. Thanks, Gao Xiang > > Brian > > > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c > > index e72730f85af1..fd2cbf414b80 100644 > > --- a/fs/xfs/xfs_trans.c > > +++ b/fs/xfs/xfs_trans.c > > @@ -419,7 +419,6 @@ xfs_trans_mod_sb( > > tp->t_res_frextents_delta += delta; > > break; > > case XFS_TRANS_SB_DBLOCKS: > > - ASSERT(delta > 0); > > tp->t_dblocks_delta += delta; > > break; > > case XFS_TRANS_SB_AGCOUNT: > > -- > > 2.27.0 > > >
On Wed, Feb 03, 2021 at 10:51:46PM +0800, Gao Xiang wrote: > Hi Brian, > > On Wed, Feb 03, 2021 at 09:23:37AM -0500, Brian Foster wrote: > > On Tue, Jan 26, 2021 at 08:56:20PM +0800, Gao Xiang wrote: > > > As the first step of shrinking, this attempts to enable shrinking > > > unused space in the last allocation group by fixing up freespace > > > btree, agi, agf and adjusting super block and use a helper > > > xfs_ag_shrink_space() to fixup the last AG. > > > > > > This can be all done in one transaction for now, so I think no > > > additional protection is needed. > > > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > > > --- > > > fs/xfs/xfs_fsops.c | 64 ++++++++++++++++++++++++++++++---------------- > > > fs/xfs/xfs_trans.c | 1 - > > > 2 files changed, 42 insertions(+), 23 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c > > > index 6c4ab5e31054..4bcea22f7b3f 100644 > > > --- a/fs/xfs/xfs_fsops.c > > > +++ b/fs/xfs/xfs_fsops.c > > > @@ -38,7 +38,7 @@ xfs_resizefs_init_new_ags( > > > struct aghdr_init_data *id, > > > xfs_agnumber_t oagcount, > > > xfs_agnumber_t nagcount, > > > - xfs_rfsblock_t *delta) > > > + int64_t *delta) > > > { > > > xfs_rfsblock_t nb = mp->m_sb.sb_dblocks + *delta; > > > int error; > > > @@ -76,33 +76,41 @@ xfs_growfs_data_private( > > > xfs_agnumber_t nagcount; > > > xfs_agnumber_t nagimax = 0; > > > xfs_rfsblock_t nb, nb_div, nb_mod; > > > - xfs_rfsblock_t delta; > > > + int64_t delta; > > > xfs_agnumber_t oagcount; > > > struct xfs_trans *tp; > > > + bool extend; > > > struct aghdr_init_data id = {}; > > > > > > nb = in->newblocks; > > > - if (nb < mp->m_sb.sb_dblocks) > > > - return -EINVAL; > > > - if ((error = xfs_sb_validate_fsb_count(&mp->m_sb, nb))) > > > + if (nb == mp->m_sb.sb_dblocks) > > > + return 0; > > > + > > > + error = xfs_sb_validate_fsb_count(&mp->m_sb, nb); > > > + if (error) > > > return error; > > > - error = xfs_buf_read_uncached(mp->m_ddev_targp, > > > + > > > + if (nb > mp->m_sb.sb_dblocks) { > > > + error = xfs_buf_read_uncached(mp->m_ddev_targp, > > > XFS_FSB_TO_BB(mp, nb) - XFS_FSS_TO_BB(mp, 1), > > > XFS_FSS_TO_BB(mp, 1), 0, &bp, NULL); > > > - if (error) > > > - return error; > > > - xfs_buf_relse(bp); > > > + if (error) > > > + return error; > > > + xfs_buf_relse(bp); > > > + } > > > > > > nb_div = nb; > > > nb_mod = do_div(nb_div, mp->m_sb.sb_agblocks); > > > nagcount = nb_div + (nb_mod != 0); > > > if (nb_mod && nb_mod < XFS_MIN_AG_BLOCKS) { > > > nagcount--; > > > - nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks; > > > - if (nb < mp->m_sb.sb_dblocks) > > > + if (nagcount < 2) > > > return -EINVAL; > > > > What's the reason for the nagcount < 2 check? IIRC we warn about this > > configuration at mkfs time, but allow it to proceed. Is it just that we > > don't want to accidentally put the fs into an agcount == 1 state that > > was originally formatted with >1 AGs? > > Darrick once asked for avoiding shrinking the filesystem which has > only 1 AG. > > > > > What about the case where we attempt to grow an agcount == 1 fs but > > don't enlarge enough to add the second AG? Does this change error > > behavior in that case? > > Yeah, thanks for catching this! If growfs allows 1 AG case before, > I think it needs to be refined. Let me update this in the next version! > > > > > > + nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks; > > > } > > > + > > > delta = nb - mp->m_sb.sb_dblocks; > > > + extend = (delta > 0); > > > oagcount = mp->m_sb.sb_agcount; > > > > > > /* allocate the new per-ag structures */ > > > @@ -110,22 +118,34 @@ xfs_growfs_data_private( > > > error = xfs_initialize_perag(mp, nagcount, &nagimax); > > > if (error) > > > return error; > > > + } else if (nagcount < oagcount) { > > > + /* TODO: shrinking the entire AGs hasn't yet completed */ > > > + return -EINVAL; > > > } > > > > > > error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata, > > > - XFS_GROWFS_SPACE_RES(mp), 0, XFS_TRANS_RESERVE, &tp); > > > + (extend ? XFS_GROWFS_SPACE_RES(mp) : -delta), 0, > > > + XFS_TRANS_RESERVE, &tp); > > > if (error) > > > return error; > > > > > > - error = xfs_resizefs_init_new_ags(mp, &id, oagcount, nagcount, &delta); > > > - if (error) > > > - goto out_trans_cancel; > > > - > > > + if (extend) { > > > + error = xfs_resizefs_init_new_ags(mp, &id, oagcount, > > > + nagcount, &delta); > > > + if (error) > > > + goto out_trans_cancel; > > > + } > > > xfs_trans_agblocks_delta(tp, id.nfree); > > > > It looks like id isn't used until the resize call above. Is this call > > relevant for the shrink case? > > I think it has nothing to do for the shrink the last AG case as well > (id.nfree == 0 here) but maybe use for the later shrinking the whole > AGs patchset. I can move into if (extend) in the next version. > > > > > > > > > - /* 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? Brian > > > > > - error = xfs_ag_extend_space(mp, tp, &id, delta); > > > + if (extend) { > > > + error = xfs_ag_extend_space(mp, tp, &id, delta); > > > + } else { > > > + id.agno = nagcount - 1; > > > + error = xfs_ag_shrink_space(mp, &tp, &id, -delta); > > > > xfs_ag_shrink_space() looks like it only accesses id->agno. Perhaps just > > pass in agno for now..? > > Both way are ok, yet in my incomplete shrink whole empty AGs patchset, > it seems more natural to pass in &id rather than agno (since > id.agno = nagcount - 1 will be stayed in some new helper > e.g. xfs_shrink_ags()) > > > > > > + } > > > + > > > if (error) > > > goto out_trans_cancel; > > > } > > > @@ -137,15 +157,15 @@ xfs_growfs_data_private( > > > */ > > > if (nagcount > oagcount) > > > xfs_trans_mod_sb(tp, XFS_TRANS_SB_AGCOUNT, nagcount - oagcount); > > > - if (nb > mp->m_sb.sb_dblocks) > > > + if (nb != mp->m_sb.sb_dblocks) > > > xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS, > > > nb - mp->m_sb.sb_dblocks); > > > > Maybe use delta here? > > The reason is the same as above, `delta' here was changed due to > xfs_resizefs_init_new_ags(), which is not nb - mp->m_sb.sb_dblocks > anymore. so `extend` boolean is used (rather than just use delta > 0) > > > > > > if (id.nfree) > > > xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree); > > > > > > > id.nfree tracks newly added free space in the growfs space. Is it not > > used in the shrink case because the allocation handles this for us? > > Yeah, I'm afraid so. This is some common code, and also used in my > shrinking the whole AGs patchset. > > > > > > /* > > > - * update in-core counters now to reflect the real numbers > > > - * (especially sb_fdblocks) > > > + * update in-core counters now to reflect the real numbers (especially > > > + * sb_fdblocks). And xfs_validate_sb_write() can pass for shrinkfs. > > > */ > > > if (xfs_sb_version_haslazysbcount(&mp->m_sb)) > > > xfs_log_sb(tp); > > > @@ -165,7 +185,7 @@ xfs_growfs_data_private( > > > * If we expanded the last AG, free the per-AG reservation > > > * so we can reinitialize it with the new size. > > > */ > > > - if (delta) { > > > + if (extend && delta) { > > > struct xfs_perag *pag; > > > > > > pag = xfs_perag_get(mp, id.agno); > > > > We call xfs_fs_reserve_ag_blocks() a bit further down before we exit > > this function. xfs_ag_shrink_space() from the previous patch is intended > > to deal with perag reservation changes for shrink, but it looks like the > > reserve call further down could potentially reset mp->m_finobt_nores to > > false if it previously might have been set to true. > > Yeah, if my understanding is correct, I might need to call > xfs_fs_reserve_ag_blocks() only for growfs case as well for > mp->m_finobt_nores = true case. > > Thanks, > Gao Xiang > > > > > Brian > > > > > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c > > > index e72730f85af1..fd2cbf414b80 100644 > > > --- a/fs/xfs/xfs_trans.c > > > +++ b/fs/xfs/xfs_trans.c > > > @@ -419,7 +419,6 @@ xfs_trans_mod_sb( > > > tp->t_res_frextents_delta += delta; > > > break; > > > case XFS_TRANS_SB_DBLOCKS: > > > - ASSERT(delta > 0); > > > tp->t_dblocks_delta += delta; > > > break; > > > case XFS_TRANS_SB_AGCOUNT: > > > -- > > > 2.27.0 > > > > > >
On Wed, Feb 03, 2021 at 10:51:46PM +0800, Gao Xiang wrote: > Hi Brian, > > On Wed, Feb 03, 2021 at 09:23:37AM -0500, Brian Foster wrote: > > On Tue, Jan 26, 2021 at 08:56:20PM +0800, Gao Xiang wrote: > > > As the first step of shrinking, this attempts to enable shrinking > > > unused space in the last allocation group by fixing up freespace > > > btree, agi, agf and adjusting super block and use a helper > > > xfs_ag_shrink_space() to fixup the last AG. > > > > > > This can be all done in one transaction for now, so I think no > > > additional protection is needed. > > > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > > > --- > > > fs/xfs/xfs_fsops.c | 64 ++++++++++++++++++++++++++++++---------------- > > > fs/xfs/xfs_trans.c | 1 - > > > 2 files changed, 42 insertions(+), 23 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c > > > index 6c4ab5e31054..4bcea22f7b3f 100644 > > > --- a/fs/xfs/xfs_fsops.c > > > +++ b/fs/xfs/xfs_fsops.c > > > @@ -38,7 +38,7 @@ xfs_resizefs_init_new_ags( > > > struct aghdr_init_data *id, > > > xfs_agnumber_t oagcount, > > > xfs_agnumber_t nagcount, > > > - xfs_rfsblock_t *delta) > > > + int64_t *delta) > > > { > > > xfs_rfsblock_t nb = mp->m_sb.sb_dblocks + *delta; > > > int error; > > > @@ -76,33 +76,41 @@ xfs_growfs_data_private( > > > xfs_agnumber_t nagcount; > > > xfs_agnumber_t nagimax = 0; > > > xfs_rfsblock_t nb, nb_div, nb_mod; > > > - xfs_rfsblock_t delta; > > > + int64_t delta; > > > xfs_agnumber_t oagcount; > > > struct xfs_trans *tp; > > > + bool extend; > > > struct aghdr_init_data id = {}; > > > > > > nb = in->newblocks; > > > - if (nb < mp->m_sb.sb_dblocks) > > > - return -EINVAL; > > > - if ((error = xfs_sb_validate_fsb_count(&mp->m_sb, nb))) > > > + if (nb == mp->m_sb.sb_dblocks) > > > + return 0; > > > + > > > + error = xfs_sb_validate_fsb_count(&mp->m_sb, nb); > > > + if (error) > > > return error; > > > - error = xfs_buf_read_uncached(mp->m_ddev_targp, > > > + > > > + if (nb > mp->m_sb.sb_dblocks) { > > > + error = xfs_buf_read_uncached(mp->m_ddev_targp, > > > XFS_FSB_TO_BB(mp, nb) - XFS_FSS_TO_BB(mp, 1), > > > XFS_FSS_TO_BB(mp, 1), 0, &bp, NULL); > > > - if (error) > > > - return error; > > > - xfs_buf_relse(bp); > > > + if (error) > > > + return error; > > > + xfs_buf_relse(bp); > > > + } > > > > > > nb_div = nb; > > > nb_mod = do_div(nb_div, mp->m_sb.sb_agblocks); > > > nagcount = nb_div + (nb_mod != 0); > > > if (nb_mod && nb_mod < XFS_MIN_AG_BLOCKS) { > > > nagcount--; > > > - nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks; > > > - if (nb < mp->m_sb.sb_dblocks) > > > + if (nagcount < 2) > > > return -EINVAL; > > > > What's the reason for the nagcount < 2 check? IIRC we warn about this > > configuration at mkfs time, but allow it to proceed. Is it just that we > > don't want to accidentally put the fs into an agcount == 1 state that > > was originally formatted with >1 AGs? > > Darrick once asked for avoiding shrinking the filesystem which has > only 1 AG. It's worth mentioning why in a comment though: /* * XFS doesn't really support single-AG filesystems, so do not * permit callers to remove the filesystem's second and last AG. */ if (shrink && new_agcount < 2) return -EHAHANOYOUDONT; But as Brian points out, we /do/ allow adding a second AG to a single-AG fs. > > > > What about the case where we attempt to grow an agcount == 1 fs but > > don't enlarge enough to add the second AG? Does this change error > > behavior in that case? > > Yeah, thanks for catching this! If growfs allows 1 AG case before, > I think it needs to be refined. Let me update this in the next version! > > > > > > + nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks; > > > } > > > + > > > delta = nb - mp->m_sb.sb_dblocks; > > > + extend = (delta > 0); > > > oagcount = mp->m_sb.sb_agcount; > > > > > > /* allocate the new per-ag structures */ > > > @@ -110,22 +118,34 @@ xfs_growfs_data_private( > > > error = xfs_initialize_perag(mp, nagcount, &nagimax); > > > if (error) > > > return error; > > > + } else if (nagcount < oagcount) { > > > + /* TODO: shrinking the entire AGs hasn't yet completed */ > > > + return -EINVAL; > > > } > > > > > > error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata, > > > - XFS_GROWFS_SPACE_RES(mp), 0, XFS_TRANS_RESERVE, &tp); > > > + (extend ? XFS_GROWFS_SPACE_RES(mp) : -delta), 0, > > > + XFS_TRANS_RESERVE, &tp); > > > if (error) > > > return error; > > > > > > - error = xfs_resizefs_init_new_ags(mp, &id, oagcount, nagcount, &delta); > > > - if (error) > > > - goto out_trans_cancel; > > > - > > > + if (extend) { > > > + error = xfs_resizefs_init_new_ags(mp, &id, oagcount, > > > + nagcount, &delta); > > > + if (error) > > > + goto out_trans_cancel; > > > + } > > > xfs_trans_agblocks_delta(tp, id.nfree); > > > > It looks like id isn't used until the resize call above. Is this call > > relevant for the shrink case? > > I think it has nothing to do for the shrink the last AG case as well > (id.nfree == 0 here) but maybe use for the later shrinking the whole > AGs patchset. I can move into if (extend) in the next version. > > > > > > > > > - /* 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... I've never liked how the meaning of "delta" changes through the function, and it clearly trips up reviewers. This variable isn't the delta between the old dblocks and the new dblocks, it's really a resizefs cursor that tells us how much work we still have to do. > > > > > - error = xfs_ag_extend_space(mp, tp, &id, delta); > > > + if (extend) { > > > + error = xfs_ag_extend_space(mp, tp, &id, delta); > > > + } else { > > > + id.agno = nagcount - 1; > > > + error = xfs_ag_shrink_space(mp, &tp, &id, -delta); > > > > xfs_ag_shrink_space() looks like it only accesses id->agno. Perhaps just > > pass in agno for now..? > > Both way are ok, yet in my incomplete shrink whole empty AGs patchset, > it seems more natural to pass in &id rather than agno (since > id.agno = nagcount - 1 will be stayed in some new helper > e.g. xfs_shrink_ags()) @id is struct aghdr_init_data, but shrinking shouldn't initialize any AG headers. Are you planning to make use of it in shrink, either now or later on? > > > > > > + } > > > + > > > if (error) > > > goto out_trans_cancel; > > > } > > > @@ -137,15 +157,15 @@ xfs_growfs_data_private( > > > */ > > > if (nagcount > oagcount) > > > xfs_trans_mod_sb(tp, XFS_TRANS_SB_AGCOUNT, nagcount - oagcount); > > > - if (nb > mp->m_sb.sb_dblocks) > > > + if (nb != mp->m_sb.sb_dblocks) > > > xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS, > > > nb - mp->m_sb.sb_dblocks); > > > > Maybe use delta here? > > The reason is the same as above, `delta' here was changed due to > xfs_resizefs_init_new_ags(), which is not nb - mp->m_sb.sb_dblocks > anymore. so `extend` boolean is used (rather than just use delta > 0) Long question: The reason why we use (nb - dblocks) is because growfs is an all or nothing operation -- either we succeed in writing new empty AGs and inflating the (former) last AG of the fs, or we don't do anything at all. We don't allow partial growing; if we did, then delta would be relevant here. I think we get away with not needing to run transactions for each AG because those new AGs are inaccessible until we commit the new agcount/dblocks, right? In your design for the fs shrinker, do you anticipate being able to eliminate all the eligible AGs in a single transaction? Or do you envision only tackling one AG at a time? And can we be partially successful with a shrink? e.g. we succeed at eliminating the last AG, but then the one before that isn't empty and so we bail out, but by that point we did actually make the fs a little bit smaller. There's this comment at the bottom of xfs_growfs_data() that says that we can return error codes if the secondary sb update fails, even if the new size is already live. This convinces me that it's always been the case that callers of the growfs ioctl are supposed to re-query the fs geometry afterwards to find out if the fs size changed, even if the ioctl itself returns an error... which implies that partial grow/shrink are a possibility. > > > > > > if (id.nfree) > > > xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree); > > > > > > > id.nfree tracks newly added free space in the growfs space. Is it not > > used in the shrink case because the allocation handles this for us? > > Yeah, I'm afraid so. This is some common code, and also used in my > shrinking the whole AGs patchset. > > > > > > /* > > > - * update in-core counters now to reflect the real numbers > > > - * (especially sb_fdblocks) > > > + * update in-core counters now to reflect the real numbers (especially > > > + * sb_fdblocks). And xfs_validate_sb_write() can pass for shrinkfs. > > > */ > > > if (xfs_sb_version_haslazysbcount(&mp->m_sb)) > > > xfs_log_sb(tp); > > > @@ -165,7 +185,7 @@ xfs_growfs_data_private( > > > * If we expanded the last AG, free the per-AG reservation > > > * so we can reinitialize it with the new size. > > > */ > > > - if (delta) { > > > + if (extend && delta) { > > > struct xfs_perag *pag; > > > > > > pag = xfs_perag_get(mp, id.agno); > > > > We call xfs_fs_reserve_ag_blocks() a bit further down before we exit > > this function. xfs_ag_shrink_space() from the previous patch is intended > > to deal with perag reservation changes for shrink, but it looks like the > > reserve call further down could potentially reset mp->m_finobt_nores to > > false if it previously might have been set to true. > > Yeah, if my understanding is correct, I might need to call > xfs_fs_reserve_ag_blocks() only for growfs case as well for > mp->m_finobt_nores = true case. I suppose it's worth trying in the finobt_nores==true case. :) --D > > Thanks, > Gao Xiang > > > > > Brian > > > > > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c > > > index e72730f85af1..fd2cbf414b80 100644 > > > --- a/fs/xfs/xfs_trans.c > > > +++ b/fs/xfs/xfs_trans.c > > > @@ -419,7 +419,6 @@ xfs_trans_mod_sb( > > > tp->t_res_frextents_delta += delta; > > > break; > > > case XFS_TRANS_SB_DBLOCKS: > > > - ASSERT(delta > 0); > > > tp->t_dblocks_delta += delta; > > > break; > > > case XFS_TRANS_SB_AGCOUNT: > > > -- > > > 2.27.0 > > > > > >
On Wed, Feb 03, 2021 at 10:12:11AM -0800, Darrick J. Wong wrote: > On Wed, Feb 03, 2021 at 10:51:46PM +0800, Gao Xiang wrote: > > Hi Brian, > > > > On Wed, Feb 03, 2021 at 09:23:37AM -0500, Brian Foster wrote: > > > On Tue, Jan 26, 2021 at 08:56:20PM +0800, Gao Xiang wrote: > > > > As the first step of shrinking, this attempts to enable shrinking > > > > unused space in the last allocation group by fixing up freespace > > > > btree, agi, agf and adjusting super block and use a helper > > > > xfs_ag_shrink_space() to fixup the last AG. > > > > > > > > This can be all done in one transaction for now, so I think no > > > > additional protection is needed. > > > > > > > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > > > > --- > > > > fs/xfs/xfs_fsops.c | 64 ++++++++++++++++++++++++++++++---------------- > > > > fs/xfs/xfs_trans.c | 1 - > > > > 2 files changed, 42 insertions(+), 23 deletions(-) > > > > > > > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c > > > > index 6c4ab5e31054..4bcea22f7b3f 100644 > > > > --- a/fs/xfs/xfs_fsops.c > > > > +++ b/fs/xfs/xfs_fsops.c > > > > @@ -38,7 +38,7 @@ xfs_resizefs_init_new_ags( > > > > struct aghdr_init_data *id, > > > > xfs_agnumber_t oagcount, > > > > xfs_agnumber_t nagcount, > > > > - xfs_rfsblock_t *delta) > > > > + int64_t *delta) > > > > { > > > > xfs_rfsblock_t nb = mp->m_sb.sb_dblocks + *delta; > > > > int error; > > > > @@ -76,33 +76,41 @@ xfs_growfs_data_private( > > > > xfs_agnumber_t nagcount; > > > > xfs_agnumber_t nagimax = 0; > > > > xfs_rfsblock_t nb, nb_div, nb_mod; > > > > - xfs_rfsblock_t delta; > > > > + int64_t delta; > > > > xfs_agnumber_t oagcount; > > > > struct xfs_trans *tp; > > > > + bool extend; > > > > struct aghdr_init_data id = {}; > > > > > > > > nb = in->newblocks; > > > > - if (nb < mp->m_sb.sb_dblocks) > > > > - return -EINVAL; > > > > - if ((error = xfs_sb_validate_fsb_count(&mp->m_sb, nb))) > > > > + if (nb == mp->m_sb.sb_dblocks) > > > > + return 0; > > > > + > > > > + error = xfs_sb_validate_fsb_count(&mp->m_sb, nb); > > > > + if (error) > > > > return error; > > > > - error = xfs_buf_read_uncached(mp->m_ddev_targp, > > > > + > > > > + if (nb > mp->m_sb.sb_dblocks) { > > > > + error = xfs_buf_read_uncached(mp->m_ddev_targp, > > > > XFS_FSB_TO_BB(mp, nb) - XFS_FSS_TO_BB(mp, 1), > > > > XFS_FSS_TO_BB(mp, 1), 0, &bp, NULL); > > > > - if (error) > > > > - return error; > > > > - xfs_buf_relse(bp); > > > > + if (error) > > > > + return error; > > > > + xfs_buf_relse(bp); > > > > + } > > > > > > > > nb_div = nb; > > > > nb_mod = do_div(nb_div, mp->m_sb.sb_agblocks); > > > > nagcount = nb_div + (nb_mod != 0); > > > > if (nb_mod && nb_mod < XFS_MIN_AG_BLOCKS) { > > > > nagcount--; > > > > - nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks; > > > > - if (nb < mp->m_sb.sb_dblocks) > > > > + if (nagcount < 2) > > > > return -EINVAL; > > > > > > What's the reason for the nagcount < 2 check? IIRC we warn about this > > > configuration at mkfs time, but allow it to proceed. Is it just that we > > > don't want to accidentally put the fs into an agcount == 1 state that > > > was originally formatted with >1 AGs? > > > > Darrick once asked for avoiding shrinking the filesystem which has > > only 1 AG. > > It's worth mentioning why in a comment though: > > /* > * XFS doesn't really support single-AG filesystems, so do not > * permit callers to remove the filesystem's second and last AG. > */ > if (shrink && new_agcount < 2) > return -EHAHANOYOUDONT; > > But as Brian points out, we /do/ allow adding a second AG to a single-AG > fs. > > > > > > > What about the case where we attempt to grow an agcount == 1 fs but > > > don't enlarge enough to add the second AG? Does this change error > > > behavior in that case? > > > > Yeah, thanks for catching this! If growfs allows 1 AG case before, > > I think it needs to be refined. Let me update this in the next version! > > > > > > > > > + nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks; > > > > } > > > > + > > > > delta = nb - mp->m_sb.sb_dblocks; > > > > + extend = (delta > 0); > > > > oagcount = mp->m_sb.sb_agcount; > > > > > > > > /* allocate the new per-ag structures */ > > > > @@ -110,22 +118,34 @@ xfs_growfs_data_private( > > > > error = xfs_initialize_perag(mp, nagcount, &nagimax); > > > > if (error) > > > > return error; > > > > + } else if (nagcount < oagcount) { > > > > + /* TODO: shrinking the entire AGs hasn't yet completed */ > > > > + return -EINVAL; > > > > } > > > > > > > > error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata, > > > > - XFS_GROWFS_SPACE_RES(mp), 0, XFS_TRANS_RESERVE, &tp); > > > > + (extend ? XFS_GROWFS_SPACE_RES(mp) : -delta), 0, > > > > + XFS_TRANS_RESERVE, &tp); > > > > if (error) > > > > return error; > > > > > > > > - error = xfs_resizefs_init_new_ags(mp, &id, oagcount, nagcount, &delta); > > > > - if (error) > > > > - goto out_trans_cancel; > > > > - > > > > + if (extend) { > > > > + error = xfs_resizefs_init_new_ags(mp, &id, oagcount, > > > > + nagcount, &delta); > > > > + if (error) > > > > + goto out_trans_cancel; > > > > + } > > > > xfs_trans_agblocks_delta(tp, id.nfree); > > > > > > It looks like id isn't used until the resize call above. Is this call > > > relevant for the shrink case? > > > > I think it has nothing to do for the shrink the last AG case as well > > (id.nfree == 0 here) but maybe use for the later shrinking the whole > > AGs patchset. I can move into if (extend) in the next version. > > > > > > > > > > > > > - /* 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... > > I've never liked how the meaning of "delta" changes through the > function, and it clearly trips up reviewers. This variable isn't the > delta between the old dblocks and the new dblocks, it's really a > resizefs cursor that tells us how much work we still have to do. > > > > > > > > - error = xfs_ag_extend_space(mp, tp, &id, delta); > > > > + if (extend) { > > > > + error = xfs_ag_extend_space(mp, tp, &id, delta); > > > > + } else { > > > > + id.agno = nagcount - 1; > > > > + error = xfs_ag_shrink_space(mp, &tp, &id, -delta); > > > > > > xfs_ag_shrink_space() looks like it only accesses id->agno. Perhaps just > > > pass in agno for now..? > > > > Both way are ok, yet in my incomplete shrink whole empty AGs patchset, > > it seems more natural to pass in &id rather than agno (since > > id.agno = nagcount - 1 will be stayed in some new helper > > e.g. xfs_shrink_ags()) > > @id is struct aghdr_init_data, but shrinking shouldn't initialize any AG > headers. Are you planning to make use of it in shrink, either now or > later on? > > > > > > > > > > + } > > > > + > > > > if (error) > > > > goto out_trans_cancel; > > > > } > > > > @@ -137,15 +157,15 @@ xfs_growfs_data_private( > > > > */ > > > > if (nagcount > oagcount) > > > > xfs_trans_mod_sb(tp, XFS_TRANS_SB_AGCOUNT, nagcount - oagcount); > > > > - if (nb > mp->m_sb.sb_dblocks) > > > > + if (nb != mp->m_sb.sb_dblocks) > > > > xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS, > > > > nb - mp->m_sb.sb_dblocks); > > > > > > Maybe use delta here? > > > > The reason is the same as above, `delta' here was changed due to > > xfs_resizefs_init_new_ags(), which is not nb - mp->m_sb.sb_dblocks > > anymore. so `extend` boolean is used (rather than just use delta > 0) > > Long question: > > The reason why we use (nb - dblocks) is because growfs is an all or > nothing operation -- either we succeed in writing new empty AGs and > inflating the (former) last AG of the fs, or we don't do anything at > all. We don't allow partial growing; if we did, then delta would be > relevant here. I think we get away with not needing to run transactions > for each AG because those new AGs are inaccessible until we commit the > new agcount/dblocks, right? > > In your design for the fs shrinker, do you anticipate being able to > eliminate all the eligible AGs in a single transaction? Or do you > envision only tackling one AG at a time? And can we be partially > successful with a shrink? e.g. we succeed at eliminating the last AG, > but then the one before that isn't empty and so we bail out, but by that > point we did actually make the fs a little bit smaller. > > There's this comment at the bottom of xfs_growfs_data() that says that > we can return error codes if the secondary sb update fails, even if the > new size is already live. This convinces me that it's always been the > case that callers of the growfs ioctl are supposed to re-query the fs > geometry afterwards to find out if the fs size changed, even if the > ioctl itself returns an error... which implies that partial grow/shrink > are a possibility. And of course I got so buried in building up to my long question that I forgot to ask it: If the design of the shrinker requires incremental shrinking, should we support incremental growfs too? --D > > > > > > > > > if (id.nfree) > > > > xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree); > > > > > > > > > > id.nfree tracks newly added free space in the growfs space. Is it not > > > used in the shrink case because the allocation handles this for us? > > > > Yeah, I'm afraid so. This is some common code, and also used in my > > shrinking the whole AGs patchset. > > > > > > > > > /* > > > > - * update in-core counters now to reflect the real numbers > > > > - * (especially sb_fdblocks) > > > > + * update in-core counters now to reflect the real numbers (especially > > > > + * sb_fdblocks). And xfs_validate_sb_write() can pass for shrinkfs. > > > > */ > > > > if (xfs_sb_version_haslazysbcount(&mp->m_sb)) > > > > xfs_log_sb(tp); > > > > @@ -165,7 +185,7 @@ xfs_growfs_data_private( > > > > * If we expanded the last AG, free the per-AG reservation > > > > * so we can reinitialize it with the new size. > > > > */ > > > > - if (delta) { > > > > + if (extend && delta) { > > > > struct xfs_perag *pag; > > > > > > > > pag = xfs_perag_get(mp, id.agno); > > > > > > We call xfs_fs_reserve_ag_blocks() a bit further down before we exit > > > this function. xfs_ag_shrink_space() from the previous patch is intended > > > to deal with perag reservation changes for shrink, but it looks like the > > > reserve call further down could potentially reset mp->m_finobt_nores to > > > false if it previously might have been set to true. > > > > Yeah, if my understanding is correct, I might need to call > > xfs_fs_reserve_ag_blocks() only for growfs case as well for > > mp->m_finobt_nores = true case. > > I suppose it's worth trying in the finobt_nores==true case. :) > > --D > > > > > Thanks, > > Gao Xiang > > > > > > > > Brian > > > > > > > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c > > > > index e72730f85af1..fd2cbf414b80 100644 > > > > --- a/fs/xfs/xfs_trans.c > > > > +++ b/fs/xfs/xfs_trans.c > > > > @@ -419,7 +419,6 @@ xfs_trans_mod_sb( > > > > tp->t_res_frextents_delta += delta; > > > > break; > > > > case XFS_TRANS_SB_DBLOCKS: > > > > - ASSERT(delta > 0); > > > > tp->t_dblocks_delta += delta; > > > > break; > > > > case XFS_TRANS_SB_AGCOUNT: > > > > -- > > > > 2.27.0 > > > > > > > > >
Hi Darrick, On Wed, Feb 03, 2021 at 10:12:11AM -0800, Darrick J. Wong wrote: > On Wed, Feb 03, 2021 at 10:51:46PM +0800, Gao Xiang wrote: ... > > > > > > > + } > > > > + > > > > if (error) > > > > goto out_trans_cancel; > > > > } > > > > @@ -137,15 +157,15 @@ xfs_growfs_data_private( > > > > */ > > > > if (nagcount > oagcount) > > > > xfs_trans_mod_sb(tp, XFS_TRANS_SB_AGCOUNT, nagcount - oagcount); > > > > - if (nb > mp->m_sb.sb_dblocks) > > > > + if (nb != mp->m_sb.sb_dblocks) > > > > xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS, > > > > nb - mp->m_sb.sb_dblocks); > > > > > > Maybe use delta here? > > > > The reason is the same as above, `delta' here was changed due to > > xfs_resizefs_init_new_ags(), which is not nb - mp->m_sb.sb_dblocks > > anymore. so `extend` boolean is used (rather than just use delta > 0) > > Long question: > > The reason why we use (nb - dblocks) is because growfs is an all or > nothing operation -- either we succeed in writing new empty AGs and > inflating the (former) last AG of the fs, or we don't do anything at > all. We don't allow partial growing; if we did, then delta would be > relevant here. I think we get away with not needing to run transactions > for each AG because those new AGs are inaccessible until we commit the > new agcount/dblocks, right? > > In your design for the fs shrinker, do you anticipate being able to > eliminate all the eligible AGs in a single transaction? Or do you > envision only tackling one AG at a time? And can we be partially > successful with a shrink? e.g. we succeed at eliminating the last AG, > but then the one before that isn't empty and so we bail out, but by that > point we did actually make the fs a little bit smaller. Thanks for your question. I'm about to sleep, I might try to answer your question here. As for my current experiement / understanding, I think eliminating all the empty AGs + shrinking the tail AG in a single transaction is possible, that is what I'm done for now; 1) check the rest AGs are empty (from the nagcount AG to the oagcount - 1 AG) and mark them all inactive (AGs freezed); 2) consume an extent from the (nagcount - 1) AG; 3) decrease the number of agcount from oagcount to nagcount. Both 2) and 3) can be done in the same transaction, and after 1) the state of such empty AGs is fixed as well. So on-disk fs and runtime states are all in atomic. > > There's this comment at the bottom of xfs_growfs_data() that says that > we can return error codes if the secondary sb update fails, even if the > new size is already live. This convinces me that it's always been the > case that callers of the growfs ioctl are supposed to re-query the fs > geometry afterwards to find out if the fs size changed, even if the > ioctl itself returns an error... which implies that partial grow/shrink > are a possibility. > I didn't realize that possibility but if my understanding is correct the above process is described as above so no need to use incremental shrinking by its design. But it also support incremental shrinking if users try to use the ioctl for multiple times. If I'm wrong, kindly point out, many thanks in advance! Thanks, Gao Xiang
On Thu, Feb 04, 2021 at 03:02:17AM +0800, Gao Xiang wrote: > Hi Darrick, > > On Wed, Feb 03, 2021 at 10:12:11AM -0800, Darrick J. Wong wrote: > > On Wed, Feb 03, 2021 at 10:51:46PM +0800, Gao Xiang wrote: > > ... > > > > > > > > > > + } > > > > > + > > > > > if (error) > > > > > goto out_trans_cancel; > > > > > } > > > > > @@ -137,15 +157,15 @@ xfs_growfs_data_private( > > > > > */ > > > > > if (nagcount > oagcount) > > > > > xfs_trans_mod_sb(tp, XFS_TRANS_SB_AGCOUNT, nagcount - oagcount); > > > > > - if (nb > mp->m_sb.sb_dblocks) > > > > > + if (nb != mp->m_sb.sb_dblocks) > > > > > xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS, > > > > > nb - mp->m_sb.sb_dblocks); > > > > > > > > Maybe use delta here? > > > > > > The reason is the same as above, `delta' here was changed due to > > > xfs_resizefs_init_new_ags(), which is not nb - mp->m_sb.sb_dblocks > > > anymore. so `extend` boolean is used (rather than just use delta > 0) > > > > Long question: > > > > The reason why we use (nb - dblocks) is because growfs is an all or > > nothing operation -- either we succeed in writing new empty AGs and > > inflating the (former) last AG of the fs, or we don't do anything at > > all. We don't allow partial growing; if we did, then delta would be > > relevant here. I think we get away with not needing to run transactions > > for each AG because those new AGs are inaccessible until we commit the > > new agcount/dblocks, right? > > > > In your design for the fs shrinker, do you anticipate being able to > > eliminate all the eligible AGs in a single transaction? Or do you > > envision only tackling one AG at a time? And can we be partially > > successful with a shrink? e.g. we succeed at eliminating the last AG, > > but then the one before that isn't empty and so we bail out, but by that > > point we did actually make the fs a little bit smaller. > > Thanks for your question. I'm about to sleep, I might try to answer > your question here. > > As for my current experiement / understanding, I think eliminating all > the empty AGs + shrinking the tail AG in a single transaction is possible, > that is what I'm done for now; > 1) check the rest AGs are empty (from the nagcount AG to the oagcount - 1 > AG) and mark them all inactive (AGs freezed); Add some words, there might raise up some additional assistance transactions (e.g. if we'd like to confirm bmbt has the only one extent rather than just do some basic math to confirm the whole AG is empty) we might need to put all AGFL free blocks from AGFL to bmbt as well. Yet that process is independent from the main shrinking transaction. And in principle have no visible impact to users. I'll reply the rest suggestions tomorrow, thanks for the review again! Thanks, Gao Xiang > 2) consume an extent from the (nagcount - 1) AG; > 3) decrease the number of agcount from oagcount to nagcount. > > Both 2) and 3) can be done in the same transaction, and after 1) the state > of such empty AGs is fixed as well. So on-disk fs and runtime states are > all in atomic. > > > > > There's this comment at the bottom of xfs_growfs_data() that says that > > we can return error codes if the secondary sb update fails, even if the > > new size is already live. This convinces me that it's always been the > > case that callers of the growfs ioctl are supposed to re-query the fs > > geometry afterwards to find out if the fs size changed, even if the > > ioctl itself returns an error... which implies that partial grow/shrink > > are a possibility. > > > > I didn't realize that possibility but if my understanding is correct > the above process is described as above so no need to use incremental > shrinking by its design. But it also support incremental shrinking if > users try to use the ioctl for multiple times. > > If I'm wrong, kindly point out, many thanks in advance! > > Thanks, > Gao Xiang >
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
Hi Darrick, On Wed, Feb 03, 2021 at 10:12:11AM -0800, Darrick J. Wong wrote: > On Wed, Feb 03, 2021 at 10:51:46PM +0800, Gao Xiang wrote: ... > > > > nb_div = nb; > > > > nb_mod = do_div(nb_div, mp->m_sb.sb_agblocks); > > > > nagcount = nb_div + (nb_mod != 0); > > > > if (nb_mod && nb_mod < XFS_MIN_AG_BLOCKS) { > > > > nagcount--; > > > > - nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks; > > > > - if (nb < mp->m_sb.sb_dblocks) > > > > + if (nagcount < 2) > > > > return -EINVAL; > > > > > > What's the reason for the nagcount < 2 check? IIRC we warn about this > > > configuration at mkfs time, but allow it to proceed. Is it just that we > > > don't want to accidentally put the fs into an agcount == 1 state that > > > was originally formatted with >1 AGs? > > > > Darrick once asked for avoiding shrinking the filesystem which has > > only 1 AG. > > It's worth mentioning why in a comment though: > > /* > * XFS doesn't really support single-AG filesystems, so do not > * permit callers to remove the filesystem's second and last AG. > */ > if (shrink && new_agcount < 2) > return -EHAHANOYOUDONT; > > But as Brian points out, we /do/ allow adding a second AG to a single-AG > fs. (cont.) ok, thanks for this. anyway, I will cover such case in the next version. > > > > > > > What about the case where we attempt to grow an agcount == 1 fs but > > > don't enlarge enough to add the second AG? Does this change error > > > behavior in that case? > > > > Yeah, thanks for catching this! If growfs allows 1 AG case before, > > I think it needs to be refined. Let me update this in the next version! > > > > > > > > > + nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks; > > > > } > > > > + > > > > delta = nb - mp->m_sb.sb_dblocks; > > > > + extend = (delta > 0); > > > > oagcount = mp->m_sb.sb_agcount; > > > > > > > > /* allocate the new per-ag structures */ > > > > @@ -110,22 +118,34 @@ xfs_growfs_data_private( > > > > error = xfs_initialize_perag(mp, nagcount, &nagimax); > > > > if (error) > > > > return error; > > > > + } else if (nagcount < oagcount) { > > > > + /* TODO: shrinking the entire AGs hasn't yet completed */ > > > > + return -EINVAL; > > > > } > > > > > > > > error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata, > > > > - XFS_GROWFS_SPACE_RES(mp), 0, XFS_TRANS_RESERVE, &tp); > > > > + (extend ? XFS_GROWFS_SPACE_RES(mp) : -delta), 0, > > > > + XFS_TRANS_RESERVE, &tp); > > > > if (error) > > > > return error; > > > > > > > > - error = xfs_resizefs_init_new_ags(mp, &id, oagcount, nagcount, &delta); > > > > - if (error) > > > > - goto out_trans_cancel; > > > > - > > > > + if (extend) { > > > > + error = xfs_resizefs_init_new_ags(mp, &id, oagcount, > > > > + nagcount, &delta); > > > > + if (error) > > > > + goto out_trans_cancel; > > > > + } > > > > xfs_trans_agblocks_delta(tp, id.nfree); > > > > > > It looks like id isn't used until the resize call above. Is this call > > > relevant for the shrink case? > > > > I think it has nothing to do for the shrink the last AG case as well > > (id.nfree == 0 here) but maybe use for the later shrinking the whole > > AGs patchset. I can move into if (extend) in the next version. > > > > > > > > > > > > > - /* 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... > > I've never liked how the meaning of "delta" changes through the > function, and it clearly trips up reviewers. This variable isn't the > delta between the old dblocks and the new dblocks, it's really a > resizefs cursor that tells us how much work we still have to do. I found the first patch of this patchset has been merged into for-next, so some new idea about this? (split delta into 2 variables or some else way you'd prefer? so I could update in the next version as a whole...) > > > > > > > > - error = xfs_ag_extend_space(mp, tp, &id, delta); > > > > + if (extend) { > > > > + error = xfs_ag_extend_space(mp, tp, &id, delta); > > > > + } else { > > > > + id.agno = nagcount - 1; > > > > + error = xfs_ag_shrink_space(mp, &tp, &id, -delta); > > > > > > xfs_ag_shrink_space() looks like it only accesses id->agno. Perhaps just > > > pass in agno for now..? > > > > Both way are ok, yet in my incomplete shrink whole empty AGs patchset, > > it seems more natural to pass in &id rather than agno (since > > id.agno = nagcount - 1 will be stayed in some new helper > > e.g. xfs_shrink_ags()) > > @id is struct aghdr_init_data, but shrinking shouldn't initialize any AG > headers. Are you planning to make use of it in shrink, either now or > later on? I tried to use it as a global context structure for shrinking the whole AGs and the tail AG since I'm not sure we need to introduce another new structure to make it more complex, but yeah the naming is somewhat confusing now. > ... > > > > > > > /* > > > > - * update in-core counters now to reflect the real numbers > > > > - * (especially sb_fdblocks) > > > > + * update in-core counters now to reflect the real numbers (especially > > > > + * sb_fdblocks). And xfs_validate_sb_write() can pass for shrinkfs. > > > > */ > > > > if (xfs_sb_version_haslazysbcount(&mp->m_sb)) > > > > xfs_log_sb(tp); > > > > @@ -165,7 +185,7 @@ xfs_growfs_data_private( > > > > * If we expanded the last AG, free the per-AG reservation > > > > * so we can reinitialize it with the new size. > > > > */ > > > > - if (delta) { > > > > + if (extend && delta) { > > > > struct xfs_perag *pag; > > > > > > > > pag = xfs_perag_get(mp, id.agno); > > > > > > We call xfs_fs_reserve_ag_blocks() a bit further down before we exit > > > this function. xfs_ag_shrink_space() from the previous patch is intended > > > to deal with perag reservation changes for shrink, but it looks like the > > > reserve call further down could potentially reset mp->m_finobt_nores to > > > false if it previously might have been set to true. > > > > Yeah, if my understanding is correct, I might need to call > > xfs_fs_reserve_ag_blocks() only for growfs case as well for > > mp->m_finobt_nores = true case. > > I suppose it's worth trying in the finobt_nores==true case. :) > I didn't notice such trick before, will find some clue about this as well. Also as Brian mentioned, I'm not sure why xfs_ag_resv_free() the last AG and xfs_fs_reserve_ag_blocks() after AGF/AGI are unlocked for growfs... I think there could be some race window if some other fs allocation operations in parellel? Thanks, Gao Xiang > --D
On Thu, Feb 04, 2021 at 03:02:17AM +0800, Gao Xiang wrote: > Hi Darrick, > > On Wed, Feb 03, 2021 at 10:12:11AM -0800, Darrick J. Wong wrote: > > On Wed, Feb 03, 2021 at 10:51:46PM +0800, Gao Xiang wrote: > > ... > > > > > > > > > > + } > > > > > + > > > > > if (error) > > > > > goto out_trans_cancel; > > > > > } > > > > > @@ -137,15 +157,15 @@ xfs_growfs_data_private( > > > > > */ > > > > > if (nagcount > oagcount) > > > > > xfs_trans_mod_sb(tp, XFS_TRANS_SB_AGCOUNT, nagcount - oagcount); > > > > > - if (nb > mp->m_sb.sb_dblocks) > > > > > + if (nb != mp->m_sb.sb_dblocks) > > > > > xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS, > > > > > nb - mp->m_sb.sb_dblocks); > > > > > > > > Maybe use delta here? > > > > > > The reason is the same as above, `delta' here was changed due to > > > xfs_resizefs_init_new_ags(), which is not nb - mp->m_sb.sb_dblocks > > > anymore. so `extend` boolean is used (rather than just use delta > 0) > > > > Long question: > > > > The reason why we use (nb - dblocks) is because growfs is an all or > > nothing operation -- either we succeed in writing new empty AGs and > > inflating the (former) last AG of the fs, or we don't do anything at > > all. We don't allow partial growing; if we did, then delta would be > > relevant here. I think we get away with not needing to run transactions > > for each AG because those new AGs are inaccessible until we commit the > > new agcount/dblocks, right? > > > > In your design for the fs shrinker, do you anticipate being able to > > eliminate all the eligible AGs in a single transaction? Or do you > > envision only tackling one AG at a time? And can we be partially > > successful with a shrink? e.g. we succeed at eliminating the last AG, > > but then the one before that isn't empty and so we bail out, but by that > > point we did actually make the fs a little bit smaller. > > Thanks for your question. I'm about to sleep, I might try to answer > your question here. > > As for my current experiement / understanding, I think eliminating all > the empty AGs + shrinking the tail AG in a single transaction is possible, > that is what I'm done for now; > 1) check the rest AGs are empty (from the nagcount AG to the oagcount - 1 > AG) and mark them all inactive (AGs freezed); > 2) consume an extent from the (nagcount - 1) AG; > 3) decrease the number of agcount from oagcount to nagcount. > > Both 2) and 3) can be done in the same transaction, and after 1) the state > of such empty AGs is fixed as well. So on-disk fs and runtime states are > all in atomic. > > > > > There's this comment at the bottom of xfs_growfs_data() that says that > > we can return error codes if the secondary sb update fails, even if the > > new size is already live. This convinces me that it's always been the > > case that callers of the growfs ioctl are supposed to re-query the fs > > geometry afterwards to find out if the fs size changed, even if the > > ioctl itself returns an error... which implies that partial grow/shrink > > are a possibility. > > > > I didn't realize that possibility but if my understanding is correct > the above process is described as above so no need to use incremental > shrinking by its design. But it also support incremental shrinking if > users try to use the ioctl for multiple times. > This was one of the things I wondered about on an earlier versions of this work; whether we wanted to shrink to be deliberately incremental or not. I suspect that somewhat applies to even this version without AG truncation because technically we could allocate as much as possible out of end of the last AG and shrink by that amount. My initial thought was that if the implementation is going to be opportunistic (i.e., we provide no help to actually free up targeted space), perhaps an incremental implementation is a useful means to allow the operation to make progress. E.g., run a shrink, observe it didn't fully complete, shuffle around some files, repeat, etc. IIRC, one of the downsides of that sort of approach is any use case where the goal is an underlying storage device resize. I suppose an underlying device resize could also be opportunistic, but it seems more likely to me that use case would prefer an all or nothing approach, particularly if associated userspace tools don't really know how to handle a partially successful fs shrink. Do we have any idea how other tools/fs' behave in this regard (I thought ext4 supported shrink)? FWIW, it also seems potentially annoying to ask for a largish shrink only for the tool to hand back something relatively tiny. Based on your design description, it occurs to me that perhaps the ideal outcome is an implementation that supports a fully atomic all-or-nothing shrink (assuming this is reasonably possible), but supports an optional incremental mode specified by the interface. IOW, if we have the ability to perform all-or-nothing, then it _seems_ like a minor interface enhancement to support incremental on top of that as opposed to the other way around. Therefore, perhaps that should be the initial goal until shown to be too complex or otherwise problematic..? Brian > If I'm wrong, kindly point out, many thanks in advance! > > Thanks, > Gao Xiang >
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
Hi Brian, On Thu, Feb 04, 2021 at 07:33:03AM -0500, Brian Foster wrote: > On Thu, Feb 04, 2021 at 03:02:17AM +0800, Gao Xiang wrote: .... > > > > > > Long question: > > > > > > The reason why we use (nb - dblocks) is because growfs is an all or > > > nothing operation -- either we succeed in writing new empty AGs and > > > inflating the (former) last AG of the fs, or we don't do anything at > > > all. We don't allow partial growing; if we did, then delta would be > > > relevant here. I think we get away with not needing to run transactions > > > for each AG because those new AGs are inaccessible until we commit the > > > new agcount/dblocks, right? > > > > > > In your design for the fs shrinker, do you anticipate being able to > > > eliminate all the eligible AGs in a single transaction? Or do you > > > envision only tackling one AG at a time? And can we be partially > > > successful with a shrink? e.g. we succeed at eliminating the last AG, > > > but then the one before that isn't empty and so we bail out, but by that > > > point we did actually make the fs a little bit smaller. > > > > Thanks for your question. I'm about to sleep, I might try to answer > > your question here. > > > > As for my current experiement / understanding, I think eliminating all > > the empty AGs + shrinking the tail AG in a single transaction is possible, > > that is what I'm done for now; > > 1) check the rest AGs are empty (from the nagcount AG to the oagcount - 1 > > AG) and mark them all inactive (AGs freezed); > > 2) consume an extent from the (nagcount - 1) AG; > > 3) decrease the number of agcount from oagcount to nagcount. > > > > Both 2) and 3) can be done in the same transaction, and after 1) the state > > of such empty AGs is fixed as well. So on-disk fs and runtime states are > > all in atomic. > > > > > > > > There's this comment at the bottom of xfs_growfs_data() that says that > > > we can return error codes if the secondary sb update fails, even if the > > > new size is already live. This convinces me that it's always been the > > > case that callers of the growfs ioctl are supposed to re-query the fs > > > geometry afterwards to find out if the fs size changed, even if the > > > ioctl itself returns an error... which implies that partial grow/shrink > > > are a possibility. > > > > > > > I didn't realize that possibility but if my understanding is correct > > the above process is described as above so no need to use incremental > > shrinking by its design. But it also support incremental shrinking if > > users try to use the ioctl for multiple times. > > > > This was one of the things I wondered about on an earlier versions of > this work; whether we wanted to shrink to be deliberately incremental or > not. I suspect that somewhat applies to even this version without AG > truncation because technically we could allocate as much as possible out > of end of the last AG and shrink by that amount. My initial thought was > that if the implementation is going to be opportunistic (i.e., we > provide no help to actually free up targeted space), perhaps an > incremental implementation is a useful means to allow the operation to > make progress. E.g., run a shrink, observe it didn't fully complete, > shuffle around some files, repeat, etc. > > IIRC, one of the downsides of that sort of approach is any use case > where the goal is an underlying storage device resize. I suppose an > underlying device resize could also be opportunistic, but it seems more > likely to me that use case would prefer an all or nothing approach, > particularly if associated userspace tools don't really know how to > handle a partially successful fs shrink. Do we have any idea how other > tools/fs' behave in this regard (I thought ext4 supported shrink)? FWIW, > it also seems potentially annoying to ask for a largish shrink only for > the tool to hand back something relatively tiny. > > Based on your design description, it occurs to me that perhaps the ideal > outcome is an implementation that supports a fully atomic all-or-nothing > shrink (assuming this is reasonably possible), but supports an optional > incremental mode specified by the interface. IOW, if we have the ability > to perform all-or-nothing, then it _seems_ like a minor interface > enhancement to support incremental on top of that as opposed to the > other way around. Therefore, perhaps that should be the initial goal > until shown to be too complex or otherwise problematic..? > I cannot say too much of this, yet my current observation is that shrinking tail empty AG [+ empty AGs (optional)] in one transaction is practical (I don't see any barrier so far [1]). I'm implementing an atomic all-or-nothing truncation and userspace can utilize it to implement in all-or-nothing way (I saw Dave's spaceman work before) or incremental way (by using binary search approach and multiple ioctls)... In principle, supporting the ioctl with the extra partial shrinking feature is practial as well (but additional work might need to be done). And also, I'm not sure it's user-friendly since most end-users might want an all-or-nothing shrinking (at least in the fs truncation step) result. btw, afaik (my limited understanding), Ext4 shrinking is an offline approach so it's somewhat easier to implement (no need to consider any runtime impact), which is also considered as an all-or-nothing truncation as well (Although it also supports -M to shrink the filesystem to the minimum size, I think it can be implemented by multiple all-or-nothing shrink ioctls...) Thanks, Gao Xiang [1] it's somewhat outdated yet I'd like to finish this tail AG patchset first https://git.kernel.org/pub/scm/linux/kernel/git/xiang/linux.git/log/?h=xfs/shrink2 > Brian >
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
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c index 6c4ab5e31054..4bcea22f7b3f 100644 --- a/fs/xfs/xfs_fsops.c +++ b/fs/xfs/xfs_fsops.c @@ -38,7 +38,7 @@ xfs_resizefs_init_new_ags( struct aghdr_init_data *id, xfs_agnumber_t oagcount, xfs_agnumber_t nagcount, - xfs_rfsblock_t *delta) + int64_t *delta) { xfs_rfsblock_t nb = mp->m_sb.sb_dblocks + *delta; int error; @@ -76,33 +76,41 @@ xfs_growfs_data_private( xfs_agnumber_t nagcount; xfs_agnumber_t nagimax = 0; xfs_rfsblock_t nb, nb_div, nb_mod; - xfs_rfsblock_t delta; + int64_t delta; xfs_agnumber_t oagcount; struct xfs_trans *tp; + bool extend; struct aghdr_init_data id = {}; nb = in->newblocks; - if (nb < mp->m_sb.sb_dblocks) - return -EINVAL; - if ((error = xfs_sb_validate_fsb_count(&mp->m_sb, nb))) + if (nb == mp->m_sb.sb_dblocks) + return 0; + + error = xfs_sb_validate_fsb_count(&mp->m_sb, nb); + if (error) return error; - error = xfs_buf_read_uncached(mp->m_ddev_targp, + + if (nb > mp->m_sb.sb_dblocks) { + error = xfs_buf_read_uncached(mp->m_ddev_targp, XFS_FSB_TO_BB(mp, nb) - XFS_FSS_TO_BB(mp, 1), XFS_FSS_TO_BB(mp, 1), 0, &bp, NULL); - if (error) - return error; - xfs_buf_relse(bp); + if (error) + return error; + xfs_buf_relse(bp); + } nb_div = nb; nb_mod = do_div(nb_div, mp->m_sb.sb_agblocks); nagcount = nb_div + (nb_mod != 0); if (nb_mod && nb_mod < XFS_MIN_AG_BLOCKS) { nagcount--; - nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks; - if (nb < mp->m_sb.sb_dblocks) + if (nagcount < 2) return -EINVAL; + nb = (xfs_rfsblock_t)nagcount * mp->m_sb.sb_agblocks; } + delta = nb - mp->m_sb.sb_dblocks; + extend = (delta > 0); oagcount = mp->m_sb.sb_agcount; /* allocate the new per-ag structures */ @@ -110,22 +118,34 @@ xfs_growfs_data_private( error = xfs_initialize_perag(mp, nagcount, &nagimax); if (error) return error; + } else if (nagcount < oagcount) { + /* TODO: shrinking the entire AGs hasn't yet completed */ + return -EINVAL; } error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata, - XFS_GROWFS_SPACE_RES(mp), 0, XFS_TRANS_RESERVE, &tp); + (extend ? XFS_GROWFS_SPACE_RES(mp) : -delta), 0, + XFS_TRANS_RESERVE, &tp); if (error) return error; - error = xfs_resizefs_init_new_ags(mp, &id, oagcount, nagcount, &delta); - if (error) - goto out_trans_cancel; - + if (extend) { + error = xfs_resizefs_init_new_ags(mp, &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 there are some blocks in the last AG, resize it. */ if (delta) { - error = xfs_ag_extend_space(mp, tp, &id, delta); + if (extend) { + error = xfs_ag_extend_space(mp, tp, &id, delta); + } else { + id.agno = nagcount - 1; + error = xfs_ag_shrink_space(mp, &tp, &id, -delta); + } + if (error) goto out_trans_cancel; } @@ -137,15 +157,15 @@ xfs_growfs_data_private( */ if (nagcount > oagcount) xfs_trans_mod_sb(tp, XFS_TRANS_SB_AGCOUNT, nagcount - oagcount); - if (nb > mp->m_sb.sb_dblocks) + if (nb != mp->m_sb.sb_dblocks) xfs_trans_mod_sb(tp, XFS_TRANS_SB_DBLOCKS, nb - mp->m_sb.sb_dblocks); if (id.nfree) xfs_trans_mod_sb(tp, XFS_TRANS_SB_FDBLOCKS, id.nfree); /* - * update in-core counters now to reflect the real numbers - * (especially sb_fdblocks) + * update in-core counters now to reflect the real numbers (especially + * sb_fdblocks). And xfs_validate_sb_write() can pass for shrinkfs. */ if (xfs_sb_version_haslazysbcount(&mp->m_sb)) xfs_log_sb(tp); @@ -165,7 +185,7 @@ xfs_growfs_data_private( * If we expanded the last AG, free the per-AG reservation * so we can reinitialize it with the new size. */ - if (delta) { + if (extend && delta) { struct xfs_perag *pag; pag = xfs_perag_get(mp, id.agno); diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c index e72730f85af1..fd2cbf414b80 100644 --- a/fs/xfs/xfs_trans.c +++ b/fs/xfs/xfs_trans.c @@ -419,7 +419,6 @@ xfs_trans_mod_sb( tp->t_res_frextents_delta += delta; break; case XFS_TRANS_SB_DBLOCKS: - ASSERT(delta > 0); tp->t_dblocks_delta += delta; break; case XFS_TRANS_SB_AGCOUNT:
As the first step of shrinking, this attempts to enable shrinking unused space in the last allocation group by fixing up freespace btree, agi, agf and adjusting super block and use a helper xfs_ag_shrink_space() to fixup the last AG. This can be all done in one transaction for now, so I think no additional protection is needed. Signed-off-by: Gao Xiang <hsiangkao@redhat.com> --- fs/xfs/xfs_fsops.c | 64 ++++++++++++++++++++++++++++++---------------- fs/xfs/xfs_trans.c | 1 - 2 files changed, 42 insertions(+), 23 deletions(-)