[PATCH 0/2] btrfs-progs: fix csum metadata reservation and add a basic test case for itb

[PATCH 0/2] btrfs-progs: fix csum metadata reservation and add a basic test case for itb

[Qu Wenruo]

The current csum conversion is using an incorrect parameter for
btrfs_start_transaction(), which is 16K times larger than the expected
metadata space, thus it always fail with -ENOSPC.

Fix that first, then add a basic test case using the same populate_fs()
from convert test cases, and iterate through all the support csum
algorithms.

Qu Wenruo (2):
  btrfs-progs: change-csum: fix the wrong metadata space reservation
  btrfs-progs: misc-tests: add a test case for basic csum conversion

 .../064-csum-conversion-basic/test.sh         | 32 +++++++++++++++++++
 tune/change-csum.c                            | 22 +++++++++----
 2 files changed, 47 insertions(+), 7 deletions(-)
 create mode 100755 tests/misc-tests/064-csum-conversion-basic/test.sh

--
2.45.2

[PATCH 1/2] btrfs-progs: change-csum: fix the wrong metadata space reservation

[Qu Wenruo]

[BUG]
`btrfstune --csum` would always fail for a newly created btrfs:

 # truncates -s 1G test.img
 # ./mkfs.btrfs -f test.img
 # ./btrsftune --csum xxhash test.img
 WARNING: Experimental build with unstable or unfinished features
 WARNING: Switching checksums is experimental, do not use for valuable data!

 Proceed to switch checksums
 ERROR: failed to start transaction: Unknown error -28
 ERROR: failed to start transaction: No space left on device
 ERROR: failed to generate new data csums: No space left on device
 ERROR: btrfstune failed

[CAUSE]
After commit e79f18a4a75f ("btrfs-progs: introduce a basic metadata free
space reservation check"), btrfs_start_transaction() would check the
metadata space.

But at the time of introduction of csum conversion, the parameter for
btrfs_start_transaction() is incorrect.

The 2nd parameter is the *number* of items to be added (if we're
deleting items, just pass 1).
However commit 08a3bd7694b6 ("btrfs-progs: tune: add the ability to
generate new data checksums") is using the item size, not the number of
items to be added.

This means we're passing a number 8 * nodesize times larger than the
original size, no wonder we would error out with -ENOSPC.

[FIX]
Use proper calcuation to convert the new csum item size to number of
leaves needed, and double it just in case.

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

diff --git a/tune/change-csum.c b/tune/change-csum.c
index f5fc3c7f32cb..371e0aa76fc1 100644
--- a/tune/change-csum.c
+++ b/tune/change-csum.c
@@ -224,14 +224,26 @@ out:
  * item.
  */
 #define CSUM_CHANGE_BYTES_THRESHOLD	(SZ_2M)
+
+static unsigned int calc_csum_change_nr_items(struct btrfs_fs_info *fs_info,
+					      u16 new_csum_type)
+{
+	const u32 new_csum_size = btrfs_csum_type_size(new_csum_type);
+	const u32 csum_item_size = CSUM_CHANGE_BYTES_THRESHOLD /
+				   fs_info->sectorsize * new_csum_size;
+
+	return round_up(csum_item_size, fs_info->nodesize) / fs_info->nodesize * 2;
+}
+
 static int generate_new_data_csums_range(struct btrfs_fs_info *fs_info, u64 start,
 					 u16 new_csum_type)
 {
+	const unsigned int nr_items = calc_csum_change_nr_items(fs_info,
+								new_csum_type);
 	struct btrfs_root *csum_root = btrfs_csum_root(fs_info, 0);
 	struct btrfs_trans_handle *trans;
 	struct btrfs_path path = { 0 };
 	struct btrfs_key key;
-	const u32 new_csum_size = btrfs_csum_type_size(new_csum_type);
 	void *csum_buffer;
 	u64 converted_bytes = 0;
 	u64 last_csum;
@@ -248,9 +260,7 @@ static int generate_new_data_csums_range(struct btrfs_fs_info *fs_info, u64 star
 	if (!csum_buffer)
 		return -ENOMEM;
 
-	trans = btrfs_start_transaction(csum_root,
-			CSUM_CHANGE_BYTES_THRESHOLD / fs_info->sectorsize *
-			new_csum_size);
+	trans = btrfs_start_transaction(csum_root, nr_items);
 	if (IS_ERR(trans)) {
 		ret = PTR_ERR(trans);
 		errno = -ret;
@@ -306,9 +316,7 @@ static int generate_new_data_csums_range(struct btrfs_fs_info *fs_info, u64 star
 				return -EUCLEAN;
 			if (ret < 0)
 				goto out;
-			trans = btrfs_start_transaction(csum_root,
-					CSUM_CHANGE_BYTES_THRESHOLD /
-					fs_info->sectorsize * new_csum_size);
+			trans = btrfs_start_transaction(csum_root, nr_items);
 			if (IS_ERR(trans)) {
 				ret = PTR_ERR(trans);
 				goto out;
-- 
2.45.2

[PATCH 2/2] btrfs-progs: misc-tests: add a test case for basic csum conversion

[Qu Wenruo]

The new test case would:

- Create a btrfs with crc32c csum
- Populate the fs
- Convert the fs to the following csums:
  * xxhash
  * blake2
  * sha256
  * crc32c

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 .../064-csum-conversion-basic/test.sh         | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100755 tests/misc-tests/064-csum-conversion-basic/test.sh

diff --git a/tests/misc-tests/064-csum-conversion-basic/test.sh b/tests/misc-tests/064-csum-conversion-basic/test.sh
new file mode 100755
index 000000000000..2f8be0e9b324
--- /dev/null
+++ b/tests/misc-tests/064-csum-conversion-basic/test.sh
@@ -0,0 +1,32 @@
+#!/bin/bash
+#
+# Verify the csum conversion works as expected.
+#
+source "$TEST_TOP/common" || exit
+source "$TEST_TOP/common.convert" || exit
+
+check_experimental_build
+setup_root_helper
+prepare_test_dev
+
+convert_to_csum()
+{
+	local new_csum=$1
+
+	run_check "$TOP/btrfstune" --csum "$new_csum" "$TEST_DEV"
+	run_check "$TOP/btrfs" check --check-data-csum "$TEST_DEV"
+}
+
+run_check_mkfs_test_dev --csum crc32c
+
+# We only mount the fs 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
+
+convert_to_csum xxhash
+convert_to_csum blake2
+convert_to_csum sha256
+convert_to_csum crc32c
-- 
2.45.2

Re: [PATCH 0/2] btrfs-progs: fix csum metadata reservation and add a basic test case for itb

[David Sterba]

On Tue, Jun 18, 2024 at 04:20:47PM +0930, Qu Wenruo wrote:
> The current csum conversion is using an incorrect parameter for
> btrfs_start_transaction(), which is 16K times larger than the expected
> metadata space, thus it always fail with -ENOSPC.
> 
> Fix that first, then add a basic test case using the same populate_fs()
> from convert test cases, and iterate through all the support csum
> algorithms.
> 
> Qu Wenruo (2):
>   btrfs-progs: change-csum: fix the wrong metadata space reservation
>   btrfs-progs: misc-tests: add a test case for basic csum conversion

Added to devel, thanks.