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

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

> Fixs: #467
Fixes: #467

+++ b/testcases/kernel/syscalls/io_submit/Makefile
@@ -6,5 +6,6 @@ top_srcdir              ?= ../../../..
 include $(top_srcdir)/include/mk/testcases.mk
 
 LDLIBS                 += $(AIO_LIBS)

BTW Only io_submit01.c uses libaio, but all files are linked against -laio
because Makefile does not prefix that setup with io_submit01.

+LDFLAGS                        += -pthread

So for the same reason I would prefix this for this test only:
io_submit04:	LDFLAGS	+= -pthread
...
> +++ b/testcases/kernel/syscalls/io_submit/io_submit04.c
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023 Wei Gao <wegao@suse.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Similarily to the preadv2, this is a basic test for io_submit
s/Similarily/Similarly/

But I would remove "Similarily to the preadv2"

> + * RWF_NOWAIT flag, we are attempting to force io_submit return
> + * EAGAIN with thread concurently running threads.
> + *
nit: empty blank line.
> + */

...
> +static void *writer_thread(void *unused)
> +{
> +	while (!stop)
> +		write_test();
> +
> +	return unused;
> +}
> +
> +static void drop_caches(void)
> +{
> +	SAFE_FILE_PRINTF("/proc/sys/vm/drop_caches", "3");
> +}
> +
> +static void *cache_dropper(void *unused)
> +{
> +	unsigned int drop_cnt = 0;
> +
> +	while (!stop) {
> +		drop_caches();
> +		drop_cnt++;
> +	}
> +
> +	tst_res(TINFO, "Cache dropped %u times", drop_cnt);
> +
> +	return unused;
> +}
> +
> +static unsigned int io_submit(void)
> +{
> +	struct io_event evbuf;
> +	struct timespec timeout = { .tv_sec = 1 };

> +
> +	TST_EXP_VAL_SILENT(tst_syscall(__NR_io_submit, ctx, 1, iocbs), 1);

It fails with EOPNOTSUPP on TMPDIR on tmpfs (e.g. on Tumbleweed).

# ./io_submit04
...
tst_supported_fs_types.c:49: TINFO: mkfs is not needed for tmpfs
tst_test.c:1701: TINFO: === Testing on ext2 ===
tst_test.c:1117: TINFO: Formatting /dev/loop0 with ext2 opts='' extra opts=''
mke2fs 1.47.0 (5-Feb-2023)
tst_test.c:1131: TINFO: Mounting /dev/loop0 to /tmp/LTP_io_bhotMR/mntpoint fstyp=ext2 flags=0
io_submit04.c:85: TFAIL: tst_syscall(__NR_io_submit, ctx, 1, iocbs) retval -1 != 1: EOPNOTSUPP (95)

io_submit04.c:87: TFAIL: tst_syscall(__NR_io_getevents, ctx, 1, 1, &evbuf, &timeout) retval 0 != 1: SUCCESS (0)
io_submit04.c:85: TFAIL: tst_syscall(__NR_io_submit, ctx, 1, iocbs) retval -1 != 1: EOPNOTSUPP (95)
io_submit04.c:87: TFAIL: tst_syscall(__NR_io_getevents, ctx, 1, 1, &evbuf, &timeout) retval 0 != 1: SUCCESS (0)
io_submit04.c:85: TFAIL: tst_syscall(__NR_io_submit, ctx, 1, iocbs) retval -1 != 1: EOPNOTSUPP (95)
io_submit04.c:87: TFAIL: tst_syscall(__NR_io_getevents, ctx, 1, 1, &evbuf, &timeout) retval 0 != 1: SUCCESS (0)

First, it would make sense to quit this filesystem test on first error, because
it does not make sense to run test 100x when it fails. But that will need to
redesign way how you test it, otherwise cleanup breaks.

Why? Because .mntpoint = MNTPOINT does not mean it will cd into the directory.

You need to add:
	SAFE_CHDIR(MNTPOINT);

(This need is a bit confusing, I don't remember the reason why test does not do it.)

And if you add it, you finds that it fails on various filesystems, you need to
add:

	.skip_filesystems = (const char *const[]) {
		"vfat",
		"exfat",
		"ntfs",
		"tmpfs",
		NULL
	},

Maybe even fuse would be problematic, but not sure (on my system ntfs was
actually tested via fuse).

And make check warning:

$ make check-io_submit04
CHECK testcases/kernel/syscalls/io_submit/io_submit04.c
io_submit04.c:63: WARNING: Prefer using '"%s...", __func__' to using 'drop_caches', this function's name, in a string
make: [../../../../include/mk/rules.mk:56: check-io_submit04] Error 1 (ignored)

> +
> +	TST_EXP_VAL_SILENT(tst_syscall(__NR_io_getevents, ctx, 1, 1, &evbuf,
> +			&timeout), 1);
> +
> +	if (evbuf.res == -EAGAIN)
> +		return 1;
> +	else
> +		return 0;
nit: return evbuf.res == -EAGAIN
> +}
...

> +static inline void io_prep_option(struct iocb *cb, int fd, void *buf,
> +			size_t count, long long offset, unsigned int opcode)
> +{
> +	memset(cb, 0, sizeof(*cb));
> +	cb->aio_fildes = fd;
> +	cb->aio_lio_opcode = opcode;
> +	cb->aio_buf = (uint64_t)r_buf;

Compile warning (I guess it's not a first time I ask to pay attention to the
compile warnings):

io_submit04.c: In function ‘io_prep_option’:
io_submit04.c:35:66: warning: unused parameter ‘buf’ [-Wunused-parameter]
   35 | static inline void io_prep_option(struct iocb *cb, int fd, void *buf,

=> maybe use actually the parameter buf, instead of using the static r_buf?
	cb->aio_buf = (uint64_t)buf;

Or, you may just remove void *buf, as r_buf is static anyway and used in the
setup.

> +	cb->aio_offset = offset;
> +	cb->aio_nbytes = count;
> +	cb->aio_rw_flags = RWF_NOWAIT;
> +}
> +
...
> +static void setup(void)
> +{
> +
nit: empty line ^
> +	TST_EXP_PASS_SILENT(tst_syscall(__NR_io_setup, 1, &ctx));
I guess this should tst_brk() if failed, right?

Therefore just:
if (!tst_syscall(__NR_io_setup, 1, &ctx))
	tst_brk(TBROK, ...);

> +
> +	fd = SAFE_OPEN(TEST_FILE, O_RDWR | O_CREAT, MODE);
> +
> +	memset(w_buf, 'a', BUF_LEN);
> +	memset(r_buf, 'b', BUF_LEN);
> +
> +	io_prep_option(&iocb, fd, r_buf, BUF_LEN, 0, IOCB_CMD_PREAD);
NOTE: Here you pass r_buf

> +}
> +
> +static void cleanup(void)
> +{
> +	if (fd > 0)
> +		SAFE_CLOSE(fd);
> +
> +	if (tst_syscall(__NR_io_destroy, ctx))
> +		tst_brk(TBROK | TERRNO, "io_destroy() failed");
Maybe just tst_res(TWARN | TERRNO, ...) because it's in a cleanup function?
> +}
> +
> +
> +static void run(void)
> +{
> +
> +	pthread_t reader, dropper, writer;
> +	void *eagains;
> +
> +	stop = 0;
> +
> +	SAFE_PTHREAD_CREATE(&dropper, NULL, cache_dropper, NULL);
> +	SAFE_PTHREAD_CREATE(&reader, NULL, nowait_reader, NULL);
> +	SAFE_PTHREAD_CREATE(&writer, NULL, writer_thread, NULL);
> +
> +	while (!stop && tst_remaining_runtime())
> +		usleep(100000);
> +
> +	stop = 1;
I'm not really sure about this stop = 1 setup, which is done also on
nowait_reader(). This is probably obvious to the other reviewers.

> +
> +	SAFE_PTHREAD_JOIN(reader, &eagains);
> +	SAFE_PTHREAD_JOIN(dropper, NULL);
> +	SAFE_PTHREAD_JOIN(writer, NULL);
> +
> +	if (eagains)
> +		tst_res(TPASS, "Got some EAGAIN");
> +	else
> +		tst_res(TFAIL, "Haven't got EAGAIN");
> +
> +}

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