Subject: getting rid of uvn_attach()
To: None <tech-kern@netbsd.org>
From: Antti Kantee <pooka@cs.hut.fi>
List: tech-kern
Date: 07/17/2007 15:55:04
--J/dobhs11T7y2rNN
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

I was discussing the use of VXLOCK in uvn_attach() with Chuck a while
ago, and we agreed that it should go away.  Chuck suggested that the
whole of uvn_attach() could go away, since all it really does is set
the vnode's size.

The following patch does away with uvn_attach().  Regular files have
their size already set by file systems and the patch adds spec_mmap()
for setting the size of device special files.

The patch also adds a call to VOP_MMAP() along the exec path, since
an implicit mmap is happening there.  I've been running it for quite a
while now.

comments?

-- 
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"

--J/dobhs11T7y2rNN
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="uvn_attach_begone.diff"

Index: kern/exec_subr.c
===================================================================
RCS file: /cvsroot/src/sys/kern/exec_subr.c,v
retrieving revision 1.52
diff -p -u -r1.52 exec_subr.c
--- kern/exec_subr.c	4 Mar 2007 06:03:03 -0000	1.52
+++ kern/exec_subr.c	17 Jul 2007 12:45:18 -0000
@@ -176,13 +176,12 @@ vmcmd_map_pagedvn(struct lwp *l, struct 
 		return(EINVAL);
 
 	/*
-	 * first, attach to the object
+	 * check the file system's opinion about mmapping the file
 	 */
 
-        uobj = uvn_attach(vp, VM_PROT_READ|VM_PROT_EXECUTE);
-        if (uobj == NULL)
-                return(ENOMEM);
-	VREF(vp);
+	error = VOP_MMAP(vp, 0, p->p_cred, l);
+	if (error)
+		return error;
 
 	if ((vp->v_flag & VMAPPED) == 0) {
 		vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);
@@ -199,8 +198,10 @@ vmcmd_map_pagedvn(struct lwp *l, struct 
 #endif /* PAX_MPROTECT */
 
 	/*
-	 * do the map
+	 * do the map, reference the object for this map entry
 	 */
+	uobj = &vp->v_uobj;
+	vref(vp);
 
 	error = uvm_map(&p->p_vmspace->vm_map, &cmd->ev_addr, cmd->ev_len,
 		uobj, cmd->ev_offset, 0,
Index: kern/kern_exec.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_exec.c,v
retrieving revision 1.245
diff -p -u -r1.245 kern_exec.c
--- kern/kern_exec.c	17 May 2007 14:51:38 -0000	1.245
+++ kern/kern_exec.c	17 Jul 2007 12:45:19 -0000
@@ -290,7 +290,6 @@ check_exec(struct lwp *l, struct exec_pa
 #endif /* PAX_SEGVGUARD */
 
 	/* now we have the file, get the exec header */
-	uvn_attach(vp, VM_PROT_READ);
 	error = vn_rdwr(UIO_READ, vp, epp->ep_hdr, epp->ep_hdrlen, 0,
 			UIO_SYSSPACE, 0, l->l_cred, &resid, NULL);
 	if (error)
Index: kern/vfs_syscalls.c
===================================================================
RCS file: /cvsroot/src/sys/kern/vfs_syscalls.c,v
retrieving revision 1.321
diff -p -u -r1.321 vfs_syscalls.c
--- kern/vfs_syscalls.c	14 Jul 2007 15:41:31 -0000	1.321
+++ kern/vfs_syscalls.c	17 Jul 2007 12:45:20 -0000
@@ -1616,11 +1616,6 @@ dofhopen(struct lwp *l, const void *ufhp
 	}
 	if ((error = VOP_OPEN(vp, flags, cred, l)) != 0)
 		goto bad;
-	if (vp->v_type == VREG &&
-	    uvn_attach(vp, flags & FWRITE ? VM_PROT_WRITE : 0) == NULL) {
-		error = EIO;
-		goto bad;
-	}
 	if (flags & FWRITE)
 		vp->v_writecount++;
 
Index: kern/vfs_vnops.c
===================================================================
RCS file: /cvsroot/src/sys/kern/vfs_vnops.c,v
retrieving revision 1.139
diff -p -u -r1.139 vfs_vnops.c
--- kern/vfs_vnops.c	19 May 2007 22:11:24 -0000	1.139
+++ kern/vfs_vnops.c	17 Jul 2007 12:45:21 -0000
@@ -202,11 +202,6 @@ vn_open(struct nameidata *ndp, int fmode
 	}
 	if ((error = VOP_OPEN(vp, fmode, cred, l)) != 0)
 		goto bad;
-	if (vp->v_type == VREG &&
-	    uvn_attach(vp, fmode & FWRITE ? VM_PROT_WRITE : 0) == NULL) {
-		error = EIO;
-		goto bad;
-	}
 	if (fmode & FWRITE)
 		vp->v_writecount++;
 
Index: miscfs/specfs/spec_vnops.c
===================================================================
RCS file: /cvsroot/src/sys/miscfs/specfs/spec_vnops.c,v
retrieving revision 1.100
diff -p -u -r1.100 spec_vnops.c
--- miscfs/specfs/spec_vnops.c	9 Jul 2007 21:10:59 -0000	1.100
+++ miscfs/specfs/spec_vnops.c	17 Jul 2007 12:45:21 -0000
@@ -520,6 +520,28 @@ spec_kqfilter(v)
 }
 
 /*
+ * Allow mapping of only D_DISK.  This is called only for VBLK.
+ */
+int
+spec_mmap(v)
+	void *v;
+{
+	struct vop_mmap_args /* {
+		struct vnode *a_vp;
+		int a_fflags;
+		kauth_cred_t a_cred;
+		struct lwp *a_l;
+	} */ *ap = v;
+	struct vnode *vp = ap->a_vp;
+
+	KASSERT(vp->v_type == VBLK);
+	if (bdev_type(vp->v_rdev) != D_DISK)
+		return EINVAL;
+
+	return 0;
+}
+
+/*
  * Synch buffers associated with a block device
  */
 /* ARGSUSED */
Index: miscfs/specfs/specdev.h
===================================================================
RCS file: /cvsroot/src/sys/miscfs/specfs/specdev.h,v
retrieving revision 1.30
diff -p -u -r1.30 specdev.h
--- miscfs/specfs/specdev.h	14 May 2006 21:32:21 -0000	1.30
+++ miscfs/specfs/specdev.h	17 Jul 2007 12:45:21 -0000
@@ -118,7 +118,7 @@ int	spec_ioctl(void *);
 int	spec_poll(void *);
 int	spec_kqfilter(void *);
 #define spec_revoke	genfs_revoke
-#define	spec_mmap	genfs_mmap
+int	spec_mmap(void *);
 int	spec_fsync(void *);
 #define	spec_seek	genfs_nullop		/* XXX should query device */
 #define	spec_remove	genfs_badop
Index: uvm/uvm_extern.h
===================================================================
RCS file: /cvsroot/src/sys/uvm/uvm_extern.h,v
retrieving revision 1.131
diff -p -u -r1.131 uvm_extern.h
--- uvm/uvm_extern.h	9 Jul 2007 21:11:36 -0000	1.131
+++ uvm/uvm_extern.h	17 Jul 2007 12:45:22 -0000
@@ -714,7 +714,6 @@ void			uvm_deallocate(struct vm_map *, v
 void			uvm_vnp_setsize(struct vnode *, voff_t);
 void			uvm_vnp_setwritesize(struct vnode *, voff_t);
 void			uvm_vnp_sync(struct mount *);
-struct uvm_object	*uvn_attach(void *, vm_prot_t);
 int			uvn_findpages(struct uvm_object *, voff_t,
 			    int *, struct vm_page **, int);
 void			uvm_vnp_zerorange(struct vnode *, off_t, size_t);
Index: uvm/uvm_mmap.c
===================================================================
RCS file: /cvsroot/src/sys/uvm/uvm_mmap.c,v
retrieving revision 1.112
diff -p -u -r1.112 uvm_mmap.c
--- uvm/uvm_mmap.c	15 May 2007 19:47:46 -0000	1.112
+++ uvm/uvm_mmap.c	17 Jul 2007 12:45:22 -0000
@@ -1117,12 +1117,8 @@ uvm_mmap(map, addr, size, prot, maxprot,
 			if (error) {
 				return error;
 			}
-
-			uobj = uvn_attach((void *)vp, (flags & MAP_SHARED) ?
-			   maxprot : (maxprot & ~VM_PROT_WRITE));
-
-			/* XXX for now, attach doesn't gain a ref */
-			VREF(vp);
+			vref(vp);
+			uobj = &vp->v_uobj;
 
 			/*
 			 * If the vnode is being mapped with PROT_EXEC,
Index: uvm/uvm_vnode.c
===================================================================
RCS file: /cvsroot/src/sys/uvm/uvm_vnode.c,v
retrieving revision 1.83
diff -p -u -r1.83 uvm_vnode.c
--- uvm/uvm_vnode.c	9 Jul 2007 21:11:37 -0000	1.83
+++ uvm/uvm_vnode.c	17 Jul 2007 12:45:22 -0000
@@ -105,118 +105,6 @@ struct uvm_pagerops uvm_vnodeops = {
  */
 
 /*
- * uvn_attach
- *
- * attach a vnode structure to a VM object.  if the vnode is already
- * attached, then just bump the reference count by one and return the
- * VM object.   if not already attached, attach and return the new VM obj.
- * the "accessprot" tells the max access the attaching thread wants to
- * our pages.
- *
- * => caller must _not_ already be holding the lock on the uvm_object.
- * => in fact, nothing should be locked so that we can sleep here.
- * => note that uvm_object is first thing in vnode structure, so their
- *    pointers are equiv.
- */
-
-struct uvm_object *
-uvn_attach(void *arg, vm_prot_t accessprot)
-{
-	struct vnode *vp = arg;
-	struct uvm_object *uobj = &vp->v_uobj;
-	struct vattr vattr;
-	int result;
-	struct partinfo pi;
-	voff_t used_vnode_size;
-	UVMHIST_FUNC("uvn_attach"); UVMHIST_CALLED(maphist);
-
-	UVMHIST_LOG(maphist, "(vn=0x%x)", arg,0,0,0);
-	used_vnode_size = (voff_t)0;
-
-	/*
-	 * first get a lock on the uobj.
-	 */
-
-	simple_lock(&uobj->vmobjlock);
-	while (vp->v_flag & VXLOCK) {
-		vp->v_flag |= VXWANT;
-		UVMHIST_LOG(maphist, "  SLEEPING on blocked vn",0,0,0,0);
-		UVM_UNLOCK_AND_WAIT(uobj, &uobj->vmobjlock, false,
-		    "uvn_attach", 0);
-		simple_lock(&uobj->vmobjlock);
-		UVMHIST_LOG(maphist,"  WOKE UP",0,0,0,0);
-	}
-
-	/*
-	 * if we're mapping a BLK device, make sure it is a disk.
-	 */
-	if (vp->v_type == VBLK) {
-		if (bdev_type(vp->v_rdev) != D_DISK) {
-			simple_unlock(&uobj->vmobjlock);
-			UVMHIST_LOG(maphist,"<- done (VBLK not D_DISK!)",
-				    0,0,0,0);
-			return(NULL);
-		}
-	}
-	KASSERT(vp->v_type == VREG || vp->v_type == VBLK);
-
-	/*
-	 * set up our idea of the size
-	 * if this hasn't been done already.
-	 */
-	if (vp->v_size == VSIZENOTSET) {
-
-
-	vp->v_flag |= VXLOCK;
-	simple_unlock(&uobj->vmobjlock); /* drop lock in case we sleep */
-		/* XXX: curproc? */
-	if (vp->v_type == VBLK) {
-		/*
-		 * We could implement this as a specfs getattr call, but:
-		 *
-		 *	(1) VOP_GETATTR() would get the file system
-		 *	    vnode operation, not the specfs operation.
-		 *
-		 *	(2) All we want is the size, anyhow.
-		 */
-		result = bdev_ioctl(vp->v_rdev, DIOCGPART, (void *)&pi,
-		    FREAD, curlwp);
-		if (result == 0) {
-			/* XXX should remember blocksize */
-			used_vnode_size = (voff_t)pi.disklab->d_secsize *
-			    (voff_t)pi.part->p_size;
-		}
-	} else {
-		result = VOP_GETATTR(vp, &vattr, curlwp->l_cred, curlwp);
-		if (result == 0)
-			used_vnode_size = vattr.va_size;
-	}
-
-	/* relock object */
-	simple_lock(&uobj->vmobjlock);
-
-	if (vp->v_flag & VXWANT) {
-		wakeup(vp);
-	}
-	vp->v_flag &= ~(VXLOCK|VXWANT);
-
-	if (result != 0) {
-		simple_unlock(&uobj->vmobjlock);
-		UVMHIST_LOG(maphist,"<- done (VOP_GETATTR FAILED!)", 0,0,0,0);
-		return(NULL);
-	}
-	vp->v_size = vp->v_writesize = used_vnode_size;
-
-	}
-
-	simple_unlock(&uobj->vmobjlock);
-	UVMHIST_LOG(maphist,"<- done, refcnt=%d", vp->v_usecount,
-	    0, 0, 0);
-	return uobj;
-}
-
-
-/*
  * uvn_reference
  *
  * duplicate a reference to a VM object.  Note that the reference

--J/dobhs11T7y2rNN--