Re: [PATCH 2/3] ext4: test for DAX + journaling corruption

[Eryu Guan]

On Mon, Sep 11, 2017 at 10:45:20PM -0600, Ross Zwisler wrote:
> Add a regression test for the following kernel commit:
> 
>   ext4: prevent data corruption with journaling + DAX
> 
> The test passes if either we successfully compare the data between the mmap
> with journaling turned on and the one with journaling turned off, or if we
> fail the chattr command to turn on or off journaling.  The latter is how we
> prevent this issue in the kernel.

Yeah, I noticed that mounting ext4 with "-o dax,data=journal" is not
allowed, enabling data journaling on a dax mount should be stopped too.

> 
> Signed-off-by: Ross Zwisler 
> ---
>  .gitignore  |  1 +
>  src/Makefile|  2 +-
>  src/t_ext4_dax_journal_corruption.c | 93 
> +
>  tests/ext4/030  | 68 +++
>  tests/ext4/030.out  |  2 +
>  tests/ext4/group|  1 +
>  6 files changed, 166 insertions(+), 1 deletion(-)
>  create mode 100644 src/t_ext4_dax_journal_corruption.c
>  create mode 100755 tests/ext4/030
>  create mode 100644 tests/ext4/030.out
> 
> diff --git a/.gitignore b/.gitignore
> index 2accc37..4bdc5bf 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -154,6 +154,7 @@
>  /src/t_mmap_stale_pmd
>  /src/t_mmap_cow_race
>  /src/t_mmap_fallocate
> +/src/t_ext4_dax_journal_corruption

Better to add new entry in alphabetical order, I know there're already
some out-of-order entries there, but this one is not affected and better
to stop adding new ones :)

>  
>  # dmapi/ binaries
>  /dmapi/src/common/cmd/read_invis
> diff --git a/src/Makefile b/src/Makefile
> index b8aff49..e6558e2 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -13,7 +13,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
>   multi_open_unlink dmiperf unwritten_sync genhashnames t_holes \
>   t_mmap_writev t_truncate_cmtime dirhash_collide t_rename_overwrite \
>   holetest t_truncate_self t_mmap_dio af_unix t_mmap_stale_pmd \
> - t_mmap_cow_race t_mmap_fallocate fsync-err
> + t_mmap_cow_race t_mmap_fallocate fsync-err t_ext4_dax_journal_corruption
>  
>  LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
>   preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
> diff --git a/src/t_ext4_dax_journal_corruption.c 
> b/src/t_ext4_dax_journal_corruption.c
> new file mode 100644
> index 000..e0d63f8
> --- /dev/null
> +++ b/src/t_ext4_dax_journal_corruption.c
> @@ -0,0 +1,93 @@
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define PAGE(a) ((a)*0x1000)
> +#define STRLEN 256
> +
> +void err_exit(char *op)
> +{
> + fprintf(stderr, "%s: %s\n", op, strerror(errno));
> + exit(1);
> +}
> +
> +void chattr_cmd(char *chattr, char *cmd, char *file)
> +{
> + int ret;
> + char command[STRLEN];
> +
> + ret = snprintf(command, STRLEN, "%s %s %s 2>/dev/null", chattr, cmd, 
> file);
> + if (ret < 0)
> + err_exit("snprintf");
> +
> + ret = system(command);
> + if (ret) /* Success - the kernel fix is to have this chattr fail */
> + exit(77);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + int fd, err, len = PAGE(1);
> + char *data, *dax_data, *chattr, *file;
> + char string[STRLEN];
> +
> + if (argc < 3) {
> + printf("Usage: %s  \n", 
> basename(argv[0]));
> + exit(0);
> + }
> +
> + chattr = argv[1];
> + file = argv[2];
> +
> + srand(time(NULL));
> + snprintf(string, STRLEN, "random number %d\n", rand());
> +
> + fd = open(file, O_RDWR|O_CREAT, S_IRUSR|S_IWUSR);
> + if (fd < 0)
> + err_exit("fd");
> +
> + /* begin with journaling off and DAX on */
> + chattr_cmd(chattr, "-j", file);
> +
> + ftruncate(fd, 0);
> + fallocate(fd, 0, 0, len);
> +
> + dax_data = mmap(NULL, len, PROT_READ, MAP_SHARED, fd, 0);
> + if (!dax_data)
> + err_exit("mmap dax_data");
> +
> + /* turns on journaling, and turns off DAX */
> + chattr_cmd(chattr, "+j", file);

I'm a bit confused here, just from the test code, it's not obvious to me
how DAX is turned off. I looked at the kernel code and there's a comment
saying: "Update inode->i_flags after EXT4_INODE_JOURNAL_DATA was
updated. E.g. S_DAX may get cleared / set." But isn't the per-inode dax
flag proposal rejected?

Anyway, some comments to explain how is DAX being turned off would be
good here.

> +
> + data = mmap(NULL, len, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
> + if (!data)
> + err_exit("mmap data");
> +
> + /*
> +  * Write the data using the non-DAX mapping, and try and read it back
> +  * using the DAX mapping.
> +  */
> + strcpy(data, string);
> + if (strcmp(dax_data, string) != 0)
> + 

[PATCH 2/3] ext4: test for DAX + journaling corruption

[Ross Zwisler]

Add a regression test for the following kernel commit:

  ext4: prevent data corruption with journaling + DAX

The test passes if either we successfully compare the data between the mmap
with journaling turned on and the one with journaling turned off, or if we
fail the chattr command to turn on or off journaling.  The latter is how we
prevent this issue in the kernel.

Signed-off-by: Ross Zwisler 
---
 .gitignore  |  1 +
 src/Makefile|  2 +-
 src/t_ext4_dax_journal_corruption.c | 93 +
 tests/ext4/030  | 68 +++
 tests/ext4/030.out  |  2 +
 tests/ext4/group|  1 +
 6 files changed, 166 insertions(+), 1 deletion(-)
 create mode 100644 src/t_ext4_dax_journal_corruption.c
 create mode 100755 tests/ext4/030
 create mode 100644 tests/ext4/030.out

diff --git a/.gitignore b/.gitignore
index 2accc37..4bdc5bf 100644
--- a/.gitignore
+++ b/.gitignore
@@ -154,6 +154,7 @@
 /src/t_mmap_stale_pmd
 /src/t_mmap_cow_race
 /src/t_mmap_fallocate
+/src/t_ext4_dax_journal_corruption
 
 # dmapi/ binaries
 /dmapi/src/common/cmd/read_invis
diff --git a/src/Makefile b/src/Makefile
index b8aff49..e6558e2 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -13,7 +13,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
multi_open_unlink dmiperf unwritten_sync genhashnames t_holes \
t_mmap_writev t_truncate_cmtime dirhash_collide t_rename_overwrite \
holetest t_truncate_self t_mmap_dio af_unix t_mmap_stale_pmd \
-   t_mmap_cow_race t_mmap_fallocate fsync-err
+   t_mmap_cow_race t_mmap_fallocate fsync-err t_ext4_dax_journal_corruption
 
 LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
diff --git a/src/t_ext4_dax_journal_corruption.c 
b/src/t_ext4_dax_journal_corruption.c
new file mode 100644
index 000..e0d63f8
--- /dev/null
+++ b/src/t_ext4_dax_journal_corruption.c
@@ -0,0 +1,93 @@
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define PAGE(a) ((a)*0x1000)
+#define STRLEN 256
+
+void err_exit(char *op)
+{
+   fprintf(stderr, "%s: %s\n", op, strerror(errno));
+   exit(1);
+}
+
+void chattr_cmd(char *chattr, char *cmd, char *file)
+{
+   int ret;
+   char command[STRLEN];
+
+   ret = snprintf(command, STRLEN, "%s %s %s 2>/dev/null", chattr, cmd, 
file);
+   if (ret < 0)
+   err_exit("snprintf");
+
+   ret = system(command);
+   if (ret) /* Success - the kernel fix is to have this chattr fail */
+   exit(77);
+}
+
+int main(int argc, char *argv[])
+{
+   int fd, err, len = PAGE(1);
+   char *data, *dax_data, *chattr, *file;
+   char string[STRLEN];
+
+   if (argc < 3) {
+   printf("Usage: %s  \n", 
basename(argv[0]));
+   exit(0);
+   }
+
+   chattr = argv[1];
+   file = argv[2];
+
+   srand(time(NULL));
+   snprintf(string, STRLEN, "random number %d\n", rand());
+
+   fd = open(file, O_RDWR|O_CREAT, S_IRUSR|S_IWUSR);
+   if (fd < 0)
+   err_exit("fd");
+
+   /* begin with journaling off and DAX on */
+   chattr_cmd(chattr, "-j", file);
+
+   ftruncate(fd, 0);
+   fallocate(fd, 0, 0, len);
+
+   dax_data = mmap(NULL, len, PROT_READ, MAP_SHARED, fd, 0);
+   if (!dax_data)
+   err_exit("mmap dax_data");
+
+   /* turns on journaling, and turns off DAX */
+   chattr_cmd(chattr, "+j", file);
+
+   data = mmap(NULL, len, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
+   if (!data)
+   err_exit("mmap data");
+
+   /*
+* Write the data using the non-DAX mapping, and try and read it back
+* using the DAX mapping.
+*/
+   strcpy(data, string);
+   if (strcmp(dax_data, string) != 0)
+   printf("Data miscompare\n");
+
+   err = munmap(data, len);
+   if (err < 0)
+   err_exit("munmap data");
+
+   err = munmap(dax_data, len);
+   if (err < 0)
+   err_exit("munmap dax_data");
+
+   err = close(fd);
+   if (err < 0)
+   err_exit("close");
+   return 0;
+}
diff --git a/tests/ext4/030 b/tests/ext4/030
new file mode 100755
index 000..3ac4952
--- /dev/null
+++ b/tests/ext4/030
@@ -0,0 +1,68 @@
+#! /bin/bash
+# FS QA Test ext4/030
+#
+# This is a regression test for kernel patch:
+#   ext4: prevent data corruption with journaling + DAX
+# created by Ross Zwisler 
+#
+#---
+# Copyright (c) 2017 Intel Corporation.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public