Re: kern/41374: getnewvnode deadlock

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

The following reply was made to PR kern/41374; it has been noted by GNATS.

From: yamt%mwd.biglobe.ne.jp@localhost (YAMAMOTO Takashi)
To: ad%netbsd.org@localhost
Cc: gnats-bugs%NetBSD.org@localhost, kern-bug-people%netbsd.org@localhost, 
gnats-admin%netbsd.org@localhost,
        netbsd-bugs%netbsd.org@localhost
Subject: Re: kern/41374: getnewvnode deadlock
Date: Sun, 10 May 2009 09:44:41 +0000 (UTC)

 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 {