[PATCH 0/2] btrfs-progs: convert: fix csum generation for migrated ranges

[PATCH 0/2] btrfs-progs: convert: fix csum generation for migrated ranges

[Qu Wenruo]

There is an internal report that btrfs/012 failed on 64K page size
systems.

It turns out that with 64K block size for ext4, even an empty ext4 can
lead to csum errors for the image file.

The root cause is the bad csum generation, which read incorrect data
from the disk, and leads to bad csum generated. (while the on-disk data
is still correct).

This patchset would fix the bug and add a test case for it.

Qu Wenruo (2):
  btrfs-progs: convert: fix bad csum for migrated range.
  btrfs-progs: tests/convert: add a test case to check the csum for the
    image file

 convert/main.c                                   | 12 ++++++++++--
 .../023-64k-blocksize-migrated/test.sh           | 16 ++++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)
 create mode 100755 tests/convert-tests/023-64k-blocksize-migrated/test.sh

-- 
2.40.1

[PATCH 1/2] btrfs-progs: convert: fix bad csum for migrated range.

[Qu Wenruo]

[BUG]
There is a report that btrfs-convert leads to bad csum for the image
file.

The reproducer looks like this:
(note the 64K block size, it's used to force a certain chunk layout)

 # touch test.img
 # truncate -s 10G test.img
 # mkfs.ext4 -b 64K test.img
 # btrfs-convert -N 64K test.img
 # btrfs check --check-data-csum test.img
 Opening filesystem to check...
 Checking filesystem on /home/adam/test.img
 UUID: 39d49537-a9f5-47f1-b6ab-7857707b9133
 [1/7] checking root items
 [2/7] checking extents
 [3/7] checking free space cache
 [4/7] checking fs roots
 [5/7] checking csums against data
 mirror 1 bytenr 4563140608 csum 0x3f1fa0ef expected csum 0xa4c4c072
 mirror 1 bytenr 4563206144 csum 0x55dcf0d3 expected csum 0xa4c4c072
 mirror 1 bytenr 4563271680 csum 0x4491b00a expected csum 0xa4c4c072
 mirror 1 bytenr 4563337216 csum 0x655d1f61 expected csum 0xa4c4c072
 mirror 1 bytenr 4563402752 csum 0xd37114d3 expected csum 0xa4c4c072
 mirror 1 bytenr 4563468288 csum 0x4c2dab30 expected csum 0xa4c4c072
 mirror 1 bytenr 4563533824 csum 0xa80fceed expected csum 0xa4c4c072
 mirror 1 bytenr 4563599360 csum 0xaf610db8 expected csum 0xa4c4c072
 mirror 1 bytenr 4563795968 csum 0x67b3c8a0 expected csum 0xa4c4c072
 ERROR: errors found in csum tree
 [6/7] checking root refs
 ...

[CAUSE]
Above initial failure is for logical bytenr of 4563140608, which is
inside the relocated range of the image file offset [0, 1M).

During convert, we migrate the original image file ranges which would
later be covered by super and other reserved ranges.

The migration happens
- Read out the original data
- Reserve a new file extent
- Write the data back to the file extent
  Note that, the new file extent can be inside some new data chunks,
  thus it's no longer 1:1 mapped.
- Generate the new csum for the new file extent

The problem happens at the last stage. We should read out the data from
the new file extent, but we call read_disk_extent() using the logical
bytenr, however read_disk_extent() is not doing logical -> physical
mapping.

Thus we will read some garbage, not the newly written data, and use
those garbage to generate csum. And caused the above problem.

[FIX]
Instead of read_disk_extent(), call read_data_from_disk(), which would
do the proper logical -> physical mapping, thus would fix the bug.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 convert/main.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/convert/main.c b/convert/main.c
index 941b5ce3244d..46c6dfc571ff 100644
--- a/convert/main.c
+++ b/convert/main.c
@@ -191,10 +191,18 @@ static int csum_disk_extent(struct btrfs_trans_handle *trans,
 	if (!buffer)
 		return -ENOMEM;
 	for (offset = 0; offset < num_bytes; offset += blocksize) {
-		ret = read_disk_extent(root, disk_bytenr + offset,
-					blocksize, buffer);
+		u64 read_len = blocksize;
+
+		ret = read_data_from_disk(root->fs_info, buffer,
+					  disk_bytenr + offset, &read_len, 0);
 		if (ret)
 			break;
+		if (read_len == 0) {
+			error("failed to read logical bytenr %llu",
+			      disk_bytenr + offset);
+			ret = -EIO;
+			break;
+		}
 		ret = btrfs_csum_file_block(trans,
 					    disk_bytenr + num_bytes,
 					    disk_bytenr + offset,
-- 
2.40.1

[PATCH 2/2] btrfs-progs: tests/convert: add a test case to check the csum for the image file

[Qu Wenruo]

The new test case would create an empty ext4 with 64K block size, which
can lead to a new data chunk which is no longer 1:1 mapped.

Then convert the fs and verify it with --check-data-csum to make sure
the image file is fine.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 .../023-64k-blocksize-migrated/test.sh           | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100755 tests/convert-tests/023-64k-blocksize-migrated/test.sh

diff --git a/tests/convert-tests/023-64k-blocksize-migrated/test.sh b/tests/convert-tests/023-64k-blocksize-migrated/test.sh
new file mode 100755
index 000000000000..99808a74314c
--- /dev/null
+++ b/tests/convert-tests/023-64k-blocksize-migrated/test.sh
@@ -0,0 +1,16 @@
+#!/bin/sh
+# Make sure the migrated range doesn't cause csum errors
+
+source "$TEST_TOP/common" || exit
+source "$TEST_TOP/common.convert" || exit
+
+setup_root_helper
+prepare_test_dev 10G
+
+check_global_prereq mkfs.ext4
+check_prereq btrfs-convert
+check_prereq btrfs
+
+run_check mkfs.ext4 -b 64K -F "$TEST_DEV"
+run_check $SUDO_HELPER "$TOP/btrfs-convert" -N 64K "$TEST_DEV"
+run_check $SUDO_HELPER "$TOP/btrfs" check --check-data-csum "$TEST_DEV"
-- 
2.40.1

Re: [PATCH 2/2] btrfs-progs: tests/convert: add a test case to check the csum for the image file

[Anand Jain]

LGTM

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

Re: [PATCH 0/2] btrfs-progs: convert: fix csum generation for migrated ranges

[David Sterba]

On Tue, May 09, 2023 at 08:43:13AM +0800, Qu Wenruo wrote:
> There is an internal report that btrfs/012 failed on 64K page size
> systems.
> 
> It turns out that with 64K block size for ext4, even an empty ext4 can
> lead to csum errors for the image file.
> 
> The root cause is the bad csum generation, which read incorrect data
> from the disk, and leads to bad csum generated. (while the on-disk data
> is still correct).
> 
> This patchset would fix the bug and add a test case for it.
> 
> Qu Wenruo (2):
>   btrfs-progs: convert: fix bad csum for migrated range.
>   btrfs-progs: tests/convert: add a test case to check the csum for the
>     image file

Added to devel, thanks.