| Message ID | 170899915319.896550.14222768162023866668.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] generic/604: try to make race occur reliably | expand |
On Mon, Feb 26, 2024 at 06:02:21PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > These three tests examine two things -- first, can xfs CoW staging > extent recovery handle corruptions in the refcount btree gracefully; and > second, can we avoid leaking incore inodes and dquots. > > The only cheap way to check the second condition is to rmmod and > modprobe the XFS module, which triggers leak detection when rmmod tears > down the caches. Currently, the entire test is _notrun if module > reloading doesn't work. > > Unfortunately, these tests never run for the majority of XFS developers > because their testbeds either compile the xfs kernel driver into vmlinux > statically or the rootfs is xfs so the module cannot be reloaded. The > author's testbed boots from NFS and does not have this limitation. > > Because we've had repeated instances of CoW recovery regressions not > being caught by testing until for-next hits my machine, let's make the > module reloading optional in all three tests to improve coverage. > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > common/module | 34 +++++++++++++++++++++++++++++----- > tests/xfs/434 | 3 +-- > tests/xfs/435 | 3 +-- > tests/xfs/436 | 3 +-- > 4 files changed, 32 insertions(+), 11 deletions(-) > > > diff --git a/common/module b/common/module > index 6efab71d34..f6814be34e 100644 > --- a/common/module > +++ b/common/module > @@ -48,12 +48,15 @@ _require_loadable_module() > modprobe "${module}" || _notrun "${module} load failed" > } > > -# Check that the module for FSTYP can be loaded. > -_require_loadable_fs_module() > +# Test if the module for FSTYP can be unloaded and reloaded. > +# > +# If not, returns 1 if $FSTYP is not a loadable module; 2 if the module could > +# not be unloaded; or 3 if loading the module fails. > +_test_loadable_fs_module() > { > local module="$1" > > - modinfo "${module}" > /dev/null 2>&1 || _notrun "${module}: must be a module." > + modinfo "${module}" > /dev/null 2>&1 || return 1 > > # Unload test fs, try to reload module, remount > local had_testfs="" > @@ -68,8 +71,29 @@ _require_loadable_fs_module() > modprobe "${module}" || load_ok=0 > test -n "${had_scratchfs}" && _scratch_mount 2> /dev/null > test -n "${had_testfs}" && _test_mount 2> /dev/null > - test -z "${unload_ok}" || _notrun "Require module ${module} to be unloadable" > - test -z "${load_ok}" || _notrun "${module} load failed" > + test -z "${unload_ok}" || return 2 > + test -z "${load_ok}" || return 3 > + return 0 > +} > + > +_require_loadable_fs_module() > +{ > + local module="$1" > + > + _test_loadable_fs_module "${module}" > + ret=$? > + case "$ret" in > + 1) > + _notrun "${module}: must be a module." > + ;; > + 2) > + _notrun "${module}: module could not be unloaded" > + ;; > + 3) > + _notrun "${module}: module reload failed" > + ;; > + esac > + return "${ret}" I think nobody checks the return value of a _require_xxx helper. The _require helper generally notrun or keep running. So if ret=0, then return directly, other return values trigger different _notrun. > } > > # Print the value of a filesystem module parameter > diff --git a/tests/xfs/434 b/tests/xfs/434 > index 12d1a0c9da..ca80e12753 100755 > --- a/tests/xfs/434 > +++ b/tests/xfs/434 > @@ -30,7 +30,6 @@ _begin_fstest auto quick clone fsr > > # real QA test starts here > _supported_fs xfs > -_require_loadable_fs_module "xfs" > _require_quota > _require_scratch_reflink > _require_cp_reflink > @@ -77,7 +76,7 @@ _scratch_unmount 2> /dev/null > rm -f ${RESULT_DIR}/require_scratch > > echo "See if we leak" > -_reload_fs_module "xfs" > +_test_loadable_fs_module "xfs" > > # success, all done > status=0 > diff --git a/tests/xfs/435 b/tests/xfs/435 > index 44135c7653..b52e9287df 100755 > --- a/tests/xfs/435 > +++ b/tests/xfs/435 > @@ -24,7 +24,6 @@ _begin_fstest auto quick clone > > # real QA test starts here > _supported_fs xfs > -_require_loadable_fs_module "xfs" > _require_quota > _require_scratch_reflink > _require_cp_reflink > @@ -55,7 +54,7 @@ _scratch_unmount 2> /dev/null > rm -f ${RESULT_DIR}/require_scratch > > echo "See if we leak" > -_reload_fs_module "xfs" > +_test_loadable_fs_module "xfs" So we don't care about if the fs module reload success or not, just try it then keep running? Thanks, Zorro > > # success, all done > status=0 > diff --git a/tests/xfs/436 b/tests/xfs/436 > index d010362785..02bcd66900 100755 > --- a/tests/xfs/436 > +++ b/tests/xfs/436 > @@ -27,7 +27,6 @@ _begin_fstest auto quick clone fsr > > # real QA test starts here > _supported_fs xfs > -_require_loadable_fs_module "xfs" > _require_scratch_reflink > _require_cp_reflink > _require_xfs_io_command falloc # fsr requires support for preallocation > @@ -72,7 +71,7 @@ _scratch_unmount 2> /dev/null > rm -f ${RESULT_DIR}/require_scratch > > echo "See if we leak" > -_reload_fs_module "xfs" > +_test_loadable_fs_module "xfs" > > # success, all done > status=0 > >
On Tue, Feb 27, 2024 at 01:31:36PM +0800, Zorro Lang wrote: > On Mon, Feb 26, 2024 at 06:02:21PM -0800, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > These three tests examine two things -- first, can xfs CoW staging > > extent recovery handle corruptions in the refcount btree gracefully; and > > second, can we avoid leaking incore inodes and dquots. > > > > The only cheap way to check the second condition is to rmmod and > > modprobe the XFS module, which triggers leak detection when rmmod tears > > down the caches. Currently, the entire test is _notrun if module > > reloading doesn't work. > > > > Unfortunately, these tests never run for the majority of XFS developers > > because their testbeds either compile the xfs kernel driver into vmlinux > > statically or the rootfs is xfs so the module cannot be reloaded. The > > author's testbed boots from NFS and does not have this limitation. > > > > Because we've had repeated instances of CoW recovery regressions not > > being caught by testing until for-next hits my machine, let's make the > > module reloading optional in all three tests to improve coverage. > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > common/module | 34 +++++++++++++++++++++++++++++----- > > tests/xfs/434 | 3 +-- > > tests/xfs/435 | 3 +-- > > tests/xfs/436 | 3 +-- > > 4 files changed, 32 insertions(+), 11 deletions(-) > > > > > > diff --git a/common/module b/common/module > > index 6efab71d34..f6814be34e 100644 > > --- a/common/module > > +++ b/common/module > > @@ -48,12 +48,15 @@ _require_loadable_module() > > modprobe "${module}" || _notrun "${module} load failed" > > } > > > > -# Check that the module for FSTYP can be loaded. > > -_require_loadable_fs_module() > > +# Test if the module for FSTYP can be unloaded and reloaded. > > +# > > +# If not, returns 1 if $FSTYP is not a loadable module; 2 if the module could > > +# not be unloaded; or 3 if loading the module fails. > > +_test_loadable_fs_module() > > { > > local module="$1" > > > > - modinfo "${module}" > /dev/null 2>&1 || _notrun "${module}: must be a module." > > + modinfo "${module}" > /dev/null 2>&1 || return 1 > > > > # Unload test fs, try to reload module, remount > > local had_testfs="" > > @@ -68,8 +71,29 @@ _require_loadable_fs_module() > > modprobe "${module}" || load_ok=0 > > test -n "${had_scratchfs}" && _scratch_mount 2> /dev/null > > test -n "${had_testfs}" && _test_mount 2> /dev/null > > - test -z "${unload_ok}" || _notrun "Require module ${module} to be unloadable" > > - test -z "${load_ok}" || _notrun "${module} load failed" > > + test -z "${unload_ok}" || return 2 > > + test -z "${load_ok}" || return 3 > > + return 0 > > +} > > + > > +_require_loadable_fs_module() > > +{ > > + local module="$1" > > + > > + _test_loadable_fs_module "${module}" > > + ret=$? > > + case "$ret" in > > + 1) > > + _notrun "${module}: must be a module." > > + ;; > > + 2) > > + _notrun "${module}: module could not be unloaded" > > + ;; > > + 3) > > + _notrun "${module}: module reload failed" > > + ;; > > + esac > > + return "${ret}" > > I think nobody checks the return value of a _require_xxx helper. The > _require helper generally notrun or keep running. So if ret=0, then > return directly, other return values trigger different _notrun. Ok. It's fine to let it run off the end, then. > > } > > > > # Print the value of a filesystem module parameter > > diff --git a/tests/xfs/434 b/tests/xfs/434 > > index 12d1a0c9da..ca80e12753 100755 > > --- a/tests/xfs/434 > > +++ b/tests/xfs/434 > > @@ -30,7 +30,6 @@ _begin_fstest auto quick clone fsr > > > > # real QA test starts here > > _supported_fs xfs > > -_require_loadable_fs_module "xfs" > > _require_quota > > _require_scratch_reflink > > _require_cp_reflink > > @@ -77,7 +76,7 @@ _scratch_unmount 2> /dev/null > > rm -f ${RESULT_DIR}/require_scratch > > > > echo "See if we leak" > > -_reload_fs_module "xfs" > > +_test_loadable_fs_module "xfs" > > > > # success, all done > > status=0 > > diff --git a/tests/xfs/435 b/tests/xfs/435 > > index 44135c7653..b52e9287df 100755 > > --- a/tests/xfs/435 > > +++ b/tests/xfs/435 > > @@ -24,7 +24,6 @@ _begin_fstest auto quick clone > > > > # real QA test starts here > > _supported_fs xfs > > -_require_loadable_fs_module "xfs" > > _require_quota > > _require_scratch_reflink > > _require_cp_reflink > > @@ -55,7 +54,7 @@ _scratch_unmount 2> /dev/null > > rm -f ${RESULT_DIR}/require_scratch > > > > echo "See if we leak" > > -_reload_fs_module "xfs" > > +_test_loadable_fs_module "xfs" > > So we don't care about if the fs module reload success or not, just > try it then keep running? Welll... the "test" actually does everything that we wanted to do (unmount, rmmod, modprobe, remount) so that's why I use it here. --D > Thanks, > Zorro > > > > > # success, all done > > status=0 > > diff --git a/tests/xfs/436 b/tests/xfs/436 > > index d010362785..02bcd66900 100755 > > --- a/tests/xfs/436 > > +++ b/tests/xfs/436 > > @@ -27,7 +27,6 @@ _begin_fstest auto quick clone fsr > > > > # real QA test starts here > > _supported_fs xfs > > -_require_loadable_fs_module "xfs" > > _require_scratch_reflink > > _require_cp_reflink > > _require_xfs_io_command falloc # fsr requires support for preallocation > > @@ -72,7 +71,7 @@ _scratch_unmount 2> /dev/null > > rm -f ${RESULT_DIR}/require_scratch > > > > echo "See if we leak" > > -_reload_fs_module "xfs" > > +_test_loadable_fs_module "xfs" > > > > # success, all done > > status=0 > > > > > >
diff --git a/common/module b/common/module index 6efab71d34..f6814be34e 100644 --- a/common/module +++ b/common/module @@ -48,12 +48,15 @@ _require_loadable_module() modprobe "${module}" || _notrun "${module} load failed" } -# Check that the module for FSTYP can be loaded. -_require_loadable_fs_module() +# Test if the module for FSTYP can be unloaded and reloaded. +# +# If not, returns 1 if $FSTYP is not a loadable module; 2 if the module could +# not be unloaded; or 3 if loading the module fails. +_test_loadable_fs_module() { local module="$1" - modinfo "${module}" > /dev/null 2>&1 || _notrun "${module}: must be a module." + modinfo "${module}" > /dev/null 2>&1 || return 1 # Unload test fs, try to reload module, remount local had_testfs="" @@ -68,8 +71,29 @@ _require_loadable_fs_module() modprobe "${module}" || load_ok=0 test -n "${had_scratchfs}" && _scratch_mount 2> /dev/null test -n "${had_testfs}" && _test_mount 2> /dev/null - test -z "${unload_ok}" || _notrun "Require module ${module} to be unloadable" - test -z "${load_ok}" || _notrun "${module} load failed" + test -z "${unload_ok}" || return 2 + test -z "${load_ok}" || return 3 + return 0 +} + +_require_loadable_fs_module() +{ + local module="$1" + + _test_loadable_fs_module "${module}" + ret=$? + case "$ret" in + 1) + _notrun "${module}: must be a module." + ;; + 2) + _notrun "${module}: module could not be unloaded" + ;; + 3) + _notrun "${module}: module reload failed" + ;; + esac + return "${ret}" } # Print the value of a filesystem module parameter diff --git a/tests/xfs/434 b/tests/xfs/434 index 12d1a0c9da..ca80e12753 100755 --- a/tests/xfs/434 +++ b/tests/xfs/434 @@ -30,7 +30,6 @@ _begin_fstest auto quick clone fsr # real QA test starts here _supported_fs xfs -_require_loadable_fs_module "xfs" _require_quota _require_scratch_reflink _require_cp_reflink @@ -77,7 +76,7 @@ _scratch_unmount 2> /dev/null rm -f ${RESULT_DIR}/require_scratch echo "See if we leak" -_reload_fs_module "xfs" +_test_loadable_fs_module "xfs" # success, all done status=0 diff --git a/tests/xfs/435 b/tests/xfs/435 index 44135c7653..b52e9287df 100755 --- a/tests/xfs/435 +++ b/tests/xfs/435 @@ -24,7 +24,6 @@ _begin_fstest auto quick clone # real QA test starts here _supported_fs xfs -_require_loadable_fs_module "xfs" _require_quota _require_scratch_reflink _require_cp_reflink @@ -55,7 +54,7 @@ _scratch_unmount 2> /dev/null rm -f ${RESULT_DIR}/require_scratch echo "See if we leak" -_reload_fs_module "xfs" +_test_loadable_fs_module "xfs" # success, all done status=0 diff --git a/tests/xfs/436 b/tests/xfs/436 index d010362785..02bcd66900 100755 --- a/tests/xfs/436 +++ b/tests/xfs/436 @@ -27,7 +27,6 @@ _begin_fstest auto quick clone fsr # real QA test starts here _supported_fs xfs -_require_loadable_fs_module "xfs" _require_scratch_reflink _require_cp_reflink _require_xfs_io_command falloc # fsr requires support for preallocation @@ -72,7 +71,7 @@ _scratch_unmount 2> /dev/null rm -f ${RESULT_DIR}/require_scratch echo "See if we leak" -_reload_fs_module "xfs" +_test_loadable_fs_module "xfs" # success, all done status=0