[PATCH v2 0/2] btrfs-progs: mkfs free-space-info bug

[PATCH v2 0/2] 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. This seems like a bad idea, I considered
adding to the message to signify that this is not a big issue and maybe
point them to this patchset to get the fix. Open to ideas on how to
handle this.

Leo Martins (2):
  btrfs-progs: delete free space when deleting block group
  btrfs-progs: check free space maps to block group

 common/clear-cache.c            |  3 ++-
 kernel-shared/extent-tree.c     | 10 ++++++++++
 kernel-shared/free-space-tree.c |  3 +++
 3 files changed, 15 insertions(+), 1 deletion(-)

-- 
2.43.5

[PATCH v2 1/2] 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:
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..df704e4d 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 v2 2/2] 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.

To reiterate what I mentioned in the cover letter this will cause all
filesystems created with mkfs.btrfs to print this warning.

Signed-off-by: Leo Martins <loemra.dev@gmail.com>
---
 common/clear-cache.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/common/clear-cache.c b/common/clear-cache.c
index 6493866d..09ef5741 100644
--- a/common/clear-cache.c
+++ b/common/clear-cache.c
@@ -165,7 +165,8 @@ 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");
 			ret = -EINVAL;
-- 
2.43.5

Re: [PATCH v2 0/2] btrfs-progs: mkfs free-space-info bug

[Josef Bacik]

On Mon, Sep 30, 2024 at 03:23:10PM -0700, Leo Martins wrote:
> 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. This seems like a bad idea, I considered
> adding to the message to signify that this is not a big issue and maybe
> point them to this patchset to get the fix. Open to ideas on how to
> handle this.
> 
> Leo Martins (2):
>   btrfs-progs: delete free space when deleting block group
>   btrfs-progs: check free space maps to block group
> 

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH v2 0/2] btrfs-progs: mkfs free-space-info bug

[Boris Burkov]

On Mon, Sep 30, 2024 at 03:23:10PM -0700, Leo Martins wrote:
> 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. This seems like a bad idea, I considered
> adding to the message to signify that this is not a big issue and maybe
> point them to this patchset to get the fix. Open to ideas on how to
> handle this.

A few thoughts:
- You should also add a diagnostic to the in-kernel tree checker (but not
  one that flips us read-only)
- You can add support in check --repair for removing the free space info
- I don't think it hurts to use some softer language in the warning, but
  fundamentally people are going to be hitting it and dealing with it
  eventually, so better to do include it than not. Otherwise, it's just
  a timebomb until we write some kernel logic that DOES choke on this
  problem.

  I don't think there is a precedent for a very soft warning in fsck
  output, but if there is, let's just follow that. I certainly wouldn't
  hate links to a wiki, for example :)

Boris

> 
> Leo Martins (2):
>   btrfs-progs: delete free space when deleting block group
>   btrfs-progs: check free space maps to block group
> 
>  common/clear-cache.c            |  3 ++-
>  kernel-shared/extent-tree.c     | 10 ++++++++++
>  kernel-shared/free-space-tree.c |  3 +++
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> -- 
> 2.43.5
>