* [RFC Patch v3 01/18] copy the correct page to memory
2014-09-05 9:10 [RFC Patch v3 00/18] Some bugfix patches Wen Congyang
@ 2014-09-05 9:10 ` Wen Congyang
2014-09-08 11:27 ` Ian Campbell
2014-09-05 9:10 ` [RFC Patch v3 02/18] csum the correct page Wen Congyang
` (16 subsequent siblings)
17 siblings, 1 reply; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:10 UTC (permalink / raw)
To: xen devel
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, Hong Tao, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 01/18] copy the correct page to memory
2014-09-05 9:10 ` [RFC Patch v3 01/18] copy the correct page to memory Wen Congyang
@ 2014-09-08 11:27 ` Ian Campbell
2014-09-08 11:58 ` Andrew Cooper
0 siblings, 1 reply; 57+ messages in thread
From: Ian Campbell @ 2014-09-08 11:27 UTC (permalink / raw)
To: Wen Congyang, Andrew Cooper
Cc: Lai Jiangshan, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Hong Tao, Yang Hongyang
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;
>
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 01/18] copy the correct page to memory
2014-09-08 11:27 ` Ian Campbell
@ 2014-09-08 11:58 ` Andrew Cooper
0 siblings, 0 replies; 57+ messages in thread
From: Andrew Cooper @ 2014-09-08 11:58 UTC (permalink / raw)
To: Ian Campbell, Wen Congyang
Cc: Lai Jiangshan, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Hong Tao, Yang Hongyang
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
^ permalink raw reply [flat|nested] 57+ messages in thread
* [RFC Patch v3 02/18] csum the correct page
2014-09-05 9:10 [RFC Patch v3 00/18] Some bugfix patches Wen Congyang
2014-09-05 9:10 ` [RFC Patch v3 01/18] copy the correct page to memory Wen Congyang
@ 2014-09-05 9:10 ` Wen Congyang
2014-09-08 11:28 ` Ian Campbell
2014-09-05 9:10 ` [RFC Patch v3 03/18] don't zero out ioreq page Wen Congyang
` (15 subsequent siblings)
17 siblings, 1 reply; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:10 UTC (permalink / raw)
To: xen devel
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 02/18] csum the correct page
2014-09-05 9:10 ` [RFC Patch v3 02/18] csum the correct page Wen Congyang
@ 2014-09-08 11:28 ` Ian Campbell
0 siblings, 0 replies; 57+ messages in thread
From: Ian Campbell @ 2014-09-08 11:28 UTC (permalink / raw)
To: Wen Congyang
Cc: Lai Jiangshan, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Yang Hongyang
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++ )
^ permalink raw reply [flat|nested] 57+ messages in thread
* [RFC Patch v3 03/18] don't zero out ioreq page
2014-09-05 9:10 [RFC Patch v3 00/18] Some bugfix patches Wen Congyang
2014-09-05 9:10 ` [RFC Patch v3 01/18] copy the correct page to memory Wen Congyang
2014-09-05 9:10 ` [RFC Patch v3 02/18] csum the correct page Wen Congyang
@ 2014-09-05 9:10 ` Wen Congyang
2014-09-05 9:25 ` Paul Durrant
2014-09-05 10:45 ` David Vrabel
2014-09-05 9:10 ` [RFC Patch v3 04/18] blktap2: dynamic allocate aio_requests to avoid -EBUSY error Wen Congyang
` (14 subsequent siblings)
17 siblings, 2 replies; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:10 UTC (permalink / raw)
To: xen devel
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, Paul Durrant, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 03/18] don't zero out ioreq page
2014-09-05 9:10 ` [RFC Patch v3 03/18] don't zero out ioreq page Wen Congyang
@ 2014-09-05 9:25 ` Paul Durrant
2014-09-05 9:33 ` Wen Congyang
2014-09-05 10:45 ` David Vrabel
1 sibling, 1 reply; 57+ messages in thread
From: Paul Durrant @ 2014-09-05 9:25 UTC (permalink / raw)
To: Wen Congyang, xen devel
Cc: Ian Campbell, Jiang Yunhong, Eddie Dong, Ian Jackson,
Yang Hongyang, Lai Jiangshan
> -----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
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 03/18] don't zero out ioreq page
2014-09-05 9:25 ` Paul Durrant
@ 2014-09-05 9:33 ` Wen Congyang
2014-09-05 9:39 ` Paul Durrant
0 siblings, 1 reply; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:33 UTC (permalink / raw)
To: Paul Durrant, xen devel
Cc: Ian Campbell, Jiang Yunhong, Eddie Dong, Ian Jackson,
Yang Hongyang, Lai Jiangshan
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
>
> .
>
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 03/18] don't zero out ioreq page
2014-09-05 9:33 ` Wen Congyang
@ 2014-09-05 9:39 ` Paul Durrant
0 siblings, 0 replies; 57+ messages in thread
From: Paul Durrant @ 2014-09-05 9:39 UTC (permalink / raw)
To: Wen Congyang, xen devel
Cc: Lai Jiangshan, Jiang Yunhong, Eddie Dong, Ian Jackson,
Yang Hongyang, Ian Campbell
> -----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
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC Patch v3 03/18] don't zero out ioreq page
2014-09-05 9:10 ` [RFC Patch v3 03/18] don't zero out ioreq page Wen Congyang
2014-09-05 9:25 ` Paul Durrant
@ 2014-09-05 10:45 ` David Vrabel
2014-09-12 7:33 ` Wen Congyang
1 sibling, 1 reply; 57+ messages in thread
From: David Vrabel @ 2014-09-05 10:45 UTC (permalink / raw)
To: Wen Congyang, xen devel
Cc: Ian Campbell, Ian Jackson, Jiang Yunhong, Dong Eddie,
Paul Durrant, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC Patch v3 03/18] don't zero out ioreq page
2014-09-05 10:45 ` David Vrabel
@ 2014-09-12 7:33 ` Wen Congyang
0 siblings, 0 replies; 57+ messages in thread
From: Wen Congyang @ 2014-09-12 7:33 UTC (permalink / raw)
To: David Vrabel, xen devel
Cc: Ian Campbell, Ian Jackson, Jiang Yunhong, Dong Eddie,
Paul Durrant, Yang Hongyang, Lai Jiangshan
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
> .
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [RFC Patch v3 04/18] blktap2: dynamic allocate aio_requests to avoid -EBUSY error
2014-09-05 9:10 [RFC Patch v3 00/18] Some bugfix patches Wen Congyang
` (2 preceding siblings ...)
2014-09-05 9:10 ` [RFC Patch v3 03/18] don't zero out ioreq page Wen Congyang
@ 2014-09-05 9:10 ` Wen Congyang
2014-09-08 11:34 ` Ian Campbell
2014-09-24 18:22 ` Shriram Rajagopalan
2014-09-05 9:10 ` [RFC Patch v3 05/18] blktap2: return the correct dev path Wen Congyang
` (13 subsequent siblings)
17 siblings, 2 replies; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:10 UTC (permalink / raw)
To: xen devel
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, Shriram Rajagopalan, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 04/18] blktap2: dynamic allocate aio_requests to avoid -EBUSY error
2014-09-05 9:10 ` [RFC Patch v3 04/18] blktap2: dynamic allocate aio_requests to avoid -EBUSY error Wen Congyang
@ 2014-09-08 11:34 ` Ian Campbell
2014-09-24 18:22 ` Shriram Rajagopalan
1 sibling, 0 replies; 57+ messages in thread
From: Ian Campbell @ 2014-09-08 11:34 UTC (permalink / raw)
To: Wen Congyang
Cc: Lai Jiangshan, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Shriram Rajagopalan, Yang Hongyang
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.
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC Patch v3 04/18] blktap2: dynamic allocate aio_requests to avoid -EBUSY error
2014-09-05 9:10 ` [RFC Patch v3 04/18] blktap2: dynamic allocate aio_requests to avoid -EBUSY error Wen Congyang
2014-09-08 11:34 ` Ian Campbell
@ 2014-09-24 18:22 ` Shriram Rajagopalan
1 sibling, 0 replies; 57+ messages in thread
From: Shriram Rajagopalan @ 2014-09-24 18:22 UTC (permalink / raw)
To: Wen Congyang
Cc: Lai Jiangshan, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Yang Hongyang, Ian Campbell
[-- 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
^ permalink raw reply [flat|nested] 57+ messages in thread
* [RFC Patch v3 05/18] blktap2: return the correct dev path
2014-09-05 9:10 [RFC Patch v3 00/18] Some bugfix patches Wen Congyang
` (3 preceding siblings ...)
2014-09-05 9:10 ` [RFC Patch v3 04/18] blktap2: dynamic allocate aio_requests to avoid -EBUSY error Wen Congyang
@ 2014-09-05 9:10 ` Wen Congyang
2014-09-05 9:10 ` [RFC Patch v3 06/18] blktap2: use correct way to get free event id Wen Congyang
` (12 subsequent siblings)
17 siblings, 0 replies; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:10 UTC (permalink / raw)
To: xen devel
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, Shriram Rajagopalan, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* [RFC Patch v3 06/18] blktap2: use correct way to get free event id
2014-09-05 9:10 [RFC Patch v3 00/18] Some bugfix patches Wen Congyang
` (4 preceding siblings ...)
2014-09-05 9:10 ` [RFC Patch v3 05/18] blktap2: return the correct dev path Wen Congyang
@ 2014-09-05 9:10 ` Wen Congyang
2014-09-05 9:10 ` [RFC Patch v3 07/18] blktap2: don't return negative " Wen Congyang
` (11 subsequent siblings)
17 siblings, 0 replies; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:10 UTC (permalink / raw)
To: xen devel
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, Shriram Rajagopalan, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* [RFC Patch v3 07/18] blktap2: don't return negative event id
2014-09-05 9:10 [RFC Patch v3 00/18] Some bugfix patches Wen Congyang
` (5 preceding siblings ...)
2014-09-05 9:10 ` [RFC Patch v3 06/18] blktap2: use correct way to get free event id Wen Congyang
@ 2014-09-05 9:10 ` Wen Congyang
2014-09-05 9:10 ` [RFC Patch v3 08/18] blktap2: use correct way to define array Wen Congyang
` (10 subsequent siblings)
17 siblings, 0 replies; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:10 UTC (permalink / raw)
To: xen devel
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, Shriram Rajagopalan, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* [RFC Patch v3 08/18] blktap2: use correct way to define array.
2014-09-05 9:10 [RFC Patch v3 00/18] Some bugfix patches Wen Congyang
` (6 preceding siblings ...)
2014-09-05 9:10 ` [RFC Patch v3 07/18] blktap2: don't return negative " Wen Congyang
@ 2014-09-05 9:10 ` Wen Congyang
2014-09-05 9:10 ` [RFC Patch v3 09/18] block-remus: fix memory leak Wen Congyang
` (9 subsequent siblings)
17 siblings, 0 replies; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:10 UTC (permalink / raw)
To: xen devel
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, Shriram Rajagopalan, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* [RFC Patch v3 09/18] block-remus: fix memory leak
2014-09-05 9:10 [RFC Patch v3 00/18] Some bugfix patches Wen Congyang
` (7 preceding siblings ...)
2014-09-05 9:10 ` [RFC Patch v3 08/18] blktap2: use correct way to define array Wen Congyang
@ 2014-09-05 9:10 ` Wen Congyang
2014-09-24 19:37 ` Shriram Rajagopalan
2014-09-05 9:10 ` [RFC Patch v3 10/18] block-remus: pass uuid to the callback td_open Wen Congyang
` (8 subsequent siblings)
17 siblings, 1 reply; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:10 UTC (permalink / raw)
To: xen devel
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, Shriram Rajagopalan, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 09/18] block-remus: fix memory leak
2014-09-05 9:10 ` [RFC Patch v3 09/18] block-remus: fix memory leak Wen Congyang
@ 2014-09-24 19:37 ` Shriram Rajagopalan
2014-09-25 5:23 ` Wen Congyang
0 siblings, 1 reply; 57+ messages in thread
From: Shriram Rajagopalan @ 2014-09-24 19:37 UTC (permalink / raw)
To: Wen Congyang
Cc: Lai Jiangshan, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Yang Hongyang, Ian Campbell
[-- 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
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC Patch v3 09/18] block-remus: fix memory leak
2014-09-24 19:37 ` Shriram Rajagopalan
@ 2014-09-25 5:23 ` Wen Congyang
2014-09-25 11:14 ` Shriram Rajagopalan
0 siblings, 1 reply; 57+ messages in thread
From: Wen Congyang @ 2014-09-25 5:23 UTC (permalink / raw)
To: rshriram
Cc: Lai Jiangshan, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Yang Hongyang, Ian Campbell
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
>>
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC Patch v3 09/18] block-remus: fix memory leak
2014-09-25 5:23 ` Wen Congyang
@ 2014-09-25 11:14 ` Shriram Rajagopalan
2014-09-26 2:29 ` Wen Congyang
0 siblings, 1 reply; 57+ messages in thread
From: Shriram Rajagopalan @ 2014-09-25 11:14 UTC (permalink / raw)
To: Wen Congyang
Cc: Lai Jiangshan, Dong Eddie, Jiang Yunhong, Ian Jackson, xen devel,
FNST-Yang Hongyang, Ian Campbell
[-- 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
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC Patch v3 09/18] block-remus: fix memory leak
2014-09-25 11:14 ` Shriram Rajagopalan
@ 2014-09-26 2:29 ` Wen Congyang
0 siblings, 0 replies; 57+ messages in thread
From: Wen Congyang @ 2014-09-26 2:29 UTC (permalink / raw)
To: rshriram
Cc: Lai Jiangshan, Dong Eddie, Jiang Yunhong, Ian Jackson, xen devel,
FNST-Yang Hongyang, Ian Campbell
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
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [RFC Patch v3 10/18] block-remus: pass uuid to the callback td_open
2014-09-05 9:10 [RFC Patch v3 00/18] Some bugfix patches Wen Congyang
` (8 preceding siblings ...)
2014-09-05 9:10 ` [RFC Patch v3 09/18] block-remus: fix memory leak Wen Congyang
@ 2014-09-05 9:10 ` Wen Congyang
2014-09-24 19:27 ` Shriram Rajagopalan
2014-09-05 9:10 ` [RFC Patch v3 11/18] block-remus: use correct way to get remus_image Wen Congyang
` (7 subsequent siblings)
17 siblings, 1 reply; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:10 UTC (permalink / raw)
To: xen devel
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, Shriram Rajagopalan, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 10/18] block-remus: pass uuid to the callback td_open
2014-09-05 9:10 ` [RFC Patch v3 10/18] block-remus: pass uuid to the callback td_open Wen Congyang
@ 2014-09-24 19:27 ` Shriram Rajagopalan
0 siblings, 0 replies; 57+ messages in thread
From: Shriram Rajagopalan @ 2014-09-24 19:27 UTC (permalink / raw)
To: Wen Congyang
Cc: Lai Jiangshan, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Yang Hongyang, Ian Campbell
[-- 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
^ permalink raw reply [flat|nested] 57+ messages in thread
* [RFC Patch v3 11/18] block-remus: use correct way to get remus_image
2014-09-05 9:10 [RFC Patch v3 00/18] Some bugfix patches Wen Congyang
` (9 preceding siblings ...)
2014-09-05 9:10 ` [RFC Patch v3 10/18] block-remus: pass uuid to the callback td_open Wen Congyang
@ 2014-09-05 9:10 ` Wen Congyang
2014-09-24 19:26 ` Shriram Rajagopalan
2014-09-05 9:10 ` [RFC Patch v3 12/18] block-remus: fix bug in tdremus_close() Wen Congyang
` (6 subsequent siblings)
17 siblings, 1 reply; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:10 UTC (permalink / raw)
To: xen devel
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, Shriram Rajagopalan, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 11/18] block-remus: use correct way to get remus_image
2014-09-05 9:10 ` [RFC Patch v3 11/18] block-remus: use correct way to get remus_image Wen Congyang
@ 2014-09-24 19:26 ` Shriram Rajagopalan
0 siblings, 0 replies; 57+ messages in thread
From: Shriram Rajagopalan @ 2014-09-24 19:26 UTC (permalink / raw)
To: Wen Congyang
Cc: Lai Jiangshan, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Yang Hongyang, Ian Campbell
[-- 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
^ permalink raw reply [flat|nested] 57+ messages in thread
* [RFC Patch v3 12/18] block-remus: fix bug in tdremus_close()
2014-09-05 9:10 [RFC Patch v3 00/18] Some bugfix patches Wen Congyang
` (10 preceding siblings ...)
2014-09-05 9:10 ` [RFC Patch v3 11/18] block-remus: use correct way to get remus_image Wen Congyang
@ 2014-09-05 9:10 ` Wen Congyang
2014-09-24 19:24 ` Shriram Rajagopalan
2014-09-05 9:10 ` [RFC Patch v3 13/18] don't call client_flush() when switching to unprotected mode Wen Congyang
` (5 subsequent siblings)
17 siblings, 1 reply; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:10 UTC (permalink / raw)
To: xen devel
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, Shriram Rajagopalan, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 12/18] block-remus: fix bug in tdremus_close()
2014-09-05 9:10 ` [RFC Patch v3 12/18] block-remus: fix bug in tdremus_close() Wen Congyang
@ 2014-09-24 19:24 ` Shriram Rajagopalan
0 siblings, 0 replies; 57+ messages in thread
From: Shriram Rajagopalan @ 2014-09-24 19:24 UTC (permalink / raw)
To: Wen Congyang
Cc: Lai Jiangshan, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Yang Hongyang, Ian Campbell
[-- 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
^ permalink raw reply [flat|nested] 57+ messages in thread
* [RFC Patch v3 13/18] don't call client_flush() when switching to unprotected mode
2014-09-05 9:10 [RFC Patch v3 00/18] Some bugfix patches Wen Congyang
` (11 preceding siblings ...)
2014-09-05 9:10 ` [RFC Patch v3 12/18] block-remus: fix bug in tdremus_close() Wen Congyang
@ 2014-09-05 9:10 ` Wen Congyang
2014-09-05 9:10 ` [RFC Patch v3 14/18] pass correct file to qemu if we use blktap2 Wen Congyang
` (4 subsequent siblings)
17 siblings, 0 replies; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:10 UTC (permalink / raw)
To: xen devel
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, Shriram Rajagopalan, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* [RFC Patch v3 14/18] pass correct file to qemu if we use blktap2
2014-09-05 9:10 [RFC Patch v3 00/18] Some bugfix patches Wen Congyang
` (12 preceding siblings ...)
2014-09-05 9:10 ` [RFC Patch v3 13/18] don't call client_flush() when switching to unprotected mode Wen Congyang
@ 2014-09-05 9:10 ` Wen Congyang
2014-09-08 11:35 ` Ian Campbell
2014-09-05 9:10 ` [RFC Patch v3 15/18] support blktap remus in xl Wen Congyang
` (3 subsequent siblings)
17 siblings, 1 reply; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:10 UTC (permalink / raw)
To: xen devel
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, Shriram Rajagopalan, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 14/18] pass correct file to qemu if we use blktap2
2014-09-05 9:10 ` [RFC Patch v3 14/18] pass correct file to qemu if we use blktap2 Wen Congyang
@ 2014-09-08 11:35 ` Ian Campbell
0 siblings, 0 replies; 57+ messages in thread
From: Ian Campbell @ 2014-09-08 11:35 UTC (permalink / raw)
To: Wen Congyang
Cc: Lai Jiangshan, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Shriram Rajagopalan, Yang Hongyang
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>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [RFC Patch v3 15/18] support blktap remus in xl
2014-09-05 9:10 [RFC Patch v3 00/18] Some bugfix patches Wen Congyang
` (13 preceding siblings ...)
2014-09-05 9:10 ` [RFC Patch v3 14/18] pass correct file to qemu if we use blktap2 Wen Congyang
@ 2014-09-05 9:10 ` Wen Congyang
2014-09-08 11:39 ` Ian Campbell
2014-09-05 9:10 ` [RFC Patch v3 16/18] update libxl__device_disk_from_xs_be() to support blktap device Wen Congyang
` (2 subsequent siblings)
17 siblings, 1 reply; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:10 UTC (permalink / raw)
To: xen devel
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, Shriram Rajagopalan, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 15/18] support blktap remus in xl
2014-09-05 9:10 ` [RFC Patch v3 15/18] support blktap remus in xl Wen Congyang
@ 2014-09-08 11:39 ` Ian Campbell
2014-09-10 7:19 ` Wen Congyang
0 siblings, 1 reply; 57+ messages in thread
From: Ian Campbell @ 2014-09-08 11:39 UTC (permalink / raw)
To: Wen Congyang
Cc: Lai Jiangshan, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Shriram Rajagopalan, Yang Hongyang
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.
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC Patch v3 15/18] support blktap remus in xl
2014-09-08 11:39 ` Ian Campbell
@ 2014-09-10 7:19 ` Wen Congyang
2014-09-10 10:04 ` Ian Campbell
0 siblings, 1 reply; 57+ messages in thread
From: Wen Congyang @ 2014-09-10 7:19 UTC (permalink / raw)
To: Ian Campbell, Ian Jackson
Cc: Lai Jiangshan, Jiang Yunhong, Dong Eddie, xen devel,
Shriram Rajagopalan, Yang Hongyang
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.
>
> .
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC Patch v3 15/18] support blktap remus in xl
2014-09-10 7:19 ` Wen Congyang
@ 2014-09-10 10:04 ` Ian Campbell
2014-09-10 10:36 ` Wen Congyang
0 siblings, 1 reply; 57+ messages in thread
From: Ian Campbell @ 2014-09-10 10:04 UTC (permalink / raw)
To: Wen Congyang
Cc: Lai Jiangshan, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Shriram Rajagopalan, Yang Hongyang
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.
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC Patch v3 15/18] support blktap remus in xl
2014-09-10 10:04 ` Ian Campbell
@ 2014-09-10 10:36 ` Wen Congyang
0 siblings, 0 replies; 57+ messages in thread
From: Wen Congyang @ 2014-09-10 10:36 UTC (permalink / raw)
To: Ian Campbell
Cc: Lai Jiangshan, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Shriram Rajagopalan, Yang Hongyang
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.
>
> .
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* [RFC Patch v3 16/18] update libxl__device_disk_from_xs_be() to support blktap device
2014-09-05 9:10 [RFC Patch v3 00/18] Some bugfix patches Wen Congyang
` (14 preceding siblings ...)
2014-09-05 9:10 ` [RFC Patch v3 15/18] support blktap remus in xl Wen Congyang
@ 2014-09-05 9:10 ` Wen Congyang
2014-09-08 11:42 ` Ian Campbell
2014-09-05 9:11 ` [RFC Patch v3 17/18] read nictype from xenstore Wen Congyang
2014-09-05 9:11 ` [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state Wen Congyang
17 siblings, 1 reply; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:10 UTC (permalink / raw)
To: xen devel
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, Shriram Rajagopalan, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 16/18] update libxl__device_disk_from_xs_be() to support blktap device
2014-09-05 9:10 ` [RFC Patch v3 16/18] update libxl__device_disk_from_xs_be() to support blktap device Wen Congyang
@ 2014-09-08 11:42 ` Ian Campbell
2014-09-09 1:57 ` Wen Congyang
2014-09-11 7:58 ` Wen Congyang
0 siblings, 2 replies; 57+ messages in thread
From: Ian Campbell @ 2014-09-08 11:42 UTC (permalink / raw)
To: Wen Congyang
Cc: Lai Jiangshan, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Shriram Rajagopalan, Yang Hongyang
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.
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 16/18] update libxl__device_disk_from_xs_be() to support blktap device
2014-09-08 11:42 ` Ian Campbell
@ 2014-09-09 1:57 ` Wen Congyang
2014-09-11 7:58 ` Wen Congyang
1 sibling, 0 replies; 57+ messages in thread
From: Wen Congyang @ 2014-09-09 1:57 UTC (permalink / raw)
To: Ian Campbell
Cc: Lai Jiangshan, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Shriram Rajagopalan, Yang Hongyang
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.
>
> .
>
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 16/18] update libxl__device_disk_from_xs_be() to support blktap device
2014-09-08 11:42 ` Ian Campbell
2014-09-09 1:57 ` Wen Congyang
@ 2014-09-11 7:58 ` Wen Congyang
2014-09-12 8:53 ` Wei Liu
1 sibling, 1 reply; 57+ messages in thread
From: Wen Congyang @ 2014-09-11 7:58 UTC (permalink / raw)
To: Ian Campbell
Cc: Lai Jiangshan, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Shriram Rajagopalan, Yang Hongyang
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.
>
> .
>
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 16/18] update libxl__device_disk_from_xs_be() to support blktap device
2014-09-11 7:58 ` Wen Congyang
@ 2014-09-12 8:53 ` Wei Liu
2014-09-12 9:03 ` Wen Congyang
0 siblings, 1 reply; 57+ messages in thread
From: Wei Liu @ 2014-09-12 8:53 UTC (permalink / raw)
To: Wen Congyang
Cc: wei.liu2, Ian Campbell, Ian Jackson, Jiang Yunhong, Dong Eddie,
xen devel, Shriram Rajagopalan, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 16/18] update libxl__device_disk_from_xs_be() to support blktap device
2014-09-12 8:53 ` Wei Liu
@ 2014-09-12 9:03 ` Wen Congyang
2014-09-12 10:35 ` Wei Liu
0 siblings, 1 reply; 57+ messages in thread
From: Wen Congyang @ 2014-09-12 9:03 UTC (permalink / raw)
To: Wei Liu
Cc: Ian Campbell, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Shriram Rajagopalan, Yang Hongyang, Lai Jiangshan
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
> .
>
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 16/18] update libxl__device_disk_from_xs_be() to support blktap device
2014-09-12 9:03 ` Wen Congyang
@ 2014-09-12 10:35 ` Wei Liu
0 siblings, 0 replies; 57+ messages in thread
From: Wei Liu @ 2014-09-12 10:35 UTC (permalink / raw)
To: Wen Congyang
Cc: Wei Liu, Ian Campbell, Dong Eddie, Jiang Yunhong, Ian Jackson,
xen devel, Shriram Rajagopalan, Yang Hongyang, Lai Jiangshan
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
> > .
> >
^ permalink raw reply [flat|nested] 57+ messages in thread
* [RFC Patch v3 17/18] read nictype from xenstore
2014-09-05 9:10 [RFC Patch v3 00/18] Some bugfix patches Wen Congyang
` (15 preceding siblings ...)
2014-09-05 9:10 ` [RFC Patch v3 16/18] update libxl__device_disk_from_xs_be() to support blktap device Wen Congyang
@ 2014-09-05 9:11 ` Wen Congyang
2014-09-08 11:41 ` Ian Campbell
2014-09-05 9:11 ` [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state Wen Congyang
17 siblings, 1 reply; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:11 UTC (permalink / raw)
To: xen devel
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, Yang Hongyang, Lai Jiangshan
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.
2014-09-05 9:10 [RFC Patch v3 00/18] Some bugfix patches Wen Congyang
` (16 preceding siblings ...)
2014-09-05 9:11 ` [RFC Patch v3 17/18] read nictype from xenstore Wen Congyang
@ 2014-09-05 9:11 ` Wen Congyang
2014-09-10 15:06 ` Aravind Gopalakrishnan
2014-09-18 0:05 ` Aravind Gopalakrishnan
17 siblings, 2 replies; 57+ messages in thread
From: Wen Congyang @ 2014-09-05 9:11 UTC (permalink / raw)
To: xen devel
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, Tim Deegan, Aravind Gopalakrishnan, Jun Nakajima,
Yang Hongyang, Suravee Suthikulpanit, Lai Jiangshan
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
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.
2014-09-05 9:11 ` [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state Wen Congyang
@ 2014-09-10 15:06 ` Aravind Gopalakrishnan
2014-09-11 6:10 ` Wen Congyang
2014-09-11 10:35 ` Tim Deegan
2014-09-18 0:05 ` Aravind Gopalakrishnan
1 sibling, 2 replies; 57+ messages in thread
From: Aravind Gopalakrishnan @ 2014-09-10 15:06 UTC (permalink / raw)
To: Wen Congyang, xen devel
Cc: Ian Campbell, Ian Jackson, Jiang Yunhong, Dong Eddie, Tim Deegan,
Jun Nakajima, Yang Hongyang, Suravee Suthikulpanit, Lai Jiangshan
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.
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.
2014-09-10 15:06 ` Aravind Gopalakrishnan
@ 2014-09-11 6:10 ` Wen Congyang
2014-09-11 10:35 ` Tim Deegan
1 sibling, 0 replies; 57+ messages in thread
From: Wen Congyang @ 2014-09-11 6:10 UTC (permalink / raw)
To: Aravind Gopalakrishnan, xen devel
Cc: Ian Campbell, Ian Jackson, Jiang Yunhong, Dong Eddie, Tim Deegan,
Jun Nakajima, Yang Hongyang, Suravee Suthikulpanit, Lai Jiangshan
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.
> .
>
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.
2014-09-10 15:06 ` Aravind Gopalakrishnan
2014-09-11 6:10 ` Wen Congyang
@ 2014-09-11 10:35 ` Tim Deegan
2014-09-12 3:14 ` Wen Congyang
2014-09-17 7:56 ` Wen Congyang
1 sibling, 2 replies; 57+ messages in thread
From: Tim Deegan @ 2014-09-11 10:35 UTC (permalink / raw)
To: Aravind Gopalakrishnan
Cc: Ian Campbell, Wen Congyang, Ian Jackson, Jiang Yunhong,
Dong Eddie, xen devel, Jun Nakajima, Yang Hongyang,
Suravee Suthikulpanit, Lai Jiangshan
[-- 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
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.
2014-09-11 10:35 ` Tim Deegan
@ 2014-09-12 3:14 ` Wen Congyang
2014-09-12 15:43 ` Tim Deegan
2014-09-17 7:56 ` Wen Congyang
1 sibling, 1 reply; 57+ messages in thread
From: Wen Congyang @ 2014-09-12 3:14 UTC (permalink / raw)
To: Tim Deegan, Aravind Gopalakrishnan
Cc: Lai Jiangshan, Ian Jackson, Jiang Yunhong, Dong Eddie, xen devel,
Jun Nakajima, Yang Hongyang, Suravee Suthikulpanit, Ian Campbell
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.
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.
2014-09-12 3:14 ` Wen Congyang
@ 2014-09-12 15:43 ` Tim Deegan
0 siblings, 0 replies; 57+ messages in thread
From: Tim Deegan @ 2014-09-12 15:43 UTC (permalink / raw)
To: Wen Congyang
Cc: Ian Campbell, Dong Eddie, Jiang Yunhong, Ian Jackson, xen devel,
Aravind Gopalakrishnan, Jun Nakajima, Yang Hongyang,
Suravee Suthikulpanit, Lai Jiangshan
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.
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.
2014-09-11 10:35 ` Tim Deegan
2014-09-12 3:14 ` Wen Congyang
@ 2014-09-17 7:56 ` Wen Congyang
2014-09-17 14:29 ` Aravind Gopalakrishnan
2014-09-18 0:05 ` Aravind Gopalakrishnan
1 sibling, 2 replies; 57+ messages in thread
From: Wen Congyang @ 2014-09-17 7:56 UTC (permalink / raw)
To: Aravind Gopalakrishnan
Cc: Yang Hongyang, Ian Campbell, Dong Eddie, Jiang Yunhong,
Ian Jackson, Tim Deegan, Jun Nakajima, xen devel,
Suravee Suthikulpanit, Lai Jiangshan
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.
>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.
2014-09-17 7:56 ` Wen Congyang
@ 2014-09-17 14:29 ` Aravind Gopalakrishnan
2014-09-18 0:05 ` Aravind Gopalakrishnan
1 sibling, 0 replies; 57+ messages in thread
From: Aravind Gopalakrishnan @ 2014-09-17 14:29 UTC (permalink / raw)
To: Wen Congyang
Cc: Yang Hongyang, Ian Campbell, Dong Eddie, Jiang Yunhong,
Ian Jackson, Tim Deegan, Jun Nakajima, xen devel,
Suravee Suthikulpanit, Lai Jiangshan
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.
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.
2014-09-17 7:56 ` Wen Congyang
2014-09-17 14:29 ` Aravind Gopalakrishnan
@ 2014-09-18 0:05 ` Aravind Gopalakrishnan
1 sibling, 0 replies; 57+ messages in thread
From: Aravind Gopalakrishnan @ 2014-09-18 0:05 UTC (permalink / raw)
To: Wen Congyang
Cc: Yang Hongyang, Ian Campbell, Dong Eddie, Jiang Yunhong,
Ian Jackson, Tim Deegan, Jun Nakajima, xen devel,
Suravee Suthikulpanit, Lai Jiangshan
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.
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state.
2014-09-05 9:11 ` [RFC Patch v3 18/18] x86/hvm: Always set pending event injection when loading VMC[BS] state Wen Congyang
2014-09-10 15:06 ` Aravind Gopalakrishnan
@ 2014-09-18 0:05 ` Aravind Gopalakrishnan
1 sibling, 0 replies; 57+ messages in thread
From: Aravind Gopalakrishnan @ 2014-09-18 0:05 UTC (permalink / raw)
To: Wen Congyang, xen devel
Cc: Ian Campbell, Ian Jackson, Jiang Yunhong, Dong Eddie, Tim Deegan,
Jun Nakajima, Yang Hongyang, Suravee Suthikulpanit, Lai Jiangshan
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>
^ permalink raw reply [flat|nested] 57+ messages in thread