[PATCHSET 0/2] fstests: enhance fail_make_request

[PATCHSET 0/2] fstests: enhance fail_make_request

[Darrick J. Wong]

Hi all,

This series starts by refactoring boilerplate code around
fail_make_request (aka error injection in the block layer) and then
enhances it to play nicely with multi-device XFS filesystems.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=fix-fail-make-request
---
 common/fail_make_request |   63 ++++++++++++++++++++++++++++++++++++++++++++++
 common/rc                |    7 -----
 tests/btrfs/088          |   14 ++++------
 tests/btrfs/150          |   13 ++++-----
 tests/generic/019        |   40 ++++-------------------------
 5 files changed, 78 insertions(+), 59 deletions(-)
 create mode 100644 common/fail_make_request

[PATCH 1/2] common: refactor fail_make_request boilerplate

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Refactor the control functions from generic/019 into a common helper to
be used by all three tests that use fail_make_requests.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/fail_make_request |   47 ++++++++++++++++++++++++++++++++++++++++++++++
 common/rc                |    7 -------
 tests/btrfs/088          |   14 +++++---------
 tests/btrfs/150          |   13 +++++--------
 tests/generic/019        |   40 +++++----------------------------------
 5 files changed, 62 insertions(+), 59 deletions(-)
 create mode 100644 common/fail_make_request


diff --git a/common/fail_make_request b/common/fail_make_request
new file mode 100644
index 00000000..581d176a
--- /dev/null
+++ b/common/fail_make_request
@@ -0,0 +1,47 @@
+##/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# common functions for setting up and tearing down block device error injection
+
+_require_fail_make_request()
+{
+    [ -f "$DEBUGFS_MNT/fail_make_request/probability" ] \
+	|| _notrun "$DEBUGFS_MNT/fail_make_request \
+ not found. Seems that CONFIG_FAULT_INJECTION_DEBUG_FS kernel config option not enabled"
+}
+
+_allow_fail_make_request()
+{
+    local prob="${1:-100}"
+    local times="${2:-9999999}"
+    local verbose="${3:-0}"
+
+    echo "Allow global fail_make_request feature"
+    echo "$prob" > $DEBUGFS_MNT/fail_make_request/probability
+    echo "$times" > $DEBUGFS_MNT/fail_make_request/times
+    echo "$verbose" > $DEBUGFS_MNT/fail_make_request/verbose
+}
+
+_disallow_fail_make_request()
+{
+    echo "Disallow global fail_make_request feature"
+    echo 0 > $DEBUGFS_MNT/fail_make_request/probability
+    echo 0 > $DEBUGFS_MNT/fail_make_request/times
+    echo 0 > $DEBUGFS_MNT/fail_make_request/verbose
+}
+
+_start_fail_scratch_dev()
+{
+    local SYSFS_BDEV=`_sysfs_dev $SCRATCH_DEV`
+    echo "Force SCRATCH_DEV device failure"
+    echo " echo 1 > $SYSFS_BDEV/make-it-fail" >> $seqres.full
+    echo 1 > $SYSFS_BDEV/make-it-fail
+}
+
+_stop_fail_scratch_dev()
+{
+    local SYSFS_BDEV=`_sysfs_dev $SCRATCH_DEV`
+    echo "Make SCRATCH_DEV device operable again"
+    echo " echo 0 > $SYSFS_BDEV/make-it-fail" >> $seqres.full
+    echo 0 > $SYSFS_BDEV/make-it-fail
+}
diff --git a/common/rc b/common/rc
index 197c9415..09c81be6 100644
--- a/common/rc
+++ b/common/rc
@@ -2887,13 +2887,6 @@ _require_debugfs()
     [ -d "$DEBUGFS_MNT/boot_params" ] || _notrun "Debugfs not mounted"
 }
 
-_require_fail_make_request()
-{
-    [ -f "$DEBUGFS_MNT/fail_make_request/probability" ] \
-	|| _notrun "$DEBUGFS_MNT/fail_make_request \
- not found. Seems that CONFIG_FAULT_INJECTION_DEBUG_FS kernel config option not enabled"
-}
-
 # Disable extent zeroing for ext4 on the given device
 _ext4_disable_extent_zeroout()
 {
diff --git a/tests/btrfs/088 b/tests/btrfs/088
index d9c5528b..59972ae7 100755
--- a/tests/btrfs/088
+++ b/tests/btrfs/088
@@ -18,27 +18,23 @@ _begin_fstest auto quick metadata
 
 # Import common functions.
 . ./common/filter
+. ./common/fail_make_request
 
 # real QA test starts here
 _supported_fs btrfs
 _require_scratch
 _require_fail_make_request
 
-SYSFS_BDEV=`_sysfs_dev $SCRATCH_DEV`
-
 enable_io_failure()
 {
-	echo 100 > $DEBUGFS_MNT/fail_make_request/probability
-	echo 1000 > $DEBUGFS_MNT/fail_make_request/times
-	echo 0 > $DEBUGFS_MNT/fail_make_request/verbose
-	echo 1 > $SYSFS_BDEV/make-it-fail
+	_allow_fail_make_request 100 1000 > /dev/null
+	_start_fail_scratch_dev > /dev/null
 }
 
 disable_io_failure()
 {
-	echo 0 > $SYSFS_BDEV/make-it-fail
-	echo 0 > $DEBUGFS_MNT/fail_make_request/probability
-	echo 0 > $DEBUGFS_MNT/fail_make_request/times
+	_stop_fail_scratch_dev > /dev/null
+	_disallow_fail_make_request > /dev/null
 }
 
 # We will abort a btrfs transaction later, which always produces a warning in
diff --git a/tests/btrfs/150 b/tests/btrfs/150
index c5e9c709..a7d7d9cc 100755
--- a/tests/btrfs/150
+++ b/tests/btrfs/150
@@ -15,6 +15,7 @@ _begin_fstest auto quick dangerous read_repair
 
 # Import common functions.
 . ./common/filter
+. ./common/fail_make_request
 
 # real QA test starts here
 
@@ -25,22 +26,18 @@ _require_fail_make_request
 _require_scratch_dev_pool 2
 _scratch_dev_pool_get 2
 
-SYSFS_BDEV=`_sysfs_dev $SCRATCH_DEV`
 enable_io_failure()
 {
-	echo 100 > $DEBUGFS_MNT/fail_make_request/probability
-	echo 1000 > $DEBUGFS_MNT/fail_make_request/times
-	echo 0 > $DEBUGFS_MNT/fail_make_request/verbose
+	_allow_fail_make_request 100 1000 > /dev/null
 	echo 1 > $DEBUGFS_MNT/fail_make_request/task-filter
-	echo 1 > $SYSFS_BDEV/make-it-fail
+	_start_fail_scratch_dev > /dev/null
 }
 
 disable_io_failure()
 {
-	echo 0 > $DEBUGFS_MNT/fail_make_request/probability
-	echo 0 > $DEBUGFS_MNT/fail_make_request/times
+	_disallow_fail_make_request > /dev/null
 	echo 0 > $DEBUGFS_MNT/fail_make_request/task-filter
-	echo 0 > $SYSFS_BDEV/make-it-fail
+	_stop_fail_scratch_dev > /dev/null
 }
 
 _check_minimal_fs_size $(( 1024 * 1024 * 1024 ))
diff --git a/tests/generic/019 b/tests/generic/019
index 45c91624..b68dd90c 100755
--- a/tests/generic/019
+++ b/tests/generic/019
@@ -14,47 +14,17 @@ fio_config=$tmp.fio
 
 # Import common functions.
 . ./common/filter
+. ./common/fail_make_request
 _supported_fs generic
 _require_scratch
 _require_block_device $SCRATCH_DEV
 _require_fail_make_request
 
-SYSFS_BDEV=`_sysfs_dev $SCRATCH_DEV`
-
-allow_fail_make_request()
-{
-    echo "Allow global fail_make_request feature"
-    echo 100 > $DEBUGFS_MNT/fail_make_request/probability
-    echo 9999999 > $DEBUGFS_MNT/fail_make_request/times
-    echo 0 >  /sys/kernel/debug/fail_make_request/verbose
-}
-
-disallow_fail_make_request()
-{
-    echo "Disallow global fail_make_request feature"
-    echo 0 > $DEBUGFS_MNT/fail_make_request/probability
-    echo 0 > $DEBUGFS_MNT/fail_make_request/times
-}
-
-start_fail_scratch_dev()
-{
-    echo "Force SCRATCH_DEV device failure"
-    echo " echo 1 > $SYSFS_BDEV/make-it-fail" >> $seqres.full
-    echo 1 > $SYSFS_BDEV/make-it-fail
-}
-
-stop_fail_scratch_dev()
-{
-    echo "Make SCRATCH_DEV device operable again"
-    echo " echo 0 > $SYSFS_BDEV/make-it-fail" >> $seqres.full
-    echo 0 > $SYSFS_BDEV/make-it-fail
-}
-
 # Override the default cleanup function.
 _cleanup()
 {
 	kill $fs_pid $fio_pid &> /dev/null
-	disallow_fail_make_request
+	_disallow_fail_make_request
 	cd /
 	rm -r -f $tmp.*
 }
@@ -129,7 +99,7 @@ _workout()
 
 	# Let's it work for awhile, and force device failure
 	sleep $RUN_TIME
-	start_fail_scratch_dev
+	_start_fail_scratch_dev
 	# After device turns in to failed state filesystem may yet not know about
 	# that so buffered write(2) may succeed, but any integrity operations
 	# such as (sync, fsync, fdatasync, direct-io) should fail.
@@ -147,7 +117,7 @@ _workout()
 	run_check _scratch_unmount
 	# Once filesystem was umounted no one is able to write to block device
 	# It is now safe to bring device back to normal state
-	stop_fail_scratch_dev
+	_stop_fail_scratch_dev
 
 	# In order to check that filesystem is able to recover journal on mount(2)
 	# perform mount/umount, after that all errors should be fixed
@@ -159,7 +129,7 @@ _workout()
 
 _scratch_mkfs >> $seqres.full 2>&1 || _fail "mkfs failed"
 _scratch_mount
-allow_fail_make_request
+_allow_fail_make_request
 _workout
 status=$?
 exit

Re: [PATCH 1/2] common: refactor fail_make_request boilerplate

[Christoph Hellwig]

On Tue, Jul 26, 2022 at 12:49:00PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Refactor the control functions from generic/019 into a common helper to
> be used by all three tests that use fail_make_requests.

There is btrfs/271 as well with similar boilerplate as well, which
is why I've been looking into refactoring it before.

> +_start_fail_scratch_dev()
> +{
> +    local SYSFS_BDEV=`_sysfs_dev $SCRATCH_DEV`
> +    echo "Force SCRATCH_DEV device failure"
> +    echo " echo 1 > $SYSFS_BDEV/make-it-fail" >> $seqres.full
> +    echo 1 > $SYSFS_BDEV/make-it-fail
> +}

.. and for that use cases we need to pass an explicit device here, and
not just default to $SCRATCH_DEV.  

>  enable_io_failure()
>  {
> +	_allow_fail_make_request 100 1000 > /dev/null
> +	_start_fail_scratch_dev > /dev/null
>  }
>  
>  disable_io_failure()
>  {
> +	_stop_fail_scratch_dev > /dev/null
> +	_disallow_fail_make_request > /dev/null
>  }

Let's just drop these wrappers.

Otherwise this looks really nice.

[PATCH 2/2] fail_make_request: teach helpers about external devices

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Teach the fail_make_request helpers about external log and realtime
devices so that we can use generic/019 on exotic XFS configurations.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/fail_make_request |   28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)


diff --git a/common/fail_make_request b/common/fail_make_request
index 581d176a..9f8ea500 100644
--- a/common/fail_make_request
+++ b/common/fail_make_request
@@ -30,18 +30,34 @@ _disallow_fail_make_request()
     echo 0 > $DEBUGFS_MNT/fail_make_request/verbose
 }
 
+_bdev_fail_make_request()
+{
+    local bdev="$1"
+    local status="$2"
+    local sysfs_bdev=$(_sysfs_dev $bdev)
+
+    echo " echo $status > $sysfs_bdev/make-it-fail" >> $seqres.full
+    echo "$status" > $sysfs_bdev/make-it-fail
+}
+
 _start_fail_scratch_dev()
 {
-    local SYSFS_BDEV=`_sysfs_dev $SCRATCH_DEV`
     echo "Force SCRATCH_DEV device failure"
-    echo " echo 1 > $SYSFS_BDEV/make-it-fail" >> $seqres.full
-    echo 1 > $SYSFS_BDEV/make-it-fail
+
+    _bdev_fail_make_request $SCRATCH_DEV 1
+    [ "$USE_EXTERNAL" = yes -a ! -z "$SCRATCH_LOGDEV" ] && \
+        _bdev_fail_make_request $SCRATCH_LOGDEV 1
+    [ "$USE_EXTERNAL" = yes -a ! -z "$SCRATCH_RTDEV" ] && \
+        _bdev_fail_make_request $SCRATCH_RTDEV 1
 }
 
 _stop_fail_scratch_dev()
 {
-    local SYSFS_BDEV=`_sysfs_dev $SCRATCH_DEV`
     echo "Make SCRATCH_DEV device operable again"
-    echo " echo 0 > $SYSFS_BDEV/make-it-fail" >> $seqres.full
-    echo 0 > $SYSFS_BDEV/make-it-fail
+
+    [ "$USE_EXTERNAL" = yes -a ! -z "$SCRATCH_RTDEV" ] && \
+        _bdev_fail_make_request $SCRATCH_RTDEV 0
+    [ "$USE_EXTERNAL" = yes -a ! -z "$SCRATCH_LOGDEV" ] && \
+        _bdev_fail_make_request $SCRATCH_LOGDEV 0
+    _bdev_fail_make_request $SCRATCH_DEV 0
 }

Re: [PATCH 2/2] fail_make_request: teach helpers about external devices

[Christoph Hellwig]

Ah, and this adds the per-bdev helpers.  So this looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

I can add an incremental patch to convert btrfs/271

Re: [PATCH 2/2] fail_make_request: teach helpers about external devices

[Darrick J. Wong]

On Thu, Jul 28, 2022 at 12:02:24PM -0700, Christoph Hellwig wrote:
> Ah, and this adds the per-bdev helpers.  So this looks fine:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> I can add an incremental patch to convert btrfs/271

Yes please. :)

--D

[PATCHSET v2 0/2] fstests: enhance fail_make_request

[Darrick J. Wong]

Hi all,

This series starts by refactoring boilerplate code around
fail_make_request (aka error injection in the block layer) and then
enhances it to play nicely with multi-device XFS filesystems.

v2: add hch reviews

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=fix-fail-make-request
---
 common/fail_make_request |   63 ++++++++++++++++++++++++++++++++++++++++++++++
 common/rc                |    7 -----
 tests/btrfs/088          |   14 ++++------
 tests/btrfs/150          |   13 ++++-----
 tests/generic/019        |   40 ++++-------------------------
 5 files changed, 78 insertions(+), 59 deletions(-)
 create mode 100644 common/fail_make_request

[PATCH 2/2] fail_make_request: teach helpers about external devices

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Teach the fail_make_request helpers about external log and realtime
devices so that we can use generic/019 on exotic XFS configurations.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 common/fail_make_request |   28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)


diff --git a/common/fail_make_request b/common/fail_make_request
index 581d176a..9f8ea500 100644
--- a/common/fail_make_request
+++ b/common/fail_make_request
@@ -30,18 +30,34 @@ _disallow_fail_make_request()
     echo 0 > $DEBUGFS_MNT/fail_make_request/verbose
 }
 
+_bdev_fail_make_request()
+{
+    local bdev="$1"
+    local status="$2"
+    local sysfs_bdev=$(_sysfs_dev $bdev)
+
+    echo " echo $status > $sysfs_bdev/make-it-fail" >> $seqres.full
+    echo "$status" > $sysfs_bdev/make-it-fail
+}
+
 _start_fail_scratch_dev()
 {
-    local SYSFS_BDEV=`_sysfs_dev $SCRATCH_DEV`
     echo "Force SCRATCH_DEV device failure"
-    echo " echo 1 > $SYSFS_BDEV/make-it-fail" >> $seqres.full
-    echo 1 > $SYSFS_BDEV/make-it-fail
+
+    _bdev_fail_make_request $SCRATCH_DEV 1
+    [ "$USE_EXTERNAL" = yes -a ! -z "$SCRATCH_LOGDEV" ] && \
+        _bdev_fail_make_request $SCRATCH_LOGDEV 1
+    [ "$USE_EXTERNAL" = yes -a ! -z "$SCRATCH_RTDEV" ] && \
+        _bdev_fail_make_request $SCRATCH_RTDEV 1
 }
 
 _stop_fail_scratch_dev()
 {
-    local SYSFS_BDEV=`_sysfs_dev $SCRATCH_DEV`
     echo "Make SCRATCH_DEV device operable again"
-    echo " echo 0 > $SYSFS_BDEV/make-it-fail" >> $seqres.full
-    echo 0 > $SYSFS_BDEV/make-it-fail
+
+    [ "$USE_EXTERNAL" = yes -a ! -z "$SCRATCH_RTDEV" ] && \
+        _bdev_fail_make_request $SCRATCH_RTDEV 0
+    [ "$USE_EXTERNAL" = yes -a ! -z "$SCRATCH_LOGDEV" ] && \
+        _bdev_fail_make_request $SCRATCH_LOGDEV 0
+    _bdev_fail_make_request $SCRATCH_DEV 0
 }