diff mbox series

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

Message ID 170899915319.896550.14222768162023866668.stgit@frogsfrogsfrogs (mailing list archive)
State New
Headers show
Series [1/8] generic/604: try to make race occur reliably | expand

Commit Message

Darrick J. Wong Feb. 27, 2024, 2:02 a.m. UTC 
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(-)

Comments

Zorro Lang Feb. 27, 2024, 5:31 a.m. UTC | #1 
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
> 
>
Darrick J. Wong Feb. 28, 2024, 1:28 a.m. UTC | #2 
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 mbox series

Patch

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