Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 1 Mar 2012 21:31:03 +0200
From:      Gleb Kurtsou <gleb.kurtsou@gmail.com>
To:        Florian Smeets <flo@FreeBSD.org>
Cc:        decke@FreeBSD.org, Konstantin Belousov <kib@FreeBSD.org>, "current@freebsd.org" <current@FreeBSD.org>
Subject:   Re: Processes getting stuck in state tmpfs
Message-ID:  <20120301193102.GA35301@reks>
In-Reply-To: <4F469F94.5080606@FreeBSD.org>
References:  <4F358F01.1090508@FreeBSD.org> <20120211102006.GA1274@reks> <4F469F94.5080606@FreeBSD.org>

next in thread | previous in thread | raw e-mail | index | archive | help

--C7zPtVaVf+AK4Oqc
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline

Could you test the patch attached.

It's also available here as seperate commits:
https://github.com/glk/freebsd-head/commits/tmpfs-rename

Thanks,
Gleb.

On (23/02/2012 21:20), Florian Smeets wrote:
> On 11.02.12 11:20, Gleb Kurtsou wrote:
> > On (10/02/2012 22:41), Florian Smeets wrote:
> >> Hi,
> >>
> >> if you set WRKDIRPREFIX to a tmpfs mountpoint and try to build audio/gsm
> >> from ports one of the mv processes gets stuck in state tmpfs quite
> >> often. Traces from a kernel with WITTNESS and DEBUG_VFS_LOCKS are
> >> available here http://tb.smeets.im/~flo/tmpfs.txt
> > 
> > It's because of incorrect vnode locking order in tmpfs_rename. Issue is
> > known and tmpfs is not the only file system suffering from it (e.g. ext2).
> > 
> > There two ways of working around it in tree:
> > * UFS: try locking vnode, unlock all vnodes on failure, restart,
> > relookup vnodes needed.
> > * ZFS: introduce directory entry locks to guarantee fvp won't disappear,
> > fdvp can be safely traversed, etc. That won't be easy..
> > 
> > UFS-way would be a good temporal solution, but I think we should work on
> > improving VOP_RENAME() in a long run.
> > 
> > I'll try to prepare a patch in several days.
> > 
> 
> Hey Gleb,
> 
> did you get anywhere with this?
> 
> Thanks,
> Florian
> 



--C7zPtVaVf+AK4Oqc
Content-Type: text/plain; charset=utf-8
Content-Disposition: attachment; filename="tmpfs-rename-deadlock.patch.txt"

diff --git a/sys/fs/tmpfs/tmpfs_subr.c b/sys/fs/tmpfs/tmpfs_subr.c
index d2b2245..fe596aa 100644
--- a/sys/fs/tmpfs/tmpfs_subr.c
+++ b/sys/fs/tmpfs/tmpfs_subr.c
@@ -42,6 +42,7 @@ __FBSDID("$FreeBSD$");
 #include <sys/proc.h>
 #include <sys/stat.h>
 #include <sys/systm.h>
+#include <sys/sysctl.h>
 #include <sys/vnode.h>
 #include <sys/vmmeter.h>
 
@@ -56,6 +57,8 @@ __FBSDID("$FreeBSD$");
 #include <fs/tmpfs/tmpfs_fifoops.h>
 #include <fs/tmpfs/tmpfs_vnops.h>
 
+SYSCTL_NODE(_vfs, OID_AUTO, tmpfs, CTLFLAG_RW, 0, "tmpfs file system");
+
 /* --------------------------------------------------------------------- */
 
 /*
@@ -320,9 +323,11 @@ loop:
 		MPASS((node->tn_vpstate & TMPFS_VNODE_DOOMED) == 0);
 		VI_LOCK(vp);
 		TMPFS_NODE_UNLOCK(node);
-		vholdl(vp);
-		(void) vget(vp, lkflag | LK_INTERLOCK | LK_RETRY, curthread);
-		vdrop(vp);
+		error = vget(vp, lkflag | LK_INTERLOCK, curthread);
+		if (error != 0) {
+			vp = NULL;
+			goto out;
+		}
 
 		/*
 		 * Make sure the vnode is still there after
@@ -420,11 +425,13 @@ unlock:
 out:
 	*vpp = vp;
 
-	MPASS(IFF(error == 0, *vpp != NULL && VOP_ISLOCKED(*vpp)));
 #ifdef INVARIANTS
-	TMPFS_NODE_LOCK(node);
-	MPASS(*vpp == node->tn_vnode);
-	TMPFS_NODE_UNLOCK(node);
+	if (error == 0) {
+		MPASS(*vpp != NULL && VOP_ISLOCKED(*vpp));
+		TMPFS_NODE_LOCK(node);
+		MPASS(*vpp == node->tn_vnode);
+		TMPFS_NODE_UNLOCK(node);
+	}
 #endif
 
 	return error;
diff --git a/sys/fs/tmpfs/tmpfs_vnops.c b/sys/fs/tmpfs/tmpfs_vnops.c
index 93fea8b..dd54679 100644
--- a/sys/fs/tmpfs/tmpfs_vnops.c
+++ b/sys/fs/tmpfs/tmpfs_vnops.c
@@ -46,6 +46,7 @@ __FBSDID("$FreeBSD$");
 #include <sys/sf_buf.h>
 #include <sys/stat.h>
 #include <sys/systm.h>
+#include <sys/sysctl.h>
 #include <sys/unistd.h>
 #include <sys/vnode.h>
 
@@ -58,6 +59,13 @@ __FBSDID("$FreeBSD$");
 #include <fs/tmpfs/tmpfs_vnops.h>
 #include <fs/tmpfs/tmpfs.h>
 
+SYSCTL_DECL(_vfs_tmpfs);
+
+static volatile int tmpfs_rename_restarts;
+SYSCTL_INT(_vfs_tmpfs, OID_AUTO, rename_restarts, CTLFLAG_RD,
+    __DEVOLATILE(int *, &tmpfs_rename_restarts), 0,
+    "Times rename had to restart due to lock contention");
+
 /* --------------------------------------------------------------------- */
 
 static int
@@ -920,6 +928,118 @@ out:
 /* --------------------------------------------------------------------- */
 
 static int
+tmpfs_rename_relock(struct vnode *fdvp, struct vnode **fvpp,
+    struct vnode *tdvp, struct vnode **tvpp,
+    struct componentname *fcnp, struct componentname *tcnp)
+{
+	struct vnode *nvp;
+	struct mount *mp;
+	struct tmpfs_dirent *de;
+	int error;
+
+	VOP_UNLOCK(tdvp, 0);
+	if (*tvpp != NULL && *tvpp != tdvp)
+		VOP_UNLOCK(*tvpp, 0);
+	mp = fdvp->v_mount;
+
+relock:
+	atomic_add_int(&tmpfs_rename_restarts, 1);
+	error = vn_lock(fdvp, LK_EXCLUSIVE);
+	if (error)
+		goto releout;
+	if (vn_lock(tdvp, LK_EXCLUSIVE | LK_NOWAIT) != 0) {
+		VOP_UNLOCK(fdvp, 0);
+		error = vn_lock(tdvp, LK_EXCLUSIVE);
+		if (error)
+			goto releout;
+		VOP_UNLOCK(tdvp, 0);
+		goto relock;
+	}
+	/*
+	 * Re-resolve fvp to be certain it still exists and fetch the
+	 * correct vnode.
+	 */
+	de = tmpfs_dir_lookup(VP_TO_TMPFS_DIR(fdvp), NULL, fcnp);
+	if (de == NULL) {
+		VOP_UNLOCK(fdvp, 0);
+		VOP_UNLOCK(tdvp, 0);
+		if ((fcnp->cn_flags & ISDOTDOT) != 0 ||
+		    (fcnp->cn_namelen == 1 && fcnp->cn_nameptr[0] == '.'))
+			error = EINVAL;
+		else
+			error = ENOENT;
+		goto releout;
+	}
+	error = tmpfs_alloc_vp(mp, de->td_node, LK_EXCLUSIVE | LK_NOWAIT, &nvp);
+	if (error != 0) {
+		VOP_UNLOCK(fdvp, 0);
+		VOP_UNLOCK(tdvp, 0);
+		if (error != EBUSY)
+			goto releout;
+		error = tmpfs_alloc_vp(mp, de->td_node, LK_EXCLUSIVE, &nvp);
+		if (error != 0)
+			goto releout;
+		VOP_UNLOCK(nvp, 0);
+		vrele(*fvpp);
+		*fvpp = nvp;
+		goto relock;
+	}
+	vrele(*fvpp);
+	*fvpp = nvp;
+	VOP_UNLOCK(*fvpp, 0);
+	/*
+	 * Re-resolve tvp and acquire the vnode lock if present.
+	 */
+	de = tmpfs_dir_lookup(VP_TO_TMPFS_DIR(tdvp), NULL, tcnp);
+	/*
+	 * If tvp disappeared we just carry on.
+	 */
+	if (de == NULL && *tvpp != NULL) {
+		vrele(*tvpp);
+		*tvpp = NULL;
+	}
+	/*
+	 * Get the tvp ino if the lookup succeeded.  We may have to restart
+	 * if the non-blocking acquire fails.
+	 */
+	if (de != NULL) {
+		nvp = NULL;
+		error = tmpfs_alloc_vp(mp, de->td_node,
+		    LK_EXCLUSIVE | LK_NOWAIT, &nvp);
+		if (*tvpp != NULL)
+			vrele(*tvpp);
+		*tvpp = nvp;
+		if (error != 0) {
+			VOP_UNLOCK(fdvp, 0);
+			VOP_UNLOCK(tdvp, 0);
+			if (error != EBUSY)
+				goto releout;
+			error = tmpfs_alloc_vp(mp, de->td_node, LK_EXCLUSIVE,
+			    &nvp);
+			if (error != 0)
+				goto releout;
+			VOP_UNLOCK(nvp, 0);
+			if (*tvpp == fdvp) {
+				error = ENOTEMPTY;
+				goto releout;
+			}
+			goto relock;
+		}
+	}
+
+	return (0);
+
+releout:
+	vrele(fdvp);
+	vrele(*fvpp);
+	vrele(tdvp);
+	if (*tvpp != NULL)
+		vrele(*tvpp);
+
+	return (error);
+}
+
+static int
 tmpfs_rename(struct vop_rename_args *v)
 {
 	struct vnode *fdvp = v->a_fdvp;
@@ -928,6 +1048,7 @@ tmpfs_rename(struct vop_rename_args *v)
 	struct vnode *tdvp = v->a_tdvp;
 	struct vnode *tvp = v->a_tvp;
 	struct componentname *tcnp = v->a_tcnp;
+	struct mount *mp = NULL;
 
 	char *newname;
 	int error;
@@ -943,8 +1064,6 @@ tmpfs_rename(struct vop_rename_args *v)
 	MPASS(fcnp->cn_flags & HASBUF);
 	MPASS(tcnp->cn_flags & HASBUF);
 
-  	tnode = (tvp == NULL) ? NULL : VP_TO_TMPFS_NODE(tvp);
-
 	/* Disallow cross-device renames.
 	 * XXX Why isn't this done by the caller? */
 	if (fvp->v_mount != tdvp->v_mount ||
@@ -953,9 +1072,6 @@ tmpfs_rename(struct vop_rename_args *v)
 		goto out;
 	}
 
-	tmp = VFS_TO_TMPFS(tdvp->v_mount);
-	tdnode = VP_TO_TMPFS_DIR(tdvp);
-
 	/* If source and target are the same file, there is nothing to do. */
 	if (fvp == tvp) {
 		error = 0;
@@ -964,8 +1080,30 @@ tmpfs_rename(struct vop_rename_args *v)
 
 	/* If we need to move the directory between entries, lock the
 	 * source so that we can safely operate on it. */
-	if (fdvp != tdvp && fdvp != tvp)
-		vn_lock(fdvp, LK_EXCLUSIVE | LK_RETRY);
+	if (fdvp != tdvp && fdvp != tvp) {
+		if (vn_lock(fdvp, LK_EXCLUSIVE | LK_NOWAIT) != 0) {
+			mp = tdvp->v_mount;
+			error = vfs_busy(mp, 0);
+			if (error != 0) {
+				mp = NULL;
+				goto out;
+			}
+			error = tmpfs_rename_relock(fdvp, &fvp, tdvp, &tvp,
+			    fcnp, tcnp);
+			if (error != 0) {
+				vfs_unbusy(mp);
+				return (error);
+			}
+			if (fvp == tvp) {
+				error = 0;
+				goto out_locked;
+			}
+		}
+	}
+
+	tmp = VFS_TO_TMPFS(tdvp->v_mount);
+	tdnode = VP_TO_TMPFS_DIR(tdvp);
+	tnode = (tvp == NULL) ? NULL : VP_TO_TMPFS_NODE(tvp);
 	fdnode = VP_TO_TMPFS_DIR(fdvp);
 	fnode = VP_TO_TMPFS_NODE(fvp);
 	de = tmpfs_dir_lookup(fdnode, fnode, fcnp);
@@ -1157,6 +1295,9 @@ out:
 	vrele(fdvp);
 	vrele(fvp);
 
+	if (mp != NULL)
+		vfs_unbusy(mp);
+
 	return error;
 }
 

--C7zPtVaVf+AK4Oqc--



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