From nobody Sun Aug 3 22:11:49 2025 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4bwDS624Spz63tqs; Sun, 03 Aug 2025 22:11:50 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R10" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4bwDS61KGqz3sVQ; Sun, 03 Aug 2025 22:11:50 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1754259110; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=KE4g8tjQcuNHaGFwNBZ/UCJ+eYfm4ZX/3MpYlWx6PzE=; b=DrAq0PSfhp36Oinj1BfTWSwqtFu4gr0DRdl4ojIqzQBvzcq8SWEr3IgTpdonF+5FgaOP37 kVsh8WzY3ky/HmbnjPDKDrUDGWpFSsSiOux4irtp9qVOAccqcJHMBsr0IjSgGQBeF66vw8 n+HWJ6ZZeEShx2QYk6gX9yDFq+IT8Q15UmPbquKlFdJ2ah9XyWMQGXlyb+vYHR7JkiJrlD 5piEOqxt2J+t8P9NZgLRjffsb/lt6c6ghAfarIzOaJM8B2S8MQanhtSdh/eYCgMS3NwLGX 3v+Vo+wC/hyRmLh8tJNCX4k27x9wivcdt9V4R8evAVokrgcj8SgvXRG8SezuEg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1754259110; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=KE4g8tjQcuNHaGFwNBZ/UCJ+eYfm4ZX/3MpYlWx6PzE=; b=atrheNSR63nKNwdJ+PMo1Y46LVCfjnQvaVYFju3jDfTisSrRGyOs3RYjQVGQ2y51JOn9HJ 6BeHzHT1XkUJkj3eTOaTZ0pXMOuSrYk9XlKN5V52paKx4qcm9cf91I1J873Bk8/ZGSUc9G ygP1awCIjJdVMZNdO3qHdcQgzjSSmBkxyTOhnhjXyKjKjMy27bgwMC92vaFreH7i5GHntD lYXQHIsByZOzue+VZ5ul6yngBz5Y3EzxYt3uodfMfRA5GF5hBYW9GbN1hpl6yYQlT5hmk8 2BSqJpNrxTNqsC+WWfMva6ZZToQpG4vdwIXLLFfVdYIFosy7Ms4KRzK1GqXeKw== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1754259110; a=rsa-sha256; cv=none; b=qg+3TmDANPV7nE2Ej3TrDgWid/tMfRycu+1n1I+5YveGeZj8S0h9wZVfp/9bOFXXZBe6cd 4VNFowhLwiD2fxuCP6xpKylYjqC+LmbTECkP14rsRhHZQXxaTcgso4mn1OSjsP6hE13/P+ nkGL/qQAJRLuY/vMnIzsl/i0q6T7CKTd9axlSbAZ08oLcui7B1LTxAUHS/jQFQhGxXjDtF PR/boM7xGp4wIAcAoqwqvdqG+QfEiemDUYfccalpfAybDooqthejwCoUrKnj5R0Phkuu84 PXBmr2UDsUBjH3BnOH9ByiFNa9XqJUQ9b3uJQY1emU4iP/KIQSf82OhUjxB75A== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4bwDS60jXVz6Cb; Sun, 03 Aug 2025 22:11:50 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.18.1/8.18.1) with ESMTP id 573MBotp065808; Sun, 3 Aug 2025 22:11:50 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 573MBnoC065805; Sun, 3 Aug 2025 22:11:49 GMT (envelope-from git) Date: Sun, 3 Aug 2025 22:11:49 GMT Message-Id: <202508032211.573MBnoC065805@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Mark Johnston Subject: git: 3bd8fab2415b - main - vfs: Move DEBUG_VFS_LOCKS checks to INVARIANTS List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: markj X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 3bd8fab2415bf517d169fed2aa345ef08a977a98 Auto-Submitted: auto-generated The branch main has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=3bd8fab2415bf517d169fed2aa345ef08a977a98 commit 3bd8fab2415bf517d169fed2aa345ef08a977a98 Author: Mark Johnston AuthorDate: 2025-07-17 21:54:32 +0000 Commit: Mark Johnston 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 #include -#if defined(DEBUG_VFS_LOCKS) && (!defined(INVARIANTS) || !defined(WITNESS)) -#error DEBUG_VFS_LOCKS requires INVARIANTS and WITNESS -#endif - #ifdef DDB #include #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");