On Thu, Jan 16, 2025 at 03:28:02PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@xxxxxxxxxx> > > 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". 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. 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? > 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. 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.... > 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. -Dave. -- Dave Chinner david@xxxxxxxxxxxxx
