btrfs-progs: fix csum metadata reservation and add a basic test case for it #820
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[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 e79f18a ("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 08a3bd7 ("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>
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>
kdave
pushed a commit
that referenced
this pull request
Jun 19, 2024
[BUG] 'btrfstune --csum' would always fail for a newly created btrfs: # truncate -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 e79f18a ("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() was incorrect. The 2nd parameter is the *number* of items to be added (if we're deleting items, just pass 1). However commit 08a3bd7 ("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. Pull-request: #820 Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
|
Merged to devel, thanks. |
adam900710
added a commit
that referenced
this pull request
Jun 20, 2024
[BUG] 'btrfstune --csum' would always fail for a newly created btrfs: # truncate -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 e79f18a ("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() was incorrect. The 2nd parameter is the *number* of items to be added (if we're deleting items, just pass 1). However commit 08a3bd7 ("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. Pull-request: #820 Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
kdave
pushed a commit
that referenced
this pull request
Jun 24, 2024
[BUG] 'btrfstune --csum' would always fail for a newly created btrfs: # truncate -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 e79f18a ("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() was incorrect. The 2nd parameter is the *number* of items to be added (if we're deleting items, just pass 1). However commit 08a3bd7 ("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. Pull-request: #820 Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.