vfs: Move DEBUG_VFS_LOCKS checks to INVARIANTS
ClosedPublic
Actions

Authored by markj on Jul 18 2025, 1:22 AM.

Details

Reviewers

kib

Commits

rG3bd8fab2415b: vfs: Move DEBUG_VFS_LOCKS checks to INVARIANTS

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%.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 65549
Build 62432: arc lint + arc unit

Event Timeline

markj created this revision.Jul 18 2025, 1:22 AM

Herald added subscribers: olce, imp. · View Herald TranscriptJul 18 2025, 1:22 AM

markj requested review of this revision.Jul 18 2025, 1:22 AM

Harbormaster completed remote builds in B65549: Diff 158722.Jul 18 2025, 1:22 AM

markj added a parent revision: D51401: inotify: Fix a couple of locking bugs.Jul 18 2025, 1:22 AM

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.

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 ?

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'.

This revision is now accepted and ready to land.Jul 18 2025, 10:47 PM

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.