The branch main has been updated by markj:
URL:
https://cgit.FreeBSD.org/src/commit/?id=3bd8fab2415bf517d169fed2aa345ef08a977a98
commit 3bd8fab2415bf517d169fed2aa345ef08a977a98
Author: Mark Johnston <ma...@freebsd.org>
AuthorDate: 2025-07-17 21:54:32 +0000
Commit: Mark Johnston <ma...@freebsd.org>
CommitDate: 2025-08-03 21:27:28 +0000
vfs: Move DEBUG_VFS_LOCKS checks to INVARIANTS
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, and the overhead
over DEBUG_VFS_LOCKS does not appear to be high enough to prohibit
folding it into INVARIANTS, so let's do that.
The change makes vnode lock assertions useful in plain INVARIANTS
kernels, and guards VOP debug routines on INVARIANTS rather than
DEBUG_VFS_LOCKS. Further, invariants are now checked by plain
assertions rather than having various sysctls to finely control what
happens the checks fail. The extra complexity didn't seem particularly
useful and is at odds with how we handle debugging most everywhere else.
Reviewed by: kib
MFC after: 2 weeks
Differential Revision: https://reviews.freebsd.org/D51402
---
sys/kern/vfs_subr.c | 103 ++++++++++++++-----------------------------------
sys/sys/vnode.h | 8 ++--
sys/tools/vnode_if.awk | 6 +--
3 files changed, 37 insertions(+), 80 deletions(-)
diff --git a/sys/kern/vfs_subr.c b/sys/kern/vfs_subr.c
index 29774cf87393..4eac8bb0c8bb 100644
--- a/sys/kern/vfs_subr.c
+++ b/sys/kern/vfs_subr.c
@@ -97,10 +97,6 @@
#include <vm/vnode_pager.h>
#include <vm/uma.h>
-#if defined(DEBUG_VFS_LOCKS) && (!defined(INVARIANTS) || !defined(WITNESS))
-#error DEBUG_VFS_LOCKS requires INVARIANTS and WITNESS
-#endif
-
#ifdef DDB
#include <ddb/ddb.h>
#endif
@@ -5736,102 +5732,69 @@ extattr_check_cred(struct vnode *vp, int
attrnamespace, struct ucred *cred,
}
}
-#ifdef DEBUG_VFS_LOCKS
-int vfs_badlock_ddb = 1; /* Drop into debugger on violation. */
-SYSCTL_INT(_debug, OID_AUTO, vfs_badlock_ddb, CTLFLAG_RW, &vfs_badlock_ddb, 0,
- "Drop into debugger on lock violation");
-
-int vfs_badlock_mutex = 1; /* Check for interlock across VOPs. */
-SYSCTL_INT(_debug, OID_AUTO, vfs_badlock_mutex, CTLFLAG_RW, &vfs_badlock_mutex,
- 0, "Check for interlock across VOPs");
-
-int vfs_badlock_print = 1; /* Print lock violations. */
-SYSCTL_INT(_debug, OID_AUTO, vfs_badlock_print, CTLFLAG_RW, &vfs_badlock_print,
- 0, "Print lock violations");
-
-int vfs_badlock_vnode = 1; /* Print vnode details on lock violations. */
-SYSCTL_INT(_debug, OID_AUTO, vfs_badlock_vnode, CTLFLAG_RW, &vfs_badlock_vnode,
- 0, "Print vnode details on lock violations");
-
-#ifdef KDB
-int vfs_badlock_backtrace = 1; /* Print backtrace at lock violations. */
-SYSCTL_INT(_debug, OID_AUTO, vfs_badlock_backtrace, CTLFLAG_RW,
- &vfs_badlock_backtrace, 0, "Print backtrace at lock violations");
-#endif
-
-static void
-vfs_badlock(const char *msg, const char *str, struct vnode *vp)
-{
-
-#ifdef KDB
- if (vfs_badlock_backtrace)
- kdb_backtrace();
-#endif
- if (vfs_badlock_vnode)
- vn_printf(vp, "vnode ");
- if (vfs_badlock_print)
- printf("%s: %p %s\n", str, (void *)vp, msg);
- if (vfs_badlock_ddb)
- kdb_enter(KDB_WHY_VFSLOCK, "lock violation");
-}
-
+#ifdef INVARIANTS
void
assert_vi_locked(struct vnode *vp, const char *str)
{
-
- if (vfs_badlock_mutex && !mtx_owned(VI_MTX(vp)))
- vfs_badlock("interlock is not locked but should be", str, vp);
+ VNASSERT(mtx_owned(VI_MTX(vp)), vp,
+ ("%s: vnode interlock is not locked but should be", str));
}
void
assert_vi_unlocked(struct vnode *vp, const char *str)
{
-
- if (vfs_badlock_mutex && mtx_owned(VI_MTX(vp)))
- vfs_badlock("interlock is locked but should not be", str, vp);
+ VNASSERT(!mtx_owned(VI_MTX(vp)), vp,
+ ("%s: vnode interlock is locked but should not be", str));
}
void
assert_vop_locked(struct vnode *vp, const char *str)
{
+ bool locked;
+
if (KERNEL_PANICKED() || vp == NULL)
return;
#ifdef WITNESS
- if ((vp->v_irflag & VIRF_CROSSMP) == 0 &&
- witness_is_owned(&vp->v_vnlock->lock_object) == -1)
+ locked = !((vp->v_irflag & VIRF_CROSSMP) == 0 &&
+ witness_is_owned(&vp->v_vnlock->lock_object) == -1);
#else
- int locked = VOP_ISLOCKED(vp);
- if (locked == 0 || locked == LK_EXCLOTHER)
+ int state = VOP_ISLOCKED(vp);
+ locked = state != 0 && state != LK_EXCLOTHER;
#endif
- vfs_badlock("is not locked but should be", str, vp);
+ VNASSERT(locked, vp, ("%s: vnode is not locked but should be", str));
}
void
assert_vop_unlocked(struct vnode *vp, const char *str)
{
+ bool locked;
+
if (KERNEL_PANICKED() || vp == NULL)
return;
#ifdef WITNESS
- if ((vp->v_irflag & VIRF_CROSSMP) == 0 &&
- witness_is_owned(&vp->v_vnlock->lock_object) == 1)
+ locked = (vp->v_irflag & VIRF_CROSSMP) == 0 &&
+ witness_is_owned(&vp->v_vnlock->lock_object) == 1;
#else
- if (VOP_ISLOCKED(vp) == LK_EXCLUSIVE)
+ locked = VOP_ISLOCKED(vp) == LK_EXCLUSIVE;
#endif
- vfs_badlock("is locked but should not be", str, vp);
+ VNASSERT(!locked, vp, ("%s: vnode is locked but should not be", str));
}
void
assert_vop_elocked(struct vnode *vp, const char *str)
{
+ bool locked;
+
if (KERNEL_PANICKED() || vp == NULL)
return;
- if (VOP_ISLOCKED(vp) != LK_EXCLUSIVE)
- vfs_badlock("is not exclusive locked but should be", str, vp);
+ locked = VOP_ISLOCKED(vp) == LK_EXCLUSIVE;
+ VNASSERT(locked, vp,
+ ("%s: vnode is not exclusive locked but should be", str));
}
-#endif /* DEBUG_VFS_LOCKS */
+#endif /* INVARIANTS */
void
vop_rename_fail(struct vop_rename_args *ap)
@@ -5852,7 +5815,7 @@ vop_rename_pre(void *ap)
{
struct vop_rename_args *a = ap;
-#ifdef DEBUG_VFS_LOCKS
+#ifdef INVARIANTS
struct mount *tmp;
if (a->a_tvp)
@@ -5895,7 +5858,7 @@ vop_rename_pre(void *ap)
vhold(a->a_tvp);
}
-#ifdef DEBUG_VFS_LOCKS
+#ifdef INVARIANTS
void
vop_fplookup_vexec_debugpre(void *ap __unused)
{
@@ -6008,13 +5971,7 @@ vop_strategy_debugpre(void *ap)
if ((bp->b_flags & B_CLUSTER) != 0)
return;
- if (!KERNEL_PANICKED() && !BUF_ISLOCKED(bp)) {
- if (vfs_badlock_print)
- printf(
- "VOP_STRATEGY: bp is not locked but should be\n");
- if (vfs_badlock_ddb)
- kdb_enter(KDB_WHY_VFSLOCK, "lock violation");
- }
+ BUF_ASSERT_LOCKED(bp);
}
void
@@ -6063,7 +6020,7 @@ vop_need_inactive_debugpost(void *ap, int rc)
ASSERT_VI_LOCKED(a->a_vp, "VOP_NEED_INACTIVE");
}
-#endif
+#endif /* INVARIANTS */
void
vop_allocate_post(void *ap, int rc)
@@ -6229,7 +6186,7 @@ vop_mkdir_post(void *ap, int rc)
}
}
-#ifdef DEBUG_VFS_LOCKS
+#ifdef INVARIANTS
void
vop_mkdir_debugpost(void *ap, int rc)
{
@@ -6677,7 +6634,7 @@ vfs_knlunlock(void *arg)
static void
vfs_knl_assert_lock(void *arg, int what)
{
-#ifdef DEBUG_VFS_LOCKS
+#ifdef INVARIANTS
struct vnode *vp = arg;
if (what == LA_LOCKED)
diff --git a/sys/sys/vnode.h b/sys/sys/vnode.h
index a416fddcddc3..074769d55c2d 100644
--- a/sys/sys/vnode.h
+++ b/sys/sys/vnode.h
@@ -538,7 +538,7 @@ extern struct vnodeop_desc *vnodeop_descs[];
#define VOPARG_OFFSETTO(s_type, s_offset, struct_p) \
((s_type)(((char*)(struct_p)) + (s_offset)))
-#ifdef DEBUG_VFS_LOCKS
+#ifdef INVARIANTS
/*
* Support code to aid in debugging VFS locking problems. Not totally
* reliable since if the thread sleeps between changing the lock
@@ -572,7 +572,7 @@ void assert_vop_unlocked(struct vnode *vp, const
char *str);
VNPASS(!seqc_in_modify(_vp->v_seqc), _vp); \
} while (0)
-#else /* !DEBUG_VFS_LOCKS */
+#else /* !INVARIANTS */
#define ASSERT_VI_LOCKED(vp, str) ((void)0)
#define ASSERT_VI_UNLOCKED(vp, str) ((void)0)
@@ -583,7 +583,7 @@ void assert_vop_unlocked(struct vnode *vp, const
char *str);
#define ASSERT_VOP_IN_SEQC(vp) ((void)0)
#define ASSERT_VOP_NOT_IN_SEQC(vp) ((void)0)
-#endif /* DEBUG_VFS_LOCKS */
+#endif /* INVARIANTS */
/*
* This call works for vnodes in the kernel.
@@ -956,7 +956,7 @@ void vop_symlink_pre(void *a);
void vop_symlink_post(void *a, int rc);
int vop_sigdefer(struct vop_vector *vop, struct vop_generic_args *a);
-#ifdef DEBUG_VFS_LOCKS
+#ifdef INVARIANTS
void vop_fdatasync_debugpre(void *a);
void vop_fdatasync_debugpost(void *a, int rc);
void vop_fplookup_vexec_debugpre(void *a);
diff --git a/sys/tools/vnode_if.awk b/sys/tools/vnode_if.awk
index e829105197cc..74b11e6cb27d 100644
--- a/sys/tools/vnode_if.awk
+++ b/sys/tools/vnode_if.awk
@@ -86,7 +86,7 @@ function add_debug_code(name, arg, pos, ind)
function add_debugpre(name)
{
if (lockdata[name, "debugpre"]) {
- printc("#ifdef DEBUG_VFS_LOCKS");
+ printc("#ifdef INVARIANTS");
printc("\t"lockdata[name, "debugpre"]"(a);");
printc("#endif");
}
@@ -95,7 +95,7 @@ function add_debugpre(name)
function add_debugpost(name)
{
if (lockdata[name, "debugpost"]) {
- printc("#ifdef DEBUG_VFS_LOCKS");
+ printc("#ifdef INVARIANTS");
printc("\t"lockdata[name, "debugpost"]"(a, rc);");
printc("#endif");
}
@@ -340,7 +340,7 @@ while ((getline < srcfile) > 0) {
for (i = 0; i < numargs; ++i)
printh("\ta.a_" args[i] " = " args[i] ";");
if (can_inline(name)) {
- printh("\n#if !defined(DEBUG_VFS_LOCKS) &&
!defined(INVARIANTS) && !defined(KTR)");
+ printh("\n#if !defined(INVARIANTS) && !defined(KTR)");
printh("\tif (!SDT_PROBES_ENABLED())");
printh("\t\treturn (" args[0]"->v_op->"name"(&a));");
printh("\telse");
