Re: [PATCH 03/20] btrfs: handle block group lookup error when it's being removed

David Sterba
2024-01-25T16:13:17Z

On Thu, Jan 25, 2024 at 02:28:02PM +1030, Qu Wenruo wrote:
>
>
> On 2024/1/25 07:47, David Sterba wrote:
> > The unlikely case of lookup error in btrfs_remove_block_group() can be
> > handled properly, in its caller this would lead to a transaction abort.
> > We can't do anything else, a block group must have been loaded first.
> >
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> > fs/btrfs/block-group.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> > index 1905d76772a9..16a2b8609989 100644
> > --- a/fs/btrfs/block-group.c
> > +++ b/fs/btrfs/block-group.c
> > @@ -1063,7 +1063,9 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
> > bool remove_rsv = false;
> >
> > block_group = btrfs_lookup_block_group(fs_info, map->start);
> > - BUG_ON(!block_group);
> > + if (!block_group)
> > + return -ENOENT;
>
> This -ENOENT return value is fine, as the only caller would call
> btrfs_abort_transaction() to be noisy enough.
>
> And talking about btrfs_abort_transaction(), I think we can call it
> early to make debug a little easierly.

There are several patterns, one is that transaction abort is called by
the function that started it. It's not consistent but as a hint abort
can be used anywhere if things go so bad that it's impossible to roll
back, eg. in a middle of a big loop setting up block groups and such.

> > +
> > BUG_ON(!block_group->ro);
>
> But shouldn't we also handle the RO case?

Of course but it's fixing a different problem with tracking of read-only
status of a bg and I will not mix that to that patch.