[Qemu-devel] [PATCH] qed: replace vm_clock with rt_clock for qemu-tool compatibility

[Qemu-devel] [PATCH] qed: replace vm_clock with rt_clock for qemu-tool compatibility

[Stefan Hajnoczi]

The QED dirty bit timer marks the file clean after allocating writes
have drained.  This is cheaper than clearing/setting the dirty bit on
each allocating write because the timer introduces a grace period which
can be extended if more allocating writes arrive.

The vm_clock was used in an attempt to prevent modifying the image file
when live migration has stopped the VM.  Unfortunately vm_clock is
unavailable in the qemu-tool environment and will abort(3)!

Since QED currently does not support live migration, just replace
vm_clock with rt_clock and add comments explaining the migration
blocker.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
Zhi Yong: This patch is needed in addition to the qemu_init_main_loop() patches
you sent recently.  Without this patch QED may read the vm_clock, which calls
abort(3) in qemu-tool.c.  Together, our patches make QED work again in qemu-img
and qemu-io.

 block/qed.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index a041d31..fdb90e3 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -353,10 +353,7 @@ static void qed_start_need_check_timer(BDRVQEDState *s)
 {
     trace_qed_start_need_check_timer(s);
 
-    /* Use vm_clock so we don't alter the image file while suspended for
-     * migration.
-     */
-    qemu_mod_timer(s->need_check_timer, qemu_get_clock_ns(vm_clock) +
+    qemu_mod_timer(s->need_check_timer, qemu_get_clock_ns(rt_clock) +
                    get_ticks_per_sec() * QED_NEED_CHECK_TIMEOUT);
 }
 
@@ -494,9 +491,18 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
         }
     }
 
-    s->need_check_timer = qemu_new_timer_ns(vm_clock,
+    s->need_check_timer = qemu_new_timer_ns(rt_clock,
                                             qed_need_check_timer_cb, s);
 
+    /* There are two issues with live migration:
+     *
+     * 1. The destination will open the image file and see the dirty bit is
+     *    set, causing it to "repair" the image while the source still has it
+     *    open for writing.
+     *
+     * 2. The timer used for clearing the dirty bit uses rt_clock and can in
+     *    theory fire when the VM is not running during migration.
+     */
     error_set(&s->migration_blocker,
               QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
               "qed", bs->device_name, "live migration");
-- 
1.7.9

Re: [Qemu-devel] [PATCH] qed: replace vm_clock with rt_clock for qemu-tool compatibility

[Zhi Yong Wu]

On Sun, Feb 26, 2012 at 10:55 PM, Stefan Hajnoczi
<stefanha@linux.vnet.ibm.com> wrote:
> The QED dirty bit timer marks the file clean after allocating writes
> have drained.  This is cheaper than clearing/setting the dirty bit on
> each allocating write because the timer introduces a grace period which
> can be extended if more allocating writes arrive.
>
> The vm_clock was used in an attempt to prevent modifying the image file
> when live migration has stopped the VM.  Unfortunately vm_clock is
> unavailable in the qemu-tool environment and will abort(3)!
>
> Since QED currently does not support live migration, just replace
> vm_clock with rt_clock and add comments explaining the migration
> blocker.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
> Zhi Yong: This patch is needed in addition to the qemu_init_main_loop() patches
> you sent recently.  Without this patch QED may read the vm_clock, which calls
> abort(3) in qemu-tool.c.  Together, our patches make QED work again in qemu-img
Since vm_clock is created via qemu_init_main_loop(), when QED read
vm_clock, why will this call abort()?
Can you elaborate this? what is its call path?

> and qemu-io.
>
>  block/qed.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/block/qed.c b/block/qed.c
> index a041d31..fdb90e3 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -353,10 +353,7 @@ static void qed_start_need_check_timer(BDRVQEDState *s)
>  {
>     trace_qed_start_need_check_timer(s);
>
> -    /* Use vm_clock so we don't alter the image file while suspended for
> -     * migration.
> -     */
> -    qemu_mod_timer(s->need_check_timer, qemu_get_clock_ns(vm_clock) +
> +    qemu_mod_timer(s->need_check_timer, qemu_get_clock_ns(rt_clock) +
>                    get_ticks_per_sec() * QED_NEED_CHECK_TIMEOUT);
>  }
>
> @@ -494,9 +491,18 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
>         }
>     }
>
> -    s->need_check_timer = qemu_new_timer_ns(vm_clock,
> +    s->need_check_timer = qemu_new_timer_ns(rt_clock,
>                                             qed_need_check_timer_cb, s);
>
> +    /* There are two issues with live migration:
> +     *
> +     * 1. The destination will open the image file and see the dirty bit is
> +     *    set, causing it to "repair" the image while the source still has it
> +     *    open for writing.
> +     *
> +     * 2. The timer used for clearing the dirty bit uses rt_clock and can in
> +     *    theory fire when the VM is not running during migration.
> +     */
>     error_set(&s->migration_blocker,
>               QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
>               "qed", bs->device_name, "live migration");
> --
> 1.7.9
>
>



-- 
Regards,

Zhi Yong Wu

Re: [Qemu-devel] [PATCH] qed: replace vm_clock with rt_clock for qemu-tool compatibility

[Paolo Bonzini]

On 02/27/2012 08:35 AM, Zhi Yong Wu wrote:
> Since vm_clock is created via qemu_init_main_loop(), when QED read
> vm_clock, why will this call abort()?
> Can you elaborate this? what is its call path?
> 

It will crash in cpu_get_clock() (in qemu-tool.c).

Paolo

Re: [Qemu-devel] [PATCH] qed: replace vm_clock with rt_clock for qemu-tool compatibility

[Zhi Yong Wu]

On Mon, Feb 27, 2012 at 4:42 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 02/27/2012 08:35 AM, Zhi Yong Wu wrote:
>> Since vm_clock is created via qemu_init_main_loop(), when QED read
>> vm_clock, why will this call abort()?
>> Can you elaborate this? what is its call path?
>>
>
> It will crash in cpu_get_clock() (in qemu-tool.c).
nice, got it. thanks.
>
> Paolo



-- 
Regards,

Zhi Yong Wu

Re: [Qemu-devel] [PATCH] qed: replace vm_clock with rt_clock for qemu-tool compatibility

[Kevin Wolf]

Am 27.02.2012 09:42, schrieb Paolo Bonzini:
> On 02/27/2012 08:35 AM, Zhi Yong Wu wrote:
>> Since vm_clock is created via qemu_init_main_loop(), when QED read
>> vm_clock, why will this call abort()?
>> Can you elaborate this? what is its call path?
>>
> 
> It will crash in cpu_get_clock() (in qemu-tool.c).

The fix isn't very nice if it makes migration impossible. I'd like to
introduce a similar timer in qcow2 which does support migration and
breaking it is not an option. So what about (completely untested)...

diff --git a/qemu-tool.c b/qemu-tool.c
index 183a583..edb84f5 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -61,7 +61,7 @@ void monitor_protocol_event(MonitorEvent event,
QObject *data)

 int64_t cpu_get_clock(void)
 {
-    abort();
+    return 0;
 }

 int64_t cpu_get_icount(void)

Kevin

Re: [Qemu-devel] [PATCH] qed: replace vm_clock with rt_clock for qemu-tool compatibility

[Stefan Hajnoczi]

On Mon, Feb 27, 2012 at 9:31 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 27.02.2012 09:42, schrieb Paolo Bonzini:
>> On 02/27/2012 08:35 AM, Zhi Yong Wu wrote:
>>> Since vm_clock is created via qemu_init_main_loop(), when QED read
>>> vm_clock, why will this call abort()?
>>> Can you elaborate this? what is its call path?
>>>
>>
>> It will crash in cpu_get_clock() (in qemu-tool.c).
>
> The fix isn't very nice if it makes migration impossible. I'd like to
> introduce a similar timer in qcow2 which does support migration and
> breaking it is not an option. So what about (completely untested)...
>
> diff --git a/qemu-tool.c b/qemu-tool.c
> index 183a583..edb84f5 100644
> --- a/qemu-tool.c
> +++ b/qemu-tool.c
> @@ -61,7 +61,7 @@ void monitor_protocol_event(MonitorEvent event,
> QObject *data)
>
>  int64_t cpu_get_clock(void)
>  {
> -    abort();
> +    return 0;
>  }

This is what we used to do, I'm okay with it.

Stefan

Re: [Qemu-devel] [PATCH] qed: replace vm_clock with rt_clock for qemu-tool compatibility

[Zhi Yong Wu]

On Sun, Feb 26, 2012 at 10:55 PM, Stefan Hajnoczi
<stefanha@linux.vnet.ibm.com> wrote:
> The QED dirty bit timer marks the file clean after allocating writes
> have drained.  This is cheaper than clearing/setting the dirty bit on
> each allocating write because the timer introduces a grace period which
> can be extended if more allocating writes arrive.
>
> The vm_clock was used in an attempt to prevent modifying the image file
Why can vm_clock do this work but other clock can not? If you're
available, can you elaborate what the difference among the three
clocks is? such as vm_clock, rt_clock, and host_clock.

> when live migration has stopped the VM.  Unfortunately vm_clock is
> unavailable in the qemu-tool environment and will abort(3)!
>
> Since QED currently does not support live migration, just replace
> vm_clock with rt_clock and add comments explaining the migration
> blocker.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
> Zhi Yong: This patch is needed in addition to the qemu_init_main_loop() patches
> you sent recently.  Without this patch QED may read the vm_clock, which calls
> abort(3) in qemu-tool.c.  Together, our patches make QED work again in qemu-img
> and qemu-io.
>
>  block/qed.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/block/qed.c b/block/qed.c
> index a041d31..fdb90e3 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -353,10 +353,7 @@ static void qed_start_need_check_timer(BDRVQEDState *s)
>  {
>     trace_qed_start_need_check_timer(s);
>
> -    /* Use vm_clock so we don't alter the image file while suspended for
> -     * migration.
> -     */
> -    qemu_mod_timer(s->need_check_timer, qemu_get_clock_ns(vm_clock) +
> +    qemu_mod_timer(s->need_check_timer, qemu_get_clock_ns(rt_clock) +
>                    get_ticks_per_sec() * QED_NEED_CHECK_TIMEOUT);
>  }
>
> @@ -494,9 +491,18 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
>         }
>     }
>
> -    s->need_check_timer = qemu_new_timer_ns(vm_clock,
> +    s->need_check_timer = qemu_new_timer_ns(rt_clock,
>                                             qed_need_check_timer_cb, s);
>
> +    /* There are two issues with live migration:
> +     *
> +     * 1. The destination will open the image file and see the dirty bit is
> +     *    set, causing it to "repair" the image while the source still has it
> +     *    open for writing.
> +     *
> +     * 2. The timer used for clearing the dirty bit uses rt_clock and can in
> +     *    theory fire when the VM is not running during migration.
> +     */
>     error_set(&s->migration_blocker,
>               QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
>               "qed", bs->device_name, "live migration");
> --
> 1.7.9
>
>



-- 
Regards,

Zhi Yong Wu

Re: [Qemu-devel] [PATCH] qed: replace vm_clock with rt_clock for qemu-tool compatibility

[Stefan Hajnoczi]

On Mon, Feb 27, 2012 at 2:20 PM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
> On Sun, Feb 26, 2012 at 10:55 PM, Stefan Hajnoczi
> <stefanha@linux.vnet.ibm.com> wrote:
>> The QED dirty bit timer marks the file clean after allocating writes
>> have drained.  This is cheaper than clearing/setting the dirty bit on
>> each allocating write because the timer introduces a grace period which
>> can be extended if more allocating writes arrive.
>>
>> The vm_clock was used in an attempt to prevent modifying the image file
> Why can vm_clock do this work but other clock can not? If you're
> available, can you elaborate what the difference among the three
> clocks is? such as vm_clock, rt_clock, and host_clock.

See qemu-timer.h.

Stefan

Re: [Qemu-devel] [PATCH] qed: replace vm_clock with rt_clock for qemu-tool compatibility

[Zhi Yong Wu]

On Mon, Feb 27, 2012 at 10:41 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Feb 27, 2012 at 2:20 PM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote:
>> On Sun, Feb 26, 2012 at 10:55 PM, Stefan Hajnoczi
>> <stefanha@linux.vnet.ibm.com> wrote:
>>> The QED dirty bit timer marks the file clean after allocating writes
>>> have drained.  This is cheaper than clearing/setting the dirty bit on
>>> each allocating write because the timer introduces a grace period which
>>> can be extended if more allocating writes arrive.
>>>
>>> The vm_clock was used in an attempt to prevent modifying the image file
>> Why can vm_clock do this work but other clock can not? If you're
>> available, can you elaborate what the difference among the three
>> clocks is? such as vm_clock, rt_clock, and host_clock.
>
> See qemu-timer.h.
thanks
>
> Stefan



-- 
Regards,

Zhi Yong Wu