Message ID | 20210420084410.16179-1-pvorel@suse.cz |
---|---|
State | Changes Requested |
Headers | show |
Series | [v5,1/2] splice02: Generate input in C | expand |
Hi! > SAFE_CLOSE(fd); > + fd = SAFE_OPEN(TEST_FILENAME, O_RDONLY); > + to_check = st.st_size; > + > + psize = sysconf(_SC_PAGESIZE); > + > + tst_res(TINFO, "checking file content"); > + do { > + i = 0; > + size = to_check > psize ? psize : to_check; > + > + map = SAFE_MMAP(NULL, size, PROT_READ, MAP_PRIVATE, fd, > + st.st_size - to_check); Huh, why do we loop backward over the file? Maybe we can just do simple loop here that would be easier to understand: blocks = LTP_ALIGN(st.st_size, page_size) / page_size; for (block = 0; block < blocks; block++) { map = SAFE_MMAP(NULL, pagesize, PROT_READ, MAP_PRIVATE, fd, block * pagesize); to_check = (block+1) * page_size < st.st_size ? page_size : st.st_size % page_size; for (i = 0; i < to_check; i++) { if (map[i] != get_letter(block * page_size + i)) fail++; } SAFE_MUNMAP(map, size); } [Beware I haven't tested the code :-)]
> Hi! > > SAFE_CLOSE(fd); > > + fd = SAFE_OPEN(TEST_FILENAME, O_RDONLY); > > + to_check = st.st_size; > > + > > + psize = sysconf(_SC_PAGESIZE); > > + > > + tst_res(TINFO, "checking file content"); > > + do { > > + i = 0; > > + size = to_check > psize ? psize : to_check; > > + > > + map = SAFE_MMAP(NULL, size, PROT_READ, MAP_PRIVATE, fd, > > + st.st_size - to_check); > Huh, why do we loop backward over the file? > Maybe we can just do simple loop here that would be easier to > understand: > blocks = LTP_ALIGN(st.st_size, page_size) / page_size; > for (block = 0; block < blocks; block++) { > map = SAFE_MMAP(NULL, pagesize, PROT_READ, MAP_PRIVATE, fd, block * pagesize); > to_check = (block+1) * page_size < st.st_size ? page_size : st.st_size % page_size; > for (i = 0; i < to_check; i++) { > if (map[i] != get_letter(block * page_size + i)) > fail++; > } > SAFE_MUNMAP(map, size); > } > [Beware I haven't tested the code :-)] Yep. In your code you expect that written letter change each time. But original code writes the same letter for whole buffer (using memset()). I guess it does not matter whether I keep writing with memset() as is and adapt the checking code (code you proposed) or if I use your code for checking and adapt writing code: create buffer 494 (19x 26 letters), memset() it only once. Or do you have any preference of these? Also I have to replace 2x return with goto cleanup (to close fd and exit the child). Kind regards, Petr
Hi! > Yep. In your code you expect that written letter change each time. > But original code writes the same letter for whole buffer (using memset()). Ah, right, the mapping in the original code maps the remaning size to be written to a letter and uses it for whole block. I guess that it may be easier to understand if the letter was defined as an function of a position in the file like I have expected in my snippet. That way we can also produce different patterns without changing the test code (and we can later on introduce a library functions to fill buffer and check buffer as well, these would generally take a pointer to a buffer, size and an offset). That would mean that we will have to fill the buffer in a for loop instead of memset, but as long as the get_letter() function is inlined it should be fast enough.
Hi Cyril, > Hi! > > Yep. In your code you expect that written letter change each time. > > But original code writes the same letter for whole buffer (using memset()). > Ah, right, the mapping in the original code maps the remaning size to be > written to a letter and uses it for whole block. > I guess that it may be easier to understand if the letter was defined as > an function of a position in the file like I have expected in my > snippet. That way we can also produce different patterns without > changing the test code (and we can later on introduce a library > functions to fill buffer and check buffer as well, these would generally > take a pointer to a buffer, size and an offset). Make sense. > That would mean that we will have to fill the buffer in a for loop > instead of memset, but as long as the get_letter() function is inlined > it should be fast enough. Right. I guess trying to "align" buffer with alphabet size (e.g. multiple of 26) to avoid filling the buffer is not worth. Thus I keep buffer size 512 and fill it each time. Thanks for your suggestions and time. Kind regards, Petr
diff --git a/runtest/smoketest b/runtest/smoketest index 0c24fc1fa..2224d8f74 100644 --- a/runtest/smoketest +++ b/runtest/smoketest @@ -11,5 +11,5 @@ symlink01 symlink01 stat04 symlink01 -T stat04 utime01A symlink01 -T utime01 rename01A symlink01 -T rename01 -splice02 seq 1 20 | splice02 +splice02 splice02 -s 20 route4-change-dst route-change-dst.sh diff --git a/runtest/syscalls b/runtest/syscalls index 2d1e7b648..b89c545f0 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -1451,7 +1451,7 @@ socketpair02 socketpair02 sockioctl01 sockioctl01 splice01 splice01 -splice02 seq 1 20000 | splice02 +splice02 splice02 splice03 splice03 splice04 splice04 splice05 splice05 diff --git a/testcases/kernel/syscalls/splice/splice02.c b/testcases/kernel/syscalls/splice/splice02.c index b579667b9..cd7dfa836 100644 --- a/testcases/kernel/syscalls/splice/splice02.c +++ b/testcases/kernel/syscalls/splice/splice02.c @@ -1,40 +1,161 @@ // SPDX-License-Identifier: GPL-2.0-or-later /* * Copyright (c) Jens Axboe <axboe@kernel.dk>, 2009 + * Copyright (c) 2021 Petr Vorel <pvorel@suse.cz> + */ + +/*\ + * [DESCRIPTION] + * Original reproducer for kernel fix + * bf40d3435caf NFS: add support for splice writes + * from v2.6.31-rc1. + * * http://lkml.org/lkml/2009/4/2/55 + * + * [ALGORITHM] + * - create pipe + * - fork(), child replace stdin with pipe + * - parent write to pipe + * - child slice from pipe + * - check resulted file size and content */ #define _GNU_SOURCE +#include <fcntl.h> #include <stdio.h> #include <stdlib.h> +#include <sys/stat.h> #include <unistd.h> -#include <fcntl.h> #include "tst_test.h" +#include "lapi/mmap.h" #include "lapi/splice.h" -#define SPLICE_SIZE (64*1024) +#define BUFSIZE 512 +#define SPLICE_SIZE 1024 + +#define TEST_FILENAME "splice02-temp" -static void splice_test(void) +static char *sarg; +static int file_size; +static int pipe_fd[2]; + +static void setup(void) +{ + if (tst_parse_int(sarg, &file_size, 1, INT_MAX)) + tst_brk(TBROK, "invalid number of writes '%s', use <1,%d>", sarg, INT_MAX); +} + +static inline int get_letter(int n) +{ + return n % ('z' - 'a' + 1) + 'a'; +} + +static void do_child(void) { int fd; + size_t size, psize, to_check, i, fail = 0; + struct stat st; + char *map; + + SAFE_CLOSE(pipe_fd[1]); + SAFE_DUP2(pipe_fd[0], STDIN_FILENO); - fd = SAFE_OPEN("splice02-temp", O_WRONLY | O_CREAT | O_TRUNC, 0644); + fd = SAFE_OPEN(TEST_FILENAME, O_WRONLY | O_CREAT | O_TRUNC, 0644); - TEST(splice(STDIN_FILENO, NULL, fd, NULL, SPLICE_SIZE, 0)); - if (TST_RET < 0) { - tst_res(TFAIL, "splice failed - errno = %d : %s", - TST_ERR, strerror(TST_ERR)); - } else { - tst_res(TPASS, "splice() system call Passed"); + do { + TEST(splice(STDIN_FILENO, NULL, fd, NULL, SPLICE_SIZE, 0)); + if (TST_RET < 0) { + tst_res(TFAIL | TTERRNO, "splice failed"); + goto cleanup; + } + } while (TST_RET > 0); + + stat(TEST_FILENAME, &st); + if (st.st_size != file_size) { + tst_res(TFAIL, "file size is different from expected: %ld (%d)", + st.st_size, file_size); + return; } SAFE_CLOSE(fd); + fd = SAFE_OPEN(TEST_FILENAME, O_RDONLY); + to_check = st.st_size; + + psize = sysconf(_SC_PAGESIZE); + + tst_res(TINFO, "checking file content"); + do { + i = 0; + size = to_check > psize ? psize : to_check; + + map = SAFE_MMAP(NULL, size, PROT_READ, MAP_PRIVATE, fd, + st.st_size - to_check); + + while (i < size) { + if (map[i] != get_letter(to_check - (i / BUFSIZE * BUFSIZE))) + fail++; + i++; + } + SAFE_MUNMAP(map, size); + to_check -= size; + } while (to_check > 0); + + if (fail) { + tst_res(TFAIL, "%ld unexpected bytes found", fail); + return; + } + + tst_res(TPASS, "splice() system call passed"); + +cleanup: + SAFE_CLOSE(fd); + exit(0); +} + +static void run(void) +{ + size_t size, written, max_pipe_size, to_write; + char buf[BUFSIZE]; + + SAFE_PIPE(pipe_fd); + + if (!file_size) { + max_pipe_size = SAFE_FCNTL(pipe_fd[1], F_GETPIPE_SZ); + file_size = max_pipe_size << 4; + } + + to_write = file_size; + + if (!SAFE_FORK()) + do_child(); + + tst_res(TINFO, "writting %d bytes", file_size); + + while (to_write > 0) { + memset(buf, get_letter(to_write), BUFSIZE); + + size = to_write > BUFSIZE ? BUFSIZE : to_write; + written = SAFE_WRITE(1, pipe_fd[1], &buf, size); + to_write -= written; + } + + SAFE_CLOSE(pipe_fd[0]); + SAFE_CLOSE(pipe_fd[1]); + + tst_reap_children(); } static struct tst_test test = { - .test_all = splice_test, + .test_all = run, + .setup = setup, + .needs_checkpoints = 1, .needs_tmpdir = 1, + .forks_child = 1, .min_kver = "2.6.17", + .options = (struct tst_option[]) { + {"s:", &sarg, "-s x Size of output file in bytes (default: 16x max pipe size, i.e. 1M on intel)"}, + {} + }, };