Skip site navigation (1)Skip section navigation (2) 
Date:      Sat, 18 Aug 2018 19:14:06 +0000 (UTC)
From:      Rick Macklem <rmacklem@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r338019 - head/sys/fs/nfsserver
Message-ID:  <201808181914.w7IJE670060701@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: rmacklem
Date: Sat Aug 18 19:14:06 2018
New Revision: 338019
URL: https://svnweb.freebsd.org/changeset/base/338019

Log:
  Fix LORs between vn_start_write() and vn_lock() in nfsrv_copymr().
  
  When coding the pNFS server, I added vn_start_write() calls in nfsrv_copymr()
  done while the vnodes were locked, not realizing I had introduced LORs and
  possible deadlock when an exported file system on the MDS is suspended.
  This patch fixes the LORs by moving the vn_start_write() calls up to before
  where the vnodes are locked. For "tvp", the vn_start_write() probaby isn't
  necessary, because NFS mounts can't be suspended. However, I think doing
  so is harmless.
  Thanks go to kib@ for letting me know that I had introduced these LORs.
  This patch only affects the behaviour of the pNFS server when pnfsdscopymr(8)
  is used to recover a mirrored DS.

Modified:
  head/sys/fs/nfsserver/nfs_nfsdstate.c

Modified: head/sys/fs/nfsserver/nfs_nfsdstate.c
==============================================================================
--- head/sys/fs/nfsserver/nfs_nfsdstate.c	Sat Aug 18 18:33:50 2018	(r338018)
+++ head/sys/fs/nfsserver/nfs_nfsdstate.c	Sat Aug 18 19:14:06 2018	(r338019)
@@ -7979,7 +7979,7 @@ nfsrv_copymr(vnode_t vp, vnode_t fvp, vnode_t dvp, str
 	struct nfslayouthash *lhyp;
 	struct nfslayout *lyp, *nlyp;
 	struct nfslayouthead thl;
-	struct mount *mp;
+	struct mount *mp, *tvmp;
 	struct acl *aclp;
 	struct vattr va;
 	struct timespec mtime;
@@ -8042,6 +8042,7 @@ nfsrv_copymr(vnode_t vp, vnode_t fvp, vnode_t dvp, str
 	NFSDRECALLUNLOCK();
 
 	ret = 0;
+	mp = tvmp = NULL;
 	didprintf = 0;
 	TAILQ_INIT(&thl);
 	/* Unlock the MDS vp, so that a LayoutReturn can be done on it. */
@@ -8116,6 +8117,20 @@ tryagain2:
 		nfsrv_freelayout(&thl, lyp);
 
 	/*
+	 * Do the vn_start_write() calls here, before the MDS vnode is
+	 * locked and the tvp is created (locked) in the NFS file system
+	 * that dvp is in.
+	 * For tvmp, this probably isn't necessary, since it will be an
+	 * NFS mount and they are not suspendable at this time.
+	 */
+	if (ret == 0)
+		ret = vn_start_write(vp, &mp, V_WAIT | PCATCH);
+	if (ret == 0) {
+		tvmp = dvp->v_mount;
+		ret = vn_start_write(NULL, &tvmp, V_WAIT | PCATCH);
+	}
+
+	/*
 	 * LK_EXCLUSIVE lock the MDS vnode, so that any
 	 * proxied writes through the MDS will be blocked until we have
 	 * completed the copy and update of the extended attributes.
@@ -8123,7 +8138,7 @@ tryagain2:
 	 * changed until the copy is complete.
 	 */
 	NFSVOPLOCK(vp, LK_EXCLUSIVE | LK_RETRY);
-	if ((vp->v_iflag & VI_DOOMED) != 0) {
+	if (ret == 0 && (vp->v_iflag & VI_DOOMED) != 0) {
 		NFSD_DEBUG(4, "nfsrv_copymr: lk_exclusive doomed\n");
 		ret = ESTALE;
 	}
@@ -8148,10 +8163,7 @@ tryagain2:
 			nfsrv_zeropnfsdat = malloc(PNFSDS_COPYSIZ, M_TEMP,
 			    M_WAITOK | M_ZERO);
 		rdpos = wrpos = 0;
-		mp = NULL;
-		ret = vn_start_write(tvp, &mp, V_WAIT | PCATCH);
-		if (ret == 0)
-			ret = VOP_GETATTR(fvp, &va, cred);
+		ret = VOP_GETATTR(fvp, &va, cred);
 		aresid = 0;
 		while (ret == 0 && aresid == 0) {
 			ret = vn_rdwr(UIO_READ, fvp, dat, PNFSDS_COPYSIZ,
@@ -8191,8 +8203,6 @@ tryagain2:
 				ret = 0;
 		}
 
-		if (mp != NULL)
-			vn_finished_write(mp);
 		if (ret == 0)
 			ret = VOP_FSYNC(tvp, MNT_WAIT, p);
 
@@ -8210,18 +8220,16 @@ tryagain2:
 		acl_free(aclp);
 		free(dat, M_TEMP);
 	}
+	if (tvmp != NULL)
+		vn_finished_write(tvmp);
 
 	/* Update the extended attributes for the newly created DS file. */
-	if (ret == 0) {
-		mp = NULL;
-		ret = vn_start_write(vp, &mp, V_WAIT | PCATCH);
-		if (ret == 0)
-			ret = vn_extattr_set(vp, IO_NODELOCKED,
-			    EXTATTR_NAMESPACE_SYSTEM, "pnfsd.dsfile",
-			    sizeof(*wpf) * mirrorcnt, (char *)wpf, p);
-		if (mp != NULL)
-			vn_finished_write(mp);
-	}
+	if (ret == 0)
+		ret = vn_extattr_set(vp, IO_NODELOCKED,
+		    EXTATTR_NAMESPACE_SYSTEM, "pnfsd.dsfile",
+		    sizeof(*wpf) * mirrorcnt, (char *)wpf, p);
+	if (mp != NULL)
+		vn_finished_write(mp);
 
 	/* Get rid of the dontlist entry, so that Layouts can be issued. */
 	NFSDDONTLISTLOCK();

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201808181914.w7IJE670060701>