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

[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]



On Fri, Mar 01, 2024 at 09:51:24AM -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>
> ---
> v1.1: address some review comments from maintainer

This version looks good to me,
Reviewed-by: Zorro Lang <zlang@xxxxxxxxxx>

> ---
>  common/module |   33 ++++++++++++++++++++++++++++-----
>  tests/xfs/434 |    3 +--
>  tests/xfs/435 |    3 +--
>  tests/xfs/436 |    3 +--
>  4 files changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/common/module b/common/module
> index 6efab71d34..a8d5f492d3 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,28 @@ _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
>  }
>  
>  # 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
> 





[Index of Archives]     [Linux Filesystems Development]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux