Skip to contentSkip to conversation

block/035: test return EIO from BLKRRPART

PATCH In review Saranya Muruganandam 5 participants 13 messages Apr 5, 2024 – Apr 12, 2024 Block layer

lore
Share & tools
lore

Saranya Muruganandam

author
#0 lore raw

PATCH 1/1 block/035: test return EIO from BLKRRPART

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/[email protected]/ open here
Signed-off-by Saranya Muruganandam <[email protected]>
---
 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
tests/block/035 new file +80 -0
new file mode 100755
index 0000000..3b307f1
@@ -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"
+}
+
tests/block/035.out new file +7 -0
new file mode 100644
index 0000000..3fbfd77
@@ -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
Signature · 1 line
2.44.0.478.gd926399ef9-goog
#2 lore raw
In reply to Saranya Muruganandam

+ CC Shinichiro

Quoted 49 lines · 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/[email protected]/ open here
Signed-off-by Saranya Muruganandam <[email protected]>
---
  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

tests/block/035 new file +28 -0
new file mode 100755
index 0000000..3b307f1
@@ -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

tests/block/035 line 38

Quoted 10 lines
tests/block/035 new file +10 -0
+}
+
+
+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

tests/block/035 line 43

Quoted 5 lines
tests/block/035 new file +5 -0
+    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

tests/block/035 line 50

Quoted 7 lines
tests/block/035 new file +7 -0
+    echo 1 > /sys/block/$(basename ${TEST_DEV})/make-it-fail
+
+}
+
+disallow_fail_make_request()
+{
+    echo "Make TEST_DEV device operatable again"

same here

tests/block/035 line 53

Quoted 3 lines
tests/block/035 new file +3 -0
+    echo 0 > /sys/block/$(basename ${TEST_DEV})/make-it-fail
+
+    echo "Disallow global fail_make_request feature"

same here

Quoted 14 lines
tests/block/035 new file +14 -0
+    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

tests/block/035 line 77

Quoted 10 lines
tests/block/035 new file +10 -0
+		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 ..

Quoted 9 lines
tests/block/035 new file +3 -0
+	echo "Test complete"
+}
+
tests/block/035.out new file +0 -0
new file mode 100644
index 0000000..3fbfd77

tests/block/035.out line 7

Quoted 7 lines
tests/block/035.out new file +7 -0
@@ -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

#3 lore raw

tests/block/035 line 35

Quoted 5 lines · Chaitanya Kulkarni
tests/block/035 new file +5 -0
+
+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 ?

Review

Quoted 3 lines

+ not found. Seems that CONFIG_FAIL_MAKE_REQUEST kernel config option not enabled"

(quoted diff) +2 -0
+
+    echo "Allow global fail_make_request feature"

-ck

author
#4 lore raw

PATCH 1/1 block/035: test return EIO from BLKRRPART

In reply to Chaitanya Kulkarni

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/[email protected]/ open here
Signed-off-by Saranya Muruganandam <[email protected]>
---
 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
tests/block/035 new file +93 -0
new file mode 100755
index 0000000..67896ea
@@ -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"
+}
+
tests/block/035.out new file +3 -0
new file mode 100644
index 0000000..0f97f6b
@@ -0,0 +1,3 @@
+Running block/035
+Return EIO for BLKRRPART on bad disk
+Test complete
Signature · 1 line
2.44.0.478.gd926399ef9-goog
#5 lore raw
Quoted 8 lines · 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/[email protected]/ open here
Signed-off-by Saranya Muruganandam <[email protected]>

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.

Quoted 17 lines
---
 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
tests/block/035 new file +4 -0
new file mode 100755
index 0000000..67896ea
@@ -0,0 +1,93 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2024 Google LLC
+#

tests/block/035 line 14

Quoted 10 lines
tests/block/035 new file +10 -0
+# 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.

tests/block/035 line 23

Quoted 9 lines
tests/block/035 new file +9 -0
+DEBUGFS_MNT="/sys/kernel/debug/fail_make_request"
+
+PROBABILITY=0
+TIMES=0
+VERBOSE=0
+MAKE_FAIL=0
+
+_have_debugfs() {
+

Nit: Useless blank line?

Quoted 10 lines
tests/block/035 new file +10 -0
+	if [[ ! -d "${DEBUGFS_MNT}" ]]; then
+		SKIP_REASONS+=("debugfs does not exist")
+		return 1
+	fi
+	return 0
+}
+
+requires() {
+	_have_debugfs
+}

tests/block/035 line 43

Quoted 10 lines
tests/block/035 new file +10 -0
+
+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 ...).

tests/block/035 line 44

Quoted 1 line
tests/block/035 new file +1 -0
+

Nit: Useless blank line?

tests/block/035 line 55

Quoted 11 lines
tests/block/035 new file +11 -0
+}
+
+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.

tests/block/035 line 56

Quoted 1 line
tests/block/035 new file +1 -0
+

Nit: Useless blank line?

tests/block/035 line 61

Quoted 5 lines
tests/block/035 new file +5 -0
+}
+
+restore_fail_make_request()
+{
+	echo "${MAKE_FAIL}" > /sys/block/$(basename "${TEST_DEV}")/make-it-fail

Same comment as above for TEST_DEV_SYSFS.

Quoted 8 lines
tests/block/035 new file +8 -0
+
+	# Disallow global fail_make_request feature
+	echo "${PROBABILITY}" > "${DEBUGFS_MNT}"/probability
+	echo "${TIMES}" > "${DEBUGFS_MNT}"/times
+	echo "${VERBOSE}" > "${DEBUGFS_MNT}"/verbose
+}
+
+test_device() {

tests/block/035 line 79

Quoted 10 lines
tests/block/035 new file +10 -0
+	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.

tests/block/035 line 80

Quoted 1 line
tests/block/035 new file +1 -0
+	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

tests/block/035 line 93

Quoted 13 lines
tests/block/035 new file +13 -0
+		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.

Quoted 12 lines
tests/block/035.out new file +3 -0
new file mode 100644
index 0000000..0f97f6b
@@ -0,0 +1,3 @@
+Running block/035
+Return EIO for BLKRRPART on bad disk
+Test complete
Signature · 1 line
2.44.0.478.gd926399ef9-goog
author
#6 lore raw

PATCH 1/1 block/035: test return EIO from BLKRRPART

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/[email protected]/ open here
Signed-off-by Saranya Muruganandam <[email protected]>
---
 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
tests/block/035 new file +86 -0
new file mode 100755
index 0000000..e15f115
@@ -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"
+}
tests/block/035.out new file +3 -0
new file mode 100644
index 0000000..0f97f6b
@@ -0,0 +1,3 @@
+Running block/035
+Return EIO for BLKRRPART on bad disk
+Test complete
Signature · 1 line
2.44.0.478.gd926399ef9-goog
#7 lore raw
Quoted 8 lines · 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/[email protected]/ open here
Signed-off-by Saranya Muruganandam <[email protected]>

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

Quoted 78 lines
---
 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
tests/block/035 new file +65 -0
new file mode 100755
index 0000000..e15f115
@@ -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
+}
+

tests/block/035 line 75

Quoted 10 lines
tests/block/035 new file +10 -0
+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.

tests/block/035 line 78

Quoted 3 lines
tests/block/035 new file +3 -0
+	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.

Quoted 20 lines
tests/block/035 new file +8 -0
+		echo "Return EIO for BLKRRPART on bad disk"
+	fi
+
+	# Restore TEST_DEV device to original state
+	restore_fail_make_request
+
+	echo "Test complete"
+}
tests/block/035.out new file +3 -0
new file mode 100644
index 0000000..0f97f6b
@@ -0,0 +1,3 @@
+Running block/035
+Return EIO for BLKRRPART on bad disk
+Test complete
Signature · 1 line
2.44.0.478.gd926399ef9-goog
author
#8 lore raw

PATCH 1/1 block/035: test return EIO from BLKRRPART

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/[email protected]/ open here
Signed-off-by Saranya Muruganandam <[email protected]>
---
 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
tests/block/035 new file +86 -0
new file mode 100755
index 0000000..0ba6292
@@ -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"
+}
tests/block/035.out new file +4 -0
new file mode 100644
index 0000000..125f4b8
@@ -0,0 +1,4 @@
+Running block/035
+blockdev: ioctl error on BLKRRPART: Input/output error
+Return EIO for BLKRRPART on bad disk
+Test complete
Signature · 1 line
2.44.0.683.g7961c838ac-goog
#9 lore raw
Quoted 8 lines · 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/[email protected]/ open here
Signed-off-by Saranya Muruganandam <[email protected]>

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.

Quoted 79 lines
---
 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
tests/block/035 new file +66 -0
new file mode 100755
index 0000000..0ba6292
@@ -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() {

tests/block/035 line 76

Quoted 10 lines
tests/block/035 new file +10 -0
+	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.

Quoted 23 lines
tests/block/035 new file +10 -0
+		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"
+}
tests/block/035.out new file +4 -0
new file mode 100644
index 0000000..125f4b8
@@ -0,0 +1,4 @@
+Running block/035
+blockdev: ioctl error on BLKRRPART: Input/output error
+Return EIO for BLKRRPART on bad disk
+Test complete
Signature · 1 line
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!

tests/block/035 +4 -3
index 0ba6292..d4e67fd 100755
@@ -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
tests/block/035.out +0 -1
index 125f4b8..0f97f6b 100644
@@ -1,4 +1,3 @@
 Running block/035
-blockdev: ioctl error on BLKRRPART: Input/output error
 Return EIO for BLKRRPART on bad disk
 Test complete
author
#10 lore raw

PATCH v2 1/1 block/035: test return EIO from BLKRRPART

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/[email protected]/ open here
Signed-off-by Saranya Muruganandam <[email protected]>
---
 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
tests/block/035 new file +87 -0
new file mode 100755
index 0000000..d4e67fd
@@ -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"
+}
tests/block/035.out new file +3 -0
new file mode 100644
index 0000000..0f97f6b
@@ -0,0 +1,3 @@
+Running block/035
+Return EIO for BLKRRPART on bad disk
+Test complete
Signature · 1 line
2.44.0.683.g7961c838ac-goog
#11 lore raw
Quoted 8 lines · 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/[email protected]/ open here
Signed-off-by Saranya Muruganandam <[email protected]>

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