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

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

[Qu Wenruo]

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>
---
 .../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"
+
 run_check "$INTERNAL_BIN/btrfs-corrupt-block" -m $main_root_ptr -f generation "$TEST_DEV"
 
 # 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

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

[David Sterba]

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.

> +
>  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).

>  
>  # 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

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

[Qu Wenruo]

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
> 

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

[David Sterba]

On Tue, Oct 12, 2021 at 06:19:44PM +0800, Qu Wenruo wrote:
> >> +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.

Sorry, I was not specific, what I meant is:

"dump_super | run_check grep ..."

so only the result of grep ends up in the log.

> >>   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...

I know in this case it's clear there's a number, the point is
consistency of the whole testsuite, so one does not have to think if
this unquoted variable is ok or not. The initialization may happen in
other context or lines away or the variable name could be misleading.

IIRC I've seen some commands fail and instead of a single word in the
output there was the error message. Quoting makes it fail at least
somehow safely instead of letting the variable words be interpreted as
individual parameters.

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

[Qu Wenruo]

On 2021/10/12 18:34, David Sterba wrote:
> On Tue, Oct 12, 2021 at 06:19:44PM +0800, Qu Wenruo wrote:
>>>> +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.
>
> Sorry, I was not specific, what I meant is:
>
> "dump_super | run_check grep ..."
>
> so only the result of grep ends up in the log.

Oh, that's a super awesome trick.

That's exactly what I want.

Learnt a new trick!

Thanks,
Qu
>
>>>>    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...
>
> I know in this case it's clear there's a number, the point is
> consistency of the whole testsuite, so one does not have to think if
> this unquoted variable is ok or not. The initialization may happen in
> other context or lines away or the variable name could be misleading.
>
> IIRC I've seen some commands fail and instead of a single word in the
> output there was the error message. Quoting makes it fail at least
> somehow safely instead of letting the variable words be interpreted as
> individual parameters.
>