[PATCH v3 0/3] btrfs-progs: mkfs free-space-info bug

[PATCH v3 0/3] btrfs-progs: mkfs free-space-info bug

[Leo Martins]

Currently mkfs creates a few block-groups to bootstrap the filesystem.
Along with these block-groups some free-space-infos are created. When
the block-group cleanup happens the block-groups are deleted and the
free-space-infos are not. This patch set introduces a fix that deletes
the free-space-infos when the block-groups are deleted.

When implementing a test for my changes I found that there was already
some code in free-space-tree.c that attempts to verify that all
free-space-infos correspond to a block-group. This code only checks if
there exists a block-group that has an objectid >= free-space-info's
objectid. I added an additional check to make sure that the block-group
actually matches the free-space-info's objectid and offset.

Making this change to fsck will cause all filesystems that were created
using mkfs.btrfs to warn that there is some free-space-info that doesn't
correspond to a block-group. I've softened the language and included
instructions on how to fix the issue.

CHANGELOG:
v3:
- Added patch 3/3 to add free-space-info checking in tree-checker.
- Softened the warning in patch 2/3

Leo Martins (3):
  btrfs-progs: delete free space when deleting block group
  btrfs-progs: check free space maps to block group
  btrfs-progs: free-space-info tree-checker

 common/clear-cache.c            | 11 +++++++++--
 kernel-shared/extent-tree.c     | 10 ++++++++++
 kernel-shared/free-space-tree.c |  3 +++
 kernel-shared/tree-checker.c    | 29 +++++++++++++++++++++++++++++
 4 files changed, 51 insertions(+), 2 deletions(-)

-- 
2.43.5

[PATCH v3 1/3] btrfs-progs: remove block group free space

[Leo Martins]

In the upstream equivalent of btrfs_remove_block_group(), the
function remove_block_group_free_space() is called to delete free
spaces associated with the block group being freed. However, this
function is defined in btrfs-progs but not called anywhere.

To address this issue, I added a call to
remove_block_group_free_space() in btrfs_remove_block_group(). This
ensures that the free spaces are properly deleted when a block group
is removed.

I also added a check to remove_block_group_free_space to make sure that
free-space-tree is enabled.

Signed-off-by: Leo Martins <loemra.dev@gmail.com>
---
CHANGELOG:
v3:
- No change
v2:
- Added the check to make sure that free-space-tree is enabled
---
 kernel-shared/extent-tree.c     | 10 ++++++++++
 kernel-shared/free-space-tree.c |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/kernel-shared/extent-tree.c b/kernel-shared/extent-tree.c
index af04b9ea..f44126e2 100644
--- a/kernel-shared/extent-tree.c
+++ b/kernel-shared/extent-tree.c
@@ -3536,6 +3536,16 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
 		return ret;
 	}
 
+	/* delete free space items associated with this block group */
+	ret = remove_block_group_free_space(trans, block_group);
+	if (ret < 0) {
+		fprintf(stderr,
+			"failed to remove free space associated with block group for [%llu,%llu)\n",
+			bytenr, bytenr + len);
+		btrfs_unpin_extent(fs_info, bytenr, len);
+		return ret;
+	}
+
 	/* Now release the block_group_cache */
 	ret = free_block_group_cache(trans, fs_info, bytenr, len);
 	btrfs_unpin_extent(fs_info, bytenr, len);
diff --git a/kernel-shared/free-space-tree.c b/kernel-shared/free-space-tree.c
index 81fd57b8..61263f94 100644
--- a/kernel-shared/free-space-tree.c
+++ b/kernel-shared/free-space-tree.c
@@ -1171,6 +1171,9 @@ int remove_block_group_free_space(struct btrfs_trans_handle *trans,
 	int done = 0, nr;
 	int ret;
 
+	if (!btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE))
+		return 0;
+
 	path = btrfs_alloc_path();
 	if (!path) {
 		ret = -ENOMEM;
-- 
2.43.5

[PATCH v3 2/3] btrfs-progs: check free space maps to block group

[Leo Martins]

Check that the block-group that is found matches the objectid and offset
of the free-space-info. Without this the check only verifies that there
is some block-group that exists with objectid >= free-space-info's
objectid.

I have softened the language of the warning and included instructions on
how to fix the problem. This can be done in a couple of ways:
- btrfs check --repair
- btrfs rescue clear-space-cache v2

I chose to include btrfs rescue as it is more targeted.

Signed-off-by: Leo Martins <loemra.dev@gmail.com>
---
CHANGELOG:
v3:
- softened the warning and added instructions
---
 common/clear-cache.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/common/clear-cache.c b/common/clear-cache.c
index 6493866d..6b362f64 100644
--- a/common/clear-cache.c
+++ b/common/clear-cache.c
@@ -165,9 +165,16 @@ static int check_free_space_tree(struct btrfs_root *root)
 		}
 
 		bg = btrfs_lookup_first_block_group(fs_info, key.objectid);
-		if (!bg) {
+		if (!bg || key.objectid != bg->start ||
+		    key.offset != bg->length) {
 			fprintf(stderr,
-		"We have a space info key for a block group that doesn't exist\n");
+				"We have a space info key [%llu %u %llu] for a block group that "
+				"doesn't exist.\n",
+				key.objectid, key.type, key.offset);
+			fprintf(stderr,
+				"This is likely due to a minor bug in mkfs.btrfs that doesn't properly\n"
+				"cleanup free spaces and can be fixed using btrfs rescue "
+				"clear-space-cache v2\n");
 			ret = -EINVAL;
 			goto out;
 		}
-- 
2.43.5

[PATCH v3 3/3] btrfs-progs: free-space-info tree-checker

[Leo Martins]

Add a new check in check_leaf_item for btrfs_free_space_info. This check
performs exactly the same check that is performed in btrfs check. That
is it searches for the block group that the current free-space-info
belogns to and warns if there is none. When I was testing I found that
sometimes this would be called before the block group cache was
initialized leading to incorrect warnings so I added a check to make
sure that the cache was initialized.

I also chose to not return an error since this bug does not really affect
the ability of the system to function properly.

I'm still not convinced that this tree-checker is helpful or necessary
so if anyone has any opinions I would love to hear them! If this is
deemed helpful I will send out another patch to add this check to the
kernel tree-checker.

Signed-off-by: Leo Martins <loemra.dev@gmail.com>
---
 kernel-shared/tree-checker.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/kernel-shared/tree-checker.c b/kernel-shared/tree-checker.c
index 0be8f022..cf2e389e 100644
--- a/kernel-shared/tree-checker.c
+++ b/kernel-shared/tree-checker.c
@@ -1737,6 +1737,32 @@ static int check_raid_stripe_extent(const struct extent_buffer *leaf,
 	return 0;
 }
 
+static int check_free_space_info_item(const struct extent_buffer *leaf,
+				      const struct btrfs_key *key, int slot)
+{
+	struct btrfs_fs_info *fs_info = leaf->fs_info;
+	struct btrfs_block_group *bg;
+
+	/* block_group_cache is uninitialized at this point */
+	if (!fs_info->block_group_cache_tree.rb_node)
+		return 0;
+
+	bg = btrfs_lookup_first_block_group(fs_info, key->objectid);
+	if (unlikely(!bg || key->objectid != bg->start ||
+		     key->offset != bg->length)) {
+		generic_err(
+			leaf, slot,
+			"We have a space info key [%llu %u %llu] for a block group that "
+			"doesn't exist.\n"
+			"This is likely due to a minor bug in mkfs.btrfs that doesn't properly\n"
+			"cleanup free spaces and can be fixed using btrfs rescue "
+			"clear-space-cache v2\n",
+			key->objectid, key->type, key->offset);
+	}
+
+	return 0;
+}
+
 /*
  * Common point to switch the item-specific validation.
  */
@@ -1798,6 +1824,9 @@ static enum btrfs_tree_block_status check_leaf_item(struct extent_buffer *leaf,
 	case BTRFS_RAID_STRIPE_KEY:
 		ret = check_raid_stripe_extent(leaf, key, slot);
 		break;
+	case BTRFS_FREE_SPACE_INFO_KEY:
+		ret = check_free_space_info_item(leaf, key, slot);
+		break;
 	}
 
 	if (ret)
-- 
2.43.5

Re: [PATCH v3 2/3] btrfs-progs: check free space maps to block group

[Mark Harmstone]

Replying to this, as I've just run into this problem myself, and these patches
weren't accepted.

Patch 1 is fine.

For patch 2, there's an easier way of doing this: replace btrfs_lookup_first_block_group()
with btrfs_lookup_block_group(), which almost certainly was what was meant in
the first place.
But... doing this will cause pretty much all of the existing tests to fail,
so you'll have to fix them all in the same patch. I suspect this was the
reason that the patches didn't get accepted.

For patch 3, I wouldn't bother. I know there's some inevitable drift between
kernel-shared and the actual kernel, but adding a tree-checker to one and not
the other seems a bit too far. And if you were to add it to the kernel, that'd
add a runtime overhead for something harmless.

I suggest that when you resubmit these patches, you do it via a GitHub PR.
That way you won't have megabytes and megabytes of fixed tests sent to the
mailing list, the CI will run the tests for you automatically, and it's
less likely that it'll get forgotten about.

Mark

On 08/10/2024 1.27 am, Leo Martins wrote:
> Check that the block-group that is found matches the objectid and offset
> of the free-space-info. Without this the check only verifies that there
> is some block-group that exists with objectid >= free-space-info's
> objectid.
> 
> I have softened the language of the warning and included instructions on
> how to fix the problem. This can be done in a couple of ways:
> - btrfs check --repair
> - btrfs rescue clear-space-cache v2
> 
> I chose to include btrfs rescue as it is more targeted.
> 
> Signed-off-by: Leo Martins <loemra.dev@gmail.com>
> ---
> CHANGELOG:
> v3:
> - softened the warning and added instructions
> ---
>   common/clear-cache.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/common/clear-cache.c b/common/clear-cache.c
> index 6493866d..6b362f64 100644
> --- a/common/clear-cache.c
> +++ b/common/clear-cache.c
> @@ -165,9 +165,16 @@ static int check_free_space_tree(struct btrfs_root *root)
>   		}
>   
>   		bg = btrfs_lookup_first_block_group(fs_info, key.objectid);
> -		if (!bg) {
> +		if (!bg || key.objectid != bg->start ||
> +		    key.offset != bg->length) {
>   			fprintf(stderr,
> -		"We have a space info key for a block group that doesn't exist\n");
> +				"We have a space info key [%llu %u %llu] for a block group that "
> +				"doesn't exist.\n",
> +				key.objectid, key.type, key.offset);
> +			fprintf(stderr,
> +				"This is likely due to a minor bug in mkfs.btrfs that doesn't properly\n"
> +				"cleanup free spaces and can be fixed using btrfs rescue "
> +				"clear-space-cache v2\n");
>   			ret = -EINVAL;
>   			goto out;
>   		}