Message ID | 20211013011233.9254-1-wqu@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] btrfs-progs: test-misc: search the backup slot to use at runtime | expand |
On Wed, Oct 13, 2021 at 09:12:33AM +0800, Qu Wenruo wrote: > Test case misc/038 uses hardcoded backup slot number, this means if we > change how many transactions we commit during mkfs, it will immediately > break the tests. > > Such hardcoded tests will be a big pain for later btrfs-progs updates. > > Update it with runtime backup slot search. > > Such search is done by using current filesystem generation as a search > target and grab the slot number. > > By this, no matter how many transactions we commit during mkfs, the test > case should be able to handle it. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > Changelog: > v2: > - Use run_check() instead of manually redirect output to "$RESULT" > - Quote "$main_root_ptr" > --- > .../038-backup-root-corruption/test.sh | 47 ++++++++++++------- > 1 file changed, 29 insertions(+), 18 deletions(-) > > diff --git a/tests/misc-tests/038-backup-root-corruption/test.sh b/tests/misc-tests/038-backup-root-corruption/test.sh > index b6c3671f2c3a..bf41f1e0952b 100755 > --- a/tests/misc-tests/038-backup-root-corruption/test.sh > +++ b/tests/misc-tests/038-backup-root-corruption/test.sh > @@ -17,25 +17,35 @@ run_check $SUDO_HELPER touch "$TEST_MNT/file" > run_check_umount_test_dev > > dump_super() { > - run_check_stdout $SUDO_HELPER "$TOP/btrfs" inspect-internal dump-super -f "$TEST_DEV" > + # In this test, we will dump super block multiple times, while the > + # existing run_check*() helpers will always dump all the output into > + # the log, flooding the log and hide real important info. > + # Thus here we call "btrfs" directly. > + $SUDO_HELPER "$TOP/btrfs" inspect-internal dump-super -f "$TEST_DEV" > } > > -# Ensure currently active backup slot is the expected one (slot 3) > -backup2_root_ptr=$(dump_super | grep -A1 "backup 2" | grep backup_tree_root | awk '{print $2}') > - > main_root_ptr=$(dump_super | awk '/^root\t/{print $2}') > +# Grab current fs generation, and it will be used to determine which backup > +# slot to use > +cur_gen=$(dump_super | grep ^generation | awk '{print $2}') > +backup_gen=$(($cur_gen - 1)) > + > +# Grab the slot which matches @backup_gen > +found=$(dump_super | grep backup_tree_root | grep -n "gen: $backup_gen") > > -if [ "$backup2_root_ptr" -ne "$main_root_ptr" ]; then > - _log "Backup slot 2 not in use, trying slot 3" > - # Or use the next slot in case of free-space-tree > - backup3_root_ptr=$(dump_super | grep -A1 "backup 3" | grep backup_tree_root | awk '{print $2}') > - if [ "$backup3_root_ptr" -ne "$main_root_ptr" ]; then > - _fail "Neither backup slot 2 nor slot 3 are in use" > - fi > - _log "Backup slot 3 in use" > +if [ -z "$found" ]; then > + _fail "Unable to find a backup slot with generation $backup_gen" > fi > > -run_check "$INTERNAL_BIN/btrfs-corrupt-block" -m $main_root_ptr -f generation "$TEST_DEV" > +slot_num=$(echo $found | cut -f1 -d:) > +# To follow the dump-super output, where backup slot starts at 0. > +slot_num=$(($slot_num - 1)) What happens if we're on $slot_num == 0? Seems like this would mess up, right? Thanks, Josef
On 2021/10/14 23:36, Josef Bacik wrote: > On Wed, Oct 13, 2021 at 09:12:33AM +0800, Qu Wenruo wrote: >> Test case misc/038 uses hardcoded backup slot number, this means if we >> change how many transactions we commit during mkfs, it will immediately >> break the tests. >> >> Such hardcoded tests will be a big pain for later btrfs-progs updates. >> >> Update it with runtime backup slot search. >> >> Such search is done by using current filesystem generation as a search >> target and grab the slot number. >> >> By this, no matter how many transactions we commit during mkfs, the test >> case should be able to handle it. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> Changelog: >> v2: >> - Use run_check() instead of manually redirect output to "$RESULT" >> - Quote "$main_root_ptr" >> --- >> .../038-backup-root-corruption/test.sh | 47 ++++++++++++------- >> 1 file changed, 29 insertions(+), 18 deletions(-) >> >> diff --git a/tests/misc-tests/038-backup-root-corruption/test.sh b/tests/misc-tests/038-backup-root-corruption/test.sh >> index b6c3671f2c3a..bf41f1e0952b 100755 >> --- a/tests/misc-tests/038-backup-root-corruption/test.sh >> +++ b/tests/misc-tests/038-backup-root-corruption/test.sh >> @@ -17,25 +17,35 @@ run_check $SUDO_HELPER touch "$TEST_MNT/file" >> run_check_umount_test_dev >> >> dump_super() { >> - run_check_stdout $SUDO_HELPER "$TOP/btrfs" inspect-internal dump-super -f "$TEST_DEV" >> + # In this test, we will dump super block multiple times, while the >> + # existing run_check*() helpers will always dump all the output into >> + # the log, flooding the log and hide real important info. >> + # Thus here we call "btrfs" directly. >> + $SUDO_HELPER "$TOP/btrfs" inspect-internal dump-super -f "$TEST_DEV" >> } >> >> -# Ensure currently active backup slot is the expected one (slot 3) >> -backup2_root_ptr=$(dump_super | grep -A1 "backup 2" | grep backup_tree_root | awk '{print $2}') >> - >> main_root_ptr=$(dump_super | awk '/^root\t/{print $2}') >> +# Grab current fs generation, and it will be used to determine which backup >> +# slot to use >> +cur_gen=$(dump_super | grep ^generation | awk '{print $2}') >> +backup_gen=$(($cur_gen - 1)) >> + >> +# Grab the slot which matches @backup_gen >> +found=$(dump_super | grep backup_tree_root | grep -n "gen: $backup_gen") >> >> -if [ "$backup2_root_ptr" -ne "$main_root_ptr" ]; then >> - _log "Backup slot 2 not in use, trying slot 3" >> - # Or use the next slot in case of free-space-tree >> - backup3_root_ptr=$(dump_super | grep -A1 "backup 3" | grep backup_tree_root | awk '{print $2}') >> - if [ "$backup3_root_ptr" -ne "$main_root_ptr" ]; then >> - _fail "Neither backup slot 2 nor slot 3 are in use" >> - fi >> - _log "Backup slot 3 in use" >> +if [ -z "$found" ]; then >> + _fail "Unable to find a backup slot with generation $backup_gen" >> fi >> >> -run_check "$INTERNAL_BIN/btrfs-corrupt-block" -m $main_root_ptr -f generation "$TEST_DEV" >> +slot_num=$(echo $found | cut -f1 -d:) >> +# To follow the dump-super output, where backup slot starts at 0. >> +slot_num=$(($slot_num - 1)) > > What happens if we're on $slot_num == 0? Seems like this would mess up, right? > Thanks, Note that, the $found is from "grep -n" which starts its line number at 1. Thus $slot_num will always be >= 1, and nothing will be wrong. Thanks, Qu > > Josef >
On Fri, Oct 15, 2021 at 06:36:36AM +0800, Qu Wenruo wrote: > > > On 2021/10/14 23:36, Josef Bacik wrote: > > On Wed, Oct 13, 2021 at 09:12:33AM +0800, Qu Wenruo wrote: > > > Test case misc/038 uses hardcoded backup slot number, this means if we > > > change how many transactions we commit during mkfs, it will immediately > > > break the tests. > > > > > > Such hardcoded tests will be a big pain for later btrfs-progs updates. > > > > > > Update it with runtime backup slot search. > > > > > > Such search is done by using current filesystem generation as a search > > > target and grab the slot number. > > > > > > By this, no matter how many transactions we commit during mkfs, the test > > > case should be able to handle it. > > > > > > Signed-off-by: Qu Wenruo <wqu@suse.com> > > > --- > > > Changelog: > > > v2: > > > - Use run_check() instead of manually redirect output to "$RESULT" > > > - Quote "$main_root_ptr" > > > --- > > > .../038-backup-root-corruption/test.sh | 47 ++++++++++++------- > > > 1 file changed, 29 insertions(+), 18 deletions(-) > > > > > > diff --git a/tests/misc-tests/038-backup-root-corruption/test.sh b/tests/misc-tests/038-backup-root-corruption/test.sh > > > index b6c3671f2c3a..bf41f1e0952b 100755 > > > --- a/tests/misc-tests/038-backup-root-corruption/test.sh > > > +++ b/tests/misc-tests/038-backup-root-corruption/test.sh > > > @@ -17,25 +17,35 @@ run_check $SUDO_HELPER touch "$TEST_MNT/file" > > > run_check_umount_test_dev > > > > > > dump_super() { > > > - run_check_stdout $SUDO_HELPER "$TOP/btrfs" inspect-internal dump-super -f "$TEST_DEV" > > > + # In this test, we will dump super block multiple times, while the > > > + # existing run_check*() helpers will always dump all the output into > > > + # the log, flooding the log and hide real important info. > > > + # Thus here we call "btrfs" directly. > > > + $SUDO_HELPER "$TOP/btrfs" inspect-internal dump-super -f "$TEST_DEV" > > > } > > > > > > -# Ensure currently active backup slot is the expected one (slot 3) > > > -backup2_root_ptr=$(dump_super | grep -A1 "backup 2" | grep backup_tree_root | awk '{print $2}') > > > - > > > main_root_ptr=$(dump_super | awk '/^root\t/{print $2}') > > > +# Grab current fs generation, and it will be used to determine which backup > > > +# slot to use > > > +cur_gen=$(dump_super | grep ^generation | awk '{print $2}') > > > +backup_gen=$(($cur_gen - 1)) > > > + > > > +# Grab the slot which matches @backup_gen > > > +found=$(dump_super | grep backup_tree_root | grep -n "gen: $backup_gen") > > > > > > -if [ "$backup2_root_ptr" -ne "$main_root_ptr" ]; then > > > - _log "Backup slot 2 not in use, trying slot 3" > > > - # Or use the next slot in case of free-space-tree > > > - backup3_root_ptr=$(dump_super | grep -A1 "backup 3" | grep backup_tree_root | awk '{print $2}') > > > - if [ "$backup3_root_ptr" -ne "$main_root_ptr" ]; then > > > - _fail "Neither backup slot 2 nor slot 3 are in use" > > > - fi > > > - _log "Backup slot 3 in use" > > > +if [ -z "$found" ]; then > > > + _fail "Unable to find a backup slot with generation $backup_gen" > > > fi > > > > > > -run_check "$INTERNAL_BIN/btrfs-corrupt-block" -m $main_root_ptr -f generation "$TEST_DEV" > > > +slot_num=$(echo $found | cut -f1 -d:) > > > +# To follow the dump-super output, where backup slot starts at 0. > > > +slot_num=$(($slot_num - 1)) > > > > What happens if we're on $slot_num == 0? Seems like this would mess up, right? > > Thanks, > > Note that, the $found is from "grep -n" which starts its line number at 1. > > Thus $slot_num will always be >= 1, and nothing will be wrong. > I missed the -n part, sorry about that, you can add Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
diff --git a/tests/misc-tests/038-backup-root-corruption/test.sh b/tests/misc-tests/038-backup-root-corruption/test.sh index b6c3671f2c3a..bf41f1e0952b 100755 --- a/tests/misc-tests/038-backup-root-corruption/test.sh +++ b/tests/misc-tests/038-backup-root-corruption/test.sh @@ -17,25 +17,35 @@ run_check $SUDO_HELPER touch "$TEST_MNT/file" run_check_umount_test_dev dump_super() { - run_check_stdout $SUDO_HELPER "$TOP/btrfs" inspect-internal dump-super -f "$TEST_DEV" + # In this test, we will dump super block multiple times, while the + # existing run_check*() helpers will always dump all the output into + # the log, flooding the log and hide real important info. + # Thus here we call "btrfs" directly. + $SUDO_HELPER "$TOP/btrfs" inspect-internal dump-super -f "$TEST_DEV" } -# Ensure currently active backup slot is the expected one (slot 3) -backup2_root_ptr=$(dump_super | grep -A1 "backup 2" | grep backup_tree_root | awk '{print $2}') - main_root_ptr=$(dump_super | awk '/^root\t/{print $2}') +# Grab current fs generation, and it will be used to determine which backup +# slot to use +cur_gen=$(dump_super | grep ^generation | awk '{print $2}') +backup_gen=$(($cur_gen - 1)) + +# Grab the slot which matches @backup_gen +found=$(dump_super | grep backup_tree_root | grep -n "gen: $backup_gen") -if [ "$backup2_root_ptr" -ne "$main_root_ptr" ]; then - _log "Backup slot 2 not in use, trying slot 3" - # Or use the next slot in case of free-space-tree - backup3_root_ptr=$(dump_super | grep -A1 "backup 3" | grep backup_tree_root | awk '{print $2}') - if [ "$backup3_root_ptr" -ne "$main_root_ptr" ]; then - _fail "Neither backup slot 2 nor slot 3 are in use" - fi - _log "Backup slot 3 in use" +if [ -z "$found" ]; then + _fail "Unable to find a backup slot with generation $backup_gen" fi -run_check "$INTERNAL_BIN/btrfs-corrupt-block" -m $main_root_ptr -f generation "$TEST_DEV" +slot_num=$(echo $found | cut -f1 -d:) +# To follow the dump-super output, where backup slot starts at 0. +slot_num=$(($slot_num - 1)) + +# Save the backup slot info into the log +_log "Backup slot $slot_num will be utilized" +dump_super | run_check grep -A9 "backup $slot_num:" + +run_check "$INTERNAL_BIN/btrfs-corrupt-block" -m "$main_root_ptr" -f generation "$TEST_DEV" # Should fail because the root is corrupted run_mustfail "Unexpected successful mount" \ @@ -45,9 +55,10 @@ run_mustfail "Unexpected successful mount" \ run_check_mount_test_dev -o usebackuproot run_check_umount_test_dev -# Since we've used backup 1 as the usable root, then backup 2 should have been -# overwritten -main_root_ptr=$(dump_super | grep root | head -n1 | awk '{print $2}') -backup2_new_root_ptr=$(dump_super | grep -A1 "backup 2" | grep backup_tree_root | awk '{print $2}') +main_root_ptr=$(dump_super | awk '/^root\t/{print $2}') + +# The next slot should be overwritten +slot_num=$(( ($slot_num + 1) % 4 )) +backup_new_root_ptr=$(dump_super | grep -A1 "backup $slot_num" | grep backup_tree_root | awk '{print $2}') -[ "$backup2_root_ptr" -ne "$backup2_new_root_ptr" ] || _fail "Backup 2 not overwritten" +[ "$main_root_ptr" -ne "$backup_new_root_ptr" ] || _fail "Backup 2 not overwritten"
Test case misc/038 uses hardcoded backup slot number, this means if we change how many transactions we commit during mkfs, it will immediately break the tests. Such hardcoded tests will be a big pain for later btrfs-progs updates. Update it with runtime backup slot search. Such search is done by using current filesystem generation as a search target and grab the slot number. By this, no matter how many transactions we commit during mkfs, the test case should be able to handle it. Signed-off-by: Qu Wenruo <wqu@suse.com> --- Changelog: v2: - Use run_check() instead of manually redirect output to "$RESULT" - Quote "$main_root_ptr" --- .../038-backup-root-corruption/test.sh | 47 ++++++++++++------- 1 file changed, 29 insertions(+), 18 deletions(-)