[PATCH 0/2] fstests: refine filesystem module reload handling

[PATCH 0/2] fstests: refine filesystem module reload handling

[Anand Jain]

These changes refine module reload control in testing. Patch 1 reloads
the module earlier in run_section, ensuring each section's first test
starts with a reloaded module. Patch 2 adds FS_MODULE_RELOAD_OPTIONS to
pass module options during reload, useful for the Btrfs pre-mount
configurations that aren’t available as mount options.

Anand Jain (2):
  fstests: move fs-module reload to earlier in the run_section function
  fstests: FS_MODULE_RELOAD_OPTIONS to control filesystem module reload
    options

 check | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

-- 
2.46.1

[PATCH 1/2] fstests: move fs-module reload to earlier in the run_section function

[Anand Jain]

Reload the module before each test, instead of later in run_section.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 check | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/check b/check
index 9222cd7e4f81..d8ee73f48c77 100755
--- a/check
+++ b/check
@@ -935,6 +935,15 @@ function run_section()
 			continue
 		fi
 
+		# Reload the module after each test to check for leaks or
+		# other problems.
+		if [ -n "${TEST_FS_MODULE_RELOAD}" ]; then
+			_test_unmount 2> /dev/null
+			_scratch_unmount 2> /dev/null
+			modprobe -r fs-$FSTYP
+			modprobe fs-$FSTYP
+		fi
+
 		# record that we really tried to run this test.
 		if ((!${#loop_status[*]})); then
 			try+=("$seqnum")
@@ -1033,15 +1042,6 @@ function run_section()
 			done
 		fi
 
-		# Reload the module after each test to check for leaks or
-		# other problems.
-		if [ -n "${TEST_FS_MODULE_RELOAD}" ]; then
-			_test_unmount 2> /dev/null
-			_scratch_unmount 2> /dev/null
-			modprobe -r fs-$FSTYP
-			modprobe fs-$FSTYP
-		fi
-
 		# Scan for memory leaks after every test so that associating
 		# a leak to a particular test will be as accurate as possible.
 		_check_kmemleak || tc_status="fail"
-- 
2.46.1

Re: [PATCH 1/2] fstests: move fs-module reload to earlier in the run_section function

[Darrick J. Wong]

On Fri, Nov 15, 2024 at 11:20:51PM +0800, Anand Jain wrote:
> Reload the module before each test, instead of later in run_section.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  check | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/check b/check
> index 9222cd7e4f81..d8ee73f48c77 100755
> --- a/check
> +++ b/check
> @@ -935,6 +935,15 @@ function run_section()
>  			continue
>  		fi
>  
> +		# Reload the module after each test to check for leaks or
> +		# other problems.

Hrmm.  The nice thing about doing the reload /after/ each test is that
unloading the module will purge any per-fs slab caches that the module
creates.  The slab teardown logs any forgotten objects while $seq still
points to the test that leaked those objects.  Granted it's not a 100%
solution (that job falls to kmemcheck) but it's a cheap check.

This change makes it so that if test N leaks something, fstests reports
them for test N+1.

--D

> +		if [ -n "${TEST_FS_MODULE_RELOAD}" ]; then
> +			_test_unmount 2> /dev/null
> +			_scratch_unmount 2> /dev/null
> +			modprobe -r fs-$FSTYP
> +			modprobe fs-$FSTYP
> +		fi
> +
>  		# record that we really tried to run this test.
>  		if ((!${#loop_status[*]})); then
>  			try+=("$seqnum")
> @@ -1033,15 +1042,6 @@ function run_section()
>  			done
>  		fi
>  
> -		# Reload the module after each test to check for leaks or
> -		# other problems.
> -		if [ -n "${TEST_FS_MODULE_RELOAD}" ]; then
> -			_test_unmount 2> /dev/null
> -			_scratch_unmount 2> /dev/null
> -			modprobe -r fs-$FSTYP
> -			modprobe fs-$FSTYP
> -		fi
> -
>  		# Scan for memory leaks after every test so that associating
>  		# a leak to a particular test will be as accurate as possible.
>  		_check_kmemleak || tc_status="fail"
> -- 
> 2.46.1
> 
> 

Re: [PATCH 1/2] fstests: move fs-module reload to earlier in the run_section function

[Anand Jain]

On 16/11/24 00:39, Darrick J. Wong wrote:
> On Fri, Nov 15, 2024 at 11:20:51PM +0800, Anand Jain wrote:
>> Reload the module before each test, instead of later in run_section.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   check | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/check b/check
>> index 9222cd7e4f81..d8ee73f48c77 100755
>> --- a/check
>> +++ b/check
>> @@ -935,6 +935,15 @@ function run_section()
>>   			continue
>>   		fi
>>   
>> +		# Reload the module after each test to check for leaks or
>> +		# other problems.
> 
> Hrmm.  The nice thing about doing the reload /after/ each test is that
> unloading the module will purge any per-fs slab caches that the module
> creates.  The slab teardown logs any forgotten objects while $seq still
> points to the test that leaked those objects.  Granted it's not a 100%
> solution (that job falls to kmemcheck) but it's a cheap check.
> 
> This change makes it so that if test N leaks something, fstests reports
> them for test N+1.
> 

Agreed. It's better to report the issue at test N. I'll add this comment 
to the code in v2.

Thanks!
Anand

> --D
> 
>> +		if [ -n "${TEST_FS_MODULE_RELOAD}" ]; then
>> +			_test_unmount 2> /dev/null
>> +			_scratch_unmount 2> /dev/null
>> +			modprobe -r fs-$FSTYP
>> +			modprobe fs-$FSTYP
>> +		fi
>> +
>>   		# record that we really tried to run this test.
>>   		if ((!${#loop_status[*]})); then
>>   			try+=("$seqnum")
>> @@ -1033,15 +1042,6 @@ function run_section()
>>   			done
>>   		fi
>>   
>> -		# Reload the module after each test to check for leaks or
>> -		# other problems.
>> -		if [ -n "${TEST_FS_MODULE_RELOAD}" ]; then
>> -			_test_unmount 2> /dev/null
>> -			_scratch_unmount 2> /dev/null
>> -			modprobe -r fs-$FSTYP
>> -			modprobe fs-$FSTYP
>> -		fi
>> -
>>   		# Scan for memory leaks after every test so that associating
>>   		# a leak to a particular test will be as accurate as possible.
>>   		_check_kmemleak || tc_status="fail"
>> -- 
>> 2.46.1
>>
>>

Re: [PATCH 1/2] fstests: move fs-module reload to earlier in the run_section function

[Filipe Manana]

On Fri, Nov 15, 2024 at 3:22 PM Anand Jain <anand.jain@oracle.com> wrote:
>
> Reload the module before each test, instead of later in run_section.

Why?
The change log should give a justification for the change.
It's just stating what the code changes are doing but without any reasoning.

Thanks.

>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  check | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/check b/check
> index 9222cd7e4f81..d8ee73f48c77 100755
> --- a/check
> +++ b/check
> @@ -935,6 +935,15 @@ function run_section()
>                         continue
>                 fi
>
> +               # Reload the module after each test to check for leaks or
> +               # other problems.
> +               if [ -n "${TEST_FS_MODULE_RELOAD}" ]; then
> +                       _test_unmount 2> /dev/null
> +                       _scratch_unmount 2> /dev/null
> +                       modprobe -r fs-$FSTYP
> +                       modprobe fs-$FSTYP
> +               fi
> +
>                 # record that we really tried to run this test.
>                 if ((!${#loop_status[*]})); then
>                         try+=("$seqnum")
> @@ -1033,15 +1042,6 @@ function run_section()
>                         done
>                 fi
>
> -               # Reload the module after each test to check for leaks or
> -               # other problems.
> -               if [ -n "${TEST_FS_MODULE_RELOAD}" ]; then
> -                       _test_unmount 2> /dev/null
> -                       _scratch_unmount 2> /dev/null
> -                       modprobe -r fs-$FSTYP
> -                       modprobe fs-$FSTYP
> -               fi
> -
>                 # Scan for memory leaks after every test so that associating
>                 # a leak to a particular test will be as accurate as possible.
>                 _check_kmemleak || tc_status="fail"
> --
> 2.46.1
>
>

[PATCH 2/2] fstests: FS_MODULE_RELOAD_OPTIONS to control filesystem module reload options

[Anand Jain]

Extend module reload logic to allow passing additional options via
`FS_MODULE_RELOAD_OPTIONS`. This enhancement enables more flexible
configuration during module reloads, which can be useful for testing
specific module parameters.

Maintains existing behavior for `TEST_FS_MODULE_RELOAD`.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 check | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/check b/check
index d8ee73f48c77..ced86466a4bb 100755
--- a/check
+++ b/check
@@ -937,11 +937,12 @@ function run_section()
 
 		# Reload the module after each test to check for leaks or
 		# other problems.
-		if [ -n "${TEST_FS_MODULE_RELOAD}" ]; then
+		if [[ -n "${TEST_FS_MODULE_RELOAD}" ||
+		      -n "${FS_MODULE_RELOAD_OPTIONS}" ]]; then
 			_test_unmount 2> /dev/null
 			_scratch_unmount 2> /dev/null
 			modprobe -r fs-$FSTYP
-			modprobe fs-$FSTYP
+			modprobe fs-$FSTYP ${FS_MODULE_RELOAD_OPTIONS}
 		fi
 
 		# record that we really tried to run this test.
-- 
2.46.1

Re: [PATCH 2/2] fstests: FS_MODULE_RELOAD_OPTIONS to control filesystem module reload options

[Darrick J. Wong]

On Fri, Nov 15, 2024 at 11:20:52PM +0800, Anand Jain wrote:
> Extend module reload logic to allow passing additional options via
> `FS_MODULE_RELOAD_OPTIONS`. This enhancement enables more flexible
> configuration during module reloads, which can be useful for testing
> specific module parameters.

You might want to document the existence of this knob in the README.

> Maintains existing behavior for `TEST_FS_MODULE_RELOAD`.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  check | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/check b/check
> index d8ee73f48c77..ced86466a4bb 100755
> --- a/check
> +++ b/check
> @@ -937,11 +937,12 @@ function run_section()
>  
>  		# Reload the module after each test to check for leaks or
>  		# other problems.
> -		if [ -n "${TEST_FS_MODULE_RELOAD}" ]; then
> +		if [[ -n "${TEST_FS_MODULE_RELOAD}" ||
> +		      -n "${FS_MODULE_RELOAD_OPTIONS}" ]]; then
>  			_test_unmount 2> /dev/null
>  			_scratch_unmount 2> /dev/null
>  			modprobe -r fs-$FSTYP
> -			modprobe fs-$FSTYP
> +			modprobe fs-$FSTYP ${FS_MODULE_RELOAD_OPTIONS}

What happens if the test itself decides to reload the $FSTYP module?
Do you want these options to propagate to those reloadings as well?
AFAICT there are some btrfs/ group tests that do such things.

(Perhaps you'd be better off injecting FS_MODULE_RELOAD_OPTIONS into
/etc/modprobe.d/<somefile> to catch all these cases?)

--D

>  		fi
>  
>  		# record that we really tried to run this test.
> -- 
> 2.46.1
> 
>