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

["Darrick J. Wong" <djwong@kernel.org>]

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.

Hang on ... is this a general complaint about _scratch_mkfs_xfs dropping
MKFS_OPTIONS in favor of the specific arguments passed to it by the
test?  Or a specific complaint about xfs/157 barfing when the test
runner puts "-L foo" in the MKFS_OPTIONS that it passes to ./check?

If it's the second, then let's do this:

extract_mkfs_label() {
	set -- $MKFS_OPTIONS
	local in_l

	for arg in "$@"; do
		if [ "$in_l" = "1" ]; then
			echo "$arg"
			return 0
		elif [ "$arg" = "-L" ]; then
			in_l=1
		fi
	done
	return 1
}

check_label() {
	local existing_label
	local filter

	# Handle -L somelabel being set in MKFS_OPTIONS
	if existing_label="$(extract_mkfs_label)"; then
		filter=(sed -e "s|$existing_label|oldlabel|g")
		_scratch_mkfs_sized $fs_size >> $seqres.full
	else
		filter=(cat)
		MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" \
			_scratch_mkfs_sized $fs_size >> $seqres.full
	fi
	_scratch_xfs_db -c label | ${filter[@]}
	_scratch_xfs_admin -L newlabel "$@" >> $seqres.full
	_scratch_xfs_db -c label
	_scratch_xfs_repair -n &>> $seqres.full || echo "Check failed?"
}

--D

> Thanks,
> Zorro
> 
> > 
> > 
> 
>