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

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, May 22, 2025 at 11:08:14AM +1000, Dave Chinner wrote:
> 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()....

Or my preferred solution: a special purpose wrapper with a different
name that takes only a bdev path and has a comment that says it requires
a bdev path.

--D

> 
> -Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com
>