diff mbox series

[blktests] block/035: test return EIO from BLKRRPART

Message ID 20240405015657.751659-1-saranyamohan@google.com (mailing list archive)
State New, archived
Headers show
Series [blktests] block/035: test return EIO from BLKRRPART | expand

Commit Message

Saranya Muruganandam April 5, 2024, 1:56 a.m. UTC 
When we fail to reread the partition superblock from the disk, due to
bad sector or bad disk etc, BLKRRPART should fail with EIO.
Simulate failure for the entire block device and run
"blockdev --rereadpt" and expect it to fail and return EIO instead of
pass.

Link: https://lore.kernel.org/all/20240405014253.748627-1-saranyamohan@google.com/
Signed-off-by: Saranya Muruganandam <saranyamohan@google.com>
---
 tests/block/035     | 80 +++++++++++++++++++++++++++++++++++++++++++++
 tests/block/035.out |  7 ++++
 2 files changed, 87 insertions(+)
 create mode 100755 tests/block/035
 create mode 100644 tests/block/035.out

Comments

Christoph Hellwig April 5, 2024, 6:52 a.m. UTC | #1 
Thanks,

this patch looks good to me.
Chaitanya Kulkarni April 5, 2024, 7:52 a.m. UTC | #2 
+ CC Shinichiro

On 4/4/2024 6:56 PM, Saranya Muruganandam wrote:
> When we fail to reread the partition superblock from the disk, due to
> bad sector or bad disk etc, BLKRRPART should fail with EIO.
> Simulate failure for the entire block device and run
> "blockdev --rereadpt" and expect it to fail and return EIO instead of
> pass.
> 
> Link: https://lore.kernel.org/all/20240405014253.748627-1-saranyamohan@google.com/
> Signed-off-by: Saranya Muruganandam <saranyamohan@google.com>
> ---
>   tests/block/035     | 80 +++++++++++++++++++++++++++++++++++++++++++++
>   tests/block/035.out |  7 ++++
>   2 files changed, 87 insertions(+)
>   create mode 100755 tests/block/035
>   create mode 100644 tests/block/035.out
> 
> diff --git a/tests/block/035 b/tests/block/035
> new file mode 100755
> index 0000000..3b307f1
> --- /dev/null
> +++ b/tests/block/035
> @@ -0,0 +1,80 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2024 Saranya Muruganandam
> +#
> +# Regression test for BLKRRPART.
> +#
> +# If we fail to read the partition table due to bad sector or other IO
> +# failures, running "blockdev --rereadpt" should fail and return
> +# -EIO.  On a buggy kernel, it passes unexpectedly.
> +
> +. tests/block/rc
> +
> +DESCRIPTION="test return EIO from BLKRRPART for whole-dev"
> +QUICK=1
> +DEBUGFS_MNT="/sys/kernel/debug"
> +
> +
> +_have_debugfs() {
> +
> +	if [[ ! -d /sys/kernel/debug ]]; then
> +		SKIP_REASONS+=("debugfs does not exist")
> +		return 1
> +	fi
> +	return 0
> +}
> +
> +requires() {
> +	_have_debugfs
> +}
> +
> +
> +allow_fail_make_request()
> +{
> +    [ -f "$DEBUGFS_MNT/fail_make_request/probability" ] \
> +	|| _notrun "$DEBUGFS_MNT/fail_make_request \
> + not found. Seems that CONFIG_FAIL_MAKE_REQUEST kernel config option not enabled"
> +
> +    echo "Allow global fail_make_request feature"

I don't think we need above print

> +    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
> +
> +    echo "Force TEST_DEV device failure"

same here

> +    echo 1 > /sys/block/$(basename ${TEST_DEV})/make-it-fail
> +
> +}
> +
> +disallow_fail_make_request()
> +{
> +    echo "Make TEST_DEV device operatable again"

same here

> +    echo 0 > /sys/block/$(basename ${TEST_DEV})/make-it-fail
> +
> +    echo "Disallow global fail_make_request feature"

same here

> +    echo 0 > $DEBUGFS_MNT/fail_make_request/probability
> +    echo 0 > $DEBUGFS_MNT/fail_make_request/times
> +}
> +
> +
> +test_device() {
> +	echo "Running ${TEST_NAME}"
> +
> +	allow_fail_make_request
> +
> +	# Check rereading partitions on bad disk cannot open /dev/sdc: Input/output error
> +	local out=$(blockdev --rereadpt ${TEST_DEV} 2>&1)
> +	echo $out | grep -q "Input/output error"
> +	if [ $? -eq 0 ]; then
> +		echo "Return EIO for BLKRRPART on bad disk"
> +	else
> +		echo "Did not return EIO for BLKRRPART on bad disk"
> +	fi
> +
> +	echo $out >> "$FULL"
> +	status=$?
> +	
> +	disallow_fail_make_request
> +

instead of disallow fail make request completely, we need to save the
existing configuration before calling allow_fail_make_request and 
restore that saved configuration here, unless there is a specific reason
for not doing that which I didn't understand ..

> +	echo "Test complete"
> +}
> +
> diff --git a/tests/block/035.out b/tests/block/035.out
> new file mode 100644
> index 0000000..3fbfd77
> --- /dev/null
> +++ b/tests/block/035.out
> @@ -0,0 +1,7 @@
> +Running block/035
> +Allow global fail_make_request feature
> +Force TEST_DEV device failure
> +Return EIO for BLKRRPART on bad disk
> +Make TEST_DEV device operatable again
> +Disallow global fail_make_request feature
> +Test complete


I found several shellcheck warnings, please consider fixing those unless
they are kept for a reason (which I didn't understand why), then
consider disabling it with right shellcheck directive with appropriate
comment supporting reason:-

tests/block/035:44:25: warning: Quote this to prevent word splitting. 
[SC2046] 
tests/block/035:44:36: note: Double quote to prevent globbing and word 
splitting. [SC2086] 
tests/block/035:51:25: warning: Quote this to prevent word splitting. 
[SC2046] 
tests/block/035:51:36: note: Double quote to prevent globbing and word 
splitting. [SC2086] 
tests/block/035:65:8: warning: Declare and assign separately to avoid 
masking return values. [SC2155] 
tests/block/035:65:34: note: Double quote to prevent globbing and word 
splitting. [SC2086] 
tests/block/035:66:7: note: Double quote to prevent globbing and word 
splitting. [SC2086] 
tests/block/035:67:7: note: Check exit code directly with e.g. 'if 
mycmd;', not indirectly with $?. [SC2181] 
tests/block/035:73:7: note: Double quote to prevent globbing and word 
splitting. [SC2086] 
tests/block/035:74:2: warning: status appears unused. Verify use (or 
export if used externally). [SC2034]

-ck
Chaitanya Kulkarni April 5, 2024, 8:05 a.m. UTC | #3 
On 4/5/2024 12:52 AM, Chaitanya Kulkarni wrote:
> +
> +allow_fail_make_request()
> +{
> +    [ -f "$DEBUGFS_MNT/fail_make_request/probability" ] \
> +	|| _notrun "$DEBUGFS_MNT/fail_make_request \

also, I didn't find _notrun function, perhaps I've a broken tree or
didn't understand the code correctly ?

> + not found. Seems that CONFIG_FAIL_MAKE_REQUEST kernel config option not enabled"
> +
> +    echo "Allow global fail_make_request feature"

-ck
Bart Van Assche April 5, 2024, 5:48 p.m. UTC | #4 
On 4/4/24 18:56, Saranya Muruganandam wrote:
> +# Copyright (C) 2024 Saranya Muruganandam
The above should be changed into "Copyright (C) 2024 Google LLC" or
"Copyright 2024 Google LLC" if these instructions are still valid:
https://g3doc.corp.google.com/company/teams/opensource/copyright.md

Thanks,

Bart.
diff mbox series

Patch

diff --git a/tests/block/035 b/tests/block/035
new file mode 100755
index 0000000..3b307f1
--- /dev/null
+++ b/tests/block/035
@@ -0,0 +1,80 @@ 
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2024 Saranya Muruganandam
+#
+# Regression test for BLKRRPART.
+#
+# If we fail to read the partition table due to bad sector or other IO
+# failures, running "blockdev --rereadpt" should fail and return
+# -EIO.  On a buggy kernel, it passes unexpectedly.
+
+. tests/block/rc
+
+DESCRIPTION="test return EIO from BLKRRPART for whole-dev"
+QUICK=1
+DEBUGFS_MNT="/sys/kernel/debug"
+
+
+_have_debugfs() {
+
+	if [[ ! -d /sys/kernel/debug ]]; then
+		SKIP_REASONS+=("debugfs does not exist")
+		return 1
+	fi
+	return 0
+}
+
+requires() {
+	_have_debugfs
+}
+
+
+allow_fail_make_request()
+{
+    [ -f "$DEBUGFS_MNT/fail_make_request/probability" ] \
+	|| _notrun "$DEBUGFS_MNT/fail_make_request \
+ not found. Seems that CONFIG_FAIL_MAKE_REQUEST kernel config option not enabled"
+
+    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
+    
+    echo "Force TEST_DEV device failure"
+    echo 1 > /sys/block/$(basename ${TEST_DEV})/make-it-fail
+
+}
+
+disallow_fail_make_request()
+{
+    echo "Make TEST_DEV device operatable again"
+    echo 0 > /sys/block/$(basename ${TEST_DEV})/make-it-fail
+
+    echo "Disallow global fail_make_request feature"
+    echo 0 > $DEBUGFS_MNT/fail_make_request/probability
+    echo 0 > $DEBUGFS_MNT/fail_make_request/times
+}
+
+
+test_device() {
+	echo "Running ${TEST_NAME}"
+
+	allow_fail_make_request
+
+	# Check rereading partitions on bad disk cannot open /dev/sdc: Input/output error
+	local out=$(blockdev --rereadpt ${TEST_DEV} 2>&1)
+	echo $out | grep -q "Input/output error"
+	if [ $? -eq 0 ]; then
+		echo "Return EIO for BLKRRPART on bad disk"
+	else
+		echo "Did not return EIO for BLKRRPART on bad disk"
+	fi
+
+	echo $out >> "$FULL"
+	status=$?
+	
+	disallow_fail_make_request
+
+	echo "Test complete"
+}
+
diff --git a/tests/block/035.out b/tests/block/035.out
new file mode 100644
index 0000000..3fbfd77
--- /dev/null
+++ b/tests/block/035.out
@@ -0,0 +1,7 @@ 
+Running block/035
+Allow global fail_make_request feature
+Force TEST_DEV device failure
+Return EIO for BLKRRPART on bad disk
+Make TEST_DEV device operatable again
+Disallow global fail_make_request feature
+Test complete