From: Martin Pieuchot <mpi@grenadille.net>
Subject: Re: Simply uvn_attach() & kill UVM_VNODE_ALOCK
To: tech@openbsd.org
Date: Mon, 10 Nov 2025 10:57:48 +0000

On 09/11/25(Sun) 17:37, Martin Pieuchot wrote:
> By querying the size of the vnode before grabbing `vmobjlock'
> uvn_attach() can be greatly simplified.  Since the lock is no
> longer released, there is no need for UVM_VNODE_ALOCK.

As discussed in the hackroom, here's a simpler diff to review that do
not include the removal of UVM_VNODE_ALOCK. 

ok?

Index: uvm/uvm_vnode.c
===================================================================
RCS file: /cvs/src/sys/uvm/uvm_vnode.c,v
diff -u -p -r1.146 uvm_vnode.c
--- uvm/uvm_vnode.c	10 Nov 2025 10:53:53 -0000	1.146
+++ uvm/uvm_vnode.c	10 Nov 2025 10:55:59 -0000
@@ -146,6 +146,40 @@ uvn_attach(struct vnode *vp, vm_prot_t a
 		return NULL;
 	}
 
+	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 = (*bdevsw[major(vp->v_rdev)].d_ioctl)(vp->v_rdev,
+		    DIOCGPART, (caddr_t)&pi, FREAD, curproc);
+		if (result == 0) {
+			/* XXX should remember blocksize */
+			used_vnode_size = (u_quad_t)pi.disklab->d_secsize *
+			    (u_quad_t)DL_GETPSIZE(pi.part);
+		}
+	} else {
+		result = VOP_GETATTR(vp, &vattr, curproc->p_ucred, curproc);
+		if (result == 0)
+			used_vnode_size = vattr.va_size;
+	}
+
+	if (result != 0)
+		return NULL;
+
+	/*
+	 * make sure that the newsize fits within a vaddr_t
+	 * XXX: need to revise addressing data types
+	 */
+#ifdef DEBUG
+	if (vp->v_type == VBLK)
+		printf("used_vnode_size = %llu\n", (long long)used_vnode_size);
+#endif
+
 	if (vp->v_uvm == NULL) {
 		uvn = pool_get(&uvm_vnode_pool, PR_WAITOK | PR_ZERO);
 		KERNEL_ASSERT_LOCKED();
@@ -191,57 +225,6 @@ uvn_attach(struct vnode *vp, vm_prot_t a
 		return (&uvn->u_obj);
 	}
 
-	/*
-	 * need to call VOP_GETATTR() to get the attributes, but that could
-	 * block (due to I/O), so we want to unlock the object before calling.
-	 * however, we want to keep anyone else from playing with the object
-	 * while it is unlocked.   to do this we set UVM_VNODE_ALOCK which
-	 * prevents anyone from attaching to the vnode until we are done with
-	 * it.
-	 */
-	uvn->u_flags = UVM_VNODE_ALOCK;
-	rw_exit(uvn->u_obj.vmobjlock);
-
-	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 = (*bdevsw[major(vp->v_rdev)].d_ioctl)(vp->v_rdev,
-		    DIOCGPART, (caddr_t)&pi, FREAD, curproc);
-		if (result == 0) {
-			/* XXX should remember blocksize */
-			used_vnode_size = (u_quad_t)pi.disklab->d_secsize *
-			    (u_quad_t)DL_GETPSIZE(pi.part);
-		}
-	} else {
-		result = VOP_GETATTR(vp, &vattr, curproc->p_ucred, curproc);
-		if (result == 0)
-			used_vnode_size = vattr.va_size;
-	}
-
-	if (result != 0) {
-		rw_enter(uvn->u_obj.vmobjlock, RW_WRITE);
-		if (uvn->u_flags & UVM_VNODE_WANTED)
-			wakeup(uvn);
-		uvn->u_flags = 0;
-		rw_exit(uvn->u_obj.vmobjlock);
-		return NULL;
-	}
-
-	/*
-	 * make sure that the newsize fits within a vaddr_t
-	 * XXX: need to revise addressing data types
-	 */
-#ifdef DEBUG
-	if (vp->v_type == VBLK)
-		printf("used_vnode_size = %llu\n", (long long)used_vnode_size);
-#endif
-
 	/* now set up the uvn. */
 	KASSERT(uvn->u_obj.uo_refs == 0);
 	uvn->u_obj.uo_refs++;
@@ -266,6 +249,7 @@ uvn_attach(struct vnode *vp, vm_prot_t a
 
 	if (oldflags & UVM_VNODE_WANTED)
 		wakeup(uvn);
+	rw_exit(uvn->u_obj.vmobjlock);
 
 	return &uvn->u_obj;
 }