From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:50942 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750855AbdLEL04 (ORCPT ); Tue, 5 Dec 2017 06:26:56 -0500 Subject: Re: [PATCH 6/7] btrfs-progs: Add test for super block recovery To: Qu Wenruo , linux-btrfs@vger.kernel.org References: <1512463189-24724-1-git-send-email-nborisov@suse.com> <1512463189-24724-7-git-send-email-nborisov@suse.com> <2bff48d1-09fa-59ae-7f1f-f4a91b9bf143@suse.com> From: Nikolay Borisov Message-ID: <666447ad-8bab-2a13-3304-7b477c7cdde6@suse.com> Date: Tue, 5 Dec 2017 13:26:53 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Sender: linux-btrfs-owner@vger.kernel.org List-ID: 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 >>> >>> 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. > > 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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >