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

From: Dave Chinner <david@fromorbit.com>
To: Eric Sandeen <sandeen@redhat.com>
Cc: xfs <linux-xfs@vger.kernel.org>, Zdenek Kabelac <zkabelac@redhat.com>
Subject: Re: [PATCH] mkfs.xfs: fix ASSERT on too-small device with stripe geometry
Date: Tue, 15 Sep 2020 08:12:01 +1000	[thread overview]
Message-ID: <20200914221201.GW12131@dread.disaster.area> (raw)
In-Reply-To: <f06e8b9a-d5c8-f91f-8637-0b9f625d9d48@redhat.com>

On Mon, Sep 14, 2020 at 01:26:01PM -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>
> ---
> 
> 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();
> +	}

Ummm, shouldn't this be caught two checks later down by this:

	if (agsize > dblocks) {
               fprintf(stderr,
        _("agsize (%lld blocks) too big, data area is %lld blocks\n"),
                        (long long)agsize, (long long)dblocks);
                        usage();
        }

because the agsize has already been validated to be within
XFS_AG_MIN_BLOCKS() and XFS_AG_MAX_BLOCKS(), so if dblocks is only
10MB then the agsize must be greater than dblocks as the minimum
valid AG size is 16MB....

Also, what's with the repeated agsize < XFS_AG_MIN_BLOCKS(blocklog)
and agsize > XFS_AG_MAX_BLOCKS(blocklog) checks in that function?

> +
>  	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);
>  	}

We should never get here - this assert and code check is correct and
valid - it's pointed us directly to a logic bug in mkfs, so IMO
it should not be changed/removed.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com