Module Name: src
Committed By: yamt
Date: Sat May 16 08:29:54 UTC 2009
Modified Files:
src/sys/kern: vfs_subr.c
src/sys/sys: vnode.h
Log Message:
put a flag bit into v_usecount to prevent vtryget during getcleanvnode.
this fixes the following deadlock.
a thread doing getcleanvnode:
pick a vnode
acqure v_interlock
v_usecount++
call vclean
now, another thread doing cache_lookup:
picks the vnode
vtryget succeed
vn_lock succeed
now in vclean:
set VI_XLOCK (too late to be noticed by the competing thread)
wait on the vnode lock (this might violate locking order)
the use of a flag bit was suggested by Andrew Doran. PR/41374.
To generate a diff of this commit:
cvs rdiff -u -r1.378 -r1.379 src/sys/kern/vfs_subr.c
cvs rdiff -u -r1.206 -r1.207 src/sys/sys/vnode.h
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_subr.c
diff -u src/sys/kern/vfs_subr.c:1.378 src/sys/kern/vfs_subr.c:1.379
--- src/sys/kern/vfs_subr.c:1.378 Sun May 3 16:52:54 2009
+++ src/sys/kern/vfs_subr.c Sat May 16 08:29:53 2009
@@ -1,4 +1,4 @@
-/* $NetBSD: vfs_subr.c,v 1.378 2009/05/03 16:52:54 pooka Exp $ */
+/* $NetBSD: vfs_subr.c,v 1.379 2009/05/16 08:29:53 yamt Exp $ */
/*-
* Copyright (c) 1997, 1998, 2004, 2005, 2007, 2008 The NetBSD Foundation, Inc.
@@ -76,12 +76,22 @@
* change from a non-zero value to zero, again the interlock must be
* held.
*
- * Changing the usecount from a non-zero value to a non-zero value can
- * safely be done using atomic operations, without the interlock held.
+ * There's a flag bit, VC_XLOCK, embedded in v_usecount.
+ * To raise v_usecount, if the VC_XLOCK bit is set in it, the interlock
+ * must be held.
+ * To modify the VC_XLOCK bit, the interlock must be held.
+ * We always keep the usecount (v_usecount & VC_MASK) non-zero while the
+ * VC_XLOCK bit is set.
+ *
+ * Unless the VC_XLOCK bit is set, changing the usecount from a non-zero
+ * value to a non-zero value can safely be done using atomic operations,
+ * without the interlock held.
+ * Even if the VC_XLOCK bit is set, decreasing the usecount to a non-zero
+ * value can be done using atomic operations, without the interlock held.
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: vfs_subr.c,v 1.378 2009/05/03 16:52:54 pooka Exp $");
+__KERNEL_RCSID(0, "$NetBSD: vfs_subr.c,v 1.379 2009/05/16 08:29:53 yamt Exp $");
#include "opt_ddb.h"
#include "opt_compat_netbsd.h"
@@ -361,8 +371,10 @@
* before doing this. If the vnode gains another reference while
* being cleaned out then we lose - retry.
*/
- atomic_inc_uint(&vp->v_usecount);
+ atomic_add_int(&vp->v_usecount, 1 + VC_XLOCK);
vclean(vp, DOCLOSE);
+ KASSERT(vp->v_usecount >= 1 + VC_XLOCK);
+ atomic_add_int(&vp->v_usecount, -VC_XLOCK);
if (vp->v_usecount == 1) {
/* We're about to dirty it. */
vp->v_iflag &= ~VI_CLEAN;
@@ -1229,7 +1241,7 @@
return false;
}
for (use = vp->v_usecount;; use = next) {
- if (use == 0) {
+ if (use == 0 || __predict_false((use & VC_XLOCK) != 0)) {
/* Need interlock held if first reference. */
return false;
}
@@ -1318,9 +1330,10 @@
u_int use, next;
for (use = vp->v_usecount;; use = next) {
- if (use == 1) {
+ if (use == 1) {
return false;
}
+ KASSERT((use & VC_MASK) > 1);
next = atomic_cas_uint(&vp->v_usecount, use, use - 1);
if (__predict_true(next == use)) {
return true;
Index: src/sys/sys/vnode.h
diff -u src/sys/sys/vnode.h:1.206 src/sys/sys/vnode.h:1.207
--- src/sys/sys/vnode.h:1.206 Sun May 3 16:52:55 2009
+++ src/sys/sys/vnode.h Sat May 16 08:29:53 2009
@@ -1,4 +1,4 @@
-/* $NetBSD: vnode.h,v 1.206 2009/05/03 16:52:55 pooka Exp $ */
+/* $NetBSD: vnode.h,v 1.207 2009/05/16 08:29:53 yamt Exp $ */
/*-
* Copyright (c) 2008 The NetBSD Foundation, Inc.
@@ -250,6 +250,12 @@
#define VSIZENOTSET ((voff_t)-1)
/*
+ * v_usecount; see the comment in vfs_subr.c
+ */
+#define VC_XLOCK 0x80000000
+#define VC_MASK 0x7fffffff
+
+/*
* Vnode attributes. A field value of VNOVAL represents a field whose value
* is unavailable (getattr) or which is not to be changed (setattr).
*/
