Re: [PATCH v2 2/2] selftests: cachestat: catch failing fsync test on tmpfs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
>




[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux