[PATCH blktests v1 0/3] add blkdev type environment variable

[Daniel Wagner]

Hi Shinichiro,

On Wed, Apr 03, 2024 at 04:54:28AM +0000, Shinichiro Kawasaki wrote:
> On the other hand, I see that the series has a couple of drawbacks:
> 
> 1) When blktests users run with the default knob only, the test coverage will be
>    smaller. To keep the current test coverage, the users need to run the check
>    script twice: nvmet_blkdev_type=file and nvmet_blkdev_type=device. Some users
>    may not do it and lose the test coverage. And some users, e.g., CKI project,
>    need to adjust their script for this change.
> 
> 2) When the users run the check script twice to keep the test coverage, some
>    test cases are executed twice under the exact same test conditions. This
>    will waste some of the test run effort.

Yes, I agree. These drawbacks should be addressed somehow.

> To avoid the drawbacks, how about this?
> 
> - Do not provide nvmet_blkdev_type as a knob for users. Keep it as just a global
>   variable in tests/nvme/rc. (It should be renamed to clarify that it is not for
>   users.)
> 
> - Introduce a helper function to do the same test twice for nvmet_blkdev_type=
>   file and nvmet_blkdev_type=device. Call this helper function from a single
>   test case to cover both the blkdev types.
> 
> I attach a patch at the end of this email to show the ideas above. It applies
> the idea to nvme/006 as an example. What do you think?

Ideally we don't have to introduce additional common setup logic into
each test. Also for debugging purpose it might sense to run a test only
in one configuration. So it might make sense still to have user visible
environment variable

  nvmet_blkdev_types="file device"

as default.

> -test() {
> -	echo "Running ${TEST_NAME}"
> -
> +do_test() {
>  	_setup_nvmet
>  
>  	_nvmet_target_setup
>  
>  	_nvmet_target_cleanup
> +}
> +
> +test() {
> +	echo "Running ${TEST_NAME}"
> +
> +	_nvmet_run_for_each_blkdev_type do_test

I was wondering if the nvmet_run_for_each_blkdev_type logic could be in
common code, so we don't have to add a do_test function. Basically
having a common code for a bunch of configuration variables (matrix
tests). This could also be useful for nvmet_trtype etc.

The generic setup could be requested via the require hook.

requires() = {

 _nvmet_setup_target
}

What do you think about this idea?

Thanks,
Daniel