| Message ID | 65177ca9d943c043f88d8ea034d1e625af3d0e58.1710867187.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fstests: various RAID56 related fixes for btrfs | expand |
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?
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
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
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
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.
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 }
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(-)