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
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@xxxxxxxxxxxxx
