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

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

[Saranya Muruganandam]

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"
+    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
-- 
2.44.0.478.gd926399ef9-goog

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

[Christoph Hellwig]

Thanks,

this patch looks good to me.

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

[Chaitanya Kulkarni]

+ 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

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

[Chaitanya Kulkarni]

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

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

[Bart Van Assche]

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.

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

[Saranya Muruganandam]

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     | 93 +++++++++++++++++++++++++++++++++++++++++++++
 tests/block/035.out |  3 ++
 2 files changed, 96 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..67896ea
--- /dev/null
+++ b/tests/block/035
@@ -0,0 +1,93 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2024 Google LLC
+#
+# 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/fail_make_request"
+
+PROBABILITY=0
+TIMES=0
+VERBOSE=0
+MAKE_FAIL=0
+
+_have_debugfs() {
+
+	if [[ ! -d "${DEBUGFS_MNT}" ]]; then
+		SKIP_REASONS+=("debugfs does not exist")
+		return 1
+	fi
+	return 0
+}
+
+requires() {
+	_have_debugfs
+}
+
+save_fail_make_request()
+{
+	# Save existing global fail_make_request settings
+	PROBABILITY=$(cat "${DEBUGFS_MNT}"/probability)
+	TIMES=$(cat "${DEBUGFS_MNT}"/times)
+	VERBOSE=$(cat "${DEBUGFS_MNT}"/verbose)
+
+	# Save TEST_DEV make-it-fail setting
+	MAKE_FAIL=$(cat /sys/block/$(basename "${TEST_DEV}")/make-it-fail)
+
+}
+
+allow_fail_make_request()
+{
+	# Allow global fail_make_request feature
+	echo 100 > "${DEBUGFS_MNT}"/probability
+	echo 9999999 > "${DEBUGFS_MNT}"/times
+	echo 0 > "${DEBUGFS_MNT}"/verbose
+
+	# Force TEST_DEV device failure
+	echo 1 > /sys/block/$(basename "${TEST_DEV}")/make-it-fail
+
+}
+
+restore_fail_make_request()
+{
+	echo "${MAKE_FAIL}" > /sys/block/$(basename "${TEST_DEV}")/make-it-fail
+
+	# Disallow global fail_make_request feature
+	echo "${PROBABILITY}" > "${DEBUGFS_MNT}"/probability
+	echo "${TIMES}" > "${DEBUGFS_MNT}"/times
+	echo "${VERBOSE}" > "${DEBUGFS_MNT}"/verbose
+}
+
+test_device() {
+	echo "Running ${TEST_NAME}"
+
+	# Save configuration
+	save_fail_make_request
+
+	# set up device for failure
+	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)
+	if [[ $(echo "${out}" | grep -q "Input/output error") -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"
+
+	# Restore TEST_DEV device to original state
+	restore_fail_make_request
+
+	echo "Test complete"
+}
+
diff --git a/tests/block/035.out b/tests/block/035.out
new file mode 100644
index 0000000..0f97f6b
--- /dev/null
+++ b/tests/block/035.out
@@ -0,0 +1,3 @@
+Running block/035
+Return EIO for BLKRRPART on bad disk
+Test complete
-- 
2.44.0.478.gd926399ef9-goog

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

[Shinichiro Kawasaki]

On Apr 06, 2024 / 04:46, 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>

Hello Saranya, thank you for the patch.

I ran the test case with the kernel version v6.9-rc2, and observed the test
case passes. This is unexpected. My expectation is that the test case fails and
reports that the BLKRRPART does not fail with EIO. If I misunderstanding
anything, please let me know. I made comments about this in line based on my
understanding.

I saw the number of shellcheck warnings has got reduced (thanks!). Still I see
a few warnings and commented on them in line.

Also I made some nit comments, which I don't care much. If they are reasonable,
please reflect to v3.

> ---
>  tests/block/035     | 93 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/block/035.out |  3 ++
>  2 files changed, 96 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..67896ea
> --- /dev/null
> +++ b/tests/block/035
> @@ -0,0 +1,93 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2024 Google LLC
> +#
> +# 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

Nit: I would put a blank line here, since the global variables above are
configurations passed to blktests framework, and the global variable below
is a definition for this test case.

> +DEBUGFS_MNT="/sys/kernel/debug/fail_make_request"
> +
> +PROBABILITY=0
> +TIMES=0
> +VERBOSE=0
> +MAKE_FAIL=0
> +
> +_have_debugfs() {
> +

Nit: Useless blank line?

> +	if [[ ! -d "${DEBUGFS_MNT}" ]]; then
> +		SKIP_REASONS+=("debugfs does not exist")
> +		return 1
> +	fi
> +	return 0
> +}
> +
> +requires() {
> +	_have_debugfs
> +}
> +
> +save_fail_make_request()
> +{
> +	# Save existing global fail_make_request settings
> +	PROBABILITY=$(cat "${DEBUGFS_MNT}"/probability)
> +	TIMES=$(cat "${DEBUGFS_MNT}"/times)
> +	VERBOSE=$(cat "${DEBUGFS_MNT}"/verbose)
> +
> +	# Save TEST_DEV make-it-fail setting
> +	MAKE_FAIL=$(cat /sys/block/$(basename "${TEST_DEV}")/make-it-fail)

Blktests provides "${TEST_DEV_SYSFS}" which can be used in place of
"/sys/block/$(basename "${TEST_DEV}")".

        MAKE_FAIL=$(cat "${TEST_DEV_SYSFS}"/make-it-fail)

It will avoid the shellcheck warn SC2046 for $(basename ...).

> +

Nit: Useless blank line?

> +}
> +
> +allow_fail_make_request()
> +{
> +	# Allow global fail_make_request feature
> +	echo 100 > "${DEBUGFS_MNT}"/probability
> +	echo 9999999 > "${DEBUGFS_MNT}"/times
> +	echo 0 > "${DEBUGFS_MNT}"/verbose
> +
> +	# Force TEST_DEV device failure
> +	echo 1 > /sys/block/$(basename "${TEST_DEV}")/make-it-fail

Same comment as above for TEST_DEV_SYSFS.

> +

Nit: Useless blank line?

> +}
> +
> +restore_fail_make_request()
> +{
> +	echo "${MAKE_FAIL}" > /sys/block/$(basename "${TEST_DEV}")/make-it-fail

Same comment as above for TEST_DEV_SYSFS.

> +
> +	# Disallow global fail_make_request feature
> +	echo "${PROBABILITY}" > "${DEBUGFS_MNT}"/probability
> +	echo "${TIMES}" > "${DEBUGFS_MNT}"/times
> +	echo "${VERBOSE}" > "${DEBUGFS_MNT}"/verbose
> +}
> +
> +test_device() {
> +	echo "Running ${TEST_NAME}"
> +
> +	# Save configuration
> +	save_fail_make_request
> +
> +	# set up device for failure
> +	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)

On the kernel v6.9-rc2, the blockdev command above succeeds. I think this is
unexpected, and test case should fail in that case, like,

     if blockdev --rereadpt "${TEST_DEV}" &> "$FULL"; then
          echo "blockdev --rereadpt command succeeded"
     fi

Also, to avoid the shellcheck warn about the local variable "out", let's not use
it. The content of the variable is written to $FULL anyway.

> +	if [[ $(echo "${out}" | grep -q "Input/output error") -eq 0 ]]; then
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This line looks wrong to me. $(command) runs the command and handles its output
as a variable. "grep -q" writes nothing to output, then the marked part in the
line is always an empty variable. [[ X -eq Y ]] evaluates the empty variable as
0, then the condition check above is always true.

I guess you wanted to check that the exit status of "grep -q" is 0 or not. It
can be checked as follows:

       if grep -q "Input/output error" "$FULL"; then

If you keep the "$out" variable, it will be as follows:

       if grep -q "Input/output error" <<< "$out"; then

> +		echo "Return EIO for BLKRRPART on bad disk"
> +	else
> +		echo "Did not return EIO for BLKRRPART on bad disk"
> +	fi
> +
> +	echo "${out}" >> "$FULL"
> +
> +	# Restore TEST_DEV device to original state
> +	restore_fail_make_request
> +
> +	echo "Test complete"
> +}
> +

Nit: this blank line is not needed, and the git apply command warns about it.

> diff --git a/tests/block/035.out b/tests/block/035.out
> new file mode 100644
> index 0000000..0f97f6b
> --- /dev/null
> +++ b/tests/block/035.out
> @@ -0,0 +1,3 @@
> +Running block/035
> +Return EIO for BLKRRPART on bad disk
> +Test complete
> -- 
> 2.44.0.478.gd926399ef9-goog
> 

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

[Saranya Muruganandam]

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     | 86 +++++++++++++++++++++++++++++++++++++++++++++
 tests/block/035.out |  3 ++
 2 files changed, 89 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..e15f115
--- /dev/null
+++ b/tests/block/035
@@ -0,0 +1,86 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2024 Google LLC
+#
+# 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/fail_make_request"
+PROBABILITY=0
+TIMES=0
+VERBOSE=0
+MAKE_FAIL=0
+
+_have_debugfs() {
+	if [[ ! -d "${DEBUGFS_MNT}" ]]; then
+		SKIP_REASONS+=("debugfs does not exist")
+		return 1
+	fi
+	return 0
+}
+
+requires() {
+	_have_debugfs
+}
+
+save_fail_make_request()
+{
+	# Save existing global fail_make_request settings
+	PROBABILITY=$(cat "${DEBUGFS_MNT}"/probability)
+	TIMES=$(cat "${DEBUGFS_MNT}"/times)
+	VERBOSE=$(cat "${DEBUGFS_MNT}"/verbose)
+
+	# Save TEST_DEV make-it-fail setting
+	MAKE_FAIL=$(cat "${TEST_DEV_SYSFS}"/make-it-fail)
+}
+
+allow_fail_make_request()
+{
+	# Allow global fail_make_request feature
+	echo 100 > "${DEBUGFS_MNT}"/probability
+	echo 9999999 > "${DEBUGFS_MNT}"/times
+	echo 0 > "${DEBUGFS_MNT}"/verbose
+
+	# Force TEST_DEV device failure
+	echo 1 > "${TEST_DEV_SYSFS}"/make-it-fail
+}
+
+restore_fail_make_request()
+{
+	echo "${MAKE_FAIL}" > "${TEST_DEV_SYSFS}"/make-it-fail
+
+	# Disallow global fail_make_request feature
+	echo "${PROBABILITY}" > "${DEBUGFS_MNT}"/probability
+	echo "${TIMES}" > "${DEBUGFS_MNT}"/times
+	echo "${VERBOSE}" > "${DEBUGFS_MNT}"/verbose
+}
+
+test_device() {
+	echo "Running ${TEST_NAME}"
+
+	# Save configuration
+	save_fail_make_request
+
+	# set up device for failure
+	allow_fail_make_request
+
+	# Check rereading partitions on bad disk cannot open /dev/sdc: Input/output error
+	if blockdev --rereadpt "${TEST_DEV}" &> "$FULL"; then
+		echo "Did not return EIO for BLKRRPART on bad disk"
+	else
+		echo "Return EIO for BLKRRPART on bad disk"
+	fi
+
+	# Restore TEST_DEV device to original state
+	restore_fail_make_request
+
+	echo "Test complete"
+}
diff --git a/tests/block/035.out b/tests/block/035.out
new file mode 100644
index 0000000..0f97f6b
--- /dev/null
+++ b/tests/block/035.out
@@ -0,0 +1,3 @@
+Running block/035
+Return EIO for BLKRRPART on bad disk
+Test complete
-- 
2.44.0.478.gd926399ef9-goog

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

[Shinichiro Kawasaki]

On Apr 10, 2024 / 00:15, 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>

Thanks for the update. Looks cleaner. Still I have two more comments in line.

> ---
>  tests/block/035     | 86 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/block/035.out |  3 ++
>  2 files changed, 89 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..e15f115
> --- /dev/null
> +++ b/tests/block/035
> @@ -0,0 +1,86 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2024 Google LLC
> +#
> +# 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/fail_make_request"
> +PROBABILITY=0
> +TIMES=0
> +VERBOSE=0
> +MAKE_FAIL=0
> +
> +_have_debugfs() {
> +	if [[ ! -d "${DEBUGFS_MNT}" ]]; then
> +		SKIP_REASONS+=("debugfs does not exist")
> +		return 1
> +	fi
> +	return 0
> +}
> +
> +requires() {
> +	_have_debugfs
> +}
> +
> +save_fail_make_request()
> +{
> +	# Save existing global fail_make_request settings
> +	PROBABILITY=$(cat "${DEBUGFS_MNT}"/probability)
> +	TIMES=$(cat "${DEBUGFS_MNT}"/times)
> +	VERBOSE=$(cat "${DEBUGFS_MNT}"/verbose)
> +
> +	# Save TEST_DEV make-it-fail setting
> +	MAKE_FAIL=$(cat "${TEST_DEV_SYSFS}"/make-it-fail)
> +}
> +
> +allow_fail_make_request()
> +{
> +	# Allow global fail_make_request feature
> +	echo 100 > "${DEBUGFS_MNT}"/probability
> +	echo 9999999 > "${DEBUGFS_MNT}"/times
> +	echo 0 > "${DEBUGFS_MNT}"/verbose
> +
> +	# Force TEST_DEV device failure
> +	echo 1 > "${TEST_DEV_SYSFS}"/make-it-fail
> +}
> +
> +restore_fail_make_request()
> +{
> +	echo "${MAKE_FAIL}" > "${TEST_DEV_SYSFS}"/make-it-fail
> +
> +	# Disallow global fail_make_request feature
> +	echo "${PROBABILITY}" > "${DEBUGFS_MNT}"/probability
> +	echo "${TIMES}" > "${DEBUGFS_MNT}"/times
> +	echo "${VERBOSE}" > "${DEBUGFS_MNT}"/verbose
> +}
> +
> +test_device() {
> +	echo "Running ${TEST_NAME}"
> +
> +	# Save configuration
> +	save_fail_make_request
> +
> +	# set up device for failure
> +	allow_fail_make_request
> +
> +	# Check rereading partitions on bad disk cannot open /dev/sdc: Input/output error

Nit: /dev/sdc is not valid here. TEST_DEV is the appropriate word, I think.

> +	if blockdev --rereadpt "${TEST_DEV}" &> "$FULL"; then
> +		echo "Did not return EIO for BLKRRPART on bad disk"
> +	else

Why did you remove the grep for "Input/output error" in "$FULL" here? Without
this check, this test case allows other errors than EIO. This is inconsistent
with the commit message and the comments in this test case.

As I commented on the kernel side patch, "blockdev --rereadpt" returned
unexpected EINVAL. I think this case should catch it.

> +		echo "Return EIO for BLKRRPART on bad disk"
> +	fi
> +
> +	# Restore TEST_DEV device to original state
> +	restore_fail_make_request
> +
> +	echo "Test complete"
> +}
> diff --git a/tests/block/035.out b/tests/block/035.out
> new file mode 100644
> index 0000000..0f97f6b
> --- /dev/null
> +++ b/tests/block/035.out
> @@ -0,0 +1,3 @@
> +Running block/035
> +Return EIO for BLKRRPART on bad disk
> +Test complete
> -- 
> 2.44.0.478.gd926399ef9-goog
> 

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

[Saranya Muruganandam]

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     | 86 +++++++++++++++++++++++++++++++++++++++++++++
 tests/block/035.out |  4 +++
 2 files changed, 90 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..0ba6292
--- /dev/null
+++ b/tests/block/035
@@ -0,0 +1,86 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2024 Google LLC
+#
+# 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/fail_make_request"
+PROBABILITY=0
+TIMES=0
+VERBOSE=0
+MAKE_FAIL=0
+
+_have_debugfs() {
+	if [[ ! -d "${DEBUGFS_MNT}" ]]; then
+		SKIP_REASONS+=("debugfs does not exist")
+		return 1
+	fi
+	return 0
+}
+
+requires() {
+	_have_debugfs
+}
+
+save_fail_make_request()
+{
+	# Save existing global fail_make_request settings
+	PROBABILITY=$(cat "${DEBUGFS_MNT}"/probability)
+	TIMES=$(cat "${DEBUGFS_MNT}"/times)
+	VERBOSE=$(cat "${DEBUGFS_MNT}"/verbose)
+
+	# Save TEST_DEV make-it-fail setting
+	MAKE_FAIL=$(cat "${TEST_DEV_SYSFS}"/make-it-fail)
+}
+
+allow_fail_make_request()
+{
+	# Allow global fail_make_request feature
+	echo 100 > "${DEBUGFS_MNT}"/probability
+	echo 9999999 > "${DEBUGFS_MNT}"/times
+	echo 0 > "${DEBUGFS_MNT}"/verbose
+
+	# Force TEST_DEV device failure
+	echo 1 > "${TEST_DEV_SYSFS}"/make-it-fail
+}
+
+restore_fail_make_request()
+{
+	echo "${MAKE_FAIL}" > "${TEST_DEV_SYSFS}"/make-it-fail
+
+	# Disallow global fail_make_request feature
+	echo "${PROBABILITY}" > "${DEBUGFS_MNT}"/probability
+	echo "${TIMES}" > "${DEBUGFS_MNT}"/times
+	echo "${VERBOSE}" > "${DEBUGFS_MNT}"/verbose
+}
+
+test_device() {
+	echo "Running ${TEST_NAME}"
+
+	# Save configuration
+	save_fail_make_request
+
+	# set up device for failure
+	allow_fail_make_request
+
+	# Check rereading partitions on bad disk cannot open $TEST_DEV: Input/output error
+	if blockdev --rereadpt "${TEST_DEV}" | grep -q "Input/output error" &> "$FULL"; then
+		echo "Did not return EIO for BLKRRPART on bad disk"
+	else
+		echo "Return EIO for BLKRRPART on bad disk"
+	fi
+
+	# Restore TEST_DEV device to original state
+	restore_fail_make_request
+
+	echo "Test complete"
+}
diff --git a/tests/block/035.out b/tests/block/035.out
new file mode 100644
index 0000000..125f4b8
--- /dev/null
+++ b/tests/block/035.out
@@ -0,0 +1,4 @@
+Running block/035
+blockdev: ioctl error on BLKRRPART: Input/output error
+Return EIO for BLKRRPART on bad disk
+Test complete
-- 
2.44.0.683.g7961c838ac-goog

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

[Shinichiro Kawasaki]

On Apr 10, 2024 / 17:02, 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>

I ran the test case with this patch on the kernel v6.9-rc3, then observed the
output below.

block/035 => sdc (test return EIO from BLKRRPART for whole-dev) [failed]
    runtime  0.049s  ...  0.041s
    --- tests/block/035.out     2024-04-11 13:40:17.938655578 +0900
    +++ /home/shin/Blktests/blktests/results/sdc/block/035.out.bad      2024-04-11 13:40:19.808650194 +0900
    @@ -1,4 +1,3 @@
     Running block/035
    -blockdev: ioctl error on BLKRRPART: Input/output error
     Return EIO for BLKRRPART on bad disk
     Test complete

Failure is expected but the output is weird. The line "Return EIO for BLKRRPART
on bad disk" indicates that EIO was returned, which is wrong. Please find my
comments in line about this.

BTW, "The canonical patch format" section in Linux kernel Documentation/process/
submitting-patches.rst describes the patch version descriptor (v1, v2...) and
the patch changelog below the '---' separator line. Those practices help
reviewing kernel patches. I encourage to follow them for blktests patches also.

> ---
>  tests/block/035     | 86 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/block/035.out |  4 +++
>  2 files changed, 90 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..0ba6292
> --- /dev/null
> +++ b/tests/block/035
> @@ -0,0 +1,86 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2024 Google LLC
> +#
> +# 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/fail_make_request"
> +PROBABILITY=0
> +TIMES=0
> +VERBOSE=0
> +MAKE_FAIL=0
> +
> +_have_debugfs() {
> +	if [[ ! -d "${DEBUGFS_MNT}" ]]; then
> +		SKIP_REASONS+=("debugfs does not exist")
> +		return 1
> +	fi
> +	return 0
> +}
> +
> +requires() {
> +	_have_debugfs
> +}
> +
> +save_fail_make_request()
> +{
> +	# Save existing global fail_make_request settings
> +	PROBABILITY=$(cat "${DEBUGFS_MNT}"/probability)
> +	TIMES=$(cat "${DEBUGFS_MNT}"/times)
> +	VERBOSE=$(cat "${DEBUGFS_MNT}"/verbose)
> +
> +	# Save TEST_DEV make-it-fail setting
> +	MAKE_FAIL=$(cat "${TEST_DEV_SYSFS}"/make-it-fail)
> +}
> +
> +allow_fail_make_request()
> +{
> +	# Allow global fail_make_request feature
> +	echo 100 > "${DEBUGFS_MNT}"/probability
> +	echo 9999999 > "${DEBUGFS_MNT}"/times
> +	echo 0 > "${DEBUGFS_MNT}"/verbose
> +
> +	# Force TEST_DEV device failure
> +	echo 1 > "${TEST_DEV_SYSFS}"/make-it-fail
> +}
> +
> +restore_fail_make_request()
> +{
> +	echo "${MAKE_FAIL}" > "${TEST_DEV_SYSFS}"/make-it-fail
> +
> +	# Disallow global fail_make_request feature
> +	echo "${PROBABILITY}" > "${DEBUGFS_MNT}"/probability
> +	echo "${TIMES}" > "${DEBUGFS_MNT}"/times
> +	echo "${VERBOSE}" > "${DEBUGFS_MNT}"/verbose
> +}
> +
> +test_device() {
> +	echo "Running ${TEST_NAME}"
> +
> +	# Save configuration
> +	save_fail_make_request
> +
> +	# set up device for failure
> +	allow_fail_make_request
> +
> +	# Check rereading partitions on bad disk cannot open $TEST_DEV: Input/output error
> +	if blockdev --rereadpt "${TEST_DEV}" | grep -q "Input/output error" &> "$FULL"; then

On the v6.9-rc3 kernel, blockdev --rereadpt command succeeds and prints nothing.
Then grep -q command receives nothing. It fails to match and returns non-zero
status. Then "Return EIO for BLKRRPART on bad disk" is printed even when
blockdev does not output "Input/output error". The if statement logic should be
reverted.

The grep -q command prints nothing, then the redirect to "$FULL" is meaningless.
I think it's the better to redirect blockdev command output to $FULL

The line above is not working as expected on the kernel with your fix patch
either. The blockdev command prints the "Input/output error" message to stderr.
However the pipeline '|' only passes stdout of the blockdev command. Then the
grep command does not receive the message. That's why the message "blockdev:
ioctl error on BLKRRPART: Input/output error" is printed in the test case
output. I suggest not to use the pipeline.

> +		echo "Did not return EIO for BLKRRPART on bad disk"
> +	else
> +		echo "Return EIO for BLKRRPART on bad disk"
> +	fi
> +
> +	# Restore TEST_DEV device to original state
> +	restore_fail_make_request
> +
> +	echo "Test complete"
> +}
> diff --git a/tests/block/035.out b/tests/block/035.out
> new file mode 100644
> index 0000000..125f4b8
> --- /dev/null
> +++ b/tests/block/035.out
> @@ -0,0 +1,4 @@
> +Running block/035
> +blockdev: ioctl error on BLKRRPART: Input/output error
> +Return EIO for BLKRRPART on bad disk
> +Test complete
> -- 
> 2.44.0.683.g7961c838ac-goog
>

Based on my understandings, I suggest the additional change below on top of this
patch. If you are okay with it, I can fold it in when I apply the patch. Or you
can respin the patch and repost. I'm fine with either way. Please let me know
your preference. (If you repost, please modify the test case number from
block/035 to block/036.) Thanks!

diff --git a/tests/block/035 b/tests/block/035
index 0ba6292..d4e67fd 100755
--- a/tests/block/035
+++ b/tests/block/035
@@ -73,10 +73,11 @@ test_device() {
 	allow_fail_make_request
 
 	# Check rereading partitions on bad disk cannot open $TEST_DEV: Input/output error
-	if blockdev --rereadpt "${TEST_DEV}" | grep -q "Input/output error" &> "$FULL"; then
-		echo "Did not return EIO for BLKRRPART on bad disk"
-	else
+	blockdev --rereadpt "${TEST_DEV}" &> "$FULL"
+	if grep -q "Input/output error" "$FULL"; then
 		echo "Return EIO for BLKRRPART on bad disk"
+	else
+		echo "Did not return EIO for BLKRRPART on bad disk"
 	fi
 
 	# Restore TEST_DEV device to original state
diff --git a/tests/block/035.out b/tests/block/035.out
index 125f4b8..0f97f6b 100644
--- a/tests/block/035.out
+++ b/tests/block/035.out
@@ -1,4 +1,3 @@
 Running block/035
-blockdev: ioctl error on BLKRRPART: Input/output error
 Return EIO for BLKRRPART on bad disk
 Test complete

[PATCH v2] block/035: test return EIO from BLKRRPART

[Saranya Muruganandam]

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     | 87 +++++++++++++++++++++++++++++++++++++++++++++
 tests/block/035.out |  3 ++
 2 files changed, 90 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..d4e67fd
--- /dev/null
+++ b/tests/block/035
@@ -0,0 +1,87 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2024 Google LLC
+#
+# 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/fail_make_request"
+PROBABILITY=0
+TIMES=0
+VERBOSE=0
+MAKE_FAIL=0
+
+_have_debugfs() {
+	if [[ ! -d "${DEBUGFS_MNT}" ]]; then
+		SKIP_REASONS+=("debugfs does not exist")
+		return 1
+	fi
+	return 0
+}
+
+requires() {
+	_have_debugfs
+}
+
+save_fail_make_request()
+{
+	# Save existing global fail_make_request settings
+	PROBABILITY=$(cat "${DEBUGFS_MNT}"/probability)
+	TIMES=$(cat "${DEBUGFS_MNT}"/times)
+	VERBOSE=$(cat "${DEBUGFS_MNT}"/verbose)
+
+	# Save TEST_DEV make-it-fail setting
+	MAKE_FAIL=$(cat "${TEST_DEV_SYSFS}"/make-it-fail)
+}
+
+allow_fail_make_request()
+{
+	# Allow global fail_make_request feature
+	echo 100 > "${DEBUGFS_MNT}"/probability
+	echo 9999999 > "${DEBUGFS_MNT}"/times
+	echo 0 > "${DEBUGFS_MNT}"/verbose
+
+	# Force TEST_DEV device failure
+	echo 1 > "${TEST_DEV_SYSFS}"/make-it-fail
+}
+
+restore_fail_make_request()
+{
+	echo "${MAKE_FAIL}" > "${TEST_DEV_SYSFS}"/make-it-fail
+
+	# Disallow global fail_make_request feature
+	echo "${PROBABILITY}" > "${DEBUGFS_MNT}"/probability
+	echo "${TIMES}" > "${DEBUGFS_MNT}"/times
+	echo "${VERBOSE}" > "${DEBUGFS_MNT}"/verbose
+}
+
+test_device() {
+	echo "Running ${TEST_NAME}"
+
+	# Save configuration
+	save_fail_make_request
+
+	# set up device for failure
+	allow_fail_make_request
+
+	# Check rereading partitions on bad disk cannot open $TEST_DEV: Input/output error
+	blockdev --rereadpt "${TEST_DEV}" &> "$FULL"
+	if grep -q "Input/output error" "$FULL"; then
+		echo "Return EIO for BLKRRPART on bad disk"
+	else
+		echo "Did not return EIO for BLKRRPART on bad disk"
+	fi
+
+	# Restore TEST_DEV device to original state
+	restore_fail_make_request
+
+	echo "Test complete"
+}
diff --git a/tests/block/035.out b/tests/block/035.out
new file mode 100644
index 0000000..0f97f6b
--- /dev/null
+++ b/tests/block/035.out
@@ -0,0 +1,3 @@
+Running block/035
+Return EIO for BLKRRPART on bad disk
+Test complete
-- 
2.44.0.683.g7961c838ac-goog

Re: [PATCH v2] block/035: test return EIO from BLKRRPART

[Shinichiro Kawasaki]

On Apr 11, 2024 / 23:47, 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>

I've applied it, thanks. Please note that I modified the test case number from
block/035 to block/036.