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

[Qu Wenruo <wqu@suse.com>]

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