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

[Nikolay Borisov <nborisov@suse.com>]

On  5.12.2017 11:33, Qu Wenruo wrote:
> 
> 
> 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.

Then the test suite is not very consistent, since I copied this loopback
handling from some other test.

> 
>> +
>> +# 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
I'm reluctant to change this function any more.  It has comments on all
logical steps and is self-contained and I'd rather keep it that way.

> 
>> +
>> +	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.

Yeah, tell me about it, super recovery actually has 5 return values:

7985fe64e0e2 ("Btrfs-progs: add super-recover to recover bad supers")

    There will be five kinds of return values:

    0: all supers are valid, no need to recover
    1: usage or syntax error
    2: recover all bad superblocks successfully
    3: fail to recover bad superblocks
    4: abort to recover bad superblocks


> 
> 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
>>
>