Re: kern/41374: getnewvnode deadlock

[yamt%mwd.biglobe.ne.jp@localhost (YAMAMOTO Takashi)]

hi,

> On Thu, May 07, 2009 at 04:45:01AM +0000, yamt%mwd.biglobe.ne.jp@localhost 
> wrote:
> 
>>      in the case of getcleanvnode,
>>      set VI_XLOCK before incrementing v_usecount?
> 
> I think insufficient because vtryget() is lockless.

i was thinking something like the following.  (untested)

> How about making XLOCK
> a flag bit in v_useconut?

it sounds like a good idea.

YAMAMOTO Takashi

Index: sys/vnode.h
===================================================================
RCS file: /cvsroot/src/sys/sys/vnode.h,v
retrieving revision 1.191.4.2
diff -u -p -r1.191.4.2 vnode.h
--- sys/vnode.h 4 May 2009 08:14:36 -0000       1.191.4.2
+++ sys/vnode.h 10 May 2009 09:42:33 -0000
@@ -227,6 +227,7 @@ typedef struct vnode vnode_t;
 #define        VI_WRMAP        0x00000400      /* might have PROT_WRITE u. 
mappings */
 #define        VI_WRMAPDIRTY   0x00000800      /* might have dirty pages */
 #define        VI_XLOCK        0x00001000      /* vnode is locked to change 
type */
+#define        VI_CLEANING     0x00002000      /* being cleaned by vclean */
 #define        VI_ONWORKLST    0x00004000      /* On syncer work-list */
 #define        VI_MARKER       0x00008000      /* Dummy marker vnode */
 #define        VI_LAYER        0x00020000      /* vnode is on a layer 
filesystem */
@@ -243,7 +244,7 @@ typedef struct vnode vnode_t;
 
 #define        VNODE_FLAGBITS \
     "\20\1ROOT\2SYSTEM\3ISTTY\4MAPPED\5MPSAFE\6LOCKSWORK\11TEXT\12EXECMAP" \
-    "\13WRMAP\14WRMAPDIRTY\15XLOCK\17ONWORKLST\20MARKER" \
+    "\13WRMAP\14WRMAPDIRTY\15XLOCK\16CLEANING\17ONWORKLST\20MARKER" \
     "\22LAYER\24CLEAN\25INACTPEND\26INACTREDO\27FREEING" \
     "\30INACTNOW\31DIROP" 
 
Index: kern/vfs_subr.c
===================================================================
RCS file: /cvsroot/src/sys/kern/vfs_subr.c,v
retrieving revision 1.336.4.2
diff -u -p -r1.336.4.2 vfs_subr.c
--- kern/vfs_subr.c     4 May 2009 08:13:49 -0000       1.336.4.2
+++ kern/vfs_subr.c     10 May 2009 09:42:34 -0000
@@ -361,6 +361,17 @@ try_nextlist:
         * before doing this.  If the vnode gains another reference while
         * being cleaned out then we lose - retry.
         */
+       KASSERT(mutex_owned(&vp->v_interlock));
+       KASSERT((vp->v_iflag & VI_XLOCK) == 0);
+       KASSERT((vp->v_iflag & VI_CLEAN) == 0);
+       KASSERT(vp->v_usecount == 0);
+       /*
+        * after we increment v_usecount below, another thread can
+        * do vtryget successfully.  set VI_XLOCK before that, so that
+        * the competing thread can notice our activities.
+        */
+       vp->v_iflag |= VI_XLOCK;
+       membar_producer();
        atomic_inc_uint(&vp->v_usecount);
        vclean(vp, DOCLOSE);
        if (vp->v_usecount == 1) {
@@ -1783,8 +1794,8 @@ vclean(vnode_t *vp, int flags)
        KASSERT(vp->v_usecount != 0);
 
        /* If cleaning is already in progress wait until done and return. */
-       if (vp->v_iflag & VI_XLOCK) {
-               vwait(vp, VI_XLOCK);
+       if (vp->v_iflag & VI_CLEANING) {
+               vwait(vp, VI_CLEANING);
                return;
        }
 
@@ -1797,7 +1808,7 @@ vclean(vnode_t *vp, int flags)
         * Prevent the vnode from being recycled or brought into use
         * while we clean it out.
         */
-       vp->v_iflag |= VI_XLOCK;
+       vp->v_iflag |= VI_XLOCK | VI_CLEANING;
        if (vp->v_iflag & VI_EXECMAP) {
                atomic_add_int(&uvmexp.execpages, -vp->v_uobj.uo_npages);
                atomic_add_int(&uvmexp.filepages, vp->v_uobj.uo_npages);
@@ -1857,7 +1868,7 @@ vclean(vnode_t *vp, int flags)
        vp->v_tag = VT_NON;
        vp->v_vnlock = &vp->v_lock;
        KNOTE(&vp->v_klist, NOTE_REVOKE);
-       vp->v_iflag &= ~(VI_XLOCK | VI_FREEING);
+       vp->v_iflag &= ~(VI_XLOCK | VI_FREEING | VI_CLEANING);
        vp->v_vflag &= ~VV_LOCKSWORK;
        if ((flags & DOCLOSE) != 0) {
                vp->v_iflag |= VI_CLEAN;
Index: kern/vfs_cache.c
===================================================================
RCS file: /cvsroot/src/sys/kern/vfs_cache.c,v
retrieving revision 1.74.4.2
diff -u -p -r1.74.4.2 vfs_cache.c
--- kern/vfs_cache.c    4 May 2009 08:13:49 -0000       1.74.4.2
+++ kern/vfs_cache.c    10 May 2009 09:42:34 -0000
@@ -464,6 +464,15 @@ cache_lookup_raw(struct vnode *dvp, stru
                return ENOENT;
        }
        if (vtryget(vp)) {
+               membar_consumer();
+               if ((vp->v_iflag & VI_XLOCK) != 0) {
+                       mutex_exit(&ncp->nc_lock);
+                       mutex_exit(&cpup->cpu_lock);
+                       vrele(vp);
+                       COUNT(cpup->cpu_stats, ncs_falsehits);
+                       *vpp = NULL;
+                       return -1;
+               }
                mutex_exit(&ncp->nc_lock);
                mutex_exit(&cpup->cpu_lock);
        } else {