Re: kern/41374: getnewvnode deadlock

[Andrew Doran <ad%netbsd.org@localhost>]

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

From: Andrew Doran <ad%netbsd.org@localhost>
To: YAMAMOTO Takashi <yamt%mwd.biglobe.ne.jp@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: Mon, 11 May 2009 21:06:03 +0000

 On Sun, May 10, 2009 at 09:44:41AM +0000, YAMAMOTO Takashi wrote:
 
 > > How about making XLOCK
 > > a flag bit in v_useconut?
 > 
 > it sounds like a good idea.
 
 On second thought what you propose looks fine to me. Putting a flag bit into
 v_usecount would have the advantage that no memory barrier would be required
 in vtryget().
 
 > @@ -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;
 >      }
 
 Can't this allow a concurrent vclean(), e.g. via revoke or forced unmount?
 
 > @@ -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);
 
 I spent a while today trying to remember why vget() does not do vtryget().
 Of course it's due to VI_FREEING. I wonder if we could change the order of
 events in ufs_inactive() to this, allowing us to remove VI_FREEING (I don't
 know what ZFS will require):
 
        UFS_UPDATE(vp, ...);
        ufs_ihashrem(ip);
        UFS_VFREE(vp, ...);
 
 I'm not yet sure if fsck can handle it the other way around.