Re: [PATCH blktests] block/044: basic block error injection sanity test

[Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

[-- Attachment #1: Type: text/plain, Size: 3966 bytes --]

Hi Christoph, thanks for the patch. I ran the test case with block/for-next
kernel branch tip and confirmed that it is working as expected.

Please find my comments in line. FYI, I atttach the patch which reflects my
comments. If you are fine with the changes, please let me know so that I can
fold in the change and apply this patch.

On Jun 22, 2026 / 18:08, Christoph Hellwig wrote:
> Test the basic block layer error injection functionality.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  tests/block/044     | 71 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/block/044.out |  9 ++++++
>  2 files changed, 80 insertions(+)
>  create mode 100755 tests/block/044
>  create mode 100644 tests/block/044.out
> 
> diff --git a/tests/block/044 b/tests/block/044
> new file mode 100755
> index 000000000000..8baf9fa59c68
> --- /dev/null
> +++ b/tests/block/044
> @@ -0,0 +1,71 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0

Nit: Majority of the blktests test cases have GPL-3.0+. If you do not mind,
I suggest GPL-3.0+.

> +# Copyright (c) 2026 Christoph Hellwig.
> +#
> +# Basic block error injection test.
> +
> +. tests/block/rc
> +. common/scsi_debug
> +
> +DESCRIPTION="basic block error injection test"
> +QUICK=1
> +
> +requires()
> +{	

Nit: a stray tab in the line above.

I suggest to add the line below.

       _have_kernel_option BLK_ERROR_INJECTION

This way, we can confirm the kernel has the required changes and
the dependent feature is enabled.

> +	_have_loadable_scsi_debug
> +	_have_program xfs_io
> +}
> +
> +# load and remove scsi_debug once to test the static_key bug in the
> +# initial commit
> +_test_load_unload()
> +{
> +	if ! _init_scsi_debug dev_size_mb=500; then
> +		return 1
> +	fi
> +
> +	local dev=${SCSI_DEBUG_DEVICES[0]}
> +	local debugfs_file="/sys/kernel/debug/block/$dev/error_injection"
> +	if [[ ! -f "${debugfs_file}" ]]; then
> +		SKIP_REASONS+=("error injection not supported")
> +		_exit_scsi_debug
> +		return 1
> +	fi

With BLK_ERROR_INJECTION check in requires(), 7 lines above will not be
required. This will simplify this bash function.

> +	echo "Testing unload without rules"
> +	_exit_scsi_debug
> +}
> +
> +_test_rules()
> +{
> +	if ! _init_scsi_debug dev_size_mb=500; then
> +		return 1
> +	fi
> +
> +	local dev=${SCSI_DEBUG_DEVICES[0]}
> +	local debugfs_file="/sys/kernel/debug/block/$dev/error_injection"
> +
> +	echo "Testing valid rules"
> +	echo "add,op=WRITE,status=RESOURCE,start=0,nr_sectors=8" > $debugfs_file
> +	echo "add,op=READ,status=IOERR,start=16,nr_sectors=8" > $debugfs_file
> +	xfs_io -d -c 'pwrite -q 0 4096' /dev/$dev
> +	xfs_io -d -c 'pread -q 0 4096' /dev/$dev
> +	xfs_io -d -c 'pwrite -q 4096 4096' /dev/$dev
> +	xfs_io -d -c 'pread -q 8192 8192' /dev/$dev
> +
> +	echo "Testing invalid rules"
> +	echo "op=READ,status=IOERR" > $debugfs_file
> +	echo "add,op=READ,status=EIO,start=32" > $debugfs_file

$debugfs_file and $dev in this function require double quotation. Otherwise,
shellcheck will warn about them.

$ make check-parallel
Running shellcheck with 4 parallel jobs...
tests/block/044:48:61: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/block/044:49:58: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/block/044:50:39: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/block/044:51:38: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/block/044:52:42: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/block/044:53:41: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/block/044:56:32: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/block/044:57:43: note: Double quote to prevent globbing and word splitting. [SC2086]

> +	_exit_scsi_debug
> +}
> +
> +test()
> +{
> +	echo "Running ${TEST_NAME}"
> +
> +	local ret
> +

The local variable declaration is not required.



[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1885 bytes --]

diff --git a/tests/block/044 b/tests/block/044
index 8baf9fa..d31aa4c 100755
--- a/tests/block/044
+++ b/tests/block/044
@@ -11,7 +11,8 @@ DESCRIPTION="basic block error injection test"
 QUICK=1
 
 requires()
-{	
+{
+	_have_kernel_option BLK_ERROR_INJECTION
 	_have_loadable_scsi_debug
 	_have_program xfs_io
 }
@@ -23,14 +24,6 @@ _test_load_unload()
 	if ! _init_scsi_debug dev_size_mb=500; then
 		return 1
 	fi
-
-	local dev=${SCSI_DEBUG_DEVICES[0]}
-	local debugfs_file="/sys/kernel/debug/block/$dev/error_injection"
-	if [[ ! -f "${debugfs_file}" ]]; then
-		SKIP_REASONS+=("error injection not supported")
-		_exit_scsi_debug
-		return 1
-	fi
 	echo "Testing unload without rules"
 	_exit_scsi_debug
 }
@@ -45,16 +38,16 @@ _test_rules()
 	local debugfs_file="/sys/kernel/debug/block/$dev/error_injection"
 
 	echo "Testing valid rules"
-	echo "add,op=WRITE,status=RESOURCE,start=0,nr_sectors=8" > $debugfs_file
-	echo "add,op=READ,status=IOERR,start=16,nr_sectors=8" > $debugfs_file
-	xfs_io -d -c 'pwrite -q 0 4096' /dev/$dev
-	xfs_io -d -c 'pread -q 0 4096' /dev/$dev
-	xfs_io -d -c 'pwrite -q 4096 4096' /dev/$dev
-	xfs_io -d -c 'pread -q 8192 8192' /dev/$dev
+	echo "add,op=WRITE,status=RESOURCE,start=0,nr_sectors=8" > "$debugfs_file"
+	echo "add,op=READ,status=IOERR,start=16,nr_sectors=8" > "$debugfs_file"
+	xfs_io -d -c 'pwrite -q 0 4096' /dev/"$dev"
+	xfs_io -d -c 'pread -q 0 4096' /dev/"$dev"
+	xfs_io -d -c 'pwrite -q 4096 4096' /dev/"$dev"
+	xfs_io -d -c 'pread -q 8192 8192' /dev/"$dev"
 
 	echo "Testing invalid rules"
-	echo "op=READ,status=IOERR" > $debugfs_file
-	echo "add,op=READ,status=EIO,start=32" > $debugfs_file
+	echo "op=READ,status=IOERR" > "$debugfs_file"
+	echo "add,op=READ,status=EIO,start=32" > "$debugfs_file"
 	_exit_scsi_debug
 }
 
@@ -62,8 +55,6 @@ test()
 {
 	echo "Running ${TEST_NAME}"
 
-	local ret
-
 	_test_load_unload
 	_test_rules