diff mbox series

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

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

Commit Message

Saranya Muruganandam April 6, 2024, 4:46 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     | 93 +++++++++++++++++++++++++++++++++++++++++++++
 tests/block/035.out |  3 ++
 2 files changed, 96 insertions(+)
 create mode 100755 tests/block/035
 create mode 100644 tests/block/035.out

Comments

Shin'ichiro Kawasaki April 8, 2024, 6:45 a.m. UTC | #1 
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
>
diff mbox series

Patch

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