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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx
>
