Re: [PATCH] btrfs-progs: test-misc: search the backup slot to use at runtime

[Qu Wenruo <wqu@suse.com>]

On 2021/10/12 18:12, David Sterba wrote:
> On Tue, Oct 12, 2021 at 04:37:12PM +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>
> 
> Thanks, that's the perfect solution.
> 
>>   .../038-backup-root-corruption/test.sh        | 45 ++++++++++++-------
>>   1 file changed, 28 insertions(+), 17 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..315eac9a2904 100755
>> --- a/tests/misc-tests/038-backup-root-corruption/test.sh
>> +++ b/tests/misc-tests/038-backup-root-corruption/test.sh
>> @@ -17,24 +17,34 @@ 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
>>   
>> +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 | grep -A9 "backup $slot_num:" >> "$RESULTS"
> 
> Please don't use the $RESULTS in tests, it's an implementation detail
> and there should always be helpers hiding, in this case it's run_check.

In this particular case, won't run_check dump all the output into the log?

As here we only want the grep result to be output, not the full dump_super.

> 
>> +
>>   run_check "$INTERNAL_BIN/btrfs-corrupt-block" -m $main_root_ptr -f generation "$TEST_DEV"
>                                                      ^^^^^^^^^^^^^^
> 
> Please always quote variables (unless there's a reason not to like for
> $SUDO_HELPER).

Forgot it again, but it should be safe as $main_root_ptr should just be 
a u64 number.

But yeah, I should quote it...

Thanks,
Qu

> 
>>   
>>   # Should fail because the root is corrupted
>> @@ -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"
>> -- 
>> 2.33.0
>