Re: [PATCH v4 2/4] btrfs: extent-tree: Kill BUG_ON() in __btrfs_free_extent() and do better comment

Qu Wenruo
2020-08-14T08:07:29Z

On 2020/8/14 下午4:01, David Sterba wrote:
> On Fri, Aug 14, 2020 at 08:52:19AM +0800, Qu Wenruo wrote:
>>
>>
>> On 2020/8/13 下午10:10, David Sterba wrote:
>>> On Wed, Aug 12, 2020 at 02:05:07PM +0800, Qu Wenruo wrote:
>>>> @@ -2987,13 +3049,20 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
>>>> found_extent = 1;
>>>> break;
>>>> }
>>>> +
>>>> + /* Quick path didn't find the EXTEMT/METADATA_ITEM */
>>>> if (path->slots[0] - extent_slot > 5)
>>>> break;
>>>> extent_slot--;
>>>> }
>>>>
>>>> if (!found_extent) {
>>>> - BUG_ON(iref);
>>>> + if (unlikely(iref)) {
>>>> + btrfs_crit(info,
>>>> +"invalid iref, no EXTENT/METADATA_ITEM found but has inline extent ref");
>>>> + goto err_dump_abort;
>>>
>>> This is not calling transaction abort at the place where it happens,
>>> here and several other places too.
>>
>> Did you mean, we want the btrfs_abort_transaction() call not merged
>> under one tag, so that we can get the kernel warning with the line number?
>>
>> If so, that's indeed the case, we lose the exact line number.
>>
>> But we still have the unique error message to locate the problem without
>> much hassle (it's less obvious than the line number thought).
>>
>> Thus to me, we don't lose much debug info anyway.
>
> https://btrfs.wiki.kernel.org/index.php/Development_notes#Error_handling_and_transaction_abort
>
> "Please keep all transaction abort exactly at the place where they happen
> and do not merge them to one. This pattern should be used everwhere and
> is important when debugging because we can pinpoint the line in the code
> from the syslog message and do not have to guess which way it got to the
> merged call."
>
> It's very convenient to paste the file:line number from a stacktrace
> report and land exactly in the spot where it failed.
>
OK, makse sense.

Will go that direction in next update.

Thanks,
Qu