From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:41974 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753179AbdLEKEv (ORCPT ); Tue, 5 Dec 2017 05:04:51 -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> From: Nikolay Borisov Message-ID: <2bff48d1-09fa-59ae-7f1f-f4a91b9bf143@suse.com> Date: Tue, 5 Dec 2017 12:04:49 +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 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. > >> + >> +# 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 >> >