[Qemu-devel] [STABLE 0.13 0/4][PULL]: Migration fixes

[Qemu-devel] [STABLE 0.13 0/4][PULL]: Migration fixes

[Luiz Capitulino]

Anthony,

The following migration fixes have been sent to the list some time ago
but have not been applied yet. They fix important bugs and seem good
to me (I've tested them too).

The changes (since 96638e706cd431528690cf611adcb04e1bc3255d) are available
in the following repository:

    git://repo.or.cz/qemu/qmp-unstable.git patches-for-0.13

Please, also note that they should be applied in master too (except for
the 'cont' one).

Alex Williamson (1):
      savevm: Reset last block info at beginning of each save

Amit Shah (1):
      migration: Accept 'cont' only after successful incoming migration

Avi Kivity (1):
      QEMUFileBuffered: indicate that we're ready when the underlying file is ready

Marcelo Tosatti (1):
      set proper migration status on ->write error (v5)

 arch_init.c     |    7 +++++--
 buffered_file.c |   12 +++++++++++-
 migration.c     |   10 +++++++++-
 monitor.c       |    4 ++++
 qerror.c        |    4 ++++
 qerror.h        |    3 +++
 sysemu.h        |    1 +
 vl.c            |    2 ++
 8 files changed, 39 insertions(+), 4 deletions(-)

[Qemu-devel] [PATCH 1/4] set proper migration status on ->write error (v5)

[Luiz Capitulino]

From: Marcelo Tosatti <mtosatti@redhat.com>

If ->write fails, declare migration status as MIG_STATE_ERROR.

Also, in buffered_file.c, ->close the object in case of an
error.

Fixes "migrate -d "exec:dd of=file", where dd fails to open file.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 buffered_file.c |    4 +++-
 migration.c     |    8 +++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index 54dc6c2..be147d6 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -222,8 +222,10 @@ static void buffered_rate_tick(void *opaque)
 {
     QEMUFileBuffered *s = opaque;
 
-    if (s->has_error)
+    if (s->has_error) {
+        buffered_close(s);
         return;
+    }
 
     qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + 100);
 
diff --git a/migration.c b/migration.c
index 650eb78..dbb8fd6 100644
--- a/migration.c
+++ b/migration.c
@@ -314,8 +314,14 @@ ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)
     if (ret == -1)
         ret = -(s->get_error(s));
 
-    if (ret == -EAGAIN)
+    if (ret == -EAGAIN) {
         qemu_set_fd_handler2(s->fd, NULL, NULL, migrate_fd_put_notify, s);
+    } else if (ret < 0) {
+        if (s->mon) {
+            monitor_resume(s->mon);
+        }
+        s->state = MIG_STATE_ERROR;
+    }
 
     return ret;
 }
-- 
1.7.2.1.97.g3235b

Re: [Qemu-devel] [PATCH 1/4] set proper migration status on ->write error (v5)

[Anthony Liguori]

On 08/19/2010 08:18 AM, Luiz Capitulino wrote:
> From: Marcelo Tosatti<mtosatti@redhat.com>
>
> If ->write fails, declare migration status as MIG_STATE_ERROR.
>
> Also, in buffered_file.c, ->close the object in case of an
> error.
>
> Fixes "migrate -d "exec:dd of=file", where dd fails to open file.
>
> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>    

Applied.  Thanks.

Regards,

Anthony Liguori
> ---
>   buffered_file.c |    4 +++-
>   migration.c     |    8 +++++++-
>   2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/buffered_file.c b/buffered_file.c
> index 54dc6c2..be147d6 100644
> --- a/buffered_file.c
> +++ b/buffered_file.c
> @@ -222,8 +222,10 @@ static void buffered_rate_tick(void *opaque)
>   {
>       QEMUFileBuffered *s = opaque;
>
> -    if (s->has_error)
> +    if (s->has_error) {
> +        buffered_close(s);
>           return;
> +    }
>
>       qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + 100);
>
> diff --git a/migration.c b/migration.c
> index 650eb78..dbb8fd6 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -314,8 +314,14 @@ ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)
>       if (ret == -1)
>           ret = -(s->get_error(s));
>
> -    if (ret == -EAGAIN)
> +    if (ret == -EAGAIN) {
>           qemu_set_fd_handler2(s->fd, NULL, NULL, migrate_fd_put_notify, s);
> +    } else if (ret<  0) {
> +        if (s->mon) {
> +            monitor_resume(s->mon);
> +        }
> +        s->state = MIG_STATE_ERROR;
> +    }
>
>       return ret;
>   }
>    

[Qemu-devel] [PATCH 2/4] QEMUFileBuffered: indicate that we're ready when the underlying file is ready

[Luiz Capitulino]

From: Avi Kivity <avi@redhat.com>

QEMUFileBuffered stops writing when the underlying QEMUFile is not ready,
and tells its producer so.  However, when the underlying QEMUFile becomes
ready, it neglects to pass that information along, resulting in stoppage
of all data until the next tick (a tenths of a second).

Usually this doesn't matter, because most QEMUFiles used with QEMUFileBuffered
are almost always ready, but in the case of exec: migration this is not true,
due to the small pipe buffers used to connect to the target process.  The
result is very slow migration.

Fix by detecting the readiness notification and propagating it.  The detection
is a little ugly since QEMUFile overloads put_buffer() to send it, but that's
the suject for a different patch.

Signed-off-by: Avi Kivity <avi@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 buffered_file.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index be147d6..1836e7e 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -156,6 +156,14 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
         offset = size;
     }
 
+    if (pos == 0 && size == 0) {
+        DPRINTF("file is ready\n");
+        if (s->bytes_xfer <= s->xfer_limit) {
+            DPRINTF("notifying client\n");
+            s->put_ready(s->opaque);
+        }
+    }
+
     return offset;
 }
 
-- 
1.7.2.1.97.g3235b

[Qemu-devel] [PATCH 3/4] migration: Accept 'cont' only after successful incoming migration

[Luiz Capitulino]

From: Amit Shah <amit.shah@redhat.com>

When a 'cont' is issued on a VM that's just waiting for an incoming
migration, the VM reboots and boots into the guest, possibly corrupting
its storage since it could be shared with another VM running elsewhere.

Ensure that a VM started with '-incoming' is only run when an incoming
migration successfully completes.

A new qerror, QERR_MIGRATION_EXPECTED, is added to signal that 'cont'
failed due to no incoming migration has been attempted yet.

Reported-by: Laine Stump <laine@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 migration.c |    2 ++
 monitor.c   |    4 ++++
 qerror.c    |    4 ++++
 qerror.h    |    3 +++
 sysemu.h    |    1 +
 vl.c        |    2 ++
 6 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/migration.c b/migration.c
index dbb8fd6..468d517 100644
--- a/migration.c
+++ b/migration.c
@@ -67,6 +67,8 @@ void process_incoming_migration(QEMUFile *f)
     qemu_announce_self();
     DPRINTF("successfully loaded vm state\n");
 
+    incoming_expected = false;
+
     if (autostart)
         vm_start();
 }
diff --git a/monitor.c b/monitor.c
index 45fd482..5366c36 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1056,6 +1056,10 @@ static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     struct bdrv_iterate_context context = { mon, 0 };
 
+    if (incoming_expected) {
+        qerror_report(QERR_MIGRATION_EXPECTED);
+        return -1;
+    }
     bdrv_iterate(encrypted_bdrv_it, &context);
     /* only resume the vm if all keys are set and valid */
     if (!context.err) {
diff --git a/qerror.c b/qerror.c
index 2f6f590..0af3ab3 100644
--- a/qerror.c
+++ b/qerror.c
@@ -141,6 +141,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Using KVM without %(capability), %(feature) unavailable",
     },
     {
+        .error_fmt = QERR_MIGRATION_EXPECTED,
+        .desc      = "An incoming migration is expected before this command can be executed",
+    },
+    {
         .error_fmt = QERR_MISSING_PARAMETER,
         .desc      = "Parameter '%(name)' is missing",
     },
diff --git a/qerror.h b/qerror.h
index 9ad00b4..62802ea 100644
--- a/qerror.h
+++ b/qerror.h
@@ -121,6 +121,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_KVM_MISSING_CAP \
     "{ 'class': 'KVMMissingCap', 'data': { 'capability': %s, 'feature': %s } }"
 
+#define QERR_MIGRATION_EXPECTED \
+    "{ 'class': 'MigrationExpected', 'data': {} }"
+
 #define QERR_MISSING_PARAMETER \
     "{ 'class': 'MissingParameter', 'data': { 'name': %s } }"
 
diff --git a/sysemu.h b/sysemu.h
index 9c988bb..a1f6466 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -99,6 +99,7 @@ typedef enum DisplayType
 } DisplayType;
 
 extern int autostart;
+extern int incoming_expected;
 extern int bios_size;
 
 typedef enum {
diff --git a/vl.c b/vl.c
index ba6ee11..c2e7cc1 100644
--- a/vl.c
+++ b/vl.c
@@ -182,6 +182,7 @@ int nb_nics;
 NICInfo nd_table[MAX_NICS];
 int vm_running;
 int autostart;
+int incoming_expected; /* Started with -incoming and waiting for incoming */
 static int rtc_utc = 1;
 static int rtc_date_offset = -1; /* -1 means no change */
 QEMUClock *rtc_clock;
@@ -2557,6 +2558,7 @@ int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_incoming:
                 incoming = optarg;
+                incoming_expected = true;
                 break;
             case QEMU_OPTION_nodefaults:
                 default_serial = 0;
-- 
1.7.2.1.97.g3235b

[Qemu-devel] [PATCH 4/4] savevm: Reset last block info at beginning of each save

[Luiz Capitulino]

From: Alex Williamson <alex.williamson@redhat.com>

If we save more than once we need to reset the last block info or else
only the first save has the actual block info and each subsequent save
will only use continue flags, making them unloadable independently.

Found-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Acked-by: Glauber Costa <glommer@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 arch_init.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 47bb4b2..e0bd18c 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -104,10 +104,11 @@ static int is_dup_page(uint8_t *page, uint8_t ch)
     return 1;
 }
 
+static RAMBlock *last_block;
+static ram_addr_t last_offset;
+
 static int ram_save_block(QEMUFile *f)
 {
-    static RAMBlock *last_block = NULL;
-    static ram_addr_t last_offset = 0;
     RAMBlock *block = last_block;
     ram_addr_t offset = last_offset;
     ram_addr_t current_addr;
@@ -231,6 +232,8 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
     if (stage == 1) {
         RAMBlock *block;
         bytes_transferred = 0;
+        last_block = NULL;
+        last_offset = 0;
 
         /* Make sure all dirty bits are set */
         QLIST_FOREACH(block, &ram_list.blocks, next) {
-- 
1.7.2.1.97.g3235b

Re: [Qemu-devel] [PATCH 4/4] savevm: Reset last block info at beginning of each save

[Anthony Liguori]

On 08/19/2010 08:18 AM, Luiz Capitulino wrote:
> From: Alex Williamson<alex.williamson@redhat.com>
>
> If we save more than once we need to reset the last block info or else
> only the first save has the actual block info and each subsequent save
> will only use continue flags, making them unloadable independently.
>
> Found-by: Miguel Di Ciurcio Filho<miguel.filho@gmail.com>
> Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
> Acked-by: Glauber Costa<glommer@redhat.com>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>    

Applied.  Thanks.

Regards,

Anthony Liguori
> ---
>   arch_init.c |    7 +++++--
>   1 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 47bb4b2..e0bd18c 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -104,10 +104,11 @@ static int is_dup_page(uint8_t *page, uint8_t ch)
>       return 1;
>   }
>
> +static RAMBlock *last_block;
> +static ram_addr_t last_offset;
> +
>   static int ram_save_block(QEMUFile *f)
>   {
> -    static RAMBlock *last_block = NULL;
> -    static ram_addr_t last_offset = 0;
>       RAMBlock *block = last_block;
>       ram_addr_t offset = last_offset;
>       ram_addr_t current_addr;
> @@ -231,6 +232,8 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
>       if (stage == 1) {
>           RAMBlock *block;
>           bytes_transferred = 0;
> +        last_block = NULL;
> +        last_offset = 0;
>
>           /* Make sure all dirty bits are set */
>           QLIST_FOREACH(block,&ram_list.blocks, next) {
>    

[Qemu-devel] Re: [STABLE 0.13 0/4][PULL]: Migration fixes

[Anthony Liguori]

On 08/19/2010 08:18 AM, Luiz Capitulino wrote:
> Anthony,
>
> The following migration fixes have been sent to the list some time ago
> but have not been applied yet. They fix important bugs and seem good
> to me (I've tested them too).
>
> The changes (since 96638e706cd431528690cf611adcb04e1bc3255d) are available
> in the following repository:
>
>      git://repo.or.cz/qemu/qmp-unstable.git patches-for-0.13
>
> Please, also note that they should be applied in master too (except for
> the 'cont' one).
>    

Things need to be applied to master first, and then applied to 
stable-0.13 via a cherry pick.

Why should the 'cont' one be applied to stable but not master?

Regards,

Anthony Liguori

> Alex Williamson (1):
>        savevm: Reset last block info at beginning of each save
>
> Amit Shah (1):
>        migration: Accept 'cont' only after successful incoming migration
>
> Avi Kivity (1):
>        QEMUFileBuffered: indicate that we're ready when the underlying file is ready
>
> Marcelo Tosatti (1):
>        set proper migration status on ->write error (v5)
>
>   arch_init.c     |    7 +++++--
>   buffered_file.c |   12 +++++++++++-
>   migration.c     |   10 +++++++++-
>   monitor.c       |    4 ++++
>   qerror.c        |    4 ++++
>   qerror.h        |    3 +++
>   sysemu.h        |    1 +
>   vl.c            |    2 ++
>   8 files changed, 39 insertions(+), 4 deletions(-)
>    

[Qemu-devel] Re: [STABLE 0.13 0/4][PULL]: Migration fixes

[Luiz Capitulino]

On Thu, 19 Aug 2010 08:44:09 -0500
Anthony Liguori <aliguori@linux.vnet.ibm.com> wrote:

> On 08/19/2010 08:18 AM, Luiz Capitulino wrote:
> > Anthony,
> >
> > The following migration fixes have been sent to the list some time ago
> > but have not been applied yet. They fix important bugs and seem good
> > to me (I've tested them too).
> >
> > The changes (since 96638e706cd431528690cf611adcb04e1bc3255d) are available
> > in the following repository:
> >
> >      git://repo.or.cz/qemu/qmp-unstable.git patches-for-0.13
> >
> > Please, also note that they should be applied in master too (except for
> > the 'cont' one).
> >    
> 
> Things need to be applied to master first, and then applied to 
> stable-0.13 via a cherry pick.

Can you do so then please? I've added the 'STABLE 0.13' tag just to
make sure they are not forgotten for 0.13.

> Why should the 'cont' one be applied to stable but not master?

It's already applied there.

[Qemu-devel] Re: [STABLE 0.13 0/4][PULL]: Migration fixes

[Anthony Liguori]

On 08/19/2010 08:51 AM, Luiz Capitulino wrote:
> On Thu, 19 Aug 2010 08:44:09 -0500
> Anthony Liguori<aliguori@linux.vnet.ibm.com>  wrote:
>
>    
>> On 08/19/2010 08:18 AM, Luiz Capitulino wrote:
>>      
>>> Anthony,
>>>
>>> The following migration fixes have been sent to the list some time ago
>>> but have not been applied yet. They fix important bugs and seem good
>>> to me (I've tested them too).
>>>
>>> The changes (since 96638e706cd431528690cf611adcb04e1bc3255d) are available
>>> in the following repository:
>>>
>>>       git://repo.or.cz/qemu/qmp-unstable.git patches-for-0.13
>>>
>>> Please, also note that they should be applied in master too (except for
>>> the 'cont' one).
>>>
>>>        
>> Things need to be applied to master first, and then applied to
>> stable-0.13 via a cherry pick.
>>      
> Can you do so then please?

Yup, doing it right now along with the backlog of other patches.

Regards,

Anthony Liguori

>   I've added the 'STABLE 0.13' tag just to
> make sure they are not forgotten for 0.13.
>
>    
>> Why should the 'cont' one be applied to stable but not master?
>>      
> It's already applied there.
>