Re: [PATCH v2] btrfs: replace BUG_ON() in btrfs_csum_one_bio() with proper error handling

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

On 2021/8/19 下午8:23, David Sterba wrote:
> On Thu, Aug 19, 2021 at 09:08:00AM +0800, Qu Wenruo wrote:
>> I want to play safe by never bothering the WARN_ON() in if () condition
>> anymore.
>>
>> The fact that some if (WARN_ON()) is acceptable and some is not is
>> already worrying.
>>
>> Can we just stick to no-WARN_ON-in-if policy?
>
> So what would be the reocommended way to do that? Also with the obvious
> question "is the warning needed at all?"

IMHO we should use WARN() other than WARN_ON().
The WARN_ON(), even with proper condition, is not providing enough info.

We want the warning mostly for fstests to catch the error, so that we
know it's something wrong.
>
> 	if (condition) {
> 		WARN_ON(1);
> 		...
> 	}
>
> This will hide the condition in the warning report, the value is usually
> in RAX and sometimes it helps to analyze what happend. We'd have to
> either duplicate it like

The condition itself is not really that helpful, compared to proper
warning message.

In fact, the line number is more useful than the condition itself.

>
> 	if (condition) {
> 		WARN_ON(condition);
> 		...
> 	}
>
> or use a temporary variable.
>
> Another question is what to do with current if+WARN calls. Lots of them
> are there without a comment. Ideally if there's a need for a warning
> even on a production build, there should be a message also printed.

We can add comments on them when we're going to refactor the code.

But historic problems shouldn't be a blockage for us to update our
current code guideline.

>
> Lots of them seem to be an assert in disguise (like in
> extract_ordered_extent) or are called before returning EUCLEAN. We've
> talked about this a few times before, we'd need more fine grained
> warnings/assertions or have them better documented. There are 300+ of
> them so that's a lot for a single pass audit, but at least some of them
> share a common pattern and can be unified.
>

This already shows that we need a proper guideline for how to do proper
warning.

Thanks,
Qu