Re: [xfstests PATCH v4 0/5] new tests for writeback error reporting behavior

Re: [xfstests PATCH v4 0/5] new tests for writeback error reporting behavior

[Eryu Guan]

On Mon, Jun 12, 2017 at 08:42:08AM -0400, Jeff Layton wrote:
> v4: respin set based on Eryu's comments
> 
> These tests are companion tests to the patchset I recently posted with
> the cover letter:
> 
>     [PATCH v6 00/20] fs: enhanced writeback error reporting with errseq_t (pile #1)
> 
> This set just adds 3 new xfstests to test writeback behavior. One generic
> filesystem test, one test for raw block devices, and one test for btrfs.
> The tests work with dmerror to ensure that writeback fails, and then
> tests how the kernel reports errors afterward.
> 
> xfs, ext2/3/4 and btrfs all pass on a kernel with the patchset above.

xfs, ext3/4 passed generic/999, btrfs passed btrfs/999 (with some local
modifications, see reply to btrfs/999), but ext2 (using ext4 driver) and
btrfs failed generic/999 on my host. (See test log at the end of mail.)

In the ext2 case, this test requires an external log device to run, but
ext2 has no journal at all, I wonder if we should _notrun on ext2.

btrfs doesn't support external log device either, it should not run this
generic test either.

I think _require_logdev() should be updated too, to do a check on
$FSTYP, only allows filesystems that have external log device support to
continue to run.

> 
> The one comment I couldn't really address from earlier review is that
> we don't have a great way for xfstests to tell what sort of error
> reporting behavior it should expect from the running kernel. That
> makes it difficult to tell whether failure is expected during a given
> run.
> 
> Maybe that's OK though and we should just let unconverted filesystems
> fail this test?

If there's really no good way to tell if current fs supports this new
behavior, I think this is fine, strictly speaking, it's not a new
feature anyway.

Thanks,
Eryu

P.S. ext2 and btrfs failure in generic/999 run (I renumbered it to
generic/441)

[root@ibm-x3550m3-05 xfstests]# ./check generic/441 generic/442
FSTYP         -- ext2
PLATFORM      -- Linux/x86_64 ibm-x3550m3-05 4.12.0-rc4.jlayton+
MKFS_OPTIONS  -- /dev/sdc2
MOUNT_OPTIONS -- -o acl,user_xattr -o context=system_u:object_r:root_t:s0 /dev/sdc2 /mnt/testarea/scratch

generic/441 4s ... - output mismatch (see /root/xfstests/results//generic/441.out.bad)
    --- tests/generic/441.out   2017-06-13 15:52:09.928413126 +0800
    +++ /root/xfstests/results//generic/441.out.bad     2017-06-13 16:21:41.414112226 +0800
    @@ -1,3 +1,3 @@
     QA output created by 441
     Format and mount
    -Test passed!
    +Third fsync on fd[0] failed: Input/output error
    ...
    (Run 'diff -u tests/generic/441.out /root/xfstests/results//generic/441.out.bad'  to see the entire diff)

[root@ibm-x3550m3-05 xfstests]# ./check generic/441 generic/442
FSTYP         -- btrfs
PLATFORM      -- Linux/x86_64 ibm-x3550m3-05 4.12.0-rc4.jlayton+
MKFS_OPTIONS  -- /dev/sdc2
MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/sdc2 /mnt/testarea/scratch

generic/441 4s ... - output mismatch (see /root/xfstests/results//generic/441.out.bad)
    --- tests/generic/441.out   2017-06-13 15:52:09.928413126 +0800
    +++ /root/xfstests/results//generic/441.out.bad     2017-06-13 16:25:17.483273992 +0800
    @@ -1,3 +1,3 @@
     QA output created by 441
     Format and mount
    -Test passed!
    +Third fsync on fd[0] failed: Read-only file system
    ...
    (Run 'diff -u tests/generic/441.out /root/xfstests/results//generic/441.out.bad'  to see the entire diff)
> 
> Jeff Layton (5):
>   generic: add a writeback error handling test
>   ext4: allow ext4 to use $SCRATCH_LOGDEV
>   generic: test writeback error handling on dmerror devices
>   ext3: allow it to put journal on a separate device when doing
>     scratch_mkfs
>   btrfs: make a btrfs version of writeback error reporting test
> 
>  .gitignore                 |   1 +
>  common/dmerror             |  13 ++-
>  common/rc                  |  14 ++-
>  doc/auxiliary-programs.txt |  16 ++++
>  src/Makefile               |   2 +-
>  src/dmerror                |  44 +++++++++
>  src/fsync-err.c            | 223 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/999            |  93 +++++++++++++++++++
>  tests/btrfs/group          |   1 +
>  tests/generic/998          |  64 +++++++++++++
>  tests/generic/998.out      |   2 +
>  tests/generic/999          |  77 ++++++++++++++++
>  tests/generic/999.out      |   3 +
>  tests/generic/group        |   2 +
>  14 files changed, 548 insertions(+), 7 deletions(-)
>  create mode 100755 src/dmerror
>  create mode 100644 src/fsync-err.c
>  create mode 100755 tests/btrfs/999
>  create mode 100755 tests/generic/998
>  create mode 100644 tests/generic/998.out
>  create mode 100755 tests/generic/999
>  create mode 100644 tests/generic/999.out
> 
> -- 
> 2.13.0
> 

Re: [xfstests PATCH v4 0/5] new tests for writeback error reporting behavior

[Jeff Layton]

On Tue, 2017-06-13 at 16:32 +0800, Eryu Guan wrote:
> On Mon, Jun 12, 2017 at 08:42:08AM -0400, Jeff Layton wrote:
> > v4: respin set based on Eryu's comments
> > 
> > These tests are companion tests to the patchset I recently posted with
> > the cover letter:
> > 
> >     [PATCH v6 00/20] fs: enhanced writeback error reporting with errseq_t (pile #1)
> > 
> > This set just adds 3 new xfstests to test writeback behavior. One generic
> > filesystem test, one test for raw block devices, and one test for btrfs.
> > The tests work with dmerror to ensure that writeback fails, and then
> > tests how the kernel reports errors afterward.
> > 
> > xfs, ext2/3/4 and btrfs all pass on a kernel with the patchset above.
> 
> xfs, ext3/4 passed generic/999, btrfs passed btrfs/999 (with some local
> modifications, see reply to btrfs/999), but ext2 (using ext4 driver) and
> btrfs failed generic/999 on my host. (See test log at the end of mail.)
> 
> In the ext2 case, this test requires an external log device to run, but
> ext2 has no journal at all, I wonder if we should _notrun on ext2.
> 

Technically it doesn't _require_ an external log device, it just won't
pass on ext3/4 and xfs without one. Not sure how to convey that in a
_require directive here.

If you remove _require_logdev, it should pass on ext2 under the ext4.ko
driver as well.

ext2.ko still has issues though as the block device ends up getting
flagged with errors twice.

> btrfs doesn't support external log device either, it should not run this
> generic test either.
> 

Agreed. We just want it to run btrfs/999

> I think _require_logdev() should be updated too, to do a check on
> $FSTYP, only allows filesystems that have external log device support to
> continue to run.
> 

Ok.

> > 
> > The one comment I couldn't really address from earlier review is that
> > we don't have a great way for xfstests to tell what sort of error
> > reporting behavior it should expect from the running kernel. That
> > makes it difficult to tell whether failure is expected during a given
> > run.
> > 
> > Maybe that's OK though and we should just let unconverted filesystems
> > fail this test?
> 
> If there's really no good way to tell if current fs supports this new
> behavior, I think this is fine, strictly speaking, it's not a new
> feature anyway.
> 
> Thanks,
> Eryu
> 
> P.S. ext2 and btrfs failure in generic/999 run (I renumbered it to
> generic/441)
> 
> [root@ibm-x3550m3-05 xfstests]# ./check generic/441 generic/442
> FSTYP         -- ext2
> PLATFORM      -- Linux/x86_64 ibm-x3550m3-05 4.12.0-rc4.jlayton+
> MKFS_OPTIONS  -- /dev/sdc2
> MOUNT_OPTIONS -- -o acl,user_xattr -o context=system_u:object_r:root_t:s0 /dev/sdc2 /mnt/testarea/scratch
> 
> generic/441 4s ... - output mismatch (see /root/xfstests/results//generic/441.out.bad)
>     --- tests/generic/441.out   2017-06-13 15:52:09.928413126 +0800
>     +++ /root/xfstests/results//generic/441.out.bad     2017-06-13 16:21:41.414112226 +0800
>     @@ -1,3 +1,3 @@
>      QA output created by 441
>      Format and mount
>     -Test passed!
>     +Third fsync on fd[0] failed: Input/output error
>     ...
>     (Run 'diff -u tests/generic/441.out /root/xfstests/results//generic/441.out.bad'  to see the entire diff)
> 

That means that it returned EIO on fsync when the error should have been
cleared. I'll see if I can reproduce it on my setup.

> [root@ibm-x3550m3-05 xfstests]# ./check generic/441 generic/442
> FSTYP         -- btrfs
> PLATFORM      -- Linux/x86_64 ibm-x3550m3-05 4.12.0-rc4.jlayton+
> MKFS_OPTIONS  -- /dev/sdc2
> MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/sdc2 /mnt/testarea/scratch
> 
> generic/441 4s ... - output mismatch (see /root/xfstests/results//generic/441.out.bad)
>     --- tests/generic/441.out   2017-06-13 15:52:09.928413126 +0800
>     +++ /root/xfstests/results//generic/441.out.bad     2017-06-13 16:25:17.483273992 +0800
>     @@ -1,3 +1,3 @@
>      QA output created by 441
>      Format and mount
>     -Test passed!
>     +Third fsync on fd[0] failed: Read-only file system
>     ...
>     (Run 'diff -u tests/generic/441.out /root/xfstests/results//generic/441.out.bad'  to see the entire diff)

That's expected on btrfs. It needs btrfs/999 test to do this correctly.

> > 
> > Jeff Layton (5):
> >   generic: add a writeback error handling test
> >   ext4: allow ext4 to use $SCRATCH_LOGDEV
> >   generic: test writeback error handling on dmerror devices
> >   ext3: allow it to put journal on a separate device when doing
> >     scratch_mkfs
> >   btrfs: make a btrfs version of writeback error reporting test
> > 
> >  .gitignore                 |   1 +
> >  common/dmerror             |  13 ++-
> >  common/rc                  |  14 ++-
> >  doc/auxiliary-programs.txt |  16 ++++
> >  src/Makefile               |   2 +-
> >  src/dmerror                |  44 +++++++++
> >  src/fsync-err.c            | 223 +++++++++++++++++++++++++++++++++++++++++++++
> >  tests/btrfs/999            |  93 +++++++++++++++++++
> >  tests/btrfs/group          |   1 +
> >  tests/generic/998          |  64 +++++++++++++
> >  tests/generic/998.out      |   2 +
> >  tests/generic/999          |  77 ++++++++++++++++
> >  tests/generic/999.out      |   3 +
> >  tests/generic/group        |   2 +
> >  14 files changed, 548 insertions(+), 7 deletions(-)
> >  create mode 100755 src/dmerror
> >  create mode 100644 src/fsync-err.c
> >  create mode 100755 tests/btrfs/999
> >  create mode 100755 tests/generic/998
> >  create mode 100644 tests/generic/998.out
> >  create mode 100755 tests/generic/999
> >  create mode 100644 tests/generic/999.out
> > 
> > -- 
> > 2.13.0
> > 

-- 
Jeff Layton <jlayton@redhat.com>

Re: [xfstests PATCH v4 1/5] generic: add a writeback error handling test

[Eryu Guan]

On Mon, Jun 12, 2017 at 08:42:09AM -0400, Jeff Layton wrote:
> I'm working on a set of kernel patches to change how writeback errors
> are handled and reported in the kernel. Instead of reporting a
> writeback error to only the first fsync caller on the file, I aim
> to make the kernel report them once on every file description.
> 
> This patch adds a test for the new behavior. Basically, open many fds
> to the same file, turn on dm_error, write to each of the fds, and then
> fsync them all to ensure that they all get an error back.
> 
> To do that, I'm adding a new tools/dmerror script that the C program
> can use to load the error table. For now, that's all it can do, but
> we can fill it out with other commands as necessary.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  .gitignore                 |   1 +
>  common/dmerror             |  13 ++-
>  doc/auxiliary-programs.txt |  16 ++++
>  src/Makefile               |   2 +-
>  src/dmerror                |  44 +++++++++
>  src/fsync-err.c            | 223 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/999          |  77 ++++++++++++++++
>  tests/generic/999.out      |   3 +
>  tests/generic/group        |   1 +
>  9 files changed, 374 insertions(+), 6 deletions(-)
>  create mode 100755 src/dmerror
>  create mode 100644 src/fsync-err.c
>  create mode 100755 tests/generic/999
>  create mode 100644 tests/generic/999.out
> 
> diff --git a/.gitignore b/.gitignore
> index 39664b0a7f53..56e863b2c8dc 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -72,6 +72,7 @@
>  /src/fs_perms
>  /src/fssum
>  /src/fstest
> +/src/fsync-err
>  /src/fsync-tester
>  /src/ftrunc
>  /src/genhashnames
> diff --git a/common/dmerror b/common/dmerror
> index d46c5d0b7266..238baa213b1f 100644
> --- a/common/dmerror
> +++ b/common/dmerror
> @@ -23,22 +23,25 @@ if [ $? -eq 0 ]; then
>  	_notrun "Cannot run tests with DAX on dmerror devices"
>  fi
>  
> -_dmerror_init()
> +_dmerror_setup()
>  {
>  	local dm_backing_dev=$SCRATCH_DEV
>  
> -	$DMSETUP_PROG remove error-test > /dev/null 2>&1
> -
>  	local blk_dev_size=`blockdev --getsz $dm_backing_dev`
>  
>  	DMERROR_DEV='/dev/mapper/error-test'
>  
>  	DMLINEAR_TABLE="0 $blk_dev_size linear $dm_backing_dev 0"
>  
> +	DMERROR_TABLE="0 $blk_dev_size error $dm_backing_dev 0"
> +}
> +
> +_dmerror_init()
> +{
> +	_dmerror_setup
> +	$DMSETUP_PROG remove error-test > /dev/null 2>&1
>  	$DMSETUP_PROG create error-test --table "$DMLINEAR_TABLE" || \
>  		_fatal "failed to create dm linear device"
> -
> -	DMERROR_TABLE="0 $blk_dev_size error $dm_backing_dev 0"
>  }
>  
>  _dmerror_mount()
> diff --git a/doc/auxiliary-programs.txt b/doc/auxiliary-programs.txt
> index 21ef118596b6..bcab453c4335 100644
> --- a/doc/auxiliary-programs.txt
> +++ b/doc/auxiliary-programs.txt
> @@ -16,6 +16,8 @@ note the dependency with:
>  Contents:
>  
>   - af_unix		-- Create an AF_UNIX socket
> + - dmerror		-- fault injection block device control
> + - fsync-err		-- tests fsync error reporting after failed writeback
>   - open_by_handle	-- open_by_handle_at syscall exercise
>   - stat_test		-- statx syscall exercise
>   - t_dir_type		-- print directory entries and their file type
> @@ -30,6 +32,20 @@ af_unix
>  
>  	The af_unix program creates an AF_UNIX socket at the given location.
>  
> +dmerror
> +
> +	dmerror is a program for creating, destroying and controlling a
> +	fault injection device. The device can be set up as initially
> +	working and then flip to throwing errors for testing purposes.
> +
> +fsync-err
> +
> +	Specialized program for testing how the kernel reports errors that
> +	occur during writeback. Works in conjunction with the dmerror script
> +	in tools/ to write data to a device, and then force it to fail
> +	writeback and test that errors are reported during fsync and cleared
> +	afterward.
> +
>  open_by_handle
>  
>  	The open_by_handle program exercises the open_by_handle_at() system
> diff --git a/src/Makefile b/src/Makefile
> index 6b0e4b022485..abd0fff34a64 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
> +	t_mmap_cow_race t_mmap_fallocate fsync-err

src/dmerror should be installed on 'make install' too.

-	$(LTINSTALL) -m 755 fill2attr fill2fs fill2fs_check scaleread.sh $(PKG_LIB_DIR)/src
+	$(LTINSTALL) -m 755 fill2attr fill2fs fill2fs_check scaleread.sh dmerror $(PKG_LIB_DIR)/src

>  
>  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/dmerror b/src/dmerror
> new file mode 100755
> index 000000000000..4aaf682ee5f9
> --- /dev/null
> +++ b/src/dmerror
> @@ -0,0 +1,44 @@
> +#!/bin/bash
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2017, Jeff Layton <jlayton@redhat.com>
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +
> +. ./common/config
> +. ./common/dmerror
> +
> +_dmerror_setup
> +
> +case $1 in
> +cleanup)
> +	_dmerror_cleanup
> +	;;
> +init)
> +	_dmerror_init
> +	;;
> +load_error_table)
> +	_dmerror_load_error_table
> +	;;
> +load_working_table)
> +	_dmerror_load_working_table
> +	;;
> +*)
> +	echo "Usage: $0 {init|cleanup|load_error_table|load_working_table}"
> +	exit 1
> +	;;
> +esac
> +
> +status=0
> +exit
> diff --git a/src/fsync-err.c b/src/fsync-err.c
> new file mode 100644
> index 000000000000..5b3bdd3ada07
> --- /dev/null
> +++ b/src/fsync-err.c
> @@ -0,0 +1,223 @@
> +/*
> + * fsync-err.c: test whether writeback errors are reported to all open fds
> + * 		and properly cleared as expected after being seen once on each
> + *
> + * Copyright (c) 2017: Jeff Layton <jlayton@redhat.com>
> + */
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <getopt.h>
> +
> +/*
> + * btrfs has a fixed stripewidth of 64k, so we need to write enough data to
> + * ensure that we hit both stripes by default.
> + */
> +#define DEFAULT_BUFSIZE (65 * 1024)
> +
> +/* default number of fds to open */
> +#define DEFAULT_NUM_FDS	10
> +
> +static void usage()
> +{
> +	printf("Usage: fsync-err [ -b bufsize ] [ -n num_fds ] -d dmerror path <filename>\n");
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	int *fd, ret, i, numfds = DEFAULT_NUM_FDS;
> +	char *fname, *buf;
> +	char *dmerror_path = NULL;
> +	char *cmdbuf;
> +	size_t cmdsize, bufsize = DEFAULT_BUFSIZE;
> +
> +	while ((i = getopt(argc, argv, "b:d:n:")) != -1) {
> +		switch (i) {
> +		case 'b':
> +			bufsize = strtol(optarg, &buf, 0);
> +			if (*buf != '\0') {
> +				printf("bad string conversion: %s\n", optarg);
> +				return 1;
> +			}
> +			break;
> +		case 'd':
> +			dmerror_path = optarg;
> +			break;
> +		case 'n':
> +			numfds = strtol(optarg, &buf, 0);
> +			if (*buf != '\0') {
> +				printf("bad string conversion: %s\n", optarg);
> +				return 1;
> +			}
> +			break;
> +		}
> +	}
> +
> +	if (argc < 1) {
> +		usage();
> +		return 1;
> +	}
> +
> +	if (!dmerror_path) {
> +		printf("Must specify dmerror path with -d option!\n");
> +		return 1;
> +	}
> +
> +	/* Remaining argument is filename */
> +	fname = argv[optind];
> +
> +	fd = calloc(numfds, sizeof(*fd));
> +	if (!fd) {
> +		printf("malloc failed: %m\n");
> +		return 1;
> +	}
> +
> +	for (i = 0; i < numfds; ++i) {
> +		fd[i] = open(fname, O_WRONLY | O_CREAT | O_TRUNC, 0644);
> +		if (fd[i] < 0) {
> +			printf("open of fd[%d] failed: %m\n", i);
> +			return 1;
> +		}
> +	}
> +
> +	buf = malloc(bufsize);
> +	if (!buf) {
> +		printf("malloc failed: %m\n");
> +		return 1;
> +	}
> +
> +	/* fill it with some junk */
> +	memset(buf, 0x7c, bufsize);
> +
> +	for (i = 0; i < numfds; ++i) {
> +		ret = write(fd[i], buf, bufsize);
> +		if (ret < 0) {
> +			printf("First write on fd[%d] failed: %m\n", i);
> +			return 1;
> +		}
> +	}
> +
> +	for (i = 0; i < numfds; ++i) {
> +		ret = fsync(fd[i]);
> +		if (ret < 0) {
> +			printf("First fsync on fd[%d] failed: %m\n", i);
> +			return 1;
> +		}
> +	}
> +
> +	/* enough for path + dmerror command string  (and then some) */
> +	cmdsize = strlen(dmerror_path) + 64;
> +
> +	cmdbuf = malloc(cmdsize);
> +	if (!cmdbuf) {
> +		printf("malloc failed: %m\n");
> +		return 1;
> +	}
> +
> +	ret = snprintf(cmdbuf, cmdsize, "%s load_error_table", dmerror_path);
> +	if (ret < 0 || ret >= cmdsize) {
> +		printf("sprintf failure: %d\n", ret);
> +		return 1;
> +	}
> +
> +	/* flip the device to non-working mode */
> +	ret = system(cmdbuf);
> +	if (ret) {
> +		if (WIFEXITED(ret))
> +			printf("system: program exited: %d\n",
> +					WEXITSTATUS(ret));
> +		else
> +			printf("system: 0x%x\n", (int)ret);
> +
> +		return 1;
> +	}
> +
> +	for (i = 0; i < numfds; ++i) {
> +		ret = write(fd[i], buf, bufsize);
> +		if (ret < 0) {
> +			printf("Second write on fd[%d] failed: %m\n", i);
> +			return 1;
> +		}
> +	}
> +
> +	for (i = 0; i < numfds; ++i) {
> +		ret = fsync(fd[i]);
> +		/* Now, we EXPECT the error! */
> +		if (ret >= 0) {
> +			printf("Success on second fsync on fd[%d]!\n", i);
> +			return 1;
> +		}
> +	}
> +
> +	for (i = 0; i < numfds; ++i) {
> +		ret = fsync(fd[i]);
> +		if (ret < 0) {
> +			/*
> +			 * We did a failed write and fsync on each fd before.
> +			 * Now the error should be clear since we've not done
> +			 * any writes since then.
> +			 */
> +			printf("Third fsync on fd[%d] failed: %m\n", i);
> +			return 1;
> +		}
> +	}
> +
> +	/* flip the device to working mode */
> +	ret = snprintf(cmdbuf, cmdsize, "%s load_working_table", dmerror_path);
> +	if (ret < 0 || ret >= cmdsize) {
> +		printf("sprintf failure: %d\n", ret);
> +		return 1;
> +	}
> +
> +	ret = system(cmdbuf);
> +	if (ret) {
> +		if (WIFEXITED(ret))
> +			printf("system: program exited: %d\n",
> +					WEXITSTATUS(ret));
> +		else
> +			printf("system: 0x%x\n", (int)ret);
> +
> +		return 1;
> +	}
> +
> +	for (i = 0; i < numfds; ++i) {
> +		ret = fsync(fd[i]);
> +		if (ret < 0) {
> +			/* The error should still be clear */
> +			printf("fsync after healing device on fd[%d] failed: %m\n", i);
> +			return 1;
> +		}
> +	}
> +
> +	/*
> +	 * reopen each file one at a time to ensure the same inode stays
> +	 * in core. fsync each one to make sure we see no errors on a fresh
> +	 * open of the inode.
> +	 */
> +	for (i = 0; i < numfds; ++i) {
> +		ret = close(fd[i]);
> +		if (ret < 0) {
> +			printf("Close of fd[%d] returned unexpected error: %m\n", i);
> +			return 1;
> +		}
> +		fd[i] = open(fname, O_WRONLY | O_CREAT | O_TRUNC, 0644);
> +		if (fd[i] < 0) {
> +			printf("Second open of fd[%d] failed: %m\n", i);
> +			return 1;
> +		}
> +		ret = fsync(fd[i]);
> +		if (ret < 0) {
> +			/* New opens should not return an error */
> +			printf("First fsync after reopen of fd[%d] failed: %m\n", i);
> +			return 1;
> +		}
> +	}
> +
> +	printf("Test passed!\n");
> +	return 0;
> +}
> diff --git a/tests/generic/999 b/tests/generic/999
> new file mode 100755
> index 000000000000..f998671ef19e
> --- /dev/null
> +++ b/tests/generic/999
> @@ -0,0 +1,77 @@
> +#! /bin/bash
> +# FS QA Test No. 999
> +#
> +# Open a file several times, write to it, fsync on all fds and make sure that
> +# they all return 0. Change the device to start throwing errors. Write again
> +# on all fds and fsync on all fds. Ensure that we get errors on all of them.
> +# Then fsync on all one last time and verify that all return 0.
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2017, Jeff Layton <jlayton@redhat.com>
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1    # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +    cd /
> +    rm -rf $tmp.* $testdir
> +    _dmerror_cleanup

Minor nits, please use tab for indention.

Thanks,
Eryu

> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +. ./common/dmerror
> +
> +# real QA test starts here
> +_supported_os Linux
> +_require_scratch
> +_require_logdev
> +_require_dm_target error
> +_require_test_program fsync-err
> +_require_test_program dmerror
> +
> +rm -f $seqres.full
> +
> +echo "Format and mount"
> +_scratch_mkfs > $seqres.full 2>&1
> +_dmerror_init
> +_dmerror_mount
> +
> +_require_fs_space $SCRATCH_MNT 65536
> +
> +testfile=$SCRATCH_MNT/fsync-err-test
> +
> +$here/src/fsync-err -d $here/src/dmerror $testfile
> +
> +# success, all done
> +_dmerror_load_working_table
> +_dmerror_unmount
> +_dmerror_cleanup
> +
> +# fs may be corrupt after this -- attempt to repair it
> +_repair_scratch_fs >> $seqres.full
> +
> +status=0
> +exit
> diff --git a/tests/generic/999.out b/tests/generic/999.out
> new file mode 100644
> index 000000000000..2e48492ff6d1
> --- /dev/null
> +++ b/tests/generic/999.out
> @@ -0,0 +1,3 @@
> +QA output created by 999
> +Format and mount
> +Test passed!
> diff --git a/tests/generic/group b/tests/generic/group
> index 5d3e4dcf732e..b56bae8f04f0 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -442,3 +442,4 @@
>  437 auto quick
>  438 auto
>  439 auto quick punch
> +999 auto quick
> -- 
> 2.13.0
> 

Re: [xfstests PATCH v4 2/5] ext4: allow ext4 to use $SCRATCH_LOGDEV

[Eryu Guan]

On Mon, Jun 12, 2017 at 08:42:10AM -0400, Jeff Layton wrote:
> The writeback error handling test requires that you put the journal on a
> separate device. This allows us to use dmerror to simulate data
> writeback failure, without affecting the journal.
> 
> xfs already has infrastructure for this (a'la $SCRATCH_LOGDEV), so wire
> up the ext4 code so that it can do the same thing when _scratch_mkfs is
> called.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  common/rc | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index 87e6ff08b18d..08807ac7c22a 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -676,6 +676,9 @@ _scratch_mkfs_ext4()
>  	local tmp=`mktemp`
>  	local mkfs_status
>  
> +	[ "$USE_EXTERNAL" = yes -a ! -z "$SCRATCH_LOGDEV" ] && \
> +	    $mkfs_cmd -O journal_dev $SCRATCH_LOGDEV && \
> +	    mkfs_cmd="$mkfs_cmd $MKFS_OPTIONS -J device=$SCRATCH_LOGDEV"

This $MKFS_OPTIONS should be added to the first command when creating
the journal device so that journal dev has the same block size as data
dev, there's no need to update mkfs_cmd string.

The external log dev support for ext3 patch has similar issue.

Thanks,
Eryu

>  
>  	_scratch_do_mkfs "$mkfs_cmd" "$mkfs_filter" $* 2>$tmp.mkfserr 1>$tmp.mkfsstd
>  	mkfs_status=$?
> -- 
> 2.13.0
> 

Re: [xfstests PATCH v4 5/5] btrfs: make a btrfs version of writeback error reporting test

[Eryu Guan]

On Mon, Jun 12, 2017 at 08:42:13AM -0400, Jeff Layton wrote:
> Make a new btrfs/999 test that works the way Chris Mason suggested:
> 
> Build a filesystem with 2 devices that stripes the data across
> both devices, but mirrors metadata across both. Then, make one
> of the devices fail and see how fsync is handled.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  tests/btrfs/999   | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++

Missing btrfs/999.out file

>  tests/btrfs/group |  1 +
>  2 files changed, 94 insertions(+)
>  create mode 100755 tests/btrfs/999
> 
> diff --git a/tests/btrfs/999 b/tests/btrfs/999
> new file mode 100755
> index 000000000000..84031cc0d913
> --- /dev/null
> +++ b/tests/btrfs/999
> @@ -0,0 +1,93 @@
> +#! /bin/bash
> +# FS QA Test No. 999
> +#
> +# Open a file several times, write to it, fsync on all fds and make sure that
> +# they all return 0. Change the device to start throwing errors. Write again
> +# on all fds and fsync on all fds. Ensure that we get errors on all of them.
> +# Then fsync on all one last time and verify that all return 0.
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2017, Jeff Layton <jlayton@redhat.com>
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1    # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +    cd /
> +    rm -rf $tmp.* $testdir
> +    _dmerror_cleanup
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +. ./common/dmerror
> +
> +# real QA test starts here
> +_supported_os Linux
> +_require_dm_target error
> +_require_test_program fsync-err
> +_require_test_program dmerror
> +
> +# bring up dmerror device
> +_scratch_unmount
> +_dmerror_init
> +
> +# Replace first device with error-test device
> +old_SCRATCH_DEV=$SCRATCH_DEV
> +SCRATCH_DEV_POOL=`echo $SCRATCH_DEV_POOL | perl -pe "s#$SCRATCH_DEV#$DMERROR_DEV#"`
> +SCRATCH_DEV=$DMERROR_DEV
> +
> +_require_scratch
> +_require_scratch_dev_pool

Need "_require_scratch_dev_pool_equal_size" too, since test creates
raid1 profile for metadata.

Thanks,
Eryu

> +
> +rm -f $seqres.full
> +
> +echo "Format and mount"
> +
> +_scratch_pool_mkfs "-d raid0 -m raid1" > $seqres.full 2>&1
> +_scratch_mount
> +
> +# How much do we need to write? We need to hit all of the stripes. btrfs uses
> +# a fixed 64k stripesize, so write enough to hit each one
> +number_of_devices=`echo $SCRATCH_DEV_POOL | wc -w`
> +write_kb=$(($number_of_devices * 64))
> +_require_fs_space $SCRATCH_MNT $write_kb
> +
> +testfile=$SCRATCH_MNT/fsync-err-test
> +
> +SCRATCH_DEV=$old_SCRATCH_DEV
> +$here/src/fsync-err -b $(($write_kb * 1024)) -d $here/src/dmerror $testfile
> +
> +# success, all done
> +_dmerror_load_working_table
> +
> +# fs may be corrupt after this -- attempt to repair it
> +_repair_scratch_fs >> $seqres.full
> +
> +# remove dmerror device
> +_dmerror_cleanup
> +
> +status=0
> +exit
> diff --git a/tests/btrfs/group b/tests/btrfs/group
> index 6f19619e877c..8dbdfbfe29fd 100644
> --- a/tests/btrfs/group
> +++ b/tests/btrfs/group
> @@ -145,3 +145,4 @@
>  141 auto quick
>  142 auto quick
>  143 auto quick
> +999 auto quick
> -- 
> 2.13.0
> 

Re: [xfstests PATCH v4 5/5] btrfs: make a btrfs version of writeback error reporting test

[Jeff Layton]

On Tue, 2017-06-13 at 16:40 +0800, Eryu Guan wrote:
> On Mon, Jun 12, 2017 at 08:42:13AM -0400, Jeff Layton wrote:
> > Make a new btrfs/999 test that works the way Chris Mason suggested:
> > 
> > Build a filesystem with 2 devices that stripes the data across
> > both devices, but mirrors metadata across both. Then, make one
> > of the devices fail and see how fsync is handled.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  tests/btrfs/999   | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 
> Missing btrfs/999.out file
> 
> >  tests/btrfs/group |  1 +
> >  2 files changed, 94 insertions(+)
> >  create mode 100755 tests/btrfs/999
> > 
> > diff --git a/tests/btrfs/999 b/tests/btrfs/999
> > new file mode 100755
> > index 000000000000..84031cc0d913
> > --- /dev/null
> > +++ b/tests/btrfs/999
> > @@ -0,0 +1,93 @@
> > +#! /bin/bash
> > +# FS QA Test No. 999
> > +#
> > +# Open a file several times, write to it, fsync on all fds and make sure that
> > +# they all return 0. Change the device to start throwing errors. Write again
> > +# on all fds and fsync on all fds. Ensure that we get errors on all of them.
> > +# Then fsync on all one last time and verify that all return 0.
> > +#
> > +#-----------------------------------------------------------------------
> > +# Copyright (c) 2017, Jeff Layton <jlayton@redhat.com>
> > +#
> > +# This program is free software; you can redistribute it and/or
> > +# modify it under the terms of the GNU General Public License as
> > +# published by the Free Software Foundation.
> > +#
> > +# This program is distributed in the hope that it would be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program; if not, write the Free Software Foundation,
> > +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> > +#-----------------------------------------------------------------------
> > +
> > +seq=`basename $0`
> > +seqres=$RESULT_DIR/$seq
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> > +tmp=/tmp/$$
> > +status=1    # failure is the default!
> > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > +
> > +_cleanup()
> > +{
> > +    cd /
> > +    rm -rf $tmp.* $testdir
> > +    _dmerror_cleanup
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +. ./common/dmerror
> > +
> > +# real QA test starts here
> > +_supported_os Linux
> > +_require_dm_target error
> > +_require_test_program fsync-err
> > +_require_test_program dmerror
> > +
> > +# bring up dmerror device
> > +_scratch_unmount
> > +_dmerror_init
> > +
> > +# Replace first device with error-test device
> > +old_SCRATCH_DEV=$SCRATCH_DEV
> > +SCRATCH_DEV_POOL=`echo $SCRATCH_DEV_POOL | perl -pe "s#$SCRATCH_DEV#$DMERROR_DEV#"`
> > +SCRATCH_DEV=$DMERROR_DEV
> > +
> > +_require_scratch
> > +_require_scratch_dev_pool
> 
> Need "_require_scratch_dev_pool_equal_size" too, since test creates
> raid1 profile for metadata.
> 
> Thanks,
> Eryu
> 

Is this really needed?

I've been running this test on btrfs with devices that are not of equal
size, and it seems to work just fine. The test doesn't write a lot of
data (just a few megs at most), so I don't think we'll run out of space
unless you have some really small devices in there.

> > +
> > +rm -f $seqres.full
> > +
> > +echo "Format and mount"
> > +
> > +_scratch_pool_mkfs "-d raid0 -m raid1" > $seqres.full 2>&1
> > +_scratch_mount
> > +
> > +# How much do we need to write? We need to hit all of the stripes. btrfs uses
> > +# a fixed 64k stripesize, so write enough to hit each one
> > +number_of_devices=`echo $SCRATCH_DEV_POOL | wc -w`
> > +write_kb=$(($number_of_devices * 64))
> > +_require_fs_space $SCRATCH_MNT $write_kb
> > +
> > +testfile=$SCRATCH_MNT/fsync-err-test
> > +
> > +SCRATCH_DEV=$old_SCRATCH_DEV
> > +$here/src/fsync-err -b $(($write_kb * 1024)) -d $here/src/dmerror $testfile
> > +
> > +# success, all done
> > +_dmerror_load_working_table
> > +
> > +# fs may be corrupt after this -- attempt to repair it
> > +_repair_scratch_fs >> $seqres.full
> > +
> > +# remove dmerror device
> > +_dmerror_cleanup
> > +
> > +status=0
> > +exit
> > diff --git a/tests/btrfs/group b/tests/btrfs/group
> > index 6f19619e877c..8dbdfbfe29fd 100644
> > --- a/tests/btrfs/group
> > +++ b/tests/btrfs/group
> > @@ -145,3 +145,4 @@
> >  141 auto quick
> >  142 auto quick
> >  143 auto quick
> > +999 auto quick
> > -- 
> > 2.13.0
> > 

-- 
Jeff Layton <jlayton@redhat.com>

Re: [xfstests PATCH v4 5/5] btrfs: make a btrfs version of writeback error reporting test

[Eryu Guan]

On Wed, Jun 14, 2017 at 07:55:17AM -0400, Jeff Layton wrote:
> On Tue, 2017-06-13 at 16:40 +0800, Eryu Guan wrote:
> > On Mon, Jun 12, 2017 at 08:42:13AM -0400, Jeff Layton wrote:
> > > Make a new btrfs/999 test that works the way Chris Mason suggested:
> > > 
> > > Build a filesystem with 2 devices that stripes the data across
> > > both devices, but mirrors metadata across both. Then, make one
> > > of the devices fail and see how fsync is handled.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > ---
> > >  tests/btrfs/999   | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 
> > Missing btrfs/999.out file
> > 
> > >  tests/btrfs/group |  1 +
> > >  2 files changed, 94 insertions(+)
> > >  create mode 100755 tests/btrfs/999
> > > 
> > > diff --git a/tests/btrfs/999 b/tests/btrfs/999
> > > new file mode 100755
> > > index 000000000000..84031cc0d913
> > > --- /dev/null
> > > +++ b/tests/btrfs/999
> > > @@ -0,0 +1,93 @@
> > > +#! /bin/bash
> > > +# FS QA Test No. 999
> > > +#
> > > +# Open a file several times, write to it, fsync on all fds and make sure that
> > > +# they all return 0. Change the device to start throwing errors. Write again
> > > +# on all fds and fsync on all fds. Ensure that we get errors on all of them.
> > > +# Then fsync on all one last time and verify that all return 0.
> > > +#
> > > +#-----------------------------------------------------------------------
> > > +# Copyright (c) 2017, Jeff Layton <jlayton@redhat.com>
> > > +#
> > > +# This program is free software; you can redistribute it and/or
> > > +# modify it under the terms of the GNU General Public License as
> > > +# published by the Free Software Foundation.
> > > +#
> > > +# This program is distributed in the hope that it would be useful,
> > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > +# GNU General Public License for more details.
> > > +#
> > > +# You should have received a copy of the GNU General Public License
> > > +# along with this program; if not, write the Free Software Foundation,
> > > +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> > > +#-----------------------------------------------------------------------
> > > +
> > > +seq=`basename $0`
> > > +seqres=$RESULT_DIR/$seq
> > > +echo "QA output created by $seq"
> > > +
> > > +here=`pwd`
> > > +tmp=/tmp/$$
> > > +status=1    # failure is the default!
> > > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > > +
> > > +_cleanup()
> > > +{
> > > +    cd /
> > > +    rm -rf $tmp.* $testdir
> > > +    _dmerror_cleanup
> > > +}
> > > +
> > > +# get standard environment, filters and checks
> > > +. ./common/rc
> > > +. ./common/filter
> > > +. ./common/dmerror
> > > +
> > > +# real QA test starts here
> > > +_supported_os Linux
> > > +_require_dm_target error
> > > +_require_test_program fsync-err
> > > +_require_test_program dmerror
> > > +
> > > +# bring up dmerror device
> > > +_scratch_unmount
> > > +_dmerror_init
> > > +
> > > +# Replace first device with error-test device
> > > +old_SCRATCH_DEV=$SCRATCH_DEV
> > > +SCRATCH_DEV_POOL=`echo $SCRATCH_DEV_POOL | perl -pe "s#$SCRATCH_DEV#$DMERROR_DEV#"`
> > > +SCRATCH_DEV=$DMERROR_DEV
> > > +
> > > +_require_scratch
> > > +_require_scratch_dev_pool
> > 
> > Need "_require_scratch_dev_pool_equal_size" too, since test creates
> > raid1 profile for metadata.

Sorry, it's not needed here. I got confused with device replace
operation, only "replace" needs this require rule. Thanks for
confirming!

Eryu