On 2021/8/19 下午8:23, David Sterba wrote:

On Thu, Aug 19, 2021 at 09:08:00AM +0800, Qu Wenruo wrote:

quoted

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