If any of you are running NetBSD-current and feeling brave, the patch
below may be of interest.  If you're one of the 5 people in the world
who understands vnode locking protocols, then I'd appreciate review.



From: Greg Troxel <gdt_at_ir.bbn.com>
To: tech-kern_at_netbsd.org
Subject: vnode locking in coda_lookup
Date: Wed, 04 Apr 2007 19:38:14 -0400
Message-ID: <rmiabxnemmx.fsf_at_fnord.ir.bbn.com>
MIME-Version: 1.0

I have attempted to fix vnode locking in coda's lookup routine.
Basically, coda_lookup (in sys/coda/coda_vnops.c) never followed the
special rules for IS_DOTDOT.  I've read the rules, and looked at
ufs_lookup, but I'm not sure this is right.  It does survive 

  for i in `cd /usr/bin && ls`; do cp -pf /usr/bin/$i ../$i & done
and 
  for i in `cd .. && ls`; do cat ../$i  > /dev/null & done

I have also cleaned up and rewritten comments, some of which probably
date from Mach days.

I know that wrstuden@ in the past said that LK_RETRY was not correct.
ufs_lookup does it, and I'm still unclear on this.  I do have a trusty
Thinkpad 600E crashbox for testing.

So two questions:

  is this correct?

  is this clearly better than what was there?



--- coda_vnops.c.~1.52.~	2007-03-12 14:44:43.000000000 -0400
+++ coda_vnops.c	2007-04-04 19:26:49.000000000 -0400
@@ -885,22 +885,20 @@ coda_lookup(void *v)
 {
 /* true args */
     struct vop_lookup_args *ap = v;
+    /* (locked) vnode of dir in which to do lookup */
     struct vnode *dvp = ap->a_dvp;
     struct cnode *dcp = VTOC(dvp);
+    /* output variable for result */
     struct vnode **vpp = ap->a_vpp;
-    /*
-     * It looks as though ap->a_cnp->ni_cnd->cn_nameptr holds the rest
-     * of the string to xlate, and that we must try to get at least
-     * ap->a_cnp->ni_cnd->cn_namelen of those characters to macth.  I
-     * could be wrong.
-     */
-    struct componentname  *cnp = ap->a_cnp;
+    /* name to lookup */
+    struct componentname *cnp = ap->a_cnp;
     kauth_cred_t cred = cnp->cn_cred;
     struct lwp *l = cnp->cn_lwp;
 /* locals */
     struct cnode *cp;
     const char *nm = cnp->cn_nameptr;
     int len = cnp->cn_namelen;
+    int flags = cnp->cn_flags;
     CodaFid VFid;
     int	vtype;
     int error = 0;
@@ -910,6 +908,17 @@ coda_lookup(void *v)
     CODADEBUG(CODA_LOOKUP, myprintf(("lookup: %s in %s\n",
 				   nm, coda_f2s(&dcp->c_fid))););
 
+    /*
+     * XXX componentname flags in MODMASK are not handled at all */
+     */
+
+    /*
+     * The overall strategy is to switch on the lookup type and get a
+     * result vnode that is vref'd but not locked.  Then, the code at
+     * exit: switches on ., .., and regular lookups and does the right
+     * locking.
+     */
+
     /* Check for lookup of control object. */
     if (IS_CTL_NAME(dvp, nm, len)) {
 	*vpp = coda_ctlvp;
@@ -918,6 +927,7 @@ coda_lookup(void *v)
 	goto exit;
     }
 
+    /* Avoid trying to hand venus an unreasonably long name. */
     if (len+1 > CODA_MAXNAMLEN) {
 	MARK_INT_FAIL(CODA_LOOKUP_STATS);
 	CODADEBUG(CODA_LOOKUP, myprintf(("name too long: lookup, %s (%s)\n",
@@ -926,8 +936,13 @@ coda_lookup(void *v)
 	error = EINVAL;
 	goto exit;
     }
-    /* First try to look the file up in the cfs name cache */
-    /* lock the parent vnode? */
+
+    /*
+     * Try to resolve the lookup in the minicache.  If that fails, ask
+     * venus to do the lookup.  XXX The interaction between vnode
+     * locking and coda locking to avoid deadlocks on .. lookups is
+     * not clear.
+     */
     cp = coda_nc_lookup(dcp, nm, len, cred);
     if (cp) {
 	*vpp = CTOV(cp);
@@ -935,8 +950,7 @@ coda_lookup(void *v)
 	CODADEBUG(CODA_LOOKUP,
 		 myprintf(("lookup result %d vpp %p\n",error,*vpp));)
     } else {
-
-	/* The name wasn't cached, so we need to contact Venus */
+	/* The name wasn't cached, so ask Venus. */
 	error = venus_lookup(vtomi(dvp), &dcp->c_fid, nm, len, cred, l, &VFid, &vtype);
 
 	if (error) {
@@ -952,9 +966,13 @@ coda_lookup(void *v)
 
 	    cp = make_coda_node(&VFid, dvp->v_mount, vtype);
 	    *vpp = CTOV(cp);
+	    /* vpp is now vrefed. */
 
-	    /* enter the new vnode in the Name Cache only if the top bit isn't set */
-	    /* And don't enter a new vnode for an invalid one! */
+	    /*
+	     * Unless this vnode is marked CODA_NOCACHE, enter it into
+	     * the coda name cache to avoid a future venus round-trip.
+	     * XXX Interaction with componentname NOCACHE is unclear.
+	     */
 	    if (!(vtype & CODA_NOCACHE))
 		coda_nc_enter(VTOC(dvp), nm, len, cred, VTOC(*vpp));
 	}
@@ -978,6 +996,11 @@ coda_lookup(void *v)
 	cnp->cn_flags |= SAVENAME;
 	*ap->a_vpp = NULL;
     }
+    /*
+     * XXX If creating and we found something, does that need to be an
+     * error?
+     */
+
 
     /*
      * If we are removing, and we are at the last element, and we
@@ -997,17 +1020,25 @@ coda_lookup(void *v)
     }
 
     /*
-     * If the lookup went well, we need to lock the child.
+     * If the lookup succeeded, we must generally lock the returned
+     * vnode.  This could be a ., .., or normal lookup.  See
+     * vnodeops(9) for the details.
      */
     if (!error || (error == EJUSTRETURN)) {
-	/* The parent is locked, and may be the same as the child */
+	/* Lookup has a value and it isn't "."? */
 	if (*ap->a_vpp && (*ap->a_vpp != dvp)) {
-	    /* Different, go ahead and lock it. */
-	    vn_lock(*ap->a_vpp, LK_EXCLUSIVE|LK_RETRY);
+	    if (flags & ISDOTDOT)
+		/* ..: unlock parent */
+		VOP_UNLOCK(dvp, 0);
+	    /* all but .: lock child */
+	    vn_lock(*ap->a_vpp, LK_EXCLUSIVE | LK_RETRY);
+	    if (flags & ISDOTDOT)
+		/* ..: relock parent */
+	        vn_lock(dvp, LK_EXCLUSIVE | LK_RETRY);
 	}
+	/* else .: leave dvp locked */
     } else {
-	/* If the lookup failed, we need to ensure that the leaf is NULL */
-	/* Don't change any locking? */
+	/* The lookup failed, so return NULL.  Leave dvp locked. */
 	*ap->a_vpp = NULL;
     }
     return(error);