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

[Mark Harmstone <mark@harmstone.com>]

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;
>   		}