From nobody Mon Aug 25 15:22:48 2025 X-Original-To: dev-commits-src-branches@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 4c9ZL10P8tz65vxk; Mon, 25 Aug 2025 15:22:49 +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 "R12" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4c9ZL06rcWz3XMN; Mon, 25 Aug 2025 15:22:48 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1756135369; 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=aBuQBMZJ9GygwPNDIuUwtQnCZdfqZ3zHJKaDEblL4OI=; b=cATHqrgdPd3UVWlqWYicnQMIyw4YzchiLtNErDEQmlrmBVJOnwxciHp1eZnyPhlXU7NJHv YvDoI72F/+r+X4eUZpHSy3UOiQtO3nGxJ5cZG9yUeqqzXrg/wbfOKzWcxL8QINNwAdBZM2 /PpGTsH8tstHENVEanYucrwDTg2XIxVEOqOeE/93KxKFNGae1zYmZBjXpzI70yjC9qJfXd pPSEVtDhfibiWTOYiJzTSvnptmNogYrnnyQfqIcKpuj7yCmaGoKoMdQtgCCNiZL4vndDNV yzZPLHAtlfanX/6wJGgmZEs1Rd5FSy48qaIiKfXH1fOWJX+6RGeBNpJF6JvqPA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1756135369; 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=aBuQBMZJ9GygwPNDIuUwtQnCZdfqZ3zHJKaDEblL4OI=; b=sQLg14rHA+QWY5SgiBrDs/myb+OMWLvVy4KaLQ+HzCga8/UtaKjFbL2H25tPknRrAfegrJ 4rg6YvKN0V2LvcrUT4B2tw7iofanudvsr10GO3e1dmEqT0Epkl+CQ/sJ4EzICjuwpdWyNp 2tUQDpo5e/epxGloasdvz77YxT4Ch/pSnD5ChUAdfNaKLxVGM+Z9YLw92c35YysZEY7S24 nDTwJCtmXBoEM6bQDYG/LH1t2pkquZ9R1+XjK3Bma7lt7Kgj+opYoFx3i91xxMgt04rfJh dnx5VtzNLLQ7SKJPM0G1J5F9+jIFsgjjPWwIi57q7qBGMKb5VuYfA/S6ME89tA== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1756135369; a=rsa-sha256; cv=none; b=EQgEDK5xZRC4BIsR5ZsI34/S58AnxuCe7S7Wr4gfDD1/VeuZ0b2iBZIRG+lmixVCnIvX2j aBIdOm4T4gqCuvCX4dUV7KozqgIzfgVv50BhLSceoZlYRDWM1s/p93ErtAkuyeFFrY2Nfb lCaFhAM8EKkHdk94qSxbstO3N15TaejxkY+69Qmk/uD4hCcivSbra3UHi1GAeq3CvOoyCk d6+3/JNs+XolkO91Flgy2/vVCqePTP3Qr52LqaudpJM67YNs5s5+slcUQzAK8WaBem70pU JBfPvo/YTU/HH75blqVtWmVkgubWH4rLUQojsm0Ha+6YohgX3eaUoCj6WL4q6g== ARC-Authentication-Results: i=1; mx1.freebsd.org; none 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 4c9ZL06QXlzvl6; Mon, 25 Aug 2025 15:22:48 +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 57PFMmTB053855; Mon, 25 Aug 2025 15:22:48 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 57PFMmhQ053852; Mon, 25 Aug 2025 15:22:48 GMT (envelope-from git) Date: Mon, 25 Aug 2025 15:22:48 GMT Message-Id: <202508251522.57PFMmhQ053852@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Mark Johnston Subject: git: 10bb227a7d10 - stable/14 - vfs: Move DEBUG_VFS_LOCKS checks to INVARIANTS List-Id: Commits to the stable branches of the FreeBSD src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-branches List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-branches@freebsd.org Sender: owner-dev-commits-src-branches@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/stable/14 X-Git-Reftype: branch X-Git-Commit: 10bb227a7d10cbdc859473424cc0c1ab0adbf503 Auto-Submitted: auto-generated The branch stable/14 has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=10bb227a7d10cbdc859473424cc0c1ab0adbf503 commit 10bb227a7d10cbdc859473424cc0c1ab0adbf503 Author: Mark Johnston AuthorDate: 2025-07-17 21:54:32 +0000 Commit: Mark Johnston CommitDate: 2025-08-25 13:57:56 +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 (cherry picked from commit 3bd8fab2415bf517d169fed2aa345ef08a977a98) --- 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 7ff6ac5f5b44..c0322593dead 100644 --- a/sys/kern/vfs_subr.c +++ b/sys/kern/vfs_subr.c @@ -100,10 +100,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 @@ -5667,102 +5663,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) @@ -5783,7 +5746,7 @@ vop_rename_pre(void *ap) { struct vop_rename_args *a = ap; -#ifdef DEBUG_VFS_LOCKS +#ifdef INVARIANTS if (a->a_tvp) ASSERT_VI_UNLOCKED(a->a_tvp, "VOP_RENAME"); ASSERT_VI_UNLOCKED(a->a_tdvp, "VOP_RENAME"); @@ -5819,7 +5782,7 @@ vop_rename_pre(void *ap) vhold(a->a_tvp); } -#ifdef DEBUG_VFS_LOCKS +#ifdef INVARIANTS void vop_fplookup_vexec_debugpre(void *ap __unused) { @@ -5932,13 +5895,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 @@ -5987,7 +5944,7 @@ vop_need_inactive_debugpost(void *ap, int rc) ASSERT_VI_LOCKED(a->a_vp, "VOP_NEED_INACTIVE"); } -#endif +#endif /* INVARIANTS */ void vop_create_pre(void *ap) @@ -6113,7 +6070,7 @@ vop_mkdir_post(void *ap, int rc) VFS_KNOTE_LOCKED(dvp, NOTE_WRITE | NOTE_LINK); } -#ifdef DEBUG_VFS_LOCKS +#ifdef INVARIANTS void vop_mkdir_debugpost(void *ap, int rc) { @@ -6549,7 +6506,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 e32756b906ab..22839659f047 100644 --- a/sys/sys/vnode.h +++ b/sys/sys/vnode.h @@ -533,7 +533,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 @@ -567,7 +567,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) @@ -578,7 +578,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. @@ -949,7 +949,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 b477bbc91c43..e4fb29f4a8f0 100644 --- a/sys/tools/vnode_if.awk +++ b/sys/tools/vnode_if.awk @@ -89,7 +89,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"); } @@ -98,7 +98,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"); } @@ -342,7 +342,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");