Subject: VOP_CLOSE, was Re: Locking errors
To: None <tech-kern@netbsd.org>
From: Dr. Bill Studenmund <wrstuden@loki.stanford.edu>
List: tech-kern
Date: 02/09/1999 16:44:14
Last I wrote, I was having problems with trying to lock a vnode in
VOP_CLOSE, which is supposed to take an unlocked vnode. Jason's suggestion
was to just have VOP_CLOSE take a locked vnode.

Turns out most of the places in the kernel which call VOP_CLOSE pass it a
locked vnode now! nfs_close certainly acts like it has a lock on the
vnode, too.

So here are my mods to change the vnode protocol so that VOP_CLOSE takes a
locked vnode. I've run an i386 with these changes(*), and everything's
fine except for coda. coda's close routine, coda_close, needs some work,
and I'd love to talk to rvb about it (right now coda_close calls vrele,
which is a BIG no-no if we ever want to stack an fs on top of coda). 

Comments? Both Jason & I would love to get these in for 1.4.

Take care,

Bill

(*) My first effort was to rename VOP_CLOSE to VOP_CLOSE1 - it was a fast
way to find all VOP_CLOSE instances & audit them. :-)


>From wrstuden@nas.nasa.gov Tue Feb  9 16:27:19 1999
Date: Tue, 9 Feb 1999 16:27:07 -0800 (PST)
From: Bill Studenmund <wrstuden@nas.nasa.gov>
To: wrstuden@fastloki.stanford.edu

Index: kern/exec_script.c
===================================================================
RCS file: /cvs-src/NetBSD-mss3a/src/sys/kern/exec_script.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 exec_script.c
--- exec_script.c	1998/12/21 18:12:42	1.1.1.1
+++ exec_script.c	1999/02/09 22:57:32
@@ -248,7 +248,9 @@
 		 * list will be used.
 		 */
 		if ((epp->ep_flags & EXEC_HASFD) == 0) {
+			vn_lock(scriptvp, LK_EXCLUSIVE | LK_RETRY);
 			VOP_CLOSE(scriptvp, FREAD, p->p_ucred, p);
+			VOP_UNLOCK(scriptvp, 0);
 			vrele(scriptvp);
 		}
 
@@ -284,7 +286,9 @@
                 epp->ep_flags &= ~EXEC_HASFD;
                 (void) fdrelease(p, epp->ep_fd);
         } else if (scriptvp) {
+		vn_lock(scriptvp, LK_EXCLUSIVE | LK_RETRY);
 		VOP_CLOSE(scriptvp, FREAD, p->p_ucred, p);
+		VOP_UNLOCK(scriptvp, 0);
 		vrele(scriptvp);
 	}
 
Index: kern/kern_exec.c
===================================================================
RCS file: /cvs-src/NetBSD-mss3a/src/sys/kern/kern_exec.c,v
retrieving revision 1.1.1.2
diff -u -r1.1.1.2 kern_exec.c
--- kern_exec.c	1999/01/25 01:05:54	1.1.1.2
+++ kern_exec.c	1999/02/09 22:57:52
@@ -133,7 +133,7 @@
 	if ((error = VOP_OPEN(vp, FREAD, p->p_ucred, p)) != 0)
 		goto bad1;
 
-	/* unlock vp, since we don't need it locked from here on out. */
+	/* unlock vp, since we need it unlocked from here on out. */
 	VOP_UNLOCK(vp, 0);
 
 	/* now we have the file, get the exec header */
@@ -186,7 +186,9 @@
 	 * unlock and close the vnode, restore the old one, free the
 	 * pathname buf, and punt.
 	 */
+	vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
 	VOP_CLOSE(vp, FREAD, p->p_ucred, p);
+	VOP_UNLOCK(vp, 0);
 	vrele(vp);
 	FREE(ndp->ni_cnd.cn_pnbuf, M_NAMEI);
 	return error;
@@ -478,7 +480,9 @@
 #endif
 
 	FREE(nid.ni_cnd.cn_pnbuf, M_NAMEI);
+	vn_lock(pack.ep_vp, LK_EXCLUSIVE | LK_RETRY);
 	VOP_CLOSE(pack.ep_vp, FREAD, cred, p);
+	VOP_UNLOCK(pack.ep_vp, 0);
 	vrele(pack.ep_vp);
 
 	/* setup new registers and do misc. setup. */
@@ -506,7 +510,9 @@
 		(void) fdrelease(p, pack.ep_fd);
 	}
 	/* close and put the exec'd file */
+	vn_lock(pack.ep_vp, LK_EXCLUSIVE | LK_RETRY);
 	VOP_CLOSE(pack.ep_vp, FREAD, cred, p);
+	VOP_UNLOCK(pack.ep_vp, 0);
 	vrele(pack.ep_vp);
 	FREE(nid.ni_cnd.cn_pnbuf, M_NAMEI);
 #if defined(UVM)
@@ -535,7 +541,9 @@
 	if (pack.ep_emul_arg)
 		FREE(pack.ep_emul_arg, M_TEMP);
 	FREE(nid.ni_cnd.cn_pnbuf, M_NAMEI);
+	vn_lock(pack.ep_vp, LK_EXCLUSIVE | LK_RETRY);
 	VOP_CLOSE(pack.ep_vp, FREAD, cred, p);
+	VOP_UNLOCK(pack.ep_vp, 0);
 	vrele(pack.ep_vp);
 #if defined(UVM)
 	uvm_km_free_wakeup(exec_map, (vaddr_t) argp, NCARGS);
Index: kern/vfs_subr.c
===================================================================
RCS file: /cvs-src/NetBSD-mss3a/src/sys/kern/vfs_subr.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 vfs_subr.c
--- vfs_subr.c	1998/12/21 18:12:48	1.1.1.1
+++ vfs_subr.c	1999/02/09 23:00:01
@@ -914,6 +914,8 @@
 /*
  * Vnode release.
  * If count drops to zero, call inactive routine and return to freelist.
+ *
+ * May be passed a locked or unlocked vnode, which will be unlocked on exit.
  */
 void
 vrele(vp)
Index: kern/vfs_vnops.c
===================================================================
RCS file: /cvs-src/NetBSD-mss3a/src/sys/kern/vfs_vnops.c,v
retrieving revision 1.4
diff -u -r1.4 vfs_vnops.c
--- vfs_vnops.c	1999/01/29 05:31:39	1.4
+++ vfs_vnops.c	1999/02/09 22:58:28
@@ -190,6 +190,8 @@
 
 /*
  * Vnode close call
+ *
+ * Note: takes an unlocked vnode, while VOP_CLOSE takes a locked node.
  */
 int
 vn_close(vp, flags, cred, p)
@@ -202,7 +204,9 @@
 
 	if (flags & FWRITE)
 		vp->v_writecount--;
+	vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
 	error = VOP_CLOSE(vp, flags, cred, p);
+	VOP_UNLOCK(vp, 0);
 	vrele(vp);
 	return (error);
 }
Index: kern/vnode_if.src
===================================================================
RCS file: /cvs-src/NetBSD-mss3a/src/sys/kern/vnode_if.src,v
retrieving revision 1.4
diff -u -r1.4 vnode_if.src
--- vnode_if.src	1999/01/29 05:31:40	1.4
+++ vnode_if.src	1999/02/02 21:45:53
@@ -98,7 +98,7 @@
 };
 
 #
-#% close      vp      U U U
+#% close      vp      L L L
 #
 vop_close {
 	IN struct vnode *vp;
Index: miscfs/union/union_subr.c
===================================================================
RCS file: /cvs-src/NetBSD-mss3a/src/sys/miscfs/union/union_subr.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 union_subr.c
--- union_subr.c	1998/12/21 18:13:21	1.1.1.1
+++ union_subr.c	1999/02/10 00:07:02
@@ -677,8 +677,8 @@
 		error = VOP_OPEN(lvp, FREAD, cred, p);
 		if (error == 0) {
 			error = union_copyfile(lvp, uvp, cred, p);
-			VOP_UNLOCK(lvp, 0);
 			(void) VOP_CLOSE(lvp, FREAD, cred, p);
+			VOP_UNLOCK(lvp, 0);
 		}
 #ifdef UNION_DIAGNOSTIC
 		if (error == 0)
@@ -687,8 +687,8 @@
 
 	}
 	un->un_flags &= ~UN_ULOCK;
-	VOP_UNLOCK(uvp, 0);
 	union_vn_close(uvp, FWRITE, cred, p);
+	VOP_UNLOCK(uvp, 0);
 	vn_lock(uvp, LK_EXCLUSIVE | LK_RETRY);
 	un->un_flags |= UN_ULOCK;