Re: [PATCH] btrfs: handle errors properly in update_inline_extent_backref()

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

On 2023/8/10 02:59, David Sterba wrote:
> On Wed, Aug 09, 2023 at 03:08:21PM +0800, Qu Wenruo wrote:
>> [PROBLEM]
>> Inside function update_inline_extent_backref(), we have several
>> BUG_ON()s along with some ASSERT()s which can be triggered by corrupted
>> filesystem.
>>
>> [ANAYLYSE]
>> Most of those BUG_ON()s and ASSERT()s are just a way of handling
>> unexpected on-disk data.
>>
>> Although we have tree-checker to rule out obviously incorrect extent
>> tree blocks, it's not enough for those ones.
>>
>> Thus we need proper error handling for them.
>>
>> [FIX]
>> Thankfully all the callers of update_inline_extent_backref() would
>> eventually handle the errror by aborting the current transaction.
>>
>> So this patch would do the proper error handling by:
>>
>> - Make update_inline_extent_backref() to return int
>>    The return value would be either 0 or -EUCLEAN.
>>
>> - Replace BUG_ON()s and ASSERT()s with proper error handling
>>    This includes:
>>    * Dump the bad extent tree leaf
>>    * Output an error message for the cause
>>      This would include the extent bytenr, num_bytes (if needed),
>>      the bad values and expected good values.
>>    * Return -EUCLEAN
>>
>>    Note here we remove all the WARN_ON()s, as eventually the transaction
>>    would be aborted, thus a backtrace would be triggered anyway.
>>
>> - Better comments on why we expect refs == 1 and refs_to_mode == -1 for
>>    tree blocks
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/extent-tree.c | 80 ++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 65 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index 3cae798499e2..45e325523e81 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -381,11 +381,11 @@ int btrfs_get_extent_inline_ref_type(const struct extent_buffer *eb,
>>   		}
>>   	}
>>
>> +	WARN_ON(1);
>>   	btrfs_print_leaf(eb);
>>   	btrfs_err(eb->fs_info,
>>   		  "eb %llu iref 0x%lx invalid extent inline ref type %d",
>>   		  eb->start, (unsigned long)iref, type);
>> -	WARN_ON(1);
>
> Do we even want to print the warning here? There's the whole leaf, error
> message, why would we need the stack trace?

Following the principle I mentioned in another thread, you're right, we
don't need the warning as long as we move the transaction abort closer
to the error site.

Otherwise we have two possible sites calling this function, thus harder
to locate the real call chain.

Thanks,
Qu