On 2017年12月05日 19:26, Nikolay Borisov wrote:
>
>
> On 5.12.2017 13:12, Qu Wenruo wrote:
>>
>>
>> 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 <nbori...@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.
>>
>> 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)?
>
> I have no objection, but it's out of the scope of the current series.Yep, could be done as another patchset. Thanks, Qu > >> >> 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 majord...@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
signature.asc
Description: OpenPGP digital signature
