Date: Thu, 24 Dec 2015 09:19:34 -0800 From: John Baldwin <jhb@freebsd.org> To: freebsd-hackers@freebsd.org Cc: Daniel Braniss <danny@cs.huji.ac.il>, Yuri <yuri@rawbw.com>, Rick Macklem <rmacklem@uoguelph.ca>, 'Konstantin Belousov' <kib@freebsd.org> Subject: Re: Should DEBUG_VFS_LOCKS messages be reported as bugs? Message-ID: <4247765.dyXyDaofWi@ralph.baldwin.cx> In-Reply-To: <E6FCF3FE-A00C-4591-9D13-F309715DBF6B@cs.huji.ac.il> References: <567791E9.50207@rawbw.com> <567BA189.8080405@rawbw.com> <E6FCF3FE-A00C-4591-9D13-F309715DBF6B@cs.huji.ac.il>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday, December 24, 2015 10:56:09 AM Daniel Braniss wrote:
>=20
> > On 24 Dec 2015, at 09:40, Yuri <yuri@rawbw.com> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4247765.dyXyDaofWi>