Re: [PATCH 6/7] btrfs-progs: Add test for super block recovery

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3530 bytes --]



On 2017年12月05日 16:39, Nikolay Borisov wrote:
> This functionality regressed some time ago and it was never caught. Seems no
> one complained of that, but to be sure add a regression test to prevent future 
> regressions.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

One nitpick for the patch sequence, normally we put fix before test
case, to avoid breaking bisect.

> ---
>  tests/fsck-tests/029-superblock-recovery/test.sh | 64 ++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>  create mode 100755 tests/fsck-tests/029-superblock-recovery/test.sh
> 
> diff --git a/tests/fsck-tests/029-superblock-recovery/test.sh b/tests/fsck-tests/029-superblock-recovery/test.sh
> new file mode 100755
> index 000000000000..beb78d6ccc22
> --- /dev/null
> +++ b/tests/fsck-tests/029-superblock-recovery/test.sh
> @@ -0,0 +1,64 @@
> +#!/bin/bash
> +# Test that any superblock is correctly detected
> +# and fixed by btrfs rescue
> +
> +source "$TOP/tests/common"
> +
> +check_prereq btrfs
> +check_prereq mkfs.btrfs
> +check_prereq btrfs-select-super
> +
> +setup_root_helper
> +
> +rm -f dev1
> +run_check truncate -s 260G dev1
> +loop=$(run_check_stdout $SUDO_HELPER losetup --find --show dev1)

We have function to do it already.
prepare_test_dev will use loopback device as fallback if $TEST_DEV is
not specified.
Tt can handle size well, and it also uses sparse file so no need to
worry about disk usage.

> +
> +# Create the test file system.
> +run_check $SUDO_HELPER "$TOP"/mkfs.btrfs -f "$loop"
> +
> +function check_corruption {
> +	local sb_offset=$1
> +	local source_sb=$2
> +
> +
> +	# First we ensure we can mount it successfully
> +	run_check $SUDO_HELPER mount $loop "$TEST_MNT"
> +	run_check $SUDO_HELPER umount "$TEST_MNT"
> +
> +	# Now corrupt 1k of the superblock at sb_offset
> +	run_check $SUDO_HELPER dd bs=1K count=1 seek=$(($sb_offset + 1)) if=/dev/zero of="$loop"
> +
> +	#if corrupting one of the sb copies, copy it over the initial superblock
> +	if [ ! -z $source_sb ]; then
> +		local shift_val=$((16 << $source_sb * 12 ))
> +		run_check $SUDO_HELPER dd bs=1K count=4 seek=64 skip=$shift_val if="$loop" of="$loop"
> +	fi

Personally speaking, corrupt 64K (1st super) then corrupt the desired
copy could make the function easier.
Although we need to split the check part from this function, resulting
something like:

corrupt_super 64k
corrupt_super 64m
check_super_recover

> +
> +	run_mustfail "Mounted fs with corrupted superblock" \
> +		$SUDO_HELPER mount $loop "$TEST_MNT"
> +
> +	# Now run btrfs rescue which should fix the superblock. It uses 2
> +	# to signal success of recovery use mayfail to ignore that retval
> +	# but still log the output of the command
> +	run_mayfail $SUDO_HELPER "$TOP"/btrfs rescue super-recover -yv "$loop"
> +	if [ $? != 2 ]; then
> +		_fail "couldn't rescue super"
> +	fi

It's understandable to have return value other than 0 to distinguish
health fs from repairable fs.
But at least let's also put this into man page.

Thanks,
Qu

> +
> +	run_check $SUDO_HELPER mount $loop "$TEST_MNT"
> +	run_check $SUDO_HELPER umount "$TEST_MNT"
> +}
> +
> +_log "Corrupting first superblock"
> +check_corruption 64
> +
> +_log "Corrupting second superblock"
> +check_corruption 65536 1
> +
> +_log "Corrupting third superblock"
> +check_corruption 268435456 2
> +
> +# Cleanup
> +run_check $SUDO_HELPER losetup -d "$loop"
> +rm -f dev1
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]