[PATCH 0/2] btrfs-progs: rely on "btrfstune --csum" to replace "btrfs check --init-csum-tree"

[PATCH 0/2] btrfs-progs: rely on "btrfstune --csum" to replace "btrfs check --init-csum-tree"

[Qu Wenruo]

We had a report that "btrfs check --init-csum-tree" corrupted a
seemingly fine btrfs (which can originally pass "btrfs check
--readonly").

The root cause is in how we rebuild the csum tree, in the btrfs check
code, we screw up the csum tree root, then rely on the extent tree
repair code to finish the damage we introduced.

This can lead to unexpected corner cases, if the fs is already fine,
there is no need for such risky move.

Considering there are valid ways to cause data csum mismatch (mostly
O_DIRECT and modifying the buffer when it's still under writeback), some
users expect to use "btrfs check --init-csum-tree" to fix the csum
mismatch, which can lead to the same corruption.

Instead this patchset would recommend the end users to go "btrfstune
--csum", as it is way less risky by its design, and no more damage to
the fs caused by ourselves.

I hope we can completely go that direction when the "btrfstune --csum"
option is moved out of experimental features.

Qu Wenruo (2):
  btrfs-progs: tune: allow --csum to rebuild csum tree
  btrfs-progs: check: add advice to avoid --init-csum-tree

 check/main.c       |  9 +++++++++
 tune/change-csum.c | 35 +++++++++++++++++++++++++----------
 tune/main.c        |  3 ++-
 3 files changed, 36 insertions(+), 11 deletions(-)

--
2.41.0

[PATCH 1/2] btrfs-progs: tune: allow --csum to rebuild csum tree

[Qu Wenruo]

[PROBLEM]
Currently "btrfs check --init-csum-tree" goes a very tricky way to
rebuild csum tree, by wiping the csum tree root and let the extent tree
repair code to handle the remaining part of the old csum tree.

We already have a report that such method has caused data corruption on
a seemingly fine btrfs (the report ran "btrfs check --readonly" first,
which has no error reported).

[ENHANCEMENT]
On the other hand, we have a new experimental feature, "btrfstune
--csum", which would go a much safer way to convert data csum, by
inserting new data csum items using the new csum type, then replace the
old csum items with the new ones.

This is way safer, and we have even tested the interrupted cases of such
conversion, and if the fs is fine in the first place, we won't rely on
extent tree repair code at all.

This patch would:

- Fix the spelling of check_csum_change_requirement()

- Remove the csum type check in check_csum_change_requirement()

- Do not verify data csum if we're rebuilding using the same csum type
  As we may want to rebuilt the csum tree to fix the csum mismatch.

  This would require a new flag, VERIFY_SKIP_CSUM for
  read_verify_one_data_sector().

- Skip metadata csum rewrite completely if csum type matches

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tune/change-csum.c | 35 +++++++++++++++++++++++++----------
 tune/main.c        |  3 ++-
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/tune/change-csum.c b/tune/change-csum.c
index e7bc22526fd1..b5e70c9e4f80 100644
--- a/tune/change-csum.c
+++ b/tune/change-csum.c
@@ -76,12 +76,6 @@ static int check_csum_change_requreiment(struct btrfs_fs_info *fs_info, u16 new_
 		error("running dev-replace detected, please finish or cancel it.");
 		return -EINVAL;
 	}
-
-	if (fs_info->csum_type == new_csum_type) {
-		error("the fs is already using csum type %s (%u)",
-		      btrfs_super_csum_name(new_csum_type), new_csum_type);
-		return -EINVAL;
-	}
 	return 0;
 }
 
@@ -120,13 +114,18 @@ static int get_last_csum_bytenr(struct btrfs_fs_info *fs_info, u64 *result)
 	return 0;
 }
 
+#define VERIFY_OUTPUT_ERROR	(1 << 0)
+#define VERIFY_SKIP_CSUM	(1 << 1)
+
 static int read_verify_one_data_sector(struct btrfs_fs_info *fs_info,
 				       u64 logical, void *data_buf,
 				       const void *old_csums, u16 old_csum_type,
-				       bool output_error)
+				       unsigned int flags)
 {
 	const u32 sectorsize = fs_info->sectorsize;
 	int num_copies = btrfs_num_copies(fs_info, logical, sectorsize);
+	const bool output_error = flags & VERIFY_OUTPUT_ERROR;
+	const bool skip_csum = flags & VERIFY_SKIP_CSUM;
 	bool found_good = false;
 
 	for (int mirror = 1; mirror <= num_copies; mirror++) {
@@ -141,6 +140,11 @@ static int read_verify_one_data_sector(struct btrfs_fs_info *fs_info,
 			error("failed to read logical %llu: %m", logical);
 			continue;
 		}
+		if (skip_csum) {
+			found_good = true;
+			break;
+		}
+
 		btrfs_csum_data(fs_info, fs_info->csum_type, data_buf, csum_has,
 				sectorsize);
 		if (memcmp(csum_has, old_csums, fs_info->csum_size) == 0) {
@@ -169,16 +173,20 @@ static int generate_new_csum_range(struct btrfs_trans_handle *trans,
 	const u32 sectorsize = fs_info->sectorsize;
 	int ret = 0;
 	void *buf;
+	unsigned int flags = VERIFY_OUTPUT_ERROR;
 
 	buf = malloc(fs_info->sectorsize);
 	if (!buf)
 		return -ENOMEM;
 
+	/* We're rebuilding the csum tree, no need to verify the old data csum. */
+	if (fs_info->csum_type == new_csum_type)
+		flags |= VERIFY_SKIP_CSUM;
+
 	for (u64 cur = logical; cur < logical + length; cur += sectorsize) {
 		ret = read_verify_one_data_sector(fs_info, cur, buf, old_csums +
 				(cur - logical) / sectorsize * fs_info->csum_size,
-				fs_info->csum_type, true);
-
+				fs_info->csum_type, flags);
 		if (ret < 0) {
 			error("failed to recover a good copy for data at logical %llu",
 			      logical);
@@ -565,6 +573,13 @@ static int change_meta_csums(struct btrfs_fs_info *fs_info, u16 new_csum_type)
 		error("failed to update super flags: %m");
 	}
 
+	/*
+	 * The new csum type matches the old one, it's to rebuild data csum.
+	 * No need to change metadata csum.
+	 */
+	if (fs_info->csum_type == new_csum_type)
+		goto out;
+
 	/*
 	 * Disable metadata csum checks first, as we may hit tree blocks with
 	 * either old or new csums.
@@ -811,7 +826,7 @@ static int determine_csum_type(struct btrfs_fs_info *fs_info, u64 logical,
 	if (!buf)
 		return -ENOMEM;
 	ret = read_verify_one_data_sector(fs_info, logical, buf, csum_expected,
-					  csum_type, false);
+					  csum_type, 0);
 	if (ret < 0)
 		ret = 1;
 	free(buf);
diff --git a/tune/main.c b/tune/main.c
index 73d09c34a897..b611d3171918 100644
--- a/tune/main.c
+++ b/tune/main.c
@@ -121,7 +121,8 @@ static const char * const tune_usage[] = {
 #if EXPERIMENTAL
 	"",
 	"EXPERIMENTAL FEATURES:",
-	OPTLINE("--csum CSUM", "switch checksum for data and metadata to CSUM"),
+	OPTLINE("--csum CSUM", "switch checksum for data and metadata to CSUM, "
+			"would only rebuild csum tree if CSUM is the same as the existing one"),
 #endif
 	NULL
 };
-- 
2.41.0

[PATCH 2/2] btrfs-progs: check: add advice to avoid --init-csum-tree

[Qu Wenruo]

We already had a report about "btrfs check --init-csum-tree" screwing up
a seemingly fine btrfs (which can pass "btrfs check --readonly" without
error).

This is mostly due to the fact that --init-csum-tree just screw up the
csum tree root, and rely on extent tree repair code to repair the damage
caused by ourselves.

This patch would recommend the end users to go "btrfstune --csum" if
that option is present.

This has some extra benefits:

- More protection/tests on interrupred conversion

- No unrelated changes to subvolume and extent trees
  But this requires the original fs to be mostly fine.

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

diff --git a/check/main.c b/check/main.c
index d66e10e8fd4d..9a52463269b1 100644
--- a/check/main.c
+++ b/check/main.c
@@ -10153,6 +10153,15 @@ static int cmd_check(const struct cmd_struct *cmd, int argc, char **argv)
 		printf("\tsome software or hardware bugs can fatally damage a volume.\n");
 		printf("\tThe operation will start in %d seconds.\n", delay);
 		printf("\tUse Ctrl-C to stop it.\n");
+
+#if EXPERIMENTAL
+		if (init_csum_tree) {
+			printf("\n");
+			printf("\t--init-csum-tree is considered dangerous, if your fs is fine,\n");
+			printf("\tplease use \"btrfstune --csum\" instead, which is considered\n");
+			printf("\tmuch safer\n");
+		}
+#endif
 		while (delay) {
 			printf("%2d", delay--);
 			fflush(stdout);
-- 
2.41.0

Re: [PATCH 0/2] btrfs-progs: rely on "btrfstune --csum" to replace "btrfs check --init-csum-tree"

[David Sterba]

On Tue, Aug 22, 2023 at 03:15:16PM +0800, Qu Wenruo wrote:
> We had a report that "btrfs check --init-csum-tree" corrupted a
> seemingly fine btrfs (which can originally pass "btrfs check
> --readonly").
> 
> The root cause is in how we rebuild the csum tree, in the btrfs check
> code, we screw up the csum tree root, then rely on the extent tree
> repair code to finish the damage we introduced.
> 
> This can lead to unexpected corner cases, if the fs is already fine,
> there is no need for such risky move.
> 
> Considering there are valid ways to cause data csum mismatch (mostly
> O_DIRECT and modifying the buffer when it's still under writeback), some
> users expect to use "btrfs check --init-csum-tree" to fix the csum
> mismatch, which can lead to the same corruption.
> 
> Instead this patchset would recommend the end users to go "btrfstune
> --csum", as it is way less risky by its design, and no more damage to
> the fs caused by ourselves.
> 
> I hope we can completely go that direction when the "btrfstune --csum"
> option is moved out of experimental features.
> 
> Qu Wenruo (2):
>   btrfs-progs: tune: allow --csum to rebuild csum tree
>   btrfs-progs: check: add advice to avoid --init-csum-tree

We can go farther and label the option as dangerous in a more visible
way, currently there's no warning and it's marked as dangerous only in
documentation and not even in the help text. I'd like to deprecate the
option and remove it from 'check' completely.

Adding support to btrfstune --csum to rebuild the checksum tree is OK.