Re: [PATCH 4/5] xfs: Add support for testing metadump v2

[Chandan Babu R <chandanbabu@kernel.org>]

On Tue, Jan 02, 2024 at 09:57:28 PM -0800, Darrick J. Wong wrote:
> On Tue, Jan 02, 2024 at 02:13:51PM +0530, Chandan Babu R wrote:
>> This commit adds the ability to test metadump v2 to existing metadump tests.
>> 
>> Signed-off-by: Chandan Babu R <chandanbabu@kernel.org>
>> ---
>>  tests/xfs/129     |  63 ++++++++++++---
>>  tests/xfs/129.out |   4 +-
>>  tests/xfs/234     |  63 ++++++++++++---
>>  tests/xfs/234.out |   4 +-
>>  tests/xfs/253     | 195 ++++++++++++++++++++++++++--------------------
>>  tests/xfs/291     |  25 +++++-
>>  tests/xfs/432     |  29 +++++--
>>  tests/xfs/432.out |   3 +-
>>  tests/xfs/503     |  94 +++++++++++++---------
>>  tests/xfs/503.out |  12 +--
>>  10 files changed, 326 insertions(+), 166 deletions(-)
>> 
>> diff --git a/tests/xfs/129 b/tests/xfs/129
>> index 6f2ef564..7226d57d 100755
>> --- a/tests/xfs/129
>> +++ b/tests/xfs/129
>> @@ -16,7 +16,11 @@ _cleanup()
>>  {
>>      cd /
>>      _scratch_unmount > /dev/null 2>&1
>> -    rm -rf $tmp.* $testdir $metadump_file $TEST_DIR/image
>> +    [[ -n $logdev && $logdev != "none" && $logdev != $SCRATCH_LOGDEV ]] && \
>> +	    _destroy_loop_device $logdev
>> +    [[ -n $datadev ]] && _destroy_loop_device $datadev
>> +    rm -rf $tmp.* $testdir $metadump_file $TEST_DIR/data-image \
>> +       $TEST_DIR/log-image
>>  }
>>  
>>  # Import common functions.
>> @@ -47,18 +51,57 @@ seq 1 2 $((nr_blks - 1)) | while read nr; do
>>  			$testdir/file2 $((nr * blksz)) $blksz >> $seqres.full
>>  done
>>  
>> -echo "Create metadump file"
>>  _scratch_unmount
>> -_scratch_xfs_metadump $metadump_file
>>  
>> -# Now restore the obfuscated one back and take a look around
>> -echo "Restore metadump"
>> -SCRATCH_DEV=$TEST_DIR/image _scratch_xfs_mdrestore $metadump_file
>> -SCRATCH_DEV=$TEST_DIR/image _scratch_mount
>> -SCRATCH_DEV=$TEST_DIR/image _scratch_unmount
>> +max_md_version=1
>> +_scratch_metadump_v2_supported && max_md_version=2
>>  
>> -echo "Check restored fs"
>> -_check_generic_filesystem $metadump_file
>> +echo "Create metadump file, restore it and check restored fs"
>> +for md_version in $(seq 1 $max_md_version); do
>> +	# Determine the version to be passed to metadump/mdrestore
>> +	version=""
>> +	if [[ $max_md_version == 2 ]]; then
>> +		version="-v $md_version"
>> +	fi
>> +
>> +	_scratch_xfs_metadump $metadump_file $version
>> +
>> +	# Now restore the obfuscated one back and take a look around
>> +
>> +	# Metadump v2 files can contain contents dumped from an external log
>> +	# device. Use a temporary file to hold the log device contents restored
>> +	# from such a metadump file.
>> +	slogdev=$TEST_DIR/log-image
>> +	if [[ -z $version || $version == "-v 1" || -z $SCRATCH_LOGDEV ]]; then
>> +		slogdev=""
>> +	fi
>> +
>> + 	SCRATCH_DEV=$TEST_DIR/data-image SCRATCH_LOGDEV=$slogdev \
>
>    ^ space before tab
>

Sorry, I will fix it.

>> +		   _scratch_xfs_mdrestore $metadump_file
>> +
>> +	datadev=$(_create_loop_device $TEST_DIR/data-image)
>> +
>> +	logdev=${SCRATCH_LOGDEV}
>> +	if [[ -s $TEST_DIR/log-image ]]; then
>> +		logdev=$(_create_loop_device $TEST_DIR/log-image)
>> +	fi
>> +
>> +	SCRATCH_DEV=$datadev SCRATCH_LOGDEV=$logdev _scratch_mount
>> +	SCRATCH_DEV=$datadev SCRATCH_LOGDEV=$logdev _scratch_unmount
>> +
>> +	[[ -z $logdev ]] && logdev=none
>> +	_check_xfs_filesystem $datadev $logdev none
>> +
>> +	if [[ -s $TEST_DIR/log-image ]]; then
>> +		_destroy_loop_device $logdev
>> +		logdev=""
>> +		rm -f $TEST_DIR/log-image
>> +	fi
>> +
>> +	_destroy_loop_device $datadev
>> +	datadev=""
>> +	rm -f $TEST_DIR/data-image
>> +done
>
> Given the major differences between v1 and v2, would the two metadump
> tests be easier to understand if this loop were unrolled?
>

Yes, you are right. Unrolling the loop will improve the readability of the
code. I will make the required changes.

> I like that it mdrestores to a separate device to preserve the original
> $SCRATCH_DEV metadata between metadump v1 and v2 testing.
>
>>  # success, all done
>>  status=0
>> diff --git a/tests/xfs/129.out b/tests/xfs/129.out
>> index da6f43fd..0f24c431 100644
>> --- a/tests/xfs/129.out
>> +++ b/tests/xfs/129.out
>> @@ -1,6 +1,4 @@
>>  QA output created by 129
>>  Create the original file blocks
>>  Reflink every other block
>> -Create metadump file
>> -Restore metadump
>> -Check restored fs
>> +Create metadump file, restore it and check restored fs
>> diff --git a/tests/xfs/234 b/tests/xfs/234
>> index 57d447c0..2f6b1f65 100755
>> --- a/tests/xfs/234
>> +++ b/tests/xfs/234
>> @@ -16,7 +16,11 @@ _cleanup()
>>  {
>>      cd /
>>      _scratch_unmount > /dev/null 2>&1
>> -    rm -rf $tmp.* $metadump_file $TEST_DIR/image
>> +    [[ -n $logdev && $logdev != "none" && $logdev != $SCRATCH_LOGDEV ]] && \
>> +	    _destroy_loop_device $logdev
>> +    [[ -n $datadev ]] && _destroy_loop_device $datadev
>> +    rm -rf $tmp.* $testdir $metadump_file $TEST_DIR/image \
>> +       $TEST_DIR/log-image
>>  }
>>  
>>  # Import common functions.
>> @@ -47,18 +51,57 @@ seq 1 2 $((nr_blks - 1)) | while read nr; do
>>  	$XFS_IO_PROG -c "fpunch $((nr * blksz)) $blksz" $testdir/file1 >> $seqres.full
>>  done
>>  
>> -echo "Create metadump file"
>>  _scratch_unmount
>> -_scratch_xfs_metadump $metadump_file
>>  
>> -# Now restore the obfuscated one back and take a look around
>> -echo "Restore metadump"
>> -SCRATCH_DEV=$TEST_DIR/image _scratch_xfs_mdrestore $metadump_file
>> -SCRATCH_DEV=$TEST_DIR/image _scratch_mount
>> -SCRATCH_DEV=$TEST_DIR/image _scratch_unmount
>> +max_md_version=1
>> +_scratch_metadump_v2_supported && max_md_version=2
>>  
>> -echo "Check restored fs"
>> -_check_generic_filesystem $metadump_file
>> +echo "Create metadump file, restore it and check restored fs"
>> +for md_version in $(seq 1 $max_md_version); do
>> +	# Determine the version to be passed to metadump/mdrestore
>> +	version=""
>> +	if [[ $max_md_version == 2 ]]; then
>> +		version="-v $md_version"
>> +	fi
>> +
>> +	_scratch_xfs_metadump $metadump_file $version
>> +
>> +	# Now restore the obfuscated one back and take a look around
>> +
>> +	# Metadump v2 files can contain contents dumped from an external log
>> +	# device. Use a temporary file to hold the log device contents restored
>> +	# from such a metadump file.
>> +	slogdev=$TEST_DIR/log-image
>> +	if [[ -z $version || $version == "-v 1" || -z $SCRATCH_LOGDEV ]]; then
>> +		slogdev=""
>> +	fi
>> +
>> +	SCRATCH_DEV=$TEST_DIR/data-image SCRATCH_LOGDEV=$slogdev \
>> +		   _scratch_xfs_mdrestore $metadump_file
>> +
>> +	datadev=$(_create_loop_device $TEST_DIR/data-image)
>> +
>> +	logdev=${SCRATCH_LOGDEV}
>> +	if [[ -s $TEST_DIR/log-image ]]; then
>> +		logdev=$(_create_loop_device $TEST_DIR/log-image)
>> +	fi
>> +
>> +	SCRATCH_DEV=$datadev SCRATCH_LOGDEV=$logdev _scratch_mount
>> +	SCRATCH_DEV=$datadev SCRATCH_LOGDEV=$logdev _scratch_unmount
>> +
>> +	[[ -z $logdev ]] && logdev=none
>> +	_check_xfs_filesystem $datadev $logdev none
>> +
>> +	if [[ -s $TEST_DIR/log-image ]]; then
>> +		_destroy_loop_device $logdev
>> +		logdev=""
>> +		rm -f $TEST_DIR/log-image
>> +	fi
>> +
>> +	_destroy_loop_device $datadev
>> +	datadev=""
>> +	rm -f $TEST_DIR/data-image
>> +done
>>  
>>  # success, all done
>>  status=0
>> diff --git a/tests/xfs/234.out b/tests/xfs/234.out
>> index 463d4660..fc2ddd77 100644
>> --- a/tests/xfs/234.out
>> +++ b/tests/xfs/234.out
>> @@ -1,6 +1,4 @@
>>  QA output created by 234
>>  Create the original file blocks
>>  Punch every other block
>> -Create metadump file
>> -Restore metadump
>> -Check restored fs
>> +Create metadump file, restore it and check restored fs
>> diff --git a/tests/xfs/253 b/tests/xfs/253
>> index ce902477..b69a1faf 100755
>> --- a/tests/xfs/253
>> +++ b/tests/xfs/253
>> @@ -52,114 +52,139 @@ function create_file() {
>>  echo "Disciplyne of silence is goed."
>>  
>>  _scratch_mkfs >/dev/null 2>&1
>> -_scratch_mount
>>  
>> -# Initialize and mount the scratch filesystem, then create a bunch
>> -# of files that exercise the original problem.
>> -#
>> -# The problem arose when a file name produced a hash that contained
>> -# either 0x00 (string terminator) or 0x27 ('/' character) in a
>> -# spot used to determine a character in an obfuscated name.  This
>> -# occurred in one of 5 spots at the end of the name, at position
>> -# (last-4), (last-3), (last-2), (last-1), or (last).
>
> I wonder, could you create the scratch fs and only then move into
> testing v1 and v2 metadump?  Rather than doing the create_file stuff
> twice?  Or do we actually end up with a different fs between the two
> iterations?
>

Yes, Creating the fs once should be sufficient. I will implement the changes
that have been suggested.

> The other two tests here look good enough for now...
>
> --D
>

-- 
Chandan