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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2024/1/26 02:42, David Sterba wrote:
> 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.

In this case, I don't think we can do anything to really rollback, thus
aborting early would help debug and provide a better call trace.

Thanks,
Qu

>
>>> +
>>>    	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.
>