Re: [PATCH 7/8] xfs/43[4-6]: make module reloading optional

["Darrick J. Wong" <djwong@kernel.org>]

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