Re: [PATCH 4/4] xfs/432: fix metadump loop device blocksize problems

[Dave Chinner <david@fromorbit.com>]

On Wed, May 21, 2025 at 03:41:51PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Make sure the lba size of the loop devices created for the metadump
> tests actually match that of the real SCRATCH_ devices or else the tests
> will fail.
> 
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> ---
>  common/metadump |   12 ++++++++++--
>  common/rc       |    7 +++++++
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/common/metadump b/common/metadump
> index 61ba3cbb91647c..4ae03c605563fc 100644
> --- a/common/metadump
> +++ b/common/metadump
> @@ -76,6 +76,7 @@ _xfs_verify_metadump_v1()
>  
>  	# Create loopdev for data device so we can mount the fs
>  	METADUMP_DATA_LOOP_DEV=$(_create_loop_device $data_img)
> +	_force_loop_device_blocksize $METADUMP_DATA_LOOP_DEV $SCRATCH_DEV

That doesn't look right. You're passing the scratch device as a
block size parameter. 

>  
>  	# Mount fs, run an extra test, fsck, and unmount
>  	SCRATCH_DEV=$METADUMP_DATA_LOOP_DEV _scratch_mount
> @@ -123,12 +124,19 @@ _xfs_verify_metadump_v2()
>  
>  	# Create loopdev for data device so we can mount the fs
>  	METADUMP_DATA_LOOP_DEV=$(_create_loop_device $data_img)
> +	_force_loop_device_blocksize $METADUMP_DATA_LOOP_DEV $SCRATCH_DEV
>  
>  	# Create loopdev for log device if we recovered anything
> -	test -s "$log_img" && METADUMP_LOG_LOOP_DEV=$(_create_loop_device $log_img)
> +	if [ -s "$log_img" ]; then
> +		METADUMP_LOG_LOOP_DEV=$(_create_loop_device $log_img)
> +		_force_loop_device_blocksize $METADUMP_LOG_LOOP_DEV $SCRATCH_LOGDEV
> +	fi
>  
>  	# Create loopdev for rt device if we recovered anything
> -	test -s "$rt_img" && METADUMP_RT_LOOP_DEV=$(_create_loop_device $rt_img)
> +	if [ -s "$rt_img" ]; then
> +		METADUMP_RT_LOOP_DEV=$(_create_loop_device $rt_img)
> +		_force_loop_device_blocksize $METADUMP_RT_LOOP_DEV $SCRATCH_RTDEV
> +	fi
>  
>  	# Mount fs, run an extra test, fsck, and unmount
>  	SCRATCH_DEV=$METADUMP_DATA_LOOP_DEV SCRATCH_LOGDEV=$METADUMP_LOG_LOOP_DEV SCRATCH_RTDEV=$METADUMP_RT_LOOP_DEV _scratch_mount
> diff --git a/common/rc b/common/rc
> index 4e3917a298e072..9e27f7a4afba44 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -4527,6 +4527,8 @@ _create_loop_device()
>  }
>  
>  # Configure the loop device however needed to support the given block size.
> +# The first argument is the loop device; the second is either an integer block
> +# size, or a different block device whose blocksize we want to match.
>  _force_loop_device_blocksize()
>  {
>  	local loopdev="$1"
> @@ -4539,6 +4541,11 @@ _force_loop_device_blocksize()
>  		return 1
>  	fi
>  
> +	# second argument is really a bdev; copy its lba size
> +	if [ -b "$blksize" ]; then
> +		blksize="$(blockdev --getss "${blksize}")"
> +	fi

Oh, you're overloading the second parameter with different types -
that's pretty nasty.  It would be much cleaner to write a wrapper
function that extracts the block size from the device before calling
_force_loop_device_blocksize()....

-Dave.

-- 
Dave Chinner
david@fromorbit.com