mbox series

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

Message ID cover.1727732460.git.loemra.dev@gmail.com (mailing list archive)
Headers show
Series btrfs-progs: mkfs free-space-info bug | expand

Message

Leo Martins Sept. 30, 2024, 10:23 p.m. UTC
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(-)

Comments

Josef Bacik Oct. 1, 2024, 8:21 p.m. UTC | #1
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
Boris Burkov Oct. 2, 2024, 6:45 p.m. UTC | #2
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
>