| Message ID | 173706974243.1927324.9105721327110864014.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/23] generic/476: fix fsstress process management | expand |
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". 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.
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 >
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.
diff --git a/common/rc b/common/rc index 885669beeb5e26..4419cfc3188374 100644 --- a/common/rc +++ b/common/rc @@ -441,6 +441,7 @@ _try_scratch_mount() [ $mount_ret -ne 0 ] && return $mount_ret _idmapped_mount $SCRATCH_DEV $SCRATCH_MNT _prepare_for_eio_shutdown $SCRATCH_DEV + return $mount_ret } # mount scratch device with given options and _fail if mount fails @@ -658,6 +659,7 @@ _test_mount() [ $mount_ret -ne 0 ] && return $mount_ret _idmapped_mount $TEST_DEV $TEST_DIR _prepare_for_eio_shutdown $TEST_DEV + return $mount_ret } _test_unmount() @@ -4469,6 +4471,12 @@ _destroy_loop_device() losetup -d $dev || _fail "Cannot destroy loop device $dev" } +# Find the loop bdev for a given file, if there is one. +_find_loop_device() +{ + losetup --list -n -O NAME -j "$1" +} + _scale_fsstress_args() { local args="" diff --git a/common/xfs b/common/xfs index 0417a40adba3e2..c68bd6d7c773ac 100644 --- a/common/xfs +++ b/common/xfs @@ -1110,6 +1110,12 @@ _xfs_prepare_for_eio_shutdown() local dev="$1" local ctlfile="error/fail_at_unmount" + # Is this a regular file? Check if there's a loop device somewhere. + # Hopefully that lines up with a mounted filesystem. + if [ ! -b "$dev" ]; then + dev=$(_find_loop_device "$1" | tail -n 1) + fi + # Once we enable IO errors, it's possible that a writer thread will # trip over EIO, cancel the transaction, and shut down the system. # This is expected behavior, so we need to remove the "Internal error"