From: mpi <mpi@openbsd.org>
Date: Tue, 12 Oct 2021 07:37:42 +0000 (+0000)
Subject: Revert the fix for the deadlock between uvn_io() and uvn_flush().
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=8542cb70f46edc65cef663d50716b9d52666c4ea;p=openbsd

Revert the fix for the deadlock between uvn_io() and uvn_flush().

This fix (ab)use the vnode lock to serialize access to some fields of
the corresponding pages associated with UVM vnode object and this will
create new deadlocks with the introduction of a per-uobj lock.

ok anton@
---

diff --git a/sys/uvm/uvm_vnode.c b/sys/uvm/uvm_vnode.c
index 95e5ece4a15..9c70fb49390 100644
--- a/sys/uvm/uvm_vnode.c
+++ b/sys/uvm/uvm_vnode.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: uvm_vnode.c,v 1.114 2021/06/16 09:02:21 mpi Exp $	*/
+/*	$OpenBSD: uvm_vnode.c,v 1.115 2021/10/12 07:37:42 mpi Exp $	*/
 /*	$NetBSD: uvm_vnode.c,v 1.36 2000/11/24 20:34:01 chs Exp $	*/
 
 /*
@@ -90,9 +90,6 @@ int		 uvn_io(struct uvm_vnode *, vm_page_t *, int, int, int);
 int		 uvn_put(struct uvm_object *, vm_page_t *, int, boolean_t);
 void		 uvn_reference(struct uvm_object *);
 
-int		 uvm_vnode_lock(struct uvm_vnode *);
-void		 uvm_vnode_unlock(struct uvm_vnode *);
-
 /*
  * master pager structure
  */
@@ -878,16 +875,11 @@ uvn_cluster(struct uvm_object *uobj, voff_t offset, voff_t *loffset,
 int
 uvn_put(struct uvm_object *uobj, struct vm_page **pps, int npages, int flags)
 {
-	struct uvm_vnode *uvn = (struct uvm_vnode *)uobj;
 	int retval;
 
 	KERNEL_ASSERT_LOCKED();
 
-	retval = uvm_vnode_lock(uvn);
-	if (retval)
-		return retval;
-	retval = uvn_io(uvn, pps, npages, flags, UIO_WRITE);
-	uvm_vnode_unlock(uvn);
+	retval = uvn_io((struct uvm_vnode*)uobj, pps, npages, flags, UIO_WRITE);
 
 	return retval;
 }
@@ -905,10 +897,9 @@ int
 uvn_get(struct uvm_object *uobj, voff_t offset, struct vm_page **pps,
     int *npagesp, int centeridx, vm_prot_t access_type, int advice, int flags)
 {
-	struct uvm_vnode *uvn = (struct uvm_vnode *)uobj;
 	voff_t current_offset;
 	struct vm_page *ptmp;
-	int lcv, result, gotpages, retval;
+	int lcv, result, gotpages;
 	boolean_t done;
 
 	KERNEL_ASSERT_LOCKED();
@@ -982,18 +973,6 @@ uvn_get(struct uvm_object *uobj, voff_t offset, struct vm_page **pps,
 			return VM_PAGER_UNLOCK;
 	}
 
-	/*
-	 * Before getting non-resident pages which must be populate with data
-	 * using I/O on the backing vnode, lock the same vnode. Such pages are
-	 * about to be allocated and busied (i.e. PG_BUSY) by the current
-	 * thread. Allocating and busying the page(s) before acquiring the
-	 * vnode lock could cause a deadlock with uvn_flush() which acquires the
-	 * vnode lock before waiting on pages to become unbusy and then flushed.
-	 */
-	retval = uvm_vnode_lock(uvn);
-	if (retval)
-		return retval;
-
 	/*
 	 * step 2: get non-resident or busy pages.
 	 * data structures are unlocked.
@@ -1080,15 +1059,14 @@ uvn_get(struct uvm_object *uobj, voff_t offset, struct vm_page **pps,
 		 * we have a "fake/busy/clean" page that we just allocated.  do
 		 * I/O to fill it with valid data.
 		 */
-		result = uvn_io(uvn, &ptmp, 1, PGO_SYNCIO, UIO_READ);
+		result = uvn_io((struct uvm_vnode *) uobj, &ptmp, 1,
+		    PGO_SYNCIO, UIO_READ);
 
 		/*
 		 * I/O done.  because we used syncio the result can not be
 		 * PEND or AGAIN.
 		 */
 		if (result != VM_PAGER_OK) {
-			uvm_vnode_unlock(uvn);
-
 			if (ptmp->pg_flags & PG_WANTED)
 				wakeup(ptmp);
 
@@ -1119,15 +1097,12 @@ uvn_get(struct uvm_object *uobj, voff_t offset, struct vm_page **pps,
 
 	}
 
-	uvm_vnode_unlock(uvn);
-
 	return (VM_PAGER_OK);
 }
 
 /*
  * uvn_io: do I/O to a vnode
  *
- * => uvn: the backing vnode must be locked
  * => prefer map unlocked (not required)
  * => flags: PGO_SYNCIO -- use sync. I/O
  * => XXX: currently we use VOP_READ/VOP_WRITE which are only sync.
@@ -1213,17 +1188,43 @@ uvn_io(struct uvm_vnode *uvn, vm_page_t *pps, int npages, int flags, int rw)
 	}
 
 	/* do the I/O!  (XXX: curproc?) */
-	if (rw == UIO_READ)
-		result = VOP_READ(vn, &uio, 0, curproc->p_ucred);
-	else
-		result = VOP_WRITE(vn, &uio,
-		    (flags & PGO_PDFREECLUST) ? IO_NOCACHE : 0,
-		    curproc->p_ucred);
+	/*
+	 * This process may already have this vnode locked, if we faulted in
+	 * copyin() or copyout() on a region backed by this vnode
+	 * while doing I/O to the vnode.  If this is the case, don't
+	 * panic.. instead, return the error to the user.
+	 *
+	 * XXX this is a stopgap to prevent a panic.
+	 * Ideally, this kind of operation *should* work.
+	 */
+	result = 0;
+	if ((uvn->u_flags & UVM_VNODE_VNISLOCKED) == 0)
+		result = vn_lock(vn, LK_EXCLUSIVE | LK_RECURSEFAIL);
+
+	if (result == 0) {
+		/* NOTE: vnode now locked! */
+		if (rw == UIO_READ)
+			result = VOP_READ(vn, &uio, 0, curproc->p_ucred);
+		else
+			result = VOP_WRITE(vn, &uio,
+			    (flags & PGO_PDFREECLUST) ? IO_NOCACHE : 0,
+			    curproc->p_ucred);
+
+		if ((uvn->u_flags & UVM_VNODE_VNISLOCKED) == 0)
+			VOP_UNLOCK(vn);
+
+	}
 
 	if (netunlocked)
 		NET_LOCK();
 
-	/* zero out rest of buffer (if needed) */
+
+	/* NOTE: vnode now unlocked (unless vnislocked) */
+	/*
+	 * result == unix style errno (0 == OK!)
+	 *
+	 * zero out rest of buffer (if needed)
+	 */
 	if (result == 0) {
 		got = wanted - uio.uio_resid;
 
@@ -1244,14 +1245,13 @@ uvn_io(struct uvm_vnode *uvn, vm_page_t *pps, int npages, int flags, int rw)
 		wakeup(&uvn->u_nio);
 	}
 
-	if (result == 0)
+	if (result == 0) {
 		return VM_PAGER_OK;
-
-	if (result == EIO) {
-		/* Signal back to uvm_vnode_unlock(). */
-		uvn->u_flags |= UVM_VNODE_IOERROR;
-	}
-	return VM_PAGER_ERROR;
+	} else {
+		while (rebooting)
+			tsleep_nsec(&rebooting, PVM, "uvndead", INFSLP);
+		return VM_PAGER_ERROR;
+ 	}
 }
 
 /*
@@ -1468,52 +1468,3 @@ uvm_vnp_sync(struct mount *mp)
 
 	rw_exit_write(&uvn_sync_lock);
 }
-
-int
-uvm_vnode_lock(struct uvm_vnode *uvn)
-{
-	int error;
-	int netunlocked = 0;
-
-	if (uvn->u_flags & UVM_VNODE_VNISLOCKED)
-		return VM_PAGER_OK;
-
-	/*
-	 * This thread may already have the net lock, if we faulted in copyin()
-	 * or copyout() in the network stack.
-	 */
-	if (rw_status(&netlock) == RW_WRITE) {
-		NET_UNLOCK();
-		netunlocked = 1;
-	}
-
-	/*
-	 * This thread may already have this vnode locked, if we faulted in
-	 * copyin() or copyout() on a region backed by this vnode
-	 * while doing I/O to the vnode. If this is the case, don't panic but
-	 * instead return an error; as dictated by the LK_RECURSEFAIL flag.
-	 *
-	 * XXX this is a stopgap to prevent a panic.
-	 * Ideally, this kind of operation *should* work.
-	 */
-	error = vn_lock(uvn->u_vnode, LK_EXCLUSIVE | LK_RECURSEFAIL);
-	if (netunlocked)
-		NET_LOCK();
-	return error ? VM_PAGER_ERROR : VM_PAGER_OK;
-}
-
-void
-uvm_vnode_unlock(struct uvm_vnode *uvn)
-{
-	int error;
-
-	if ((uvn->u_flags & UVM_VNODE_VNISLOCKED) == 0)
-		VOP_UNLOCK(uvn->u_vnode);
-
-	error = uvn->u_flags & UVM_VNODE_IOERROR;
-	uvn->u_flags &= ~UVM_VNODE_IOERROR;
-	if (error) {
-		while (rebooting)
-			tsleep_nsec(&rebooting, PVM, "uvndead", INFSLP);
-	}
-}
diff --git a/sys/uvm/uvm_vnode.h b/sys/uvm/uvm_vnode.h
index ff67e3f0bf4..1716511b91e 100644
--- a/sys/uvm/uvm_vnode.h
+++ b/sys/uvm/uvm_vnode.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: uvm_vnode.h,v 1.17 2021/03/04 08:38:48 mpi Exp $	*/
+/*	$OpenBSD: uvm_vnode.h,v 1.18 2021/10/12 07:37:42 mpi Exp $	*/
 /*	$NetBSD: uvm_vnode.h,v 1.9 2000/03/26 20:54:48 kleink Exp $	*/
 
 /*
@@ -84,7 +84,6 @@ struct uvm_vnode {
 					   i/o sync to clear so it can do
 					   i/o */
 #define UVM_VNODE_WRITEABLE	0x200	/* uvn has pages that are writeable */
-#define UVM_VNODE_IOERROR	0x400	/* i/o error occurred in uvn_io() */
 
 /*
  * UVM_VNODE_BLOCKED: any condition that should new processes from