[PATCH v2] mkfs.xfs: fix ASSERT on too-small device with stripe geometry

[PATCH v2] mkfs.xfs: fix ASSERT on too-small device with stripe geometry

[Pavel Reichl]

When a too-small device is created with stripe geometry, we hit an
assert in align_ag_geometry():

mkfs.xfs: xfs_mkfs.c:2834: align_ag_geometry: Assertion `cfg->agcount != 0' failed.

This is because align_ag_geometry() finds that the size of the last
(only) AG is too small, and attempts to trim it off.  Obviously 0
AGs is invalid, and we hit the ASSERT.

Reported-by: Zdenek Kabelac <zkabelac@redhat.com>
Suggested-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 include/xfs_multidisk.h | 14 +++++++-------
 mkfs/xfs_mkfs.c         |  6 +++---
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/xfs_multidisk.h b/include/xfs_multidisk.h
index 1b9936ec..abfb50ce 100644
--- a/include/xfs_multidisk.h
+++ b/include/xfs_multidisk.h
@@ -14,7 +14,6 @@
 #define	XFS_DFL_BLOCKSIZE_LOG	12		/* 4096 byte blocks */
 #define	XFS_DINODE_DFL_LOG	8		/* 256 byte inodes */
 #define	XFS_DINODE_DFL_CRC_LOG	9		/* 512 byte inodes for CRCs */
-#define	XFS_MIN_DATA_BLOCKS	100
 #define	XFS_MIN_INODE_PERBLOCK	2		/* min inodes per block */
 #define	XFS_DFL_IMAXIMUM_PCT	25		/* max % of space for inodes */
 #define	XFS_MIN_REC_DIRSIZE	12		/* 4096 byte dirblocks (V2) */
@@ -25,13 +24,14 @@
 						 * accept w/o warnings
 						 */
 
-#define XFS_AG_BYTES(bblog)	((long long)BBSIZE << (bblog))
-#define	XFS_AG_MIN_BYTES	((XFS_AG_BYTES(15)))	/* 16 MB */
-#define	XFS_AG_MAX_BYTES	((XFS_AG_BYTES(31)))	/* 1 TB */
-#define XFS_AG_MIN_BLOCKS(blog)	(XFS_AG_MIN_BYTES >> (blog))
-#define XFS_AG_MAX_BLOCKS(blog)	((XFS_AG_MAX_BYTES - 1) >> (blog))
+#define XFS_AG_BYTES(bblog)		((long long)BBSIZE << (bblog))
+#define	XFS_MIN_DATA_BLOCKS(cfg)	(XFS_AG_MIN_BLOCKS((cfg)->blocklog))
+#define	XFS_AG_MIN_BYTES		((XFS_AG_BYTES(15)))	/* 16 MB */
+#define	XFS_AG_MAX_BYTES		((XFS_AG_BYTES(31)))	/* 1 TB */
+#define XFS_AG_MIN_BLOCKS(blog)		(XFS_AG_MIN_BYTES >> (blog))
+#define XFS_AG_MAX_BLOCKS(blog)		((XFS_AG_MAX_BYTES - 1) >> (blog))
 
-#define XFS_MAX_AGNUMBER	((xfs_agnumber_t)(NULLAGNUMBER - 1))
+#define XFS_MAX_AGNUMBER		((xfs_agnumber_t)(NULLAGNUMBER - 1))
 
 /*
  * These values define what we consider a "multi-disk" filesystem. That is, a
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index a687f385..204dfff1 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -2530,10 +2530,10 @@ _("size %s specified for data subvolume is too large, maximum is %lld blocks\n")
 		cfg->dblocks = DTOBT(xi->dsize, cfg->blocklog);
 	}
 
-	if (cfg->dblocks < XFS_MIN_DATA_BLOCKS) {
+	if (cfg->dblocks < XFS_MIN_DATA_BLOCKS(cfg)) {
 		fprintf(stderr,
-_("size %lld of data subvolume is too small, minimum %d blocks\n"),
-			(long long)cfg->dblocks, XFS_MIN_DATA_BLOCKS);
+_("size %lld of data subvolume is too small, minimum %lld blocks\n"),
+			(long long)cfg->dblocks, XFS_MIN_DATA_BLOCKS(cfg));
 		usage();
 	}
 
-- 
2.26.2

Re: [PATCH v2] mkfs.xfs: fix ASSERT on too-small device with stripe geometry

[Christoph Hellwig]

On Wed, Sep 16, 2020 at 10:40:56PM +0200, Pavel Reichl wrote:
> When a too-small device is created with stripe geometry, we hit an
> assert in align_ag_geometry():
> 
> mkfs.xfs: xfs_mkfs.c:2834: align_ag_geometry: Assertion `cfg->agcount != 0' failed.
> 
> This is because align_ag_geometry() finds that the size of the last
> (only) AG is too small, and attempts to trim it off.  Obviously 0
> AGs is invalid, and we hit the ASSERT.
> 
> Reported-by: Zdenek Kabelac <zkabelac@redhat.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Pavel Reichl <preichl@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2] mkfs.xfs: fix ASSERT on too-small device with stripe geometry

[Carlos Maiolino]

On Wed, Sep 16, 2020 at 10:40:56PM +0200, Pavel Reichl wrote:
> When a too-small device is created with stripe geometry, we hit an
> assert in align_ag_geometry():
> 
> mkfs.xfs: xfs_mkfs.c:2834: align_ag_geometry: Assertion `cfg->agcount != 0' failed.
> 
> This is because align_ag_geometry() finds that the size of the last
> (only) AG is too small, and attempts to trim it off.  Obviously 0
> AGs is invalid, and we hit the ASSERT.
> 

Patch looks good, next time please keep the Changelog :)
Cheers.

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> Reported-by: Zdenek Kabelac <zkabelac@redhat.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  include/xfs_multidisk.h | 14 +++++++-------
>  mkfs/xfs_mkfs.c         |  6 +++---
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/include/xfs_multidisk.h b/include/xfs_multidisk.h
> index 1b9936ec..abfb50ce 100644
> --- a/include/xfs_multidisk.h
> +++ b/include/xfs_multidisk.h
> @@ -14,7 +14,6 @@
>  #define	XFS_DFL_BLOCKSIZE_LOG	12		/* 4096 byte blocks */
>  #define	XFS_DINODE_DFL_LOG	8		/* 256 byte inodes */
>  #define	XFS_DINODE_DFL_CRC_LOG	9		/* 512 byte inodes for CRCs */
> -#define	XFS_MIN_DATA_BLOCKS	100
>  #define	XFS_MIN_INODE_PERBLOCK	2		/* min inodes per block */
>  #define	XFS_DFL_IMAXIMUM_PCT	25		/* max % of space for inodes */
>  #define	XFS_MIN_REC_DIRSIZE	12		/* 4096 byte dirblocks (V2) */
> @@ -25,13 +24,14 @@
>  						 * accept w/o warnings
>  						 */
>  
> -#define XFS_AG_BYTES(bblog)	((long long)BBSIZE << (bblog))
> -#define	XFS_AG_MIN_BYTES	((XFS_AG_BYTES(15)))	/* 16 MB */
> -#define	XFS_AG_MAX_BYTES	((XFS_AG_BYTES(31)))	/* 1 TB */
> -#define XFS_AG_MIN_BLOCKS(blog)	(XFS_AG_MIN_BYTES >> (blog))
> -#define XFS_AG_MAX_BLOCKS(blog)	((XFS_AG_MAX_BYTES - 1) >> (blog))
> +#define XFS_AG_BYTES(bblog)		((long long)BBSIZE << (bblog))
> +#define	XFS_MIN_DATA_BLOCKS(cfg)	(XFS_AG_MIN_BLOCKS((cfg)->blocklog))
> +#define	XFS_AG_MIN_BYTES		((XFS_AG_BYTES(15)))	/* 16 MB */
> +#define	XFS_AG_MAX_BYTES		((XFS_AG_BYTES(31)))	/* 1 TB */
> +#define XFS_AG_MIN_BLOCKS(blog)		(XFS_AG_MIN_BYTES >> (blog))
> +#define XFS_AG_MAX_BLOCKS(blog)		((XFS_AG_MAX_BYTES - 1) >> (blog))
>  
> -#define XFS_MAX_AGNUMBER	((xfs_agnumber_t)(NULLAGNUMBER - 1))
> +#define XFS_MAX_AGNUMBER		((xfs_agnumber_t)(NULLAGNUMBER - 1))
>  
>  /*
>   * These values define what we consider a "multi-disk" filesystem. That is, a
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index a687f385..204dfff1 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2530,10 +2530,10 @@ _("size %s specified for data subvolume is too large, maximum is %lld blocks\n")
>  		cfg->dblocks = DTOBT(xi->dsize, cfg->blocklog);
>  	}
>  
> -	if (cfg->dblocks < XFS_MIN_DATA_BLOCKS) {
> +	if (cfg->dblocks < XFS_MIN_DATA_BLOCKS(cfg)) {
>  		fprintf(stderr,
> -_("size %lld of data subvolume is too small, minimum %d blocks\n"),
> -			(long long)cfg->dblocks, XFS_MIN_DATA_BLOCKS);
> +_("size %lld of data subvolume is too small, minimum %lld blocks\n"),
> +			(long long)cfg->dblocks, XFS_MIN_DATA_BLOCKS(cfg));
>  		usage();
>  	}
>  
> -- 
> 2.26.2
> 

-- 
Carlos

[PATCH] mkfs.xfs: fix ASSERT on too-small device with stripe geometry

[Eric Sandeen]

When a too-small device is created with stripe geometry, we hit an
assert in align_ag_geometry():

# truncate --size=10444800 testfile
# mkfs.xfs -dsu=65536,sw=1 testfile 
mkfs.xfs: xfs_mkfs.c:2834: align_ag_geometry: Assertion `cfg->agcount != 0' failed.

This is because align_ag_geometry() finds that the size of the last
(only) AG is too small, and attempts to trim it off.  Obviously 0
AGs is invalid, and we hit the ASSERT.

Fix this by skipping the last-ag-trim if there is only one AG, and
add a new test to validate_ag_geometry() which offers a very specific,
clear warning if the device (in dblocks) is smaller than the minimum
allowed AG size.

Reported-by: Zdenek Kabelac <zkabelac@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index a687f385..da8c5986 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1038,6 +1038,15 @@ validate_ag_geometry(
 	uint64_t	agsize,
 	uint64_t	agcount)
 {
+	/* Is this device simply too small? */
+	if (dblocks < XFS_AG_MIN_BLOCKS(blocklog)) {
+		fprintf(stderr,
+	_("device (%lld blocks) too small, need at least %lld blocks\n"),
+			(long long)dblocks,
+			(long long)XFS_AG_MIN_BLOCKS(blocklog));
+		usage();
+	}
+
 	if (agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
 		fprintf(stderr,
 	_("agsize (%lld blocks) too small, need at least %lld blocks\n"),
@@ -2827,11 +2836,12 @@ validate:
 	 * and drop the blocks.
 	 */
 	if (cfg->dblocks % cfg->agsize != 0 &&
+	     cfg->agcount > 1 &&
 	     (cfg->dblocks % cfg->agsize < XFS_AG_MIN_BLOCKS(cfg->blocklog))) {
+printf("%d %d %d\n", cfg->dblocks, cfg->agsize, cfg->dblocks % cfg->agsize);
 		ASSERT(!cli_opt_set(&dopts, D_AGCOUNT));
 		cfg->dblocks = (xfs_rfsblock_t)((cfg->agcount - 1) * cfg->agsize);
 		cfg->agcount--;
-		ASSERT(cfg->agcount != 0);
 	}
 
 	validate_ag_geometry(cfg->blocklog, cfg->dblocks,

[PATCH V2] mkfs.xfs: fix ASSERT on too-small device with stripe geometry

[Eric Sandeen]

When a too-small device is created with stripe geometry, we hit an
assert in align_ag_geometry():

# truncate --size=10444800 testfile
# mkfs.xfs -dsu=65536,sw=1 testfile 
mkfs.xfs: xfs_mkfs.c:2834: align_ag_geometry: Assertion `cfg->agcount != 0' failed.

This is because align_ag_geometry() finds that the size of the last
(only) AG is too small, and attempts to trim it off.  Obviously 0
AGs is invalid, and we hit the ASSERT.

Fix this by skipping the last-ag-trim if there is only one AG, and
add a new test to validate_ag_geometry() which offers a very specific,
clear warning if the device (in dblocks) is smaller than the minimum
allowed AG size.

Reported-by: Zdenek Kabelac <zkabelac@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: remove stray printf, sorry

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index a687f385..2139aedb 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1038,6 +1038,15 @@ validate_ag_geometry(
 	uint64_t	agsize,
 	uint64_t	agcount)
 {
+	/* Is this device simply too small? */
+	if (dblocks < XFS_AG_MIN_BLOCKS(blocklog)) {
+		fprintf(stderr,
+	_("device (%lld blocks) too small, need at least %lld blocks\n"),
+			(long long)dblocks,
+			(long long)XFS_AG_MIN_BLOCKS(blocklog));
+		usage();
+	}
+
 	if (agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
 		fprintf(stderr,
 	_("agsize (%lld blocks) too small, need at least %lld blocks\n"),
@@ -2827,11 +2836,11 @@ validate:
 	 * and drop the blocks.
 	 */
 	if (cfg->dblocks % cfg->agsize != 0 &&
+	     cfg->agcount > 1 &&
 	     (cfg->dblocks % cfg->agsize < XFS_AG_MIN_BLOCKS(cfg->blocklog))) {
 		ASSERT(!cli_opt_set(&dopts, D_AGCOUNT));
 		cfg->dblocks = (xfs_rfsblock_t)((cfg->agcount - 1) * cfg->agsize);
 		cfg->agcount--;
-		ASSERT(cfg->agcount != 0);
 	}
 
 	validate_ag_geometry(cfg->blocklog, cfg->dblocks,

Re: [PATCH V2] mkfs.xfs: fix ASSERT on too-small device with stripe geometry

[Darrick J. Wong]

On Mon, Sep 14, 2020 at 02:00:52PM -0500, Eric Sandeen wrote:
> When a too-small device is created with stripe geometry, we hit an
> assert in align_ag_geometry():
> 
> # truncate --size=10444800 testfile
> # mkfs.xfs -dsu=65536,sw=1 testfile 
> mkfs.xfs: xfs_mkfs.c:2834: align_ag_geometry: Assertion `cfg->agcount != 0' failed.
> 
> This is because align_ag_geometry() finds that the size of the last
> (only) AG is too small, and attempts to trim it off.  Obviously 0
> AGs is invalid, and we hit the ASSERT.
> 
> Fix this by skipping the last-ag-trim if there is only one AG, and
> add a new test to validate_ag_geometry() which offers a very specific,
> clear warning if the device (in dblocks) is smaller than the minimum
> allowed AG size.
> 
> Reported-by: Zdenek Kabelac <zkabelac@redhat.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Seems fine to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> V2: remove stray printf, sorry
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index a687f385..2139aedb 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -1038,6 +1038,15 @@ validate_ag_geometry(
>  	uint64_t	agsize,
>  	uint64_t	agcount)
>  {
> +	/* Is this device simply too small? */
> +	if (dblocks < XFS_AG_MIN_BLOCKS(blocklog)) {
> +		fprintf(stderr,
> +	_("device (%lld blocks) too small, need at least %lld blocks\n"),
> +			(long long)dblocks,
> +			(long long)XFS_AG_MIN_BLOCKS(blocklog));
> +		usage();
> +	}
> +
>  	if (agsize < XFS_AG_MIN_BLOCKS(blocklog)) {
>  		fprintf(stderr,
>  	_("agsize (%lld blocks) too small, need at least %lld blocks\n"),
> @@ -2827,11 +2836,11 @@ validate:
>  	 * and drop the blocks.
>  	 */
>  	if (cfg->dblocks % cfg->agsize != 0 &&
> +	     cfg->agcount > 1 &&
>  	     (cfg->dblocks % cfg->agsize < XFS_AG_MIN_BLOCKS(cfg->blocklog))) {
>  		ASSERT(!cli_opt_set(&dopts, D_AGCOUNT));
>  		cfg->dblocks = (xfs_rfsblock_t)((cfg->agcount - 1) * cfg->agsize);
>  		cfg->agcount--;
> -		ASSERT(cfg->agcount != 0);
>  	}
>  
>  	validate_ag_geometry(cfg->blocklog, cfg->dblocks,
> 
>