Re: git: 7b6fe2428a97 - main - DEBUG_VFS_LOCKS: use witness if available

On Sun, Apr 09, 2023 at 09:34:34PM +0000, Konstantin Belousov wrote:
> The branch main has been updated by kib:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=7b6fe2428a97921e8df882d0a24b87094c37b468
> 
> commit 7b6fe2428a97921e8df882d0a24b87094c37b468
> Author:     Konstantin Belousov <kib@FreeBSD.org>
> AuthorDate: 2023-04-08 06:15:00 +0000
> Commit:     Konstantin Belousov <kib@FreeBSD.org>
> CommitDate: 2023-04-09 21:34:12 +0000
> 
>     DEBUG_VFS_LOCKS: use witness if available
>     
>     The assert_vop_locked messages are ignored, and file/line information
>     is not too useful. Fixing this without changing both witness and VFS
>     asserts KPIs is not possible.

What was the motivation for this change?
At first glance it seems regressive. I've at least found the assert
messages, as well as the vnode state dumping done by vfs_badlock(),
to be really useful for quick debugging of locking issues.  This is
especially true when optimization makes the backtrace and frame info
less than useful; see commit 5a4a83fd0e67a0d7787d2f3e09ef0e5552a1ffb6
for a recent example.

>     
>     Reviewed by:    markj (previous version)
>     Tested by:      pho
>     Sponsored by:   The FreeBSD Foundation
>     MFC after:      1 week
>     Differential revision:  https://reviews.freebsd.org/D39464
> ---
>  sys/kern/vfs_lookup.c |  1 +
>  sys/kern/vfs_subr.c   | 16 +++++++++++++---
>  sys/sys/vnode.h       |  1 +
>  3 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/sys/kern/vfs_lookup.c b/sys/kern/vfs_lookup.c
> index e7f1deea0fae..172aa4b4f576 100644
> --- a/sys/kern/vfs_lookup.c
> +++ b/sys/kern/vfs_lookup.c
> @@ -162,6 +162,7 @@ nameiinit(void *dummy __unused)
>  	vfs_vector_op_register(&crossmp_vnodeops);
>  	getnewvnode("crossmp", NULL, &crossmp_vnodeops, &vp_crossmp);
>  	vp_crossmp->v_state = VSTATE_CONSTRUCTED;
> +	vp_crossmp->v_irflag |= VIRF_CROSSMP;
>  }
>  SYSINIT(vfs, SI_SUB_VFS, SI_ORDER_SECOND, nameiinit, NULL);
>  
> diff --git a/sys/kern/vfs_subr.c b/sys/kern/vfs_subr.c
> index c2b1f71502cd..7e7315f827a1 100644
> --- a/sys/kern/vfs_subr.c
> +++ b/sys/kern/vfs_subr.c
> @@ -5452,14 +5452,18 @@ assert_vi_unlocked(struct vnode *vp, const char *str)
>  void
>  assert_vop_locked(struct vnode *vp, const char *str)
>  {
> -	int locked;
> -
>  	if (KERNEL_PANICKED() || vp == NULL)
>  		return;
>  
> -	locked = VOP_ISLOCKED(vp);
> +#ifdef WITNESS
> +	if ((vp->v_irflag & VIRF_CROSSMP) == 0)
> +		witness_assert(&vp->v_vnlock->lock_object, LA_LOCKED,
> +		    __FILE__, __LINE__);
> +#else
> +	int locked = VOP_ISLOCKED(vp);
>  	if (locked == 0 || locked == LK_EXCLOTHER)
>  		vfs_badlock("is not locked but should be", str, vp);
> +#endif
>  }
>  
>  void
> @@ -5468,8 +5472,14 @@ assert_vop_unlocked(struct vnode *vp, const char *str)
>  	if (KERNEL_PANICKED() || vp == NULL)
>  		return;
>  
> +#ifdef WITNESS
> +	if ((vp->v_irflag & VIRF_CROSSMP) == 0)
> +		witness_assert(&vp->v_vnlock->lock_object, LA_UNLOCKED,
> +		    __FILE__, __LINE__);
> +#else
>  	if (VOP_ISLOCKED(vp) == LK_EXCLUSIVE)
>  		vfs_badlock("is locked but should not be", str, vp);
> +#endif
>  }
>  
>  void
> diff --git a/sys/sys/vnode.h b/sys/sys/vnode.h
> index 5e3f81de0236..fa889826867e 100644
> --- a/sys/sys/vnode.h
> +++ b/sys/sys/vnode.h
> @@ -228,6 +228,7 @@ _Static_assert(sizeof(struct vnode) <= 448, "vnode size crosses 448 bytes");
>  				   never cleared once set */
>  #define	VIRF_MOUNTPOINT	0x0004	/* This vnode is mounted on */
>  #define	VIRF_TEXT_REF	0x0008	/* Executable mappings ref the vnode */
> +#define	VIRF_CROSSMP	0x0010	/* Cross-mp vnode, no locking */
>  
>  #define	VI_UNUSED0	0x0001	/* unused */
>  #define	VI_MOUNT	0x0002	/* Mount in progress */