Re: [PATCH] mkfs: custom agcount that renders AG size < XFS_AG_MIN_BYTES gives "Assertion failed. Aborted"

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Jul 05, 2022 at 06:12:56PM +0000, Srikanth C S wrote:
> 
> 
> > On Tue, Jul 05, 2022 at 01:55:36PM +1000, Dave Chinner wrote:
> > > On Tue, Jul 05, 2022 at 08:49:58AM +0530, Srikanth C S wrote:
> > > > For a 2GB FS we have
> > > > $ mkfs.xfs -f -d agcount=129 test.img
> > > > mkfs.xfs: xfs_mkfs.c:3021: align_ag_geometry: Assertion
> > `!cli_opt_set(&dopts, D_AGCOUNT)' failed.
> > > > Aborted
> > >
> > > Ok, that's because the size of the last AG is too small when trying to
> > > align the AG size to stripe geometry. It fails an assert that says "we
> > > should not get here if the agcount was specified on the CLI".
> > >
> > > >
> > > > With this change we have
> > > > $ mkfs.xfs -f -d agcount=129 test.img Invalid value 129 for -d
> > > > agcount option. Value is too large.
> > 
> > What version of mkfs is this?
> > 
> > $ truncate -s 2g /tmp/a
> > $ mkfs.xfs -V
> > mkfs.xfs version 5.18.0
> > $ mkfs.xfs -f -d agcount=129 /tmp/a
> > agsize (4065 blocks) too small, need at least 4096 blocks
> > 
> 
> For the same version I get Assertion failed
> $ truncate -s 2g /tmp/a
> $ mkfs.xfs -V
> mkfs.xfs version 5.18.0
> $ mkfs.xfs -f -d agcount=129 /tmp/a
> mkfs.xfs: xfs_mkfs.c:3033: align_ag_geometry: Assertion `!cli_opt_set(&dopts, D_AGCOUNT)' failed.
> Aborted (core dumped)

ahaha, the distro package got built with -DNDEBUG, which turned off
ASSERTions.  With my upstream tot build I see this problem too...

> > > OK, but....
> > >
> > > >
> > > > Signed-off-by: Srikanth C S <srikanth.c.s@oracle.com>
> > > > ---
> > > >  mkfs/xfs_mkfs.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index
> > > > 057b3b09..32dcbfff 100644
> > > > --- a/mkfs/xfs_mkfs.c
> > > > +++ b/mkfs/xfs_mkfs.c
> > > > @@ -2897,6 +2897,13 @@ _("agsize (%s) not a multiple of fs blk size
> > (%d)\n"),
> > > >  		cfg->agcount = cli->agcount;
> > > >  		cfg->agsize = cfg->dblocks / cfg->agcount +
> > > >  				(cfg->dblocks % cfg->agcount != 0);
> > > > +		if (cfg->agsize < XFS_AG_MIN_BYTES >> cfg->blocklog)
> > > > +		{
> > > > +			fprintf(stderr,
> > > > +_("Invalid value %lld for -d agcount option. Value is too large.\n"),
> > > > +				(long long)cli->agcount);
> > > > +			usage();
> > > > +		}
> > >
> > > .... that's not where we validate the calculated ag size. That happens
> > > via align_ag_geometry() -> validate_ag_geometry(). i.e. we can't
> > > reject an AG specification until after we've applied all the necessary
> > > modifiers to it first (such as stripe alignment requirements).
> > >
> > > IOWs, we do actually check for valid AG sizes, it's just that this
> > > specific case hit an ASSERT() check before we got to validating the AG
> > > size. I'm pretty sure just removing the ASSERT - which assumes that
> > > "-d agcount=xxx" is not so large that it produces undersized AGs -
> > > will fix the problem and result in the correct error message being
> > > returned.

...so yeah, what Dave said. :)

--D

> > 
> > (Agreed.)
> > 
> > --D
> > 
> > > Cheers,
> > >
> > > Dave.
> > >
> > > --
> > > Dave Chinner
> > > david@fromorbit.com