--IJpNTDwzlM2Ie8A6
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Sep 19, 2005 at 12:27:40AM +0200, Manuel Bouyer wrote:
> Hi,
> I've tried your patch and it doesn't work well: on my test system,
> a reboot of a dumU cause the xbdback driver to go catatonic. It seems that
> it never gets the CMSG_BLKIF_BE_DESTROY message, possibly because its
> CMSG_BLKIF_BE_DISCONNECT reply is missed. xend sends a CMSG_BLKIF_BE_CREATE
> anyway, and things start going wrong here.
> It's possible that not sending the reply message in-place cause this,
> and in fact all other users of ctrl_if_send_response() do a in-place
> reply.

OK, the problem was because you didn't initialize disconnect_rspid.
I also did a few more cleanups. The new patch is attached (against current)

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

--IJpNTDwzlM2Ie8A6
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="xbdback-async-disconnect.patch2"

Index: xbdback.c
===================================================================
RCS file: /cvsroot/src/sys/arch/xen/xen/xbdback.c,v
retrieving revision 1.15
diff -u -r1.15 xbdback.c
--- xbdback.c	10 Sep 2005 18:00:49 -0000	1.15
+++ xbdback.c	19 Sep 2005 21:40:21 -0000
@@ -75,7 +75,7 @@
 struct xbdback_instance;
 
 /* state of a xbdback instance */
-typedef enum {CONNECTED, DISCONNECTED} xbdback_state_t;
+typedef enum {CONNECTED, DISCONNECTING, DISCONNECTED} xbdback_state_t;
 
 /*
  * Since there are a variety of conditions that can block our I/O
@@ -94,7 +94,7 @@
 	SLIST_ENTRY(xbdback_instance) next;
 	domid_t domid;		/* attached to this domain */
 	uint32_t handle;	/* domain-specific handle */
-	xbdback_state_t status;
+	volatile xbdback_state_t status;
 	/* parameters for the communication */
 	unsigned int evtchn;
 	paddr_t ma_ring;
@@ -102,6 +102,9 @@
 	blkif_ring_t *blk_ring;
 	BLKIF_RING_IDX resp_prod; /* our current reply index */
 	BLKIF_RING_IDX req_cons; /* our current request index */
+	/* disconnection must be postponed until all I/O is done */
+	volatile unsigned refcnt;
+	uint8_t disconnect_rspid; /* request id of the disconnect request */
 	/* 
 	 * State for I/O processing/coalescing follows; this has to
 	 * live here instead of on the stack because of the
@@ -124,6 +127,16 @@
 
 	SLIST_HEAD(, xbd_vbd) vbds; /* list of virtual block devices */
 };
+/* Manipulation of the above reference count. */
+/* XXXjld@panix.com: not MP-safe, and move the i386 asm elsewhere. */
+#define xbdi_get(xbdip) (++(xbdip)->refcnt)
+#define xbdi_put(xbdip)                                      \
+do {                                                         \
+	__asm __volatile("decl %0"                           \
+	    : "=m"((xbdip)->refcnt) : "m"((xbdip)->refcnt)); \
+	if (0 == (xbdip)->refcnt)                            \
+               xbdback_finish_disconnect(xbdip);             \
+} while (/* CONSTCOND */ 0)
 
 /* each xbdback instance has a list of vbds associated with it */
 struct xbd_vbd {
@@ -202,6 +215,7 @@
 
 static void xbdback_ctrlif_rx(ctrl_msg_t *, unsigned long);
 static int  xbdback_evthandler(void *);
+static void xbdback_finish_disconnect(struct xbdback_instance *);
 
 static struct xbdback_instance *xbdif_lookup(domid_t, uint32_t);
 static struct xbd_vbd * vbd_lookup(struct xbdback_instance *, blkif_vdev_t);
@@ -317,6 +331,8 @@
 		xbdi->domid = req->domid;
 		xbdi->handle = req->blkif_handle;
 		xbdi->status = DISCONNECTED;
+		xbdi->refcnt = 1;
+		xbdi->blk_ring = NULL;
 		SLIST_INIT(&xbdi->vbds);
 		SLIST_INSERT_HEAD(&xbdback_instances, xbdi, next);
 		
@@ -333,10 +349,13 @@
 			req->status = BLKIF_BE_STATUS_INTERFACE_NOT_FOUND;
 			goto end;
 		}
-		if (xbdi->status == CONNECTED) {
+		if (xbdi->status != DISCONNECTED) {
 			req->status = BLKIF_BE_STATUS_INTERFACE_CONNECTED;
 			goto end;
 		}
+		if (xbdi->blk_ring != NULL)
+			uvm_km_free(kernel_map, (vaddr_t)xbdi->blk_ring,
+			    PAGE_SIZE, UVM_KMF_VAONLY);
 		SLIST_REMOVE(&xbdback_instances, xbdi, xbdback_instance, next);
 		free(xbdi, M_DEVBUF);
 		req->status = BLKIF_BE_STATUS_OKAY;
@@ -355,15 +374,20 @@
 			req->status = BLKIF_BE_STATUS_INTERFACE_NOT_FOUND;
 			goto end;
 		}
-		if (xbdi->status == CONNECTED) {
+		if (xbdi->status != DISCONNECTED) {
 			req->status = BLKIF_BE_STATUS_INTERFACE_CONNECTED;
 			goto end;
 		}
-		ring_addr = uvm_km_alloc(kernel_map, PAGE_SIZE, 0,
-		    UVM_KMF_VAONLY);
-		if (ring_addr == 0) {
-			req->status = BLKIF_BE_STATUS_OUT_OF_MEMORY;
-			goto end;
+		if (xbdi->blk_ring == NULL) {
+			ring_addr = uvm_km_alloc(kernel_map, PAGE_SIZE, 0,
+			    UVM_KMF_VAONLY);
+			if (ring_addr == 0) {
+				req->status = BLKIF_BE_STATUS_OUT_OF_MEMORY;
+				goto end;
+			}
+		} else {
+			ring_addr = (vaddr_t)xbdi->blk_ring;
+			xbdi->blk_ring = NULL;
 		}
 
 		xbdi->ma_ring = req->shmem_frame << PAGE_SHIFT;
@@ -388,6 +412,7 @@
 		printf("xbd backend %d for domain %d using event channel %d\n",
 		    xbdi->handle, xbdi->domid, xbdi->evtchn);
 		hypervisor_enable_event(xbdi->evtchn);
+		xbdi->status = CONNECTED;
 		req->status = BLKIF_BE_STATUS_OKAY;
 		break;
 	}
@@ -395,7 +420,7 @@
 	{
 		blkif_be_disconnect_t *req =
 		    (blkif_be_disconnect_t *)&msg->msg[0];
-		vaddr_t ring_addr;
+		int s;
 
 		if (msg->length != sizeof(blkif_be_disconnect_t))
 			goto error;
@@ -406,12 +431,12 @@
 		}
 		hypervisor_mask_event(xbdi->evtchn);
 		event_remove_handler(xbdi->evtchn, xbdback_evthandler, xbdi);
-		ring_addr = (vaddr_t)xbdi->blk_ring;
-		pmap_remove(pmap_kernel(), ring_addr, ring_addr + PAGE_SIZE);
-		uvm_km_free(kernel_map, ring_addr, PAGE_SIZE,
-		    UVM_KMF_VAONLY);
-		req->status = BLKIF_BE_STATUS_OKAY;
-		break;
+		xbdi->status = DISCONNECTING;
+		xbdi->disconnect_rspid = msg->id;
+		s = splbio();
+		xbdi_put(xbdi);
+		splx(s);
+		return;
 	}
 	case CMSG_BLKIF_BE_VBD_CREATE:
 	{
@@ -590,6 +615,30 @@
 	return;
 }
 
+static void xbdback_finish_disconnect(struct xbdback_instance *xbdi)
+{
+	ctrl_msg_t cmsg;
+	blkif_be_disconnect_t *pdisc = (blkif_be_disconnect_t *)&cmsg.msg;
+	vaddr_t ring_addr;
+	
+	KASSERT(xbdi->status == DISCONNECTING);
+
+       	ring_addr = (vaddr_t)xbdi->blk_ring;
+	pmap_remove(pmap_kernel(), ring_addr, ring_addr + PAGE_SIZE);
+	xbdi->status = DISCONNECTED;
+
+	memset(&cmsg, 0, sizeof(cmsg));
+	cmsg.type = CMSG_BLKIF_BE;
+	cmsg.subtype = CMSG_BLKIF_BE_DISCONNECT;
+	cmsg.id = xbdi->disconnect_rspid;
+	cmsg.length = sizeof(blkif_be_disconnect_t);
+	pdisc->domid = xbdi->domid;
+	pdisc->blkif_handle = xbdi->handle;
+	pdisc->status = BLKIF_BE_STATUS_OKAY;
+
+	ctrl_if_send_response(&cmsg);
+}
+
 static struct xbdback_instance *
 xbdif_lookup(domid_t dom , uint32_t handle)
 {
@@ -618,6 +667,7 @@
 xbdback_evthandler(void *arg)
 {
 	struct xbdback_instance *xbdi = arg;
+
 	if (xbdi->cont == NULL) {
 		xbdi->cont = xbdback_co_main;
 		xbdback_trampoline(xbdi, xbdi);
@@ -728,7 +778,7 @@
 		printf("xbdback_io domain %d: unknown vbd %d\n",
 		       xbdi->domid, xbdi->xen_req->device);
 		error = EINVAL;
-		goto end;	
+		goto end;
 	}
 	if (xbdi->xen_req->operation == BLKIF_OP_WRITE) {
 		if (xbdi->req_vbd->flags & VBD_F_RO) {
@@ -854,6 +904,8 @@
 	vaddr_t start_offset; /* start offset in vm area */
 	int buf_flags;
 
+	xbdi_get(xbdi);
+	
 	xbd_io = xbdi->io = obj;
 	xbd_io->xio_xbdi = xbdi;
 	SLIST_INIT(&xbd_io->xio_rq);
@@ -1067,6 +1119,7 @@
 		    xbd_req->rq_operation, error);
 		xbdback_pool_put(&xbdback_request_pool, xbd_req);
 	}
+	xbdi_put(xbdi);
 	xbdback_pool_put(&xbdback_io_pool, xbd_io);
 }
 

--IJpNTDwzlM2Ie8A6--