On Mon, Mar 22, 2021 at 09:42:55AM -0700, Darrick J. Wong wrote:
> On Mon, Mar 22, 2021 at 08:50:28PM +0800, Gao Xiang wrote:
> > On Mon, Mar 22, 2021 at 08:43:19AM -0400, Brian Foster wrote:
> > > On Mon, Mar 22, 2021 at 08:36:52PM +0800, Gao Xiang wrote:
> > > > On Mon, Mar 22, 2021 at 08:26:41AM -0400, Brian Foster wrote:
> > > > > On Mon, Mar 22, 2021 at 08:07:22PM +0800, Gao Xiang wrote:
> > > > > > On Mon, Mar 22, 2021 at 07:30:40AM -0400, Brian Foster wrote:
> > > > > > > On Fri, Mar 05, 2021 at 10:57:02AM +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.
> > > > > > > >
> > > > > > > > Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > > > > > > > Signed-off-by: Gao Xiang <hsiangkao@xxxxxxxxxx>
> > > > > > > > ---
> > > > > > > > fs/xfs/xfs_fsops.c | 88 ++++++++++++++++++++++++++++------------------
> > > > > > > > fs/xfs/xfs_trans.c | 1 -
> > > > > > > > 2 files changed, 53 insertions(+), 36 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> > > > > > > > index fc9e799b2ae3..71cba61a451c 100644
> > > > > > > > --- a/fs/xfs/xfs_fsops.c
> > > > > > > > +++ b/fs/xfs/xfs_fsops.c
> > > > > ...
> > > > > > > > @@ -115,10 +120,15 @@ xfs_growfs_data_private(
> > > > > > > > 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)
> > > > > > > > - return -EINVAL;
> > > > > > > > }
> > > > > > > > delta = nb - mp->m_sb.sb_dblocks;
> > > > > > > > + /*
> > > > > > > > + * XFS doesn't really support single-AG filesystems, so do not
> > > > > > > > + * permit callers to remove the filesystem's second and last AG.
> > > > > > > > + */
> > > > > > > > + if (delta < 0 && nagcount < 2)
> > > > > > > > + return -EINVAL;
> > > > > > > > +
> > > > > > >
> > > > > > > What if the filesystem is already single AG? Unless I'm missing
> > > > > > > something, we already have a check a bit further down that prevents
> > > > > > > removal of AGs in the first place.
> > > > > >
> > > > > > I think it tends to forbid (return -EINVAL) shrinking the filesystem with
> > > > > > a single AG only? Am I missing something?
> > > > > >
> > > > >
> > > > > My assumption was this check means one can't shrink a filesystem that is
> > > > > already agcount == 1. The comment refers to preventing shrink from
> > > > > causing an agcount == 1 fs. What is the intent?
>
> Both of those things.
>
> > > >
> > > > I think it means the latter -- preventing shrink from causing an agcount == 1
> > > > fs. since nagcount (new agcount) <= 1?
> > > >
> > >
> > > Right, so that leads to my question... does this check also fail a
> > > shrink on an fs that is already agcount == 1? If so, why? I know
> > > technically it's not a supported configuration, but mkfs allows it.
> >
> > Ah, I'm not sure if Darrick would like to forbid agcount == 1 shrinking
> > functionitity completely, see the previous comment:
> > https://lore.kernel.org/r/20201014160633.GD9832@magnolia/
> >
> > (please ignore the modification at that time, since it was buggy...)
>
> Given the confusion I propose a new comment:
>
> /*
> * Reject filesystems with a single AG because they are not
> * supported, and reject a shrink operation that would cause a
> * filesystem to become unsupported.
> */
> if (delta < 0 && nagcount < 2)
> return -EINVAL;
>
ok, will update this comment. thanks for your suggestion!
Thanks,
Gao Xiang
> --D
