Date: Thu, 11 Aug 2016 22:53:03 +0000 From: Rick Macklem <rmacklem@uoguelph.ca> To: Harry Schmalzbauer <freebsd@omnilan.de>, Mark Johnston <markj@freebsd.org> Cc: FreeBSD Stable <freebsd-stable@freebsd.org>, "kib@FreeBSD.org" <kib@FreeBSD.org> Subject: Re: unionfs bugs, a partial patch and some comments [Was: Re: 1-BETA3 Panic: __lockmgr_args: downgrade a recursed lockmgr nfs @ /usr/local/share/deploy-tools/RELENG_11/src/sys/fs/unionfs/union_vnops.c:1905] Message-ID: <YTOPR01MB0412B2A08F1A3C1A3B2EB160DD1E0@YTOPR01MB0412.CANPRD01.PROD.OUTLOOK.COM> In-Reply-To: <57A9A6C0.9060609@omnilan.de> References: <57A79E24.8000100@omnilan.de> <YQBPR01MB0401201977AEA8A803F27B23DD1A0@YQBPR01MB0401.CANPRD01.PROD.OUTLOOK.COM> <57A83C78.1070403@omnilan.de> <20160809060213.GA67664@raichu>,<57A9A6C0.9060609@omnilan.de>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] Harry Schmalzbauer wrote: Bezüglich Mark Johnston's Nachricht vom 09.08.2016 08:02 (localtime): … >> >> Just for anybody else needing unionfs: >> https://people.freebsd.org/~attilio/unionfs_missing_insmntque_lock.patch >> >> This patch still applies and I'm successfully using this (unmodified) up >> to FreeBSD-10.3 and never had any panic in all these years. > > Having spent some time looking at unionfs, I'm a bit skeptical that this > patch will address the panic you reported earlier, though I'd be > interested to know if it does. [stuff snipped for brevity] I took a look at this. (I know nothing about unionfs, but a little w.r.t. the VFS). I can confirm that this function (unionfs_nodeget()) is weird and appears to be broken to me. The function calls insmntque() before it initializes the vnode, which seems racey, especially if it isn't LK_EXCLUSIVE locked. Also, line#s 278-281: if (uppervp != NULLVP) vp->v_vnlock = uppervp->v_vnlock; else vp->v_vnlock = lowervp->v_vnlock; so your patch isn't locking the vnode lock that it actually uses. I think the vp argument to insmntque() is required to be LK_EXCLUSIVE locked mostly so other threads won't fiddle with the vnode until this function is done with it, but I am not sure? I think a more correct version of this (not saying it would be correct[😉], would call insmntque() later in the function, after it has been initialized. (This means that the cleanup if it fails is more involved, but...) I've attached a patch (untested) that does this. Maybe you could try it? rick ps: I've cc'd Kostik, in case he has some insight w.r.t. how this should be handled? [-- Attachment #2 --] --- fs/unionfs/union_subr.c.sav 2016-08-11 18:20:10.585999000 -0400 +++ fs/unionfs/union_subr.c 2016-08-11 18:40:51.119220000 -0400 @@ -255,11 +255,6 @@ unionfs_nodeget(struct mount *mp, struct free(unp, M_UNIONFSNODE); return (error); } - error = insmntque(vp, mp); /* XXX: Too early for mpsafe fs */ - if (error != 0) { - free(unp, M_UNIONFSNODE); - return (error); - } if (dvp != NULLVP) vref(dvp); if (uppervp != NULLVP) @@ -293,6 +288,28 @@ unionfs_nodeget(struct mount *mp, struct (lowervp != NULLVP && ump->um_lowervp == lowervp)) vp->v_vflag |= VV_ROOT; + /* + * Not sure if LK_RETRY is needed here? + * Normally, this would be done with a lockmgr() call, but in + * this case, v_vnlock is actually a vnode lock for either the + * uppervp or lowervp, so I used the vn_lock() call. + */ + vn_lock(vp, LK_EXCLUSIVE | LK_RETRY); + error = insmntque(vp, mp); + if (error != 0) { + if (dvp != NULLVP) + vrele(dvp); + if (uppervp != NULLVP) + vrele(uppervp); + if (lowervp != NULLVP) + vrele(lowervp); + free(unp->un_hashtbl, M_UNIONFSHASH); + free(unp->un_path, M_UNIONFSPATH); + free(unp, M_UNIONFSNODE); + return (error); + } + VOP_UNLOCK(vp, 0); + if (path != NULL && dvp != NULLVP && vt == VDIR) *vpp = unionfs_ins_cached_vnode(unp, dvp, path); if ((*vpp) != NULLVP) { @@ -314,6 +331,7 @@ unionfs_nodeget(struct mount *mp, struct unionfs_nodeget_out: if (lkflags & LK_TYPE_MASK) + /* Should there be a check for VI_DOOMED here? */ vn_lock(vp, lkflags | LK_RETRY); return (0);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?YTOPR01MB0412B2A08F1A3C1A3B2EB160DD1E0>