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@xxxxxxxxxx>
>>>> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
>>>> ---
>>>>
>>>> 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.
>
