Date: Thu, 18 Aug 2016 00:03:33 +0000 From: Rick Macklem <rmacklem@uoguelph.ca> To: Konstantin Belousov <kostikbel@gmail.com> Cc: "kib@FreeBSD.org" <kib@FreeBSD.org>, Mark Johnston <markj@freebsd.org>, FreeBSD Stable <freebsd-stable@freebsd.org>, Harry Schmalzbauer <freebsd@omnilan.de> 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: <YTXPR01MB018919BE87B12E458144E218DD140@YTXPR01MB0189.CANPRD01.PROD.OUTLOOK.COM> In-Reply-To: <20160812123950.GO83214@kib.kiev.ua> References: <57A79E24.8000100@omnilan.de> <YQBPR01MB0401201977AEA8A803F27B23DD1A0@YQBPR01MB0401.CANPRD01.PROD.OUTLOOK.COM> <57A83C78.1070403@omnilan.de> <20160809060213.GA67664@raichu> <57A9A6C0.9060609@omnilan.de> <YTOPR01MB0412B2A08F1A3C1A3B2EB160DD1E0@YTOPR01MB0412.CANPRD01.PROD.OUTLOOK.COM>, <20160812123950.GO83214@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] Kostik wrote: [stuff snipped] >insmnque() performs the cleanup on its own, and that default cleanup isnot suitable >for the situation. I think that insmntque1() would betterfit your requirements, your >need to move the common code into a helper.It seems that >unionfs_ins_cached_vnode() cleanup could reuse it. <https://lists.freebsd.org>; I've attached an updated patch (untested like the last one). This one creates a custom version insmntque_stddtr() that first calls unionfs_noderem() and then does the same stuff as insmntque_stddtr(). This looks like it does the required stuff (unionfs_noderem() is what the unionfs VOP_RECLAIM() does). It switches the node back to using its own v_vnlock that is exclusively locked, among other things. rick [-- Attachment #2 --] --- fs/unionfs/union_subr.c.sav 2016-08-11 18:20:10.585999000 -0400 +++ fs/unionfs/union_subr.c 2016-08-17 19:38:14.795023000 -0400 @@ -101,6 +101,23 @@ unionfs_get_hashhead(struct vnode *dvp, } /* + * Clean up function for insmntque1(). + * First, call unionfs_noderem() and then do the same as insmntque_stddtr(). + * Maybe insmntque_stddtr() should become non-static, so I can just call + * it here? + */ +static void +unionfs_insmntque_dtr(struct vnode *vp, void *dtr_arg) +{ + + unionfs_noderem(vp, curthread); + vp->v_data = NULL; + vp->v_op = &dead_vnodeops; + vgone(vp); + vput(vp); +} + +/* * Get the cached vnode. */ static struct vnode * @@ -255,11 +272,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 +305,18 @@ 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 = insmntque1(vp, mp, unionfs_insmntque_dtr, NULL); + if (error != 0) + 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 +338,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?YTXPR01MB018919BE87B12E458144E218DD140>