[RFC Patch v3 00/18] Some bugfix patches

[RFC Patch v3 00/18] Some bugfix patches

[Wen Congyang]

These bugs are found when we implement COLO, or rebase
COLO to upstream xen. They are independent patches, so
post them in separate series.

Patch 1-3: fix bugs in xc_domain_restore()
Patch 4-16: blktap related bugfix
Patch 17: libxl nic related bugfix
Patch 18: hypervisor bugfix. We find this bug before
          rebasing colo to newest xen.

The codes are also hosted on github:
https://github.com/wencongyang/xen/commits/bugfix

Hong Tao (1):
  copy the correct page to memory

Lai Jiangshan (1):
  blktap2: dynamic allocate aio_requests to avoid -EBUSY error

Wen Congyang (16):
  csum the correct page
  don't zero out ioreq page
  blktap2: return the correct dev path
  blktap2: use correct way to get free event id
  blktap2: don't return negative event id
  blktap2: use correct way to define array.
  block-remus: fix memory leak
  block-remus: pass uuid to the callback td_open
  block-remus: use correct way to get remus_image
  block-remus: fix bug in tdremus_close()
  don't call client_flush() when switching to unprotected mode
  pass correct file to qemu if we use blktap2
  support blktap remus in xl
  update libxl__device_disk_from_xs_be() to support blktap device
  read nictype from xenstore
  x86/hvm: Always set pending event injection when loading VMC[BS]
    state.

 tools/blktap2/drivers/block-aio.c         |  41 ++++++++-
 tools/blktap2/drivers/block-cache.c       |   4 +-
 tools/blktap2/drivers/block-log.c         |   4 +-
 tools/blktap2/drivers/block-qcow.c        |   5 +-
 tools/blktap2/drivers/block-ram.c         |   5 +-
 tools/blktap2/drivers/block-remus.c       | 134 +++++++++++++++++-------------
 tools/blktap2/drivers/block-vhd.c         |   5 +-
 tools/blktap2/drivers/scheduler.c         |  33 +++++++-
 tools/blktap2/drivers/tapdisk-control.c   |  11 +--
 tools/blktap2/drivers/tapdisk-disktype.c  |  12 +--
 tools/blktap2/drivers/tapdisk-disktype.h  |   2 +-
 tools/blktap2/drivers/tapdisk-interface.c |   3 +-
 tools/blktap2/drivers/tapdisk.h           |   2 +-
 tools/libxc/xc_domain_restore.c           |  29 +++++--
 tools/libxl/libxl.c                       |  51 +++++++++++-
 tools/libxl/libxl_blktap2.c               |  33 ++++++++
 tools/libxl/libxl_device.c                |   4 +-
 tools/libxl/libxl_dm.c                    |  19 ++++-
 tools/libxl/libxl_internal.h              |   4 +
 tools/libxl/libxl_noblktap2.c             |   6 ++
 tools/libxl/libxl_types.idl               |   1 +
 tools/libxl/libxl_utils.c                 |  21 +++++
 tools/libxl/libxl_utils.h                 |   1 +
 tools/libxl/libxlu_disk_l.l               |   1 +
 xen/arch/x86/hvm/svm/svm.c                |  15 ++--
 xen/arch/x86/hvm/vmx/vmx.c                |  25 +++---
 26 files changed, 347 insertions(+), 124 deletions(-)

-- 
1.9.3

[RFC Patch v3 01/18] copy the correct page to memory

[Wen Congyang]

From: Hong Tao <bobby.hong@huawei.com>

apply_batch() only handles MAX_BATCH_SIZE pages at one time. If
there is some bogus/unmapped/allocate-only/broken page, we will
skip it. So when we call apply_batch() again, the first page's
index is curbatch - invalid_pages. invalid_pages stores the number
of bogus/unmapped/allocate-only/broken pages we have found.

In many cases, invalid_pages is 0, so we don't catch this error.

Signed-off-by: Hong Tao <bobby.hong@huawei.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 tools/libxc/xc_domain_restore.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index b9a56d5..bec716c 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -1106,7 +1106,7 @@ static int pagebuf_get(xc_interface *xch, struct restore_ctx *ctx,
 static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx,
                        xen_pfn_t* region_mfn, unsigned long* pfn_type, int pae_extended_cr3,
                        struct xc_mmu* mmu,
-                       pagebuf_t* pagebuf, int curbatch)
+                       pagebuf_t* pagebuf, int curbatch, int *invalid_pages)
 {
     int i, j, curpage, nr_mfns;
     int k, scount;
@@ -1121,6 +1121,12 @@ static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx,
     struct domain_info_context *dinfo = &ctx->dinfo;
     int* pfn_err = NULL;
     int rc = -1;
+    int local_invalid_pages = 0;
+    /* We have handled curbatch pages before this batch, and there are
+     * *invalid_pages pages that are not in pagebuf->pages. So the first
+     * page for this page is (curbatch - *invalid_pages) page.
+     */
+    int first_page = curbatch - *invalid_pages;
 
     unsigned long mfn, pfn, pagetype;
 
@@ -1293,10 +1299,13 @@ static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx,
         pfn      = pagebuf->pfn_types[i + curbatch] & ~XEN_DOMCTL_PFINFO_LTAB_MASK;
         pagetype = pagebuf->pfn_types[i + curbatch] &  XEN_DOMCTL_PFINFO_LTAB_MASK;
 
-        if ( pagetype == XEN_DOMCTL_PFINFO_XTAB 
+        if ( pagetype == XEN_DOMCTL_PFINFO_XTAB
              || pagetype == XEN_DOMCTL_PFINFO_XALLOC)
+        {
+            local_invalid_pages++;
             /* a bogus/unmapped/allocate-only page: skip it */
             continue;
+        }
 
         if ( pagetype == XEN_DOMCTL_PFINFO_BROKEN )
         {
@@ -1306,6 +1315,8 @@ static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx,
                       "dom=%d, pfn=%lx\n", dom, pfn);
                 goto err_mapped;
             }
+
+            local_invalid_pages++;
             continue;
         }
 
@@ -1344,7 +1355,7 @@ static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx,
             }
         }
         else
-            memcpy(page, pagebuf->pages + (curpage + curbatch) * PAGE_SIZE,
+            memcpy(page, pagebuf->pages + (first_page + curpage) * PAGE_SIZE,
                    PAGE_SIZE);
 
         pagetype &= XEN_DOMCTL_PFINFO_LTABTYPE_MASK;
@@ -1418,6 +1429,7 @@ static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx,
     } /* end of 'batch' for loop */
 
     rc = nraces;
+    *invalid_pages += local_invalid_pages;
 
   err_mapped:
     munmap(region_base, j*PAGE_SIZE);
@@ -1621,7 +1633,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
  loadpages:
     for ( ; ; )
     {
-        int j, curbatch;
+        int j, curbatch, invalid_pages;
 
         xc_report_progress_step(xch, n, dinfo->p2m_size);
 
@@ -1665,11 +1677,13 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
 
         /* break pagebuf into batches */
         curbatch = 0;
+        invalid_pages = 0;
         while ( curbatch < j ) {
             int brc;
 
             brc = apply_batch(xch, dom, ctx, region_mfn, pfn_type,
-                              pae_extended_cr3, mmu, &pagebuf, curbatch);
+                              pae_extended_cr3, mmu, &pagebuf, curbatch,
+                              &invalid_pages);
             if ( brc < 0 )
                 goto out;
 
-- 
1.9.3

[RFC Patch v3 02/18] csum the correct page

[Wen Congyang]

In verify mode, we map the guest memory, and the guest page is
region_base + i * PAGE_SIZE. So we should csum page (region_base
+ i * PAGE_SIZE), not (region_base + (i+curbatch) * PAGE_SIZE)

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 tools/libxc/xc_domain_restore.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index bec716c..fb4ddfc 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -1405,7 +1405,7 @@ static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx,
 
                 DPRINTF("************** pfn=%lx type=%lx gotcs=%08lx "
                         "actualcs=%08lx\n", pfn, pagebuf->pfn_types[pfn],
-                        csum_page(region_base + (i + curbatch)*PAGE_SIZE),
+                        csum_page(region_base + i * PAGE_SIZE),
                         csum_page(buf));
 
                 for ( v = 0; v < 4; v++ )
-- 
1.9.3

[RFC Patch v3 03/18] don't zero out ioreq page

[Wen Congyang]

ioreq page may contain some pending I/O requests, and we need to
handle the pending I/O req after migration.

TODO:
1. update qemu to handle the pending I/O req

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Cc: Paul Durrant <paul.durrant@citrix.com>
---
 tools/libxc/xc_domain_restore.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index fb4ddfc..21a6177 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -2310,8 +2310,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
     }
 
     /* These comms pages need to be zeroed at the start of day */
-    if ( xc_clear_domain_page(xch, dom, tailbuf.u.hvm.magicpfns[0]) ||
-         xc_clear_domain_page(xch, dom, tailbuf.u.hvm.magicpfns[1]) ||
+    if ( xc_clear_domain_page(xch, dom, tailbuf.u.hvm.magicpfns[1]) ||
          xc_clear_domain_page(xch, dom, tailbuf.u.hvm.magicpfns[2]) )
     {
         PERROR("error zeroing magic pages");
-- 
1.9.3

[RFC Patch v3 04/18] blktap2: dynamic allocate aio_requests to avoid -EBUSY error

[Wen Congyang]

From: Lai Jiangshan <laijs@cn.fujitsu.com>

In normal case, there are at most TAPDISK_DATA_REQUESTS request
at the same time. But in remus mode, the write requests are
forwarded from the master side, and cached in block-remus. All
cached requests will be forwarded to aio driver when syncing
primary vm and backup vm. In this case, The number of requests
may be more than TAPDISK_DATA_REQUESTS. So aio driver can't hanlde
these requests at the same time, it will cause tapdisk2 exit.

We don't know how many requests will be handled, so dynamic allocate
aio_requests to avoid this error.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
Signed-off-by: Jiang Yunhong <yunhong.jiang@intel.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 tools/blktap2/drivers/block-aio.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/tools/blktap2/drivers/block-aio.c b/tools/blktap2/drivers/block-aio.c
index f398da2..10ab20b 100644
--- a/tools/blktap2/drivers/block-aio.c
+++ b/tools/blktap2/drivers/block-aio.c
@@ -55,9 +55,10 @@ struct tdaio_state {
 	int                  fd;
 	td_driver_t         *driver;
 
+	int                  aio_max_count;
 	int                  aio_free_count;	
 	struct aio_request   aio_requests[MAX_AIO_REQS];
-	struct aio_request  *aio_free_list[MAX_AIO_REQS];
+	struct aio_request   **aio_free_list;
 };
 
 /*Get Image size, secsize*/
@@ -122,6 +123,11 @@ int tdaio_open(td_driver_t *driver, const char *name, td_flag_t flags)
 
 	memset(prv, 0, sizeof(struct tdaio_state));
 
+	prv->aio_free_list = malloc(MAX_AIO_REQS * sizeof(*prv->aio_free_list));
+	if (!prv->aio_free_list)
+		return -ENOMEM;
+
+	prv->aio_max_count = MAX_AIO_REQS;
 	prv->aio_free_count = MAX_AIO_REQS;
 	for (i = 0; i < MAX_AIO_REQS; i++)
 		prv->aio_free_list[i] = &prv->aio_requests[i];
@@ -159,6 +165,28 @@ done:
 	return ret;	
 }
 
+static int tdaio_refill(struct tdaio_state *prv)
+{
+	struct aio_request **new, *new_req;
+	int i, max = prv->aio_max_count + MAX_AIO_REQS;
+
+	new = realloc(prv->aio_free_list, max * sizeof(*prv->aio_free_list));
+	if (!new)
+		return -1;
+	prv->aio_free_list = new;
+
+	new_req = calloc(MAX_AIO_REQS, sizeof(*new_req));
+	if (!new_req)
+		return -1;
+
+	prv->aio_max_count = max;
+	prv->aio_free_count = MAX_AIO_REQS;
+	for (i = 0; i < MAX_AIO_REQS; i++)
+		prv->aio_free_list[i] = &new_req[i];
+
+	return 0;
+}
+
 void tdaio_complete(void *arg, struct tiocb *tiocb, int err)
 {
 	struct aio_request *aio = (struct aio_request *)arg;
@@ -207,8 +235,10 @@ void tdaio_queue_write(td_driver_t *driver, td_request_t treq)
 	size    = treq.secs * driver->info.sector_size;
 	offset  = treq.sec  * (uint64_t)driver->info.sector_size;
 
-	if (prv->aio_free_count == 0)
-		goto fail;
+	if (prv->aio_free_count == 0) {
+		if (tdaio_refill(prv) < 0)
+			goto fail;
+	}
 
 	aio        = prv->aio_free_list[--prv->aio_free_count];
 	aio->treq  = treq;
-- 
1.9.3

[RFC Patch v3 05/18] blktap2: return the correct dev path

[Wen Congyang]

The user uses TAPDISK_MESSAGE_OPEN to pass the devpath to tapdisk2,
and will use TAPDISK_MESSAGE_LIST to query and get the pid of the
tapdisk2.

The devpath's format is: driver:params[|driver:params[...]].
The first vbd image only contains the first params, and we will
return driver:params, not devpath. The devpath is stored in
vbd->name, so return vbd->name instead of image->name.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 tools/blktap2/drivers/tapdisk-control.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/tools/blktap2/drivers/tapdisk-control.c b/tools/blktap2/drivers/tapdisk-control.c
index 0b5cf3c..3a4ec8e 100644
--- a/tools/blktap2/drivers/tapdisk-control.c
+++ b/tools/blktap2/drivers/tapdisk-control.c
@@ -270,15 +270,10 @@ tapdisk_control_list(struct tapdisk_control_connection *connection,
 		response.u.list.state   = vbd->state;
 		response.u.list.path[0] = 0;
 
-		if (!list_empty(&vbd->images)) {
-			td_image_t *image = list_entry(vbd->images.next,
-						       td_image_t, next);
+		if (vbd->name)
 			snprintf(response.u.list.path,
 				 sizeof(response.u.list.path),
-				 "%s:%s",
-				 tapdisk_disk_types[image->type]->name,
-				 image->name);
-		}
+				 "%s", vbd->name);
 
 		tapdisk_control_write_message(connection->socket, &response, 2);
 	}
-- 
1.9.3

[RFC Patch v3 06/18] blktap2: use correct way to get free event id

[Wen Congyang]

If we register/unregister event too many times, and we use event id
from 1 again. But we don't check it if it is used.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 tools/blktap2/drivers/scheduler.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/tools/blktap2/drivers/scheduler.c b/tools/blktap2/drivers/scheduler.c
index 6b8d009..dd608dd 100644
--- a/tools/blktap2/drivers/scheduler.c
+++ b/tools/blktap2/drivers/scheduler.c
@@ -160,6 +160,31 @@ scheduler_run_events(scheduler_t *s)
 	}
 }
 
+static int
+get_free_id(scheduler_t *s)
+{
+	event_t *event, *tmp;
+	int old_uuid = s->uuid;
+	int id = s->uuid++;
+
+	if (!s->uuid)
+		s->uuid++;
+
+retry:
+	scheduler_for_each_event(s, event, tmp)
+		if (event->id == id) {
+			id = s->uuid++;
+			if (!s->uuid)
+				s->uuid++;
+			if (id == old_uuid)
+				return 0;
+
+			goto retry;
+		}
+
+	return id;
+}
+
 int
 scheduler_register_event(scheduler_t *s, char mode, int fd,
 			 int timeout, event_cb_t cb, void *private)
@@ -187,10 +212,12 @@ scheduler_register_event(scheduler_t *s, char mode, int fd,
 	event->deadline = now.tv_sec + timeout;
 	event->cb       = cb;
 	event->private  = private;
-	event->id       = s->uuid++;
+	event->id       = get_free_id(s);
 
-	if (!s->uuid)
-		s->uuid++;
+	if (!event->id) {
+		free(event);
+		return -EBUSY;
+	}
 
 	list_add_tail(&event->next, &s->events);
 
-- 
1.9.3

[RFC Patch v3 07/18] blktap2: don't return negative event id

[Wen Congyang]

If we find some error when registering a new event, we will return
a negative value. So we should skip negative event id.

Also fix a wrong check of return value.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 tools/blktap2/drivers/scheduler.c       | 8 ++++----
 tools/blktap2/drivers/tapdisk-control.c | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/blktap2/drivers/scheduler.c b/tools/blktap2/drivers/scheduler.c
index dd608dd..e07528b 100644
--- a/tools/blktap2/drivers/scheduler.c
+++ b/tools/blktap2/drivers/scheduler.c
@@ -167,15 +167,15 @@ get_free_id(scheduler_t *s)
 	int old_uuid = s->uuid;
 	int id = s->uuid++;
 
-	if (!s->uuid)
-		s->uuid++;
+	if (s->uuid < 0)
+		s->uuid = 1;
 
 retry:
 	scheduler_for_each_event(s, event, tmp)
 		if (event->id == id) {
 			id = s->uuid++;
-			if (!s->uuid)
-				s->uuid++;
+			if (s->uuid < 0)
+				s->uuid = 1;
 			if (id == old_uuid)
 				return 0;
 
diff --git a/tools/blktap2/drivers/tapdisk-control.c b/tools/blktap2/drivers/tapdisk-control.c
index 3a4ec8e..4e5f748 100644
--- a/tools/blktap2/drivers/tapdisk-control.c
+++ b/tools/blktap2/drivers/tapdisk-control.c
@@ -700,7 +700,7 @@ tapdisk_control_accept(event_id_t id, char mode, void *private)
 					    connection->socket, 0,
 					    tapdisk_control_handle_request,
 					    connection);
-	if (err == -1) {
+	if (err < 0) {
 		close(fd);
 		free(connection);
 		EPRINTF("failed to register new control event: %d\n", err);
-- 
1.9.3

[RFC Patch v3 08/18] blktap2: use correct way to define array.

[Wen Congyang]

Currently, we use the following way to define an array:
type array[] = {
    [index] = xxx,
    0,
};
So array[index+1] will be NULL. If index is not the last
index, it will override another index.

tapdisk_vbd_index is not defined, but array[DISK_TYPE_VINDEX]
is overridden, so we don't find this problem when building
the source.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 tools/blktap2/drivers/tapdisk-disktype.c | 12 ++----------
 tools/blktap2/drivers/tapdisk-disktype.h |  2 +-
 2 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/tools/blktap2/drivers/tapdisk-disktype.c b/tools/blktap2/drivers/tapdisk-disktype.c
index e9a6890..8d1383b 100644
--- a/tools/blktap2/drivers/tapdisk-disktype.c
+++ b/tools/blktap2/drivers/tapdisk-disktype.c
@@ -82,12 +82,6 @@ static const disk_info_t block_cache_disk = {
        1,
 };
 
-static const disk_info_t vhd_index_disk = {
-       "vhdi",
-       "vhd index image (vhdi)",
-       1,
-};
-
 static const disk_info_t log_disk = {
 	"log",
 	"write logger (log)",
@@ -110,9 +104,8 @@ const disk_info_t *tapdisk_disk_types[] = {
 	[DISK_TYPE_QCOW]	= &qcow_disk,
 	[DISK_TYPE_BLOCK_CACHE] = &block_cache_disk,
 	[DISK_TYPE_LOG]	= &log_disk,
-	[DISK_TYPE_VINDEX]	= &vhd_index_disk,
 	[DISK_TYPE_REMUS]	= &remus_disk,
-	0,
+	[DISK_TYPE_MAX]		= NULL,
 };
 
 extern struct tap_disk tapdisk_aio;
@@ -137,10 +130,9 @@ const struct tap_disk *tapdisk_disk_drivers[] = {
 	[DISK_TYPE_RAM]         = &tapdisk_ram,
 	[DISK_TYPE_QCOW]        = &tapdisk_qcow,
 	[DISK_TYPE_BLOCK_CACHE] = &tapdisk_block_cache,
-	[DISK_TYPE_VINDEX]      = &tapdisk_vhd_index,
 	[DISK_TYPE_LOG]         = &tapdisk_log,
 	[DISK_TYPE_REMUS]       = &tapdisk_remus,
-	0,
+	[DISK_TYPE_MAX]         = NULL,
 };
 
 int
diff --git a/tools/blktap2/drivers/tapdisk-disktype.h b/tools/blktap2/drivers/tapdisk-disktype.h
index b697eea..c574990 100644
--- a/tools/blktap2/drivers/tapdisk-disktype.h
+++ b/tools/blktap2/drivers/tapdisk-disktype.h
@@ -39,7 +39,7 @@
 #define DISK_TYPE_BLOCK_CACHE 7
 #define DISK_TYPE_LOG         8
 #define DISK_TYPE_REMUS       9
-#define DISK_TYPE_VINDEX      10
+#define DISK_TYPE_MAX         10
 
 #define DISK_TYPE_NAME_MAX    32
 
-- 
1.9.3

[RFC Patch v3 09/18] block-remus: fix memory leak

[Wen Congyang]

Fix the following two memory leak:
1. If s->ramdisk.prev is not NULL, we merge the write requests in
   s->ramdisk.h into s->ramdisk.prev, and then destroy s->ramdisk.h.
   But we forget to free hash value when destroying s->ramdisk.h.
2. When write requests is finished, replicated_write_callback() will
   be called. We forget free the buff in this function.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
Signed-off-by: Jiang Yunhong <yunhong.jiang@intel.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 tools/blktap2/drivers/block-remus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/blktap2/drivers/block-remus.c b/tools/blktap2/drivers/block-remus.c
index 079588d..4ce9dbe 100644
--- a/tools/blktap2/drivers/block-remus.c
+++ b/tools/blktap2/drivers/block-remus.c
@@ -602,7 +602,7 @@ static int ramdisk_start_flush(td_driver_t *driver)
 		}
 		free(sectors);
 
-		hashtable_destroy (s->ramdisk.h, 0);
+		hashtable_destroy (s->ramdisk.h, 1);
 	} else
 		s->ramdisk.prev = s->ramdisk.h;
 
-- 
1.9.3

[RFC Patch v3 10/18] block-remus: pass uuid to the callback td_open

[Wen Congyang]

remus's callback td_open needs uuid, but it is hard coded as 0.
After commit 4b1af8, the vbd's uuid is the minor of the blktap
device, not 0.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 tools/blktap2/drivers/block-aio.c         | 3 ++-
 tools/blktap2/drivers/block-cache.c       | 3 ++-
 tools/blktap2/drivers/block-log.c         | 3 ++-
 tools/blktap2/drivers/block-qcow.c        | 3 ++-
 tools/blktap2/drivers/block-ram.c         | 3 ++-
 tools/blktap2/drivers/block-remus.c       | 8 ++------
 tools/blktap2/drivers/block-vhd.c         | 3 ++-
 tools/blktap2/drivers/tapdisk-interface.c | 4 +++-
 tools/blktap2/drivers/tapdisk.h           | 2 +-
 9 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/tools/blktap2/drivers/block-aio.c b/tools/blktap2/drivers/block-aio.c
index 10ab20b..1b560e5 100644
--- a/tools/blktap2/drivers/block-aio.c
+++ b/tools/blktap2/drivers/block-aio.c
@@ -111,7 +111,8 @@ static int tdaio_get_image_info(int fd, td_disk_info_t *info)
 }
 
 /* Open the disk file and initialize aio state. */
-int tdaio_open(td_driver_t *driver, const char *name, td_flag_t flags)
+int tdaio_open(td_driver_t *driver, const char *name, td_flag_t flags,
+	       td_uuid_t uuid)
 {
 	int i, fd, ret, o_flags;
 	struct tdaio_state *prv;
diff --git a/tools/blktap2/drivers/block-cache.c b/tools/blktap2/drivers/block-cache.c
index 1d2f4eb..cd6ea6a 100644
--- a/tools/blktap2/drivers/block-cache.c
+++ b/tools/blktap2/drivers/block-cache.c
@@ -517,7 +517,8 @@ block_cache_put_request(block_cache_t *cache, block_cache_request_t *breq)
 }
 
 static int
-block_cache_open(td_driver_t *driver, const char *name, td_flag_t flags)
+block_cache_open(td_driver_t *driver, const char *name, td_flag_t flags,
+		 td_uuid_t uuid)
 {
 	int i, err;
 	radix_tree_t *tree;
diff --git a/tools/blktap2/drivers/block-log.c b/tools/blktap2/drivers/block-log.c
index 5330cdc..7b33b63 100644
--- a/tools/blktap2/drivers/block-log.c
+++ b/tools/blktap2/drivers/block-log.c
@@ -585,7 +585,8 @@ static void ctl_request(event_id_t id, char mode, void *private)
 
 static int tdlog_close(td_driver_t*);
 
-static int tdlog_open(td_driver_t* driver, const char* name, td_flag_t flags)
+static int tdlog_open(td_driver_t* driver, const char* name, td_flag_t flags,
+		      td_uuid_t uuid)
 {
   struct tdlog_state* s = (struct tdlog_state*)driver->data;
   int rc;
diff --git a/tools/blktap2/drivers/block-qcow.c b/tools/blktap2/drivers/block-qcow.c
index b45bcaa..64dfafc 100644
--- a/tools/blktap2/drivers/block-qcow.c
+++ b/tools/blktap2/drivers/block-qcow.c
@@ -865,7 +865,8 @@ out:
 }
 
 /* Open the disk file and initialize qcow state. */
-int tdqcow_open (td_driver_t *driver, const char *name, td_flag_t flags)
+int tdqcow_open (td_driver_t *driver, const char *name, td_flag_t flags,
+		 td_uuid_t uuid)
 {
 	int fd, len, i, ret, size, o_flags;
 	td_disk_info_t *bs = &(driver->info);
diff --git a/tools/blktap2/drivers/block-ram.c b/tools/blktap2/drivers/block-ram.c
index a859481..b64a194 100644
--- a/tools/blktap2/drivers/block-ram.c
+++ b/tools/blktap2/drivers/block-ram.c
@@ -108,7 +108,8 @@ static int get_image_info(int fd, td_disk_info_t *info)
 }
 
 /* Open the disk file and initialize ram state. */
-int tdram_open (td_driver_t *driver, const char *name, td_flag_t flags)
+int tdram_open (td_driver_t *driver, const char *name, td_flag_t flags,
+		td_uuid_t uuid)
 {
 	char *p;
 	uint64_t size;
diff --git a/tools/blktap2/drivers/block-remus.c b/tools/blktap2/drivers/block-remus.c
index 4ce9dbe..504f6b4 100644
--- a/tools/blktap2/drivers/block-remus.c
+++ b/tools/blktap2/drivers/block-remus.c
@@ -1633,18 +1633,14 @@ static int ctl_register(struct tdremus_state *s)
 /* interface */
 
 static int tdremus_open(td_driver_t *driver, const char *name,
-			td_flag_t flags)
+			td_flag_t flags, td_uuid_t uuid)
 {
 	struct tdremus_state *s = (struct tdremus_state *)driver->data;
 	int rc;
 
 	RPRINTF("opening %s\n", name);
 
-	/* first we need to get the underlying vbd for this driver stack. To do so we
-	 * need to know the vbd's id. Fortunately, for tapdisk2 this is hard-coded as
-	 * 0 (see tapdisk2.c)
-	 */
-	device_vbd = tapdisk_server_get_vbd(0);
+	device_vbd = tapdisk_server_get_vbd(uuid);
 
 	memset(s, 0, sizeof(*s));
 	s->server_fd.fd = -1;
diff --git a/tools/blktap2/drivers/block-vhd.c b/tools/blktap2/drivers/block-vhd.c
index 76ea5bd..06e9c89 100644
--- a/tools/blktap2/drivers/block-vhd.c
+++ b/tools/blktap2/drivers/block-vhd.c
@@ -675,7 +675,8 @@ __vhd_open(td_driver_t *driver, const char *name, vhd_flag_t flags)
 }
 
 static int
-_vhd_open(td_driver_t *driver, const char *name, td_flag_t flags)
+_vhd_open(td_driver_t *driver, const char *name, td_flag_t flags,
+	  td_uuid_t uuid)
 {
 	vhd_flag_t vhd_flags = 0;
 
diff --git a/tools/blktap2/drivers/tapdisk-interface.c b/tools/blktap2/drivers/tapdisk-interface.c
index 2e51883..36b5393 100644
--- a/tools/blktap2/drivers/tapdisk-interface.c
+++ b/tools/blktap2/drivers/tapdisk-interface.c
@@ -63,6 +63,7 @@ __td_open(td_image_t *image, td_disk_info_t *info)
 {
 	int err;
 	td_driver_t *driver;
+	td_vbd_t *vbd = image->private;
 
 	driver = image->driver;
 	if (!driver) {
@@ -78,7 +79,8 @@ __td_open(td_image_t *image, td_disk_info_t *info)
 	}
 
 	if (!td_flag_test(driver->state, TD_DRIVER_OPEN)) {
-		err = driver->ops->td_open(driver, image->name, image->flags);
+		err = driver->ops->td_open(driver, image->name, image->flags,
+					   vbd->uuid);
 		if (err) {
 			if (!image->driver)
 				tapdisk_driver_free(driver);
diff --git a/tools/blktap2/drivers/tapdisk.h b/tools/blktap2/drivers/tapdisk.h
index 66d508e..459eaec 100644
--- a/tools/blktap2/drivers/tapdisk.h
+++ b/tools/blktap2/drivers/tapdisk.h
@@ -157,7 +157,7 @@ struct tap_disk {
 	const char                  *disk_type;
 	td_flag_t                    flags;
 	int                          private_data_size;
-	int (*td_open)               (td_driver_t *, const char *, td_flag_t);
+	int (*td_open)               (td_driver_t *, const char *, td_flag_t, td_uuid_t);
 	int (*td_close)              (td_driver_t *);
 	int (*td_get_parent_id)      (td_driver_t *, td_disk_id_t *);
 	int (*td_validate_parent)    (td_driver_t *, td_driver_t *, td_flag_t);
-- 
1.9.3

[RFC Patch v3 11/18] block-remus: use correct way to get remus_image

[Wen Congyang]

We set remus_image in backup_read(). If we do flush
before the first read operation, remus_image will be
NULL. Pass image to remus via the callback td_open().

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 tools/blktap2/drivers/block-aio.c         | 6 ++++--
 tools/blktap2/drivers/block-cache.c       | 5 +++--
 tools/blktap2/drivers/block-log.c         | 5 +++--
 tools/blktap2/drivers/block-qcow.c        | 6 ++++--
 tools/blktap2/drivers/block-ram.c         | 6 ++++--
 tools/blktap2/drivers/block-remus.c       | 8 ++++----
 tools/blktap2/drivers/block-vhd.c         | 6 ++++--
 tools/blktap2/drivers/tapdisk-interface.c | 3 +--
 tools/blktap2/drivers/tapdisk.h           | 2 +-
 9 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/tools/blktap2/drivers/block-aio.c b/tools/blktap2/drivers/block-aio.c
index 1b560e5..27ba07d 100644
--- a/tools/blktap2/drivers/block-aio.c
+++ b/tools/blktap2/drivers/block-aio.c
@@ -40,6 +40,7 @@
 #include "tapdisk.h"
 #include "tapdisk-driver.h"
 #include "tapdisk-interface.h"
+#include "tapdisk-image.h"
 
 #define MAX_AIO_REQS         TAPDISK_DATA_REQUESTS
 
@@ -111,11 +112,12 @@ static int tdaio_get_image_info(int fd, td_disk_info_t *info)
 }
 
 /* Open the disk file and initialize aio state. */
-int tdaio_open(td_driver_t *driver, const char *name, td_flag_t flags,
-	       td_uuid_t uuid)
+int tdaio_open(td_driver_t *driver, td_image_t *image, td_uuid_t uuid)
 {
 	int i, fd, ret, o_flags;
 	struct tdaio_state *prv;
+	const char *name = image->name;
+	td_flag_t flags = image->flags;
 
 	ret = 0;
 	prv = (struct tdaio_state *)driver->data;
diff --git a/tools/blktap2/drivers/block-cache.c b/tools/blktap2/drivers/block-cache.c
index cd6ea6a..ff2c773 100644
--- a/tools/blktap2/drivers/block-cache.c
+++ b/tools/blktap2/drivers/block-cache.c
@@ -517,12 +517,13 @@ block_cache_put_request(block_cache_t *cache, block_cache_request_t *breq)
 }
 
 static int
-block_cache_open(td_driver_t *driver, const char *name, td_flag_t flags,
-		 td_uuid_t uuid)
+block_cache_open(td_driver_t *driver, td_image_t *image, td_uuid_t uuid)
 {
 	int i, err;
 	radix_tree_t *tree;
 	block_cache_t *cache;
+	const char *name = image->name;
+	td_flag_t flags = image->flags;
 
 	if (!td_flag_test(flags, TD_OPEN_RDONLY))
 		return -EINVAL;
diff --git a/tools/blktap2/drivers/block-log.c b/tools/blktap2/drivers/block-log.c
index 7b33b63..80351d3 100644
--- a/tools/blktap2/drivers/block-log.c
+++ b/tools/blktap2/drivers/block-log.c
@@ -585,11 +585,12 @@ static void ctl_request(event_id_t id, char mode, void *private)
 
 static int tdlog_close(td_driver_t*);
 
-static int tdlog_open(td_driver_t* driver, const char* name, td_flag_t flags,
-		      td_uuid_t uuid)
+static int tdlog_open(td_driver_t* driver, td_image_t *image, td_uuid_t uuid)
 {
   struct tdlog_state* s = (struct tdlog_state*)driver->data;
   int rc;
+  const char *name = image->name;
+  td_flag_t flags = image->flags;
 
   memset(s, 0, sizeof(*s));
 
diff --git a/tools/blktap2/drivers/block-qcow.c b/tools/blktap2/drivers/block-qcow.c
index 64dfafc..c63bd9d 100644
--- a/tools/blktap2/drivers/block-qcow.c
+++ b/tools/blktap2/drivers/block-qcow.c
@@ -45,6 +45,7 @@
 #include "qcow.h"
 #include "blk.h"
 #include "atomicio.h"
+#include "tapdisk-image.h"
 
 /* *BSD has no O_LARGEFILE */
 #ifndef O_LARGEFILE
@@ -865,14 +866,15 @@ out:
 }
 
 /* Open the disk file and initialize qcow state. */
-int tdqcow_open (td_driver_t *driver, const char *name, td_flag_t flags,
-		 td_uuid_t uuid)
+int tdqcow_open (td_driver_t *driver, td_image_t *image, td_uuid_t uuid)
 {
 	int fd, len, i, ret, size, o_flags;
 	td_disk_info_t *bs = &(driver->info);
 	struct tdqcow_state   *s  = (struct tdqcow_state *)driver->data;
 	QCowHeader header;
 	uint64_t final_cluster = 0;
+	const char *name = image->name;
+	td_flag_t flags = image->flags;
 
  	DPRINTF("QCOW: Opening %s\n", name);
 
diff --git a/tools/blktap2/drivers/block-ram.c b/tools/blktap2/drivers/block-ram.c
index b64a194..3e148ab 100644
--- a/tools/blktap2/drivers/block-ram.c
+++ b/tools/blktap2/drivers/block-ram.c
@@ -40,6 +40,7 @@
 #include "tapdisk.h"
 #include "tapdisk-driver.h"
 #include "tapdisk-interface.h"
+#include "tapdisk-image.h"
 
 char *img;
 long int   disksector_size;
@@ -108,13 +109,14 @@ static int get_image_info(int fd, td_disk_info_t *info)
 }
 
 /* Open the disk file and initialize ram state. */
-int tdram_open (td_driver_t *driver, const char *name, td_flag_t flags,
-		td_uuid_t uuid)
+int tdram_open (td_driver_t *driver, td_image_t *image, td_uuid_t uuid)
 {
 	char *p;
 	uint64_t size;
 	int i, fd, ret = 0, count = 0, o_flags;
 	struct tdram_state *prv = (struct tdram_state *)driver->data;
+	const char *name = image->name;
+	td_flag_t flags = image->flags;
 
 	connections++;
 
diff --git a/tools/blktap2/drivers/block-remus.c b/tools/blktap2/drivers/block-remus.c
index 504f6b4..23a908a 100644
--- a/tools/blktap2/drivers/block-remus.c
+++ b/tools/blktap2/drivers/block-remus.c
@@ -1152,8 +1152,6 @@ void backup_queue_read(td_driver_t *driver, td_request_t treq)
 {
 	struct tdremus_state *s = (struct tdremus_state *)driver->data;
 	int i;
-	if(!remus_image)
-		remus_image = treq.image;
 	
 	/* check if this read is queued in any currently ongoing flush */
 	if (ramdisk_read(&s->ramdisk, treq.sec, treq.secs, treq.buf)) {
@@ -1632,15 +1630,17 @@ static int ctl_register(struct tdremus_state *s)
 
 /* interface */
 
-static int tdremus_open(td_driver_t *driver, const char *name,
-			td_flag_t flags, td_uuid_t uuid)
+static int tdremus_open(td_driver_t *driver, td_image_t *image, td_uuid_t uuid)
 {
 	struct tdremus_state *s = (struct tdremus_state *)driver->data;
 	int rc;
+	const char *name = image->name;
+	td_flag_t flags = image->flags;
 
 	RPRINTF("opening %s\n", name);
 
 	device_vbd = tapdisk_server_get_vbd(uuid);
+	remus_image = image;
 
 	memset(s, 0, sizeof(*s));
 	s->server_fd.fd = -1;
diff --git a/tools/blktap2/drivers/block-vhd.c b/tools/blktap2/drivers/block-vhd.c
index 06e9c89..b20f724 100644
--- a/tools/blktap2/drivers/block-vhd.c
+++ b/tools/blktap2/drivers/block-vhd.c
@@ -59,6 +59,7 @@
 #include "tapdisk-driver.h"
 #include "tapdisk-interface.h"
 #include "tapdisk-disktype.h"
+#include "tapdisk-image.h"
 
 unsigned int SPB;
 
@@ -675,10 +676,11 @@ __vhd_open(td_driver_t *driver, const char *name, vhd_flag_t flags)
 }
 
 static int
-_vhd_open(td_driver_t *driver, const char *name, td_flag_t flags,
-	  td_uuid_t uuid)
+_vhd_open(td_driver_t *driver, td_image_t *image, td_uuid_t uuid)
 {
 	vhd_flag_t vhd_flags = 0;
+	const char *name = image->name;
+	td_flag_t flags = image->flags;
 
 	if (flags & TD_OPEN_RDONLY)
 		vhd_flags |= VHD_FLAG_OPEN_RDONLY;
diff --git a/tools/blktap2/drivers/tapdisk-interface.c b/tools/blktap2/drivers/tapdisk-interface.c
index 36b5393..a29de64 100644
--- a/tools/blktap2/drivers/tapdisk-interface.c
+++ b/tools/blktap2/drivers/tapdisk-interface.c
@@ -79,8 +79,7 @@ __td_open(td_image_t *image, td_disk_info_t *info)
 	}
 
 	if (!td_flag_test(driver->state, TD_DRIVER_OPEN)) {
-		err = driver->ops->td_open(driver, image->name, image->flags,
-					   vbd->uuid);
+		err = driver->ops->td_open(driver, image, vbd->uuid);
 		if (err) {
 			if (!image->driver)
 				tapdisk_driver_free(driver);
diff --git a/tools/blktap2/drivers/tapdisk.h b/tools/blktap2/drivers/tapdisk.h
index 459eaec..3c3b51d 100644
--- a/tools/blktap2/drivers/tapdisk.h
+++ b/tools/blktap2/drivers/tapdisk.h
@@ -157,7 +157,7 @@ struct tap_disk {
 	const char                  *disk_type;
 	td_flag_t                    flags;
 	int                          private_data_size;
-	int (*td_open)               (td_driver_t *, const char *, td_flag_t, td_uuid_t);
+	int (*td_open)               (td_driver_t *, td_image_t *, td_uuid_t);
 	int (*td_close)              (td_driver_t *);
 	int (*td_get_parent_id)      (td_driver_t *, td_disk_id_t *);
 	int (*td_validate_parent)    (td_driver_t *, td_driver_t *, td_flag_t);
-- 
1.9.3

[RFC Patch v3 12/18] block-remus: fix bug in tdremus_close()

[Wen Congyang]

We close ctl_fd.fd, but we don't unregister ctl_fd.id. It will
cause select() return fails, and the user cannot talk with
tapdisk2.

This patch also does some cleanup.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 tools/blktap2/drivers/block-remus.c | 90 ++++++++++++++++++++++---------------
 1 file changed, 53 insertions(+), 37 deletions(-)

diff --git a/tools/blktap2/drivers/block-remus.c b/tools/blktap2/drivers/block-remus.c
index 23a908a..55363a3 100644
--- a/tools/blktap2/drivers/block-remus.c
+++ b/tools/blktap2/drivers/block-remus.c
@@ -151,9 +151,6 @@ typedef struct poll_fd {
 } poll_fd_t;
 
 struct tdremus_state {
-//  struct tap_disk* driver;
-	void* driver_data;
-
   /* XXX: this is needed so that the server can perform operations on
    * the driver from the stream_fd event handler. fix this. */
 	td_driver_t *tdremus_driver;
@@ -731,12 +728,26 @@ static int mwrite(int fd, void* buf, size_t len)
 
 static void inline close_stream_fd(struct tdremus_state *s)
 {
+	if (s->stream_fd.fd < 0)
+		return;
+
 	/* XXX: -2 is magic. replace with macro perhaps? */
 	tapdisk_server_unregister_event(s->stream_fd.id);
 	close(s->stream_fd.fd);
 	s->stream_fd.fd = -2;
 }
 
+static void close_server_fd(struct tdremus_state *s)
+{
+	if (s->server_fd.fd < 0)
+		return;
+
+	tapdisk_server_unregister_event(s->server_fd.id);
+	s->server_fd.id = -1;
+	close(s->stream_fd.fd);
+	s->stream_fd.fd = -1;
+}
+
 /* primary functions */
 static void remus_client_event(event_id_t, char mode, void *private);
 static void remus_connect_event(event_id_t id, char mode, void *private);
@@ -1347,12 +1358,7 @@ static int unprotected_start(td_driver_t *driver)
 	/* close the server socket */
 	close_stream_fd(s);
 
-	/* unregister the replication stream */
-	tapdisk_server_unregister_event(s->server_fd.id);
-
-	/* close the replication stream */
-	close(s->server_fd.fd);
-	s->server_fd.fd = -1;
+	close_server_fd(s);
 
 	/* install the unprotected read/write handlers */
 	tapdisk_remus.td_queue_read = unprotected_queue_read;
@@ -1553,27 +1559,27 @@ static int ctl_open(td_driver_t *driver, const char* name)
 			s->ctl_path[i] = '_';
 	}
 	if (asprintf(&s->msg_path, "%s.msg", s->ctl_path) < 0)
-		goto err_ctlfifo;
+		goto err_setmsgfifo;
 
 	if (mkfifo(s->ctl_path, S_IRWXU|S_IRWXG|S_IRWXO) && errno != EEXIST) {
 		RPRINTF("error creating control FIFO %s: %d\n", s->ctl_path, errno);
-		goto err_msgfifo;
+		goto err_mkctlfifo;
 	}
 
 	if (mkfifo(s->msg_path, S_IRWXU|S_IRWXG|S_IRWXO) && errno != EEXIST) {
 		RPRINTF("error creating message FIFO %s: %d\n", s->msg_path, errno);
-		goto err_msgfifo;
+		goto err_mkmsgfifo;
 	}
 
 	/* RDWR so that fd doesn't block select when no writer is present */
 	if ((s->ctl_fd.fd = open(s->ctl_path, O_RDWR)) < 0) {
 		RPRINTF("error opening control FIFO %s: %d\n", s->ctl_path, errno);
-		goto err_msgfifo;
+		goto err_openctlfifo;
 	}
 
 	if ((s->msg_fd.fd = open(s->msg_path, O_RDWR)) < 0) {
 		RPRINTF("error opening message FIFO %s: %d\n", s->msg_path, errno);
-		goto err_openctlfifo;
+		goto err_openmsgfifo;
 	}
 
 	RPRINTF("control FIFO %s\n", s->ctl_path);
@@ -1581,36 +1587,45 @@ static int ctl_open(td_driver_t *driver, const char* name)
 
 	return 0;
 
- err_openctlfifo:
+err_openmsgfifo:
 	close(s->ctl_fd.fd);
- err_msgfifo:
+	s->ctl_fd.fd = -1;
+err_openctlfifo:
+	unlink(s->ctl_path);
+err_mkmsgfifo:
+	unlink(s->msg_path);
+err_mkctlfifo:
 	free(s->msg_path);
 	s->msg_path = NULL;
- err_ctlfifo:
+err_setmsgfifo:
 	free(s->ctl_path);
 	s->ctl_path = NULL;
 	return -1;
 }
 
-static void ctl_close(td_driver_t *driver)
+static void ctl_close(struct tdremus_state *s)
 {
-	struct tdremus_state *s = (struct tdremus_state *)driver->data;
-
-	/* TODO: close *all* connections */
-
-	if(s->ctl_fd.fd)
+	if(s->ctl_fd.fd) {
 		close(s->ctl_fd.fd);
+		s->ctl_fd.fd = -1;
+	}
 
 	if (s->ctl_path) {
 		unlink(s->ctl_path);
 		free(s->ctl_path);
 		s->ctl_path = NULL;
 	}
+
 	if (s->msg_path) {
 		unlink(s->msg_path);
 		free(s->msg_path);
 		s->msg_path = NULL;
 	}
+
+	if (s->msg_fd.fd) {
+		close(s->msg_fd.fd);
+		s->msg_fd.fd = -1;
+	}
 }
 
 static int ctl_register(struct tdremus_state *s)
@@ -1628,6 +1643,16 @@ static int ctl_register(struct tdremus_state *s)
 	return 0;
 }
 
+static void ctl_unregister(struct tdremus_state *s)
+{
+	RPRINTF("unregistering ctl fifo\n");
+
+	if (s->ctl_fd.id >= 0) {
+		tapdisk_server_unregister_event(s->ctl_fd.id);
+		s->ctl_fd.id = -1;
+	}
+}
+
 /* interface */
 
 static int tdremus_open(td_driver_t *driver, td_image_t *image, td_uuid_t uuid)
@@ -1658,13 +1683,12 @@ static int tdremus_open(td_driver_t *driver, td_image_t *image, td_uuid_t uuid)
 
 	if ((rc = ctl_open(driver, name))) {
 		RPRINTF("error setting up control channel\n");
-		free(s->driver_data);
 		return rc;
 	}
 
 	if ((rc = ctl_register(s))) {
 		RPRINTF("error registering control channel\n");
-		free(s->driver_data);
+		ctl_close(s);
 		return rc;
 	}
 
@@ -1687,19 +1711,11 @@ static int tdremus_close(td_driver_t *driver)
 	RPRINTF("closing\n");
 	if (s->ramdisk.inprogress)
 		hashtable_destroy(s->ramdisk.inprogress, 0);
-	
-	if (s->driver_data) {
-		free(s->driver_data);
-		s->driver_data = NULL;
-	}
-	if (s->server_fd.fd >= 0) {
-		close(s->server_fd.fd);
-		s->server_fd.fd = -1;
-	}
-	if (s->stream_fd.fd >= 0)
-		close_stream_fd(s);
 
-	ctl_close(driver);
+	close_server_fd(s);
+	close_stream_fd(s);
+	ctl_unregister(s);
+	ctl_close(s);
 
 	return 0;
 }
-- 
1.9.3

[RFC Patch v3 13/18] don't call client_flush() when switching to unprotected mode

[Wen Congyang]

If we switch the mode from primary to unprotected, the connection
between primary and backup cannot be used.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 tools/blktap2/drivers/block-remus.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/tools/blktap2/drivers/block-remus.c b/tools/blktap2/drivers/block-remus.c
index 55363a3..d358b44 100644
--- a/tools/blktap2/drivers/block-remus.c
+++ b/tools/blktap2/drivers/block-remus.c
@@ -892,6 +892,7 @@ static void primary_queue_write(td_driver_t *driver, td_request_t treq)
 }
 
 
+/* It is called when the user writes "flush" to control file */
 static int client_flush(td_driver_t *driver)
 {
 	struct tdremus_state *s = (struct tdremus_state *)driver->data;
@@ -902,7 +903,8 @@ static int client_flush(td_driver_t *driver)
 		/* connection not yet established, nothing to flush */
 		return 0;
 
-	if (mwrite(s->stream_fd.fd, TDREMUS_COMMIT, strlen(TDREMUS_COMMIT)) < 0) {
+	if (mwrite(s->stream_fd.fd, TDREMUS_COMMIT,
+	    strlen(TDREMUS_COMMIT)) < 0) {
 		RPRINTF("error flushing output");
 		close_stream_fd(s);
 		return -1;
@@ -931,7 +933,6 @@ static int primary_start(td_driver_t *driver)
 
 	tapdisk_remus.td_queue_read = primary_queue_read;
 	tapdisk_remus.td_queue_write = primary_queue_write;
-	s->queue_flush = client_flush;
 
 	s->stream_fd.fd = -1;
 	s->stream_fd.id = -1;
@@ -1510,16 +1511,23 @@ static void ctl_request(event_id_t id, char mode, void *private)
 		return;
 	}
 
-	/* TODO: need to get driver somehow */
 	msg[rc] = '\0';
-	if (!strncmp(msg, "flush", 5)) {
-		if (s->queue_flush)
-			if ((rc = s->queue_flush(driver))) {
-				RPRINTF("error passing flush request to backup");
-				ctl_respond(s, TDREMUS_FAIL);
-			}
-	} else {
+	if (strncmp(msg, "flush", 5)) {
 		RPRINTF("unknown command: %s\n", msg);
+		ctl_respond(s, TDREMUS_FAIL);
+		return;
+	}
+
+	if (s->mode != mode_primary) {
+		RPRINTF("We are not in primary mode\n");
+		ctl_respond(s, TDREMUS_FAIL);
+		return;
+	}
+
+	rc = client_flush(driver);
+	if (rc) {
+		RPRINTF("error passing flush request to backup");
+		ctl_respond(s, TDREMUS_FAIL);
 	}
 }
 
-- 
1.9.3

[RFC Patch v3 14/18] pass correct file to qemu if we use blktap2

[Wen Congyang]

If we use blktap2, the correct file should be blktap device
not the pdev_path.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 tools/libxl/libxl_dm.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 103cbca..fbc82fd 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -718,6 +718,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                 libxl__device_disk_dev_number(disks[i].vdev, &disk, &part);
             const char *format = qemu_disk_format_string(disks[i].format);
             char *drive;
+            const char *pdev_path;
 
             if (dev_number == -1) {
                 LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "unable to determine"
@@ -747,6 +748,12 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                     continue;
                 }
 
+                if (disks[i].backend == LIBXL_DISK_BACKEND_TAP)
+                    pdev_path = libxl__blktap_devpath(gc, disks[i].pdev_path,
+                                                      disks[i].format);
+                else
+                    pdev_path = disks[i].pdev_path;
+
                 /*
                  * Explicit sd disks are passed through as is.
                  *
@@ -756,11 +763,11 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                 if (strncmp(disks[i].vdev, "sd", 2) == 0)
                     drive = libxl__sprintf
                         (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,cache=writeback",
-                         disks[i].pdev_path, disk, format);
+                         pdev_path, disk, format);
                 else if (disk < 4)
                     drive = libxl__sprintf
                         (gc, "file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback",
-                         disks[i].pdev_path, disk, format);
+                         pdev_path, disk, format);
                 else
                     continue; /* Do not emulate this disk */
             }
-- 
1.9.3

[RFC Patch v3 15/18] support blktap remus in xl

[Wen Congyang]

With this patch, we can use blktap remus like this:
disk = [ 'format=remus,devtype=disk,access=w,vdev=hda,backendtype=tap,target=192.168.3.1:9000|aio:filename' ]

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 tools/libxl/libxl_blktap2.c   | 33 +++++++++++++++++++++++++++++++++
 tools/libxl/libxl_device.c    |  4 +++-
 tools/libxl/libxl_dm.c        |  8 ++++++++
 tools/libxl/libxl_internal.h  |  4 ++++
 tools/libxl/libxl_noblktap2.c |  6 ++++++
 tools/libxl/libxl_types.idl   |  1 +
 tools/libxl/libxlu_disk_l.l   |  1 +
 7 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_blktap2.c b/tools/libxl/libxl_blktap2.c
index 2053403..7bbdfc8 100644
--- a/tools/libxl/libxl_blktap2.c
+++ b/tools/libxl/libxl_blktap2.c
@@ -32,6 +32,10 @@ char *libxl__blktap_devpath(libxl__gc *gc,
     tap_list_t tap;
     int err;
 
+    if (format == LIBXL_DISK_FORMAT_REMUS)
+        if (libxl__blktap_get_real_format(gc, disk, format) < 0)
+            return NULL;
+
     type = libxl__device_disk_string_of_format(format);
     err = tap_ctl_find(type, disk, &tap);
     if (err == 0) {
@@ -84,6 +88,35 @@ int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *params)
     return 0;
 }
 
+libxl_disk_format libxl__blktap_get_real_format(libxl__gc *gc,
+                                                const char *disk,
+                                                libxl_disk_format format)
+{
+    const char *type;
+
+    if (format != LIBXL_DISK_FORMAT_REMUS)
+        return format;
+
+    /* The format of disk: ip:port|xxx:file */
+    type = strchr(disk, '|');
+    if (!type) {
+        LOG(ERROR, "Unable to parse params %s", disk);
+        return ERROR_FAIL;
+    }
+
+    type++;
+
+    /* libxl only supports aio/vhd(see disk_try_backend()) */
+    if (!strncmp(type, "aio:", 4))
+        return LIBXL_DISK_FORMAT_RAW;
+    else if (!strncmp(type, "vhd:", 4))
+        return LIBXL_DISK_FORMAT_VHD;
+
+    LOG(ERROR, "Unsupported format: %s", type);
+
+    return ERROR_FAIL;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index f8a2e1b..901d9e2 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -211,7 +211,8 @@ static int disk_try_backend(disk_try_backend_args *a,
             return 0;
         }
         if (!(a->disk->format == LIBXL_DISK_FORMAT_RAW ||
-              a->disk->format == LIBXL_DISK_FORMAT_VHD)) {
+              a->disk->format == LIBXL_DISK_FORMAT_VHD ||
+              a->disk->format == LIBXL_DISK_FORMAT_REMUS)) {
             goto bad_format;
         }
         return backend;
@@ -295,6 +296,7 @@ char *libxl__device_disk_string_of_format(libxl_disk_format format)
         case LIBXL_DISK_FORMAT_VHD: return "vhd";
         case LIBXL_DISK_FORMAT_RAW:
         case LIBXL_DISK_FORMAT_EMPTY: return "aio";
+        case LIBXL_DISK_FORMAT_REMUS: return "remus";
         default: return NULL;
     }
 }
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index fbc82fd..bf11973 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -719,6 +719,7 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
             const char *format = qemu_disk_format_string(disks[i].format);
             char *drive;
             const char *pdev_path;
+            libxl_disk_format real_format;
 
             if (dev_number == -1) {
                 LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "unable to determine"
@@ -726,6 +727,13 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                 continue;
             }
 
+            if (disks[i].format == LIBXL_DISK_FORMAT_REMUS) {
+                real_format = libxl__blktap_get_real_format(gc,
+                                                            disks[i].pdev_path,
+                                                            disks[i].format);
+                format = qemu_disk_format_string(real_format);
+            }
+
             if (disks[i].is_cdrom) {
                 if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY)
                     drive = libxl__sprintf
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 04c9378..ca77451 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1531,6 +1531,10 @@ _hidden char *libxl__blktap_devpath(libxl__gc *gc,
  */
 _hidden int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *params);
 
+_hidden libxl_disk_format libxl__blktap_get_real_format(libxl__gc *gc,
+                                                        const char *disk,
+                                                        libxl_disk_format format);
+
 _hidden int libxl__device_from_disk(libxl__gc *gc, uint32_t domid,
                                    libxl_device_disk *disk,
                                    libxl__device *device);
diff --git a/tools/libxl/libxl_noblktap2.c b/tools/libxl/libxl_noblktap2.c
index 5a86ed1..38696ec 100644
--- a/tools/libxl/libxl_noblktap2.c
+++ b/tools/libxl/libxl_noblktap2.c
@@ -33,6 +33,12 @@ int libxl__device_destroy_tapdisk(libxl__gc *gc, const char *params)
     return 0;
 }
 
+libxl_disk_format libxl__blktap_get_real_format(const char *disk,
+                                                libxl_disk_format format)
+{
+    return format;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 931c9e9..04a29b3 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -85,6 +85,7 @@ libxl_disk_format = Enumeration("disk_format", [
     (3, "VHD"),
     (4, "RAW"),
     (5, "EMPTY"),
+    (6, "REMUS"),
     ])
 
 libxl_disk_backend = Enumeration("disk_backend", [
diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
index 1a5deb5..d9ff8a1 100644
--- a/tools/libxl/libxlu_disk_l.l
+++ b/tools/libxl/libxlu_disk_l.l
@@ -102,6 +102,7 @@ static void setformat(DiskParseContext *dpc, const char *str) {
     else if (!strcmp(str,"qcow2"))  DSET(dpc,format,FORMAT,str,QCOW2);
     else if (!strcmp(str,"vhd"))    DSET(dpc,format,FORMAT,str,VHD);
     else if (!strcmp(str,"empty"))  DSET(dpc,format,FORMAT,str,EMPTY);
+    else if (!strcmp(str,"remus"))  DSET(dpc,format,FORMAT,str,REMUS);
     else xlu__disk_err(dpc,str,"unknown value for format");
 }
 
-- 
1.9.3

[RFC Patch v3 16/18] update libxl__device_disk_from_xs_be() to support blktap device

[Wen Congyang]

If the disk backend is blktap device, we store "format:pdev_path"
in tapdisk-params, and store "phy" in type. So use tapdisk-params
to set libxl_device_disk instead of params and type.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
---
 tools/libxl/libxl.c       | 43 +++++++++++++++++++++++++++++++++++++++++--
 tools/libxl/libxl_utils.c | 21 +++++++++++++++++++++
 tools/libxl/libxl_utils.h |  1 +
 3 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2ae5fca..09cefab 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2363,6 +2363,46 @@ static int libxl__device_disk_from_xs_be(libxl__gc *gc,
         goto cleanup;
     }
 
+    disk->format = LIBXL_DISK_FORMAT_UNKNOWN;
+
+    /* "tapdisk-params" is only for tapdisk */
+    tmp = xs_read(ctx->xsh, XBT_NULL,
+                  libxl__sprintf(gc, "%s/tapdisk-params", be_path), &len);
+    if (tmp) {
+        char *pdev_path;
+        /* tmp is "format:pdev_path" */
+        pdev_path = strchr(tmp, ':');
+        if (!pdev_path) {
+            LOG(ERROR, "corrupted tapdisk-params: \"%s\"\n", tmp);
+            free(tmp);
+            goto cleanup;
+        }
+        disk->pdev_path = strdup(pdev_path + 1);
+        *pdev_path = '\0';
+        rc = libxl_string_to_format(ctx, tmp, &disk->format);
+        if (rc) {
+            LOG(ERROR, "unknown disk format: %s\n", tmp);
+            free(tmp);
+            goto cleanup;
+        }
+        if (disk->format != LIBXL_DISK_FORMAT_VHD &&
+            disk->format != LIBXL_DISK_FORMAT_RAW &&
+            disk->format != LIBXL_DISK_FORMAT_REMUS) {
+            LOG(ERROR, "unsupported tapdisk format: %s\n", tmp);
+            free(tmp);
+            goto cleanup;
+        }
+        free(tmp);
+
+        /*
+         * The backend is tapdisk, so we store tapdev in params, and
+         * phy in type(see device_disk_add())
+         */
+        disk->backend = LIBXL_DISK_BACKEND_TAP;
+
+        goto skip_type;
+    }
+
     /* "params" may not be present; but everything else must be. */
     tmp = xs_read(ctx->xsh, XBT_NULL,
                   libxl__sprintf(gc, "%s/params", be_path), &len);
@@ -2382,6 +2422,7 @@ static int libxl__device_disk_from_xs_be(libxl__gc *gc,
     }
     libxl_string_to_backend(ctx, tmp, &(disk->backend));
 
+skip_type:
     disk->vdev = xs_read(ctx->xsh, XBT_NULL,
                          libxl__sprintf(gc, "%s/dev", be_path), &len);
     if (!disk->vdev) {
@@ -2415,8 +2456,6 @@ static int libxl__device_disk_from_xs_be(libxl__gc *gc,
     }
     disk->is_cdrom = !strcmp(tmp, "cdrom");
 
-    disk->format = LIBXL_DISK_FORMAT_UNKNOWN;
-
     return 0;
 cleanup:
     libxl_device_disk_dispose(disk);
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 58df4f3..ad7cf92 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -319,6 +319,27 @@ out:
     return rc;
 }
 
+int libxl_string_to_format(libxl_ctx *ctx, char *s, libxl_disk_format *format)
+{
+    int rc = 0;
+    if (!strcmp(s, "aio")) {
+        *format = LIBXL_DISK_FORMAT_RAW;
+    } else if (!strcmp(s, "qcow")) {
+        *format = LIBXL_DISK_FORMAT_QCOW;
+    } else if (!strcmp(s, "qcow2")) {
+        *format = LIBXL_DISK_FORMAT_QCOW2;
+    } else if (!strcmp(s, "vhd")) {
+        *format = LIBXL_DISK_FORMAT_VHD;
+    } else if (!strcmp(s, "remus")) {
+        *format = LIBXL_DISK_FORMAT_REMUS;
+    } else {
+        *format = LIBXL_DISK_FORMAT_UNKNOWN;
+        rc = ERROR_FAIL;
+    }
+
+    return rc;
+}
+
 int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
                              void **data_r, int *datalen_r) {
     GC_INIT(ctx);
diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
index 117b229..9178836 100644
--- a/tools/libxl/libxl_utils.h
+++ b/tools/libxl/libxl_utils.h
@@ -33,6 +33,7 @@ int libxl_get_stubdom_id(libxl_ctx *ctx, int guest_domid);
 int libxl_is_stubdom(libxl_ctx *ctx, uint32_t domid, uint32_t *target_domid);
 int libxl_create_logfile(libxl_ctx *ctx, const char *name, char **full_name);
 int libxl_string_to_backend(libxl_ctx *ctx, char *s, libxl_disk_backend *backend);
+int libxl_string_to_format(libxl_ctx *ctx, char *s, libxl_disk_format *format);
 
 int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
                              void **data_r, int *datalen_r);
-- 
1.9.3

[RFC Patch v3 17/18] read nictype from xenstore

[Wen Congyang]

We need to use nictype to get default vifname.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
 tools/libxl/libxl.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 09cefab..33590e5 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3153,7 +3153,13 @@ static int libxl__device_nic_from_xs_be(libxl__gc *gc,
     nic->script = READ_BACKEND(NOGC, "script");
 
     /* vif_ioemu nics use the same xenstore entries as vif interfaces */
-    nic->nictype = LIBXL_NIC_TYPE_VIF;
+    tmp = READ_BACKEND(gc, "type");
+    if (tmp) {
+        rc = libxl_nic_type_from_string(tmp, &nic->nictype);
+        if (rc) goto out;
+    } else {
+        nic->nictype = LIBXL_NIC_TYPE_VIF;
+    }
     nic->model = NULL; /* XXX Only for TYPE_IOEMU */
     nic->ifname = NULL; /* XXX Only for TYPE_IOEMU */
 
-- 
1.9.3

[RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.

[Wen Congyang]

In colo mode, secondary vm is running, so VM_ENTRY_INTR_INFO may
valid before restoring vmcs. If there is no pending event after
restoring vm, we should clear it.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

Also clear pending software exceptions.
Copy the fix to SVM as well.

Signed-off-by: Tim Deegan <tim@xen.org>

vmx part:
Cc: Jun Nakajima <jun.nakajima@intel.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>

svm part:
Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/arch/x86/hvm/svm/svm.c | 15 ++++++++-------
 xen/arch/x86/hvm/vmx/vmx.c | 25 ++++++++++++-------------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index b5188e6..053e511 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -321,16 +321,17 @@ static int svm_vmcb_restore(struct vcpu *v, struct hvm_hw_cpu *c)
         vmcb_set_h_cr3(vmcb, pagetable_get_paddr(p2m_get_pagetable(p2m)));
     }
 
-    if ( c->pending_valid ) 
+    if ( c->pending_valid &&
+         hvm_event_needs_reinjection(c->pending_type, c->pending_vector) )
     {
         gdprintk(XENLOG_INFO, "Re-injecting %#"PRIx32", %#"PRIx32"\n",
                  c->pending_event, c->error_code);
-
-        if ( hvm_event_needs_reinjection(c->pending_type, c->pending_vector) )
-        {
-            vmcb->eventinj.bytes = c->pending_event;
-            vmcb->eventinj.fields.errorcode = c->error_code;
-        }
+        vmcb->eventinj.bytes = c->pending_event;
+        vmcb->eventinj.fields.errorcode = c->error_code;
+    }
+    else
+    {
+        vmcb->eventinj.bytes = 0;
     }
 
     vmcb->cleanbits.bytes = 0;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 26b1ad5..6ebc945 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -510,23 +510,22 @@ static int vmx_vmcs_restore(struct vcpu *v, struct hvm_hw_cpu *c)
 
     __vmwrite(GUEST_DR7, c->dr7);
 
-    vmx_vmcs_exit(v);
-
-    paging_update_paging_modes(v);
-
-    if ( c->pending_valid )
+    if ( c->pending_valid &&
+         hvm_event_needs_reinjection(c->pending_type, c->pending_vector) )
     {
         gdprintk(XENLOG_INFO, "Re-injecting %#"PRIx32", %#"PRIx32"\n",
                  c->pending_event, c->error_code);
-
-        if ( hvm_event_needs_reinjection(c->pending_type, c->pending_vector) )
-        {
-            vmx_vmcs_enter(v);
-            __vmwrite(VM_ENTRY_INTR_INFO, c->pending_event);
-            __vmwrite(VM_ENTRY_EXCEPTION_ERROR_CODE, c->error_code);
-            vmx_vmcs_exit(v);
-        }
+        __vmwrite(VM_ENTRY_INTR_INFO, c->pending_event);
+        __vmwrite(VM_ENTRY_EXCEPTION_ERROR_CODE, c->error_code);
+    }
+    else
+    {
+        __vmwrite(VM_ENTRY_INTR_INFO, 0);
+        __vmwrite(VM_ENTRY_EXCEPTION_ERROR_CODE, 0);
     }
+    vmx_vmcs_exit(v);
+
+    paging_update_paging_modes(v);
 
     return 0;
 }
-- 
1.9.3

Re: [RFC Patch v3 03/18] don't zero out ioreq page

[Paul Durrant]

> -----Original Message-----
> From: Wen Congyang [mailto:wency@cn.fujitsu.com]
> Sent: 05 September 2014 10:11
> To: xen devel
> Cc: Ian Jackson; Ian Campbell; Eddie Dong; Jiang Yunhong; Lai Jiangshan; Yang
> Hongyang; Wen Congyang; Paul Durrant
> Subject: [RFC Patch v3 03/18] don't zero out ioreq page
> 
> ioreq page may contain some pending I/O requests, and we need to
> handle the pending I/O req after migration.
> 
> TODO:
> 1. update qemu to handle the pending I/O req
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Cc: Paul Durrant <paul.durrant@citrix.com>
> ---
>  tools/libxc/xc_domain_restore.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tools/libxc/xc_domain_restore.c
> b/tools/libxc/xc_domain_restore.c
> index fb4ddfc..21a6177 100644
> --- a/tools/libxc/xc_domain_restore.c
> +++ b/tools/libxc/xc_domain_restore.c
> @@ -2310,8 +2310,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
> uint32_t dom,
>      }
> 
>      /* These comms pages need to be zeroed at the start of day */
> -    if ( xc_clear_domain_page(xch, dom, tailbuf.u.hvm.magicpfns[0]) ||
> -         xc_clear_domain_page(xch, dom, tailbuf.u.hvm.magicpfns[1]) ||
> +    if ( xc_clear_domain_page(xch, dom, tailbuf.u.hvm.magicpfns[1]) ||
>           xc_clear_domain_page(xch, dom, tailbuf.u.hvm.magicpfns[2]) )

If we're not nuking pfn[0] then we probably shouldn't nuke pfn[1] either (buffererd ioreq). Does qemu need any modification? I don't think it makes any assumptions about the initial state of ioreqs so all you may have to do is make sure each vcpu event channel is kicked on resume (which is harmless even if there's no pending ioreq... qemu will just ignore it and wait again).

  Paul

>      {
>          PERROR("error zeroing magic pages");
> --
> 1.9.3

Re: [RFC Patch v3 03/18] don't zero out ioreq page

[Wen Congyang]

At 09/05/2014 05:25 PM, Paul Durrant Write:
>> -----Original Message-----
>> From: Wen Congyang [mailto:wency@cn.fujitsu.com]
>> Sent: 05 September 2014 10:11
>> To: xen devel
>> Cc: Ian Jackson; Ian Campbell; Eddie Dong; Jiang Yunhong; Lai Jiangshan; Yang
>> Hongyang; Wen Congyang; Paul Durrant
>> Subject: [RFC Patch v3 03/18] don't zero out ioreq page
>>
>> ioreq page may contain some pending I/O requests, and we need to
>> handle the pending I/O req after migration.
>>
>> TODO:
>> 1. update qemu to handle the pending I/O req
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Cc: Paul Durrant <paul.durrant@citrix.com>
>> ---
>>  tools/libxc/xc_domain_restore.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/tools/libxc/xc_domain_restore.c
>> b/tools/libxc/xc_domain_restore.c
>> index fb4ddfc..21a6177 100644
>> --- a/tools/libxc/xc_domain_restore.c
>> +++ b/tools/libxc/xc_domain_restore.c
>> @@ -2310,8 +2310,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
>> uint32_t dom,
>>      }
>>
>>      /* These comms pages need to be zeroed at the start of day */
>> -    if ( xc_clear_domain_page(xch, dom, tailbuf.u.hvm.magicpfns[0]) ||
>> -         xc_clear_domain_page(xch, dom, tailbuf.u.hvm.magicpfns[1]) ||
>> +    if ( xc_clear_domain_page(xch, dom, tailbuf.u.hvm.magicpfns[1]) ||
>>           xc_clear_domain_page(xch, dom, tailbuf.u.hvm.magicpfns[2]) )
> 
> If we're not nuking pfn[0] then we probably shouldn't nuke pfn[1] either (buffererd ioreq).
> 

IIRC, in my early test, if we clear pfn[1], secondary vm doesn't response(I use vnc connect to secondary vm).
But I test it again, secondary vm doesn't response even if we don't clear pfn[1]. I will clear pfn[1] in
the next version.


> Does qemu need any modification? I don't think it makes any assumptions about the initial state of ioreqs so all you may have to do is make sure each vcpu event channel is kicked on resume (which is harmless even if there's no pending ioreq... qemu will just ignore it and wait again).

Do you mean hypervisor kickes each vcpu event channel on resume? I will try it.

Thanks
Wen Congyang

> 
>   Paul
> 
>>      {
>>          PERROR("error zeroing magic pages");
>> --
>> 1.9.3
> 
> .
> 

Re: [RFC Patch v3 03/18] don't zero out ioreq page

[Paul Durrant]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Wen Congyang
> Sent: 05 September 2014 10:33
> To: Paul Durrant; xen devel
> Cc: Ian Campbell; Jiang Yunhong; Eddie Dong; Ian Jackson; Yang Hongyang; Lai
> Jiangshan
> Subject: Re: [Xen-devel] [RFC Patch v3 03/18] don't zero out ioreq page
> 
> At 09/05/2014 05:25 PM, Paul Durrant Write:
> >> -----Original Message-----
> >> From: Wen Congyang [mailto:wency@cn.fujitsu.com]
> >> Sent: 05 September 2014 10:11
> >> To: xen devel
> >> Cc: Ian Jackson; Ian Campbell; Eddie Dong; Jiang Yunhong; Lai Jiangshan;
> Yang
> >> Hongyang; Wen Congyang; Paul Durrant
> >> Subject: [RFC Patch v3 03/18] don't zero out ioreq page
> >>
> >> ioreq page may contain some pending I/O requests, and we need to
> >> handle the pending I/O req after migration.
> >>
> >> TODO:
> >> 1. update qemu to handle the pending I/O req
> >>
> >> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> >> Cc: Paul Durrant <paul.durrant@citrix.com>
> >> ---
> >>  tools/libxc/xc_domain_restore.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/tools/libxc/xc_domain_restore.c
> >> b/tools/libxc/xc_domain_restore.c
> >> index fb4ddfc..21a6177 100644
> >> --- a/tools/libxc/xc_domain_restore.c
> >> +++ b/tools/libxc/xc_domain_restore.c
> >> @@ -2310,8 +2310,7 @@ int xc_domain_restore(xc_interface *xch, int
> io_fd,
> >> uint32_t dom,
> >>      }
> >>
> >>      /* These comms pages need to be zeroed at the start of day */
> >> -    if ( xc_clear_domain_page(xch, dom, tailbuf.u.hvm.magicpfns[0]) ||
> >> -         xc_clear_domain_page(xch, dom, tailbuf.u.hvm.magicpfns[1]) ||
> >> +    if ( xc_clear_domain_page(xch, dom, tailbuf.u.hvm.magicpfns[1]) ||
> >>           xc_clear_domain_page(xch, dom, tailbuf.u.hvm.magicpfns[2]) )
> >
> > If we're not nuking pfn[0] then we probably shouldn't nuke pfn[1] either
> (buffererd ioreq).
> >
> 
> IIRC, in my early test, if we clear pfn[1], secondary vm doesn't response(I use
> vnc connect to secondary vm).
> But I test it again, secondary vm doesn't response even if we don't clear
> pfn[1]. I will clear pfn[1] in
> the next version.
> 
> 
> > Does qemu need any modification? I don't think it makes any assumptions
> about the initial state of ioreqs so all you may have to do is make sure each
> vcpu event channel is kicked on resume (which is harmless even if there's no
> pending ioreq... qemu will just ignore it and wait again).
> 
> Do you mean hypervisor kickes each vcpu event channel on resume? I will try
> it.

Yes. AFAICT QEMU won't check an ioreq structure's state unless the event for that vcpu is pending. As I said, it's harmless to send the event if the ioreq is not pending, but *not* sending the event if it is pending will lead to things wedging up.

  Paul

> 
> Thanks
> Wen Congyang
> 
> >
> >   Paul
> >
> >>      {
> >>          PERROR("error zeroing magic pages");
> >> --
> >> 1.9.3
> >
> > .
> >
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [RFC Patch v3 03/18] don't zero out ioreq page

[David Vrabel]

On 05/09/14 10:10, Wen Congyang wrote:
> ioreq page may contain some pending I/O requests, and we need to
> handle the pending I/O req after migration.
> 
> TODO:
> 1. update qemu to handle the pending I/O req

Under what conditions can there be pending I/O?  A domain suspend should
not complete until any I/O accesses are complete.

Suspending a domain that's part-way through an instruction seems mad to
me -- how are you supposed to capture this sort of state?

David

Re: [RFC Patch v3 01/18] copy the correct page to memory

[Ian Campbell]

On Fri, 2014-09-05 at 17:10 +0800, Wen Congyang wrote:
> From: Hong Tao <bobby.hong@huawei.com>
> 
> apply_batch() only handles MAX_BATCH_SIZE pages at one time. If
> there is some bogus/unmapped/allocate-only/broken page, we will
> skip it. So when we call apply_batch() again, the first page's
> index is curbatch - invalid_pages. invalid_pages stores the number
> of bogus/unmapped/allocate-only/broken pages we have found.
> 
> In many cases, invalid_pages is 0, so we don't catch this error.
> 
> Signed-off-by: Hong Tao <bobby.hong@huawei.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Hopefully all of this stuff will soon be replaced by migration v2 but we
may as well apply this fix in the meantime. CCing Andy C on the unlikely
chance that he carried this misbehaviour over into v2.

> ---
>  tools/libxc/xc_domain_restore.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
> index b9a56d5..bec716c 100644
> --- a/tools/libxc/xc_domain_restore.c
> +++ b/tools/libxc/xc_domain_restore.c
> @@ -1106,7 +1106,7 @@ static int pagebuf_get(xc_interface *xch, struct restore_ctx *ctx,
>  static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx,
>                         xen_pfn_t* region_mfn, unsigned long* pfn_type, int pae_extended_cr3,
>                         struct xc_mmu* mmu,
> -                       pagebuf_t* pagebuf, int curbatch)
> +                       pagebuf_t* pagebuf, int curbatch, int *invalid_pages)
>  {
>      int i, j, curpage, nr_mfns;
>      int k, scount;
> @@ -1121,6 +1121,12 @@ static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx,
>      struct domain_info_context *dinfo = &ctx->dinfo;
>      int* pfn_err = NULL;
>      int rc = -1;
> +    int local_invalid_pages = 0;
> +    /* We have handled curbatch pages before this batch, and there are
> +     * *invalid_pages pages that are not in pagebuf->pages. So the first
> +     * page for this page is (curbatch - *invalid_pages) page.
> +     */
> +    int first_page = curbatch - *invalid_pages;
>  
>      unsigned long mfn, pfn, pagetype;
>  
> @@ -1293,10 +1299,13 @@ static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx,
>          pfn      = pagebuf->pfn_types[i + curbatch] & ~XEN_DOMCTL_PFINFO_LTAB_MASK;
>          pagetype = pagebuf->pfn_types[i + curbatch] &  XEN_DOMCTL_PFINFO_LTAB_MASK;
>  
> -        if ( pagetype == XEN_DOMCTL_PFINFO_XTAB 
> +        if ( pagetype == XEN_DOMCTL_PFINFO_XTAB
>               || pagetype == XEN_DOMCTL_PFINFO_XALLOC)
> +        {
> +            local_invalid_pages++;
>              /* a bogus/unmapped/allocate-only page: skip it */
>              continue;
> +        }
>  
>          if ( pagetype == XEN_DOMCTL_PFINFO_BROKEN )
>          {
> @@ -1306,6 +1315,8 @@ static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx,
>                        "dom=%d, pfn=%lx\n", dom, pfn);
>                  goto err_mapped;
>              }
> +
> +            local_invalid_pages++;
>              continue;
>          }
>  
> @@ -1344,7 +1355,7 @@ static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx,
>              }
>          }
>          else
> -            memcpy(page, pagebuf->pages + (curpage + curbatch) * PAGE_SIZE,
> +            memcpy(page, pagebuf->pages + (first_page + curpage) * PAGE_SIZE,
>                     PAGE_SIZE);
>  
>          pagetype &= XEN_DOMCTL_PFINFO_LTABTYPE_MASK;
> @@ -1418,6 +1429,7 @@ static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx,
>      } /* end of 'batch' for loop */
>  
>      rc = nraces;
> +    *invalid_pages += local_invalid_pages;
>  
>    err_mapped:
>      munmap(region_base, j*PAGE_SIZE);
> @@ -1621,7 +1633,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
>   loadpages:
>      for ( ; ; )
>      {
> -        int j, curbatch;
> +        int j, curbatch, invalid_pages;
>  
>          xc_report_progress_step(xch, n, dinfo->p2m_size);
>  
> @@ -1665,11 +1677,13 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
>  
>          /* break pagebuf into batches */
>          curbatch = 0;
> +        invalid_pages = 0;
>          while ( curbatch < j ) {
>              int brc;
>  
>              brc = apply_batch(xch, dom, ctx, region_mfn, pfn_type,
> -                              pae_extended_cr3, mmu, &pagebuf, curbatch);
> +                              pae_extended_cr3, mmu, &pagebuf, curbatch,
> +                              &invalid_pages);
>              if ( brc < 0 )
>                  goto out;
>  

Re: [RFC Patch v3 02/18] csum the correct page

[Ian Campbell]

On Fri, 2014-09-05 at 17:10 +0800, Wen Congyang wrote:
> In verify mode, we map the guest memory, and the guest page is
> region_base + i * PAGE_SIZE. So we should csum page (region_base
> + i * PAGE_SIZE), not (region_base + (i+curbatch) * PAGE_SIZE)
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  tools/libxc/xc_domain_restore.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
> index bec716c..fb4ddfc 100644
> --- a/tools/libxc/xc_domain_restore.c
> +++ b/tools/libxc/xc_domain_restore.c
> @@ -1405,7 +1405,7 @@ static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx,
>  
>                  DPRINTF("************** pfn=%lx type=%lx gotcs=%08lx "
>                          "actualcs=%08lx\n", pfn, pagebuf->pfn_types[pfn],
> -                        csum_page(region_base + (i + curbatch)*PAGE_SIZE),
> +                        csum_page(region_base + i * PAGE_SIZE),
>                          csum_page(buf));
>  
>                  for ( v = 0; v < 4; v++ )

Re: [RFC Patch v3 04/18] blktap2: dynamic allocate aio_requests to avoid -EBUSY error

[Ian Campbell]

On Fri, 2014-09-05 at 17:10 +0800, Wen Congyang wrote:
> From: Lai Jiangshan <laijs@cn.fujitsu.com>
> 
> In normal case, there are at most TAPDISK_DATA_REQUESTS request
> at the same time. But in remus mode, the write requests are
> forwarded from the master side, and cached in block-remus. All
> cached requests will be forwarded to aio driver when syncing
> primary vm and backup vm. In this case, The number of requests
> may be more than TAPDISK_DATA_REQUESTS. So aio driver can't hanlde
> these requests at the same time, it will cause tapdisk2 exit.
> 
> We don't know how many requests will be handled, so dynamic allocate
> aio_requests to avoid this error.
> 
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> Signed-off-by: Jiang Yunhong <yunhong.jiang@intel.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>

blktap2 is essentially unmaintained these days. Since I think the remus
folks are the mains ones left who care about it (and these patches seem
to be Remus/COLO related) I think I'd be happy to apply the 4 blktap2
patches from this series with Shriram or Yang's Ack.

Ian.

Re: [RFC Patch v3 14/18] pass correct file to qemu if we use blktap2

[Ian Campbell]

On Fri, 2014-09-05 at 17:10 +0800, Wen Congyang wrote:
> If we use blktap2, the correct file should be blktap device
> not the pdev_path.
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [RFC Patch v3 15/18] support blktap remus in xl

[Ian Campbell]

NB, this series was presented as "Some bugfix patches". I think we are
way out of that territory and into feature land.

On Fri, 2014-09-05 at 17:10 +0800, Wen Congyang wrote:
> With this patch, we can use blktap remus like this:
> disk = [ 'format=remus,devtype=disk,access=w,vdev=hda,backendtype=tap,target=192.168.3.1:9000|aio:filename' ]

I don't think it makes sense to wedge remus in as a kind of pseudo
format as you've done here, but I'm not really sure what the actual
right answer is. I suspect some sort of remus specific field or cfg file
option. Perhaps ",remus=192.168...,target=filename".

Ian J has been following the Remus stuff more closely so perhaps he has
a better idea. (Note: he's away all of this week)

Ian.

Re: [RFC Patch v3 17/18] read nictype from xenstore

[Ian Campbell]

On Fri, 2014-09-05 at 17:11 +0800, Wen Congyang wrote:
> We need to use nictype to get default vifname.
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [RFC Patch v3 16/18] update libxl__device_disk_from_xs_be() to support blktap device

[Ian Campbell]

On Fri, 2014-09-05 at 17:10 +0800, Wen Congyang wrote:
> 
> +int libxl_string_to_format(libxl_ctx *ctx, char *s, libxl_disk_format
> *format)
> +{

This already exists as libxl_disk_format_to_string.

Ian.

Re: [RFC Patch v3 01/18] copy the correct page to memory

[Andrew Cooper]

On 08/09/14 12:27, Ian Campbell wrote:
> On Fri, 2014-09-05 at 17:10 +0800, Wen Congyang wrote:
>> From: Hong Tao <bobby.hong@huawei.com>
>>
>> apply_batch() only handles MAX_BATCH_SIZE pages at one time. If
>> there is some bogus/unmapped/allocate-only/broken page, we will
>> skip it. So when we call apply_batch() again, the first page's
>> index is curbatch - invalid_pages. invalid_pages stores the number
>> of bogus/unmapped/allocate-only/broken pages we have found.
>>
>> In many cases, invalid_pages is 0, so we don't catch this error.
>>
>> Signed-off-by: Hong Tao <bobby.hong@huawei.com>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> Hopefully all of this stuff will soon be replaced by migration v2 but we
> may as well apply this fix in the meantime. CCing Andy C on the unlikely
> chance that he carried this misbehaviour over into v2.

I rewrote all of this from scratch.

Amongst other things, the v2 code doesn't batch explicitly batch on the
restoring side.  It will deal with an individual PAGE_DATA record at
once, which will generally contain 1024 pages of data, but will not
cache some parts of this to batch better with the following PAGE_DATA
record, which appears to be the source of this bug.

As for deletion.  I am planning to delete it as part of the v2 series,
although that might get complicated if remusv2 is not ready by that point.

~Andrew

Re: [RFC Patch v3 16/18] update libxl__device_disk_from_xs_be() to support blktap device

[Wen Congyang]

At 09/08/2014 07:42 PM, Ian Campbell Write:
> On Fri, 2014-09-05 at 17:10 +0800, Wen Congyang wrote:
>>
>> +int libxl_string_to_format(libxl_ctx *ctx, char *s, libxl_disk_format
>> *format)
>> +{
> 
> This already exists as libxl_disk_format_to_string.

Yes, I don't notice it. I will remove libxl_string_to_format in next version.

Thanks
Wen Congyang

> 
> Ian.
> 
> .
> 

Re: [RFC Patch v3 15/18] support blktap remus in xl

[Wen Congyang]

At 09/08/2014 07:39 PM, Ian Campbell Write:
> NB, this series was presented as "Some bugfix patches". I think we are
> way out of that territory and into feature land.
> 
> On Fri, 2014-09-05 at 17:10 +0800, Wen Congyang wrote:
>> With this patch, we can use blktap remus like this:
>> disk = [ 'format=remus,devtype=disk,access=w,vdev=hda,backendtype=tap,target=192.168.3.1:9000|aio:filename' ]
> 
> I don't think it makes sense to wedge remus in as a kind of pseudo
> format as you've done here, but I'm not really sure what the actual
> right answer is. I suspect some sort of remus specific field or cfg file
> option. Perhaps ",remus=192.168...,target=filename".

What about this: "format=raw,backendtype=tap,filter=remus, filter-params=192.168...,target=filename"?

Thanks
Wen Congyang

> 
> Ian J has been following the Remus stuff more closely so perhaps he has
> a better idea. (Note: he's away all of this week)
> 
> Ian.
> 
> .
> 

Re: [RFC Patch v3 15/18] support blktap remus in xl

[Ian Campbell]

On Wed, 2014-09-10 at 15:19 +0800, Wen Congyang wrote:
> At 09/08/2014 07:39 PM, Ian Campbell Write:
> > NB, this series was presented as "Some bugfix patches". I think we are
> > way out of that territory and into feature land.
> > 
> > On Fri, 2014-09-05 at 17:10 +0800, Wen Congyang wrote:
> >> With this patch, we can use blktap remus like this:
> >> disk = [ 'format=remus,devtype=disk,access=w,vdev=hda,backendtype=tap,target=192.168.3.1:9000|aio:filename' ]
> > 
> > I don't think it makes sense to wedge remus in as a kind of pseudo
> > format as you've done here, but I'm not really sure what the actual
> > right answer is. I suspect some sort of remus specific field or cfg file
> > option. Perhaps ",remus=192.168...,target=filename".
> 
> What about this: "format=raw,backendtype=tap,filter=remus, filter-params=192.168...,target=filename"?

I don't know, because you've not defined what a "filter" is as an
abstract concept i.e. what else might it be used for, so I don't know
why we would prefer it to just remus= as a toplevel thing.

Ian.

Re: [RFC Patch v3 15/18] support blktap remus in xl

[Wen Congyang]

At 09/10/2014 06:04 PM, Ian Campbell Write:
> On Wed, 2014-09-10 at 15:19 +0800, Wen Congyang wrote:
>> At 09/08/2014 07:39 PM, Ian Campbell Write:
>>> NB, this series was presented as "Some bugfix patches". I think we are
>>> way out of that territory and into feature land.
>>>
>>> On Fri, 2014-09-05 at 17:10 +0800, Wen Congyang wrote:
>>>> With this patch, we can use blktap remus like this:
>>>> disk = [ 'format=remus,devtype=disk,access=w,vdev=hda,backendtype=tap,target=192.168.3.1:9000|aio:filename' ]
>>>
>>> I don't think it makes sense to wedge remus in as a kind of pseudo
>>> format as you've done here, but I'm not really sure what the actual
>>> right answer is. I suspect some sort of remus specific field or cfg file
>>> option. Perhaps ",remus=192.168...,target=filename".
>>
>> What about this: "format=raw,backendtype=tap,filter=remus, filter-params=192.168...,target=filename"?
> 
> I don't know, because you've not defined what a "filter" is as an
> abstract concept i.e. what else might it be used for, so I don't know
> why we would prefer it to just remus= as a toplevel thing.

blktap2 accepts such params: [driver:params|]driver:params
The last driver should be aio/vhd, and the params should be the filepath/devpath.
Only the last drvier operates the file/device.

I call the first driver filter driver. All I/O request will be sent to filter
driver first, the filter driver can do anything(for example, log, block...). After
this, the I/O reqeust will be forwarded to last driver.

The first driver can be cache, log, remus, or colo(implemented in another patchset)...
If we use "remus=", I think we should support "log=", "cache=", "colo="...
Too many keys...

Thanks
Wen Congyang

> 
> Ian.
> 
> .
> 

Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.

[Aravind Gopalakrishnan]

On 9/5/2014 4:11 AM, Wen Congyang wrote:
> In colo mode, secondary vm is running, so VM_ENTRY_INTR_INFO may
> valid before restoring vmcs. If there is no pending event after
> restoring vm, we should clear it.
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>
> Also clear pending software exceptions.
> Copy the fix to SVM as well.
>
> Signed-off-by: Tim Deegan <tim@xen.org>
>
> vmx part:
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> Acked-by: Kevin Tian <kevin.tian@intel.com>
>
> svm part:
> Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>   xen/arch/x86/hvm/svm/svm.c | 15 ++++++++-------
>   xen/arch/x86/hvm/vmx/vmx.c | 25 ++++++++++++-------------
>   2 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index b5188e6..053e511 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -321,16 +321,17 @@ static int svm_vmcb_restore(struct vcpu *v, struct hvm_hw_cpu *c)
>           vmcb_set_h_cr3(vmcb, pagetable_get_paddr(p2m_get_pagetable(p2m)));
>       }
>   
> -    if ( c->pending_valid )
> +    if ( c->pending_valid &&
> +         hvm_event_needs_reinjection(c->pending_type, c->pending_vector) )
>       {
>           gdprintk(XENLOG_INFO, "Re-injecting %#"PRIx32", %#"PRIx32"\n",
>                    c->pending_event, c->error_code);
> -
> -        if ( hvm_event_needs_reinjection(c->pending_type, c->pending_vector) )
> -        {
> -            vmcb->eventinj.bytes = c->pending_event;
> -            vmcb->eventinj.fields.errorcode = c->error_code;
> -        }
> +        vmcb->eventinj.bytes = c->pending_event;
> +        vmcb->eventinj.fields.errorcode = c->error_code;
> +    }
> +    else
> +    {
> +        vmcb->eventinj.bytes = 0;
>       }
>   
>       vmcb->cleanbits.bytes = 0;
>

Hi,
You mention that this 'fix' is just copied for svm. But you have not 
seen the problem of "VM_ENTRY_INTR_INFO may be valid" (whose svm 
equivalent is "vmcb->eventinj.bytes is valid").
My concern is that we should test colo mode for svm first, since, if the 
problem is never really seen on svm, then fix may not be _necessary_

At this point, my problems are with test setups. I can help testing 
scenarios, but as Wen had mentioned, 'colo testing' might be the way to 
test.
So, if I can get some pointers to how I can reproduce the issue at hand, 
then it would be very helpful.

(Tim had mentioned we could try to simulate it by running a guest that 
takes lot of faults and save-restore another guest over it. However,
I am not having much luck following this route. I got a hvm guest to 
continuously take SW exceptions on all vcpus and tried to save-restore.
I can't see vmcb->eventinj.bytes containing any valid info.)

Thanks,
-Aravind.

Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.

[Wen Congyang]

On 09/10/2014 11:06 PM, Aravind Gopalakrishnan wrote:
> On 9/5/2014 4:11 AM, Wen Congyang wrote:
>> In colo mode, secondary vm is running, so VM_ENTRY_INTR_INFO may
>> valid before restoring vmcs. If there is no pending event after
>> restoring vm, we should clear it.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>
>> Also clear pending software exceptions.
>> Copy the fix to SVM as well.
>>
>> Signed-off-by: Tim Deegan <tim@xen.org>
>>
>> vmx part:
>> Cc: Jun Nakajima <jun.nakajima@intel.com>
>> Acked-by: Kevin Tian <kevin.tian@intel.com>
>>
>> svm part:
>> Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> ---
>>   xen/arch/x86/hvm/svm/svm.c | 15 ++++++++-------
>>   xen/arch/x86/hvm/vmx/vmx.c | 25 ++++++++++++-------------
>>   2 files changed, 20 insertions(+), 20 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>> index b5188e6..053e511 100644
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -321,16 +321,17 @@ static int svm_vmcb_restore(struct vcpu *v, struct hvm_hw_cpu *c)
>>           vmcb_set_h_cr3(vmcb, pagetable_get_paddr(p2m_get_pagetable(p2m)));
>>       }
>>   -    if ( c->pending_valid )
>> +    if ( c->pending_valid &&
>> +         hvm_event_needs_reinjection(c->pending_type, c->pending_vector) )
>>       {
>>           gdprintk(XENLOG_INFO, "Re-injecting %#"PRIx32", %#"PRIx32"\n",
>>                    c->pending_event, c->error_code);
>> -
>> -        if ( hvm_event_needs_reinjection(c->pending_type, c->pending_vector) )
>> -        {
>> -            vmcb->eventinj.bytes = c->pending_event;
>> -            vmcb->eventinj.fields.errorcode = c->error_code;
>> -        }
>> +        vmcb->eventinj.bytes = c->pending_event;
>> +        vmcb->eventinj.fields.errorcode = c->error_code;
>> +    }
>> +    else
>> +    {
>> +        vmcb->eventinj.bytes = 0;
>>       }
>>         vmcb->cleanbits.bytes = 0;
>>
> 
> Hi,
> You mention that this 'fix' is just copied for svm. But you have not seen the problem of "VM_ENTRY_INTR_INFO may be valid" (whose svm equivalent is "vmcb->eventinj.bytes is valid").
> My concern is that we should test colo mode for svm first, since, if the problem is never really seen on svm, then fix may not be _necessary_

Agree.

> 
> At this point, my problems are with test setups. I can help testing scenarios, but as Wen had mentioned, 'colo testing' might be the way to test.
> So, if I can get some pointers to how I can reproduce the issue at hand, then it would be very helpful.

Currently, only COLO can trigger this problem, and COLO is not ready now. So I think this bugfix is not very important now.
If the COLO is ready, and I will give you the way to trigger this problem.

> 
> (Tim had mentioned we could try to simulate it by running a guest that takes lot of faults and save-restore another guest over it. However,
> I am not having much luck following this route. I got a hvm guest to continuously take SW exceptions on all vcpus and tried to save-restore.
> I can't see vmcb->eventinj.bytes containing any valid info.)

I don't understand this way..
If we can trigger this problem by this way, we can verify this patch now.

Thanks
Wen Congyang

> 
> Thanks,
> -Aravind.
> .
> 

Re: [RFC Patch v3 16/18] update libxl__device_disk_from_xs_be() to support blktap device

[Wen Congyang]

On 09/08/2014 07:42 PM, Ian Campbell wrote:
> On Fri, 2014-09-05 at 17:10 +0800, Wen Congyang wrote:
>>
>> +int libxl_string_to_format(libxl_ctx *ctx, char *s, libxl_disk_format
>> *format)
>> +{
> 
> This already exists as libxl_disk_format_to_string.

Another question:
We store format:file to tapdisk-params/params. But we store aio if the format
is raw. libxl_disk_format_to_string() doesn't recognize aio...

Is it ok to change the value stored in tapdisk-params/params?

Thanks
Wen Congyang

> 
> Ian.
> 
> .
> 

Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.

[Tim Deegan]

[-- Attachment #1: Type: text/plain, Size: 1418 bytes --]

At 10:06 -0500 on 10 Sep (1410340016), Aravind Gopalakrishnan wrote:
> Hi,
> You mention that this 'fix' is just copied for svm. But you have not 
> seen the problem of "VM_ENTRY_INTR_INFO may be valid" (whose svm 
> equivalent is "vmcb->eventinj.bytes is valid").
> My concern is that we should test colo mode for svm first, since, if the 
> problem is never really seen on svm, then fix may not be _necessary_

I think it's obvious by inspection of the code (and indeed the patch)
that SVM has the equivalent bug.

> At this point, my problems are with test setups. I can help testing 
> scenarios, but as Wen had mentioned, 'colo testing' might be the way to 
> test.
> So, if I can get some pointers to how I can reproduce the issue at hand, 
> then it would be very helpful.
> 
> (Tim had mentioned we could try to simulate it by running a guest that 
> takes lot of faults and save-restore another guest over it. However,
> I am not having much luck following this route. I got a hvm guest to 
> continuously take SW exceptions on all vcpus and tried to save-restore.
> I can't see vmcb->eventinj.bytes containing any valid info.)

Although I think that getting COLO running on SVM is a gret idea, it
shouldn't block acceptance of this fix.  I've attached a program
that tests for the bug.  Run it against any HVM guest (and then
destroy the guest because the test will have corrupted its state).

Cheers,

Tim.

[-- Attachment #2: injectiontest.c --]
[-- Type: text/x-csrc, Size: 2649 bytes --]


#include <err.h>
#include <stdio.h>
#include <stdlib.h>

#include <xenctrl.h>

#include <xen/hvm/save.h>

int main(int argc, char **argv)
{
    xc_interface *xch;
    int dom;

    struct {
        struct hvm_save_descriptor hdesc;
        struct hvm_save_header header;
        struct hvm_save_descriptor cdesc;
        struct hvm_hw_cpu cpu;
        struct hvm_save_descriptor ndesc;
    } buf;

    if (argc != 2 || (dom = atoi(argv[1])) <= 0)
        errx(1, "usage: injectiontest <domid>");

    buf.hdesc = (struct hvm_save_descriptor) {
        .typecode = HVM_SAVE_CODE(HEADER),
        .instance = 0,
        .length = sizeof buf.header,
    };
    buf.header = (struct hvm_save_header) {
        .magic = HVM_FILE_MAGIC,
        .version = HVM_FILE_VERSION,
    };
    buf.cdesc = (struct hvm_save_descriptor) {
        .typecode = HVM_SAVE_CODE(CPU),
        .instance = 0,
        .length = sizeof buf.cpu,
    };
    buf.ndesc = (struct hvm_save_descriptor) { 0 };

    xch = xc_interface_open(NULL, NULL, 0);

    printf("pausing domain\n");
    if (xc_domain_pause(xch, dom))
        err(1, "could not pause");

    printf("reading vcpu 0\n");
    if (xc_domain_hvm_getcontext_partial(xch, dom, HVM_SAVE_CODE(CPU),
                                         0, &buf.cpu, sizeof(buf.cpu)))
        err(1, "could not read vcpu state");

    printf("injecting an event\n");
    buf.cpu.pending_valid = 1;
    buf.cpu.pending_vector = 14; // page fault
    buf.cpu.pending_type = 3;    // H/W exception
    buf.cpu.pending_error_valid = 1;
    buf.cpu.error_code = 1;

    if (xc_domain_hvm_setcontext(xch, dom, (uint8_t *)&buf, sizeof buf))
        err(1, "could not set vcpu state");

    if (xc_domain_hvm_getcontext_partial(xch, dom, HVM_SAVE_CODE(CPU),
                                         0, &buf.cpu, sizeof(buf.cpu)))
        err(1, "could not read vcpu state");

    if (buf.cpu.pending_valid)
        printf("good: vcpu shows an event is pending\n");
    else {
        printf("bad: no event pending after one was injected\n");
        exit(-1);
    }

    printf("clearing the event\n");
    buf.cpu.pending_event = 0;

    if (xc_domain_hvm_setcontext(xch, dom, (uint8_t *)&buf, sizeof buf))
        err(1, "could not set vcpu state");

    if (xc_domain_hvm_getcontext_partial(xch, dom, HVM_SAVE_CODE(CPU),
                                         0, &buf.cpu, sizeof(buf.cpu)))
        err(1, "could not read vcpu state");

    if (!buf.cpu.pending_valid)
        printf("good: vcpu shows no event pending\n");
    else {
        printf("bad: existing event is still pending\n");
        exit(-1);
    }

    return 0;
}

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.

[Wen Congyang]

On 09/11/2014 06:35 PM, Tim Deegan wrote:
> At 10:06 -0500 on 10 Sep (1410340016), Aravind Gopalakrishnan wrote:
>> Hi,
>> You mention that this 'fix' is just copied for svm. But you have not 
>> seen the problem of "VM_ENTRY_INTR_INFO may be valid" (whose svm 
>> equivalent is "vmcb->eventinj.bytes is valid").
>> My concern is that we should test colo mode for svm first, since, if the 
>> problem is never really seen on svm, then fix may not be _necessary_
> 
> I think it's obvious by inspection of the code (and indeed the patch)
> that SVM has the equivalent bug.
> 
>> At this point, my problems are with test setups. I can help testing 
>> scenarios, but as Wen had mentioned, 'colo testing' might be the way to 
>> test.
>> So, if I can get some pointers to how I can reproduce the issue at hand, 
>> then it would be very helpful.
>>
>> (Tim had mentioned we could try to simulate it by running a guest that 
>> takes lot of faults and save-restore another guest over it. However,
>> I am not having much luck following this route. I got a hvm guest to 
>> continuously take SW exceptions on all vcpus and tried to save-restore.
>> I can't see vmcb->eventinj.bytes containing any valid info.)
> 
> Although I think that getting COLO running on SVM is a gret idea, it
> shouldn't block acceptance of this fix.  I've attached a program
> that tests for the bug.  Run it against any HVM guest (and then
> destroy the guest because the test will have corrupted its state).

Thanks for providing test program.
I run it against a HVM guest, and unpause the guest by hand. The guest
doesn't response anymore. Apply this patch and test it again, the problem
is gone.

Wen Congyang

> 
> Cheers,
> 
> Tim.
> 

Re: [RFC Patch v3 03/18] don't zero out ioreq page

[Wen Congyang]

On 09/05/2014 06:45 PM, David Vrabel wrote:
> On 05/09/14 10:10, Wen Congyang wrote:
>> ioreq page may contain some pending I/O requests, and we need to
>> handle the pending I/O req after migration.
>>
>> TODO:
>> 1. update qemu to handle the pending I/O req
> 
> Under what conditions can there be pending I/O?  A domain suspend should
> not complete until any I/O accesses are complete.

IIRC, I print ioreq.state after suspending vm, and ioreq.state can be
STATE_IOREQ_NONE, STATE_IOREQ_READY or STATE_IORESP_READY. If the
state is STATE_IOREQ_READY, we should kick vcpu event channel on resume
after migration.

But I test it again today, the state is always STATE_IOREQ_NONE...

If the state is always STATE_IOREQ_NONE, no need to touch hypervisor/Qemu
to handle pending I/O req(state is STATE_IOREQ_READY).

Thanks
Wen Congyang

> 
> Suspending a domain that's part-way through an instruction seems mad to
> me -- how are you supposed to capture this sort of state?
> 
> David
> .
> 

Re: [RFC Patch v3 16/18] update libxl__device_disk_from_xs_be() to support blktap device

[Wei Liu]

On Thu, Sep 11, 2014 at 03:58:37PM +0800, Wen Congyang wrote:
> On 09/08/2014 07:42 PM, Ian Campbell wrote:
> > On Fri, 2014-09-05 at 17:10 +0800, Wen Congyang wrote:
> >>
> >> +int libxl_string_to_format(libxl_ctx *ctx, char *s, libxl_disk_format
> >> *format)
> >> +{
> > 
> > This already exists as libxl_disk_format_to_string.
> 
> Another question:
> We store format:file to tapdisk-params/params. But we store aio if the format
> is raw. libxl_disk_format_to_string() doesn't recognize aio...
> 

I think you're talking about libxl_disk_format_from_string...

FWIW, have you looked at libxl__device_disk_string_of_format?

Wei.

> Is it ok to change the value stored in tapdisk-params/params?
> 
> Thanks
> Wen Congyang
> 
> > 
> > Ian.
> > 
> > .
> > 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [RFC Patch v3 16/18] update libxl__device_disk_from_xs_be() to support blktap device

[Wen Congyang]

On 09/12/2014 04:53 PM, Wei Liu wrote:
> On Thu, Sep 11, 2014 at 03:58:37PM +0800, Wen Congyang wrote:
>> On 09/08/2014 07:42 PM, Ian Campbell wrote:
>>> On Fri, 2014-09-05 at 17:10 +0800, Wen Congyang wrote:
>>>>
>>>> +int libxl_string_to_format(libxl_ctx *ctx, char *s, libxl_disk_format
>>>> *format)
>>>> +{
>>>
>>> This already exists as libxl_disk_format_to_string.
>>
>> Another question:
>> We store format:file to tapdisk-params/params. But we store aio if the format
>> is raw. libxl_disk_format_to_string() doesn't recognize aio...
>>
> 
> I think you're talking about libxl_disk_format_from_string...

Yes

> 
> FWIW, have you looked at libxl__device_disk_string_of_format?

char *libxl__device_disk_string_of_format(libxl_disk_format format)
{
    switch (format) {
        case LIBXL_DISK_FORMAT_QCOW: return "qcow";
        case LIBXL_DISK_FORMAT_QCOW2: return "qcow2";
        case LIBXL_DISK_FORMAT_VHD: return "vhd";
        case LIBXL_DISK_FORMAT_RAW:
        case LIBXL_DISK_FORMAT_EMPTY: return "aio";
        default: return NULL;
    }
}

If the format is LIBXL_DISK_FORMAT_RAW, we store "aio" in tapdisk-params/params,
But libxl_disk_format_from_string():
libxl_enum_string_table libxl_disk_format_string_table[] = {
    { .s = "unknown", .v = LIBXL_DISK_FORMAT_UNKNOWN },
    { .s = "qcow", .v = LIBXL_DISK_FORMAT_QCOW },
    { .s = "qcow2", .v = LIBXL_DISK_FORMAT_QCOW2 },
    { .s = "vhd", .v = LIBXL_DISK_FORMAT_VHD },
    { .s = "raw", .v = LIBXL_DISK_FORMAT_RAW },
    { .s = "empty", .v = LIBXL_DISK_FORMAT_EMPTY },
    { NULL, -1 },
};

int libxl_disk_format_from_string(const char *s, libxl_disk_format *e)
{
    return libxl__enum_from_string(libxl_disk_format_string_table,
                                   s, (int *)e);
}
If the string is "aio", libxl_disk_format_from_string() will return ERROR_FAIL.

We have two choices:
1. Introduce a new API
2. store "raw" in tapdisk-params/params

Thanks
Wen Congyang

> 
> Wei.
> 
>> Is it ok to change the value stored in tapdisk-params/params?
>>
>> Thanks
>> Wen Congyang
>>
>>>
>>> Ian.
>>>
>>> .
>>>
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
> .
> 

Re: [RFC Patch v3 16/18] update libxl__device_disk_from_xs_be() to support blktap device

[Wei Liu]

On Fri, Sep 12, 2014 at 05:03:02PM +0800, Wen Congyang wrote:
> On 09/12/2014 04:53 PM, Wei Liu wrote:
> > On Thu, Sep 11, 2014 at 03:58:37PM +0800, Wen Congyang wrote:
> >> On 09/08/2014 07:42 PM, Ian Campbell wrote:
> >>> On Fri, 2014-09-05 at 17:10 +0800, Wen Congyang wrote:
> >>>>
> >>>> +int libxl_string_to_format(libxl_ctx *ctx, char *s, libxl_disk_format
> >>>> *format)
> >>>> +{
> >>>
> >>> This already exists as libxl_disk_format_to_string.
> >>
> >> Another question:
> >> We store format:file to tapdisk-params/params. But we store aio if the format
> >> is raw. libxl_disk_format_to_string() doesn't recognize aio...
> >>
> > 
> > I think you're talking about libxl_disk_format_from_string...
> 
> Yes
> 
> > 
> > FWIW, have you looked at libxl__device_disk_string_of_format?
> 
> char *libxl__device_disk_string_of_format(libxl_disk_format format)
> {
>     switch (format) {
>         case LIBXL_DISK_FORMAT_QCOW: return "qcow";
>         case LIBXL_DISK_FORMAT_QCOW2: return "qcow2";
>         case LIBXL_DISK_FORMAT_VHD: return "vhd";
>         case LIBXL_DISK_FORMAT_RAW:
>         case LIBXL_DISK_FORMAT_EMPTY: return "aio";
>         default: return NULL;
>     }
> }
> 
> If the format is LIBXL_DISK_FORMAT_RAW, we store "aio" in tapdisk-params/params,
> But libxl_disk_format_from_string():
> libxl_enum_string_table libxl_disk_format_string_table[] = {
>     { .s = "unknown", .v = LIBXL_DISK_FORMAT_UNKNOWN },
>     { .s = "qcow", .v = LIBXL_DISK_FORMAT_QCOW },
>     { .s = "qcow2", .v = LIBXL_DISK_FORMAT_QCOW2 },
>     { .s = "vhd", .v = LIBXL_DISK_FORMAT_VHD },
>     { .s = "raw", .v = LIBXL_DISK_FORMAT_RAW },
>     { .s = "empty", .v = LIBXL_DISK_FORMAT_EMPTY },
>     { NULL, -1 },
> };
> 
> int libxl_disk_format_from_string(const char *s, libxl_disk_format *e)
> {
>     return libxl__enum_from_string(libxl_disk_format_string_table,
>                                    s, (int *)e);
> }
> If the string is "aio", libxl_disk_format_from_string() will return ERROR_FAIL.
> 

Heh, interesting. This looks like a bug to me. But I'm not sure whether
this is for backward compatibility.

Git blame shows that libxl__device_disk_string_of_format hasn't been
touched for quite a while. I don't have enough knowledge of how this
came into being.

> We have two choices:
> 1. Introduce a new API

IMHO this has the risk of becoming out-of-sync in the future,
just like libxl__device_disk_string_of_format (if it is the case for
it).

Note that functions in _libxl_types*.[ch] are autogenerated so whenever
we make change to IDL they get regenerated, so that always reflects the
current interface.

Wei.

> 2. store "raw" in tapdisk-params/params
> 
> Thanks
> Wen Congyang
> 
> > 
> > Wei.
> > 
> >> Is it ok to change the value stored in tapdisk-params/params?
> >>
> >> Thanks
> >> Wen Congyang
> >>
> >>>
> >>> Ian.
> >>>
> >>> .
> >>>
> >>
> >>
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@lists.xen.org
> >> http://lists.xen.org/xen-devel
> > .
> > 

Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.

[Tim Deegan]

At 11:14 +0800 on 12 Sep (1410516863), Wen Congyang wrote:
> On 09/11/2014 06:35 PM, Tim Deegan wrote:
> > Although I think that getting COLO running on SVM is a gret idea, it
> > shouldn't block acceptance of this fix.  I've attached a program
> > that tests for the bug.  Run it against any HVM guest (and then
> > destroy the guest because the test will have corrupted its state).
> 
> Thanks for providing test program.
> I run it against a HVM guest, and unpause the guest by hand. The guest
> doesn't response anymore. Apply this patch and test it again, the problem
> is gone.

Glad to hear it -- but to be clear, the important detail is really
whether the test itself prints 'good' or 'bad'.  The guest might
potentially be crashed by the test even if it succeeds.

Cheers,

Tim.

Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.

[Wen Congyang]

On 09/11/2014 06:35 PM, Tim Deegan wrote:
> At 10:06 -0500 on 10 Sep (1410340016), Aravind Gopalakrishnan wrote:
>> Hi,
>> You mention that this 'fix' is just copied for svm. But you have not 
>> seen the problem of "VM_ENTRY_INTR_INFO may be valid" (whose svm 
>> equivalent is "vmcb->eventinj.bytes is valid").
>> My concern is that we should test colo mode for svm first, since, if the 
>> problem is never really seen on svm, then fix may not be _necessary_
> 
> I think it's obvious by inspection of the code (and indeed the patch)
> that SVM has the equivalent bug.
> 
>> At this point, my problems are with test setups. I can help testing 
>> scenarios, but as Wen had mentioned, 'colo testing' might be the way to 
>> test.
>> So, if I can get some pointers to how I can reproduce the issue at hand, 
>> then it would be very helpful.
>>
>> (Tim had mentioned we could try to simulate it by running a guest that 
>> takes lot of faults and save-restore another guest over it. However,
>> I am not having much luck following this route. I got a hvm guest to 
>> continuously take SW exceptions on all vcpus and tried to save-restore.
>> I can't see vmcb->eventinj.bytes containing any valid info.)
> 
> Although I think that getting COLO running on SVM is a gret idea, it
> shouldn't block acceptance of this fix.  I've attached a program
> that tests for the bug.  Run it against any HVM guest (and then
> destroy the guest because the test will have corrupted its state).

Hi, Aravind Gopalakrishnan

Do you have time to test this patch with the test program?

Thanks
Wen Congyang

> 
> Cheers,
> 
> Tim.
> 

Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.

[Aravind Gopalakrishnan]

On 9/17/2014 2:56 AM, Wen Congyang wrote:
> On 09/11/2014 06:35 PM, Tim Deegan wrote:
>> At 10:06 -0500 on 10 Sep (1410340016), Aravind Gopalakrishnan wrote:
>>> Hi,
>>> You mention that this 'fix' is just copied for svm. But you have not
>>> seen the problem of "VM_ENTRY_INTR_INFO may be valid" (whose svm
>>> equivalent is "vmcb->eventinj.bytes is valid").
>>> My concern is that we should test colo mode for svm first, since, if the
>>> problem is never really seen on svm, then fix may not be _necessary_
>> I think it's obvious by inspection of the code (and indeed the patch)
>> that SVM has the equivalent bug.
>>
>>> At this point, my problems are with test setups. I can help testing
>>> scenarios, but as Wen had mentioned, 'colo testing' might be the way to
>>> test.
>>> So, if I can get some pointers to how I can reproduce the issue at hand,
>>> then it would be very helpful.
>>>
>>> (Tim had mentioned we could try to simulate it by running a guest that
>>> takes lot of faults and save-restore another guest over it. However,
>>> I am not having much luck following this route. I got a hvm guest to
>>> continuously take SW exceptions on all vcpus and tried to save-restore.
>>> I can't see vmcb->eventinj.bytes containing any valid info.)
>> Although I think that getting COLO running on SVM is a gret idea, it
>> shouldn't block acceptance of this fix.  I've attached a program
>> that tests for the bug.  Run it against any HVM guest (and then
>> destroy the guest because the test will have corrupted its state).
> Hi, Aravind Gopalakrishnan
>
> Do you have time to test this patch with the test program?
>

Wen,
Apologize for the delay, but couple of other stuff has pre-empted this.
I'll try the test today and let you know.

- Aravind.

Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.

[Aravind Gopalakrishnan]

On 9/5/14 4:11 AM, Wen Congyang wrote:
> In colo mode, secondary vm is running, so VM_ENTRY_INTR_INFO may
> valid before restoring vmcs. If there is no pending event after
> restoring vm, we should clear it.
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>
> Also clear pending software exceptions.
> Copy the fix to SVM as well.
>
> Signed-off-by: Tim Deegan <tim@xen.org>
>
> vmx part:
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> Acked-by: Kevin Tian <kevin.tian@intel.com>
>
> svm part:
> Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>   xen/arch/x86/hvm/svm/svm.c | 15 ++++++++-------
>   xen/arch/x86/hvm/vmx/vmx.c | 25 ++++++++++++-------------
>   2 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index b5188e6..053e511 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -321,16 +321,17 @@ static int svm_vmcb_restore(struct vcpu *v, struct hvm_hw_cpu *c)
>           vmcb_set_h_cr3(vmcb, pagetable_get_paddr(p2m_get_pagetable(p2m)));
>       }
>   
> -    if ( c->pending_valid )
> +    if ( c->pending_valid &&
> +         hvm_event_needs_reinjection(c->pending_type, c->pending_vector) )
>       {
>           gdprintk(XENLOG_INFO, "Re-injecting %#"PRIx32", %#"PRIx32"\n",
>                    c->pending_event, c->error_code);
> -
> -        if ( hvm_event_needs_reinjection(c->pending_type, c->pending_vector) )
> -        {
> -            vmcb->eventinj.bytes = c->pending_event;
> -            vmcb->eventinj.fields.errorcode = c->error_code;
> -        }
> +        vmcb->eventinj.bytes = c->pending_event;
> +        vmcb->eventinj.fields.errorcode = c->error_code;
> +    }
> +    else
> +    {
> +        vmcb->eventinj.bytes = 0;
>       }
>   
>       vmcb->cleanbits.bytes = 0;
>

Acked-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>

Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.

[Aravind Gopalakrishnan]

On 9/17/14 2:56 AM, Wen Congyang wrote:
> On 09/11/2014 06:35 PM, Tim Deegan wrote:
>> At 10:06 -0500 on 10 Sep (1410340016), Aravind Gopalakrishnan wrote:
>>> Hi,
>>> You mention that this 'fix' is just copied for svm. But you have not
>>> seen the problem of "VM_ENTRY_INTR_INFO may be valid" (whose svm
>>> equivalent is "vmcb->eventinj.bytes is valid").
>>> My concern is that we should test colo mode for svm first, since, if the
>>> problem is never really seen on svm, then fix may not be _necessary_
>> I think it's obvious by inspection of the code (and indeed the patch)
>> that SVM has the equivalent bug.
>>
>>> At this point, my problems are with test setups. I can help testing
>>> scenarios, but as Wen had mentioned, 'colo testing' might be the way to
>>> test.
>>> So, if I can get some pointers to how I can reproduce the issue at hand,
>>> then it would be very helpful.
>>>
>>> (Tim had mentioned we could try to simulate it by running a guest that
>>> takes lot of faults and save-restore another guest over it. However,
>>> I am not having much luck following this route. I got a hvm guest to
>>> continuously take SW exceptions on all vcpus and tried to save-restore.
>>> I can't see vmcb->eventinj.bytes containing any valid info.)
>> Although I think that getting COLO running on SVM is a gret idea, it
>> shouldn't block acceptance of this fix.  I've attached a program
>> that tests for the bug.  Run it against any HVM guest (and then
>> destroy the guest because the test will have corrupted its state).
> Hi, Aravind Gopalakrishnan
>
> Do you have time to test this patch with the test program?
>
Thanks for the test program Tim.
I tested it on a HVM guest and test prints 'good' w/ the patch applied 
and 'bad' w/o the patch.

Thanks,
-Aravind.

Re: [RFC Patch v3 04/18] blktap2: dynamic allocate aio_requests to avoid -EBUSY error

[Shriram Rajagopalan]

[-- Attachment #1.1: Type: text/plain, Size: 3694 bytes --]

On Sep 5, 2014 5:13 AM, "Wen Congyang" <wency@cn.fujitsu.com> wrote:
>
> From: Lai Jiangshan <laijs@cn.fujitsu.com>
>
> In normal case, there are at most TAPDISK_DATA_REQUESTS request
> at the same time. But in remus mode, the write requests are
> forwarded from the master side, and cached in block-remus. All
> cached requests will be forwarded to aio driver when syncing
> primary vm and backup vm. In this case, The number of requests
> may be more than TAPDISK_DATA_REQUESTS. So aio driver can't hanlde
> these requests at the same time, it will cause tapdisk2 exit.
>
> We don't know how many requests will be handled, so dynamic allocate
> aio_requests to avoid this error.
>
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> Signed-off-by: Jiang Yunhong <yunhong.jiang@intel.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
> ---
>  tools/blktap2/drivers/block-aio.c | 36
+++++++++++++++++++++++++++++++++---
>  1 file changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/tools/blktap2/drivers/block-aio.c
b/tools/blktap2/drivers/block-aio.c
> index f398da2..10ab20b 100644
> --- a/tools/blktap2/drivers/block-aio.c
> +++ b/tools/blktap2/drivers/block-aio.c
> @@ -55,9 +55,10 @@ struct tdaio_state {
>         int                  fd;
>         td_driver_t         *driver;
>
> +       int                  aio_max_count;
>         int                  aio_free_count;
>         struct aio_request   aio_requests[MAX_AIO_REQS];
> -       struct aio_request  *aio_free_list[MAX_AIO_REQS];
> +       struct aio_request   **aio_free_list;
>  };
>
>  /*Get Image size, secsize*/
> @@ -122,6 +123,11 @@ int tdaio_open(td_driver_t *driver, const char
*name, td_flag_t flags)
>
>         memset(prv, 0, sizeof(struct tdaio_state));
>
> +       prv->aio_free_list = malloc(MAX_AIO_REQS *
sizeof(*prv->aio_free_list));
> +       if (!prv->aio_free_list)
> +               return -ENOMEM;
> +
> +       prv->aio_max_count = MAX_AIO_REQS;
>         prv->aio_free_count = MAX_AIO_REQS;
>         for (i = 0; i < MAX_AIO_REQS; i++)
>                 prv->aio_free_list[i] = &prv->aio_requests[i];
> @@ -159,6 +165,28 @@ done:
>         return ret;
>  }
>
> +static int tdaio_refill(struct tdaio_state *prv)
> +{
> +       struct aio_request **new, *new_req;
> +       int i, max = prv->aio_max_count + MAX_AIO_REQS;
> +
> +       new = realloc(prv->aio_free_list, max *
sizeof(*prv->aio_free_list));
> +       if (!new)
> +               return -1;
> +       prv->aio_free_list = new;
> +
> +       new_req = calloc(MAX_AIO_REQS, sizeof(*new_req));
> +       if (!new_req)
> +               return -1;
> +
> +       prv->aio_max_count = max;
> +       prv->aio_free_count = MAX_AIO_REQS;
> +       for (i = 0; i < MAX_AIO_REQS; i++)
> +               prv->aio_free_list[i] = &new_req[i];
> +
> +       return 0;
> +}
> +
>  void tdaio_complete(void *arg, struct tiocb *tiocb, int err)
>  {
>         struct aio_request *aio = (struct aio_request *)arg;
> @@ -207,8 +235,10 @@ void tdaio_queue_write(td_driver_t *driver,
td_request_t treq)
>         size    = treq.secs * driver->info.sector_size;
>         offset  = treq.sec  * (uint64_t)driver->info.sector_size;
>
> -       if (prv->aio_free_count == 0)
> -               goto fail;
> +       if (prv->aio_free_count == 0) {
> +               if (tdaio_refill(prv) < 0)
> +                       goto fail;
> +       }
>
>         aio        = prv->aio_free_list[--prv->aio_free_count];
>         aio->treq  = treq;
> --
> 1.9.3
>

Hi
Sorry for the delay. The patch looks good to me.

Acked-by: Shriram Rajagopalan <rshriram@ <rshriram@cs.ubc.ca>cs.ubc.ca
<rshriram@cs.ubc.ca>>

[-- Attachment #1.2: Type: text/html, Size: 5201 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC Patch v3 12/18] block-remus: fix bug in tdremus_close()

[Shriram Rajagopalan]

[-- Attachment #1.1: Type: text/plain, Size: 7215 bytes --]

On Sep 5, 2014 5:16 AM, "Wen Congyang" <wency@cn.fujitsu.com> wrote:
>
> We close ctl_fd.fd, but we don't unregister ctl_fd.id. It will
> cause select() return fails, and the user cannot talk with
> tapdisk2.
>

I don't recall facing this issue upto Xen 4.1 at least. Clearly tapdisk2
code has changed a lot or the tools talking to tapdisk2 are using different
semantics.

Anyway, the code looks fine.
Acked-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>

> This patch also does some cleanup.
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
> ---
>  tools/blktap2/drivers/block-remus.c | 90
++++++++++++++++++++++---------------
>  1 file changed, 53 insertions(+), 37 deletions(-)
>
> diff --git a/tools/blktap2/drivers/block-remus.c
b/tools/blktap2/drivers/block-remus.c
> index 23a908a..55363a3 100644
> --- a/tools/blktap2/drivers/block-remus.c
> +++ b/tools/blktap2/drivers/block-remus.c
> @@ -151,9 +151,6 @@ typedef struct poll_fd {
>  } poll_fd_t;
>
>  struct tdremus_state {
> -//  struct tap_disk* driver;
> -       void* driver_data;
> -
>    /* XXX: this is needed so that the server can perform operations on
>     * the driver from the stream_fd event handler. fix this. */
>         td_driver_t *tdremus_driver;
> @@ -731,12 +728,26 @@ static int mwrite(int fd, void* buf, size_t len)
>
>  static void inline close_stream_fd(struct tdremus_state *s)
>  {
> +       if (s->stream_fd.fd < 0)
> +               return;
> +
>         /* XXX: -2 is magic. replace with macro perhaps? */
>         tapdisk_server_unregister_event(s->stream_fd.id);
>         close(s->stream_fd.fd);
>         s->stream_fd.fd = -2;
>  }
>
> +static void close_server_fd(struct tdremus_state *s)
> +{
> +       if (s->server_fd.fd < 0)
> +               return;
> +
> +       tapdisk_server_unregister_event(s->server_fd.id);
> +       s->server_fd.id = -1;
> +       close(s->stream_fd.fd);
> +       s->stream_fd.fd = -1;
> +}
> +
>  /* primary functions */
>  static void remus_client_event(event_id_t, char mode, void *private);
>  static void remus_connect_event(event_id_t id, char mode, void *private);
> @@ -1347,12 +1358,7 @@ static int unprotected_start(td_driver_t *driver)
>         /* close the server socket */
>         close_stream_fd(s);
>
> -       /* unregister the replication stream */
> -       tapdisk_server_unregister_event(s->server_fd.id);
> -
> -       /* close the replication stream */
> -       close(s->server_fd.fd);
> -       s->server_fd.fd = -1;
> +       close_server_fd(s);
>
>         /* install the unprotected read/write handlers */
>         tapdisk_remus.td_queue_read = unprotected_queue_read;
> @@ -1553,27 +1559,27 @@ static int ctl_open(td_driver_t *driver, const
char* name)
>                         s->ctl_path[i] = '_';
>         }
>         if (asprintf(&s->msg_path, "%s.msg", s->ctl_path) < 0)
> -               goto err_ctlfifo;
> +               goto err_setmsgfifo;
>
>         if (mkfifo(s->ctl_path, S_IRWXU|S_IRWXG|S_IRWXO) && errno !=
EEXIST) {
>                 RPRINTF("error creating control FIFO %s: %d\n",
s->ctl_path, errno);
> -               goto err_msgfifo;
> +               goto err_mkctlfifo;
>         }
>
>         if (mkfifo(s->msg_path, S_IRWXU|S_IRWXG|S_IRWXO) && errno !=
EEXIST) {
>                 RPRINTF("error creating message FIFO %s: %d\n",
s->msg_path, errno);
> -               goto err_msgfifo;
> +               goto err_mkmsgfifo;
>         }
>
>         /* RDWR so that fd doesn't block select when no writer is present
*/
>         if ((s->ctl_fd.fd = open(s->ctl_path, O_RDWR)) < 0) {
>                 RPRINTF("error opening control FIFO %s: %d\n",
s->ctl_path, errno);
> -               goto err_msgfifo;
> +               goto err_openctlfifo;
>         }
>
>         if ((s->msg_fd.fd = open(s->msg_path, O_RDWR)) < 0) {
>                 RPRINTF("error opening message FIFO %s: %d\n",
s->msg_path, errno);
> -               goto err_openctlfifo;
> +               goto err_openmsgfifo;
>         }
>
>         RPRINTF("control FIFO %s\n", s->ctl_path);
> @@ -1581,36 +1587,45 @@ static int ctl_open(td_driver_t *driver, const
char* name)
>
>         return 0;
>
> - err_openctlfifo:
> +err_openmsgfifo:
>         close(s->ctl_fd.fd);
> - err_msgfifo:
> +       s->ctl_fd.fd = -1;
> +err_openctlfifo:
> +       unlink(s->ctl_path);
> +err_mkmsgfifo:
> +       unlink(s->msg_path);
> +err_mkctlfifo:
>         free(s->msg_path);
>         s->msg_path = NULL;
> - err_ctlfifo:
> +err_setmsgfifo:
>         free(s->ctl_path);
>         s->ctl_path = NULL;
>         return -1;
>  }
>
> -static void ctl_close(td_driver_t *driver)
> +static void ctl_close(struct tdremus_state *s)
>  {
> -       struct tdremus_state *s = (struct tdremus_state *)driver->data;
> -
> -       /* TODO: close *all* connections */
> -
> -       if(s->ctl_fd.fd)
> +       if(s->ctl_fd.fd) {
>                 close(s->ctl_fd.fd);
> +               s->ctl_fd.fd = -1;
> +       }
>
>         if (s->ctl_path) {
>                 unlink(s->ctl_path);
>                 free(s->ctl_path);
>                 s->ctl_path = NULL;
>         }
> +
>         if (s->msg_path) {
>                 unlink(s->msg_path);
>                 free(s->msg_path);
>                 s->msg_path = NULL;
>         }
> +
> +       if (s->msg_fd.fd) {
> +               close(s->msg_fd.fd);
> +               s->msg_fd.fd = -1;
> +       }
>  }
>
>  static int ctl_register(struct tdremus_state *s)
> @@ -1628,6 +1643,16 @@ static int ctl_register(struct tdremus_state *s)
>         return 0;
>  }
>
> +static void ctl_unregister(struct tdremus_state *s)
> +{
> +       RPRINTF("unregistering ctl fifo\n");
> +
> +       if (s->ctl_fd.id >= 0) {
> +               tapdisk_server_unregister_event(s->ctl_fd.id);
> +               s->ctl_fd.id = -1;
> +       }
> +}
> +
>  /* interface */
>
>  static int tdremus_open(td_driver_t *driver, td_image_t *image,
td_uuid_t uuid)
> @@ -1658,13 +1683,12 @@ static int tdremus_open(td_driver_t *driver,
td_image_t *image, td_uuid_t uuid)
>
>         if ((rc = ctl_open(driver, name))) {
>                 RPRINTF("error setting up control channel\n");
> -               free(s->driver_data);
>                 return rc;
>         }
>
>         if ((rc = ctl_register(s))) {
>                 RPRINTF("error registering control channel\n");
> -               free(s->driver_data);
> +               ctl_close(s);
>                 return rc;
>         }
>
> @@ -1687,19 +1711,11 @@ static int tdremus_close(td_driver_t *driver)
>         RPRINTF("closing\n");
>         if (s->ramdisk.inprogress)
>                 hashtable_destroy(s->ramdisk.inprogress, 0);
> -
> -       if (s->driver_data) {
> -               free(s->driver_data);
> -               s->driver_data = NULL;
> -       }
> -       if (s->server_fd.fd >= 0) {
> -               close(s->server_fd.fd);
> -               s->server_fd.fd = -1;
> -       }
> -       if (s->stream_fd.fd >= 0)
> -               close_stream_fd(s);
>
> -       ctl_close(driver);
> +       close_server_fd(s);
> +       close_stream_fd(s);
> +       ctl_unregister(s);
> +       ctl_close(s);
>
>         return 0;
>  }
> --
> 1.9.3
>

[-- Attachment #1.2: Type: text/html, Size: 10492 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC Patch v3 11/18] block-remus: use correct way to get remus_image

[Shriram Rajagopalan]

[-- Attachment #1.1: Type: text/plain, Size: 9125 bytes --]

On Sep 5, 2014 5:15 AM, "Wen Congyang" <wency@cn.fujitsu.com> wrote:
>
> We set remus_image in backup_read(). If we do flush
> before the first read operation, remus_image will be
> NULL. Pass image to remus via the callback td_open().
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
> ---
>  tools/blktap2/drivers/block-aio.c         | 6 ++++--
>  tools/blktap2/drivers/block-cache.c       | 5 +++--
>  tools/blktap2/drivers/block-log.c         | 5 +++--
>  tools/blktap2/drivers/block-qcow.c        | 6 ++++--
>  tools/blktap2/drivers/block-ram.c         | 6 ++++--
>  tools/blktap2/drivers/block-remus.c       | 8 ++++----
>  tools/blktap2/drivers/block-vhd.c         | 6 ++++--
>  tools/blktap2/drivers/tapdisk-interface.c | 3 +--
>  tools/blktap2/drivers/tapdisk.h           | 2 +-
>  9 files changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/tools/blktap2/drivers/block-aio.c
b/tools/blktap2/drivers/block-aio.c
> index 1b560e5..27ba07d 100644
> --- a/tools/blktap2/drivers/block-aio.c
> +++ b/tools/blktap2/drivers/block-aio.c
> @@ -40,6 +40,7 @@
>  #include "tapdisk.h"
>  #include "tapdisk-driver.h"
>  #include "tapdisk-interface.h"
> +#include "tapdisk-image.h"
>
>  #define MAX_AIO_REQS         TAPDISK_DATA_REQUESTS
>
> @@ -111,11 +112,12 @@ static int tdaio_get_image_info(int fd,
td_disk_info_t *info)
>  }
>
>  /* Open the disk file and initialize aio state. */
> -int tdaio_open(td_driver_t *driver, const char *name, td_flag_t flags,
> -              td_uuid_t uuid)
> +int tdaio_open(td_driver_t *driver, td_image_t *image, td_uuid_t uuid)
>  {
>         int i, fd, ret, o_flags;
>         struct tdaio_state *prv;
> +       const char *name = image->name;
> +       td_flag_t flags = image->flags;
>
>         ret = 0;
>         prv = (struct tdaio_state *)driver->data;
> diff --git a/tools/blktap2/drivers/block-cache.c
b/tools/blktap2/drivers/block-cache.c
> index cd6ea6a..ff2c773 100644
> --- a/tools/blktap2/drivers/block-cache.c
> +++ b/tools/blktap2/drivers/block-cache.c
> @@ -517,12 +517,13 @@ block_cache_put_request(block_cache_t *cache,
block_cache_request_t *breq)
>  }
>
>  static int
> -block_cache_open(td_driver_t *driver, const char *name, td_flag_t flags,
> -                td_uuid_t uuid)
> +block_cache_open(td_driver_t *driver, td_image_t *image, td_uuid_t uuid)
>  {
>         int i, err;
>         radix_tree_t *tree;
>         block_cache_t *cache;
> +       const char *name = image->name;
> +       td_flag_t flags = image->flags;
>
>         if (!td_flag_test(flags, TD_OPEN_RDONLY))
>                 return -EINVAL;
> diff --git a/tools/blktap2/drivers/block-log.c
b/tools/blktap2/drivers/block-log.c
> index 7b33b63..80351d3 100644
> --- a/tools/blktap2/drivers/block-log.c
> +++ b/tools/blktap2/drivers/block-log.c
> @@ -585,11 +585,12 @@ static void ctl_request(event_id_t id, char mode,
void *private)
>
>  static int tdlog_close(td_driver_t*);
>
> -static int tdlog_open(td_driver_t* driver, const char* name, td_flag_t
flags,
> -                     td_uuid_t uuid)
> +static int tdlog_open(td_driver_t* driver, td_image_t *image, td_uuid_t
uuid)
>  {
>    struct tdlog_state* s = (struct tdlog_state*)driver->data;
>    int rc;
> +  const char *name = image->name;
> +  td_flag_t flags = image->flags;
>
>    memset(s, 0, sizeof(*s));
>
> diff --git a/tools/blktap2/drivers/block-qcow.c
b/tools/blktap2/drivers/block-qcow.c
> index 64dfafc..c63bd9d 100644
> --- a/tools/blktap2/drivers/block-qcow.c
> +++ b/tools/blktap2/drivers/block-qcow.c
> @@ -45,6 +45,7 @@
>  #include "qcow.h"
>  #include "blk.h"
>  #include "atomicio.h"
> +#include "tapdisk-image.h"
>
>  /* *BSD has no O_LARGEFILE */
>  #ifndef O_LARGEFILE
> @@ -865,14 +866,15 @@ out:
>  }
>
>  /* Open the disk file and initialize qcow state. */
> -int tdqcow_open (td_driver_t *driver, const char *name, td_flag_t flags,
> -                td_uuid_t uuid)
> +int tdqcow_open (td_driver_t *driver, td_image_t *image, td_uuid_t uuid)
>  {
>         int fd, len, i, ret, size, o_flags;
>         td_disk_info_t *bs = &(driver->info);
>         struct tdqcow_state   *s  = (struct tdqcow_state *)driver->data;
>         QCowHeader header;
>         uint64_t final_cluster = 0;
> +       const char *name = image->name;
> +       td_flag_t flags = image->flags;
>
>         DPRINTF("QCOW: Opening %s\n", name);
>
> diff --git a/tools/blktap2/drivers/block-ram.c
b/tools/blktap2/drivers/block-ram.c
> index b64a194..3e148ab 100644
> --- a/tools/blktap2/drivers/block-ram.c
> +++ b/tools/blktap2/drivers/block-ram.c
> @@ -40,6 +40,7 @@
>  #include "tapdisk.h"
>  #include "tapdisk-driver.h"
>  #include "tapdisk-interface.h"
> +#include "tapdisk-image.h"
>
>  char *img;
>  long int   disksector_size;
> @@ -108,13 +109,14 @@ static int get_image_info(int fd, td_disk_info_t
*info)
>  }
>
>  /* Open the disk file and initialize ram state. */
> -int tdram_open (td_driver_t *driver, const char *name, td_flag_t flags,
> -               td_uuid_t uuid)
> +int tdram_open (td_driver_t *driver, td_image_t *image, td_uuid_t uuid)
>  {
>         char *p;
>         uint64_t size;
>         int i, fd, ret = 0, count = 0, o_flags;
>         struct tdram_state *prv = (struct tdram_state *)driver->data;
> +       const char *name = image->name;
> +       td_flag_t flags = image->flags;
>
>         connections++;
>
> diff --git a/tools/blktap2/drivers/block-remus.c
b/tools/blktap2/drivers/block-remus.c
> index 504f6b4..23a908a 100644
> --- a/tools/blktap2/drivers/block-remus.c
> +++ b/tools/blktap2/drivers/block-remus.c
> @@ -1152,8 +1152,6 @@ void backup_queue_read(td_driver_t *driver,
td_request_t treq)
>  {
>         struct tdremus_state *s = (struct tdremus_state *)driver->data;
>         int i;
> -       if(!remus_image)
> -               remus_image = treq.image;
>
>         /* check if this read is queued in any currently ongoing flush */
>         if (ramdisk_read(&s->ramdisk, treq.sec, treq.secs, treq.buf)) {
> @@ -1632,15 +1630,17 @@ static int ctl_register(struct tdremus_state *s)
>
>  /* interface */
>
> -static int tdremus_open(td_driver_t *driver, const char *name,
> -                       td_flag_t flags, td_uuid_t uuid)
> +static int tdremus_open(td_driver_t *driver, td_image_t *image,
td_uuid_t uuid)
>  {
>         struct tdremus_state *s = (struct tdremus_state *)driver->data;
>         int rc;
> +       const char *name = image->name;
> +       td_flag_t flags = image->flags;
>
>         RPRINTF("opening %s\n", name);
>
>         device_vbd = tapdisk_server_get_vbd(uuid);
> +       remus_image = image;
>
>         memset(s, 0, sizeof(*s));
>         s->server_fd.fd = -1;
> diff --git a/tools/blktap2/drivers/block-vhd.c
b/tools/blktap2/drivers/block-vhd.c
> index 06e9c89..b20f724 100644
> --- a/tools/blktap2/drivers/block-vhd.c
> +++ b/tools/blktap2/drivers/block-vhd.c
> @@ -59,6 +59,7 @@
>  #include "tapdisk-driver.h"
>  #include "tapdisk-interface.h"
>  #include "tapdisk-disktype.h"
> +#include "tapdisk-image.h"
>
>  unsigned int SPB;
>
> @@ -675,10 +676,11 @@ __vhd_open(td_driver_t *driver, const char *name,
vhd_flag_t flags)
>  }
>
>  static int
> -_vhd_open(td_driver_t *driver, const char *name, td_flag_t flags,
> -         td_uuid_t uuid)
> +_vhd_open(td_driver_t *driver, td_image_t *image, td_uuid_t uuid)
>  {
>         vhd_flag_t vhd_flags = 0;
> +       const char *name = image->name;
> +       td_flag_t flags = image->flags;
>
>         if (flags & TD_OPEN_RDONLY)
>                 vhd_flags |= VHD_FLAG_OPEN_RDONLY;
> diff --git a/tools/blktap2/drivers/tapdisk-interface.c
b/tools/blktap2/drivers/tapdisk-interface.c
> index 36b5393..a29de64 100644
> --- a/tools/blktap2/drivers/tapdisk-interface.c
> +++ b/tools/blktap2/drivers/tapdisk-interface.c
> @@ -79,8 +79,7 @@ __td_open(td_image_t *image, td_disk_info_t *info)
>         }
>
>         if (!td_flag_test(driver->state, TD_DRIVER_OPEN)) {
> -               err = driver->ops->td_open(driver, image->name,
image->flags,
> -                                          vbd->uuid);
> +               err = driver->ops->td_open(driver, image, vbd->uuid);
>                 if (err) {
>                         if (!image->driver)
>                                 tapdisk_driver_free(driver);
> diff --git a/tools/blktap2/drivers/tapdisk.h
b/tools/blktap2/drivers/tapdisk.h
> index 459eaec..3c3b51d 100644
> --- a/tools/blktap2/drivers/tapdisk.h
> +++ b/tools/blktap2/drivers/tapdisk.h
> @@ -157,7 +157,7 @@ struct tap_disk {
>         const char                  *disk_type;
>         td_flag_t                    flags;
>         int                          private_data_size;
> -       int (*td_open)               (td_driver_t *, const char *,
td_flag_t, td_uuid_t);
> +       int (*td_open)               (td_driver_t *, td_image_t *,
td_uuid_t);
>         int (*td_close)              (td_driver_t *);
>         int (*td_get_parent_id)      (td_driver_t *, td_disk_id_t *);
>         int (*td_validate_parent)    (td_driver_t *, td_driver_t *,
td_flag_t);
> --
> 1.9.3
>

Acked-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>

[-- Attachment #1.2: Type: text/html, Size: 12059 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC Patch v3 10/18] block-remus: pass uuid to the callback td_open

[Shriram Rajagopalan]

[-- Attachment #1.1: Type: text/plain, Size: 7141 bytes --]

On Sep 5, 2014 5:15 AM, "Wen Congyang" <wency@cn.fujitsu.com> wrote:
>
> remus's callback td_open needs uuid, but it is hard coded as 0.
> After commit 4b1af8, the vbd's uuid is the minor of the blktap
> device, not 0.
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
> ---
>  tools/blktap2/drivers/block-aio.c         | 3 ++-
>  tools/blktap2/drivers/block-cache.c       | 3 ++-
>  tools/blktap2/drivers/block-log.c         | 3 ++-
>  tools/blktap2/drivers/block-qcow.c        | 3 ++-
>  tools/blktap2/drivers/block-ram.c         | 3 ++-
>  tools/blktap2/drivers/block-remus.c       | 8 ++------
>  tools/blktap2/drivers/block-vhd.c         | 3 ++-
>  tools/blktap2/drivers/tapdisk-interface.c | 4 +++-
>  tools/blktap2/drivers/tapdisk.h           | 2 +-
>  9 files changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/tools/blktap2/drivers/block-aio.c
b/tools/blktap2/drivers/block-aio.c
> index 10ab20b..1b560e5 100644
> --- a/tools/blktap2/drivers/block-aio.c
> +++ b/tools/blktap2/drivers/block-aio.c
> @@ -111,7 +111,8 @@ static int tdaio_get_image_info(int fd,
td_disk_info_t *info)
>  }
>
>  /* Open the disk file and initialize aio state. */
> -int tdaio_open(td_driver_t *driver, const char *name, td_flag_t flags)
> +int tdaio_open(td_driver_t *driver, const char *name, td_flag_t flags,
> +              td_uuid_t uuid)
>  {
>         int i, fd, ret, o_flags;
>         struct tdaio_state *prv;
> diff --git a/tools/blktap2/drivers/block-cache.c
b/tools/blktap2/drivers/block-cache.c
> index 1d2f4eb..cd6ea6a 100644
> --- a/tools/blktap2/drivers/block-cache.c
> +++ b/tools/blktap2/drivers/block-cache.c
> @@ -517,7 +517,8 @@ block_cache_put_request(block_cache_t *cache,
block_cache_request_t *breq)
>  }
>
>  static int
> -block_cache_open(td_driver_t *driver, const char *name, td_flag_t flags)
> +block_cache_open(td_driver_t *driver, const char *name, td_flag_t flags,
> +                td_uuid_t uuid)
>  {
>         int i, err;
>         radix_tree_t *tree;
> diff --git a/tools/blktap2/drivers/block-log.c
b/tools/blktap2/drivers/block-log.c
> index 5330cdc..7b33b63 100644
> --- a/tools/blktap2/drivers/block-log.c
> +++ b/tools/blktap2/drivers/block-log.c
> @@ -585,7 +585,8 @@ static void ctl_request(event_id_t id, char mode,
void *private)
>
>  static int tdlog_close(td_driver_t*);
>
> -static int tdlog_open(td_driver_t* driver, const char* name, td_flag_t
flags)
> +static int tdlog_open(td_driver_t* driver, const char* name, td_flag_t
flags,
> +                     td_uuid_t uuid)
>  {
>    struct tdlog_state* s = (struct tdlog_state*)driver->data;
>    int rc;
> diff --git a/tools/blktap2/drivers/block-qcow.c
b/tools/blktap2/drivers/block-qcow.c
> index b45bcaa..64dfafc 100644
> --- a/tools/blktap2/drivers/block-qcow.c
> +++ b/tools/blktap2/drivers/block-qcow.c
> @@ -865,7 +865,8 @@ out:
>  }
>
>  /* Open the disk file and initialize qcow state. */
> -int tdqcow_open (td_driver_t *driver, const char *name, td_flag_t flags)
> +int tdqcow_open (td_driver_t *driver, const char *name, td_flag_t flags,
> +                td_uuid_t uuid)
>  {
>         int fd, len, i, ret, size, o_flags;
>         td_disk_info_t *bs = &(driver->info);
> diff --git a/tools/blktap2/drivers/block-ram.c
b/tools/blktap2/drivers/block-ram.c
> index a859481..b64a194 100644
> --- a/tools/blktap2/drivers/block-ram.c
> +++ b/tools/blktap2/drivers/block-ram.c
> @@ -108,7 +108,8 @@ static int get_image_info(int fd, td_disk_info_t
*info)
>  }
>
>  /* Open the disk file and initialize ram state. */
> -int tdram_open (td_driver_t *driver, const char *name, td_flag_t flags)
> +int tdram_open (td_driver_t *driver, const char *name, td_flag_t flags,
> +               td_uuid_t uuid)
>  {
>         char *p;
>         uint64_t size;
> diff --git a/tools/blktap2/drivers/block-remus.c
b/tools/blktap2/drivers/block-remus.c
> index 4ce9dbe..504f6b4 100644
> --- a/tools/blktap2/drivers/block-remus.c
> +++ b/tools/blktap2/drivers/block-remus.c
> @@ -1633,18 +1633,14 @@ static int ctl_register(struct tdremus_state *s)
>  /* interface */
>
>  static int tdremus_open(td_driver_t *driver, const char *name,
> -                       td_flag_t flags)
> +                       td_flag_t flags, td_uuid_t uuid)
>  {
>         struct tdremus_state *s = (struct tdremus_state *)driver->data;
>         int rc;
>
>         RPRINTF("opening %s\n", name);
>
> -       /* first we need to get the underlying vbd for this driver stack.
To do so we
> -        * need to know the vbd's id. Fortunately, for tapdisk2 this is
hard-coded as
> -        * 0 (see tapdisk2.c)
> -        */
> -       device_vbd = tapdisk_server_get_vbd(0);
> +       device_vbd = tapdisk_server_get_vbd(uuid);
>
>         memset(s, 0, sizeof(*s));
>         s->server_fd.fd = -1;
> diff --git a/tools/blktap2/drivers/block-vhd.c
b/tools/blktap2/drivers/block-vhd.c
> index 76ea5bd..06e9c89 100644
> --- a/tools/blktap2/drivers/block-vhd.c
> +++ b/tools/blktap2/drivers/block-vhd.c
> @@ -675,7 +675,8 @@ __vhd_open(td_driver_t *driver, const char *name,
vhd_flag_t flags)
>  }
>
>  static int
> -_vhd_open(td_driver_t *driver, const char *name, td_flag_t flags)
> +_vhd_open(td_driver_t *driver, const char *name, td_flag_t flags,
> +         td_uuid_t uuid)
>  {
>         vhd_flag_t vhd_flags = 0;
>
> diff --git a/tools/blktap2/drivers/tapdisk-interface.c
b/tools/blktap2/drivers/tapdisk-interface.c
> index 2e51883..36b5393 100644
> --- a/tools/blktap2/drivers/tapdisk-interface.c
> +++ b/tools/blktap2/drivers/tapdisk-interface.c
> @@ -63,6 +63,7 @@ __td_open(td_image_t *image, td_disk_info_t *info)
>  {
>         int err;
>         td_driver_t *driver;
> +       td_vbd_t *vbd = image->private;
>
>         driver = image->driver;
>         if (!driver) {
> @@ -78,7 +79,8 @@ __td_open(td_image_t *image, td_disk_info_t *info)
>         }
>
>         if (!td_flag_test(driver->state, TD_DRIVER_OPEN)) {
> -               err = driver->ops->td_open(driver, image->name,
image->flags);
> +               err = driver->ops->td_open(driver, image->name,
image->flags,
> +                                          vbd->uuid);
>                 if (err) {
>                         if (!image->driver)
>                                 tapdisk_driver_free(driver);
> diff --git a/tools/blktap2/drivers/tapdisk.h
b/tools/blktap2/drivers/tapdisk.h
> index 66d508e..459eaec 100644
> --- a/tools/blktap2/drivers/tapdisk.h
> +++ b/tools/blktap2/drivers/tapdisk.h
> @@ -157,7 +157,7 @@ struct tap_disk {
>         const char                  *disk_type;
>         td_flag_t                    flags;
>         int                          private_data_size;
> -       int (*td_open)               (td_driver_t *, const char *,
td_flag_t);
> +       int (*td_open)               (td_driver_t *, const char *,
td_flag_t, td_uuid_t);
>         int (*td_close)              (td_driver_t *);
>         int (*td_get_parent_id)      (td_driver_t *, td_disk_id_t *);
>         int (*td_validate_parent)    (td_driver_t *, td_driver_t *,
td_flag_t);
> --
> 1.9.3
>

Acked-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>

[-- Attachment #1.2: Type: text/html, Size: 9196 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC Patch v3 09/18] block-remus: fix memory leak

[Shriram Rajagopalan]

[-- Attachment #1.1: Type: text/plain, Size: 1910 bytes --]

On Sep 5, 2014 5:15 AM, "Wen Congyang" <wency@cn.fujitsu.com> wrote:
>
> Fix the following two memory leak:
> 1. If s->ramdisk.prev is not NULL, we merge the write requests in
>    s->ramdisk.h into s->ramdisk.prev, and then destroy s->ramdisk.h.
>    But we forget to free hash value when destroying s->ramdisk.h.

I am responding from my phone. So I don't have the code in hand to validate
your claim. I think the merge function reuses the value block (write
request) instead of doing a memcpy. In which case, this patch will be
freeing the buffer that is queued for write. Is that right?

> 2. When write requests is finished, replicated_write_callback() will
>    be called. We forget free the buff in this function.

Wasn't that done explicitly in the write req done function, where a
free(buf) was introduced? (Vague recollection... I am not sure if I pushed
that fix upstream either :( )

Either way, if you have a working Remus setup, can you confirm that this
patch does not cause double free error? (Just this patch and no other Remus
related fixes).

>
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> Signed-off-by: Jiang Yunhong <yunhong.jiang@intel.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
> ---
>  tools/blktap2/drivers/block-remus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/blktap2/drivers/block-remus.c
b/tools/blktap2/drivers/block-remus.c
> index 079588d..4ce9dbe 100644
> --- a/tools/blktap2/drivers/block-remus.c
> +++ b/tools/blktap2/drivers/block-remus.c
> @@ -602,7 +602,7 @@ static int ramdisk_start_flush(td_driver_t *driver)
>                 }
>                 free(sectors);
>
> -               hashtable_destroy (s->ramdisk.h, 0);
> +               hashtable_destroy (s->ramdisk.h, 1);
>         } else
>                 s->ramdisk.prev = s->ramdisk.h;
>
> --
> 1.9.3
>

[-- Attachment #1.2: Type: text/html, Size: 2597 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC Patch v3 09/18] block-remus: fix memory leak

[Wen Congyang]

On 09/25/2014 03:37 AM, Shriram Rajagopalan wrote:
> On Sep 5, 2014 5:15 AM, "Wen Congyang" <wency@cn.fujitsu.com> wrote:
>>
>> Fix the following two memory leak:
>> 1. If s->ramdisk.prev is not NULL, we merge the write requests in
>>    s->ramdisk.h into s->ramdisk.prev, and then destroy s->ramdisk.h.
>>    But we forget to free hash value when destroying s->ramdisk.h.
> 
> I am responding from my phone. So I don't have the code in hand to validate
> your claim. I think the merge function reuses the value block (write
> request) instead of doing a memcpy. In which case, this patch will be
> freeing the buffer that is queued for write. Is that right?

No, if we have cached the sector in s->ramdisk.prev, we just use memcpy.
Otherwise, we alloc memory and use memcpy. Why we don't reuse the buff?
Because the buff may be in the stack...

> 
>> 2. When write requests is finished, replicated_write_callback() will
>>    be called. We forget free the buff in this function.
> 
> Wasn't that done explicitly in the write req done function, where a
> free(buf) was introduced? (Vague recollection... I am not sure if I pushed
> that fix upstream either :( )

Sorry, I forgot to update the commit message. Bug 2 has been fixed...

> 
> Either way, if you have a working Remus setup, can you confirm that this
> patch does not cause double free error? (Just this patch and no other Remus
> related fixes).

Hmm, I don't test it for newest xen, because only drbd is supported now.

Thanks
Wen COngyang

> 
>>
>> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
>> Signed-off-by: Jiang Yunhong <yunhong.jiang@intel.com>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
>> ---
>>  tools/blktap2/drivers/block-remus.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/blktap2/drivers/block-remus.c
> b/tools/blktap2/drivers/block-remus.c
>> index 079588d..4ce9dbe 100644
>> --- a/tools/blktap2/drivers/block-remus.c
>> +++ b/tools/blktap2/drivers/block-remus.c
>> @@ -602,7 +602,7 @@ static int ramdisk_start_flush(td_driver_t *driver)
>>                 }
>>                 free(sectors);
>>
>> -               hashtable_destroy (s->ramdisk.h, 0);
>> +               hashtable_destroy (s->ramdisk.h, 1);
>>         } else
>>                 s->ramdisk.prev = s->ramdisk.h;
>>
>> --
>> 1.9.3
>>
> 

Re: [RFC Patch v3 09/18] block-remus: fix memory leak

[Shriram Rajagopalan]

[-- Attachment #1.1: Type: text/plain, Size: 2660 bytes --]

On Sep 25, 2014 1:22 AM, "Wen Congyang" <wency@cn.fujitsu.com> wrote:
>
> On 09/25/2014 03:37 AM, Shriram Rajagopalan wrote:
> > On Sep 5, 2014 5:15 AM, "Wen Congyang" <wency@cn.fujitsu.com> wrote:
> >>
> >> Fix the following two memory leak:
> >> 1. If s->ramdisk.prev is not NULL, we merge the write requests in
> >>    s->ramdisk.h into s->ramdisk.prev, and then destroy s->ramdisk.h.
> >>    But we forget to free hash value when destroying s->ramdisk.h.
> >
> > I am responding from my phone. So I don't have the code in hand to
validate
> > your claim. I think the merge function reuses the value block (write
> > request) instead of doing a memcpy. In which case, this patch will be
> > freeing the buffer that is queued for write. Is that right?
>
> No, if we have cached the sector in s->ramdisk.prev, we just use memcpy.
> Otherwise, we alloc memory and use memcpy. Why we don't reuse the buff?
> Because the buff may be in the stack...
>
> >
> >> 2. When write requests is finished, replicated_write_callback() will
> >>    be called. We forget free the buff in this function.
> >
> > Wasn't that done explicitly in the write req done function, where a
> > free(buf) was introduced? (Vague recollection... I am not sure if I
pushed
> > that fix upstream either :( )
>
> Sorry, I forgot to update the commit message. Bug 2 has been fixed...
>
> >
> > Either way, if you have a working Remus setup, can you confirm that this
> > patch does not cause double free error? (Just this patch and no other
Remus
> > related fixes).
>
> Hmm, I don't test it for newest xen, because only drbd is supported now.
>

Since this is solely blktap2 related, any older version of Xen w/ Remus
would do.

> Thanks
> Wen COngyang
>
> >
> >>
> >> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> >> Signed-off-by: Jiang Yunhong <yunhong.jiang@intel.com>
> >> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> >> Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
> >> ---
> >>  tools/blktap2/drivers/block-remus.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/tools/blktap2/drivers/block-remus.c
> > b/tools/blktap2/drivers/block-remus.c
> >> index 079588d..4ce9dbe 100644
> >> --- a/tools/blktap2/drivers/block-remus.c
> >> +++ b/tools/blktap2/drivers/block-remus.c
> >> @@ -602,7 +602,7 @@ static int ramdisk_start_flush(td_driver_t *driver)
> >>                 }
> >>                 free(sectors);
> >>
> >> -               hashtable_destroy (s->ramdisk.h, 0);
> >> +               hashtable_destroy (s->ramdisk.h, 1);
> >>         } else
> >>                 s->ramdisk.prev = s->ramdisk.h;
> >>
> >> --
> >> 1.9.3
> >>
> >
>

[-- Attachment #1.2: Type: text/html, Size: 3906 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [RFC Patch v3 09/18] block-remus: fix memory leak

[Wen Congyang]

On 09/25/2014 07:14 PM, Shriram Rajagopalan wrote:
> On Sep 25, 2014 1:22 AM, "Wen Congyang" <wency@cn.fujitsu.com> wrote:
>>
>> On 09/25/2014 03:37 AM, Shriram Rajagopalan wrote:
>>> On Sep 5, 2014 5:15 AM, "Wen Congyang" <wency@cn.fujitsu.com> wrote:
>>>>
>>>> Fix the following two memory leak:
>>>> 1. If s->ramdisk.prev is not NULL, we merge the write requests in
>>>>    s->ramdisk.h into s->ramdisk.prev, and then destroy s->ramdisk.h.
>>>>    But we forget to free hash value when destroying s->ramdisk.h.
>>>
>>> I am responding from my phone. So I don't have the code in hand to
> validate
>>> your claim. I think the merge function reuses the value block (write
>>> request) instead of doing a memcpy. In which case, this patch will be
>>> freeing the buffer that is queued for write. Is that right?
>>
>> No, if we have cached the sector in s->ramdisk.prev, we just use memcpy.
>> Otherwise, we alloc memory and use memcpy. Why we don't reuse the buff?
>> Because the buff may be in the stack...
>>
>>>
>>>> 2. When write requests is finished, replicated_write_callback() will
>>>>    be called. We forget free the buff in this function.
>>>
>>> Wasn't that done explicitly in the write req done function, where a
>>> free(buf) was introduced? (Vague recollection... I am not sure if I
> pushed
>>> that fix upstream either :( )
>>
>> Sorry, I forgot to update the commit message. Bug 2 has been fixed...
>>
>>>
>>> Either way, if you have a working Remus setup, can you confirm that this
>>> patch does not cause double free error? (Just this patch and no other
> Remus
>>> related fixes).
>>
>> Hmm, I don't test it for newest xen, because only drbd is supported now.
>>
> 
> Since this is solely blktap2 related, any older version of Xen w/ Remus
> would do.

We find this bug in older version of Xen. We run pgbench to test the
performance, and it will cause OOM because tapdisk uses too many memory...

I don't test it for newest xen, but I think it's OK according to the codes

> 
>> Thanks
>> Wen COngyang
>>
>>>
>>>>
>>>> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
>>>> Signed-off-by: Jiang Yunhong <yunhong.jiang@intel.com>
>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>> Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
>>>> ---
>>>>  tools/blktap2/drivers/block-remus.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/blktap2/drivers/block-remus.c
>>> b/tools/blktap2/drivers/block-remus.c
>>>> index 079588d..4ce9dbe 100644
>>>> --- a/tools/blktap2/drivers/block-remus.c
>>>> +++ b/tools/blktap2/drivers/block-remus.c
>>>> @@ -602,7 +602,7 @@ static int ramdisk_start_flush(td_driver_t *driver)
>>>>                 }
>>>>                 free(sectors);
>>>>
>>>> -               hashtable_destroy (s->ramdisk.h, 0);
>>>> +               hashtable_destroy (s->ramdisk.h, 1);
>>>>         } else
>>>>                 s->ramdisk.prev = s->ramdisk.h;
>>>>
>>>> --
>>>> 1.9.3
>>>>
>>>
>>
>