| Message ID | 1550215053-6795-1-git-send-email-sumit.garg@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] syscalls: add syscall syncfs test | expand |
Hi Sumit, Sorry for the late reply. :-) 1) Compilation failed on older kernel(e.g. v2.6.32) because of the undefined syncfs(). According to manpage, syncfs() was first appeared in Linux 2.6.39 and library support was added to glibc in version 2.14. Perhaps, we need to check if syncfs() is defined. 2) Running this test got TBROK on older kernel(e.g. v3.10.0), as below: ------------------------------------------------------------------- syncfs01.c:63: FAIL: Failed to sync test filesystem to device ------------------------------------------------------------------- It seems that the content of /sys/block/<loopX>/stat is always zero and doesn't update even though write and sync have been done. I am looking into it, but i am not sure if this is a known bug on older kernel. Best Regards, Xiao Yang On 2019/02/15 15:17, Sumit Garg wrote: > syncfs01 tests to sync filesystem having large dirty file pages to block > device. Also, it tests all supported filesystems on a test block device. > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > --- > > Changes in v2: > 1. Remove unused header file include. > 2. Remove redundant tst_device check. > 3. Remove redundant flags from tst_test struct. > > Fixes: https://github.com/linux-test-project/ltp/issues/294 > > runtest/syscalls | 2 + > testcases/kernel/syscalls/syncfs/.gitignore | 1 + > testcases/kernel/syscalls/syncfs/Makefile | 8 +++ > testcases/kernel/syscalls/syncfs/syncfs01.c | 101 ++++++++++++++++++++++++++++ > 4 files changed, 112 insertions(+) > create mode 100644 testcases/kernel/syscalls/syncfs/.gitignore > create mode 100644 testcases/kernel/syscalls/syncfs/Makefile > create mode 100644 testcases/kernel/syscalls/syncfs/syncfs01.c > > diff --git a/runtest/syscalls b/runtest/syscalls > index 668c87c..9442740 100644 > --- a/runtest/syscalls > +++ b/runtest/syscalls > @@ -1346,6 +1346,8 @@ symlinkat01 symlinkat01 > sync01 sync01 > sync02 sync02 > > +syncfs01 syncfs01 > + > #testcases for sync_file_range > sync_file_range01 sync_file_range01 > > diff --git a/testcases/kernel/syscalls/syncfs/.gitignore b/testcases/kernel/syscalls/syncfs/.gitignore > new file mode 100644 > index 0000000..6066295 > --- /dev/null > +++ b/testcases/kernel/syscalls/syncfs/.gitignore > @@ -0,0 +1 @@ > +syncfs01 > diff --git a/testcases/kernel/syscalls/syncfs/Makefile b/testcases/kernel/syscalls/syncfs/Makefile > new file mode 100644 > index 0000000..3e6c2f4 > --- /dev/null > +++ b/testcases/kernel/syscalls/syncfs/Makefile > @@ -0,0 +1,8 @@ > +# Copyright (c) 2019 - Linaro Limited. All rights reserved. > +# SPDX-License-Identifier: GPL-2.0-or-later > + > +top_srcdir ?= ../../../.. > + > +include $(top_srcdir)/include/mk/testcases.mk > + > +include $(top_srcdir)/include/mk/generic_leaf_target.mk > diff --git a/testcases/kernel/syscalls/syncfs/syncfs01.c b/testcases/kernel/syscalls/syncfs/syncfs01.c > new file mode 100644 > index 0000000..e2d3e80 > --- /dev/null > +++ b/testcases/kernel/syscalls/syncfs/syncfs01.c > @@ -0,0 +1,101 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2019 Linaro Limited. All rights reserved. > + * Author: Sumit Garg <sumit.garg@linaro.org> > + */ > + > +/* > + * Test syncfs > + * > + * It basically tests syncfs() to sync filesystem having large dirty file > + * pages to block device. Also, it tests all supported filesystems on a test > + * block device. > + */ > + > +#define _GNU_SOURCE > +#include <stdlib.h> > +#include <stdio.h> > +#include <sys/types.h> > +#include "tst_test.h" > +#include "lapi/fs.h" > +#include "lapi/stat.h" > + > +#define MNTPOINT "mnt_point" > +#define TST_FILE MNTPOINT"/test" > +#define TST_FILE_SIZE_MB 32 > +#define SIZE_MB (1024*1024) > +#define MODE 0644 > + > +static char dev_stat_path[1024]; > +static char *buffer; > +static int fd; > + > +static void verify_syncfs(void) > +{ > + char nwrite_sec_val[BUFSIZ]; > + int counter; > + unsigned long prev_write_sec = 0, write_sec = 0; > + > + SAFE_FILE_SCANF(dev_stat_path, "%*s %*s %*s %*s %*s %*s %s", > + nwrite_sec_val); > + > + prev_write_sec = SAFE_STRTOUL(nwrite_sec_val, 0, ULONG_MAX); > + > + fd = SAFE_OPEN(TST_FILE, O_RDWR|O_CREAT, MODE); > + > + /* Filling the test file */ > + for (counter = 0; counter < TST_FILE_SIZE_MB; counter++) > + SAFE_WRITE(1, fd, buffer, SIZE_MB); > + > + TEST(syncfs(fd)); > + if (TST_RET != 0) > + tst_brk(TFAIL | TTERRNO, "syncfs(fd) failed"); > + > + SAFE_FILE_SCANF(dev_stat_path, "%*s %*s %*s %*s %*s %*s %s", > + nwrite_sec_val); > + > + write_sec = SAFE_STRTOUL(nwrite_sec_val, 0, ULONG_MAX); > + > + if ((write_sec - prev_write_sec) * 512 >= > + (TST_FILE_SIZE_MB * SIZE_MB)) > + tst_res(TPASS, "Test filesystem synced to device"); > + else > + tst_res(TFAIL, "Failed to sync test filesystem to device"); > + > + SAFE_CLOSE(fd); > +} > + > +static void setup(void) > +{ > + struct stat st; > + > + snprintf(dev_stat_path, sizeof(dev_stat_path), "/sys/block/%s/stat", > + strrchr(tst_device->dev, '/') + 1); > + > + if (stat(dev_stat_path, &st) != 0) > + tst_brk(TCONF, "Test device stat file: %s not found", > + dev_stat_path); > + > + buffer = SAFE_MALLOC(SIZE_MB); > + > + memset(buffer, 0, SIZE_MB); > +} > + > +static void cleanup(void) > +{ > + if (buffer) > + free(buffer); > + > + if (fd > 0) > + SAFE_CLOSE(fd); > +} > + > +static struct tst_test test = { > + .needs_root = 1, > + .mount_device = 1, > + .all_filesystems = 1, > + .mntpoint = MNTPOINT, > + .setup = setup, > + .cleanup = cleanup, > + .test_all = verify_syncfs, > +};
On Fri, Feb 15, 2019 at 9:17 AM Sumit Garg <sumit.garg@linaro.org> wrote: > > syncfs01 tests to sync filesystem having large dirty file pages to block > device. Also, it tests all supported filesystems on a test block device. > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > --- > > Changes in v2: > 1. Remove unused header file include. > 2. Remove redundant tst_device check. > 3. Remove redundant flags from tst_test struct. > > Fixes: https://github.com/linux-test-project/ltp/issues/294 > > runtest/syscalls | 2 + > testcases/kernel/syscalls/syncfs/.gitignore | 1 + > testcases/kernel/syscalls/syncfs/Makefile | 8 +++ > testcases/kernel/syscalls/syncfs/syncfs01.c | 101 ++++++++++++++++++++++++++++ > 4 files changed, 112 insertions(+) > create mode 100644 testcases/kernel/syscalls/syncfs/.gitignore > create mode 100644 testcases/kernel/syscalls/syncfs/Makefile > create mode 100644 testcases/kernel/syscalls/syncfs/syncfs01.c > > diff --git a/runtest/syscalls b/runtest/syscalls > index 668c87c..9442740 100644 > --- a/runtest/syscalls > +++ b/runtest/syscalls > @@ -1346,6 +1346,8 @@ symlinkat01 symlinkat01 > sync01 sync01 > sync02 sync02 > > +syncfs01 syncfs01 > + > #testcases for sync_file_range > sync_file_range01 sync_file_range01 > > diff --git a/testcases/kernel/syscalls/syncfs/.gitignore b/testcases/kernel/syscalls/syncfs/.gitignore > new file mode 100644 > index 0000000..6066295 > --- /dev/null > +++ b/testcases/kernel/syscalls/syncfs/.gitignore > @@ -0,0 +1 @@ > +syncfs01 > diff --git a/testcases/kernel/syscalls/syncfs/Makefile b/testcases/kernel/syscalls/syncfs/Makefile > new file mode 100644 > index 0000000..3e6c2f4 > --- /dev/null > +++ b/testcases/kernel/syscalls/syncfs/Makefile > @@ -0,0 +1,8 @@ > +# Copyright (c) 2019 - Linaro Limited. All rights reserved. > +# SPDX-License-Identifier: GPL-2.0-or-later > + > +top_srcdir ?= ../../../.. > + > +include $(top_srcdir)/include/mk/testcases.mk > + > +include $(top_srcdir)/include/mk/generic_leaf_target.mk > diff --git a/testcases/kernel/syscalls/syncfs/syncfs01.c b/testcases/kernel/syscalls/syncfs/syncfs01.c > new file mode 100644 > index 0000000..e2d3e80 > --- /dev/null > +++ b/testcases/kernel/syscalls/syncfs/syncfs01.c > @@ -0,0 +1,101 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2019 Linaro Limited. All rights reserved. > + * Author: Sumit Garg <sumit.garg@linaro.org> > + */ > + > +/* > + * Test syncfs > + * > + * It basically tests syncfs() to sync filesystem having large dirty file > + * pages to block device. Also, it tests all supported filesystems on a test > + * block device. > + */ > + > +#define _GNU_SOURCE > +#include <stdlib.h> > +#include <stdio.h> > +#include <sys/types.h> > +#include "tst_test.h" > +#include "lapi/fs.h" > +#include "lapi/stat.h" > + > +#define MNTPOINT "mnt_point" > +#define TST_FILE MNTPOINT"/test" > +#define TST_FILE_SIZE_MB 32 > +#define SIZE_MB (1024*1024) > +#define MODE 0644 > + > +static char dev_stat_path[1024]; > +static char *buffer; > +static int fd; > + > +static void verify_syncfs(void) > +{ > + char nwrite_sec_val[BUFSIZ]; > + int counter; > + unsigned long prev_write_sec = 0, write_sec = 0; > + > + SAFE_FILE_SCANF(dev_stat_path, "%*s %*s %*s %*s %*s %*s %s", > + nwrite_sec_val); > + > + prev_write_sec = SAFE_STRTOUL(nwrite_sec_val, 0, ULONG_MAX); Why not let scanf read %ul? > + > + fd = SAFE_OPEN(TST_FILE, O_RDWR|O_CREAT, MODE); > + > + /* Filling the test file */ > + for (counter = 0; counter < TST_FILE_SIZE_MB; counter++) > + SAFE_WRITE(1, fd, buffer, SIZE_MB); > + > + TEST(syncfs(fd)); > + if (TST_RET != 0) > + tst_brk(TFAIL | TTERRNO, "syncfs(fd) failed"); > + > + SAFE_FILE_SCANF(dev_stat_path, "%*s %*s %*s %*s %*s %*s %s", > + nwrite_sec_val); > + > + write_sec = SAFE_STRTOUL(nwrite_sec_val, 0, ULONG_MAX); > + > + if ((write_sec - prev_write_sec) * 512 >= > + (TST_FILE_SIZE_MB * SIZE_MB)) > + tst_res(TPASS, "Test filesystem synced to device"); > + else > + tst_res(TFAIL, "Failed to sync test filesystem to device"); > + > + SAFE_CLOSE(fd); > +} > + It's good to have a tests that verified syncfs() actually does what it is meant to do. It's awkward that none of the tests for fsync() fdatasync() sync() sync_file_range() check that. It would be very low hanging to have the exact same test that you wrote iterate on several test cases where the only difference is the op called on fd. (fsync,fdatasync,syncfs) should all have the same consequence wrt minimal written sectors. With a little more effort, sync() and sync_file_range() could also be added to test cases. I realize that LTP usually puts syscalls tests under the specific kernel/syscalls directory, but in this case, I believe code reuse calls for a single test that exercises all three syscalls. Thanks, Amir.
On Fri, 15 Feb 2019 at 13:25, Xiao Yang <yangx.jy@cn.fujitsu.com> wrote: > > Hi Sumit, > > Sorry for the late reply. :-) No worries. :) > > 1) Compilation failed on older kernel(e.g. v2.6.32) because of the > undefined syncfs(). > According to manpage, syncfs() was first appeared in Linux 2.6.39 and > library > support was added to glibc in version 2.14. Perhaps, we need to check if > syncfs() > is defined. > Would configuring .min_kver suffice to avoid this compilation issue? > 2) Running this test got TBROK on older kernel(e.g. v3.10.0), as below: > ------------------------------------------------------------------- > syncfs01.c:63: FAIL: Failed to sync test filesystem to device > ------------------------------------------------------------------- > It seems that the content of /sys/block/<loopX>/stat is always zero and > doesn't update even though write and sync have been done. I am looking > into it, but i am not sure if this is a known bug on older kernel. > Thanks for looking into this. I will investigate this also. -Sumit > Best Regards, > Xiao Yang > On 2019/02/15 15:17, Sumit Garg wrote: > > syncfs01 tests to sync filesystem having large dirty file pages to block > > device. Also, it tests all supported filesystems on a test block device. > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > --- > > > > Changes in v2: > > 1. Remove unused header file include. > > 2. Remove redundant tst_device check. > > 3. Remove redundant flags from tst_test struct. > > > > Fixes: https://github.com/linux-test-project/ltp/issues/294 > > > > runtest/syscalls | 2 + > > testcases/kernel/syscalls/syncfs/.gitignore | 1 + > > testcases/kernel/syscalls/syncfs/Makefile | 8 +++ > > testcases/kernel/syscalls/syncfs/syncfs01.c | 101 ++++++++++++++++++++++++++++ > > 4 files changed, 112 insertions(+) > > create mode 100644 testcases/kernel/syscalls/syncfs/.gitignore > > create mode 100644 testcases/kernel/syscalls/syncfs/Makefile > > create mode 100644 testcases/kernel/syscalls/syncfs/syncfs01.c > > > > diff --git a/runtest/syscalls b/runtest/syscalls > > index 668c87c..9442740 100644 > > --- a/runtest/syscalls > > +++ b/runtest/syscalls > > @@ -1346,6 +1346,8 @@ symlinkat01 symlinkat01 > > sync01 sync01 > > sync02 sync02 > > > > +syncfs01 syncfs01 > > + > > #testcases for sync_file_range > > sync_file_range01 sync_file_range01 > > > > diff --git a/testcases/kernel/syscalls/syncfs/.gitignore b/testcases/kernel/syscalls/syncfs/.gitignore > > new file mode 100644 > > index 0000000..6066295 > > --- /dev/null > > +++ b/testcases/kernel/syscalls/syncfs/.gitignore > > @@ -0,0 +1 @@ > > +syncfs01 > > diff --git a/testcases/kernel/syscalls/syncfs/Makefile b/testcases/kernel/syscalls/syncfs/Makefile > > new file mode 100644 > > index 0000000..3e6c2f4 > > --- /dev/null > > +++ b/testcases/kernel/syscalls/syncfs/Makefile > > @@ -0,0 +1,8 @@ > > +# Copyright (c) 2019 - Linaro Limited. All rights reserved. > > +# SPDX-License-Identifier: GPL-2.0-or-later > > + > > +top_srcdir ?= ../../../.. > > + > > +include $(top_srcdir)/include/mk/testcases.mk > > + > > +include $(top_srcdir)/include/mk/generic_leaf_target.mk > > diff --git a/testcases/kernel/syscalls/syncfs/syncfs01.c b/testcases/kernel/syscalls/syncfs/syncfs01.c > > new file mode 100644 > > index 0000000..e2d3e80 > > --- /dev/null > > +++ b/testcases/kernel/syscalls/syncfs/syncfs01.c > > @@ -0,0 +1,101 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Copyright (c) 2019 Linaro Limited. All rights reserved. > > + * Author: Sumit Garg <sumit.garg@linaro.org> > > + */ > > + > > +/* > > + * Test syncfs > > + * > > + * It basically tests syncfs() to sync filesystem having large dirty file > > + * pages to block device. Also, it tests all supported filesystems on a test > > + * block device. > > + */ > > + > > +#define _GNU_SOURCE > > +#include <stdlib.h> > > +#include <stdio.h> > > +#include <sys/types.h> > > +#include "tst_test.h" > > +#include "lapi/fs.h" > > +#include "lapi/stat.h" > > + > > +#define MNTPOINT "mnt_point" > > +#define TST_FILE MNTPOINT"/test" > > +#define TST_FILE_SIZE_MB 32 > > +#define SIZE_MB (1024*1024) > > +#define MODE 0644 > > + > > +static char dev_stat_path[1024]; > > +static char *buffer; > > +static int fd; > > + > > +static void verify_syncfs(void) > > +{ > > + char nwrite_sec_val[BUFSIZ]; > > + int counter; > > + unsigned long prev_write_sec = 0, write_sec = 0; > > + > > + SAFE_FILE_SCANF(dev_stat_path, "%*s %*s %*s %*s %*s %*s %s", > > + nwrite_sec_val); > > + > > + prev_write_sec = SAFE_STRTOUL(nwrite_sec_val, 0, ULONG_MAX); > > + > > + fd = SAFE_OPEN(TST_FILE, O_RDWR|O_CREAT, MODE); > > + > > + /* Filling the test file */ > > + for (counter = 0; counter < TST_FILE_SIZE_MB; counter++) > > + SAFE_WRITE(1, fd, buffer, SIZE_MB); > > + > > + TEST(syncfs(fd)); > > + if (TST_RET != 0) > > + tst_brk(TFAIL | TTERRNO, "syncfs(fd) failed"); > > + > > + SAFE_FILE_SCANF(dev_stat_path, "%*s %*s %*s %*s %*s %*s %s", > > + nwrite_sec_val); > > + > > + write_sec = SAFE_STRTOUL(nwrite_sec_val, 0, ULONG_MAX); > > + > > + if ((write_sec - prev_write_sec) * 512 >= > > + (TST_FILE_SIZE_MB * SIZE_MB)) > > + tst_res(TPASS, "Test filesystem synced to device"); > > + else > > + tst_res(TFAIL, "Failed to sync test filesystem to device"); > > + > > + SAFE_CLOSE(fd); > > +} > > + > > +static void setup(void) > > +{ > > + struct stat st; > > + > > + snprintf(dev_stat_path, sizeof(dev_stat_path), "/sys/block/%s/stat", > > + strrchr(tst_device->dev, '/') + 1); > > + > > + if (stat(dev_stat_path, &st) != 0) > > + tst_brk(TCONF, "Test device stat file: %s not found", > > + dev_stat_path); > > + > > + buffer = SAFE_MALLOC(SIZE_MB); > > + > > + memset(buffer, 0, SIZE_MB); > > +} > > + > > +static void cleanup(void) > > +{ > > + if (buffer) > > + free(buffer); > > + > > + if (fd > 0) > > + SAFE_CLOSE(fd); > > +} > > + > > +static struct tst_test test = { > > + .needs_root = 1, > > + .mount_device = 1, > > + .all_filesystems = 1, > > + .mntpoint = MNTPOINT, > > + .setup = setup, > > + .cleanup = cleanup, > > + .test_all = verify_syncfs, > > +}; > > >
Hi! > > 1) Compilation failed on older kernel(e.g. v2.6.32) because of the > > undefined syncfs(). > > According to manpage, syncfs() was first appeared in Linux 2.6.39 and > > library > > support was added to glibc in version 2.14. Perhaps, we need to check if > > syncfs() > > is defined. > > > > Would configuring .min_kver suffice to avoid this compilation issue? Not at all, min_kver is runtime check, which could solve the latter problem. For this you can either: * Add a configure check for syscfs() * Add a fallback syscall definition to header include/lapi/ Fallback definition is preferable solution since with that the test will still work on old userspace with new kernel.
Hi! > > + > > + fd = SAFE_OPEN(TST_FILE, O_RDWR|O_CREAT, MODE); > > + > > + /* Filling the test file */ > > + for (counter = 0; counter < TST_FILE_SIZE_MB; counter++) > > + SAFE_WRITE(1, fd, buffer, SIZE_MB); > > + > > + TEST(syncfs(fd)); > > + if (TST_RET != 0) > > + tst_brk(TFAIL | TTERRNO, "syncfs(fd) failed"); > > + > > + SAFE_FILE_SCANF(dev_stat_path, "%*s %*s %*s %*s %*s %*s %s", > > + nwrite_sec_val); > > + > > + write_sec = SAFE_STRTOUL(nwrite_sec_val, 0, ULONG_MAX); > > + > > + if ((write_sec - prev_write_sec) * 512 >= > > + (TST_FILE_SIZE_MB * SIZE_MB)) > > + tst_res(TPASS, "Test filesystem synced to device"); > > + else > > + tst_res(TFAIL, "Failed to sync test filesystem to device"); > > + > > + SAFE_CLOSE(fd); > > +} > > + > > It's good to have a tests that verified syncfs() actually does what it > is meant to do. > It's awkward that none of the tests for fsync() fdatasync() sync() > sync_file_range() > check that. > > It would be very low hanging to have the exact same test that you wrote > iterate on several test cases where the only difference is the op called on > fd. (fsync,fdatasync,syncfs) should all have the same consequence wrt > minimal written sectors. > With a little more effort, sync() and sync_file_range() could also be > added to test cases. > > I realize that LTP usually puts syscalls tests under the specific > kernel/syscalls directory, but in this case, I believe code reuse calls > for a single test that exercises all three syscalls. We can always put the common code into a header/library and still have a test for each of the syscalls as we usually do in LTP.
On Fri, 15 Feb 2019 at 17:46, Cyril Hrubis <chrubis@suse.cz> wrote: > > Hi! > > > 1) Compilation failed on older kernel(e.g. v2.6.32) because of the > > > undefined syncfs(). > > > According to manpage, syncfs() was first appeared in Linux 2.6.39 and > > > library > > > support was added to glibc in version 2.14. Perhaps, we need to check if > > > syncfs() > > > is defined. > > > > > > > Would configuring .min_kver suffice to avoid this compilation issue? > > Not at all, min_kver is runtime check, which could solve the latter > problem. > > For this you can either: > > * Add a configure check for syscfs() > > * Add a fallback syscall definition to header include/lapi/ > > Fallback definition is preferable solution since with that the test will > still work on old userspace with new kernel. > Thanks for the pointers. IIUC, you are referring to following change: - TEST(syncfs(fd)); + TEST(tst_syscall(__NR_syncfs, fd)); If yes, then I will incorporate it. -Sumit > -- > Cyril Hrubis > chrubis@suse.cz
Hi! > Thanks for the pointers. IIUC, you are referring to following change: > > - TEST(syncfs(fd)); > + TEST(tst_syscall(__NR_syncfs, fd)); > > If yes, then I will incorporate it. The most complete solution is configure check + fallback definition. Have a look at: include/lapi/execveat.h m4/ltp-execveat.m4 configure.ac
Hi! > > Thanks for the pointers. IIUC, you are referring to following change: > > > > - TEST(syncfs(fd)); > > + TEST(tst_syscall(__NR_syncfs, fd)); > > > > If yes, then I will incorporate it. > > The most complete solution is configure check + fallback definition. > > Have a look at: > > include/lapi/execveat.h > m4/ltp-execveat.m4 > configure.ac And then you have to also check for the return value to exit the test with TCONF on missing kernel support, you will most likely get EINVAL in that case as the syscall number is not known.
On Fri, 15 Feb 2019 at 14:12, Amir Goldstein <amir73il@gmail.com> wrote: > > On Fri, Feb 15, 2019 at 9:17 AM Sumit Garg <sumit.garg@linaro.org> wrote: > > > > syncfs01 tests to sync filesystem having large dirty file pages to block > > device. Also, it tests all supported filesystems on a test block device. > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > --- > > > > Changes in v2: > > 1. Remove unused header file include. > > 2. Remove redundant tst_device check. > > 3. Remove redundant flags from tst_test struct. > > > > Fixes: https://github.com/linux-test-project/ltp/issues/294 > > > > runtest/syscalls | 2 + > > testcases/kernel/syscalls/syncfs/.gitignore | 1 + > > testcases/kernel/syscalls/syncfs/Makefile | 8 +++ > > testcases/kernel/syscalls/syncfs/syncfs01.c | 101 ++++++++++++++++++++++++++++ > > 4 files changed, 112 insertions(+) > > create mode 100644 testcases/kernel/syscalls/syncfs/.gitignore > > create mode 100644 testcases/kernel/syscalls/syncfs/Makefile > > create mode 100644 testcases/kernel/syscalls/syncfs/syncfs01.c > > > > diff --git a/runtest/syscalls b/runtest/syscalls > > index 668c87c..9442740 100644 > > --- a/runtest/syscalls > > +++ b/runtest/syscalls > > @@ -1346,6 +1346,8 @@ symlinkat01 symlinkat01 > > sync01 sync01 > > sync02 sync02 > > > > +syncfs01 syncfs01 > > + > > #testcases for sync_file_range > > sync_file_range01 sync_file_range01 > > > > diff --git a/testcases/kernel/syscalls/syncfs/.gitignore b/testcases/kernel/syscalls/syncfs/.gitignore > > new file mode 100644 > > index 0000000..6066295 > > --- /dev/null > > +++ b/testcases/kernel/syscalls/syncfs/.gitignore > > @@ -0,0 +1 @@ > > +syncfs01 > > diff --git a/testcases/kernel/syscalls/syncfs/Makefile b/testcases/kernel/syscalls/syncfs/Makefile > > new file mode 100644 > > index 0000000..3e6c2f4 > > --- /dev/null > > +++ b/testcases/kernel/syscalls/syncfs/Makefile > > @@ -0,0 +1,8 @@ > > +# Copyright (c) 2019 - Linaro Limited. All rights reserved. > > +# SPDX-License-Identifier: GPL-2.0-or-later > > + > > +top_srcdir ?= ../../../.. > > + > > +include $(top_srcdir)/include/mk/testcases.mk > > + > > +include $(top_srcdir)/include/mk/generic_leaf_target.mk > > diff --git a/testcases/kernel/syscalls/syncfs/syncfs01.c b/testcases/kernel/syscalls/syncfs/syncfs01.c > > new file mode 100644 > > index 0000000..e2d3e80 > > --- /dev/null > > +++ b/testcases/kernel/syscalls/syncfs/syncfs01.c > > @@ -0,0 +1,101 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Copyright (c) 2019 Linaro Limited. All rights reserved. > > + * Author: Sumit Garg <sumit.garg@linaro.org> > > + */ > > + > > +/* > > + * Test syncfs > > + * > > + * It basically tests syncfs() to sync filesystem having large dirty file > > + * pages to block device. Also, it tests all supported filesystems on a test > > + * block device. > > + */ > > + > > +#define _GNU_SOURCE > > +#include <stdlib.h> > > +#include <stdio.h> > > +#include <sys/types.h> > > +#include "tst_test.h" > > +#include "lapi/fs.h" > > +#include "lapi/stat.h" > > + > > +#define MNTPOINT "mnt_point" > > +#define TST_FILE MNTPOINT"/test" > > +#define TST_FILE_SIZE_MB 32 > > +#define SIZE_MB (1024*1024) > > +#define MODE 0644 > > + > > +static char dev_stat_path[1024]; > > +static char *buffer; > > +static int fd; > > + > > +static void verify_syncfs(void) > > +{ > > + char nwrite_sec_val[BUFSIZ]; > > + int counter; > > + unsigned long prev_write_sec = 0, write_sec = 0; > > + > > + SAFE_FILE_SCANF(dev_stat_path, "%*s %*s %*s %*s %*s %*s %s", > > + nwrite_sec_val); > > + > > + prev_write_sec = SAFE_STRTOUL(nwrite_sec_val, 0, ULONG_MAX); > > Why not let scanf read %ul? > Hmm, will use %lu here. > > + > > + fd = SAFE_OPEN(TST_FILE, O_RDWR|O_CREAT, MODE); > > + > > + /* Filling the test file */ > > + for (counter = 0; counter < TST_FILE_SIZE_MB; counter++) > > + SAFE_WRITE(1, fd, buffer, SIZE_MB); > > + > > + TEST(syncfs(fd)); > > + if (TST_RET != 0) > > + tst_brk(TFAIL | TTERRNO, "syncfs(fd) failed"); > > + > > + SAFE_FILE_SCANF(dev_stat_path, "%*s %*s %*s %*s %*s %*s %s", > > + nwrite_sec_val); > > + > > + write_sec = SAFE_STRTOUL(nwrite_sec_val, 0, ULONG_MAX); > > + > > + if ((write_sec - prev_write_sec) * 512 >= > > + (TST_FILE_SIZE_MB * SIZE_MB)) > > + tst_res(TPASS, "Test filesystem synced to device"); > > + else > > + tst_res(TFAIL, "Failed to sync test filesystem to device"); > > + > > + SAFE_CLOSE(fd); > > +} > > + > > It's good to have a tests that verified syncfs() actually does what it > is meant to do. > It's awkward that none of the tests for fsync() fdatasync() sync() > sync_file_range() > check that. > Agree. > It would be very low hanging to have the exact same test that you wrote > iterate on several test cases where the only difference is the op called on > fd. (fsync,fdatasync,syncfs) should all have the same consequence wrt > minimal written sectors. > With a little more effort, sync() and sync_file_range() could also be > added to test cases. > Sounds good to me. > I realize that LTP usually puts syscalls tests under the specific > kernel/syscalls directory, but in this case, I believe code reuse calls > for a single test that exercises all three syscalls. > As Cyril said, will explore addition of common code in a header/library and add corresponding test-case for fsync(), fdatasync(), syncfs(), sync() and sync_file_range(). -Sumit > Thanks, > Amir.
On Fri, 15 Feb 2019 at 18:55, Cyril Hrubis <chrubis@suse.cz> wrote: > > Hi! > > > Thanks for the pointers. IIUC, you are referring to following change: > > > > > > - TEST(syncfs(fd)); > > > + TEST(tst_syscall(__NR_syncfs, fd)); > > > > > > If yes, then I will incorporate it. > > > > The most complete solution is configure check + fallback definition. > > > > Have a look at: > > > > include/lapi/execveat.h > > m4/ltp-execveat.m4 > > configure.ac > > And then you have to also check for the return value to exit the test > with TCONF on missing kernel support, you will most likely get EINVAL in > that case as the syscall number is not known. > Ok got it. Will incorporate it. -Sumit > -- > Cyril Hrubis > chrubis@suse.cz
On 02/15/2019 05:22 AM, Cyril Hrubis wrote: > Hi! >> Thanks for the pointers. IIUC, you are referring to following change: >> >> - TEST(syncfs(fd)); >> + TEST(tst_syscall(__NR_syncfs, fd)); >> >> If yes, then I will incorporate it. > > The most complete solution is configure check + fallback definition. > > Have a look at: > > include/lapi/execveat.h > m4/ltp-execveat.m4 > configure.ac For tests in testcases/kernel/syscalls, should the tests typically directly call the syscall? I'd think this is preferable to the use of C library wrappers as these tests (by their location in the tree) seem to be focused on the syscall functionality in the kernel. thanks, steve
On Sat, 16 Feb 2019 at 05:47, Steve Muckle <smuckle@google.com> wrote: > > On 02/15/2019 05:22 AM, Cyril Hrubis wrote: > > Hi! > >> Thanks for the pointers. IIUC, you are referring to following change: > >> > >> - TEST(syncfs(fd)); > >> + TEST(tst_syscall(__NR_syncfs, fd)); > >> > >> If yes, then I will incorporate it. > > > > The most complete solution is configure check + fallback definition. > > > > Have a look at: > > > > include/lapi/execveat.h > > m4/ltp-execveat.m4 > > configure.ac > > For tests in testcases/kernel/syscalls, should the tests typically > directly call the syscall? I'd think this is preferable to the use of C > library wrappers as these tests (by their location in the tree) seem to > be focused on the syscall functionality in the kernel. > Sounds reasonable to me. But as per following manpage text for syscall(): "Each architecture ABI has its own requirements on how system call arguments are passed to the kernel. For system calls that have a glibc wrapper (e.g., most system calls), glibc handles the details of copying arguments to the right registers in a manner suitable for the architecture. However, when using syscall() to make a system call, the caller might need to handle architecture-dependent details; this requirement is most commonly encountered on certain 32-bit architectures." So I am not sure how we handle this architecture specific stuff if required in test-cases. -Sumit > thanks, > steve
Hi! > >> Thanks for the pointers. IIUC, you are referring to following change: > >> > >> - TEST(syncfs(fd)); > >> + TEST(tst_syscall(__NR_syncfs, fd)); > >> > >> If yes, then I will incorporate it. > > > > The most complete solution is configure check + fallback definition. > > > > Have a look at: > > > > include/lapi/execveat.h > > m4/ltp-execveat.m4 > > configure.ac > > For tests in testcases/kernel/syscalls, should the tests typically > directly call the syscall? I'd think this is preferable to the use of C > library wrappers as these tests (by their location in the tree) seem to > be focused on the syscall functionality in the kernel. My take on this is that for a functional testing we really have to test the kernel together with the library that wraps the syscalls because otherwise bugs are bound to happen such as these: https://sourceware.org/bugzilla/show_bug.cgi?id=23069 https://sourceware.org/bugzilla/show_bug.cgi?id=23579 And I always cared about syscalls being correct on the C library level. I guess that for most of the syscalls that are just thin wrappers it does not matter since these just prepare the parameters and jump to the kernel, but in certain cases libc does quite a lot of work which is sometimes complex code I do not want to replicate that unless really needed. And with that I think that it's actually much easier to go with the libc API whenever possible rather than reviewing the libc code each time we write a testcase. Does that sound reasonable to you?
Hi! > > >> Thanks for the pointers. IIUC, you are referring to following change: > > >> > > >> - TEST(syncfs(fd)); > > >> + TEST(tst_syscall(__NR_syncfs, fd)); > > >> > > >> If yes, then I will incorporate it. > > > > > > The most complete solution is configure check + fallback definition. > > > > > > Have a look at: > > > > > > include/lapi/execveat.h > > > m4/ltp-execveat.m4 > > > configure.ac > > > > For tests in testcases/kernel/syscalls, should the tests typically > > directly call the syscall? I'd think this is preferable to the use of C > > library wrappers as these tests (by their location in the tree) seem to > > be focused on the syscall functionality in the kernel. > > > > Sounds reasonable to me. But as per following manpage text for syscall(): > > "Each architecture ABI has its own requirements on how system call > arguments are passed to the kernel. For system calls that have a > glibc wrapper (e.g., most system calls), glibc handles the details > of copying arguments to the right registers in a manner suitable for > the architecture. However, when using syscall() to make a system call, > the caller might need to handle architecture-dependent details; this > requirement is most commonly encountered on certain 32-bit > architectures." > > So I am not sure how we handle this architecture specific stuff if > required in test-cases. In more than 99% of the cases this is not an issue, as far as the arguments are passed as long int and we do not have to pass more than six, which is the common case, we don't have to do anything. In some cases we have to split 64bit values in half and pass them as two arguments, then we put the code into the lapi hader such as include/lapi/fallocate.h. Then there are cases where we pass a pointer to architecture specific structure such as the sigaction which sometimes contains a pointer to function that restores the program state after we return from the signal handler and this is where calling the kernel syscall directly gets complex. We do carry that code under include/lapi/rt_sigaction.h.
Hi Xiao, On Fri, 15 Feb 2019 at 13:25, Xiao Yang <yangx.jy@cn.fujitsu.com> wrote: > > Hi Sumit, > > Sorry for the late reply. :-) > > 1) Compilation failed on older kernel(e.g. v2.6.32) because of the > undefined syncfs(). > According to manpage, syncfs() was first appeared in Linux 2.6.39 and > library > support was added to glibc in version 2.14. Perhaps, we need to check if > syncfs() > is defined. > > 2) Running this test got TBROK on older kernel(e.g. v3.10.0), as below: > ------------------------------------------------------------------- > syncfs01.c:63: FAIL: Failed to sync test filesystem to device > ------------------------------------------------------------------- > It seems that the content of /sys/block/<loopX>/stat is always zero and > doesn't update even though write and sync have been done. I am looking > into it, but i am not sure if this is a known bug on older kernel. > Did you get a chance to root cause this issue? I am not able to reproduce setup running kernel v3.10. If possible, can you please share your setup details? As Cyril suggested, we could solve this issue via configuring min_kver value. But I am not sure regarding appropriate value. BTW, as this seems to be kernel issue (/sys/block/<loopX>/stat not updated), do we really don't want to run this test-case on old kernel where it fails? To me this failure case is value add for user of old kernel to be aware about this issue and probably fix it. -Sumit > Best Regards, > Xiao Yang
On 02/18/2019 03:57 AM, Cyril Hrubis wrote: >> For tests in testcases/kernel/syscalls, should the tests typically >> directly call the syscall? I'd think this is preferable to the use of C >> library wrappers as these tests (by their location in the tree) seem to >> be focused on the syscall functionality in the kernel. > > My take on this is that for a functional testing we really have to test > the kernel together with the library that wraps the syscalls because > otherwise bugs are bound to happen such as these: > > https://sourceware.org/bugzilla/show_bug.cgi?id=23069 > https://sourceware.org/bugzilla/show_bug.cgi?id=23579 > > And I always cared about syscalls being correct on the C library level. > > I guess that for most of the syscalls that are just thin wrappers it > does not matter since these just prepare the parameters and jump to the > kernel, but in certain cases libc does quite a lot of work which is > sometimes complex code I do not want to replicate that unless really > needed. And with that I think that it's actually much easier to go with > the libc API whenever possible rather than reviewing the libc code each > time we write a testcase. > > Does that sound reasonable to you? Sure. My concern is being able to test syscalls in Android where the C library may not have some wrappers. So far these have all been cases where one can just replace the library call with the direct syscall, as a number of recent patches have done. If I run into a case where more substantial library support is needed maybe we'll just have to focus on getting that into bionic or look at other options. thanks, steve
Hi! > Sure. My concern is being able to test syscalls in Android where the C > library may not have some wrappers. So far these have all been cases > where one can just replace the library call with the direct syscall, as > a number of recent patches have done. If I run into a case where more > substantial library support is needed maybe we'll just have to focus on > getting that into bionic or look at other options. I guess that we can also auto-generate fallback syscall wrappers for these cases so that we don't have to bother dealing with this in the actual testcases. Should be as easy as listing the function prototypes in a file and a few lines of shell.
Hi! > > Sure. My concern is being able to test syscalls in Android where the C > > library may not have some wrappers. So far these have all been cases > > where one can just replace the library call with the direct syscall, as > > a number of recent patches have done. If I run into a case where more > > substantial library support is needed maybe we'll just have to focus on > > getting that into bionic or look at other options. > > I guess that we can also auto-generate fallback syscall wrappers for > these cases so that we don't have to bother dealing with this in the > actual testcases. Should be as easy as listing the function prototypes > in a file and a few lines of shell. Thinking of this, generating fallbacks is the easy part, figuring out when to use them is the complex one. I wanted to use a trick with weak function symbols so that the linker would pick up these fallbacks only if there was no system defintion but that unfortunately does not work because glibc uses weak symbols for symbol versioning. I guess that we can use autoconf to generate fallback functions automatically if syscall wrapper is found to be missing, but that would be a bit ugly code. Also do you even use the configure script on android builds?
On Tue, 19 Feb 2019 at 12:14, Sumit Garg <sumit.garg@linaro.org> wrote: > > Hi Xiao, > > On Fri, 15 Feb 2019 at 13:25, Xiao Yang <yangx.jy@cn.fujitsu.com> wrote: > > > > Hi Sumit, > > > > Sorry for the late reply. :-) > > > > 1) Compilation failed on older kernel(e.g. v2.6.32) because of the > > undefined syncfs(). > > According to manpage, syncfs() was first appeared in Linux 2.6.39 and > > library > > support was added to glibc in version 2.14. Perhaps, we need to check if > > syncfs() > > is defined. > > > > 2) Running this test got TBROK on older kernel(e.g. v3.10.0), as below: > > ------------------------------------------------------------------- > > syncfs01.c:63: FAIL: Failed to sync test filesystem to device > > ------------------------------------------------------------------- > > It seems that the content of /sys/block/<loopX>/stat is always zero and > > doesn't update even though write and sync have been done. I am looking > > into it, but i am not sure if this is a known bug on older kernel. > > > > Did you get a chance to root cause this issue? > > I am not able to reproduce setup running kernel v3.10. If possible, > can you please share your setup details? > > As Cyril suggested, we could solve this issue via configuring min_kver > value. But I am not sure regarding appropriate value. > > BTW, as this seems to be kernel issue (/sys/block/<loopX>/stat not > updated), do we really don't want to run this test-case on old kernel > where it fails? To me this failure case is value add for user of old > kernel to be aware about this issue and probably fix it. > Thinking more about broken /sys/block/<loopX>/stat [1]. It seems that following param should be a good indicator of block device stat file being broken: io_ticks milliseconds total time this block device has been active This param should be non-zero inside test-case as there are already some file-system operations performed on the device like formatting etc. If you could confirm that you see this param as zero on older kernel (v3.10), then I will add a TCONF check for this param being non-zero as follows: - SAFE_FILE_SCANF(NULL, dev_stat_path, "%*s %*s %*s %*s %*s %*s %lu", - &dev_sec_write); + SAFE_FILE_SCANF(NULL, dev_stat_path, + "%*s %*s %*s %*s %*s %*s %lu %*s %*s %lu", + &dev_sec_write, &io_ticks); + + if (!io_ticks) + tst_brkm(TCONF, NULL, "Test device stat file: %s broken", + dev_stat_path); [1] https://www.kernel.org/doc/Documentation/block/stat.txt -Sumit > > > Best Regards, > > Xiao Yang
On 02/20/2019 07:12 AM, Cyril Hrubis wrote: > Hi! >>> Sure. My concern is being able to test syscalls in Android where the C >>> library may not have some wrappers. So far these have all been cases >>> where one can just replace the library call with the direct syscall, as >>> a number of recent patches have done. If I run into a case where more >>> substantial library support is needed maybe we'll just have to focus on >>> getting that into bionic or look at other options. >> >> I guess that we can also auto-generate fallback syscall wrappers for >> these cases so that we don't have to bother dealing with this in the >> actual testcases. Should be as easy as listing the function prototypes >> in a file and a few lines of shell. > > Thinking of this, generating fallbacks is the easy part, figuring out > when to use them is the complex one. I wanted to use a trick with weak > function symbols so that the linker would pick up these fallbacks only > if there was no system defintion but that unfortunately does not work > because glibc uses weak symbols for symbol versioning. I guess that we > can use autoconf to generate fallback functions automatically if syscall > wrapper is found to be missing, but that would be a bit ugly code. Also > do you even use the configure script on android builds? We do use the configure script. But I guess given the discussion in the other thread, hopefully both the libc wrapper and direct syscall can be tested.
diff --git a/runtest/syscalls b/runtest/syscalls index 668c87c..9442740 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -1346,6 +1346,8 @@ symlinkat01 symlinkat01 sync01 sync01 sync02 sync02 +syncfs01 syncfs01 + #testcases for sync_file_range sync_file_range01 sync_file_range01 diff --git a/testcases/kernel/syscalls/syncfs/.gitignore b/testcases/kernel/syscalls/syncfs/.gitignore new file mode 100644 index 0000000..6066295 --- /dev/null +++ b/testcases/kernel/syscalls/syncfs/.gitignore @@ -0,0 +1 @@ +syncfs01 diff --git a/testcases/kernel/syscalls/syncfs/Makefile b/testcases/kernel/syscalls/syncfs/Makefile new file mode 100644 index 0000000..3e6c2f4 --- /dev/null +++ b/testcases/kernel/syscalls/syncfs/Makefile @@ -0,0 +1,8 @@ +# Copyright (c) 2019 - Linaro Limited. All rights reserved. +# SPDX-License-Identifier: GPL-2.0-or-later + +top_srcdir ?= ../../../.. + +include $(top_srcdir)/include/mk/testcases.mk + +include $(top_srcdir)/include/mk/generic_leaf_target.mk diff --git a/testcases/kernel/syscalls/syncfs/syncfs01.c b/testcases/kernel/syscalls/syncfs/syncfs01.c new file mode 100644 index 0000000..e2d3e80 --- /dev/null +++ b/testcases/kernel/syscalls/syncfs/syncfs01.c @@ -0,0 +1,101 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2019 Linaro Limited. All rights reserved. + * Author: Sumit Garg <sumit.garg@linaro.org> + */ + +/* + * Test syncfs + * + * It basically tests syncfs() to sync filesystem having large dirty file + * pages to block device. Also, it tests all supported filesystems on a test + * block device. + */ + +#define _GNU_SOURCE +#include <stdlib.h> +#include <stdio.h> +#include <sys/types.h> +#include "tst_test.h" +#include "lapi/fs.h" +#include "lapi/stat.h" + +#define MNTPOINT "mnt_point" +#define TST_FILE MNTPOINT"/test" +#define TST_FILE_SIZE_MB 32 +#define SIZE_MB (1024*1024) +#define MODE 0644 + +static char dev_stat_path[1024]; +static char *buffer; +static int fd; + +static void verify_syncfs(void) +{ + char nwrite_sec_val[BUFSIZ]; + int counter; + unsigned long prev_write_sec = 0, write_sec = 0; + + SAFE_FILE_SCANF(dev_stat_path, "%*s %*s %*s %*s %*s %*s %s", + nwrite_sec_val); + + prev_write_sec = SAFE_STRTOUL(nwrite_sec_val, 0, ULONG_MAX); + + fd = SAFE_OPEN(TST_FILE, O_RDWR|O_CREAT, MODE); + + /* Filling the test file */ + for (counter = 0; counter < TST_FILE_SIZE_MB; counter++) + SAFE_WRITE(1, fd, buffer, SIZE_MB); + + TEST(syncfs(fd)); + if (TST_RET != 0) + tst_brk(TFAIL | TTERRNO, "syncfs(fd) failed"); + + SAFE_FILE_SCANF(dev_stat_path, "%*s %*s %*s %*s %*s %*s %s", + nwrite_sec_val); + + write_sec = SAFE_STRTOUL(nwrite_sec_val, 0, ULONG_MAX); + + if ((write_sec - prev_write_sec) * 512 >= + (TST_FILE_SIZE_MB * SIZE_MB)) + tst_res(TPASS, "Test filesystem synced to device"); + else + tst_res(TFAIL, "Failed to sync test filesystem to device"); + + SAFE_CLOSE(fd); +} + +static void setup(void) +{ + struct stat st; + + snprintf(dev_stat_path, sizeof(dev_stat_path), "/sys/block/%s/stat", + strrchr(tst_device->dev, '/') + 1); + + if (stat(dev_stat_path, &st) != 0) + tst_brk(TCONF, "Test device stat file: %s not found", + dev_stat_path); + + buffer = SAFE_MALLOC(SIZE_MB); + + memset(buffer, 0, SIZE_MB); +} + +static void cleanup(void) +{ + if (buffer) + free(buffer); + + if (fd > 0) + SAFE_CLOSE(fd); +} + +static struct tst_test test = { + .needs_root = 1, + .mount_device = 1, + .all_filesystems = 1, + .mntpoint = MNTPOINT, + .setup = setup, + .cleanup = cleanup, + .test_all = verify_syncfs, +};
syncfs01 tests to sync filesystem having large dirty file pages to block device. Also, it tests all supported filesystems on a test block device. Signed-off-by: Sumit Garg <sumit.garg@linaro.org> --- Changes in v2: 1. Remove unused header file include. 2. Remove redundant tst_device check. 3. Remove redundant flags from tst_test struct. Fixes: https://github.com/linux-test-project/ltp/issues/294 runtest/syscalls | 2 + testcases/kernel/syscalls/syncfs/.gitignore | 1 + testcases/kernel/syscalls/syncfs/Makefile | 8 +++ testcases/kernel/syscalls/syncfs/syncfs01.c | 101 ++++++++++++++++++++++++++++ 4 files changed, 112 insertions(+) create mode 100644 testcases/kernel/syscalls/syncfs/.gitignore create mode 100644 testcases/kernel/syscalls/syncfs/Makefile create mode 100644 testcases/kernel/syscalls/syncfs/syncfs01.c