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

From: Eric Sandeen <sandeen@sandeen.net>
To: Dave Chinner <david@fromorbit.com>, 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: Mon, 14 Sep 2020 18:41:18 -0500	[thread overview]
Message-ID: <26735336-2fe0-d118-1c85-5b0201246b3c@sandeen.net> (raw)
In-Reply-To: <20200914233339.GX12131@dread.disaster.area>

On 9/14/20 6:33 PM, Dave Chinner wrote:
> On Mon, Sep 14, 2020 at 05:29:17PM -0500, Eric Sandeen wrote:
>> On 9/14/20 5:12 PM, Dave Chinner wrote:
>>> 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();
>>>         }
>>
>> No, because we hit an ASSERT before we ever called this validation
>> function.
> 
> Huh, we're supposed to have already validated the data device size
> is larger than the minimum supported before we try to align the Ag
> sizes to the data dev geometry.
> 
>> The error this is trying to fix is essentially: Do not attempt to
>> trim off the last/only AG in the filesystem.
> 
> But trimming *should never happen* for single AG filesystems. If
> we've got dblocks < minimum AG size for a single AG filesystem and
> we are only discovering that when we are doing AG alignment mods,
> then we've -failed to bounds check dblocks correctly-. We should
> have errored out long before we get to aligning AG geometry.....
> 
> Yup, ok, see validate_datadev(), where we do minimum data subvolume
> size checks:
> 
>         if (cfg->dblocks < XFS_MIN_DATA_BLOCKS) {
>                 fprintf(stderr,
> _("size %lld of data subvolume is too small, minimum %d blocks\n"),
>                         (long long)cfg->dblocks, XFS_MIN_DATA_BLOCKS);
>                 usage();
>         }
> 
> .... and there's the bug:
> 
> #define XFS_MIN_DATA_BLOCKS     100

ew.  Ok, I had missed this, yuk.  Thanks, I'll resend.

Thanks,
-Eric

> 
> That's wrong and that's the bug here: minimum data device
> size is 1 whole AG, which means that this should be:
> 
> #define XFS_MIN_DATA_BLOCKS(cfg)	XFS_AG_MIN_BLOCKS((cfg)->blocklog)
> 
> Cheers,
> 
> Dave.
> 