[PATCH 0/3] btrfs-progs: check: enhance the csum mismatch error message

[PATCH 0/3] btrfs-progs: check: enhance the csum mismatch error message

[Qu Wenruo]

This patchset will change the csum mismatch error message for data csum
mismatch, from the old almost meaningless output:

  [5/7] checking csums against data
  mirror 1 bytenr 13631488 csum 19 expected csum 152 <<<
  ERROR: errors found in csum tree

To a more human readable output:

  [5/7] checking csums against data
  mirror 1 bytenr 13631488 csum 0x13fec125 expected csum 0x98757625
  ERROR: errors found in csum tree

Qu Wenruo (3):
  btrfs-progs: move btrfs_format_csum() to common/utils.[ch]
  btrfs-progs: slightly enhance btrfs_format_csum()
  btrfs-progs: check: output proper csum values for --check-data-csum

 check/main.c            | 13 ++++++++++---
 common/utils.c          | 16 ++++++++++++++++
 common/utils.h          |  4 ++++
 kernel-shared/disk-io.c | 19 ++-----------------
 4 files changed, 32 insertions(+), 20 deletions(-)

-- 
2.32.0

[PATCH 1/3] btrfs-progs: move btrfs_format_csum() to common/utils.[ch]

[Qu Wenruo]

Function btrfs_format_csum() is a special helper only used in
btrfs-progs.

Move it to common/utils.[ch] other than leaving it in
kernel-shared/disk-io.c.

Since we're moving the code, also introduce a macro,
BTRFS_CSUM_STRING_LEN, to replace open-coded string length calculation.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 common/utils.c          | 14 ++++++++++++++
 common/utils.h          |  4 ++++
 kernel-shared/disk-io.c | 19 ++-----------------
 3 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/common/utils.c b/common/utils.c
index cf3331808df4..e944c43ac40e 100644
--- a/common/utils.c
+++ b/common/utils.c
@@ -385,6 +385,20 @@ enum btrfs_csum_type parse_csum_type(const char *s)
 	return 0;
 }
 
+int btrfs_format_csum(u16 csum_type, const u8 *data, char *output)
+{
+	int i;
+	const int csum_size = btrfs_csum_type_size(csum_type);
+
+	sprintf(output, "0x");
+	for (i = 0; i < csum_size; i++) {
+		output += 2;
+		sprintf(output, "%02x", data[i]);
+	}
+
+	return csum_size;
+}
+
 int get_device_info(int fd, u64 devid,
 		struct btrfs_ioctl_dev_info_args *di_args)
 {
diff --git a/common/utils.h b/common/utils.h
index 277026aeb016..8d5e78071f16 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -43,6 +43,10 @@ enum exclusive_operation {
 };
 
 enum btrfs_csum_type parse_csum_type(const char *s);
+
+/* 2 for "0x", 2 for each byte, plus nul */
+#define BTRFS_CSUM_STRING_LEN	(2 + 2 * BTRFS_CSUM_SIZE + 1)
+int btrfs_format_csum(u16 csum_type, const u8 *data, char *output);
 u64 parse_size_from_string(const char *s);
 u64 parse_qgroupid(const char *p);
 u64 arg_strtou64(const char *str);
diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c
index 84990a521178..8b6f5ef75804 100644
--- a/kernel-shared/disk-io.c
+++ b/kernel-shared/disk-io.c
@@ -162,20 +162,6 @@ int btrfs_csum_data(struct btrfs_fs_info *fs_info, u16 csum_type, const u8 *data
 	return -1;
 }
 
-int btrfs_format_csum(u16 csum_type, const u8 *data, char *output)
-{
-	int i;
-	const int csum_size = btrfs_csum_type_size(csum_type);
-
-	sprintf(output, "0x");
-	for (i = 0; i < csum_size; i++) {
-		output += 2;
-		sprintf(output, "%02x", data[i]);
-	}
-
-	return csum_size;
-}
-
 static int __csum_tree_block_size(struct extent_buffer *buf, u16 csum_size,
 				  int verify, int silent, u16 csum_type)
 {
@@ -189,9 +175,8 @@ static int __csum_tree_block_size(struct extent_buffer *buf, u16 csum_size,
 	if (verify) {
 		if (memcmp_extent_buffer(buf, result, 0, csum_size)) {
 			if (!silent) {
-				/* "0x" plus 2 hex chars for each byte plus nul */
-				char found[2 + BTRFS_CSUM_SIZE * 2 + 1];
-				char wanted[2 + BTRFS_CSUM_SIZE * 2 + 1];
+				char found[BTRFS_CSUM_STRING_LEN];
+				char wanted[BTRFS_CSUM_STRING_LEN];
 
 				btrfs_format_csum(csum_type, result, found);
 				btrfs_format_csum(csum_type, (u8 *)buf->data, wanted);
-- 
2.32.0

[PATCH 2/3] btrfs-progs: slightly enhance btrfs_format_csum()

[Qu Wenruo]

- Change it void
  The old one always return csum_size.

- Use snprintf()

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 common/utils.c | 14 ++++++++------
 common/utils.h |  2 +-
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/common/utils.c b/common/utils.c
index e944c43ac40e..7e213146520a 100644
--- a/common/utils.c
+++ b/common/utils.c
@@ -385,18 +385,20 @@ enum btrfs_csum_type parse_csum_type(const char *s)
 	return 0;
 }
 
-int btrfs_format_csum(u16 csum_type, const u8 *data, char *output)
+void btrfs_format_csum(u16 csum_type, const u8 *data, char *output)
 {
 	int i;
+	int cur = 0;
 	const int csum_size = btrfs_csum_type_size(csum_type);
 
-	sprintf(output, "0x");
+	output[0] = '\0';
+	snprintf(output, BTRFS_CSUM_STRING_LEN, "0x");
+	cur += strlen("0x");
 	for (i = 0; i < csum_size; i++) {
-		output += 2;
-		sprintf(output, "%02x", data[i]);
+		snprintf(output + cur, BTRFS_CSUM_STRING_LEN - cur, "%02x",
+			 data[i]);
+		cur += 2;
 	}
-
-	return csum_size;
 }
 
 int get_device_info(int fd, u64 devid,
diff --git a/common/utils.h b/common/utils.h
index 8d5e78071f16..a96bbce9d234 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -46,7 +46,7 @@ enum btrfs_csum_type parse_csum_type(const char *s);
 
 /* 2 for "0x", 2 for each byte, plus nul */
 #define BTRFS_CSUM_STRING_LEN	(2 + 2 * BTRFS_CSUM_SIZE + 1)
-int btrfs_format_csum(u16 csum_type, const u8 *data, char *output);
+void btrfs_format_csum(u16 csum_type, const u8 *data, char *output);
 u64 parse_size_from_string(const char *s);
 u64 parse_qgroupid(const char *p);
 u64 arg_strtou64(const char *str);
-- 
2.32.0

[PATCH 3/3] btrfs-progs: check: output proper csum values for --check-data-csum

[Qu Wenruo]

[BUG]
When running "btrfs check --check-data-csum" on fs with corrupted data,
the error message almost makes no sense:

  $ btrfs check --check-data-csum /dev/test/test
  Opening filesystem to check...
  Checking filesystem on /dev/test/test
  UUID: c31afe0a-55bc-4e7d-aba0-9dfa9ddf8090
  [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 13631488 csum 19 expected csum 152 <<<
  ERROR: errors found in csum tree
  [6/7] checking root refs
  [7/7] checking quota groups skipped (not enabled on this FS)
  found 147456 bytes used, error(s) found
  total csum bytes: 16
  total tree bytes: 131072
  total fs tree bytes: 32768
  total extent tree bytes: 16384
  btree space waste bytes: 124799
  file data blocks allocated: 16384
   referenced 16384

[CAUSE]
We're just outputting the first byte and in decimal, which is completely
different from what we did in kernel space, nor what we did for metadata
csum mismatch.

[FIX]
Use btrfs_format_csum() for btrfs-check to output csum.

Now the result looks much better:

  [5/7] checking csums against data
  mirror 1 bytenr 13631488 csum 0x13fec125 expected csum 0x98757625

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

diff --git a/check/main.c b/check/main.c
index a27efe56eec6..587a6ff362f0 100644
--- a/check/main.c
+++ b/check/main.c
@@ -5812,12 +5812,19 @@ static int check_extent_csums(struct btrfs_root *root, u64 bytenr,
 				read_extent_buffer(eb, (char *)&csum_expected,
 						   csum_offset, csum_size);
 				if (memcmp(result, csum_expected, csum_size) != 0) {
+					char found[BTRFS_CSUM_STRING_LEN];
+					char want[BTRFS_CSUM_STRING_LEN];
+
+
 					csum_mismatch = true;
-					/* FIXME: format of the checksum value */
+					btrfs_format_csum(csum_type, result,
+							  found);
+					btrfs_format_csum(csum_type,
+							  csum_expected, want);
 					fprintf(stderr,
-			"mirror %d bytenr %llu csum %u expected csum %u\n",
+			"mirror %d bytenr %llu csum %s expected csum %s\n",
 						mirror, bytenr + tmp,
-						result[0], csum_expected[0]);
+						found, want);
 				}
 				data_checked += gfs_info->sectorsize;
 			}
-- 
2.32.0

Re: [PATCH 0/3] btrfs-progs: check: enhance the csum mismatch error message

[David Sterba]

On Thu, Aug 26, 2021 at 02:40:33PM +0800, Qu Wenruo wrote:
> This patchset will change the csum mismatch error message for data csum
> mismatch, from the old almost meaningless output:
> 
>   [5/7] checking csums against data
>   mirror 1 bytenr 13631488 csum 19 expected csum 152 <<<
>   ERROR: errors found in csum tree
> 
> To a more human readable output:
> 
>   [5/7] checking csums against data
>   mirror 1 bytenr 13631488 csum 0x13fec125 expected csum 0x98757625
>   ERROR: errors found in csum tree
> 
> Qu Wenruo (3):
>   btrfs-progs: move btrfs_format_csum() to common/utils.[ch]
>   btrfs-progs: slightly enhance btrfs_format_csum()
>   btrfs-progs: check: output proper csum values for --check-data-csum

Nice, added to devel, thanks.

Re: [PATCH 0/3] btrfs-progs: check: enhance the csum mismatch error message

[Qu Wenruo]

On 2021/8/26 下午8:28, David Sterba wrote:
> On Thu, Aug 26, 2021 at 02:40:33PM +0800, Qu Wenruo wrote:
>> This patchset will change the csum mismatch error message for data csum
>> mismatch, from the old almost meaningless output:
>>
>>    [5/7] checking csums against data
>>    mirror 1 bytenr 13631488 csum 19 expected csum 152 <<<
>>    ERROR: errors found in csum tree
>>
>> To a more human readable output:
>>
>>    [5/7] checking csums against data
>>    mirror 1 bytenr 13631488 csum 0x13fec125 expected csum 0x98757625
>>    ERROR: errors found in csum tree
>>
>> Qu Wenruo (3):
>>    btrfs-progs: move btrfs_format_csum() to common/utils.[ch]
>>    btrfs-progs: slightly enhance btrfs_format_csum()
>>    btrfs-progs: check: output proper csum values for --check-data-csum
>
> Nice, added to devel, thanks.
>

BTW, the enhancement is just some fixes found when trying to reproduce
this bug:

https://bugzilla.redhat.com/show_bug.cgi?id=1997477

If someone in the list is able to reproduce this strange bug (btrfs
check --check-data-csum reports strange random data error), please let
me know.

Thanks,
Qu