Re: CVS commit: src/sys/kern

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Fri, Jan 15, 2010 at 01:09:48AM +0200, Antti Kantee wrote:
> On Thu Jan 14 2010 at 22:41:53 +0000, Manuel Bouyer wrote:
> > Module Name:        src
> > Committed By:       bouyer
> > Date:               Thu Jan 14 22:41:53 UTC 2010
> > 
> > Modified Files:
> >     src/sys/kern: vfs_subr.c
> > 
> > Log Message:
> > Remove KASSERT(vp->v_usecount == 1) in getnewvnode() and ungetnewvnode().
> > Another process could be vget()ing the vnode and bump v_usecount while
> > getcleanvnode() is vclean()ing it (as vclean drops the interlock).
> > vget() will then wait for VI_XLOCK or VI_FREEING to clear; and we could test
> > this assertion while the other process is still slepping. We could even
> > end up in ungetnewvnode() before this other process got a chance to run.
> 
> Why doesn't the v_usecount == 1 check in getcleanvnode() work?

You're right; it's not while the first thread is in vclean() that the
second can grab a reference which will cause this issue.

Then it's probably when getcleanvnode() drops the interlock after
vclean(). This means something can add a reference to a clean vnode,
this is bad. The KASSERT() is probably right.

ufs_ihashget() looks safe for this, it drops the ufs_ihash_lock after
getting the interlock.
cache_lookup() also grabs the interlock before releasing ncp->nc_lock,
so it should be OK.
Any other place where a vnode could be cached without holding a
reference count ?

There's also UVM which could be the cause of this as v_usecount is the
uobj's reference count. But this is another can of worm, and I don't know
how to check this.

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--