Re: [PATCH 13/28] check-parallel: introduce config file support

[Dave Chinner <david@fromorbit.com>]

On Fri, May 09, 2025 at 05:31:49PM +0530, Nirjhar Roy wrote:
> On Thu, 2025-04-17 at 13:00 +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > check-parallel will use the same config file format check does,
> > and use the same code to auto-discover the config file.
> > 
> > The biggest difference is that check-parallel -requires- the use
> > of config sections, and the first section *must* be named
> > "[check-parallel]". This first section is used for defining
> > setup parameters for check parallel - loop device image file sizes,
> > etc.
> > 
> > The second biggest difference is that check-parallel does not allow
> > the config file to define devices. Any section found to contain a
> > device definition such as TEST_DEV or SCRATCH_DEV will result
> > check-parallel terminating with an error.
> > 
> > This config file format works for check-parallel invoking check,
> > too, because once a section is specified on the check command line,
> > it effectively ignores unknown values set in sections that it
> > doesn't run.  Hence it effectively skips over the [check-parallel]
> > setup section.
> > 
> > For check-parallel, each config section now defines just the
> > filesystem configuration to be tested; all the usual mount and mkfs
> > options apply, and USE_EXTERNAL must be set for testing external
> > devices.
> > 
> > This commit implements the initial [check-parallel] section support
> > and moves the build in default values for these parameters to the
> > config file setup. This means if the config file does not contain
> > all the necessary parameter values, a default value will be used for
> > it.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
....

> > @@ -388,3 +408,51 @@ _config_section_setup()
> >  		fi
> >  	fi
> >  }
> > +
> > +# check-parallel config files must:
> > +# 1. have config sections defined
> > +# 2. use the first config section for check-parallel setup
> > +# 3. not define any physical device parameter in any section
> Nit: Maybe some documentation of the above restrictions and some new
> variables introduced (like SCRATCH_DEV_SIZE, ..)in the README?

Eventually, yes. Right now stuff is still in a state of flux, so
there is no point in adding extra documentation that has to be kept
up to date.

> > +# If all these are true, then we read the first section that defines
> > +# the check-parallel config parameters and continue onwards.
> > +_config_setup_parallel()
> > +{
> > +	if [ "$iam" != "check-parallel" ]; then
> > +		echo "$iam: incorrect config file format chosen!"
> > +		exit 1;
> > +	fi
> > +
> > +	_config_file_setup
> > +
> > +	if [ "$OPTIONS_HAVE_SECTIONS" != "true" ]; then
> Most of the places the way OPTIONS_HAVE_SECTIONS is being used is as 
> if $OPTIONS_HAVE_SECTIONS; then 
>     ...
> fi
> 
> so, when OPTIONS_HAVE_SECTIONS=true, /bin/true is executed and the exit
> code of /bin/true is always a success and it is not a string comparison
> with "true" or "false".

Yes, I know.

Executing /bin/true requires creating a new process just to return
a successful exit code. This takes several milliseconds of CPU time
to create and tear down a new process context.

OTOH, doing a string comparison in the shell parser takes a couple
of microseconds of CPU time....

Which is faster and burns less energy?

>
> /bin/true always succeeds and hence if
> OPTIONS_HAVE_SECTIONS=true, then 
> if $OPTIONS_HAVE_SECTIONS is always executed. In the same way
> /bin/false always has a failure code upon exit.
> So maybe we should change this to 
> if $OPTIONS_HAVE_SECTIONS; then - just to have consistency with the
> rest of the code?
> > +		echo "$iam config file has no sections!"
> > +		exit 1;
> > +	fi
> > +
> > +	local first_section=`echo $HOST_OPTIONS_SECTIONS | cut -f1 -d" "`
> > +	if [ "$first_section" != "$iam" ]; then
> > +		echo "$iam config file has no [$iam] section"
> > +		exit 1
> > +	fi
> > +
> > +	grep DEV $HOST_OPTIONS |grep -qv SIZE
> This will incorrectly fail the check-parallel invocation even if we
> have the devices defined in the comment section - so something like the
> following is causing a failure
> 
> local.config >>
> [check-parallel]
> TEST_DEV_SIZE=2G
> #TEST_DEV=/dev/loop0

Don't put devices in the check-parallel config file.

> Maybe we should try to ignore the lines that begin with '#'
> something like - grep -v '^[[:space:]]*#' $HOST_OPTIONS | grep DEV |
> grep -qv SIZE ? 
> 
> Another case:
> 
> Let us consider another local.config file:
> 
> [check-parallel]
> TEST_DEV_SIZE=2G
> 
> [xfs_4k]
> TEST_DEV=/dev/loop0
> TEST_DIR=/mnt1/test
> SCRATCH_DEV=/dev/loop1
> SCRATCH_MNT=/mnt1/scratch
> 
> The above file runs fine ./check -s xfs_4k selftest/001

Yes, but it is an invalid check-parallel config file because it has
devices defined in it.

If check-parallel allows this, then when it runs a check instance
it will override the environment defines set by the check-parallel
runner and every runner will try to use the same devices and mount
points.

It just doesn't work.

> However, with check-parallel, it will fail and it is expected according
> to the design. But is there any specific reason to fail when the above
> configuration is perfectly suitable to run check-parallel (we have
> check-parallel, section defined)? So 
> 
> ./check-parallel -s check-parralel -D /mnt1 -x stress -t 1 selftest/001
> check-parallel config file has devices defined
>
> Instead of grepping on the entire $HOST_OPTIONS, how about we only grep
> on the check-parallel section? In this way we parse/export the
> environment variables only in check-parallel section and ignore the
> sections that have devices defined and/or check-parallel incompatible.
> ./check -s xfs_4k works anyway with [check-parallel] section defined.
> ./check -s check-parallel will fail with *DEVs not defined which is
> self explanatory.

I think you may have misunderstood the direction check-parallel is
going in.

I don't want check and check-parallel to share the same config file.
What I need is for them to use the same file format and hence be
able to share parser code.

Later in the patchset I remove the dependency that check-parallel
has on check, and at that point the check-parallel config file no
longer needs to work with check.

However, there is an intermediate period in the patchset where
check-parallel calls check, and so the config file for
check-parallel has to be compatible with check for at least that
short period in time.

To make that work, the check-parallel config file cannot have any
devices or mount points defined anywhere in it, otherwise check will
override the values that check-parallel has provided via the
environment. We don't need to over-engineer this check - if there
are any known devices defined in the config file, abort.

If it gets a false positive, I don't care because once
check-parallel no longer calls check, the "no devices in
check-parallel config file" rule becomes irrelevant because the
config files are no longer shared. At this point the config file
supports only the subset of parameters that check-parallel defines
as valid, and no more. This set of valid parameters for
check-parallel will be enumerated in future patches.

Hence all we have to do at this point is make sure that the section
parser continues to work correctly for both test runners, not try to
invent the One True Config File To Rule Them All....

> > +	parse_config_section $1
> What value is $1 holding? _config_setup_parallel is being called only
> from check-parallel without any parameters passed, right? Did you mean
> _config_setup_parallel $first_section ?

Yup. Fixed.

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