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 Tue, Aug 22, 2023 at 8:55 AM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
>
> 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)
Ah never mind, ignore this. I didn't see your change
down in the main function.

Everything LGTM!
Acked-by: Nhat Pham <nphamcs@xxxxxxxxx>
>
>
> >         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