(sorry, I was too sleepy at that time... so I didn't even realize if
I replied them all...go on replying this.. at least for some record
... sorry for annoying)
On Wed, Jan 20, 2021 at 11:25:06AM -0800, Darrick J. Wong wrote:
> On Mon, Jan 18, 2021 at 04:36:59PM +0800, Gao Xiang wrote:
...
> >
> > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > index db6ed354c465..2ae4f33b42c9 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) {
>
> Nit: nagcount < oagcount ?
(cont..)
ok, that is equal.. will update this.
>
> > + /* 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);
>
> This is a little nitpicky, but I wonder if the reorganization of
> xfs_growfs_data_private ought to be in a separate preparation patch,
> wherein you'd define xfs_ag_shrink_space as a stub that returns
> EOPNOSUPP, and make all the necessary adjustments to the caller.
>
> That way, this second patch would concentrate on replacing the
> shrink_space stub an actual implementation.
I could have a try on this. Another thought you mentioned on IRC was
seperating shrinkfs into another function, e.g.
xfs_shrinkfs_data_private()... Although Brian once mentioned he liked
to use the shared way, I'm both fine with these. So the next version
I would like to seperate it as a try. And see if it looks ok to
people.
>
> > + }
> > +
> > if (error)
> > goto out_trans_cancel;
> > }
> > @@ -137,11 +157,19 @@ 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 (especially sb_fdblocks) now
> > + * so xfs_validate_sb_write() can pass.
> > + */
> > + if (xfs_sb_version_haslazysbcount(&mp->m_sb))
> > + xfs_log_sb(tp);
>
> How do we get a failure in xfs_validate_sb_write? We're changing
> fdblocks and dblocks in the same transaction, which means that both
> counters should have changed by the number of blocks we took out of
> the filesystem, right?
>
> Is the problem that the TRANS_SB_DBLOCKS change above makes the primary
> super's sb_dblocks decrease immediately, but since we're in lazycounters
> mode we defer updating sb_fdblocks until unmount, so in the meantime
> we fail the sb write verifier because fdblocks > dblocks?
Yeah, this was mainly to deal with some sb write verifier at that time,
otherwise sb verifier would complain about this:
https://lore.kernel.org/r/20201021142230.GA30714@xxxxxxxxxxxxxxxxxx/
>
> Or: is it` the general case that we ought to be forcing fdblocks to get
> logged here even for fs grow operations? In which case this (minor)
> behavior change probably should go in a separate patch.
I think it's also needed to apply for growfs case as well, yet I didn't
observe some strange about this on growfs, but I think generally lazy
sb counters (including sb_dblocks and sb_fdblocks) might be better to be
updated immediately for all resizing cases. ok, will add another patch
to handle this...
Thanks,
Gao Xiang
>
> --D
>
