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

[Antti Kantee <pooka%cs.hut.fi@localhost>]

On Tue Sep 22 2009 at 20:34:50 +0200, Manuel Bouyer wrote:
> > > It depends on what you mean with "race-free". If you mean that the
> > > vnode returned by vget() can't be recygled, I think this is true.
> > > If you mean that vget() can't return a clean vnode then this is false:
> > > vget() can sleep in vn_lock(), and it releases the v_interlock mutex 
> > > before
> > > sleeping. While sleeping vclean() can VOP_RECLAIM() the vnode, even
> > > if v_usecount is > 1.
> > 
> > What is the practical difference of "cleaned" and "recycled" for the
> > file system driver?
> 
> >From what I understand the vnode is not on the free list yet so it can't
> be reused for something else.

For which one? ;)

No, when the reference count (and hold count) goes to zero, the vnode
goes onto the freelist.  However, before use, it must be cleaned.

> > If there is a race in vfs and XLOCK is not used properly, I think that
> > should be investigated and fixed instead of patching file systems here
> > and there.
> 
> I don't know how XLOCK is supposed to be used (and I even less know how
> to change things without creating deadlocks). As I see it XLOCK is set
> while the vnode is being cleaned, not before or after cleaning it, so
> XLOCK is not going to avoid this race anyway.

XLOCK is supposed to be set when the vnode is being cleaned.  vget()
can then detect this early on, wait for the vnode to actually be cleaned
(i.e. VOP_RECLAIM() has done its thing so a new vnode can be crafted
without problems) and return an error.

I am *guessing* that the selection for a vnode cleanup is racy and
the vnode is selected before the reference count is checked (i.e.
the situation before vget() upped the refcount for the duration of
its operation).  But I might be completely wrong too, as that doesn't
really explain what the relationship with fhtovnode is.  Maybe it just
more readily triggers the race?

However, I'm still very far from convinced the current fix is appropriate.
You should at least file a big allcaps PR about the issue.