Date: Wed, 8 Dec 2021 00:12:46 GMT From: "Jason A. Harmening" <jah@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: cfc2cfeca195 - main - unionfs: implement VOP_VPUT_PAIR Message-ID: <202112080012.1B80CkCw029124@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by jah: URL: https://cgit.FreeBSD.org/src/commit/?id=cfc2cfeca195c9ad4a4b50aaf87b466cb9458f05 commit cfc2cfeca195c9ad4a4b50aaf87b466cb9458f05 Author: Jason A. Harmening <jah@FreeBSD.org> AuthorDate: 2021-11-15 16:45:20 +0000 Commit: Jason A. Harmening <jah@FreeBSD.org> CommitDate: 2021-12-08 00:20:02 +0000 unionfs: implement VOP_VPUT_PAIR unionfs must pass VOP_VPUT_PAIR directly to the underlying FS so that it can have a chance to manage any special locking considerations that may be necessary. The unionfs implementation is based heavily on the corresponding nullfs implementation. Also note some outstanding issues with the unionfs locking scheme, as a first step in fixing those issues in a future change. Discussed with: kib Tested by: pho Differential Revision: https://reviews.freebsd.org/D33008 --- sys/fs/unionfs/union_vnops.c | 133 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+) diff --git a/sys/fs/unionfs/union_vnops.c b/sys/fs/unionfs/union_vnops.c index 381e413e18ba..ef0c4300ff0b 100644 --- a/sys/fs/unionfs/union_vnops.c +++ b/sys/fs/unionfs/union_vnops.c @@ -1868,6 +1868,16 @@ unionfs_lock(struct vop_lock1_args *ap) KASSERT_UNIONFS_VNODE(ap->a_vp); + /* + * TODO: rework the unionfs locking scheme. + * It's not guaranteed to be safe to blindly lock two vnodes on + * different mounts as is done here. Further, the entanglement + * of locking both vnodes with the various options that can be + * passed to VOP_LOCK() makes this code hard to reason about. + * Instead, consider locking only the upper vnode, or the lower + * vnode is the upper is not present, and taking separate measures + * to lock both vnodes in the few cases when that is needed. + */ error = 0; interlock = 1; uhold = 0; @@ -2535,6 +2545,128 @@ unionfs_add_writecount(struct vop_add_writecount_args *ap) return (error); } +static int +unionfs_vput_pair(struct vop_vput_pair_args *ap) +{ + struct mount *mp; + struct vnode *dvp, *vp, **vpp, *lvp, *ldvp, *uvp, *udvp, *tempvp; + struct unionfs_node *dunp, *unp; + int error, res; + + dvp = ap->a_dvp; + vpp = ap->a_vpp; + vp = NULLVP; + lvp = NULLVP; + uvp = NULLVP; + unp = NULL; + + dunp = VTOUNIONFS(dvp); + udvp = dunp->un_uppervp; + ldvp = dunp->un_lowervp; + + /* + * Underlying vnodes should be locked because the encompassing unionfs + * node is locked, but will not be referenced, as the reference will + * only be on the unionfs node. Reference them now so that the vput()s + * performed by VOP_VPUT_PAIR() will have a reference to drop. + */ + if (udvp != NULLVP) + vref(udvp); + if (ldvp != NULLVP) + vref(ldvp); + + if (vpp != NULL) + vp = *vpp; + + if (vp != NULLVP) { + unp = VTOUNIONFS(vp); + uvp = unp->un_uppervp; + lvp = unp->un_lowervp; + if (uvp != NULLVP) + vref(uvp); + if (lvp != NULLVP) + vref(lvp); + + /* + * If we're being asked to return a locked child vnode, then + * we may need to create a replacement vnode in case the + * original is reclaimed while the lock is dropped. In that + * case we'll need to ensure the mount and the underlying + * vnodes aren't also recycled during that window. + */ + if (!ap->a_unlock_vp) { + vhold(vp); + if (uvp != NULLVP) + vhold(uvp); + if (lvp != NULLVP) + vhold(lvp); + mp = vp->v_mount; + vfs_ref(mp); + } + } + + /* + * TODO: Because unionfs_lock() locks both the lower and upper vnodes + * (if available), we must also call VOP_VPUT_PAIR() on both the lower + * and upper parent/child pairs. If unionfs_lock() is reworked to lock + * only a single vnode, this code will need to change to also only + * operate on one vnode pair. + */ + ASSERT_VOP_LOCKED(ldvp, __func__); + ASSERT_VOP_LOCKED(udvp, __func__); + ASSERT_VOP_LOCKED(lvp, __func__); + ASSERT_VOP_LOCKED(uvp, __func__); + + KASSERT(lvp == NULLVP || ldvp != NULLVP, + ("%s: NULL ldvp with non-NULL lvp", __func__)); + if (ldvp != NULLVP) + res = VOP_VPUT_PAIR(ldvp, lvp != NULLVP ? &lvp : NULL, true); + KASSERT(uvp == NULLVP || udvp != NULLVP, + ("%s: NULL udvp with non-NULL uvp", __func__)); + if (udvp != NULLVP) + res = VOP_VPUT_PAIR(udvp, uvp != NULLVP ? &uvp : NULL, true); + + ASSERT_VOP_UNLOCKED(ldvp, __func__); + ASSERT_VOP_UNLOCKED(udvp, __func__); + ASSERT_VOP_UNLOCKED(lvp, __func__); + ASSERT_VOP_UNLOCKED(uvp, __func__); + + /* + * VOP_VPUT_PAIR() dropped the references we added to the underlying + * vnodes, now drop the caller's reference to the unionfs vnodes. + */ + if (vp != NULLVP && ap->a_unlock_vp) + vrele(vp); + vrele(dvp); + + if (vp == NULLVP || ap->a_unlock_vp) + return (res); + + /* + * We're being asked to return a locked vnode. At this point, the + * underlying vnodes have been unlocked, so vp may have been reclaimed. + */ + vn_lock(vp, LK_EXCLUSIVE | LK_RETRY); + if (vp->v_data == NULL && vfs_busy(mp, MBF_NOWAIT) == 0) { + vput(vp); + error = unionfs_nodeget(mp, uvp, lvp, dvp, &tempvp, NULL); + if (error == 0) { + vn_lock(tempvp, LK_EXCLUSIVE | LK_RETRY); + *vpp = tempvp; + } else + vget(vp, LK_EXCLUSIVE | LK_RETRY); + vfs_unbusy(mp); + } + if (lvp != NULLVP) + vdrop(lvp); + if (uvp != NULLVP) + vdrop(uvp); + vdrop(vp); + vfs_rel(mp); + + return (res); +} + struct vop_vector unionfs_vnodeops = { .vop_default = &default_vnodeops, @@ -2585,5 +2717,6 @@ struct vop_vector unionfs_vnodeops = { .vop_write = unionfs_write, .vop_vptofh = unionfs_vptofh, .vop_add_writecount = unionfs_add_writecount, + .vop_vput_pair = unionfs_vput_pair, }; VFS_VOP_VECTOR_REGISTER(unionfs_vnodeops);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202112080012.1B80CkCw029124>