* [PATCH] mkfs.xfs: fix ASSERT on too-small device with stripe geometry
@ 2020-09-14 18:26 Eric Sandeen
2020-09-14 18:58 ` Darrick J. Wong
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Eric Sandeen @ 2020-09-14 18:26 UTC (permalink / raw)
To: xfs; +Cc: Zdenek Kabelac
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,
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] mkfs.xfs: fix ASSERT on too-small device with stripe geometry
2020-09-14 18:26 [PATCH] mkfs.xfs: fix ASSERT on too-small device with stripe geometry Eric Sandeen
@ 2020-09-14 18:58 ` Darrick J. Wong
2020-09-14 19:00 ` [PATCH V2] " Eric Sandeen
2020-09-14 22:12 ` [PATCH] " Dave Chinner
2 siblings, 0 replies; 8+ messages in thread
From: Darrick J. Wong @ 2020-09-14 18:58 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs, Zdenek Kabelac
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();
> + }
> +
> 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);
What is this?
(The rest of the logic looks fine)
--D
> 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,
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH V2] mkfs.xfs: fix ASSERT on too-small device with stripe geometry
2020-09-14 18:26 [PATCH] mkfs.xfs: fix ASSERT on too-small device with stripe geometry Eric Sandeen
2020-09-14 18:58 ` Darrick J. Wong
@ 2020-09-14 19:00 ` Eric Sandeen
2020-09-14 19:24 ` Darrick J. Wong
2020-09-14 22:12 ` [PATCH] " Dave Chinner
2 siblings, 1 reply; 8+ messages in thread
From: Eric Sandeen @ 2020-09-14 19:00 UTC (permalink / raw)
To: xfs; +Cc: Zdenek Kabelac
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,
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH V2] mkfs.xfs: fix ASSERT on too-small device with stripe geometry
2020-09-14 19:00 ` [PATCH V2] " Eric Sandeen
@ 2020-09-14 19:24 ` Darrick J. Wong
0 siblings, 0 replies; 8+ messages in thread
From: Darrick J. Wong @ 2020-09-14 19:24 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs, Zdenek Kabelac
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,
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mkfs.xfs: fix ASSERT on too-small device with stripe geometry
2020-09-14 18:26 [PATCH] mkfs.xfs: fix ASSERT on too-small device with stripe geometry Eric Sandeen
2020-09-14 18:58 ` Darrick J. Wong
2020-09-14 19:00 ` [PATCH V2] " Eric Sandeen
@ 2020-09-14 22:12 ` Dave Chinner
2020-09-14 22:29 ` Eric Sandeen
2 siblings, 1 reply; 8+ messages in thread
From: Dave Chinner @ 2020-09-14 22:12 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs, Zdenek Kabelac
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mkfs.xfs: fix ASSERT on too-small device with stripe geometry
2020-09-14 22:12 ` [PATCH] " Dave Chinner
@ 2020-09-14 22:29 ` Eric Sandeen
2020-09-14 23:33 ` Dave Chinner
0 siblings, 1 reply; 8+ messages in thread
From: Eric Sandeen @ 2020-09-14 22:29 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs, Zdenek Kabelac
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.
The error this is trying to fix is essentially: Do not attempt to
trim off the last/only AG in the filesystem.
-Eric
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mkfs.xfs: fix ASSERT on too-small device with stripe geometry
2020-09-14 22:29 ` Eric Sandeen
@ 2020-09-14 23:33 ` Dave Chinner
2020-09-14 23:41 ` Eric Sandeen
0 siblings, 1 reply; 8+ messages in thread
From: Dave Chinner @ 2020-09-14 23:33 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs, Zdenek Kabelac
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
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.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mkfs.xfs: fix ASSERT on too-small device with stripe geometry
2020-09-14 23:33 ` Dave Chinner
@ 2020-09-14 23:41 ` Eric Sandeen
0 siblings, 0 replies; 8+ messages in thread
From: Eric Sandeen @ 2020-09-14 23:41 UTC (permalink / raw)
To: Dave Chinner, Eric Sandeen; +Cc: xfs, Zdenek Kabelac
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.
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-09-14 23:41 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-14 18:26 [PATCH] mkfs.xfs: fix ASSERT on too-small device with stripe geometry Eric Sandeen
2020-09-14 18:58 ` Darrick J. Wong
2020-09-14 19:00 ` [PATCH V2] " Eric Sandeen
2020-09-14 19:24 ` Darrick J. Wong
2020-09-14 22:12 ` [PATCH] " Dave Chinner
2020-09-14 22:29 ` Eric Sandeen
2020-09-14 23:33 ` Dave Chinner
2020-09-14 23:41 ` Eric Sandeen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.