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

[David Sterba <dsterba@suse.cz>]

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?"

	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

	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.

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.