lkml.org 
[lkml]   [2023]   [Jun]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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@nabijaczleweli.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@nabijaczleweli.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.

    \
     
     \ /
      Last update: 2023-06-28 10:29    [W:3.154 / U:2.120 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site