On Mon, Feb 26, 2024 at 06:02:21PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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
>
>
