[v1] io_submit05.c: Add test case for RWF_APPEND flag

Signed-off-by: Wei Gao <wegao@suse.com>
---
 runtest/syscalls                              |   1 +
 .../kernel/syscalls/io_submit/.gitignore      |   1 +
 .../kernel/syscalls/io_submit/io_submit05.c   | 123 ++++++++++++++++++
 3 files changed, 125 insertions(+)
 create mode 100644 testcases/kernel/syscalls/io_submit/io_submit05.c

Hi!

On 11/10/23 12:58, Wei Gao via ltp wrote:
> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
> runtest/syscalls                              |   1 +
> .../kernel/syscalls/io_submit/.gitignore      |   1 +
> .../kernel/syscalls/io_submit/io_submit05.c   | 123 ++++++++++++++++++
> 3 files changed, 125 insertions(+)
> create mode 100644 testcases/kernel/syscalls/io_submit/io_submit05.c
>
> diff --git a/runtest/syscalls b/runtest/syscalls
> index c7a0b2301..15a54f2d3 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -656,6 +656,7 @@ io_setup02 io_setup02
> io_submit01 io_submit01
> io_submit02 io_submit02
> io_submit03 io_submit03
> +io_submit05 io_submit05
>
> keyctl01 keyctl01
> keyctl02 keyctl02
> diff --git a/testcases/kernel/syscalls/io_submit/.gitignore 
> b/testcases/kernel/syscalls/io_submit/.gitignore
> index 60b07970a..cf0d4c6df 100644
> --- a/testcases/kernel/syscalls/io_submit/.gitignore
> +++ b/testcases/kernel/syscalls/io_submit/.gitignore
> @@ -1,3 +1,4 @@
> /io_submit01
> /io_submit02
> /io_submit03
> +/io_submit05
> diff --git a/testcases/kernel/syscalls/io_submit/io_submit05.c 
> b/testcases/kernel/syscalls/io_submit/io_submit05.c
> new file mode 100644
> index 000000000..32f4379e1
> --- /dev/null
> +++ b/testcases/kernel/syscalls/io_submit/io_submit05.c
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023 Wei Gao <wegao@suse.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * This is a basic test for io_submit RWF_APPEND flag.
> + *
> + */
> +
> +#include <linux/aio_abi.h>
> +
> +#include "config.h"
> +#include "tst_test.h"
> +#include "tst_safe_pthread.h"
> +#include "lapi/syscalls.h"
> +
> +#define TEST_FILE "test_file"
> +#define MODE 0777
> +#define BUF_LEN 10
> +#define MNTPOINT "mntpoint"
> +
> +static char *init_buf;
> +static char *update_buf;
> +static char *tmp_buf;
> +static int fd;
> +static aio_context_t ctx;
> +static struct iocb iocb;
> +static struct iocb *iocbs[] = {&iocb};
> +static volatile int stop;
> +
> +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)buf;
> +    cb->aio_offset = offset;
> +    cb->aio_nbytes = count;
> +    cb->aio_rw_flags = RWF_APPEND;
> +}
> +
> +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);
> +    TST_EXP_VAL_SILENT(tst_syscall(__NR_io_getevents, ctx, 1, 1, &evbuf,
> +            &timeout), 1);
> +}
This function has not return value, so it should be defined as "static 
void io_submit(void)".
> +
> +static void setup(void)
> +{
Empty space to remove.
>
> +
> +    TST_EXP_PASS_SILENT(tst_syscall(__NR_io_setup, 1, &ctx));
> +
> +    fd = SAFE_OPEN(TEST_FILE, O_RDWR | O_CREAT, MODE);
> +    SAFE_LSEEK(fd, 0, SEEK_SET);
> +
> +    memset(init_buf, 0x62, BUF_LEN);
> +    memset(update_buf, 0x61, BUF_LEN);
> +    memset(tmp_buf, 0, BUF_LEN);
> +
> +    io_prep_option(&iocb, fd, update_buf, BUF_LEN, 1, IOCB_CMD_PWRITE);
> +}
> +
> +static void cleanup(void)
> +{
> +    if (fd > 0)
> +        SAFE_CLOSE(fd);
> +
> +    if (tst_syscall(__NR_io_destroy, ctx))
> +        tst_brk(TBROK | TERRNO, "io_destroy() failed");
> +}
> +
> +
> +static void run(void)
> +{
> +    int i;
> +
> +    SAFE_WRITE(0, fd, init_buf, BUF_LEN);
> +    io_submit();
> +    SAFE_LSEEK(fd, BUF_LEN, SEEK_SET);
> +    SAFE_READ(1, fd, tmp_buf, BUF_LEN);
> +    for (i = 0; i < BUF_LEN; i++) {
> +        if (tmp_buf[i] != 0x61)
> +            break;
> +    }
> +
> +    if (i != BUF_LEN) {
> +        tst_res(TFAIL, "buffer wrong at %i have %c expected 'a'",
> +                i, tmp_buf[i]);
> +        return;
> +    }
> +
> +    tst_res(TPASS, "io_submit wrote %zi bytes successfully "
> +            "with content 'a' expectedly ", BUF_LEN);
We need to use %d instead of %zi.
> +}
> +
> +static struct tst_test test = {
> +    .needs_tmpdir = 1,
> +    .needs_kconfigs = (const char *[]) {
> +        "CONFIG_AIO=y",
> +        NULL
> +    },
> +    .setup = setup,
> +    .cleanup = cleanup,
> +    .test_all = run,
> +    .max_runtime = 60,
> +    .mntpoint = MNTPOINT,
> +    .mount_device = 1,
> +    .all_filesystems = 1,
We don't need to mount any folder and to test on multiple filesystems, 
since we are testing a regular syscall feature which is unrelated with 
the specific filesystem.
> +    .bufs = (struct tst_buffers []) {
> +        {&init_buf, .size = BUF_LEN},
> +        {&update_buf, .size = BUF_LEN},
> +        {&tmp_buf, .size = BUF_LEN},
> +        {}
> +    }
> +};

I generally think this test doesn't test the feature in a proper way. 
The meaning of the RWF_APPEND flag is that we asynchronously append data 
to a file, but in this case we only check that data has been appended at 
the start of the file, since we call lseek(fd, 0, SEEK_SET) first.

It would be probably more interesting to generate a file, then call the 
io_submit syscall with RWF_APPEND flag and to check that data has been 
actually appended to the file in a correct way.

Regards,
Andrea Cervesato

Hi Wei,

> +++ b/runtest/syscalls
> @@ -656,6 +656,7 @@ io_setup02 io_setup02
>  io_submit01 io_submit01
>  io_submit02 io_submit02
>  io_submit03 io_submit03
> +io_submit05 io_submit05
nit: you have io_submit04 patchset, that's why 04 is skipped. You send them
separately, this can lead to unapplicable patch. As we also discussed 04, could
you send them both in one patchset (even they are separate), because they
influence each other as they touch bouth runtest/syscalls and .gitignore on the
same place?

...
>  keyctl01 keyctl0> +++ b/testcases/kernel/syscalls/io_submit/io_submit05.c
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023 Wei Gao <wegao@suse.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * This is a basic test for io_submit RWF_APPEND flag.
                                       ^ nit: io_submit()
> + *
> + */
> +
> +#include <linux/aio_abi.h>
> +
> +#include "config.h"
We don't need config.h, right?

> +#include "tst_test.h"
> +#include "tst_safe_pthread.h"
And we don't need this header either.

> +#include "lapi/syscalls.h"
...

> +static char *init_buf;
> +static char *update_buf;
> +static char *tmp_buf;

> +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));
You don't need to do this, cb is static struct iocb iocb, which is {} because static.

> +	cb->aio_fildes = fd;
> +	cb->aio_lio_opcode = opcode;
> +	cb->aio_buf = (uint64_t)buf;
> +	cb->aio_offset = offset;
> +	cb->aio_nbytes = count;
> +	cb->aio_rw_flags = RWF_APPEND;
> +}
Also, I don't see much point of having this as a separate function, I would just
put this in the end of setup().

> +static unsigned int io_submit(void)
As Andrea mentioned, this should be void. But I also does not like the name...
> +{
> +	struct io_event evbuf;
Although we use only one array thus it works, man says: struct iocb **iocbpp),
thus it should have been:
struct io_event evbuf[1] = {};

Newer mind, but I would probably initialize (although it should not be needed as
we pass it to kernel thus other tests does not do it, it's a good habit):
struct io_event evbuf = {};

> +	struct timespec timeout = { .tv_sec = 1 };
> +
> +	TST_EXP_VAL_SILENT(tst_syscall(__NR_io_submit, ctx, 1, iocbs), 1);
We probably want to quit testing if __NR_io_submit fails (probably
tst_brk(TBROK)).

> +	TST_EXP_VAL_SILENT(tst_syscall(__NR_io_getevents, ctx, 1, 1, &evbuf,
> +			&timeout), 1);

Although we don't use libaio wrapper it's a bit confusing we have io_submit()
function which is not just __NR_io_submit wrapper, but call also
__NR_io_getevents. Maybe use a different name? Or, maybe, add these 4 lines into
the main test function?

Maybe also verify evbuf.res == BUF_LEN ? E.g.:

TST_EXP_VAL_SILENT(evbuf.res, BUF_LEN);

> +}

> +
> +static void setup(void)
> +{
> +
> +	TST_EXP_PASS_SILENT(tst_syscall(__NR_io_setup, 1, &ctx));
> +
> +	fd = SAFE_OPEN(TEST_FILE, O_RDWR | O_CREAT, MODE);
> +	SAFE_LSEEK(fd, 0, SEEK_SET);
> +
> +	memset(init_buf, 0x62, BUF_LEN);

> +	memset(update_buf, 0x61, BUF_LEN);
I would also use 'a' and 'b' values (slightly more readable).
And 'a' is what is going to be tested, maybe #define it at the top
(readability, as you define other constants)?

We don't need this memset() either (static variable).
But we want to memset() update_buf each run (for -iN), right?
Then in should be in the test function (again setup() is run only once,
regardless of N in -iN).

> +	memset(tmp_buf, 0, BUF_LEN);
The same about memset() aplies to tmp_buf.

> +
> +	io_prep_option(&iocb, fd, update_buf, BUF_LEN, 1, IOCB_CMD_PWRITE);
> +}
> +
> +static void cleanup(void)
> +{
> +	if (fd > 0)
> +		SAFE_CLOSE(fd);
> +
> +	if (tst_syscall(__NR_io_destroy, ctx))
> +		tst_brk(TBROK | TERRNO, "io_destroy() failed");
> +}
> +
> +
> +static void run(void)
> +{
> +	int i;
> +
> +	SAFE_WRITE(0, fd, init_buf, BUF_LEN);
> +	io_submit();
> +	SAFE_LSEEK(fd, BUF_LEN, SEEK_SET);
> +	SAFE_READ(1, fd, tmp_buf, BUF_LEN);
very nit: space before for helps readability.
> +	for (i = 0; i < BUF_LEN; i++) {
> +		if (tmp_buf[i] != 0x61)
Why not here print tst_res(TFAIL) and return, instead of break?
> +			break;
> +	}
> +
> +	if (i != BUF_LEN) {
> +		tst_res(TFAIL, "buffer wrong at %i have %c expected 'a'",
Here you hardwire 'a', which elsewhere you reference on other places as 0x61.
Again use single definition would be better.

Kind regards,
Petr

> +				i, tmp_buf[i]);
> +		return;
> +	}
> +
> +	tst_res(TPASS, "io_submit wrote %zi bytes successfully "
> +			"with content 'a' expectedly ", BUF_LEN);
> +}
> +
> +static struct tst_test test = {
> +	.needs_tmpdir = 1,
> +	.needs_kconfigs = (const char *[]) {
> +		"CONFIG_AIO=y",
> +		NULL
> +	},
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = run,
> +	.max_runtime = 60,
> +	.mntpoint = MNTPOINT,
> +	.mount_device = 1,
> +	.all_filesystems = 1,
> +	.bufs = (struct tst_buffers []) {
> +		{&init_buf, .size = BUF_LEN},
> +		{&update_buf, .size = BUF_LEN},
> +		{&tmp_buf, .size = BUF_LEN},
> +		{}
> +	}
> +};