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 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

> 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.

(Agreed.)

--D

> Cheers,
> 
> Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com