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