[PATCH 0/3] fstests: various RAID56 related fixes for btrfs

[PATCH 0/3] fstests: various RAID56 related fixes for btrfs

[Josef Bacik]

Hello,

While trying to get CI setup internally I noticed that we were sometimes failing
raid56 tests even though I had specified BTRFS_PROFILE_CONFIGS without raid56 in
them.

This is because tests where we require raid56 to work only check to see if it's
enabled by the kernel, not to check and see if we're configured to use it with
our profile configs.

One test needed to be updated to skip any configurations that weren't in our
profile configs, and then a few tests didn't use the 

_require_btrfs_fs_feature raid56

check in the top of their test.  This series fixes everything up and honors the
user settings which makes my internal CI runs clean where we don't want to test
raid56.  Thanks,

Josef

Josef Bacik (3):
  fstests: check btrfs profile configs before allowing raid56
  fstests: btrfs/195: skip raid setups not in the profile configs
  fstests: add _require_btrfs_fs_feature raid56 to a few tests

 common/btrfs    | 8 ++++++--
 tests/btrfs/195 | 8 ++++++++
 tests/btrfs/197 | 1 +
 tests/btrfs/198 | 1 +
 tests/btrfs/297 | 1 +
 5 files changed, 17 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH 1/3] fstests: check btrfs profile configs before allowing raid56

[Josef Bacik]

For some of our tests we have

_require_btrfs_fs_feature raid56

to make sure the raid56 support is loaded in the kernel.  However this
isn't the only limiting factor, we can have only zoned devices which we
already check for, but we could also have BTRFS_PROFILE_CONFIGS set
without raid5 or raid6 as an option.  Fix this by simply checking the
profile as appropriate and skip running the test if we're missing raid5
or raid6 in our profile settings.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 common/btrfs | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/common/btrfs b/common/btrfs
index b0f7f095..d9b01a48 100644
--- a/common/btrfs
+++ b/common/btrfs
@@ -111,8 +111,12 @@ _require_btrfs_fs_feature()
 		_notrun "Feature $feat not supported by the available btrfs version"
 
 	if [ $feat = "raid56" ]; then
-		# Zoned btrfs only supports SINGLE profile
-		_require_non_zoned_device "${SCRATCH_DEV}"
+		# Make sure it's in our supported configs as well
+		_btrfs_get_profile_configs
+		if [[ ! "${_btrfs_profile_configs[@]}" =~ "raid5" ]] ||
+		   [[ ! "${_btrfs_profile_configs[@]}" =~ "raid6" ]]; then
+			_notrun "raid56 excluded from profile configs"
+		fi
 	fi
 }
 
-- 
2.43.0

Re: [PATCH 1/3] fstests: check btrfs profile configs before allowing raid56

[Christoph Hellwig]

On Tue, Mar 19, 2024 at 12:55:56PM -0400, Josef Bacik wrote:
> +++ b/common/btrfs
> @@ -111,8 +111,12 @@ _require_btrfs_fs_feature()
>  		_notrun "Feature $feat not supported by the available btrfs version"
>  
>  	if [ $feat = "raid56" ]; then
> -		# Zoned btrfs only supports SINGLE profile
> -		_require_non_zoned_device "${SCRATCH_DEV}"
> +		# Make sure it's in our supported configs as well
> +		_btrfs_get_profile_configs
> +		if [[ ! "${_btrfs_profile_configs[@]}" =~ "raid5" ]] ||
> +		   [[ ! "${_btrfs_profile_configs[@]}" =~ "raid6" ]]; then
> +			_notrun "raid56 excluded from profile configs"
> +		fi

Should _require_btrfs_fs_feature check for raid5 and raid6 individually?
Right now it seems like you need both raid5 and raid6 in the profiles
to run checks that probably only exercise either?

Re: [PATCH 1/3] fstests: check btrfs profile configs before allowing raid56

[Josef Bacik]

On Tue, Mar 19, 2024 at 01:59:11PM -0700, Christoph Hellwig wrote:
> On Tue, Mar 19, 2024 at 12:55:56PM -0400, Josef Bacik wrote:
> > +++ b/common/btrfs
> > @@ -111,8 +111,12 @@ _require_btrfs_fs_feature()
> >  		_notrun "Feature $feat not supported by the available btrfs version"
> >  
> >  	if [ $feat = "raid56" ]; then
> > -		# Zoned btrfs only supports SINGLE profile
> > -		_require_non_zoned_device "${SCRATCH_DEV}"
> > +		# Make sure it's in our supported configs as well
> > +		_btrfs_get_profile_configs
> > +		if [[ ! "${_btrfs_profile_configs[@]}" =~ "raid5" ]] ||
> > +		   [[ ! "${_btrfs_profile_configs[@]}" =~ "raid6" ]]; then
> > +			_notrun "raid56 excluded from profile configs"
> > +		fi
> 
> Should _require_btrfs_fs_feature check for raid5 and raid6 individually?
> Right now it seems like you need both raid5 and raid6 in the profiles
> to run checks that probably only exercise either?
> 

The tests that use this are testing them both.  I could make them optionally
test one or the other depending on the profile you had set, but I don't think
this is particularly useful.  If you have strong feelings about it I could
alternatively add a helper that is

_require_raid_profile "profile name"

and then add those to the different tests to be more fine grained about it,
since technically this helper is about checking if a specific kernel feature is
available.  Thanks,

Josef

Re: [PATCH 1/3] fstests: check btrfs profile configs before allowing raid56

[Christoph Hellwig]

On Wed, Mar 20, 2024 at 10:40:19AM -0400, Josef Bacik wrote:
> The tests that use this are testing them both.  I could make them optionally
> test one or the other depending on the profile you had set, but I don't think
> this is particularly useful.  If you have strong feelings about it I could
> alternatively add a helper that is
> 
> _require_raid_profile "profile name"
> 
> and then add those to the different tests to be more fine grained about it,
> since technically this helper is about checking if a specific kernel feature is
> available.  Thanks,

That seems a bit more useful, but it looks like others have even
better idea here in the thread, so I'll shut up.

Re: [PATCH 1/3] fstests: check btrfs profile configs before allowing raid56

[Anand Jain]

On 3/19/24 22:25, Josef Bacik wrote:
> For some of our tests we have
> 
> _require_btrfs_fs_feature raid56
> 
> to make sure the raid56 support is loaded in the kernel.  However this
> isn't the only limiting factor, we can have only zoned devices which we
> already check for, but we could also have BTRFS_PROFILE_CONFIGS set
> without raid5 or raid6 as an option.  Fix this by simply checking the
> profile as appropriate and skip running the test if we're missing raid5
> or raid6 in our profile settings.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>   common/btrfs | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/common/btrfs b/common/btrfs
> index b0f7f095..d9b01a48 100644
> --- a/common/btrfs
> +++ b/common/btrfs
> @@ -111,8 +111,12 @@ _require_btrfs_fs_feature()
>   		_notrun "Feature $feat not supported by the available btrfs version"
>   
>   	if [ $feat = "raid56" ]; then
> -		# Zoned btrfs only supports SINGLE profile
> -		_require_non_zoned_device "${SCRATCH_DEV}"

Don't we still need to exclude the zoned device from the
RAID56 test cases?


> +		# Make sure it's in our supported configs as well
> +		_btrfs_get_profile_configs
> +		if [[ ! "${_btrfs_profile_configs[@]}" =~ "raid5" ]] ||
> +		   [[ ! "${_btrfs_profile_configs[@]}" =~ "raid6" ]]; then
> +			_notrun "raid56 excluded from profile configs"
> +		fi
>   	fi
>   }
>   

Thanks, Anand

Re: [PATCH 1/3] fstests: check btrfs profile configs before allowing raid56

[Josef Bacik]

On Wed, Mar 20, 2024 at 06:01:14PM +0530, Anand Jain wrote:
> On 3/19/24 22:25, Josef Bacik wrote:
> > For some of our tests we have
> > 
> > _require_btrfs_fs_feature raid56
> > 
> > to make sure the raid56 support is loaded in the kernel.  However this
> > isn't the only limiting factor, we can have only zoned devices which we
> > already check for, but we could also have BTRFS_PROFILE_CONFIGS set
> > without raid5 or raid6 as an option.  Fix this by simply checking the
> > profile as appropriate and skip running the test if we're missing raid5
> > or raid6 in our profile settings.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >   common/btrfs | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/common/btrfs b/common/btrfs
> > index b0f7f095..d9b01a48 100644
> > --- a/common/btrfs
> > +++ b/common/btrfs
> > @@ -111,8 +111,12 @@ _require_btrfs_fs_feature()
> >   		_notrun "Feature $feat not supported by the available btrfs version"
> >   	if [ $feat = "raid56" ]; then
> > -		# Zoned btrfs only supports SINGLE profile
> > -		_require_non_zoned_device "${SCRATCH_DEV}"
> 
> Don't we still need to exclude the zoned device from the
> RAID56 test cases?

_btrfs_get_profile_configs removes RAID5 and RAID6 from the available profiles
if the scratch device is zoned.  Thanks,

Josef

[PATCH 2/3] fstests: btrfs/195: skip raid setups not in the profile configs

[Josef Bacik]

You can specify a custom BTRFS_PROFILE_CONFIGS to skip certain raid
configurations in the tests, however btrfs/195 doesn't honor this
currently.  Fix this up by getting the profile configs and skipping any
configurations that are not listed in BTRFS_PROFILE_CONFIGS.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 tests/btrfs/195 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/btrfs/195 b/tests/btrfs/195
index 96cc4134..df8f5ed6 100755
--- a/tests/btrfs/195
+++ b/tests/btrfs/195
@@ -21,6 +21,9 @@ _require_scratch_dev_pool 4
 # Zoned btrfs only supports SINGLE profile
 _require_non_zoned_device "${SCRATCH_DEV}"
 
+# Load up the available configs
+_btrfs_get_profile_configs
+
 declare -a TEST_VECTORS=(
 # $nr_dev_min:$data:$metadata:$data_convert:$metadata_convert
 "4:single:raid1"
@@ -38,6 +41,11 @@ run_testcase() {
 	src_type=${args[1]}
 	dst_type=${args[2]}
 
+	if [[ ! "${_btrfs_profile_configs[@]}" =~ "$dst_type" ]]; then
+		echo "=== Skipping test: $1 ===" >> $seqres.full
+		return
+	fi
+
 	_scratch_dev_pool_get $num_disks
 
 	echo "=== Running test: $1 ===" >> $seqres.full
-- 
2.43.0

Re: [PATCH 2/3] fstests: btrfs/195: skip raid setups not in the profile configs

[Anand Jain]

On 3/19/24 22:25, Josef Bacik wrote:
> You can specify a custom BTRFS_PROFILE_CONFIGS to skip certain raid
> configurations in the tests, however btrfs/195 doesn't honor this
> currently.  Fix this up by getting the profile configs and skipping any
> configurations that are not listed in BTRFS_PROFILE_CONFIGS.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>

This doesn't use _require_btrfs_fs_feature raid56,
so applied. Included for the PR.

Thanks, Anand

> ---
>   tests/btrfs/195 | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/tests/btrfs/195 b/tests/btrfs/195
> index 96cc4134..df8f5ed6 100755
> --- a/tests/btrfs/195
> +++ b/tests/btrfs/195
> @@ -21,6 +21,9 @@ _require_scratch_dev_pool 4
>   # Zoned btrfs only supports SINGLE profile
>   _require_non_zoned_device "${SCRATCH_DEV}"
>   
> +# Load up the available configs
> +_btrfs_get_profile_configs
> +
>   declare -a TEST_VECTORS=(
>   # $nr_dev_min:$data:$metadata:$data_convert:$metadata_convert
>   "4:single:raid1"
> @@ -38,6 +41,11 @@ run_testcase() {
>   	src_type=${args[1]}
>   	dst_type=${args[2]}
>   
> +	if [[ ! "${_btrfs_profile_configs[@]}" =~ "$dst_type" ]]; then
> +		echo "=== Skipping test: $1 ===" >> $seqres.full
> +		return
> +	fi
> +
>   	_scratch_dev_pool_get $num_disks
>   
>   	echo "=== Running test: $1 ===" >> $seqres.full

[PATCH 3/3] fstests: add _require_btrfs_fs_feature raid56 to a few tests

[Josef Bacik]

There are a few tests that don't have the required

_require_btrfs_fs_feature raid56

check in them even tho they are raid5/6 related tests.  Add this check
in order to make sure environments that don't have raid5/6 support don't
improperly fail them.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 tests/btrfs/197 | 1 +
 tests/btrfs/198 | 1 +
 tests/btrfs/297 | 1 +
 3 files changed, 3 insertions(+)

diff --git a/tests/btrfs/197 b/tests/btrfs/197
index d259fd99..8a034fdc 100755
--- a/tests/btrfs/197
+++ b/tests/btrfs/197
@@ -32,6 +32,7 @@ _require_scratch
 _require_scratch_dev_pool 5
 # Zoned btrfs only supports SINGLE profile
 _require_non_zoned_device ${SCRATCH_DEV}
+_require_btrfs_fs_feature raid56
 
 workout()
 {
diff --git a/tests/btrfs/198 b/tests/btrfs/198
index 7d23ffce..ecce81cd 100755
--- a/tests/btrfs/198
+++ b/tests/btrfs/198
@@ -20,6 +20,7 @@ _require_scratch
 _require_scratch_dev_pool 4
 # Zoned btrfs only supports SINGLE profile
 _require_non_zoned_device ${SCRATCH_DEV}
+_require_btrfs_fs_feature raid56
 _fixed_by_kernel_commit 96c2e067ed3e3e \
 	"btrfs: skip devices without magic signature when mounting"
 
diff --git a/tests/btrfs/297 b/tests/btrfs/297
index a0023861..990b83b1 100755
--- a/tests/btrfs/297
+++ b/tests/btrfs/297
@@ -15,6 +15,7 @@ _supported_fs btrfs
 _require_odirect
 _require_non_zoned_device "${SCRATCH_DEV}"
 _require_scratch_dev_pool 3
+_require_btrfs_fs_feature raid56
 _fixed_by_kernel_commit 486c737f7fdc \
 	"btrfs: raid56: always verify the P/Q contents for scrub"
 
-- 
2.43.0

Re: [PATCH 3/3] fstests: add _require_btrfs_fs_feature raid56 to a few tests

[Naohiro Aota]

On Tue, Mar 19, 2024 at 12:55:58PM -0400, Josef Bacik wrote:
> There are a few tests that don't have the required
> 
> _require_btrfs_fs_feature raid56
> 
> check in them even tho they are raid5/6 related tests.  Add this check
> in order to make sure environments that don't have raid5/6 support don't
> improperly fail them.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  tests/btrfs/197 | 1 +
>  tests/btrfs/198 | 1 +
>  tests/btrfs/297 | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/tests/btrfs/197 b/tests/btrfs/197
> index d259fd99..8a034fdc 100755
> --- a/tests/btrfs/197
> +++ b/tests/btrfs/197
> @@ -32,6 +32,7 @@ _require_scratch
>  _require_scratch_dev_pool 5
>  # Zoned btrfs only supports SINGLE profile
>  _require_non_zoned_device ${SCRATCH_DEV}
> +_require_btrfs_fs_feature raid56

How about moving the check in the workout() side, and skip the work based
on a specified profile?

This test runs the workout with raid1, 5, 6, and 10. This check can
needlessly skip the test for raid1 and raid10 if the profile setup does not
contain raid5 and raid6.

Especially, as we already have raid1 and raid10 support on a zoned setup,
we'd like to run the tests for them on it.

>  
>  workout()
>  {
> diff --git a/tests/btrfs/198 b/tests/btrfs/198
> index 7d23ffce..ecce81cd 100755
> --- a/tests/btrfs/198
> +++ b/tests/btrfs/198
> @@ -20,6 +20,7 @@ _require_scratch
>  _require_scratch_dev_pool 4
>  # Zoned btrfs only supports SINGLE profile
>  _require_non_zoned_device ${SCRATCH_DEV}
> +_require_btrfs_fs_feature raid56

Same here.

>  _fixed_by_kernel_commit 96c2e067ed3e3e \
>  	"btrfs: skip devices without magic signature when mounting"
>  
> diff --git a/tests/btrfs/297 b/tests/btrfs/297
> index a0023861..990b83b1 100755
> --- a/tests/btrfs/297
> +++ b/tests/btrfs/297
> @@ -15,6 +15,7 @@ _supported_fs btrfs
>  _require_odirect
>  _require_non_zoned_device "${SCRATCH_DEV}"
>  _require_scratch_dev_pool 3
> +_require_btrfs_fs_feature raid56

This seems fine because it runs only raid5 and raid6.

>  _fixed_by_kernel_commit 486c737f7fdc \
>  	"btrfs: raid56: always verify the P/Q contents for scrub"
>  
> -- 
> 2.43.0
> 

Re: [PATCH 3/3] fstests: add _require_btrfs_fs_feature raid56 to a few tests

[Anand Jain]

On 3/21/24 11:25, Naohiro Aota wrote:
> On Tue, Mar 19, 2024 at 12:55:58PM -0400, Josef Bacik wrote:
>> There are a few tests that don't have the required
>>
>> _require_btrfs_fs_feature raid56
>>
>> check in them even tho they are raid5/6 related tests.  Add this check
>> in order to make sure environments that don't have raid5/6 support don't
>> improperly fail them.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>   tests/btrfs/197 | 1 +
>>   tests/btrfs/198 | 1 +
>>   tests/btrfs/297 | 1 +
>>   3 files changed, 3 insertions(+)
>>
>> diff --git a/tests/btrfs/197 b/tests/btrfs/197
>> index d259fd99..8a034fdc 100755
>> --- a/tests/btrfs/197
>> +++ b/tests/btrfs/197
>> @@ -32,6 +32,7 @@ _require_scratch
>>   _require_scratch_dev_pool 5
>>   # Zoned btrfs only supports SINGLE profile
>>   _require_non_zoned_device ${SCRATCH_DEV}
>> +_require_btrfs_fs_feature raid56
> 
> How about moving the check in the workout() side, and skip the work based
> on a specified profile?

Yeah, it's better to check for raid56 support in the workout() function
rather than at the testcase level. We can silently skip the workload if
the kernel doesn't support raid56.

> This test runs the workout with raid1, 5, 6, and 10. This check can
> needlessly skip the test for raid1 and raid10 if the profile setup does not
> contain raid5 and raid6.


> Especially, as we already have raid1 and raid10 support on a zoned setup,
> we'd like to run the tests for them on it.

This can be taken separately, not as part of this patch.

Thanks, Anand

> 
>>   
>>   workout()
>>   {
>> diff --git a/tests/btrfs/198 b/tests/btrfs/198
>> index 7d23ffce..ecce81cd 100755
>> --- a/tests/btrfs/198
>> +++ b/tests/btrfs/198
>> @@ -20,6 +20,7 @@ _require_scratch
>>   _require_scratch_dev_pool 4
>>   # Zoned btrfs only supports SINGLE profile
>>   _require_non_zoned_device ${SCRATCH_DEV}
>> +_require_btrfs_fs_feature raid56
> 
> Same here.
> 
>>   _fixed_by_kernel_commit 96c2e067ed3e3e \
>>   	"btrfs: skip devices without magic signature when mounting"
>>   
>> diff --git a/tests/btrfs/297 b/tests/btrfs/297
>> index a0023861..990b83b1 100755
>> --- a/tests/btrfs/297
>> +++ b/tests/btrfs/297
>> @@ -15,6 +15,7 @@ _supported_fs btrfs
>>   _require_odirect
>>   _require_non_zoned_device "${SCRATCH_DEV}"
>>   _require_scratch_dev_pool 3
>> +_require_btrfs_fs_feature raid56
> 
> This seems fine because it runs only raid5 and raid6.
> 
>>   _fixed_by_kernel_commit 486c737f7fdc \
>>   	"btrfs: raid56: always verify the P/Q contents for scrub"
>>   
>> -- 
>> 2.43.0