Re: [PATCH] xfs/157: mkfs does not need a specific fssize

[Dave Chinner <david@fromorbit.com>]

On Mon, Nov 04, 2024 at 03:34:26PM -0800, Darrick J. Wong wrote:
> On Mon, Nov 04, 2024 at 09:04:37PM +0800, Zorro Lang wrote:
> > On Sun, Nov 03, 2024 at 11:50:32PM -0800, Christoph Hellwig wrote:
> > > On Fri, Nov 01, 2024 at 02:49:26PM -0700, Darrick J. Wong wrote:
> > > > > How about unset the MKFS_OPTIONS for this test? As it already tests rtdev
> > > > > and logdev by itself. Or call _notrun if MKFS_OPTIONS has "rmapbt=1"?
> > > > 
> > > > That will exclude quite a few configurations.  Also, how many people
> > > > actually turn on rmapbt explicitly now?
> > > > 
> > > > > Any better idea?
> > > > 
> > > > I'm afraid not.  Maybe I should restructure the test to force the rt
> > > > device to be 500MB even when we're not using the fake rtdev?
> > > 
> > > All of this is really just bandaids or the fundamental problem that:
> > > 
> > >  - we try to abitrarily mix config and test provided options without
> > >    checking that they are compatible in general, and with what the test
> > >    is trying to specifically
> > >  - some combination of options and devices (size, block size, sequential
> > >    required zoned) fundamentally can't work
> > > 
> > > I haven't really found an easy solution for them.  In the long run I
> > > suspect we need to split tests between those that just take the options
> > > from the config and are supposed to work with all options (maybe a few
> > > notruns that fundamentally can't work).  And those that want to test
> > > specific mkfs/mount options and hard code them but don't take options
> > > from the input.
> > 
> > So how about unset extra MKFS_OPTIONS in this case ? This test has its own
> > mkfs options (-L label and logdev and rtdev and fssize).
> 
> The trouble with clearing MKFS_OPTIONS is that you then have to adjust
> the other _scratch_* calls in check_label(), and then all you're doing
> is reducing fs configuration test coverage.  If (say) there was a bug
> when changing the fs label on a rtgroups filesystem with a rt section,
> you'd never see it.

Nobody need to overload MKFS_OPTIONS or unset it.  Local test
configs are supposed to be passed as function parameters, whilst
MKFS_OPTIONS defines the global defaults.

When the two conflict, _scratch_mkfs drops the global MKFS_OPTIONS
and uses only the local parameters so the filesystem is set up with
the configuration the test expects.

In this case, MKFS_OPTIONS="-m rmapbt=1" which conflicts with the
local RTDEV/USE_EXTERNAL test setup. Because the test icurrently
overloads the global MKFS_OPTIONS with local test options, the local
test parameters are dropped along with the global paramters when
there is a conflict. Hence the mkfs_scratch call fails to set the
filesystem up the way the test expects.

IOWs, Zorro's fix is correct and the right one to make - it fixes
the failures here on all the config sections I have that have
configs that aren't compatible with RT devices.

-Dave.
-- 
Dave Chinner
david@fromorbit.com