From 17a845c8d44f453b09b21afd59182e616234e872 Mon Sep 17 00:00:00 2001 From: Tim Creech Date: Sun, 5 Mar 2017 18:15:58 -0500 Subject: [PATCH] FBSD: Remove LOCKPARENT/ISLASTCN lookup logic Currently, our afs_vop_lookup on FBSD tries to only lock 'dvp' for ISDOTDOT requests when LOCKPARENT and ISLASTCN are set. There are a couple of problems with this: - The conditional locking logic involving LOCKPARENT/ISLASTCN is only relevant in very old FreeBSD releases (per-fs checking of these flags for parent locking went away around the FreeBSD 6 era). - Our current logic here is wrong anyway, since we try to lock 'dvp' twice when those flags are set. This was mostly introduced by commit 2f6be821 (FBSD: band-aid vnode locking in lookup), which added a lock/unlock pair for 'dvp' around the lock for 'vp', even though 'dvp' was unlocked several lines earlier. This means that if we hit the relevant code path, we will deadlock, since we try to lock 'dvp' twice. To avoid this, just remove the relevant logic for LOCKPARENT/ISLASTCN, since it is only relevant for old FreeBSD releases that are not supported by us or FreeBSD. Add and rearrange some comments around here to try to more explicitly explain the relevant locking rules. [adeason@dson.org: Commit message rewrite, adding comments, removing old FreeBSD code.] Change-Id: Iaa2c55d82c50d5a8ab42c67b0996a2b4fb6e09e6 Reviewed-on: https://gerrit.openafs.org/12578 Tested-by: BuildBot Reviewed-by: Benjamin Kaduk --- src/afs/FBSD/osi_vnodeops.c | 35 +++++++++++++++++++---------------- 1 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/afs/FBSD/osi_vnodeops.c b/src/afs/FBSD/osi_vnodeops.c index d68e804..8847a55 100644 --- a/src/afs/FBSD/osi_vnodeops.c +++ b/src/afs/FBSD/osi_vnodeops.c @@ -202,7 +202,6 @@ afs_vop_lookup(ap) struct vcache *vcp; struct vnode *vp, *dvp; int flags = ap->a_cnp->cn_flags; - int lockparent; /* 1 => lockparent flag is set */ dvp = ap->a_dvp; if (dvp->v_type != VDIR) { @@ -214,12 +213,28 @@ afs_vop_lookup(ap) GETNAME(); - lockparent = flags & LOCKPARENT; - #if __FreeBSD_version < 1000021 cnp->cn_flags |= MPSAFE; /* steel */ #endif + /* + * Locking rules: + * + * - 'dvp' is locked by our caller. We must return it locked, whether we + * return success or error. + * + * - If the lookup succeeds, 'vp' must be locked before we return. + * + * - If we lock multiple vnodes, parent vnodes must be locked before + * children vnodes. + * + * As a result, looking up the parent directory (if 'flags' has ISDOTDOT + * set) is a bit of a special case. In that case, we must unlock 'dvp' + * before performing the lookup, since the lookup operation may lock the + * target vnode, and the target vnode is the parent of 'dvp' (so we must + * lock 'dvp' after locking the target vnode). + */ + if (flags & ISDOTDOT) MA_VOP_UNLOCK(dvp, 0, p); @@ -241,27 +256,15 @@ afs_vop_lookup(ap) } vp = AFSTOV(vcp); /* always get a node if no error */ - /* The parent directory comes in locked. We unlock it on return - * unless the caller wants it left locked. - * we also always return the vnode locked. */ - if (flags & ISDOTDOT) { - /* vp before dvp since we go root to leaf, and .. comes first */ + /* Must lock 'vp' before 'dvp', since 'vp' is the parent vnode. */ ma_vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, p); ma_vn_lock(dvp, LK_EXCLUSIVE | LK_RETRY, p); - /* always return the child locked */ - if (lockparent && (flags & ISLASTCN) - && (error = ma_vn_lock(dvp, LK_EXCLUSIVE, p))) { - vput(vp); - DROPNAME(); - return (error); - } } else if (vp == dvp) { /* they're the same; afs_lookup() already ref'ed the leaf. * It came in locked, so we don't need to ref OR lock it */ } else { ma_vn_lock(vp, LK_EXCLUSIVE | LK_CANRECURSE | LK_RETRY, p); - /* always return the child locked */ } *ap->a_vpp = vp; -- 1.7.1