Re: [PATCH V2 5/5] xfs: Check correctness of metadump/mdrestore's ability to work with dirty log

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

On Tue, Jan 09, 2024 at 08:57:45 AM -0800, Darrick J. Wong wrote:
> On Tue, Jan 09, 2024 at 03:50:47PM +0530, Chandan Babu R wrote:
>> Add a new test to verify if metadump/mdrestore are able to dump and restore
>> the contents of a dirty log.
>> 
>> Signed-off-by: Chandan Babu R <chandanbabu@kernel.org>
>> ---
>>  tests/xfs/801     | 178 ++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/xfs/801.out |  14 ++++
>>  2 files changed, 192 insertions(+)
>>  create mode 100755 tests/xfs/801
>>  create mode 100644 tests/xfs/801.out
>> 
>> diff --git a/tests/xfs/801 b/tests/xfs/801
>> new file mode 100755
>> index 00000000..a7866ce7
>> --- /dev/null
>> +++ b/tests/xfs/801
>> @@ -0,0 +1,178 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2024 Oracle, Inc.  All Rights Reserved.
>> +#
>> +# FS QA Test 801
>> +#
>> +# Test metadump/mdrestore's ability to dump a dirty log and restore it
>> +# correctly.
>> +#
>> +. ./common/preamble
>> +_begin_fstest auto quick metadump log logprint punch
>> +
>> +# Override the default cleanup function.
>> +_cleanup()
>> +{
>> +	cd /
>> +	rm -r -f $tmp.*
>> +	_scratch_unmount > /dev/null 2>&1
>> +	[[ -n $logdev && $logdev != "none" && $logdev != $SCRATCH_LOGDEV ]] && \
>> +		_destroy_loop_device $logdev
>> +	[[ -n $datadev ]] && _destroy_loop_device $datadev
>> +	rm -r -f $metadump_file $TEST_DIR/data-image \
>> +	   $TEST_DIR/log-image
>> +}
>> +
>> +# Import common functions.
>> +. ./common/dmflakey
>> +. ./common/inject
>> +
>> +# real QA test starts here
>> +_supported_fs xfs
>> +_require_scratch
>> +_require_test
>> +_require_loop
>> +_require_xfs_debug
>> +_require_xfs_io_error_injection log_item_pin
>> +_require_dm_target flakey
>> +_require_xfs_io_command "pwrite"
>> +_require_test_program "punch-alternating"
>> +
>> +metadump_file=${TEST_DIR}/${seq}.md
>> +testfile=${SCRATCH_MNT}/testfile
>> +
>> +echo "Format filesystem on scratch device"
>> +_scratch_mkfs >> $seqres.full 2>&1
>> +
>> +max_md_version=1
>> +_scratch_metadump_v2_supported && max_md_version=2
>> +
>> +external_log=0
>> +if [[ $USE_EXTERNAL = yes && -n "$SCRATCH_LOGDEV" ]]; then
>> +	external_log=1
>> +fi
>> +
>> +if [[ $max_md_version == 1 && $external_log == 1 ]]; then
>> +	_notrun "metadump v1 does not support external log device"
>> +fi
>> +
>> +verify_metadump_v1()
>> +{
>> +	local version=""
>> +	if [[ $max_md_version == 2 ]]; then
>> +		version="-v 1"
>> +	fi
>> +
>> +	_scratch_xfs_metadump $metadump_file -a -o $version
>> +
>> +	SCRATCH_DEV=$TEST_DIR/data-image _scratch_xfs_mdrestore $metadump_file
>> +
>> +	datadev=$(_create_loop_device $TEST_DIR/data-image)
>> +
>> +	SCRATCH_DEV=$datadev _scratch_mount
>> +	SCRATCH_DEV=$datadev _check_scratch_fs
>> +	SCRATCH_DEV=$datadev _scratch_unmount
>> +
>> +	_destroy_loop_device $datadev
>> +	datadev=""
>> +	rm -f $TEST_DIR/data-image
>> +}
>> +
>> +verify_metadump_v2()
>> +{
>> +	local version="-v 2"
>> +
>> +	_scratch_xfs_metadump $metadump_file -a -o $version
>> +
>> +	# 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=""
>> +	if [[ -n $SCRATCH_LOGDEV ]]; then
>> +		slogdev=$TEST_DIR/log-image
>
> Why not create the loopdevs here?
>

The backing files (i.e. $TEST_DIR/data-image and $TEST_DIR/log-image) have not
yet been created. They are created by the invocation of mdrestore command
below.

>> +	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)
>
> if [[ -s $slogdev ]]; then
> 	logdev=$(_create_loop_device $slogdev)
> fi
>
> When would we have logdev == SCRATCH_LOGDEV at this point in the program?

logdev == SCRATCH_LOGDEV only when using an internal log. I think a much
cleaner way of initializing logdev would be

	logdev=""

Combining this with the change suggested by you earlier, the code now looks
like the following,

	verify_metadump_v2()
	{
		local version="-v 2"
	
		_scratch_xfs_metadump $metadump_file -a -o $version
	
		# 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=""
		if [[ -n $SCRATCH_LOGDEV ]]; then
			slogdev=$TEST_DIR/log-image
		fi
	
		SCRATCH_DEV=$TEST_DIR/data-image SCRATCH_LOGDEV=$slogdev \
			   _scratch_xfs_mdrestore $metadump_file
	
		datadev=$(_create_loop_device $TEST_DIR/data-image)
	
		logdev=""
		if [[ -s $slogdev ]]; then
			logdev=$(_create_loop_device $slogdev)
		fi
	
		SCRATCH_DEV=$datadev SCRATCH_LOGDEV=$logdev _scratch_mount
		SCRATCH_DEV=$datadev SCRATCH_LOGDEV=$logdev _check_scratch_fs
		SCRATCH_DEV=$datadev SCRATCH_LOGDEV=$logdev _scratch_unmount
	
		if [[ -s $logdev ]]; then
			_destroy_loop_device $logdev
			logdev=""
			rm -f $slogdev
		fi
	
		_destroy_loop_device $datadev
		datadev=""
		rm -f $TEST_DIR/data-image
	}

-- 
Chandan