From owner-svn-src-all@FreeBSD.ORG Wed Dec 31 00:40:11 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id DD5ED16F; Wed, 31 Dec 2014 00:40:11 +0000 (UTC) Received: from svn.freebsd.org (svn.freebsd.org [IPv6:2001:1900:2254:2068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id BF9AA2778; Wed, 31 Dec 2014 00:40:11 +0000 (UTC) Received: from svn.freebsd.org ([127.0.1.70]) by svn.freebsd.org (8.14.9/8.14.9) with ESMTP id sBV0eBHj065830; Wed, 31 Dec 2014 00:40:11 GMT (envelope-from rmacklem@FreeBSD.org) Received: (from rmacklem@localhost) by svn.freebsd.org (8.14.9/8.14.9/Submit) id sBV0eBOk065827; Wed, 31 Dec 2014 00:40:11 GMT (envelope-from rmacklem@FreeBSD.org) Message-Id: <201412310040.sBV0eBOk065827@svn.freebsd.org> X-Authentication-Warning: svn.freebsd.org: rmacklem set sender to rmacklem@FreeBSD.org using -f From: Rick Macklem Date: Wed, 31 Dec 2014 00:40:11 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-10@freebsd.org Subject: svn commit: r276437 - stable/10/sys/fs/nfsserver X-SVN-Group: stable-10 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 31 Dec 2014 00:40:12 -0000 Author: rmacklem Date: Wed Dec 31 00:40:10 2014 New Revision: 276437 URL: https://svnweb.freebsd.org/changeset/base/276437 Log: MFC: r276193 A deadlock in the NFSv4 server with vfs.nfsd.enable_locallocks=1 was reported via email. This was caused by a LOR between the sleep lock used to serialize the local locking (nfsrv_locklf()) and locking the vnode. I believe this patch fixes the problem by delaying relocking of the vnode until the sleep lock is unlocked (nfsrv_unlocklf()). To avoid nfsvno_advlock() having the side effect of unlocking the vnode, unlocking the vnode was moved to before the functions that call nfsvno_advlock(). It shouldn't affect the execution of the default case where vfs.nfsd.enable_locallocks=0. Modified: stable/10/sys/fs/nfsserver/nfs_nfsdport.c stable/10/sys/fs/nfsserver/nfs_nfsdstate.c Directory Properties: stable/10/ (props changed) Modified: stable/10/sys/fs/nfsserver/nfs_nfsdport.c ============================================================================== --- stable/10/sys/fs/nfsserver/nfs_nfsdport.c Wed Dec 31 00:34:37 2014 (r276436) +++ stable/10/sys/fs/nfsserver/nfs_nfsdport.c Wed Dec 31 00:40:10 2014 (r276437) @@ -2965,12 +2965,7 @@ nfsvno_advlock(struct vnode *vp, int fty if (nfsrv_dolocallocks == 0) goto out; - - /* Check for VI_DOOMED here, so that VOP_ADVLOCK() isn't performed. */ - if ((vp->v_iflag & VI_DOOMED) != 0) { - error = EPERM; - goto out; - } + ASSERT_VOP_UNLOCKED(vp, "nfsvno_advlock: vp locked"); fl.l_whence = SEEK_SET; fl.l_type = ftype; @@ -2994,14 +2989,12 @@ nfsvno_advlock(struct vnode *vp, int fty fl.l_pid = (pid_t)0; fl.l_sysid = (int)nfsv4_sysid; - NFSVOPUNLOCK(vp, 0); if (ftype == F_UNLCK) error = VOP_ADVLOCK(vp, (caddr_t)td->td_proc, F_UNLCK, &fl, (F_POSIX | F_REMOTE)); else error = VOP_ADVLOCK(vp, (caddr_t)td->td_proc, F_SETLK, &fl, (F_POSIX | F_REMOTE)); - NFSVOPLOCK(vp, LK_EXCLUSIVE | LK_RETRY); out: NFSEXITCODE(error); Modified: stable/10/sys/fs/nfsserver/nfs_nfsdstate.c ============================================================================== --- stable/10/sys/fs/nfsserver/nfs_nfsdstate.c Wed Dec 31 00:34:37 2014 (r276436) +++ stable/10/sys/fs/nfsserver/nfs_nfsdstate.c Wed Dec 31 00:40:10 2014 (r276437) @@ -1344,6 +1344,8 @@ nfsrv_freeallnfslocks(struct nfsstate *s vnode_t tvp = NULL; uint64_t first, end; + if (vp != NULL) + ASSERT_VOP_UNLOCKED(vp, "nfsrv_freeallnfslocks: vnode locked"); lop = LIST_FIRST(&stp->ls_lock); while (lop != LIST_END(&stp->ls_lock)) { nlop = LIST_NEXT(lop, lo_lckowner); @@ -1363,9 +1365,10 @@ nfsrv_freeallnfslocks(struct nfsstate *s if (gottvp == 0) { if (nfsrv_dolocallocks == 0) tvp = NULL; - else if (vp == NULL && cansleep != 0) + else if (vp == NULL && cansleep != 0) { tvp = nfsvno_getvp(&lfp->lf_fh); - else + NFSVOPUNLOCK(tvp, 0); + } else tvp = vp; gottvp = 1; } @@ -1386,7 +1389,7 @@ nfsrv_freeallnfslocks(struct nfsstate *s lop = nlop; } if (vp == NULL && tvp != NULL) - vput(tvp); + vrele(tvp); } /* @@ -1497,7 +1500,7 @@ nfsrv_lockctrl(vnode_t vp, struct nfssta struct nfsclient *clp = NULL; u_int32_t bits; int error = 0, haslock = 0, ret, reterr; - int getlckret, delegation = 0, filestruct_locked; + int getlckret, delegation = 0, filestruct_locked, vnode_unlocked = 0; fhandle_t nfh; uint64_t first, end; uint32_t lock_flags; @@ -1587,6 +1590,11 @@ tryagain: * locking rolled back. */ NFSUNLOCKSTATE(); + if (vnode_unlocked == 0) { + ASSERT_VOP_ELOCKED(vp, "nfsrv_lockctrl1"); + vnode_unlocked = 1; + NFSVOPUNLOCK(vp, 0); + } reterr = nfsrv_locallock(vp, lfp, (new_lop->lo_flags & (NFSLCK_READ | NFSLCK_WRITE)), new_lop->lo_first, new_lop->lo_end, cfp, p); @@ -1748,6 +1756,11 @@ tryagain: if (filestruct_locked != 0) { /* Roll back local locks. */ NFSUNLOCKSTATE(); + if (vnode_unlocked == 0) { + ASSERT_VOP_ELOCKED(vp, "nfsrv_lockctrl2"); + vnode_unlocked = 1; + NFSVOPUNLOCK(vp, 0); + } nfsrv_locallock_rollback(vp, lfp, p); NFSLOCKSTATE(); nfsrv_unlocklf(lfp); @@ -1825,6 +1838,12 @@ tryagain: if (filestruct_locked != 0) { /* Roll back local locks. */ NFSUNLOCKSTATE(); + if (vnode_unlocked == 0) { + ASSERT_VOP_ELOCKED(vp, + "nfsrv_lockctrl3"); + vnode_unlocked = 1; + NFSVOPUNLOCK(vp, 0); + } nfsrv_locallock_rollback(vp, lfp, p); NFSLOCKSTATE(); nfsrv_unlocklf(lfp); @@ -1844,6 +1863,8 @@ tryagain: bits = tstp->ls_flags; bits >>= NFSLCK_SHIFT; if (new_stp->ls_flags & bits & NFSLCK_ACCESSBITS) { + KASSERT(vnode_unlocked == 0, + ("nfsrv_lockctrl: vnode unlocked1")); ret = nfsrv_clientconflict(tstp->ls_clp, &haslock, vp, p); if (ret == 1) { @@ -1875,6 +1896,8 @@ tryagain: * For setattr, just get rid of all the Delegations for other clients. */ if (new_stp->ls_flags & NFSLCK_SETATTR) { + KASSERT(vnode_unlocked == 0, + ("nfsrv_lockctrl: vnode unlocked2")); ret = nfsrv_cleandeleg(vp, lfp, clp, &haslock, p); if (ret) { /* @@ -1925,14 +1948,26 @@ tryagain: (new_lop->lo_flags & NFSLCK_WRITE) && (clp != tstp->ls_clp || (tstp->ls_flags & NFSLCK_DELEGREAD)))) { + ret = 0; if (filestruct_locked != 0) { /* Roll back local locks. */ NFSUNLOCKSTATE(); + if (vnode_unlocked == 0) { + ASSERT_VOP_ELOCKED(vp, "nfsrv_lockctrl4"); + NFSVOPUNLOCK(vp, 0); + } nfsrv_locallock_rollback(vp, lfp, p); NFSLOCKSTATE(); nfsrv_unlocklf(lfp); + NFSUNLOCKSTATE(); + NFSVOPLOCK(vp, LK_EXCLUSIVE | LK_RETRY); + vnode_unlocked = 0; + if ((vp->v_iflag & VI_DOOMED) != 0) + ret = NFSERR_SERVERFAULT; + NFSLOCKSTATE(); } - ret = nfsrv_delegconflict(tstp, &haslock, p, vp); + if (ret == 0) + ret = nfsrv_delegconflict(tstp, &haslock, p, vp); if (ret) { /* * nfsrv_delegconflict unlocks state when it @@ -1971,6 +2006,11 @@ tryagain: stateidp->other[2] = stp->ls_stateid.other[2]; if (filestruct_locked != 0) { NFSUNLOCKSTATE(); + if (vnode_unlocked == 0) { + ASSERT_VOP_ELOCKED(vp, "nfsrv_lockctrl5"); + vnode_unlocked = 1; + NFSVOPUNLOCK(vp, 0); + } /* Update the local locks. */ nfsrv_localunlock(vp, lfp, first, end, p); NFSLOCKSTATE(); @@ -2001,14 +2041,29 @@ tryagain: FREE((caddr_t)other_lop, M_NFSDLOCK); other_lop = NULL; } - ret = nfsrv_clientconflict(lop->lo_stp->ls_clp,&haslock,vp,p); + if (vnode_unlocked != 0) + ret = nfsrv_clientconflict(lop->lo_stp->ls_clp, &haslock, + NULL, p); + else + ret = nfsrv_clientconflict(lop->lo_stp->ls_clp, &haslock, + vp, p); if (ret == 1) { if (filestruct_locked != 0) { + if (vnode_unlocked == 0) { + ASSERT_VOP_ELOCKED(vp, "nfsrv_lockctrl6"); + NFSVOPUNLOCK(vp, 0); + } /* Roll back local locks. */ nfsrv_locallock_rollback(vp, lfp, p); NFSLOCKSTATE(); nfsrv_unlocklf(lfp); NFSUNLOCKSTATE(); + NFSVOPLOCK(vp, LK_EXCLUSIVE | LK_RETRY); + vnode_unlocked = 0; + if ((vp->v_iflag & VI_DOOMED) != 0) { + error = NFSERR_SERVERFAULT; + goto out; + } } /* * nfsrv_clientconflict() unlocks state when it @@ -2042,6 +2097,11 @@ tryagain: if (filestruct_locked != 0 && ret == 0) { /* Roll back local locks. */ NFSUNLOCKSTATE(); + if (vnode_unlocked == 0) { + ASSERT_VOP_ELOCKED(vp, "nfsrv_lockctrl7"); + vnode_unlocked = 1; + NFSVOPUNLOCK(vp, 0); + } nfsrv_locallock_rollback(vp, lfp, p); NFSLOCKSTATE(); nfsrv_unlocklf(lfp); @@ -2120,6 +2180,11 @@ out: nfsv4_unlock(&nfsv4rootfs_lock, 1); NFSUNLOCKV4ROOTMUTEX(); } + if (vnode_unlocked != 0) { + NFSVOPLOCK(vp, LK_EXCLUSIVE | LK_RETRY); + if (error == 0 && (vp->v_iflag & VI_DOOMED) != 0) + error = NFSERR_SERVERFAULT; + } if (other_lop) FREE((caddr_t)other_lop, M_NFSDLOCK); NFSEXITCODE2(error, nd); @@ -3227,11 +3292,14 @@ nfsrv_openupdate(vnode_t vp, struct nfss /* Get the lf lock */ nfsrv_locklf(lfp); NFSUNLOCKSTATE(); + ASSERT_VOP_ELOCKED(vp, "nfsrv_openupdate"); + NFSVOPUNLOCK(vp, 0); if (nfsrv_freeopen(stp, vp, 1, p) == 0) { NFSLOCKSTATE(); nfsrv_unlocklf(lfp); NFSUNLOCKSTATE(); } + NFSVOPLOCK(vp, LK_EXCLUSIVE | LK_RETRY); } else { (void) nfsrv_freeopen(stp, NULL, 0, p); NFSUNLOCKSTATE(); @@ -4619,7 +4687,7 @@ static int nfsrv_clientconflict(struct nfsclient *clp, int *haslockp, vnode_t vp, NFSPROC_T *p) { - int gotlock, lktype; + int gotlock, lktype = 0; /* * If lease hasn't expired, we can't fix it. @@ -4629,8 +4697,10 @@ nfsrv_clientconflict(struct nfsclient *c return (0); if (*haslockp == 0) { NFSUNLOCKSTATE(); - lktype = NFSVOPISLOCKED(vp); - NFSVOPUNLOCK(vp, 0); + if (vp != NULL) { + lktype = NFSVOPISLOCKED(vp); + NFSVOPUNLOCK(vp, 0); + } NFSLOCKV4ROOTMUTEX(); nfsv4_relref(&nfsv4rootfs_lock); do { @@ -4639,11 +4709,12 @@ nfsrv_clientconflict(struct nfsclient *c } while (!gotlock); NFSUNLOCKV4ROOTMUTEX(); *haslockp = 1; - NFSVOPLOCK(vp, lktype | LK_RETRY); - if ((vp->v_iflag & VI_DOOMED) != 0) - return (2); - else - return (1); + if (vp != NULL) { + NFSVOPLOCK(vp, lktype | LK_RETRY); + if ((vp->v_iflag & VI_DOOMED) != 0) + return (2); + } + return (1); } NFSUNLOCKSTATE(); @@ -4684,7 +4755,7 @@ nfsrv_delegconflict(struct nfsstate *stp vnode_t vp) { struct nfsclient *clp = stp->ls_clp; - int gotlock, error, lktype, retrycnt, zapped_clp; + int gotlock, error, lktype = 0, retrycnt, zapped_clp; nfsv4stateid_t tstateid; fhandle_t tfh; @@ -4801,8 +4872,10 @@ nfsrv_delegconflict(struct nfsstate *stp */ if (*haslockp == 0) { NFSUNLOCKSTATE(); - lktype = NFSVOPISLOCKED(vp); - NFSVOPUNLOCK(vp, 0); + if (vp != NULL) { + lktype = NFSVOPISLOCKED(vp); + NFSVOPUNLOCK(vp, 0); + } NFSLOCKV4ROOTMUTEX(); nfsv4_relref(&nfsv4rootfs_lock); do { @@ -4811,14 +4884,16 @@ nfsrv_delegconflict(struct nfsstate *stp } while (!gotlock); NFSUNLOCKV4ROOTMUTEX(); *haslockp = 1; - NFSVOPLOCK(vp, lktype | LK_RETRY); - if ((vp->v_iflag & VI_DOOMED) != 0) { - *haslockp = 0; - NFSLOCKV4ROOTMUTEX(); - nfsv4_unlock(&nfsv4rootfs_lock, 1); - NFSUNLOCKV4ROOTMUTEX(); - error = NFSERR_PERM; - goto out; + if (vp != NULL) { + NFSVOPLOCK(vp, lktype | LK_RETRY); + if ((vp->v_iflag & VI_DOOMED) != 0) { + *haslockp = 0; + NFSLOCKV4ROOTMUTEX(); + nfsv4_unlock(&nfsv4rootfs_lock, 1); + NFSUNLOCKV4ROOTMUTEX(); + error = NFSERR_PERM; + goto out; + } } error = -1; goto out;