[PATCH v3 0/3] btrfs-progs: btrfs-progs: csum-change enhancement

[PATCH v3 0/3] btrfs-progs: btrfs-progs: csum-change enhancement

[Qu Wenruo]

[CHANGELOG]
v3:
- Rebased to the latest devel branch
- Migrate to use "btrfs --version" to detect features

v2:
- Enhance the error injection detection
  Now instead of plain run_mustfail, we do check both the failure and
  the stderr.
  Only when the csum-change failed and stderr includes the injection
  cookie output, we know it's really the injection causing error.

The first two patches are small enhancement and bugfix:

- Fix a missing error handling
- Do multi-transaction csum deletion and rename
  Or we can generate GiB or even TiB level of dirty metadata.

Finally introduce a basic error injection based test case, which will:

- Check if we have error injection first
- Inject error at the end of data csum generation
- Make sure resume from above situation is correct

I'm not adding extra injections because I believe there would definitely
be corner cases that need to be fixed.

Qu Wenruo (3):
  btrfs-progs: csum-change: add leaf based threshold
  btrfs-progs: tests: use feature output from "btrfs --version"
  btrfs-progs: misc-tests: add a basic resume test using error injection

 tests/common                                  | 13 ++--
 .../065-csum-conversion-inject/test.sh        | 45 ++++++++++++
 tune/change-csum.c                            | 68 ++++++++++++++-----
 3 files changed, 105 insertions(+), 21 deletions(-)
 create mode 100755 tests/misc-tests/065-csum-conversion-inject/test.sh

--
2.45.2

[PATCH v3 1/3] btrfs-progs: csum-change: add leaf based threshold

[Qu Wenruo]

For "btrfstune --csum", currently we do the following operations in just
one transaction for each:

- Delete old data csums
- Change new data csums objectid

Both opeartion can modify up to GiB or even TiB level of metadata, doing
them in just one transaction is definitely going to cause problems.

This patch adds a leaf number based threshold (32 leaves), after
modifying/deleteing this many leaves, we commit a transaction to avoid
huge amount of dirty leaves piling up.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tune/change-csum.c | 68 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 52 insertions(+), 16 deletions(-)

diff --git a/tune/change-csum.c b/tune/change-csum.c
index 0f95cdb25533..66fdd207335c 100644
--- a/tune/change-csum.c
+++ b/tune/change-csum.c
@@ -371,29 +371,48 @@ static int generate_new_data_csums(struct btrfs_fs_info *fs_info, u16 new_csum_t
 	return generate_new_data_csums_range(fs_info, 0, new_csum_type);
 }
 
+/* After deleting/modifying this many leaves, commit a transaction. */
+#define CSUM_CHANGE_LEAVES_THRESHOLD	32
+
 static int delete_old_data_csums(struct btrfs_fs_info *fs_info)
 {
 	struct btrfs_root *csum_root = btrfs_csum_root(fs_info, 0);
-	struct btrfs_trans_handle *trans;
+	struct btrfs_trans_handle *trans = 0;
 	struct btrfs_path path = { 0 };
 	struct btrfs_key last_key;
+	unsigned int deleted_leaves = 0;
 	int ret;
 
 	last_key.objectid = BTRFS_EXTENT_CSUM_OBJECTID;
 	last_key.type = BTRFS_EXTENT_CSUM_KEY;
 	last_key.offset = (u64)-1;
 
-	trans = btrfs_start_transaction(csum_root, 1);
-	if (IS_ERR(trans)) {
-		ret = PTR_ERR(trans);
-		errno = -ret;
-		error("failed to start transaction to delete old data csums: %m");
-		return ret;
-	}
 	while (true) {
 		int start_slot;
 		int nr;
 
+		if (deleted_leaves >= CSUM_CHANGE_LEAVES_THRESHOLD) {
+			assert(trans);
+			ret = btrfs_commit_transaction(trans, csum_root);
+			if (ret < 0) {
+				errno = -ret;
+				error(
+		"failed to commit transaction to delete old data csums: %m");
+				return ret;
+			}
+			trans = NULL;
+			deleted_leaves = 0;
+		}
+		if (!trans) {
+			trans = btrfs_start_transaction(csum_root, 1);
+			if (IS_ERR(trans)) {
+				ret = PTR_ERR(trans);
+				errno = -ret;
+				error(
+		"failed to start transaction to delete old data csums: %m");
+				return ret;
+			}
+		}
 		ret = btrfs_search_slot(trans, csum_root, &last_key, &path, -1, 1);
 		if (ret < 0) {
 			errno = -ret;
@@ -428,6 +447,7 @@ static int delete_old_data_csums(struct btrfs_fs_info *fs_info)
 			break;
 		}
 		btrfs_release_path(&path);
+		deleted_leaves++;
 	}
 	btrfs_release_path(&path);
 	if (ret < 0)
@@ -445,9 +465,10 @@ static int delete_old_data_csums(struct btrfs_fs_info *fs_info)
 static int change_csum_objectids(struct btrfs_fs_info *fs_info)
 {
 	struct btrfs_root *csum_root = btrfs_csum_root(fs_info, 0);
-	struct btrfs_trans_handle *trans;
+	struct btrfs_trans_handle *trans = NULL;
 	struct btrfs_path path = { 0 };
 	struct btrfs_key last_key;
+	unsigned int changed_leaves = 0;
 	u64 super_flags;
 	int ret = 0;
 
@@ -455,17 +476,32 @@ static int change_csum_objectids(struct btrfs_fs_info *fs_info)
 	last_key.type = BTRFS_EXTENT_CSUM_KEY;
 	last_key.offset = (u64)-1;
 
-	trans = btrfs_start_transaction(csum_root, 1);
-	if (IS_ERR(trans)) {
-		ret = PTR_ERR(trans);
-		errno = -ret;
-		error("failed to start transaction to change csum objectids: %m");
-		return ret;
-	}
 	while (true) {
 		struct btrfs_key found_key;
 		int nr;
 
+		if (changed_leaves >= CSUM_CHANGE_LEAVES_THRESHOLD) {
+			assert(trans);
+			ret = btrfs_commit_transaction(trans, csum_root);
+			if (ret < 0) {
+				errno = -ret;
+				error(
+		"failed to commit transaction to change data csum objectid: %m");
+				return ret;
+			}
+			trans = NULL;
+			changed_leaves = 0;
+		}
+		if (!trans) {
+			trans = btrfs_start_transaction(csum_root, 1);
+			if (IS_ERR(trans)) {
+				ret = PTR_ERR(trans);
+				errno = -ret;
+				error(
+		"failed to start transaction to change csum objectids: %m");
+				return ret;
+			}
+		}
 		ret = btrfs_search_slot(trans, csum_root, &last_key, &path, 0, 1);
 		if (ret < 0)
 			goto out;
-- 
2.45.2

[PATCH v3 2/3] btrfs-progs: tests: use feature output from "btrfs --version"

[Qu Wenruo]

With the new feature matrix output in "btrfs --version" there is no need
to do the config.h hack to determine if we have certain feature.

This would provide a more reliable way to detect features.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/common | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/tests/common b/tests/common
index e996b35af787..4800ef8e77c9 100644
--- a/tests/common
+++ b/tests/common
@@ -12,11 +12,7 @@ _test_config()
 {
 	local feature="$1"
 
-	if [ ! -f "$TOP/include/config.h" ]; then
-		echo "include/config.h not exists"
-		exit 1
-	fi
-	if grep -q "$feature.*1" "${TOP}/include/config.h"; then
+	if "$TOP/btrfs" --version | grep -q "+${feature}"; then
 		return 0
 	fi
 	return 1
-- 
2.45.2

[PATCH v3 3/3] btrfs-progs: misc-tests: add a basic resume test using error injection

[Qu Wenruo]

The new test case does:

- Make sure the build has error injection support
  This is done by checking "btrfs --version" output.

- Inject error at the last commit transaction of new data csum
  generation

- Resume the csum conversion and make sure it works

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/common                                  |  7 +++
 .../065-csum-conversion-inject/test.sh        | 45 +++++++++++++++++++
 2 files changed, 52 insertions(+)
 create mode 100755 tests/misc-tests/065-csum-conversion-inject/test.sh

diff --git a/tests/common b/tests/common
index 4800ef8e77c9..f6b68a418087 100644
--- a/tests/common
+++ b/tests/common
@@ -402,6 +402,13 @@ check_regular_build()
 	fi
 }
 
+check_injection()
+{
+	if ! _test_config "INJECT"; then
+		_not_run "This test requires error injection support (make D=1)"
+	fi
+}
+
 check_prereq()
 {
 	# Internal tools for testing, not shipped with the package
diff --git a/tests/misc-tests/065-csum-conversion-inject/test.sh b/tests/misc-tests/065-csum-conversion-inject/test.sh
new file mode 100755
index 000000000000..715349c4d403
--- /dev/null
+++ b/tests/misc-tests/065-csum-conversion-inject/test.sh
@@ -0,0 +1,45 @@
+#!/bin/bash
+# Verify the csum conversion can still resume after an interruption
+
+source "$TEST_TOP/common" || exit
+source "$TEST_TOP/common.convert" || exit
+
+check_experimental_build
+check_injection
+setup_root_helper
+prepare_test_dev
+
+test_resume_data_csum_generation()
+{
+	local new_csum="$1"
+	local tmp=$(_mktemp "csum-convert")
+
+	# Error at the end of the data csum generation.
+	export INJECT="0x4de02239"
+	run_mustfail_stdout "error injection not working" \
+		"$TOP/btrfstune" --csum "$new_csum" "$TEST_DEV" &> $tmp
+	cat "$tmp" >> "$RESULTS"
+	if ! grep -q "$INJECT" "$tmp"; then
+		rm -f -- "$tmp"
+		_fail "csum conversion failed to unexpected reason"
+	fi
+	rm -f -- "$tmp"
+	unset INJECT
+	run_check "$TOP/btrfstune" --csum "$new_csum" "$TEST_DEV"
+	run_check "$TOP/btrfs" check --check-data-csum "$TEST_DEV"
+}
+
+check_injection
+
+run_check_mkfs_test_dev --csum crc32c
+
+# We only mount the filesystem once to populate its contents, later one we
+# would never mount the fs (to reduce the dependency on kernel features).
+run_check_mount_test_dev
+populate_fs
+run_check_umount_test_dev
+
+test_resume_data_csum_generation xxhash
+test_resume_data_csum_generation blake2
+test_resume_data_csum_generation sha256
+test_resume_data_csum_generation crc32c
-- 
2.45.2