linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] selftests: cachestat: fix run on older kernels
@ 2023-08-21 16:05 Andre Przywara
  2023-08-21 16:05 ` [PATCH v2 1/2] selftests: cachestat: test for cachestat availability Andre Przywara
  2023-08-21 16:05 ` [PATCH v2 2/2] selftests: cachestat: catch failing fsync test on tmpfs Andre Przywara
  0 siblings, 2 replies; 5+ messages in thread
From: Andre Przywara @ 2023-08-21 16:05 UTC (permalink / raw)
  To: Shuah Khan, Nhat Pham, Johannes Weiner
  Cc: linux-kselftest, linux-mm, linux-kernel

I ran all kernel selftests on some test machine, and stumbled upon
cachestat failing (among others).
These patches fix the run on older kernels and when the current
directory is on a tmpfs instance.

I dropped the first two fix patches from v1, since Shuah applied those
already. [PATCH v2 1/2] is almost the same as [PATCH 3/3] from v1, but
using the proper skip function from kselftest.h. I am not sure if Shuah
applied that already, if yes, it's not a big problem, the output is the
same.

Patch 2/2 implements the tmpfs detection that Nhat suggested the last
time (many thanks for pointing me to statfs and the magics!).

Cheers,
Andre

Andre Przywara (2):
  selftests: cachestat: test for cachestat availability
  selftests: cachestat: catch failing fsync test on tmpfs

 .../selftests/cachestat/test_cachestat.c      | 80 +++++++++++++++----
 1 file changed, 65 insertions(+), 15 deletions(-)

-- 
2.25.1



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2 1/2] selftests: cachestat: test for cachestat availability
  2023-08-21 16:05 [PATCH v2 0/2] selftests: cachestat: fix run on older kernels Andre Przywara
@ 2023-08-21 16:05 ` Andre Przywara
  2023-08-21 16:05 ` [PATCH v2 2/2] selftests: cachestat: catch failing fsync test on tmpfs Andre Przywara
  1 sibling, 0 replies; 5+ messages in thread
From: Andre Przywara @ 2023-08-21 16:05 UTC (permalink / raw)
  To: Shuah Khan, Nhat Pham, Johannes Weiner
  Cc: linux-kselftest, linux-mm, linux-kernel

As cachestat is a new syscall, it won't be available on older kernels,
for instance those running on a development machine. At the moment the
test reports all tests as "not ok" in this case.

Test for the cachestat syscall availability first, before doing further
tests, and bail out early with a TAP SKIP comment.

This also uses the opportunity to add the proper TAP headers, and add
one check for proper error handling (illegal file descriptor).

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Acked-by: Nhat Pham <nphamcs@gmail.com>
---
 .../selftests/cachestat/test_cachestat.c      | 20 ++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/cachestat/test_cachestat.c b/tools/testing/selftests/cachestat/test_cachestat.c
index a5a4ac8dcb76c..8f8f46c24846d 100644
--- a/tools/testing/selftests/cachestat/test_cachestat.c
+++ b/tools/testing/selftests/cachestat/test_cachestat.c
@@ -15,6 +15,8 @@
 
 #include "../kselftest.h"
 
+#define NR_TESTS	8
+
 static const char * const dev_files[] = {
 	"/dev/zero", "/dev/null", "/dev/urandom",
 	"/proc/version", "/proc"
@@ -235,7 +237,23 @@ bool test_cachestat_shmem(void)
 
 int main(void)
 {
-	int ret = 0;
+	int ret;
+
+	ksft_print_header();
+
+	ret = syscall(__NR_cachestat, -1, NULL, NULL, 0);
+	if (ret == -1 && errno == ENOSYS)
+		ksft_exit_skip("cachestat syscall not available\n");
+
+	ksft_set_plan(NR_TESTS);
+
+	if (ret == -1 && errno == EBADF) {
+		ksft_test_result_pass("bad file descriptor recognized\n");
+		ret = 0;
+	} else {
+		ksft_test_result_fail("bad file descriptor ignored\n");
+		ret = 1;
+	}
 
 	for (int i = 0; i < 5; i++) {
 		const char *dev_filename = dev_files[i];
-- 
2.25.1



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2 2/2] selftests: cachestat: catch failing fsync test on tmpfs
  2023-08-21 16:05 [PATCH v2 0/2] selftests: cachestat: fix run on older kernels Andre Przywara
  2023-08-21 16:05 ` [PATCH v2 1/2] selftests: cachestat: test for cachestat availability Andre Przywara
@ 2023-08-21 16:05 ` Andre Przywara
  2023-08-22 15:55   ` Nhat Pham
  1 sibling, 1 reply; 5+ messages in thread
From: Andre Przywara @ 2023-08-21 16:05 UTC (permalink / raw)
  To: Shuah Khan, Nhat Pham, Johannes Weiner
  Cc: linux-kselftest, linux-mm, linux-kernel

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@arm.com>
---
 .../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)
 {
 	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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 2/2] selftests: cachestat: catch failing fsync test on tmpfs
  2023-08-21 16:05 ` [PATCH v2 2/2] selftests: cachestat: catch failing fsync test on tmpfs Andre Przywara
@ 2023-08-22 15:55   ` Nhat Pham
  2023-08-22 16:58     ` Nhat Pham
  0 siblings, 1 reply; 5+ messages in thread
From: Nhat Pham @ 2023-08-22 15:55 UTC (permalink / raw)
  To: Andre Przywara
  Cc: Shuah Khan, Johannes Weiner, linux-kselftest, linux-mm, linux-kernel

On Mon, Aug 21, 2023 at 9:05 AM Andre Przywara <andre.przywara@arm.com> 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@arm.com>
> ---
>  .../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
>


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 2/2] selftests: cachestat: catch failing fsync test on tmpfs
  2023-08-22 15:55   ` Nhat Pham
@ 2023-08-22 16:58     ` Nhat Pham
  0 siblings, 0 replies; 5+ messages in thread
From: Nhat Pham @ 2023-08-22 16:58 UTC (permalink / raw)
  To: Andre Przywara
  Cc: Shuah Khan, Johannes Weiner, linux-kselftest, linux-mm, linux-kernel

On Tue, Aug 22, 2023 at 8:55 AM Nhat Pham <nphamcs@gmail.com> wrote:
>
> On Mon, Aug 21, 2023 at 9:05 AM Andre Przywara <andre.przywara@arm.com> 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@arm.com>
> > ---
> >  .../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@gmail.com>
>
>
> >         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
> >


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2023-08-22 16:59 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-21 16:05 [PATCH v2 0/2] selftests: cachestat: fix run on older kernels Andre Przywara
2023-08-21 16:05 ` [PATCH v2 1/2] selftests: cachestat: test for cachestat availability Andre Przywara
2023-08-21 16:05 ` [PATCH v2 2/2] selftests: cachestat: catch failing fsync test on tmpfs Andre Przywara
2023-08-22 15:55   ` Nhat Pham
2023-08-22 16:58     ` Nhat Pham

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox