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