From owner-freebsd-hackers@freebsd.org Thu Dec 24 17:25:43 2015 Return-Path: Delivered-To: freebsd-hackers@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id A0F9CA5119B for ; Thu, 24 Dec 2015 17:25:43 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 7AA9D1339; Thu, 24 Dec 2015 17:25:43 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 8BE9EB97A; Thu, 24 Dec 2015 12:25:42 -0500 (EST) From: John Baldwin To: freebsd-hackers@freebsd.org Cc: Daniel Braniss , Yuri , Rick Macklem , 'Konstantin Belousov' Subject: Re: Should DEBUG_VFS_LOCKS messages be reported as bugs? Date: Thu, 24 Dec 2015 09:19:34 -0800 Message-ID: <4247765.dyXyDaofWi@ralph.baldwin.cx> User-Agent: KMail/4.14.3 (FreeBSD/10.2-STABLE; KDE/4.14.3; amd64; ; ) In-Reply-To: References: <567791E9.50207@rawbw.com> <567BA189.8080405@rawbw.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Thu, 24 Dec 2015 12:25:42 -0500 (EST) X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 Dec 2015 17:25:43 -0000 On Thursday, December 24, 2015 10:56:09 AM Daniel Braniss wrote: >=20 > > On 24 Dec 2015, at 09:40, Yuri wrote: > >=20 > > On 12/23/2015 23:10, Daniel Braniss wrote: > >> I just turned off WITNESS/INVARIANTS:-) > >> the only complain I get is when I do a mountd restart, but it=E2=80= =99s harmless. > >=20 > > Which OS version? I get the messages on 10.2-STABLE with options fr= om my OP. > >=20 > > Endless messages like this: > > Dec 23 17:59:36 yuri kernel: KDB: stack backtrace: > > Dec 23 17:59:36 yuri kernel: #0 0xffffffff8097c810 at kdb_backtrace= +0x60 > > Dec 23 17:59:36 yuri kernel: #1 0xffffffff809eae7e at assert_vop_el= ocked+0x6e > > Dec 23 17:59:36 yuri kernel: #2 0xffffffff809ea6b1 at insmntque1+0x= 41 > > Dec 23 17:59:36 yuri kernel: #3 0xffffffff82e202a1 at unionfs_nodeg= et+0x281 > > Dec 23 17:59:36 yuri kernel: #4 0xffffffff82e23b06 at unionfs_looku= p+0x726 > > Dec 23 17:59:36 yuri kernel: #5 0xffffffff80e7776f at VOP_CACHEDLOO= KUP_APV+0x10f > > Dec 23 17:59:36 yuri kernel: #6 0xffffffff809d92b6 at vfs_cache_loo= kup+0xd6 > > Dec 23 17:59:36 yuri kernel: #7 0xffffffff80e775af at VOP_LOOKUP_AP= V+0x10f > > Dec 23 17:59:36 yuri kernel: #8 0xffffffff809e1892 at lookup+0x5c2 > > Dec 23 17:59:36 yuri kernel: #9 0xffffffff809e0fb4 at namei+0x4e4 > > Dec 23 17:59:36 yuri kernel: #10 0xffffffff809f683e at kern_statat_= vnhook+0xae > > Dec 23 17:59:36 yuri kernel: #11 0xffffffff809f66cd at sys_stat+0x2= d > > Dec 23 17:59:36 yuri kernel: #12 0xffffffff80d4fff4 at amd64_syscal= l+0x2c4 > > Dec 23 17:59:36 yuri kernel: #13 0xffffffff80d339db at Xfast_syscal= l+0xfb > > Dec 23 17:59:36 yuri kernel: insmntque: non-locked vp: 0xfffff80046= d233b0 is not exclusive locked but should be > >=20 > > Yuri >=20 > i have been using unionfs since it appeared, and on 10.2-stable, but = i don=E2=80=99t have WITNESS on. > BTW, it=E2=80=99s very slow if you turn WITNESS on, and so should onl= y be used for debugging, and in some > cases, the code actually works only if WITNESS is on :-( This isn't witness, this is DEBUG_VFS_LOCKS. DEBUG_VFS_LOCKS turns on assertions for vnode locks that are normally under INVARIANTS for other= locks. In this case unionfs is doing the insmntque() too early. It pr= obably needs to lock the lockmgr lock sooner (though it has to wait until it s= ets v_vnlock) and only insmtque() after the vnode lock is held. Here's a rough attempt at fixing this. I have not tested it though and= it might be very wrong. Index: union_subr.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- union_subr.c=09(revision 292560) +++ union_subr.c=09(working copy) @@ -105,7 +105,7 @@ unionfs_get_hashhead(struct vnode *dvp, char *path */ static struct vnode * unionfs_get_cached_vnode(struct vnode *uvp, struct vnode *lvp, -=09=09=09struct vnode *dvp, char *path) + struct vnode *dvp, char *path, int flags, struct thread *td) { =09struct unionfs_node_hashhead *hd; =09struct unionfs_node *unp; @@ -123,12 +123,9 @@ unionfs_get_cached_vnode(struct vnode *uvp, struct= =09=09=09vp =3D UNIONFSTOV(unp); =09=09=09VI_LOCK_FLAGS(vp, MTX_DUPOK); =09=09=09VI_UNLOCK(dvp); -=09=09=09vp->v_iflag &=3D ~VI_OWEINACT; -=09=09=09if ((vp->v_iflag & (VI_DOOMED | VI_DOINGINACT)) !=3D 0) { -=09=09=09=09VI_UNLOCK(vp); -=09=09=09=09vp =3D NULLVP; -=09=09=09} else -=09=09=09=09VI_UNLOCK(vp); +=09=09=09error =3D vget(vp, flags | LK_INTERLOCK, td); +=09=09=09if (error) +=09=09=09=09return (NULLVP); =09=09=09return (vp); =09=09} =09} @@ -142,7 +139,7 @@ unionfs_get_cached_vnode(struct vnode *uvp, struct */ static struct vnode * unionfs_ins_cached_vnode(struct unionfs_node *uncp, -=09=09=09struct vnode *dvp, char *path) + struct vnode *dvp, char *path, int flags, struct thread *td) { =09struct unionfs_node_hashhead *hd; =09struct unionfs_node *unp; @@ -153,6 +150,7 @@ unionfs_ins_cached_vnode(struct unionfs_node *uncp =09KASSERT((uncp->un_lowervp=3D=3DNULLVP || uncp->un_lowervp->v_type=3D= =3DVDIR), =09 ("unionfs_ins_cached_vnode: v_type !=3D VDIR")); =20 +retry: =09VI_LOCK(dvp); =09hd =3D unionfs_get_hashhead(dvp, path); =09LIST_FOREACH(unp, hd, un_hash) { @@ -159,14 +157,10 @@ unionfs_ins_cached_vnode(struct unionfs_node *unc= p =09=09if (!strcmp(unp->un_path, path)) { =09=09=09vp =3D UNIONFSTOV(unp); =09=09=09VI_LOCK_FLAGS(vp, MTX_DUPOK); -=09=09=09vp->v_iflag &=3D ~VI_OWEINACT; -=09=09=09if ((vp->v_iflag & (VI_DOOMED | VI_DOINGINACT)) !=3D 0) { -=09=09=09=09LIST_INSERT_HEAD(hd, uncp, un_hash); -=09=09=09=09VI_UNLOCK(vp); -=09=09=09=09vp =3D NULLVP; -=09=09=09} else -=09=09=09=09VI_UNLOCK(vp); =09=09=09VI_UNLOCK(dvp); +=09=09=09error =3D vget(vp2, flags | LK_INTERLOCK, td); +=09=09=09if (error =3D=3D ENOENT && (flags & LK_NOWAIT) =3D=3D 0) +=09=09=09=09goto retry; =09=09=09return (vp); =09=09} =09} @@ -218,7 +212,7 @@ unionfs_nodeget(struct mount *mp, struct vnode *up =09char=09 *path; =20 =09ump =3D MOUNTTOUNIONFSMOUNT(mp); -=09lkflags =3D (cnp ? cnp->cn_lkflags : 0); +=09lkflags =3D (cnp ? cnp->cn_lkflags : LK_EXCLUSIVE); =09path =3D (cnp ? cnp->cn_nameptr : NULL); =09*vpp =3D NULLVP; =20 @@ -233,19 +227,30 @@ unionfs_nodeget(struct mount *mp, struct vnode *u= p =20 =09/* check the cache */ =09if (path !=3D NULL && dvp !=3D NULLVP && vt =3D=3D VDIR) { -=09=09vp =3D unionfs_get_cached_vnode(uppervp, lowervp, dvp, path); +=09=09vp =3D unionfs_get_cached_vnode(uppervp, lowervp, dvp, path, +=09=09 lkflags, td); =09=09if (vp !=3D NULLVP) { -=09=09=09vref(vp); =09=09=09*vpp =3D vp; -=09=09=09goto unionfs_nodeget_out; +=09=09=09return (0); =09=09} =09} =20 +=09/* +=09 * We must promote to an exclusive lock for vnode creation. This +=09 * can happen if lookup is passed LOCKSHARED. +=09 */ +=09if ((lkflags & LK_TYPE_MASK) =3D=3D LK_SHARED) { +=09=09lkflags &=3D ~LK_TYPE_MASK; +=09=09lkflags |=3D LK_EXCLUSIVE; +=09} + =09if ((uppervp =3D=3D NULLVP || ump->um_uppervp !=3D uppervp) || =09 (lowervp =3D=3D NULLVP || ump->um_lowervp !=3D lowervp)) { =09=09/* dvp will be NULLVP only in case of root vnode. */ -=09=09if (dvp =3D=3D NULLVP) +=09=09if (dvp =3D=3D NULLVP) { +=09=09=09*vpp =3D NULL; =09=09=09return (EINVAL); +=09=09} =09} =09unp =3D malloc(sizeof(struct unionfs_node), =09 M_UNIONFSNODE, M_WAITOK | M_ZERO); @@ -253,11 +258,19 @@ unionfs_nodeget(struct mount *mp, struct vnode *u= p =09error =3D getnewvnode("unionfs", mp, &unionfs_vnodeops, &vp); =09if (error !=3D 0) { =09=09free(unp, M_UNIONFSNODE); +=09=09*vpp =3D NULL; =09=09return (error); =09} +=09if (uppervp !=3D NULLVP) +=09=09vp->v_vnlock =3D uppervp->v_vnlock; +=09else +=09=09vp->v_vnlock =3D lowervp->v_vnlock; +=09lockmgr(vp->v_vnlock, LK_EXCLUSIVE, NULL); =09error =3D insmntque(vp, mp);=09/* XXX: Too early for mpsafe fs */ =09if (error !=3D 0) { +=09=09vput(vp); =09=09free(unp, M_UNIONFSNODE); +=09=09*vpp =3D NULL; =09=09return (error); =09} =09if (dvp !=3D NULLVP) @@ -275,10 +288,6 @@ unionfs_nodeget(struct mount *mp, struct vnode *up= =09unp->un_uppervp =3D uppervp; =09unp->un_lowervp =3D lowervp; =09unp->un_dvp =3D dvp; -=09if (uppervp !=3D NULLVP) -=09=09vp->v_vnlock =3D uppervp->v_vnlock; -=09else -=09=09vp->v_vnlock =3D lowervp->v_vnlock; =20 =09if (path !=3D NULL) { =09=09unp->un_path =3D (char *) @@ -293,29 +302,25 @@ unionfs_nodeget(struct mount *mp, struct vnode *u= p =09 (lowervp !=3D NULLVP && ump->um_lowervp =3D=3D lowervp)) =09=09vp->v_vflag |=3D VV_ROOT; =20 -=09if (path !=3D NULL && dvp !=3D NULLVP && vt =3D=3D VDIR) -=09=09*vpp =3D unionfs_ins_cached_vnode(unp, dvp, path); -=09if ((*vpp) !=3D NULLVP) { -=09=09if (dvp !=3D NULLVP) -=09=09=09vrele(dvp); -=09=09if (uppervp !=3D NULLVP) -=09=09=09vrele(uppervp); -=09=09if (lowervp !=3D NULLVP) -=09=09=09vrele(lowervp); +=09if (path !=3D NULL && dvp !=3D NULLVP && vt =3D=3D VDIR) { +=09=09*vpp =3D unionfs_ins_cached_vnode(unp, dvp, path, lkflags, td); +=09=09if ((*vpp) !=3D NULLVP) { +=09=09=09if (dvp !=3D NULLVP) +=09=09=09=09vrele(dvp); +=09=09=09if (uppervp !=3D NULLVP) +=09=09=09=09vrele(uppervp); +=09=09=09if (lowervp !=3D NULLVP) +=09=09=09=09vrele(lowervp); =20 -=09=09unp->un_uppervp =3D NULLVP; -=09=09unp->un_lowervp =3D NULLVP; -=09=09unp->un_dvp =3D NULLVP; -=09=09vrele(vp); -=09=09vp =3D *vpp; -=09=09vref(vp); -=09} else -=09=09*vpp =3D vp; +=09=09=09unp->un_uppervp =3D NULLVP; +=09=09=09unp->un_lowervp =3D NULLVP; +=09=09=09unp->un_dvp =3D NULLVP; +=09=09=09vput(vp); +=09=09=09return (0); +=09=09} +=09} +=09*vpp =3D vp; =20 -unionfs_nodeget_out: -=09if (lkflags & LK_TYPE_MASK) -=09=09vn_lock(vp, lkflags | LK_RETRY); - =09return (0); } =20 --=20 John Baldwin