Re: [PATCH v2 49/49] multipath-tools tests: fix directio test with real device

[Benjamin Marzinski <bmarzins@redhat.com>]

On Fri, Jul 12, 2024 at 07:14:57PM +0200, Martin Wilck wrote:
> Allow setting DIO_TEST_DEV during runtime, by reading the environment
> variable. The test was fragile despite the delay, because the real
> io_getevents() call isn't guaranteed to return the number of events
> requested. Fix that. Moreover, allow reading DIO_TEST_DELAY (in us)
> from the environment. With the io_getevents fix, for me the test
> succeeded even with 0 us delay.
> 
> Change the README accordingly.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  tests/Makefile   |   8 ---
>  tests/README.md  |  29 ++++++--
>  tests/directio.c | 182 ++++++++++++++++++++++++++---------------------
>  3 files changed, 122 insertions(+), 97 deletions(-)
> 
> diff --git a/tests/Makefile b/tests/Makefile
> index 55fbf0f..02580e7 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -21,12 +21,6 @@ valgrind:	$(TESTS:%=%.vgr)
>  # test-specific compiler flags
>  # XYZ-test_FLAGS: Additional compiler flags for this test
>  
> -ifneq ($(wildcard directio_test_dev),)
> -DIO_TEST_DEV = $(shell sed -n -e 's/^[[:space:]]*DIO_TEST_DEV[[:space:]]*=[[:space:]]*\([^[:space:]\#]\+\).*/\1/p' < directio_test_dev)
> -endif
> -ifneq ($(DIO_TEST_DEV),)
> -directio-test_FLAGS := -DDIO_TEST_DEV=\"$(DIO_TEST_DEV)\"
> -endif
>  mpathvalid-test_FLAGS := -I$(mpathvaliddir)
>  features-test_FLAGS := -I$(multipathdir)/nvme
>  
> @@ -59,9 +53,7 @@ valid-test_LIBDEPS := -lmount -ludev -lpthread -ldl
>  devt-test_LIBDEPS := -ludev
>  mpathvalid-test_LIBDEPS := -ludev -lpthread -ldl
>  mpathvalid-test_OBJDEPS := $(mpathvaliddir)/mpath_valid.o
> -ifneq ($(DIO_TEST_DEV),)
>  directio-test_LIBDEPS := -laio
> -endif
>  strbuf-test_OBJDEPS := $(mpathutildir)/strbuf.o
>  sysfs-test_TESTDEPS := test-log.o
>  sysfs-test_OBJDEPS := $(multipathdir)/sysfs.o $(mpathutildir)/util.o
> diff --git a/tests/README.md b/tests/README.md
> index fd36fc1..0cae057 100644
> --- a/tests/README.md
> +++ b/tests/README.md
> @@ -13,6 +13,17 @@ If valgrind detects a bad memory access or leak, the test will fail. The
>  output of the test run, including valgrind output, is stored as
>  `<testname>.vgr`.
>  
> +## Running tests manually
> +
> +`make test` or `make -C test "$TEST.out"` will only run the test program if
> +the output files `$TEST.out` don't exist yet. To re-run the test, delete the
> +output file first. In order to run a test outside `make`, set the library
> +search path:
> +
> +    cd tests
> +    export LD_LIBRARY_PATH=.:../libmpathutil:../libmpathcmd
> +	./dmevents-test  # or whatever other test you want to run
> +
>  ## Controlling verbosity for unit tests
>  
>  Some test programs use the environment variable `MPATHTEST_VERBOSITY` to
> @@ -37,15 +48,21 @@ This test includes test items that require a access to a block device. The
>  device will be opened in read-only mode; you don't need to worry about data
>  loss. However, the user needs to specify a device to be used. Set the
>  environment variable `DIO_TEST_DEV` to the path of the device.
> -Alternatively, create a file `directio_test_dev` under
> -the `tests` directory containing a single line that sets this environment
> -variable in Bourne Shell syntax, like this:
> -
> -    DIO_TEST_DEV=/dev/sdc3
> -
>  After that, run `make directio.out` as root in the `tests` directory to
>  perform the test.
>  
> +With a real test device, the test results may note be 100% reproducible,
> +and sporadic test failures may occur under certain circumstances.
> +It may be necessary to introduce a certain delay between test
> +operations. To do so, set the environment variable `DIO_TEST_DELAY` to a
> +positive integer that determines the delay (in microseconds) after each
> +`io_submit()` operation. The default delay is 10 microseconds.
> +
> +*Note:* `DIO_TEST_DEV` doesn't have to be set during compilation of
> +`directio-test`. This used to be the case in previous versions of
> +multipath-tools. Previously, it was possible to set `DIO_TEST_DEV` in a file
> +`tests/directio_test_dev`. This is not supported any more.
> +
>  ## Adding tests
>  
>  The unit tests are based on the [cmocka test framework](https://cmocka.org/),
> diff --git a/tests/directio.c b/tests/directio.c
> index d5f84f1..763929e 100644
> --- a/tests/directio.c
> +++ b/tests/directio.c
> @@ -34,6 +34,8 @@ struct io_event mock_events[AIO_GROUP_SIZE]; /* same as the checker max */
>  int ev_off = 0;
>  struct timespec zero_timeout = { .tv_sec = 0 };
>  struct timespec full_timeout = { .tv_sec = -1 };
> +const char *test_dev = NULL;
> +unsigned int test_delay = 10000;
>  
>  #ifdef __GLIBC__
>  #define ioctl_request_t unsigned long
> @@ -45,12 +47,13 @@ int REAL_IOCTL(int fd, ioctl_request_t request, void *argp);
>  
>  int WRAP_IOCTL(int fd, ioctl_request_t request, void *argp)
>  {
> -#ifdef DIO_TEST_DEV
> -	mock_type(int);
> -	return REAL_IOCTL(fd, request, argp);
> -#else
>  	int *blocksize = (int *)argp;
>  
> +	if (test_dev) {
> +		mock_type(int);
> +		return REAL_IOCTL(fd, request, argp);
> +	}
> +
>  	assert_int_equal(fd, test_fd);
>  	/*
>  	 * On MUSL libc, the "request" arg is an int (glibc: unsigned long).
> @@ -64,88 +67,80 @@ int WRAP_IOCTL(int fd, ioctl_request_t request, void *argp)
>  	assert_non_null(blocksize);
>  	*blocksize = mock_type(int);
>  	return 0;
> -#endif
>  }
>  
>  int REAL_FCNTL (int fd, int cmd, long arg);
>  
>  int WRAP_FCNTL (int fd, int cmd, long arg)
>  {
> -#ifdef DIO_TEST_DEV
> -	return REAL_FCNTL(fd, cmd, arg);
> -#else
> +	if (test_dev)
> +		return REAL_FCNTL(fd, cmd, arg);
>  	assert_int_equal(fd, test_fd);
>  	assert_int_equal(cmd, F_GETFL);
>  	return O_DIRECT;
> -#endif
>  }
>  
>  int __real___fxstat(int ver, int fd, struct stat *statbuf);
>  
>  int __wrap___fxstat(int ver, int fd, struct stat *statbuf)
>  {
> -#ifdef DIO_TEST_DEV
> -	return __real___fxstat(ver, fd, statbuf);
> -#else
> +	if (test_dev)
> +		return __real___fxstat(ver, fd, statbuf);
> +
>  	assert_int_equal(fd, test_fd);
>  	assert_non_null(statbuf);
>  	memset(statbuf, 0, sizeof(struct stat));
>  	return 0;
> -#endif
> +
>  }
>  
>  int __real_io_setup(int maxevents, io_context_t *ctxp);
>  
>  int __wrap_io_setup(int maxevents, io_context_t *ctxp)
>  {
> -	ioctx_count++;
> -#ifdef DIO_TEST_DEV
>  	int ret = mock_type(int);
> -	assert_int_equal(ret, __real_io_setup(maxevents, ctxp));
> +
> +	if (test_dev)
> +		assert_int_equal(ret, __real_io_setup(maxevents, ctxp));
> +	ioctx_count++;
>  	return ret;
> -#else
> -	return mock_type(int);
> -#endif
>  }
>  
>  int __real_io_destroy(io_context_t ctx);
>  
>  int __wrap_io_destroy(io_context_t ctx)
>  {
> -	ioctx_count--;
> -#ifdef DIO_TEST_DEV
>  	int ret = mock_type(int);
> -	assert_int_equal(ret, __real_io_destroy(ctx));
> +
> +	ioctx_count--;
> +	if (test_dev)
> +		assert_int_equal(ret, __real_io_destroy(ctx));
> +
>  	return ret;
> -#else
> -	return mock_type(int);
> -#endif
>  }
>  
>  int __real_io_submit(io_context_t ctx, long nr, struct iocb *ios[]);
>  
>  int __wrap_io_submit(io_context_t ctx, long nr, struct iocb *ios[])
>  {
> -#ifdef DIO_TEST_DEV
> -	struct timespec dev_delay = { .tv_nsec = 100000 };
>  	int ret = mock_type(int);
> -	assert_int_equal(ret, __real_io_submit(ctx, nr, ios));
> -	nanosleep(&dev_delay, NULL);
> +
> +	if (test_dev) {
> +		struct timespec dev_delay = { .tv_nsec = test_delay };
> +		assert_int_equal(ret, __real_io_submit(ctx, nr, ios));
> +		nanosleep(&dev_delay, NULL);
> +	}
>  	return ret;
> -#else
> -	return mock_type(int);
> -#endif
>  }
>  
>  int __real_io_cancel(io_context_t ctx, struct iocb *iocb, struct io_event *evt);
>  
>  int __wrap_io_cancel(io_context_t ctx, struct iocb *iocb, struct io_event *evt)
>  {
> -#ifdef DIO_TEST_DEV
> -	return __real_io_cancel(ctx, iocb, evt);
> -#else
> -	return 0;
> -#endif
> +	if (test_dev)
> +		return __real_io_cancel(ctx, iocb, evt);
> +	else
> +		return 0;
>  }
>  
>  int REAL_IO_GETEVENTS(io_context_t ctx, long min_nr, long nr,
> @@ -155,38 +150,43 @@ int WRAP_IO_GETEVENTS(io_context_t ctx, long min_nr, long nr,
>  			struct io_event *events, struct timespec *timeout)
>  {
>  	int nr_evs;
> -#ifndef DIO_TEST_DEV
>  	struct timespec *sleep_tmo;
>  	int i;
>  	struct io_event *evs;
> -#endif
>  
>  	assert_non_null(timeout);
>  	nr_evs = mock_type(int);
>  	assert_true(nr_evs <= nr);
>  	if (!nr_evs)
>  		return 0;
> -#ifdef DIO_TEST_DEV
> -	mock_ptr_type(struct timespec *);
> -	mock_ptr_type(struct io_event *);
> -	assert_int_equal(nr_evs, REAL_IO_GETEVENTS(ctx, min_nr, nr_evs,
> -						   events, timeout));
> -#else
> -	sleep_tmo = mock_ptr_type(struct timespec *);
> -	if (sleep_tmo) {
> -		if (sleep_tmo->tv_sec < 0)
> -			nanosleep(timeout, NULL);
> -		else
> -			nanosleep(sleep_tmo, NULL);
> +	if (test_dev) {
> +		int n = 0;
> +		mock_ptr_type(struct timespec *);
> +		mock_ptr_type(struct io_event *);
> +
> +		condlog(2, "min_nr = %ld nr_evs = %d", min_nr, nr_evs);
> +		while (n < nr_evs) {
> +			min_nr = min_nr <= nr_evs - n ? min_nr : nr_evs - n;
> +			n += REAL_IO_GETEVENTS(ctx, min_nr, nr_evs - n,
> +					       events + n, timeout);
> +		}
> +		assert_int_equal(nr_evs, n);
> +	} else {
> +		sleep_tmo = mock_ptr_type(struct timespec *);
> +		if (sleep_tmo) {
> +			if (sleep_tmo->tv_sec < 0)
> +				nanosleep(timeout, NULL);
> +			else
> +				nanosleep(sleep_tmo, NULL);
> +		}
> +		if (nr_evs < 0) {
> +			errno = -nr_evs;
> +			return -1;
> +		}
> +		evs = mock_ptr_type(struct io_event *);
> +		for (i = 0; i < nr_evs; i++)
> +			events[i] = evs[i];
>  	}
> -	if (nr_evs < 0) {
> -		errno = -nr_evs;
> -		return -1;
> -	}
> -	evs = mock_ptr_type(struct io_event *);
> -	for (i = 0; i < nr_evs; i++)
> -		events[i] = evs[i];
> -#endif
>  	ev_off -= nr_evs;
>  	return nr_evs;
>  }
> @@ -259,10 +259,9 @@ static void do_libcheck_init(struct checker *c, int blocksize,
>  	assert_non_null(ct->req);
>  	if (req)
>  		*req = ct->req;
> -#ifndef DIO_TEST_DEV
> -	/* don't check fake blocksize on real devices */
> -	assert_int_equal(ct->req->blksize, blocksize);
> -#endif
> +	if (!test_dev)
> +		/* don't check fake blocksize on real devices */
> +		assert_int_equal(ct->req->blksize, blocksize);
>  }
>  
>  static int is_checker_running(struct checker *c)
> @@ -583,11 +582,11 @@ static void test_async_timeout_cancel_failed(void **state)
>  	do_check_state(&c[1], 0, 2, PATH_PENDING);
>  	return_io_getevents_none();
>  	do_check_state(&c[0], 0, 2, PATH_DOWN);
> -#ifndef DIO_TEST_DEV
> -	/* can't pick which even gets returned on real devices */
> -	return_io_getevents_nr(NULL, 1, &reqs[1], &res[1]);
> -	do_check_state(&c[1], 0, 2, PATH_UP);
> -#endif
> +	if (!test_dev) {
> +		/* can't pick which even gets returned on real devices */
> +		return_io_getevents_nr(NULL, 1, &reqs[1], &res[1]);
> +		do_check_state(&c[1], 0, 2, PATH_UP);
> +	}
>  	return_io_getevents_none();
>  	do_check_state(&c[0], 0, 2, PATH_DOWN);
>  	assert_true(is_checker_running(&c[0]));
> @@ -663,12 +662,11 @@ static void test_check_state_blksize(void **state)
>  	int blksize[] = {4096, 1024, 512};
>  	struct async_req *reqs[3];
>  	int res[] = {0,1,0};
> -#ifdef DIO_TEST_DEV
> -	/* can't pick event return state on real devices */
>  	int chk_state[] = {PATH_UP, PATH_UP, PATH_UP};
> -#else
> -	int chk_state[] = {PATH_UP, PATH_DOWN, PATH_UP};
> -#endif
> +
> +	/* can't pick event return state on real devices */
> +	if (!test_dev)
> +		chk_state[1] = PATH_DOWN;
>  
>  	assert_true(list_empty(&aio_grp_list));
>  	will_return(__wrap_io_setup, 0);
> @@ -718,20 +716,38 @@ static void test_check_state_async(void **state)
>  
>  static int setup(void **state)
>  {
> -#ifdef DIO_TEST_DEV
> -	test_fd = open(DIO_TEST_DEV, O_RDONLY);
> -	if (test_fd < 0)
> -		fail_msg("cannot open %s: %m", DIO_TEST_DEV);
> -#endif
> +	char *dl = getenv("DIO_TEST_DELAY");
> +	test_dev = getenv("DIO_TEST_DEV");
> +
> +	if (test_dev) {
> +		condlog(2, "%s: opening %s", __func__, test_dev);
> +		test_fd = open(test_dev, O_RDONLY);
> +		if (dl) {
> +			char *e;
> +			long int d = strtol(dl, &e, 10);
> +
> +			if (*e == '\0' && d >= 0 && (d * 1000) < (long)UINT_MAX)
> +				test_delay = d * 1000;
> +			else {
> +				condlog(1, "DIO_TEST_DELAY=%s is invalid", dl);
> +				return 1;
> +			}
> +		}
> +		condlog(2, "%s: delay = %u us", __func__, test_delay / 1000);
> +	}
> +	if (test_fd < 0) {
> +		fail_msg("cannot open %s: %m", test_dev);
> +		return 1;
> +	}
>  	return 0;
>  }
>  
>  static int teardown(void **state)
>  {
> -#ifdef DIO_TEST_DEV
> -	assert_true(test_fd > 0);
> -	assert_int_equal(close(test_fd), 0);
> -#endif
> +	if (test_dev) {
> +		assert_true(test_fd > 0);
> +		assert_int_equal(close(test_fd), 0);
> +	}
>  	return 0;
>  }
>  
> @@ -762,7 +778,7 @@ int main(void)
>  {
>  	int ret = 0;
>  
> -	init_test_verbosity(5);
> +	init_test_verbosity(2);
>  	ret += test_directio();
>  	return ret;
>  }
> -- 
> 2.45.2