Message-ID: <CAOQ4uxg4mGpqCMCnJcNSK9vQXU1hx8XPQDEcL0+3yM7AF9V9-A@mail.gmail.com>
Date:   Wed, 28 Jun 2023 08:30:15 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     Ahelenia Ziemiańska 
        <nabijaczleweli@...ijaczleweli.xyz>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jan Kara <jack@...e.cz>,
        Chung-Chiang Cheng <cccheng@...ology.com>, ltp@...ts.linux.it,
        Petr Vorel <pvorel@...e.cz>
Subject: Re: [LTP RFC PATCH v3] inotify13: new test for fs/splice.c functions
 vs pipes vs inotify

On Wed, Jun 28, 2023 at 3:21 AM Ahelenia Ziemiańska
<nabijaczleweli@...ijaczleweli.xyz> wrote:
>
> The only one that passes on 6.1.27-1 is sendfile_file_to_pipe.
>
> Link: https://lore.kernel.org/linux-fsdevel/jbyihkyk5dtaohdwjyivambb2gffyjs3dodpofafnkkunxq7bu@jngkdxx65pux/t/#u
> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@...ijaczleweli.xyz>
> ---
> Sorry, I missed second part of Amir's comments somehow.
> cleanup is only run at the end by default:
> run it manually to not leak fds between tests.
>
> I've parameterised the tests from the driver, instead of with macros,
> and removed the tst_tag data.
>
> Added the * [Description] tag and full commit subject to the header
> comment; leaving the lore.k.o link for now, to be turned into a SHA
> when the kernel behaviour this tests starts having a SHA.
>
> Error checking has been lifted out as well.
> Formatted in kernel style accd'g to clang-format and check-inotify13.
>
> I used the wrong address for ltp@ the first time; I've since bounced the
> patchset, and am sending this, to the correct address. They were all
> held for moderation for now.
>
>  testcases/kernel/syscalls/inotify/.gitignore  |   1 +
>  testcases/kernel/syscalls/inotify/inotify13.c | 282 ++++++++++++++++++
>  2 files changed, 283 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/inotify/inotify13.c
>
> diff --git a/testcases/kernel/syscalls/inotify/.gitignore b/testcases/kernel/syscalls/inotify/.gitignore
> index f6e5c546a..b597ea63f 100644
> --- a/testcases/kernel/syscalls/inotify/.gitignore
> +++ b/testcases/kernel/syscalls/inotify/.gitignore
> @@ -10,3 +10,4 @@
>  /inotify10
>  /inotify11
>  /inotify12
> +/inotify13
> diff --git a/testcases/kernel/syscalls/inotify/inotify13.c b/testcases/kernel/syscalls/inotify/inotify13.c
> new file mode 100644
> index 000000000..97f88053e
> --- /dev/null
> +++ b/testcases/kernel/syscalls/inotify/inotify13.c
> @@ -0,0 +1,282 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*\
> + * [Description]
> + * Verify splice-family functions (and sendfile) generate IN_ACCESS
> + * for what they read and IN_MODIFY for what they write.
> + *
> + * Regression test for 983652c69199 ("splice: report related fsnotify events") and
> + * https://lore.kernel.org/linux-fsdevel/jbyihkyk5dtaohdwjyivambb2gffyjs3dodpofafnkkunxq7bu@jngkdxx65pux/t/#u

The process of posting a test for the fix that was not yet merged
is indeed a chicken and egg situation.

What I usually do is post a draft test (like this) and link
to the post of the LTP test (and maybe a branch on github)
when posting the fix, to say how I tested the fix.

I would then put it in my TODO to re-post the LTP
test once the kernel fix has been merged.

> + */
> +
> +#define _GNU_SOURCE
> +#include "config.h"
> +
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <fcntl.h>
> +#include <stdbool.h>
> +#include <inttypes.h>
> +#include <signal.h>
> +#include <sys/mman.h>
> +#include <sys/sendfile.h>
> +
> +#include "tst_test.h"
> +#include "tst_safe_macros.h"
> +#include "inotify.h"
> +
> +#if defined(HAVE_SYS_INOTIFY_H)
> +#include <sys/inotify.h>
> +
> +static int pipes[2] = { -1, -1 };
> +static int inotify = -1;
> +static int memfd = -1;
> +static int data_pipes[2] = { -1, -1 };
> +
> +static void watch_rw(int fd)
> +{
> +       char buf[64];
> +
> +       sprintf(buf, "/proc/self/fd/%d", fd);
> +       SAFE_MYINOTIFY_ADD_WATCH(inotify, buf, IN_ACCESS | IN_MODIFY);
> +}
> +
> +static int compar(const void *l, const void *r)
> +{
> +       const struct inotify_event *lie = l;
> +       const struct inotify_event *rie = r;
> +
> +       return lie->wd - rie->wd;
> +}
> +
> +static void get_events(size_t evcnt, struct inotify_event evs[static evcnt])
> +{
> +       struct inotify_event tail, *itr = evs;
> +
> +       for (size_t left = evcnt; left; --left)
> +               SAFE_READ(true, inotify, itr++, sizeof(struct inotify_event));
> +
> +       TEST(read(inotify, &tail, sizeof(struct inotify_event)));
> +       if (TST_RET != -1)
> +               tst_brk(TFAIL, ">%zu events", evcnt);
> +       if (TST_ERR != EAGAIN)
> +               tst_brk(TFAIL | TTERRNO, "expected EAGAIN");
> +
> +       qsort(evs, evcnt, sizeof(struct inotify_event), compar);
> +}
> +
> +static void expect_transfer(const char *name, size_t size)
> +{
> +       if (TST_RET == -1)
> +               tst_brk(TBROK | TERRNO, "%s", name);
> +       if ((size_t)TST_RET != size)
> +               tst_brk(TBROK, "%s: %ld != %zu", name, TST_RET, size);
> +}
> +
> +static void expect_event(struct inotify_event *ev, int wd, uint32_t mask)
> +{
> +       if (ev->wd != wd)
> +               tst_brk(TFAIL, "expect event for wd %d got %d", wd, ev->wd);
> +       if (ev->mask != mask)
> +               tst_brk(TFAIL,
> +                       "expect event with mask %" PRIu32 " got %" PRIu32 "",
> +                       mask, ev->mask);
> +}
> +
> +// write to file, rewind, transfer accd'g to f2p, read from pipe
> +// expecting: IN_ACCESS memfd, IN_MODIFY pipes[0]
> +static void file_to_pipe(const char *name, ssize_t (*f2p)(void))
> +{
> +       struct inotify_event events[2];
> +       char buf[strlen(name)];
> +
> +       SAFE_WRITE(SAFE_WRITE_RETRY, memfd, name, strlen(name));
> +       SAFE_LSEEK(memfd, 0, SEEK_SET);
> +       watch_rw(memfd);
> +       watch_rw(pipes[0]);
> +       TEST(f2p());
> +       expect_transfer(name, strlen(name));
> +
> +       get_events(ARRAY_SIZE(events), events);
> +       expect_event(events + 0, 1, IN_ACCESS);
> +       expect_event(events + 1, 2, IN_MODIFY);

So what I meant to say is that if there are double events that
usually get merged (unless reader was fast enough to read the
first event), this is something that I could live with, but encoding
an expectation for a double event, that's not at all what I meant.

But anyway, I see that you've found a way to work around
this problem, so at least the test can expect and get a single event.

I think you are missing expect_no_more_events() here to
verify that you won't get double events.

See test inotify12 as an example for a test that encodes
expect_events per test case and also verifies there are no
unexpected extra events.

That's also an example of a more generic test template,
but your test cases are all a bit different from each other is
subtle ways, so I trust you will find the best balance between
putting generic parameterized code in the run_test() template
and putting code in the test case subroutine.

> +
> +       SAFE_READ(true, pipes[0], buf, strlen(name));
> +       if (memcmp(buf, name, strlen(name)))
> +               tst_brk(TFAIL, "buf contents bad");
> +}
> +static ssize_t splice_file_to_pipe(void)
> +{
> +       return splice(memfd, NULL, pipes[1], NULL, 128 * 1024 * 1024, 0);
> +}
> +static ssize_t sendfile_file_to_pipe(void)
> +{
> +       return sendfile(pipes[1], memfd, NULL, 128 * 1024 * 1024);
> +}
> +
> +// write to pipe, transfer with splice, rewind file, read from file
> +// expecting: IN_ACCESS pipes[0], IN_MODIFY memfd
> +static void splice_pipe_to_file(const char *name, ssize_t (*param)(void))
> +{
> +       (void)name;
> +       (void)param;
> +       struct inotify_event events[2];
> +       char buf[sizeof(__func__)];
> +
> +       SAFE_WRITE(SAFE_WRITE_RETRY, pipes[1], __func__, sizeof(__func__));
> +       watch_rw(pipes[0]);
> +       watch_rw(memfd);
> +       TEST(splice(pipes[0], NULL, memfd, NULL, 128 * 1024 * 1024, 0));
> +       expect_transfer(__func__, sizeof(__func__));
> +
> +       get_events(ARRAY_SIZE(events), events);
> +       expect_event(events + 0, 1, IN_ACCESS);
> +       expect_event(events + 1, 2, IN_MODIFY);
> +
> +       SAFE_LSEEK(memfd, 0, SEEK_SET);
> +       SAFE_READ(true, memfd, buf, sizeof(__func__));
> +       if (memcmp(buf, __func__, sizeof(__func__)))
> +               tst_brk(TFAIL, "buf contents bad");
> +}
> +
> +// write to data_pipe, transfer accd'g to p2p, read from pipe
> +// expecting: IN_ACCESS data_pipes[0], IN_MODIFY pipes[1]
> +static void pipe_to_pipe(const char *name, ssize_t (*p2p)(void))
> +{
> +       struct inotify_event events[2];
> +       char buf[strlen(name)];
> +
> +       SAFE_WRITE(SAFE_WRITE_RETRY, data_pipes[1], name, strlen(name));
> +       watch_rw(data_pipes[0]);
> +       watch_rw(pipes[1]);
> +       TEST(p2p());
> +       expect_transfer(name, strlen(name));
> +
> +       get_events(ARRAY_SIZE(events), events);
> +       expect_event(events + 0, 1, IN_ACCESS);
> +       expect_event(events + 1, 2, IN_MODIFY);
> +
> +       SAFE_READ(true, pipes[0], buf, strlen(name));
> +       if (memcmp(buf, name, strlen(name)))
> +               tst_brk(TFAIL, "buf contents bad");
> +}
> +static ssize_t splice_pipe_to_pipe(void)
> +{
> +       return splice(data_pipes[0], NULL, pipes[1], NULL, 128 * 1024 * 1024,
> +                     0);
> +}
> +static ssize_t tee_pipe_to_pipe(void)
> +{
> +       return tee(data_pipes[0], pipes[1], 128 * 1024 * 1024, 0);
> +}
> +
> +// vmsplice to pipe, read from pipe
> +// expecting: IN_MODIFY pipes[0]
> +static char vmsplice_pipe_to_mem_dt[32 * 1024];
> +static void vmsplice_pipe_to_mem(const char *name, ssize_t (*param)(void))
> +{
> +       (void)name;
> +       (void)param;
> +       struct inotify_event event;
> +       char buf[sizeof(__func__)];
> +
> +       memcpy(vmsplice_pipe_to_mem_dt, __func__, sizeof(__func__));
> +       watch_rw(pipes[0]);
> +       TEST(vmsplice(
> +               pipes[1],
> +               &(struct iovec){ .iov_base = vmsplice_pipe_to_mem_dt,
> +                                .iov_len = sizeof(vmsplice_pipe_to_mem_dt) },
> +               1, SPLICE_F_GIFT));
> +       expect_transfer(__func__, sizeof(vmsplice_pipe_to_mem_dt));
> +
> +       get_events(1, &event);
> +       expect_event(&event, 1, IN_MODIFY);
> +
> +       SAFE_READ(true, pipes[0], buf, sizeof(__func__));
> +       if (memcmp(buf, __func__, sizeof(__func__)))
> +               tst_brk(TFAIL, "buf contents bad");
> +}
> +
> +// write to pipe, vmsplice from pipe
> +// expecting: IN_ACCESS pipes[1]
> +static void vmsplice_mem_to_pipe(const char *name, ssize_t (*param)(void))
> +{
> +       (void)name;
> +       (void)param;
> +       char buf[sizeof(__func__)];
> +       struct inotify_event event;
> +
> +       SAFE_WRITE(SAFE_WRITE_RETRY, pipes[1], __func__, sizeof(__func__));
> +       watch_rw(pipes[1]);
> +       TEST(vmsplice(pipes[0],
> +                     &(struct iovec){ .iov_base = buf,
> +                                      .iov_len = sizeof(buf) },
> +                     1, 0));
> +       expect_transfer(__func__, sizeof(buf));
> +
> +       get_events(1, &event);
> +       expect_event(&event, 1, IN_ACCESS);
> +
> +       if (memcmp(buf, __func__, sizeof(__func__)))
> +               tst_brk(TFAIL, "buf contents bad");
> +}
> +
> +#define TEST_F(f, param)      \
> +       {                     \
> +               #f, f, param, \
> +       }
> +static const struct {
> +       const char *n;
> +       void (*f)(const char *name, ssize_t (*param)(void));
> +       ssize_t (*param)(void);
> +} tests[] = {
> +       TEST_F(file_to_pipe, splice_file_to_pipe),
> +       TEST_F(file_to_pipe, sendfile_file_to_pipe),
> +       TEST_F(splice_pipe_to_file, NULL),
> +       TEST_F(pipe_to_pipe, splice_pipe_to_pipe),
> +       TEST_F(pipe_to_pipe, tee_pipe_to_pipe),
> +       TEST_F(vmsplice_pipe_to_mem, NULL),
> +       TEST_F(vmsplice_mem_to_pipe, NULL),
> +};
> +
> +static void cleanup(void)
> +{
> +       if (memfd != -1)
> +               SAFE_CLOSE(memfd);
> +       if (inotify != -1)
> +               SAFE_CLOSE(inotify);
> +       if (pipes[0] != -1)
> +               SAFE_CLOSE(pipes[0]);
> +       if (pipes[1] != -1)
> +               SAFE_CLOSE(pipes[1]);
> +       if (data_pipes[0] != -1)
> +               SAFE_CLOSE(data_pipes[0]);
> +       if (data_pipes[1] != -1)
> +               SAFE_CLOSE(data_pipes[1]);
> +}
> +
> +static void run_test(unsigned int n)
> +{
> +       tst_res(TINFO, "%s", tests[n].n);
> +
> +       SAFE_PIPE2(pipes, O_CLOEXEC);
> +       SAFE_PIPE2(data_pipes, O_CLOEXEC);
> +       inotify = SAFE_MYINOTIFY_INIT1(IN_NONBLOCK | IN_CLOEXEC);
> +       memfd = memfd_create(__func__, MFD_CLOEXEC);
> +       if (memfd == -1)
> +               tst_brk(TCONF | TERRNO, "memfd");
> +       tests[n].f(tests[n].n, tests[n].param);
> +       tst_res(TPASS, "ок");
> +       cleanup();
> +}
> +
> +static struct tst_test test = {
> +       .cleanup = cleanup,
> +       .test = run_test,
> +       .tcnt = ARRAY_SIZE(tests),
> +       .tags = (const struct tst_tag[]){ {} },

I don't think this is needed for the draft...

Thanks,
Amir.

Powered by blists - more mailing lists