On 2017年12月05日 18:04, Nikolay Borisov wrote:
>
>
> 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@xxxxxxxx>
>>
>> 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.
The same feeling when I am pointed that something can be replaced by
wrappers in fstests.
Some of them can be cleaned up later.
>
>>
>>> +
>>> +# 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
Since we all agree that the return value is a messy,
maybe we could just simplify it to 0 (all valid or successful recover)
and 1 (the rest)?
Thanks,
Qu
>
>
>>
>> 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
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Attachment:
signature.asc
Description: OpenPGP digital signature
