Re: [PATCH 11/23] common/xfs: find loop devices for non-blockdevs passed to _prepare_for_eio_shutdown

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

On Tue, Jan 21, 2025 at 03:37:17PM +1100, Dave Chinner wrote:
> On Thu, Jan 16, 2025 at 03:28:02PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > xfs/336 does this somewhat sketchy thing where it mdrestores into a
> > regular file, and then does this to validate the restored metadata:
> > 
> > SCRATCH_DEV=$TEST_DIR/image _scratch_mount
> 
> That's a canonical example of what is called "stepping on a
> landmine".

60% of fstests is written in bash, all of it is a friggin land mine
because bash totally lets us do variable substitution at any time, and
any time you make a change you have to exhaustively test the whole mess
to make sure nothing broke...

(Yeah, I hate bash)

> We validate that the SCRATCH_DEV is a block device at the start of
> check and each section it reads and runs (via common/config), and
> then make the assumption in all the infrastructure that SCRATCH_DEV
> always points to a valid block device.

We do?  Can you point me to the sentence in doc/ that says this
explicitly?  There's nothing I can find in the any docs and
_try_scratch_mount does not check SCRATCH_DEV is a bdev for XFS.
That needs to be documented.

> Now we have one new test that overwrites SCRATCH_DEV temporarily
> with a file and so we have to add checks all through the
> infrastructure to handle this one whacky test?
> 
> > Unfortunately, commit 1a49022fab9b4d causes the following regression:
> > 
> >  --- /tmp/fstests/tests/xfs/336.out      2024-11-12 16:17:36.733447713 -0800
> >  +++ /var/tmp/fstests/xfs/336.out.bad    2025-01-04 19:10:39.861871114 -0800
> >  @@ -5,4 +5,5 @@ Create big file
> >   Explode the rtrmapbt
> >   Create metadump file
> >   Restore metadump
> >  -Check restored fs
> >  +Usage: _set_fs_sysfs_attr <mounted_device> <attr> <content>
> >  +(see /var/tmp/fstests/xfs/336.full for details)
> > 
> > This is due to the fact that SCRATCH_DEV is temporarily reassigned to
> > the regular file.  That path is passed straight through _scratch_mount
> > to _xfs_prepare_for_eio_shutdown, but that helper _fails because the
> > "dev" argument isn't actually a path to a block device.
> 
> _scratch_mount assumes that SCRATCH_DEV points to a valid block
> device. xfs/336 is the problem here, not the code that assumes
> SCRATCH_DEV points to a block device....
> 
> Why are these hacks needed? Why can't _xfs_verify_metadumps()
> loopdev usage be extended to handle the new rt rmap code that this
> test is supposed to be exercising? 

Because _xfs_verify_metadumps came long after xfs/336.  336 itself was
merged long ago when I was naïve and didn't think it would take quite so
long to merge the rtrmap code.

> > Fix this by detecting non-bdevs and finding (we hope) the loop device
> > that was created to handle the mount. 
> 
> What loop device? xfs/336 doesn't use loop devices at all.
> 
> Oh, this is assuming that mount will silently do a loopback mount
> when passed a file rather than a block device. IOWs, it's relying on
> some third party to do the loop device creation and hence allow it
> to be mounted.
> 
> IOWs, this change is addressing a landmine by adding another
> landmine.

Some would say that mount adding the ability to set up a loop dev was
itself *avoiding* a landmine from 90s era util-linux.

> I really think that xfs/336 needs to be fixed - one off test hacks
> like this, while they may work, only make modifying and maintaining
> the fstests infrastructure that much harder....

Yeah, it'll get cleaned up for the rtrmap fstests merge.

> > While we're at it, have the
> > helper return the exit code from mount, not _prepare_for_eio_shutdown.
> 
> That should be a separate patch.

Ok.

--D

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