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

Qu Wenruo
2020-08-14T00:52:29Z

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.

>
>> @@ -3164,6 +3267,21 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
>> out:
>> btrfs_free_path(path);
>> return ret;
>> +err_dump_abort:
>> + /*
>> + * Leaf dump can take up a lot of dmesg buffer since default nodesize
>> + * is already 16K.
>> + * So we only do full leaf dump for debug build.
>> + */
>> + if (IS_ENABLED(CONFIG_BTRFS_DEBUG)) {
>> + btrfs_crit(info, "path->slots[0]=%d extent_slot=%d",
>> + path->slots[0], extent_slot);
>> + btrfs_print_leaf(path->nodes[0]);
>
> Preceded by a potentially long-running action, so this will delay the
> acutal abort.

That's why it's only for CONFIG_BTRFS_DEBUG.

And for debug kernel, that info would definitely be more helpful for us
to locate the problem, right?

Thanks,
Qu

>
>> + }
>> +
>> + btrfs_abort_transaction(trans, -EUCLEAN);
>> + btrfs_free_path(path);
>> + return -EUCLEAN;
>> }