vfs: Move DEBUG_VFS_LOCKS checks to INVARIANTS

[markj]

Summary

It is easy to forget to configure DEBUG_VFS_LOCKS, and when one does, no
vnode lock assertions are checked when INVARIANTS is configured, so bugs
can arise. This has happened to me more than once, so let's make the
assertions useful in plain INVARIANTS kernels.

To assess the overhead, I ran ten buildkernels with and without the
patch. The results are here:
https://people.freebsd.org/~markj/debug_vfs_locks_overhead.txt

There is certainly a small increase in build times, but I think it is
small enough that the change is reasonable. In particular, with a
NODEBUG kernel the system time is reduced by almost 50%.

[kib]

May be, remove DEBUG_VFS_LOCKS altogether? If INVARIANTS + WITNESS are enabled, then let these checks be?

sys/kern/vfs_subr.c

5740

I would remove #ifdef DEBUG_VFS_LOCKS there, always providing vfs_badlock() as part of the INVARIANTS_SUPPORT (not just INVARIANTS).

5818

If done what I suggested above, this place could be simplified by removing !DEBUG_VFS_LOCKS part.

[markj]

In D51402#1173713, @kib wrote:

May be, remove DEBUG_VFS_LOCKS altogether? If INVARIANTS + WITNESS are enabled, then let these checks be?

I could. Assuming this patch is okay, I would next go and convert most of the existing DEBUG_VFS_LOCKS code to check INVARIANTS instead. Some of them I'm not sure about, e.g. vop_mkdir_debugpost(), perhaps that should be done under DIAGNOSTIC instead?

sys/kern/vfs_subr.c

5740

vfs_badlock() will call kdb_enter() by default, which is a weird behaviour for INVARIANTS. Should it do something like:

#ifdef DEBUG_VFS_LOCKS
if (vfs_badlock_ddb)
    kdb_enter(KDB_WHY_VFSLOCK, "lock violation");
#else
panic("%s: vnode %p %s", str, vp, msg);
#endif

?

[kib]

In D51402#1173864, @markj wrote:

In D51402#1173713, @kib wrote:

May be, remove DEBUG_VFS_LOCKS altogether? If INVARIANTS + WITNESS are enabled, then let these checks be?

I could. Assuming this patch is okay, I would next go and convert most of the existing DEBUG_VFS_LOCKS code to check INVARIANTS instead. Some of them I'm not sure about, e.g. vop_mkdir_debugpost(), perhaps that should be done under DIAGNOSTIC instead?

I am not sure why would VOP_MKDIR() be special. The _debugpost() does single cache lookup, which is IMO fine for INVARIANTS kernels.

We can always move some stuff under DIAGNOSTIC if people start to complain loudly that INVARIANTS kernels are unusable.

sys/kern/vfs_subr.c

5740

I would say that this is weird on its own, and I do not see a need in the separate policy for vfs. It should panic directly, and then we already have the control for 'debugger on panic'.

[markj]

In D51402#1174061, @kib wrote:

In D51402#1173864, @markj wrote:

In D51402#1173713, @kib wrote:

May be, remove DEBUG_VFS_LOCKS altogether? If INVARIANTS + WITNESS are enabled, then let these checks be?

I could. Assuming this patch is okay, I would next go and convert most of the existing DEBUG_VFS_LOCKS code to check INVARIANTS instead. Some of them I'm not sure about, e.g. vop_mkdir_debugpost(), perhaps that should be done under DIAGNOSTIC instead?

I am not sure why would VOP_MKDIR() be special. The _debugpost() does single cache lookup, which is IMO fine for INVARIANTS kernels.

My intuition is that INVARIANTS checks should be O(1) and relatively cheap (i.e., shouldn't acquire locks). But yes, that particular example is probably ok.

We can always move some stuff under DIAGNOSTIC if people start to complain loudly that INVARIANTS kernels are unusable.

Ok.

sys/kern/vfs_subr.c

5740

Ok. I agree, but I wasn't sure, maybe you use this feature and expect it to work.

[markj]

Go further towards removing DEBUG_VFS_LOCKS

[markj]

I redid the buildkernel times: https://people.freebsd.org/~markj/debug_vfs_locks_overhead2.txt