On Tue, Jan 02, 2024 at 10:10:29 AM -0800, Darrick J. Wong wrote:
> On Tue, Jan 02, 2024 at 02:13:48PM +0530, Chandan Babu R wrote:
>> xfs/253 requires the metadump to be obfuscated. However _xfs_metadump() would
>> append the '-o' option causing the metadump to be unobfuscated.
>>
>> This commit fixes the bug by modifying _xfs_metadump() to no longer append any
>> metadump options. The direct/indirect callers of this function now pass the
>> required options explicitly.
>>
>> Signed-off-by: Chandan Babu R <chandanbabu@xxxxxxxxxx>
>> ---
>> common/populate | 2 +-
>> common/xfs | 7 +++----
>> tests/xfs/291 | 2 +-
>> tests/xfs/336 | 2 +-
>> tests/xfs/432 | 2 +-
>> tests/xfs/503 | 2 +-
>
> Hmm. Shouldn't "-a -o" be lowered into xfs/168? It generates a
> metadump if a post-shrinkfs repair fails and someone needs to debug the
> resulting breakage.
Sorry, I can't find xfs_metadump being directly/indirectly executed by
xfs/168. The test invokes _scratch_xfs_repair() and that in turn executes
xfs_repair on the scratch filesystem. On failure, _fail() is invoked which
causes the script to exit after printing some messages on the terminal.
However, _check_xfs_filesystem() can generate a metadump and this patch
appends both '-a' and '-o' options to all instances of _xfs_metadump() command
lines inside _check_xfs_filesystem().
>
> I think that in general, tests should be using -a (copy entire fs
> blocks) and -o (do not obfuscate) to reduce the amount of processing
> that fstests has to do; and to make it easy for developers to examine
> the fs metadata.
>
> (Also given the number of bugs that we keep finding in the "zero unused
> parts of blocks" code, I almost always use -a to reduce the number of
> pieces that can fail.)
>
> Obviously that does not apply to tests that are exercising metadump
> itself.
>
> --D
>
>> 6 files changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/common/populate b/common/populate
>> index 3d233073..cfbfd88a 100644
>> --- a/common/populate
>> +++ b/common/populate
>> @@ -55,7 +55,7 @@ __populate_fail() {
>> case "$FSTYP" in
>> xfs)
>> _scratch_unmount
>> - _scratch_xfs_metadump "$metadump"
>> + _scratch_xfs_metadump "$metadump" -a -o
>> ;;
>> ext4)
>> _scratch_unmount
>> diff --git a/common/xfs b/common/xfs
>> index f53b33fc..38094828 100644
>> --- a/common/xfs
>> +++ b/common/xfs
>> @@ -667,7 +667,6 @@ _xfs_metadump() {
>> local compressopt="$4"
>> shift; shift; shift; shift
>> local options="$@"
>> - test -z "$options" && options="-a -o"
>>
>> if [ "$logdev" != "none" ]; then
>> options="$options -l $logdev"
>> @@ -855,7 +854,7 @@ _check_xfs_filesystem()
>> if [ "$ok" -ne 1 ] && [ "$DUMP_CORRUPT_FS" = "1" ]; then
>> local flatdev="$(basename "$device")"
>> _xfs_metadump "$seqres.$flatdev.check.md" "$device" "$logdev" \
>> - compress >> $seqres.full
>> + compress -a -o >> $seqres.full
>> fi
>>
>> # Optionally test the index rebuilding behavior.
>> @@ -888,7 +887,7 @@ _check_xfs_filesystem()
>> if [ "$rebuild_ok" -ne 1 ] && [ "$DUMP_CORRUPT_FS" = "1" ]; then
>> local flatdev="$(basename "$device")"
>> _xfs_metadump "$seqres.$flatdev.rebuild.md" "$device" \
>> - "$logdev" compress >> $seqres.full
>> + "$logdev" compress -a -o >> $seqres.full
>> fi
>> fi
>>
>> @@ -972,7 +971,7 @@ _check_xfs_filesystem()
>> if [ "$orebuild_ok" -ne 1 ] && [ "$DUMP_CORRUPT_FS" = "1" ]; then
>> local flatdev="$(basename "$device")"
>> _xfs_metadump "$seqres.$flatdev.orebuild.md" "$device" \
>> - "$logdev" compress >> $seqres.full
>> + "$logdev" compress -a -o >> $seqres.full
>> fi
>> fi
>>
>> diff --git a/tests/xfs/291 b/tests/xfs/291
>> index 600dcb2e..54448497 100755
>> --- a/tests/xfs/291
>> +++ b/tests/xfs/291
>> @@ -92,7 +92,7 @@ _scratch_xfs_check >> $seqres.full 2>&1 || _fail "xfs_check failed"
>>
>> # Yes they can! Now...
>> # Can xfs_metadump cope with this monster?
>> -_scratch_xfs_metadump $tmp.metadump || _fail "xfs_metadump failed"
>> +_scratch_xfs_metadump $tmp.metadump -a -o || _fail "xfs_metadump failed"
>> SCRATCH_DEV=$tmp.img _scratch_xfs_mdrestore $tmp.metadump || _fail "xfs_mdrestore failed"
>> SCRATCH_DEV=$tmp.img _scratch_xfs_repair -f &>> $seqres.full || \
>> _fail "xfs_repair of metadump failed"
>> diff --git a/tests/xfs/336 b/tests/xfs/336
>> index d7a074d9..43b3790c 100755
>> --- a/tests/xfs/336
>> +++ b/tests/xfs/336
>> @@ -62,7 +62,7 @@ _scratch_cycle_mount
>>
>> echo "Create metadump file"
>> _scratch_unmount
>> -_scratch_xfs_metadump $metadump_file
>> +_scratch_xfs_metadump $metadump_file -a
>>
>> # Now restore the obfuscated one back and take a look around
>> echo "Restore metadump"
>> diff --git a/tests/xfs/432 b/tests/xfs/432
>> index 66315b03..dae68fb2 100755
>> --- a/tests/xfs/432
>> +++ b/tests/xfs/432
>> @@ -86,7 +86,7 @@ echo "qualifying extent: $extlen blocks" >> $seqres.full
>> test -n "$extlen" || _notrun "could not create dir extent > 1000 blocks"
>>
>> echo "Try to metadump"
>> -_scratch_xfs_metadump $metadump_file -w
>> +_scratch_xfs_metadump $metadump_file -a -o -w
>> SCRATCH_DEV=$metadump_img _scratch_xfs_mdrestore $metadump_file
>>
>> echo "Check restored metadump image"
>> diff --git a/tests/xfs/503 b/tests/xfs/503
>> index f5710ece..8805632d 100755
>> --- a/tests/xfs/503
>> +++ b/tests/xfs/503
>> @@ -46,7 +46,7 @@ metadump_file_ag=${metadump_file}.ag
>> copy_file=$testdir/copy.img
>>
>> echo metadump
>> -_scratch_xfs_metadump $metadump_file >> $seqres.full
>> +_scratch_xfs_metadump $metadump_file -a -o >> $seqres.full
>>
>> echo metadump a
>> _scratch_xfs_metadump $metadump_file_a -a >> $seqres.full
>> --
>> 2.43.0
>>
>>
--
Chandan
