Re: [PATCH v2 2/2] selftests: cachestat: catch failing fsync test on tmpfs
From: Nhat Pham
Date: Tue Aug 22 2023 - 11:55:55 EST
On Mon, Aug 21, 2023 at 9:05 AM Andre Przywara <andre.przywara@xxxxxxx> wrote:
>
> The cachestat kselftest runs a test on a normal file, which is created
> temporarily in the current directory. Among the tests it runs there is a
> call to fsync(), which is expected to clean all dirty pages used by the
> file.
> However the tmpfs filesystem implements fsync() as noop_fsync(), so the
> call will not even attempt to clean anything when this test file happens
> to live on a tmpfs instance. This happens in an initramfs, or when the
> current directory is in /dev/shm or sometimes /tmp.
>
> To avoid this test failing wrongly, use statfs() to check which filesystem
> the test file lives on. If that is "tmpfs", we skip the fsync() test.
>
> Since the fsync test is only one part of the "normal file" test, we now
> execute this twice, skipping the fsync part on the first call.
> This way only the second test, including the fsync part, would be skipped.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
> .../selftests/cachestat/test_cachestat.c | 62 ++++++++++++++-----
> 1 file changed, 47 insertions(+), 15 deletions(-)
>
> diff --git a/tools/testing/selftests/cachestat/test_cachestat.c b/tools/testing/selftests/cachestat/test_cachestat.c
> index 8f8f46c24846d..4804c7dc7b312 100644
> --- a/tools/testing/selftests/cachestat/test_cachestat.c
> +++ b/tools/testing/selftests/cachestat/test_cachestat.c
> @@ -4,10 +4,12 @@
> #include <stdio.h>
> #include <stdbool.h>
> #include <linux/kernel.h>
> +#include <linux/magic.h>
> #include <linux/mman.h>
> #include <sys/mman.h>
> #include <sys/shm.h>
> #include <sys/syscall.h>
> +#include <sys/vfs.h>
> #include <unistd.h>
> #include <string.h>
> #include <fcntl.h>
> @@ -15,7 +17,7 @@
>
> #include "../kselftest.h"
>
> -#define NR_TESTS 8
> +#define NR_TESTS 9
>
> static const char * const dev_files[] = {
> "/dev/zero", "/dev/null", "/dev/urandom",
> @@ -91,6 +93,20 @@ bool write_exactly(int fd, size_t filesize)
> return ret;
> }
>
> +/*
> + * fsync() is implemented via noop_fsync() on tmpfs. This makes the fsync()
> + * test fail below, so we need to check for test file living on a tmpfs.
> + */
> +static bool is_on_tmpfs(int fd)
> +{
> + struct statfs statfs_buf;
> +
> + if (fstatfs(fd, &statfs_buf))
> + return false;
> +
> + return statfs_buf.f_type == TMPFS_MAGIC;
> +}
> +
> /*
> * Open/create the file at filename, (optionally) write random data to it
> * (exactly num_pages), then test the cachestat syscall on this file.
> @@ -98,13 +114,13 @@ bool write_exactly(int fd, size_t filesize)
> * If test_fsync == true, fsync the file, then check the number of dirty
> * pages.
> */
> -bool test_cachestat(const char *filename, bool write_random, bool create,
> - bool test_fsync, unsigned long num_pages, int open_flags,
> - mode_t open_mode)
> +static int test_cachestat(const char *filename, bool write_random, bool create,
> + bool test_fsync, unsigned long num_pages,
> + int open_flags, mode_t open_mode)
> {
Hmm would the semantic change a bit here?
Previously, this function returned true if passed.
But it seems like KSFT_PASS is defined as 0:
https://github.com/torvalds/linux/blob/9e38be7/tools/testing/selftests/kselftest.h#L74
So maybe we have to change from:
if (test_<test-name>())
to:
if (!test_<test-name>)())
or, explicitly as:
if (test_<test-name>() == KSFT_PASS)
> size_t PS = sysconf(_SC_PAGESIZE);
> int filesize = num_pages * PS;
> - bool ret = true;
> + int ret = KSFT_PASS;
> long syscall_ret;
> struct cachestat cs;
> struct cachestat_range cs_range = { 0, filesize };
> @@ -113,7 +129,7 @@ bool test_cachestat(const char *filename, bool write_random, bool create,
>
> if (fd == -1) {
> ksft_print_msg("Unable to create/open file.\n");
> - ret = false;
> + ret = KSFT_FAIL;
> goto out;
> } else {
> ksft_print_msg("Create/open %s\n", filename);
> @@ -122,7 +138,7 @@ bool test_cachestat(const char *filename, bool write_random, bool create,
> if (write_random) {
> if (!write_exactly(fd, filesize)) {
> ksft_print_msg("Unable to access urandom.\n");
> - ret = false;
> + ret = KSFT_FAIL;
> goto out1;
> }
> }
> @@ -133,7 +149,7 @@ bool test_cachestat(const char *filename, bool write_random, bool create,
>
> if (syscall_ret) {
> ksft_print_msg("Cachestat returned non-zero.\n");
> - ret = false;
> + ret = KSFT_FAIL;
> goto out1;
>
> } else {
> @@ -143,15 +159,17 @@ bool test_cachestat(const char *filename, bool write_random, bool create,
> if (cs.nr_cache + cs.nr_evicted != num_pages) {
> ksft_print_msg(
> "Total number of cached and evicted pages is off.\n");
> - ret = false;
> + ret = KSFT_FAIL;
> }
> }
> }
>
> if (test_fsync) {
> - if (fsync(fd)) {
> + if (is_on_tmpfs(fd)) {
> + ret = KSFT_SKIP;
> + } else if (fsync(fd)) {
> ksft_print_msg("fsync fails.\n");
> - ret = false;
> + ret = KSFT_FAIL;
> } else {
> syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
>
> @@ -162,13 +180,13 @@ bool test_cachestat(const char *filename, bool write_random, bool create,
> print_cachestat(&cs);
>
> if (cs.nr_dirty) {
> - ret = false;
> + ret = KSFT_FAIL;
> ksft_print_msg(
> "Number of dirty should be zero after fsync.\n");
> }
> } else {
> ksft_print_msg("Cachestat (after fsync) returned non-zero.\n");
> - ret = false;
> + ret = KSFT_FAIL;
> goto out1;
> }
> }
> @@ -259,7 +277,7 @@ int main(void)
> const char *dev_filename = dev_files[i];
>
> if (test_cachestat(dev_filename, false, false, false,
> - 4, O_RDONLY, 0400))
> + 4, O_RDONLY, 0400) == KSFT_PASS)
> ksft_test_result_pass("cachestat works with %s\n", dev_filename);
> else {
> ksft_test_result_fail("cachestat fails with %s\n", dev_filename);
> @@ -268,13 +286,27 @@ int main(void)
> }
>
> if (test_cachestat("tmpfilecachestat", true, true,
> - true, 4, O_CREAT | O_RDWR, 0400 | 0600))
> + false, 4, O_CREAT | O_RDWR, 0600) == KSFT_PASS)
> ksft_test_result_pass("cachestat works with a normal file\n");
> else {
> ksft_test_result_fail("cachestat fails with normal file\n");
> ret = 1;
> }
>
> + switch (test_cachestat("tmpfilecachestat", true, true,
> + true, 4, O_CREAT | O_RDWR, 0600)) {
> + case KSFT_FAIL:
> + ksft_test_result_fail("cachestat fsync fails with normal file\n");
> + ret = KSFT_FAIL;
> + break;
> + case KSFT_PASS:
> + ksft_test_result_pass("cachestat fsync works with a normal file\n");
> + break;
> + case KSFT_SKIP:
> + ksft_test_result_skip("tmpfilecachestat is on tmpfs\n");
> + break;
> + }
> +
> if (test_cachestat_shmem())
> ksft_test_result_pass("cachestat works with a shmem file\n");
> else {
> --
> 2.25.1
>