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@xxxxxxxxxx/ > Signed-off-by: Saranya Muruganandam <saranyamohan@xxxxxxxxxx> 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 >
