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

[Dave Chinner <david@fromorbit.com>]

On Tue, Jan 21, 2025 at 08:05:42PM -0800, Darrick J. Wong wrote:
> 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...

Yes, I know, which is why the moment I saw xfs/336 I called it out -
it has never run on my machines, ever...

> (Yeah, I hate bash)

Not a great fan of it myself. But it's no worse than other scripting
languages that use JIT based syntax checking from the "if it wasn't
run it ain't tested" perspective.

> > 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?

fstests configurations for block based filesystems have always been
based on block devices and mount points, not image files. 

Yes, you can pass an image file to XFS utilities and they will do
the right thing, but not all filesystems or all the infrastructure
in fstests can handle an image file masquerading as a device.

I certainly didn't expect it.....

> Can you point me to the sentence in doc/ that says this
> explicitly? 

fstests is woefully under-documented - especially when it comes to
configuration constraints and behaviours - so I doubt it is actually
specified anywhere. AFAIA it has never been raised in discussion
for a long time (not since we added network filesystem support a
long time ago, IIRC)

However, the code is pretty explicit - common/config is responsible
for setting up and validating the runtime config before any test can
run. All test and scratch devices are passed through this
validation:

_check_device()
{
        local name=$1
        local dev_needed=$2
        local dev=$3

        if [ -z "$dev" ]; then
                if [ "$dev_needed" == "required" ]; then
                        _fatal "common/config: $name is required but not defined!"
                fi
                return 0
        fi

        if [ -b "$dev" ] || ( echo $dev | grep -qE ":|//" ); then
                # block device or a network url
                return 0
	fi

	case "$FSTYP" in
        9p|fuse|tmpfs|virtiofs|afs)
	.....
	*)
                _fatal "common/config: $name ($dev) is not a block device or a network filesystem"
        esac
}
....

Basically, it says that all the test and scratch devices (including
the external ones) must be either a block device, a network URL, or
a string that the specific filesystem under test must recognise and
accept (e.g. a directory for overlay filesystems). Otherwise fstests
will fail to run with an explicit error message that says:

	<device> is not a block device or network filesystem

Nowhere in this config validation process does fstests consider
image files as a valid device configuration for a block based
filesystem.

If we need to do stuff with a image files, we have the
infrastructure to create loop devices and then operate directly on
that dynamic loop device(s) (e.g. _mkfs_dev, _mount, _unmount,
_check_xfs_filesystem, etc) that are created.

> 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's because it's validated before we start running tests and the
assumption is that nobody is screwing with SCRATCH_DEV in a way
that makes it behave vastly differently.

Consider what it means to have to do runtime checking of the
device validity in common code before we do anything with the
device. We'd have to sprinkle _check_device calls -everywhere-.

We'd also have to check logdev and rtdev variables if USE_EXTERNAL
is set, too.

That's not a viable development strategy, nor is it a maintainable
solution to the issue at hand. It's far simpler to fix one test not
to use this trick than it is to declare "nobody can trust TEST_DEV
or SCRATCH_DEV to be a valid block device" and have to handle that
everywhere those variables are used...

> That needs to be documented.

Sure.

> > > 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.

True.

But in the case of fstests we explicitly create loop
devices so that we don't have to play whacky games to find the
random loop device that mount magically creates when you pass it a
file.

Making all the image file and loop device usage consistent across
all of fstests was part of the infrastructure changes in my initial
check-parallel patchset. This was necessary because killing tests
with ctrl-c would randomly leave dangling mounts and loop devices
because many tests did not have _cleanup routines to tear down
mounts that auto-created loop devices or clean up loop
devices they created themselves properly.

Part of those changes was fixing up the mess in some XFS tests
where that mixed loop device and image file based operations
interchangably. I didn't notice x/336 because it wasn't
running on my test system and so didn't attempt to fix it at the
same time...

> > 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.

Thanks!

-Dave.

-- 
Dave Chinner
david@fromorbit.com