From: Martin Pieuchot <mpi@grenadille.net>
Subject: Simply uvn_attach() & kill UVM_VNODE_ALOCK
To: tech@openbsd.org
Date: Sun, 9 Nov 2025 17:37:41 +0000

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.

Diff below does that, ok?

Index: uvm/uvm_vnode.c
===================================================================
RCS file: /cvs/src/sys/uvm/uvm_vnode.c,v
diff -u -p -r1.143 uvm_vnode.c
--- uvm/uvm_vnode.c	8 Nov 2025 17:23:22 -0000	1.143
+++ uvm/uvm_vnode.c	9 Nov 2025 17:29:22 -0000
@@ -137,7 +137,7 @@ uvn_attach(struct vnode *vp, vm_prot_t a
 {
 	struct uvm_vnode *uvn;
 	struct vattr vattr;
-	int oldflags, result;
+	int error;
 	struct partinfo pi;
 	u_quad_t used_vnode_size = 0;
 
@@ -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.
+		 */
+		error = (*bdevsw[major(vp->v_rdev)].d_ioctl)(vp->v_rdev,
+		    DIOCGPART, (caddr_t)&pi, FREAD, curproc);
+		if (error == 0) {
+			/* XXX should remember blocksize */
+			used_vnode_size = (u_quad_t)pi.disklab->d_secsize *
+			    (u_quad_t)DL_GETPSIZE(pi.part);
+		}
+	} else {
+		error = VOP_GETATTR(vp, &vattr, curproc->p_ucred, curproc);
+		if (error == 0)
+			used_vnode_size = vattr.va_size;
+	}
+
+	if (error != 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();
@@ -168,8 +202,7 @@ uvn_attach(struct vnode *vp, vm_prot_t a
 	/*
 	 * now uvn must not be in a blocked state.
 	 * first check to see if it is already active, in which case
-	 * we can bump the reference count, check to see if we need to
-	 * add it to the writeable list, and then return.
+	 * we can bump the reference count.
 	 */
 	if (uvn->u_flags & UVM_VNODE_VALID) {	/* already active? */
 
@@ -178,94 +211,33 @@ uvn_attach(struct vnode *vp, vm_prot_t a
 			vref(vp);
 		}
 		uvn->u_obj.uo_refs++;		/* bump uvn ref! */
+	} else {
+		/* now set up the uvn. */
+		KASSERT(uvn->u_obj.uo_refs == 0);
+		uvn->u_obj.uo_refs++;
+		uvn->u_flags = UVM_VNODE_VALID|UVM_VNODE_CANPERSIST;
+		uvn->u_nio = 0;
+		uvn->u_size = used_vnode_size;
 
-		/* check for new writeable uvn */
-		if ((accessprot & PROT_WRITE) != 0 &&
-		    (uvn->u_flags & UVM_VNODE_WRITEABLE) == 0) {
-			uvn->u_flags |= UVM_VNODE_WRITEABLE;
-			KERNEL_ASSERT_LOCKED();
-			LIST_INSERT_HEAD(&uvn_wlist, uvn, u_wlist);
-		}
-
-		rw_exit(uvn->u_obj.vmobjlock);
-		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.
+		 * this reference will stay as long as there is a valid
+		 * mapping of the vnode.   dropped when the reference count
+		 * goes to zero [and we either free or persist].
 		 */
-		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;
+		vref(vp);
 	}
 
-	/*
-	 * 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++;
-	oldflags = uvn->u_flags;
-	uvn->u_flags = UVM_VNODE_VALID|UVM_VNODE_CANPERSIST;
-	uvn->u_nio = 0;
-	uvn->u_size = used_vnode_size;
-
-	/*
-	 * add a reference to the vnode.   this reference will stay as long
-	 * as there is a valid mapping of the vnode.   dropped when the
-	 * reference count goes to zero [and we either free or persist].
-	 */
-	vref(vp);
-
 	/* if write access, we need to add it to the wlist */
-	if (accessprot & PROT_WRITE) {
+	if ((accessprot & PROT_WRITE) &&
+	    !(uvn->u_flags & UVM_VNODE_WRITEABLE)) {
 		uvn->u_flags |= UVM_VNODE_WRITEABLE;	/* we are on wlist! */
 		KERNEL_ASSERT_LOCKED();
 		LIST_INSERT_HEAD(&uvn_wlist, uvn, u_wlist);
 	}
 
-	if (oldflags & UVM_VNODE_WANTED)
+	if (uvn->u_flags & UVM_VNODE_WANTED)
 		wakeup(uvn);
+	rw_exit(uvn->u_obj.vmobjlock);
 
 	return &uvn->u_obj;
 }
@@ -446,9 +418,7 @@ uvm_vnp_terminate(struct vnode *vp)
 
 	/*
 	 * must be a valid uvn that is not already dying (because XLOCK
-	 * protects us from that).   the uvn can't in the ALOCK state
-	 * because it is valid, and uvn's that are in the ALOCK state haven't
-	 * been marked valid yet.
+	 * protects us from that).
 	 */
 #ifdef DEBUG
 	/*
@@ -1507,9 +1477,6 @@ uvm_vnp_sync(struct mount *mp)
 		 * If the vnode is "blocked" it means it must be dying, which
 		 * in turn means its in the process of being flushed out so
 		 * we can safely skip it.
-		 *
-		 * note that uvn must already be valid because we found it on
-		 * the wlist (this also means it can't be ALOCK'd).
 		 */
 		if ((uvn->u_flags & UVM_VNODE_BLOCKED) != 0)
 			continue;
Index: uvm/uvm_vnode.h
===================================================================
RCS file: /cvs/src/sys/uvm/uvm_vnode.h,v
diff -u -p -r1.23 uvm_vnode.h
--- uvm/uvm_vnode.h	8 Nov 2025 17:23:22 -0000	1.23
+++ uvm/uvm_vnode.h	9 Nov 2025 17:29:22 -0000
@@ -72,7 +72,6 @@ struct uvm_vnode {
  */
 #define UVM_VNODE_VALID		0x001	/* we are attached to the vnode */
 #define UVM_VNODE_CANPERSIST	0x002	/* we can persist after ref == 0 */
-#define UVM_VNODE_ALOCK		0x004	/* uvn_attach is locked out */
 #define UVM_VNODE_DYING		0x008	/* final detach/terminate in 
 					   progress */
 #define UVM_VNODE_RELKILL	0x010	/* uvn should be killed by releasepg
@@ -92,7 +91,7 @@ struct uvm_vnode {
  * UVM_VNODE_BLOCKED: any condition that should new processes from
  * touching the vnode [set WANTED and sleep to wait for it to clear]
  */
-#define UVM_VNODE_BLOCKED (UVM_VNODE_ALOCK|UVM_VNODE_DYING|UVM_VNODE_RELKILL)
+#define UVM_VNODE_BLOCKED (UVM_VNODE_DYING|UVM_VNODE_RELKILL)
 
 struct uvm_object	*uvn_attach(struct vnode *, vm_prot_t);
 void			uvm_vnp_terminate(struct vnode *);