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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> 




[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux