[LTP] [PATCH v2] syscalls: add syscall syncfs test

Sumit Garg
2019-02-15T13:38:13Z

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.