Module Name:    src
Committed By:   hannken
Date:           Thu Dec  1 14:49:04 UTC 2016

Modified Files:
        src/sys/kern: vfs_vnode.c
        src/tests/fs/puffs: t_basic.c

Log Message:
- Change vcache_reclaim() to always call VOP_INACTIVE() before VOP_RECLAIM().
  When called from vrecycle() or vgone() there is a window where the refcount
  is greater than zero and another thread could get and release a reference
  that would miss VOP_INACTIVE() as the refcount doesn't drop to zero.

  Adjust test fs/puffs/t_basic:  test VOP_INACTIVE count being greater zero.

- Make vrecycle() more robust by checking v_usecount first and preventing
  further references across vn_lock().  Fixes a deadlock where one thread
  starts unmount, second thread locks a directory and allocates a vnode
  and first thread tries to vrecycle() the directory.
  First thread holds vfs_busy and wants vnode, second thread holds vnode
  and wants vfs_busy.

- With these fixes in place change cleanvnode() to use vget()/vrecycle()
  to reclaim the vnode.


To generate a diff of this commit:
cvs rdiff -u -r1.59 -r1.60 src/sys/kern/vfs_vnode.c
cvs rdiff -u -r1.12 -r1.13 src/tests/fs/puffs/t_basic.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

Modified files:

Index: src/sys/kern/vfs_vnode.c
diff -u src/sys/kern/vfs_vnode.c:1.59 src/sys/kern/vfs_vnode.c:1.60
--- src/sys/kern/vfs_vnode.c:1.59	Thu Nov  3 11:04:21 2016
+++ src/sys/kern/vfs_vnode.c	Thu Dec  1 14:49:03 2016
@@ -1,4 +1,4 @@
-/*	$NetBSD: vfs_vnode.c,v 1.59 2016/11/03 11:04:21 hannken Exp $	*/
+/*	$NetBSD: vfs_vnode.c,v 1.60 2016/12/01 14:49:03 hannken Exp $	*/
 
 /*-
  * Copyright (c) 1997-2011 The NetBSD Foundation, Inc.
@@ -156,7 +156,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: vfs_vnode.c,v 1.59 2016/11/03 11:04:21 hannken Exp $");
+__KERNEL_RCSID(0, "$NetBSD: vfs_vnode.c,v 1.60 2016/12/01 14:49:03 hannken Exp $");
 
 #include <sys/param.h>
 #include <sys/kernel.h>
@@ -444,16 +444,11 @@ try_nextlist:
 		KASSERT(vp->v_usecount == 0);
 		KASSERT(vp->v_freelisthd == listhd);
 
-		if (vn_lock(vp, LK_EXCLUSIVE | LK_NOWAIT) != 0)
+		if (!mutex_tryenter(vp->v_interlock))
 			continue;
-		if (!mutex_tryenter(vp->v_interlock)) {
-			VOP_UNLOCK(vp);
-			continue;
-		}
 		mp = vp->v_mount;
 		if (fstrans_start_nowait(mp, FSTRANS_SHARED) != 0) {
 			mutex_exit(vp->v_interlock);
-			VOP_UNLOCK(vp);
 			continue;
 		}
 		break;
@@ -468,21 +463,12 @@ try_nextlist:
 		return EBUSY;
 	}
 
-	/* Remove it from the freelist. */
-	TAILQ_REMOVE(listhd, vp, v_freelist);
-	vp->v_freelisthd = NULL;
 	mutex_exit(&vnode_free_list_lock);
 
-	KASSERT(vp->v_usecount == 0);
-
-	/*
-	 * The vnode is still associated with a file system, so we must
-	 * clean it out before freeing it.  We need to add a reference
-	 * before doing this.
-	 */
-	vp->v_usecount = 1;
-	vcache_reclaim(vp);
-	vrelel(vp, 0);
+	if (vget(vp, 0, true /* wait */) == 0) {
+		if (!vrecycle(vp))
+			vrele(vp);
+	}
 	fstrans_done(mp);
 
 	return 0;
@@ -949,19 +935,37 @@ holdrelel(vnode_t *vp)
 bool
 vrecycle(vnode_t *vp)
 {
-
-	if (vn_lock(vp, LK_EXCLUSIVE) != 0)
-		return false;
+	int error __diagused;
 
 	mutex_enter(vp->v_interlock);
 
+	/* Make sure we hold the last reference. */
+	VSTATE_WAIT_STABLE(vp);
 	if (vp->v_usecount != 1) {
 		mutex_exit(vp->v_interlock);
-		VOP_UNLOCK(vp);
 		return false;
 	}
+
+	/* If the vnode is already clean we're done. */
+	if (VSTATE_GET(vp) != VS_ACTIVE) {
+		VSTATE_ASSERT(vp, VS_RECLAIMED);
+		vrelel(vp, 0);
+		return true;
+	}
+
+	/* Prevent further references until the vnode is locked. */
+	VSTATE_CHANGE(vp, VS_ACTIVE, VS_BLOCKED);
+	mutex_exit(vp->v_interlock);
+
+	error = vn_lock(vp, LK_EXCLUSIVE);
+	KASSERT(error == 0);
+
+	mutex_enter(vp->v_interlock);
+	VSTATE_CHANGE(vp, VS_BLOCKED, VS_ACTIVE);
+
 	vcache_reclaim(vp);
 	vrelel(vp, 0);
+
 	return true;
 }
 
@@ -1488,8 +1492,7 @@ vcache_reclaim(vnode_t *vp)
 	/*
 	 * Clean out any cached data associated with the vnode.
 	 * If purging an active vnode, it must be closed and
-	 * deactivated before being reclaimed. Note that the
-	 * VOP_INACTIVE will unlock the vnode.
+	 * deactivated before being reclaimed.
 	 */
 	error = vinvalbuf(vp, V_SAVE, NOCRED, l, 0, 0);
 	if (error != 0) {
@@ -1502,17 +1505,12 @@ vcache_reclaim(vnode_t *vp)
 	if (active && (vp->v_type == VBLK || vp->v_type == VCHR)) {
 		 spec_node_revoke(vp);
 	}
-	if (active) {
-		VOP_INACTIVE(vp, &recycle);
-	} else {
-		/*
-		 * Any other processes trying to obtain this lock must first
-		 * wait for VS_RECLAIMED, then call the new lock operation.
-		 */
-		VOP_UNLOCK(vp);
-	}
 
-	/* Disassociate the underlying file system from the vnode. */
+	/*
+	 * Disassociate the underlying file system from the vnode.
+	 * Note that the VOP_INACTIVE will unlock the vnode.
+	 */
+	VOP_INACTIVE(vp, &recycle);
 	if (VOP_RECLAIM(vp)) {
 		vnpanic(vp, "%s: cannot reclaim", __func__);
 	}

Index: src/tests/fs/puffs/t_basic.c
diff -u src/tests/fs/puffs/t_basic.c:1.12 src/tests/fs/puffs/t_basic.c:1.13
--- src/tests/fs/puffs/t_basic.c:1.12	Sat Oct 19 17:45:00 2013
+++ src/tests/fs/puffs/t_basic.c	Thu Dec  1 14:49:04 2016
@@ -1,4 +1,4 @@
-/*	$NetBSD: t_basic.c,v 1.12 2013/10/19 17:45:00 christos Exp $	*/
+/*	$NetBSD: t_basic.c,v 1.13 2016/12/01 14:49:04 hannken Exp $	*/
 
 #include <sys/types.h>
 #include <sys/mount.h>
@@ -286,7 +286,7 @@ ATF_TC_BODY(inactive_reclaim, tc)
 	rump_sys_close(fd);
 	syncbar(FSTEST_MNTNAME);
 
-	ATF_REQUIRE_EQ(pargs->pta_vn_toserv_ops[PUFFS_VN_INACTIVE], 1);
+	ATF_REQUIRE(pargs->pta_vn_toserv_ops[PUFFS_VN_INACTIVE] > 0);
 	ATF_REQUIRE_EQ(pargs->pta_vn_toserv_ops[PUFFS_VN_RECLAIM], 1);
 
 	FSTEST_EXIT();
@@ -383,7 +383,7 @@ ATF_TC_BODY(unlink_accessible, tc)
 	syncbar(FSTEST_MNTNAME);
 
 	ATF_REQUIRE_EQ(pargs->pta_vn_toserv_ops[PUFFS_VN_RECLAIM], 1);
-	ATF_REQUIRE_EQ(pargs->pta_vn_toserv_ops[PUFFS_VN_INACTIVE], ianow+1);
+	ATF_REQUIRE(pargs->pta_vn_toserv_ops[PUFFS_VN_INACTIVE] > ianow);
 
 	ATF_REQUIRE_STREQ(buf, MAGICSTR);
 

Reply via email to