diff mbox series

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

Message ID 20210126125621.3846735-7-hsiangkao@redhat.com (mailing list archive)
State New
Headers show
Series xfs: support shrinking free space in the last AG | expand

Commit Message

Gao Xiang Jan. 26, 2021, 12:56 p.m. UTC
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(-)

Comments

Brian Foster Feb. 3, 2021, 2:23 p.m. UTC | #1
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
>
Gao Xiang Feb. 3, 2021, 2:51 p.m. UTC | #2
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
> > 
>
Brian Foster Feb. 3, 2021, 6:01 p.m. UTC | #3
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
> > > 
> > 
>
Darrick J. Wong Feb. 3, 2021, 6:12 p.m. UTC | #4
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
> > > 
> > 
>
Darrick J. Wong Feb. 3, 2021, 6:14 p.m. UTC | #5
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
> > > > 
> > > 
> >
Gao Xiang Feb. 3, 2021, 7:02 p.m. UTC | #6
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
Gao Xiang Feb. 3, 2021, 7:19 p.m. UTC | #7
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
>
Gao Xiang Feb. 4, 2021, 9:18 a.m. UTC | #8
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
Gao Xiang Feb. 4, 2021, 9:40 a.m. UTC | #9
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
Brian Foster Feb. 4, 2021, 12:33 p.m. UTC | #10
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
>
Brian Foster Feb. 4, 2021, 12:33 p.m. UTC | #11
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
Gao Xiang Feb. 4, 2021, 1:58 p.m. UTC | #12
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
>
Gao Xiang Feb. 4, 2021, 4:21 p.m. UTC | #13
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 mbox series

Patch

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: