[PATCH 0/2] btrfs-progs: check: avoid false alerts for --check-data-csum on RAID56

[PATCH 0/2] btrfs-progs: check: avoid false alerts for --check-data-csum on RAID56

[Qu Wenruo]

There is a long existing bug that btrfs-progs doesn't really support
rebuilding its data using RAID56 P/Q.

This means any read with mirror_num > 1 for RAID56 won't work, and will
just return the P/Q raw data directly.

The RAID56 ability in btrfs-progs is only for data write.

This will cause tons of false alerts for "btrfs check
--check-data-csum", making it useless as an offline to verify RAID56
data.

The proper fix will need way more code modification (btrfs-fuse supports
that, so I believe it's possible).

But for now, let's just disable mirror_num > 1 read repair for progs.

Qu Wenruo (2):
  btrfs-progs: avoid checking wrong RAID5/6 P/Q data
  btrfs-progs: tests/fsck: add test case for data csum check on raid5

 kernel-shared/disk-io.c                       |  7 +++++
 kernel-shared/volumes.c                       | 10 +++---
 .../056-raid56-false-alerts/test.sh           | 31 +++++++++++++++++++
 3 files changed, 44 insertions(+), 4 deletions(-)
 create mode 100755 tests/fsck-tests/056-raid56-false-alerts/test.sh

-- 
2.35.1

[PATCH 1/2] btrfs-progs: avoid checking wrong RAID5/6 P/Q data

[Qu Wenruo]

[BUG]
"btrfs check --check-data-csum" is causing tons of false alerts for
RAID56 systems:

  # mkfs.btrfs -f $dev1 $dev2 $dev3 -m raid1 -d raid5
  # mount $dev1 $mnt
  # xfs_io -f -c "pwrite 0 16k" $mnt/file
  # umount $mnt
  # btrfs check --check-data-csum $dev1
  ...
  [5/7] checking csums against data
  mirror 2 bytenr 389152768 csum 0x02ca4e98 expected csum 0x98757625
  mirror 2 bytenr 389156864 csum 0x02ca4e98 expected csum 0x98757625
  mirror 2 bytenr 389160960 csum 0x02ca4e98 expected csum 0x98757625
  mirror 2 bytenr 389165056 csum 0x02ca4e98 expected csum 0x98757625
  ERROR: errors found in csum tree
  [6/7] checking root refs

But scrub is completely fine, and manually inspecting the on-disk data
shows nothing wrong.

[CAUSE]
Btrfs-progs only implemented the RAID56 write support, mostly for
metadata.

It doesn't have RAID56 repair ability at all.
(Btrfs-fuse has the ability ready to be contribued to progs though).

In read_extent_data(), it always read data from the first stripe,
without any RAID56 rebuild.

[WORKAROUND]
It will take a while to port RAID56 repair into btrfs-progs.
Just reduce the btrfs_num_copies() report for RAID56 to 1, so that we
won't even try to rebuild using P/Q.

Also add a warning message for open_ctree() of btrfs-progs, so
explicitly show the lack of RAID56 rebuild ability.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 kernel-shared/disk-io.c |  7 +++++++
 kernel-shared/volumes.c | 10 ++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c
index 4964cd3827e4..426fe40b53d6 100644
--- a/kernel-shared/disk-io.c
+++ b/kernel-shared/disk-io.c
@@ -997,6 +997,13 @@ int btrfs_check_fs_compatibility(struct btrfs_super_block *sb,
 		btrfs_set_super_incompat_flags(sb, features);
 	}
 
+	/*
+	 * We don't have the ability to repair from P/Q yet, give some warning
+	 * about this
+	 */
+	if (features & BTRFS_FEATURE_INCOMPAT_RAID56)
+		printf("WARNING: repairing using RAID56 P/Q is not supported yet\n");
+
 	features = btrfs_super_compat_ro_flags(sb);
 	if (flags & OPEN_CTREE_WRITES) {
 		if (flags & OPEN_CTREE_INVALIDATE_FST) {
diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c
index e24428db8412..5845a4383d5f 100644
--- a/kernel-shared/volumes.c
+++ b/kernel-shared/volumes.c
@@ -1645,10 +1645,12 @@ int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len)
 		ret = map->num_stripes;
 	else if (map->type & BTRFS_BLOCK_GROUP_RAID10)
 		ret = map->sub_stripes;
-	else if (map->type & BTRFS_BLOCK_GROUP_RAID5)
-		ret = 2;
-	else if (map->type & BTRFS_BLOCK_GROUP_RAID6)
-		ret = 3;
+	else if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK)
+		/*
+		 * In btrfs-progs we don't yet have the ability to rebuild
+		 * from P/Q, thus currently it can only provide one mirror.
+		 */
+		ret = 1;
 	else
 		ret = 1;
 	return ret;
-- 
2.35.1

[PATCH 2/2] btrfs-progs: tests/fsck: add test case for data csum check on raid5

[Qu Wenruo]

Previously 'btrfs check --check-data-csum' will report tons of false
alerts for RAID56.

Add a test case to make sure at least no such false alerts is reported.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 .../056-raid56-false-alerts/test.sh           | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100755 tests/fsck-tests/056-raid56-false-alerts/test.sh

diff --git a/tests/fsck-tests/056-raid56-false-alerts/test.sh b/tests/fsck-tests/056-raid56-false-alerts/test.sh
new file mode 100755
index 000000000000..b82e999c7740
--- /dev/null
+++ b/tests/fsck-tests/056-raid56-false-alerts/test.sh
@@ -0,0 +1,31 @@
+#!/bin/bash
+#
+# Make sure "btrfs check --check-data-csum" won't report false alerts on RAID56
+# data.
+#
+
+source "$TEST_TOP/common"
+
+check_prereq btrfs
+check_prereq mkfs.btrfs
+check_global_prereq losetup
+
+setup_loopdevs 3
+prepare_loopdevs
+dev1=${loopdevs[1]}
+TEST_DEV=$dev1
+
+setup_root_helper
+
+run_check $SUDO_HELPERS "$TOP/mkfs.btrfs" -f -m raid1 -d raid5 ${loopdevs[@]}
+run_check_mount_test_dev
+
+run_check $SUDO_HELPER dd if=/dev/urandom of="$TEST_MNT/file" bs=16K count=1 \
+	status=noxfer > /dev/null 2>&1
+
+run_check_umount_test_dev
+
+# Check data csum should not report false alerts
+run_check "$TOP/btrfs" check --check-data-csum "$dev1"
+
+cleanup_loopdevs
-- 
2.35.1

Re: [PATCH 1/2] btrfs-progs: avoid checking wrong RAID5/6 P/Q data

[David Sterba]

On Tue, Mar 29, 2022 at 05:44:25PM +0800, Qu Wenruo wrote:
> [BUG]
> "btrfs check --check-data-csum" is causing tons of false alerts for
> RAID56 systems:
> 
>   # mkfs.btrfs -f $dev1 $dev2 $dev3 -m raid1 -d raid5
>   # mount $dev1 $mnt
>   # xfs_io -f -c "pwrite 0 16k" $mnt/file
>   # umount $mnt
>   # btrfs check --check-data-csum $dev1
>   ...
>   [5/7] checking csums against data
>   mirror 2 bytenr 389152768 csum 0x02ca4e98 expected csum 0x98757625
>   mirror 2 bytenr 389156864 csum 0x02ca4e98 expected csum 0x98757625
>   mirror 2 bytenr 389160960 csum 0x02ca4e98 expected csum 0x98757625
>   mirror 2 bytenr 389165056 csum 0x02ca4e98 expected csum 0x98757625
>   ERROR: errors found in csum tree
>   [6/7] checking root refs
> 
> But scrub is completely fine, and manually inspecting the on-disk data
> shows nothing wrong.
> 
> [CAUSE]
> Btrfs-progs only implemented the RAID56 write support, mostly for
> metadata.
> 
> It doesn't have RAID56 repair ability at all.
> (Btrfs-fuse has the ability ready to be contribued to progs though).
> 
> In read_extent_data(), it always read data from the first stripe,
> without any RAID56 rebuild.
> 
> [WORKAROUND]
> It will take a while to port RAID56 repair into btrfs-progs.
> Just reduce the btrfs_num_copies() report for RAID56 to 1, so that we
> won't even try to rebuild using P/Q.
> 
> Also add a warning message for open_ctree() of btrfs-progs, so
> explicitly show the lack of RAID56 rebuild ability.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  kernel-shared/disk-io.c |  7 +++++++
>  kernel-shared/volumes.c | 10 ++++++----
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c
> index 4964cd3827e4..426fe40b53d6 100644
> --- a/kernel-shared/disk-io.c
> +++ b/kernel-shared/disk-io.c
> @@ -997,6 +997,13 @@ int btrfs_check_fs_compatibility(struct btrfs_super_block *sb,
>  		btrfs_set_super_incompat_flags(sb, features);
>  	}
>  
> +	/*
> +	 * We don't have the ability to repair from P/Q yet, give some warning
> +	 * about this
> +	 */
> +	if (features & BTRFS_FEATURE_INCOMPAT_RAID56)
> +		printf("WARNING: repairing using RAID56 P/Q is not supported yet\n");

There's a helper for warnings, warning()

> +
>  	features = btrfs_super_compat_ro_flags(sb);
>  	if (flags & OPEN_CTREE_WRITES) {
>  		if (flags & OPEN_CTREE_INVALIDATE_FST) {
> diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c
> index e24428db8412..5845a4383d5f 100644
> --- a/kernel-shared/volumes.c
> +++ b/kernel-shared/volumes.c
> @@ -1645,10 +1645,12 @@ int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len)
>  		ret = map->num_stripes;
>  	else if (map->type & BTRFS_BLOCK_GROUP_RAID10)
>  		ret = map->sub_stripes;
> -	else if (map->type & BTRFS_BLOCK_GROUP_RAID5)
> -		ret = 2;
> -	else if (map->type & BTRFS_BLOCK_GROUP_RAID6)
> -		ret = 3;
> +	else if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK)
> +		/*
> +		 * In btrfs-progs we don't yet have the ability to rebuild
> +		 * from P/Q, thus currently it can only provide one mirror.
> +		 */
> +		ret = 1;
>  	else
>  		ret = 1;
>  	return ret;
> -- 
> 2.35.1

Re: [PATCH 2/2] btrfs-progs: tests/fsck: add test case for data csum check on raid5

[David Sterba]

On Tue, Mar 29, 2022 at 05:44:26PM +0800, Qu Wenruo wrote:
> Previously 'btrfs check --check-data-csum' will report tons of false
> alerts for RAID56.
> 
> Add a test case to make sure at least no such false alerts is reported.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  .../056-raid56-false-alerts/test.sh           | 31 +++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100755 tests/fsck-tests/056-raid56-false-alerts/test.sh
> 
> diff --git a/tests/fsck-tests/056-raid56-false-alerts/test.sh b/tests/fsck-tests/056-raid56-false-alerts/test.sh
> new file mode 100755
> index 000000000000..b82e999c7740
> --- /dev/null
> +++ b/tests/fsck-tests/056-raid56-false-alerts/test.sh
> @@ -0,0 +1,31 @@
> +#!/bin/bash
> +#
> +# Make sure "btrfs check --check-data-csum" won't report false alerts on RAID56
> +# data.
> +#
> +
> +source "$TEST_TOP/common"
> +
> +check_prereq btrfs
> +check_prereq mkfs.btrfs
> +check_global_prereq losetup
> +
> +setup_loopdevs 3
> +prepare_loopdevs
> +dev1=${loopdevs[1]}
> +TEST_DEV=$dev1
> +
> +setup_root_helper
> +
> +run_check $SUDO_HELPERS "$TOP/mkfs.btrfs" -f -m raid1 -d raid5 ${loopdevs[@]}
              ^^^^^^^^^^^^
              SUDO_HELPER

> +run_check_mount_test_dev
> +
> +run_check $SUDO_HELPER dd if=/dev/urandom of="$TEST_MNT/file" bs=16K count=1 \
> +	status=noxfer > /dev/null 2>&1

This is not fstests, it's fine to use quieting options (like
status=noxfer) but it's not necessary, and in some cases even harmful
for debugging, to redirect output to /dev/null

> +
> +run_check_umount_test_dev
> +
> +# Check data csum should not report false alerts
> +run_check "$TOP/btrfs" check --check-data-csum "$dev1"
> +
> +cleanup_loopdevs
> -- 
> 2.35.1

Re: [PATCH 0/2] btrfs-progs: check: avoid false alerts for --check-data-csum on RAID56

[David Sterba]

On Tue, Mar 29, 2022 at 05:44:24PM +0800, Qu Wenruo wrote:
> There is a long existing bug that btrfs-progs doesn't really support
> rebuilding its data using RAID56 P/Q.
> 
> This means any read with mirror_num > 1 for RAID56 won't work, and will
> just return the P/Q raw data directly.
> 
> The RAID56 ability in btrfs-progs is only for data write.
> 
> This will cause tons of false alerts for "btrfs check
> --check-data-csum", making it useless as an offline to verify RAID56
> data.
> 
> The proper fix will need way more code modification (btrfs-fuse supports
> that, so I believe it's possible).
> 
> But for now, let's just disable mirror_num > 1 read repair for progs.
> 
> Qu Wenruo (2):
>   btrfs-progs: avoid checking wrong RAID5/6 P/Q data
>   btrfs-progs: tests/fsck: add test case for data csum check on raid5

Added to devel with some fixups, thanks.