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

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

[Srikanth C S]

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

With this change we have
$ mkfs.xfs -f -d agcount=129 test.img
Invalid value 129 for -d agcount option. Value is too large.

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();	
+		}
 	} else {
 		calc_default_ag_geometry(cfg->blocklog, cfg->dblocks,
 					 cfg->dsunit, &cfg->agsize,
-- 
2.25.1

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

[Dave Chinner]

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.

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.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

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

[Darrick J. Wong]

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

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

[Srikanth C S]

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

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

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

[Darrick J. Wong]

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