Re: CVS commit: src/sys/ufs/ufs

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

On Tue, Sep 22, 2009 at 10:42:59PM +0300, Antti Kantee wrote:
> On Tue Sep 22 2009 at 21:04:14 +0200, Manuel Bouyer wrote:
> > that's not an issue with the reference count. It's an issue with vclean()
> > calling VOP_RECLAIM() even if the refcount is greater than 1, and
> > vrelel() calling vclean() even if the refcount is greater than 1,
> > when the file has been unlink(2)ed. There's a comment about this in
> > vrelel(), look for variable "recycle".
> 
> Ah, ic, probably would've been easier if I read the PR first ;)
> 
> So basically someone can vget the vnode (via fhtovp, since it's gone
> from the directory namespace) between VOP_INACTIVE and clean.
> 
> Your fix doesn't really fix this problem, since depending on timings
> the inode might still be recycled after you check it's "valid".

I don't think so because at this point the vnode is locked. I think the
race window is between vn_lock() releasing the interlock and getting the
vn_lock. After that the reference count is high enouth to prevent
vrelel() trying to clean it (vtryrele() at the start of vrelel will
make it return, and we hold the interlock at this point).


> 
> Hmm, ok, so things which might happen:
> 
> 1: VOP_REMOVE, vnode is locked, vrele is called
> 2: fhtovp before inode is reclaimed, blocks on vn_lock

It would fist block on the interlock at this point, I guess.

> 1: VOP_INACTIVE releases vnlock, returns signal to recycle vnode
> 2: gets lock, does check that it's still the same inode
> 1: reclaims vnode
> 2: boom
> 
> Since vget takes the interlock, a dirty & cheap trick might be to check
> that the reference count is still one before trying to clean the vnode in
> vrelel(), otherwise punting and letting the next release-to-0 reclaim it?

So basically ignoring what VOP_INACTIVE() says. I think this would need
another flag so that new vget() against this vnode can drop it early.
Otherwise we could have a livelock with the NFS server always getting
references to the vnode, preventing it from being recycled and
blocking other threads waiting on the inode to reuse it.

> 
> Blah, I didn't even want to think about this migrane-inducer now.
> Maybe people who have recently worked on vnode reclaiming could instead
> be the ones to comment?

that would be nice :)

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