Subject: Re: simple_lock: uninitialized lock
To: Juergen Hannken-Illjes <hannken@eis.cs.tu-bs.de>
From: Antti Kantee <pooka@netbsd.org>
List: current-users
Date: 07/24/2007 00:06:44
On Mon Jul 23 2007 at 22:52:06 +0200, Juergen Hannken-Illjes wrote:
> On Mon, Jul 23, 2007 at 10:18:28PM +0300, Antti Kantee wrote:
> > On Mon Jul 23 2007 at 19:33:41 +0200, Juergen Hannken-Illjes wrote:
> [snip]
> > > With rev. 1.289 of vfs_subr.c the lock type in function vclean()
> > > was changed from LK_DRAIN to LK_EXCLUSIVE.  This seems wrong.
> > 
> > Care to explain why you think it's wrong?  I had several theories on
> > why it could be wrong, but none of them held water.
> 
> I always took LK_DRAIN as to leave with an exclusive lock and no more
> waiters.  Using LK_EXCLUSIVE we may still have other threads waiting
> on this lock.  If thread lwp_0 in the example below had used LK_DRAIN
> it had to wait for ioflush to return from its VOP_LOCK instead of allowing
> ioflush to wake up on a now freed lock.

I assumed that locks are granted out in the order they are requested.
If that is not the case, I think the situation you describe is possible
and we should do something akin of LK_DRAIN | LK_REENABLE, or if that's
not possible, LK_DRAIN, LK_REENABLE, LK_EXCLUSIVE.

Actually, if it's easy to repeat, just try this.  It's stupid, yes,
but still non-racy because of VXLOCK and let's try to prove the theory.

Index: vfs_subr.c
===================================================================
RCS file: /cvsroot/src/sys/kern/vfs_subr.c,v
retrieving revision 1.291
diff -p -u -r1.291 vfs_subr.c
--- vfs_subr.c	17 Jul 2007 21:14:05 -0000	1.291
+++ vfs_subr.c	23 Jul 2007 21:04:41 -0000
@@ -1535,7 +1535,9 @@ vclean(struct vnode *vp, int flags, stru
 	 * We don't drain the lock because it might have been exported
 	 * to upper layers by this vnode and could still be in use.
 	 */
-	VOP_LOCK(vp, LK_EXCLUSIVE | LK_INTERLOCK);
+	VOP_LOCK(vp, LK_DRAIN | LK_INTERLOCK);
+	VOP_UNLOCK(vp, LK_REENABLE);
+	VOP_LOCK(vp, LK_EXCLUSIVE);
 
 	/*
 	 * Clean out any cached data associated with the vnode.

-- 
Antti Kantee <pooka@iki.fi>                     Of course he runs NetBSD
http://www.iki.fi/pooka/                          http://www.NetBSD.org/
    "la qualité la plus indispensable du cuisinier est l'exactitude"