VERIFY3_IMPL implementation is not robust

[avg-I]

The implementation in debug.h has an obvious C macro problem: it might evaluate its arguments more than once and that could lead to wrong values being printed if the arguments are not invariant or not idempotent.

Fortunately, this is only a problem with reporting. But that can be quite confusing when debugging a problem. For example in this issue you can see

VERIFY3(0 == nvlist_add_nvlist(config, "feature_stats", features)) failed (0 == 0)

which would imply that zero was not equal to zero.

I think that the macro should follow the pattern employed in zfs/assert.h

[behlendorf]

@avg-I yes this is definitely something of a known issue and I completely agree it can be confusing. The code was originally written this way for two reasons.

• Adding these local variables will increase stack depth. This may push us over the 8K limit in places so making a change like this is potentially destabilizing.
• No memory allocations should be performed. This allow us to safely log failures even when the VERIFY/ASSERT is located under a spin lock. We could allocate memory here as long as it's GFP_ATOMIC.

As long as we address these two concerns we could restructure this code.

[avg-I]

@behlendorf is the stack depth really of so much concern in this context?
I wouldn't expect that using intermediate variables would add more than a couple of dozens of bytes to the stack. If the stack depth is balancing near the limit then the code would be extra fragile for any kinds of code changes.

[behlendorf]

@avg-I unfortunately it is. Let's say we add 2 64-bit values to the macro, that can increase each stack frame where the macro is used by 16 bytes. We've observed that a worst case of roughly 50 stack frames isn't impossible in some of the deeper call paths coupled with entering direct memory reclaim. The works out to a rough worst case swag of an extra 800 bytes on the stack since we need to plan for the worst case.

Most Linux kernels only have 8K stacks (~7K really) to work so this is 10% of the stack. And because ZFS can from a platform with bigger stacks we've already gone through a lot of effect to slim them down to stay under the limit. But there are call paths where we're still close. The good news is that about a year ago the upstream kernel changed the default stack size to 16K. But it will take some time before that changes works it way down in to all the distributions.

[avg-I]

I think that in most cases one of the values is a constant, so the estimate could be cut in half.
Then, I do not think that nested VERIFYs are that common. But I will not make any bets 😃

[behlendorf]

It's definitely a high estimate but we are worried about the absolute worst case. We also need debug builds to be safe too so ASSERTs count! We could try explore this. I just wanted to be upfront about the not so obvious risks.