Should DEBUG_VFS_LOCKS messages be reported as bugs?
John Baldwin
jhb at freebsd.org
Thu Dec 24 17:25:43 UTC 2015
On Thursday, December 24, 2015 10:56:09 AM Daniel Braniss wrote:
>
> > On 24 Dec 2015, at 09:40, Yuri <yuri at rawbw.com> wrote:
> >
> > 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’s harmless.
> >
> > Which OS version? I get the messages on 10.2-STABLE with options from my OP.
> >
> > 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_elocked+0x6e
> > Dec 23 17:59:36 yuri kernel: #2 0xffffffff809ea6b1 at insmntque1+0x41
> > Dec 23 17:59:36 yuri kernel: #3 0xffffffff82e202a1 at unionfs_nodeget+0x281
> > Dec 23 17:59:36 yuri kernel: #4 0xffffffff82e23b06 at unionfs_lookup+0x726
> > Dec 23 17:59:36 yuri kernel: #5 0xffffffff80e7776f at VOP_CACHEDLOOKUP_APV+0x10f
> > Dec 23 17:59:36 yuri kernel: #6 0xffffffff809d92b6 at vfs_cache_lookup+0xd6
> > Dec 23 17:59:36 yuri kernel: #7 0xffffffff80e775af at VOP_LOOKUP_APV+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+0x2d
> > Dec 23 17:59:36 yuri kernel: #12 0xffffffff80d4fff4 at amd64_syscall+0x2c4
> > Dec 23 17:59:36 yuri kernel: #13 0xffffffff80d339db at Xfast_syscall+0xfb
> > Dec 23 17:59:36 yuri kernel: insmntque: non-locked vp: 0xfffff80046d233b0 is not exclusive locked but should be
> >
> > Yuri
>
> i have been using unionfs since it appeared, and on 10.2-stable, but i don’t have WITNESS on.
> BTW, it’s very slow if you turn WITNESS on, and so should only 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 probably
needs to lock the lockmgr lock sooner (though it has to wait until it sets
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
===================================================================
--- union_subr.c (revision 292560)
+++ union_subr.c (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,
- struct vnode *dvp, char *path)
+ struct vnode *dvp, char *path, int flags, struct thread *td)
{
struct unionfs_node_hashhead *hd;
struct unionfs_node *unp;
@@ -123,12 +123,9 @@ unionfs_get_cached_vnode(struct vnode *uvp, struct
vp = UNIONFSTOV(unp);
VI_LOCK_FLAGS(vp, MTX_DUPOK);
VI_UNLOCK(dvp);
- vp->v_iflag &= ~VI_OWEINACT;
- if ((vp->v_iflag & (VI_DOOMED | VI_DOINGINACT)) != 0) {
- VI_UNLOCK(vp);
- vp = NULLVP;
- } else
- VI_UNLOCK(vp);
+ error = vget(vp, flags | LK_INTERLOCK, td);
+ if (error)
+ return (NULLVP);
return (vp);
}
}
@@ -142,7 +139,7 @@ unionfs_get_cached_vnode(struct vnode *uvp, struct
*/
static struct vnode *
unionfs_ins_cached_vnode(struct unionfs_node *uncp,
- struct vnode *dvp, char *path)
+ struct vnode *dvp, char *path, int flags, struct thread *td)
{
struct unionfs_node_hashhead *hd;
struct unionfs_node *unp;
@@ -153,6 +150,7 @@ unionfs_ins_cached_vnode(struct unionfs_node *uncp
KASSERT((uncp->un_lowervp==NULLVP || uncp->un_lowervp->v_type==VDIR),
("unionfs_ins_cached_vnode: v_type != VDIR"));
+retry:
VI_LOCK(dvp);
hd = unionfs_get_hashhead(dvp, path);
LIST_FOREACH(unp, hd, un_hash) {
@@ -159,14 +157,10 @@ unionfs_ins_cached_vnode(struct unionfs_node *uncp
if (!strcmp(unp->un_path, path)) {
vp = UNIONFSTOV(unp);
VI_LOCK_FLAGS(vp, MTX_DUPOK);
- vp->v_iflag &= ~VI_OWEINACT;
- if ((vp->v_iflag & (VI_DOOMED | VI_DOINGINACT)) != 0) {
- LIST_INSERT_HEAD(hd, uncp, un_hash);
- VI_UNLOCK(vp);
- vp = NULLVP;
- } else
- VI_UNLOCK(vp);
VI_UNLOCK(dvp);
+ error = vget(vp2, flags | LK_INTERLOCK, td);
+ if (error == ENOENT && (flags & LK_NOWAIT) == 0)
+ goto retry;
return (vp);
}
}
@@ -218,7 +212,7 @@ unionfs_nodeget(struct mount *mp, struct vnode *up
char *path;
ump = MOUNTTOUNIONFSMOUNT(mp);
- lkflags = (cnp ? cnp->cn_lkflags : 0);
+ lkflags = (cnp ? cnp->cn_lkflags : LK_EXCLUSIVE);
path = (cnp ? cnp->cn_nameptr : NULL);
*vpp = NULLVP;
@@ -233,19 +227,30 @@ unionfs_nodeget(struct mount *mp, struct vnode *up
/* check the cache */
if (path != NULL && dvp != NULLVP && vt == VDIR) {
- vp = unionfs_get_cached_vnode(uppervp, lowervp, dvp, path);
+ vp = unionfs_get_cached_vnode(uppervp, lowervp, dvp, path,
+ lkflags, td);
if (vp != NULLVP) {
- vref(vp);
*vpp = vp;
- goto unionfs_nodeget_out;
+ return (0);
}
}
+ /*
+ * We must promote to an exclusive lock for vnode creation. This
+ * can happen if lookup is passed LOCKSHARED.
+ */
+ if ((lkflags & LK_TYPE_MASK) == LK_SHARED) {
+ lkflags &= ~LK_TYPE_MASK;
+ lkflags |= LK_EXCLUSIVE;
+ }
+
if ((uppervp == NULLVP || ump->um_uppervp != uppervp) ||
(lowervp == NULLVP || ump->um_lowervp != lowervp)) {
/* dvp will be NULLVP only in case of root vnode. */
- if (dvp == NULLVP)
+ if (dvp == NULLVP) {
+ *vpp = NULL;
return (EINVAL);
+ }
}
unp = malloc(sizeof(struct unionfs_node),
M_UNIONFSNODE, M_WAITOK | M_ZERO);
@@ -253,11 +258,19 @@ unionfs_nodeget(struct mount *mp, struct vnode *up
error = getnewvnode("unionfs", mp, &unionfs_vnodeops, &vp);
if (error != 0) {
free(unp, M_UNIONFSNODE);
+ *vpp = NULL;
return (error);
}
+ if (uppervp != NULLVP)
+ vp->v_vnlock = uppervp->v_vnlock;
+ else
+ vp->v_vnlock = lowervp->v_vnlock;
+ lockmgr(vp->v_vnlock, LK_EXCLUSIVE, NULL);
error = insmntque(vp, mp); /* XXX: Too early for mpsafe fs */
if (error != 0) {
+ vput(vp);
free(unp, M_UNIONFSNODE);
+ *vpp = NULL;
return (error);
}
if (dvp != NULLVP)
@@ -275,10 +288,6 @@ unionfs_nodeget(struct mount *mp, struct vnode *up
unp->un_uppervp = uppervp;
unp->un_lowervp = lowervp;
unp->un_dvp = dvp;
- if (uppervp != NULLVP)
- vp->v_vnlock = uppervp->v_vnlock;
- else
- vp->v_vnlock = lowervp->v_vnlock;
if (path != NULL) {
unp->un_path = (char *)
@@ -293,29 +302,25 @@ unionfs_nodeget(struct mount *mp, struct vnode *up
(lowervp != NULLVP && ump->um_lowervp == lowervp))
vp->v_vflag |= VV_ROOT;
- if (path != NULL && dvp != NULLVP && vt == VDIR)
- *vpp = unionfs_ins_cached_vnode(unp, dvp, path);
- if ((*vpp) != NULLVP) {
- if (dvp != NULLVP)
- vrele(dvp);
- if (uppervp != NULLVP)
- vrele(uppervp);
- if (lowervp != NULLVP)
- vrele(lowervp);
+ if (path != NULL && dvp != NULLVP && vt == VDIR) {
+ *vpp = unionfs_ins_cached_vnode(unp, dvp, path, lkflags, td);
+ if ((*vpp) != NULLVP) {
+ if (dvp != NULLVP)
+ vrele(dvp);
+ if (uppervp != NULLVP)
+ vrele(uppervp);
+ if (lowervp != NULLVP)
+ vrele(lowervp);
- unp->un_uppervp = NULLVP;
- unp->un_lowervp = NULLVP;
- unp->un_dvp = NULLVP;
- vrele(vp);
- vp = *vpp;
- vref(vp);
- } else
- *vpp = vp;
+ unp->un_uppervp = NULLVP;
+ unp->un_lowervp = NULLVP;
+ unp->un_dvp = NULLVP;
+ vput(vp);
+ return (0);
+ }
+ }
+ *vpp = vp;
-unionfs_nodeget_out:
- if (lkflags & LK_TYPE_MASK)
- vn_lock(vp, lkflags | LK_RETRY);
-
return (0);
}
--
John Baldwin
More information about the freebsd-hackers
mailing list