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

[Dave Chinner <david@fromorbit.com>]

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