Re: [LTP] [PATCH v6] io_submit04: Add test case for RWF_NOWAIT flag

[Petr Vorel <pvorel@suse.cz>]

Hi Wei, all,

> +++ b/configure.ac
> @@ -172,6 +172,7 @@ AC_CHECK_FUNCS_ONCE([ \
>  ])
>  AC_CHECK_FUNCS(mkdtemp,[],AC_MSG_ERROR(mkdtemp() not found!))

> +AC_CHECK_MEMBERS([struct iocb.aio_rw_flags],,,[#include <linux/aio_abi.h>])
FYI I double checked this is from 4.13
9830f4be159b2 ("fs: Use RWF_* flags for AIO operations")

So we need it, but it'd be worth to double check how the test run on 4.4 (if it
TCONF).  Also maybe add a comment in the code or at least mention it in the
commit message.  When we drop old SLES we will be able to remove this.

@Andrea: any hint for your agents to check supported versions
https://linux-test-project.readthedocs.io/en/latest/users/supported_systems.html#kernel-version
and whether it needs to have fallback in lapi and configure.ac?
Also we may ask not to add .min_kver which would be too old.

>  AC_CHECK_MEMBERS([struct fanotify_event_info_fid.fsid.__val],,,[#include <sys/fanotify.h>])
>  AC_CHECK_MEMBERS([struct perf_event_mmap_page.aux_head],,,[#include <linux/perf_event.h>])
>  AC_CHECK_MEMBERS([struct sigaction.sa_sigaction],[],[],[#include <signal.h>])
> diff --git a/include/lapi/aio_abi.h b/include/lapi/aio_abi.h
> new file mode 100644
> index 000000000..ac78e5500
> --- /dev/null
> +++ b/include/lapi/aio_abi.h
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2025 Wei Gao <wegao@suse.com>
> + */
> +
> +#ifndef LAPI_AIO_ABI_H__
> +#define LAPI_AIO_ABI_H__
> +
> +#include <endian.h>
> +#include <linux/aio_abi.h>
> +
> +#ifndef RWF_NOWAIT
> +# define RWF_NOWAIT 0x00000008
And this as well.
b745fafaf70c0 ("fs: Introduce RWF_NOWAIT and FMODE_AIO_NOWAIT")

> +#endif
> +
> +struct iocb_fallback {
> +	uint64_t aio_data;
> +#if __BYTE_ORDER == __LITTLE_ENDIAN
> +	uint32_t   aio_key;
> +	uint32_t aio_rw_flags;
> +#elif __BYTE_ORDER == __BIG_ENDIAN
> +	uint32_t aio_rw_flags;
> +	uint32_t   aio_key;
> +#else
> +#error edit for your odd byteorder.
> +#endif
> +	uint16_t   aio_lio_opcode;
> +	int16_t   aio_reqprio;
> +	uint32_t   aio_fildes;
> +	uint64_t   aio_buf;
> +	uint64_t   aio_nbytes;
> +	int64_t   aio_offset;
> +	uint64_t   aio_reserved2;
> +	uint32_t   aio_flags;
> +	uint32_t   aio_resfd;
> +};
> +
> +#ifndef HAVE_STRUCT_IOCB_AIO_RW_FLAGS
> +typedef struct iocb_fallback iocb;
> +#else
> +typedef struct iocb iocb;
> +#endif
> +
> +#endif /* LAPI_AIO_ABI_H__ */
...

> +++ b/testcases/kernel/syscalls/io_submit/io_submit04.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2025 Wei Gao <wegao@suse.com>
> + */
> +
> +/*\
> + * Test RWF_NOWAIT support in io_submit(), verifying that an
> + * asynchronous read operation on a blocking resource (empty pipe)
> + * will cause -EAGAIN. This is done by checking that io_getevents()
Please :manpage:`io_getevents(2)`
(to have link to https://man7.org/linux/man-pages/man2/io_getevents.2.html in
docs).

@Andrea: ^ to agents?

> + * syscall returns immediately and io_event.res is equal to -EAGAIN.
> + */
> +
> +#include "config.h"
> +#include "tst_test.h"
> +#include "lapi/syscalls.h"
> +#include "lapi/aio_abi.h"
> +
> +#define BUF_SIZE 100
> +
> +static int fd[2] = {-1, -1};
nit: Andrea (I guess you asked for it), because we hardly close STDOUT in the
test 0 is always used. I see it a bit overkill using -1 (using 0 as the default
would almost always work).

> +static aio_context_t ctx;
> +static char *buf;
> +static iocb *cb;
> +static iocb **iocbs;
> +
> +static void setup(void)
> +{
> +	if (tst_syscall(__NR_io_setup, 1, &ctx))
> +		tst_brk(TBROK | TERRNO, "io_setup failed");
> +
> +	SAFE_PIPE(fd);
> +
> +	cb->aio_fildes = fd[0];
> +	cb->aio_lio_opcode = IOCB_CMD_PREAD;
> +	cb->aio_buf = (uint64_t)buf;
> +	cb->aio_offset = 0;
> +	cb->aio_nbytes = BUF_SIZE;
> +	cb->aio_rw_flags = RWF_NOWAIT;
> +
> +	iocbs[0] = cb;
> +}
> +
> +static void cleanup(void)
> +{
> +	if (fd[0] != -1)
> +		SAFE_CLOSE(fd[0]);
> +
> +	if (fd[1] != -1)
> +		SAFE_CLOSE(fd[1]);
> +
> +	if (ctx)
> +		if (tst_syscall(__NR_io_destroy, ctx))
> +			tst_brk(TBROK | TERRNO, "io_destroy() failed");
Upper if would require { } brackets, but let's simplify:

	if (ctx && tst_syscall(__NR_io_destroy, ctx))
		tst_brk(TBROK | TERRNO, "io_destroy() failed");

> +}
> +
> +static void run(void)
> +{
> +	struct io_event evbuf;
> +	struct timespec timeout = { .tv_sec = 1 };
> +	long nr = 1;
> +
> +	TEST(tst_syscall(__NR_io_submit, ctx, nr, iocbs));
> +
> +	if (TST_RET == -1 && TST_ERR == EOPNOTSUPP) {
> +		tst_brk(TCONF, "RWF_NOWAIT not supported by kernel");
> +	} else if (TST_RET != nr) {
> +		tst_brk(TBROK | TTERRNO, "io_submit() returns %ld, expected %ld",
> +				TST_RET, nr);
> +	}

Maybe move this to setup()?

> +
> +	TEST(tst_syscall(__NR_io_getevents, ctx, 1, 1, &evbuf, &timeout));
> +
> +	if (TST_RET != 1) {
> +		tst_res(TFAIL | TTERRNO, "io_getevents() failed to get 1 event");
> +		return;
> +	}
> +
> +	if (evbuf.res == -EAGAIN)
Did you please check this on 32bit?

> +		tst_res(TPASS, "io_getevents() returned EAGAIN on read event");
> +	else
> +		tst_res(TFAIL, "io_getevents() returned with %s instead of EAGAIN",
> +			strerror(-evbuf.res));
> +}

The rest LGTM.

Kind regards,
Petr

> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_kconfigs = (const char *[]) {
> +		"CONFIG_AIO=y",
> +		NULL
> +	},
> +	.bufs = (struct tst_buffers []) {
> +		{&buf, .size = BUF_SIZE},
> +		{&cb, .size = sizeof(iocb)},
> +		{&iocbs, .size = sizeof(iocb *)},
> +		{},
> +	}
> +};

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp