On Apr 03, 2024 / 11:00, Daniel Wagner wrote:
> 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).
Actually, similar feature is implemented in the common code so that some
test case can be run twice, once for regular the block device, and one more
time for the zoned block device. You can find test cases with CAN_BE_ZONED=1
flag. They are run twice when RUN_ZONED_TESTS is set in the config.
To be precise, this applies to the test cases with test() function.
CAN_BE_ZONED has different meaning for test cases with test_device().
Now we want to run some of test cases twice for the two nvmet block device
types. This is essentially common feature as the repeated runs for the
CAN_BE_ZONED test cases. I think it's time to generalize these two uses cases
and support "repeated test case runs with different test conditions".
> This could also be useful for nvmet_trtype etc.
Yes, when I run for all of nvmet_trtype variations, I see some test cases are
repeated with same condition. Waste of time.
>
> The generic setup could be requested via the require hook.
>
> requires() = {
>
> _nvmet_setup_target
> }
Hmm, I think this abuses the hook. IMO, it's the better to introduce a new hook.
>
> What do you think about this idea?
It sounds an interesting idea :) I prototyped the common code change based on
the idea and shared it on GitHub [*]. It introduces two new config arrays
NVMET_BLKDEV_TYPES and NVMET_TR_TYPES. When these two are set in config file as
follows,
NVMET_BLKDEV_TYPES=(device file)
NVMET_TR_TYPES=(loop rdma tcp)
it will run a single test case as follows. 2 x 3 = 6 times repeptitions.
$ sudo ./check nvme/006
nvme/006(nvmet dev=device tr=loop)(create an NVMeOF target) [passed]
runtime 0.090s ... 0.091s
nvme/006(nvmet dev=device tr=rdma)(create an NVMeOF target) [passed]
runtime 0.310s ... 0.305s
nvme/006(nvmet dev=device tr=tcp)(create an NVMeOF target) [passed]
runtime 0.149s ... 0.153s
nvme/006(nvmet dev=file tr=loop)(create an NVMeOF target) [passed]
runtime 0.138s ... 0.135s
nvme/006(nvmet dev=file tr=rdma)(create an NVMeOF target) [passed]
runtime 0.300s ... 0.305s
nvme/006(nvmet dev=file tr=tcp)(create an NVMeOF target) [passed]
runtime 0.141s ... 0.147s
I hope this meets your needs.
[*] https://github.com/kawasaki/blktests/tree/conditions
