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;