| Message ID | 20110107220801.15395.38757.stgit@s20.home |
|---|---|
| State | New |
| Headers | show |
On Fri, Jan 07, 2011 at 03:13:25PM -0700, Alex Williamson wrote: > The no_migrate save state flag is currently only checked in the > last phase of migration. This means that we potentially waste > a lot of time and bandwidth with the live state handlers before > we ever check the no_migrate flags. The error message printed > when we catch a non-migratable device doesn't get printed for > a detached migration. And, no_migrate does nothing to prevent > an incoming migration to a target that includes a non-migratable > device. This attempts to fix all of these. Nod. Sounds good. > One notable difference in behavior is that an outgoing migration > now checks for non-migratable devices before ever connecting to > the target system. This means the target will remain listening > rather than exit from failure. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> Looks like a feature, but it won't be hard to make it exit if we want it for compatibility.... > --- > > v4: > - fix braces noted by Jan > - return error from qemu_savevm_state_blocked rather than fixed EINVAL > at qemu_loadvm_state(), since it'a already using errno values > > v3: > > Daniel, adding you to see if libvirt cares about the difference in > whether the target exits on migration failure as noted above. > > Also adding Michael as he's complained about the no_migrate check > happening so late in the process. > > migration.c | 4 ++++ > savevm.c | 41 +++++++++++++++++++++++++++-------------- > sysemu.h | 1 + > 3 files changed, 32 insertions(+), 14 deletions(-) > > diff --git a/migration.c b/migration.c > index e5ba51c..d593b1d 100644 > --- a/migration.c > +++ b/migration.c > @@ -88,6 +88,10 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data) > return -1; > } > > + if (qemu_savevm_state_blocked(mon)) { > + return -1; > + } > + > if (strstart(uri, "tcp:", &p)) { > s = tcp_start_outgoing_migration(mon, p, max_throttle, detach, > blk, inc); > diff --git a/savevm.c b/savevm.c > index 90aa237..34c0d1a 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -1401,19 +1401,13 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id) > return vmstate_load_state(f, se->vmsd, se->opaque, version_id); > } > > -static int vmstate_save(QEMUFile *f, SaveStateEntry *se) > +static void vmstate_save(QEMUFile *f, SaveStateEntry *se) > { > - if (se->no_migrate) { > - return -1; > - } > - > if (!se->vmsd) { /* Old style */ > se->save_state(f, se->opaque); > - return 0; > + return; > } > vmstate_save_state(f,se->vmsd, se->opaque); > - > - return 0; > } > > #define QEMU_VM_FILE_MAGIC 0x5145564d > @@ -1427,6 +1421,20 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se) > #define QEMU_VM_SECTION_FULL 0x04 > #define QEMU_VM_SUBSECTION 0x05 > > +int qemu_savevm_state_blocked(Monitor *mon) > +{ > + SaveStateEntry *se; > + > + QTAILQ_FOREACH(se, &savevm_handlers, entry) { > + if (se->no_migrate) { > + monitor_printf(mon, "state blocked by non-migratable device '%s'\n", > + se->idstr); > + return -EINVAL; > + } > + } > + return 0; > +} > + > int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable, > int shared) > { > @@ -1508,7 +1516,6 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f) > int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f) > { > SaveStateEntry *se; > - int r; > > cpu_synchronize_all_states(); > > @@ -1541,11 +1548,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f) > qemu_put_be32(f, se->instance_id); > qemu_put_be32(f, se->version_id); > > - r = vmstate_save(f, se); > - if (r < 0) { > - monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr); > - return r; > - } > + vmstate_save(f, se); > } > > qemu_put_byte(f, QEMU_VM_EOF); > @@ -1575,6 +1578,11 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f) > saved_vm_running = vm_running; > vm_stop(0); > > + ret = qemu_savevm_state_blocked(mon); > + if (ret < 0) { > + goto out; > + } > + > ret = qemu_savevm_state_begin(mon, f, 0, 0); > if (ret < 0) > goto out; > @@ -1692,6 +1700,11 @@ int qemu_loadvm_state(QEMUFile *f) > unsigned int v; > int ret; > > + ret = qemu_savevm_state_blocked(default_mon); > + if (ret < 0) { > + return ret; > + } > + > v = qemu_get_be32(f); > if (v != QEMU_VM_FILE_MAGIC) > return -EINVAL; > diff --git a/sysemu.h b/sysemu.h > index e728ea1..eefaba5 100644 > --- a/sysemu.h > +++ b/sysemu.h > @@ -75,6 +75,7 @@ void qemu_announce_self(void); > > void main_loop_wait(int nonblocking); > > +int qemu_savevm_state_blocked(Monitor *mon); > int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable, > int shared); > int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f);
On Fri, Jan 07, 2011 at 03:13:25PM -0700, Alex Williamson wrote: > The no_migrate save state flag is currently only checked in the > last phase of migration. This means that we potentially waste > a lot of time and bandwidth with the live state handlers before > we ever check the no_migrate flags. The error message printed > when we catch a non-migratable device doesn't get printed for > a detached migration. And, no_migrate does nothing to prevent > an incoming migration to a target that includes a non-migratable > device. This attempts to fix all of these. > > One notable difference in behavior is that an outgoing migration > now checks for non-migratable devices before ever connecting to > the target system. This means the target will remain listening > rather than exit from failure. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> Minor nit: The API of qemu_savevm_state_blocked is a bit strange. functions should either return 0/1 values or 0 on success negative on failure or a bool. This API asks "is it blocked" and returns -EINVAL to mean "yes". _blocked is also a bit ambigious: it seems to imply a temporary condition. How about we reverse the logic, call the new API qemu_savevm_state_supported, qemu_savevm_state_enabled, something like this? Then you can return 0 if migration is possible, -1 if not. > --- > > v4: > - fix braces noted by Jan > - return error from qemu_savevm_state_blocked rather than fixed EINVAL > at qemu_loadvm_state(), since it'a already using errno values > > v3: > > Daniel, adding you to see if libvirt cares about the difference in > whether the target exits on migration failure as noted above. > > Also adding Michael as he's complained about the no_migrate check > happening so late in the process. > > migration.c | 4 ++++ > savevm.c | 41 +++++++++++++++++++++++++++-------------- > sysemu.h | 1 + > 3 files changed, 32 insertions(+), 14 deletions(-) > > diff --git a/migration.c b/migration.c > index e5ba51c..d593b1d 100644 > --- a/migration.c > +++ b/migration.c > @@ -88,6 +88,10 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data) > return -1; > } > > + if (qemu_savevm_state_blocked(mon)) { > + return -1; > + } > + > if (strstart(uri, "tcp:", &p)) { > s = tcp_start_outgoing_migration(mon, p, max_throttle, detach, > blk, inc); > diff --git a/savevm.c b/savevm.c > index 90aa237..34c0d1a 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -1401,19 +1401,13 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id) > return vmstate_load_state(f, se->vmsd, se->opaque, version_id); > } > > -static int vmstate_save(QEMUFile *f, SaveStateEntry *se) > +static void vmstate_save(QEMUFile *f, SaveStateEntry *se) > { > - if (se->no_migrate) { > - return -1; > - } > - > if (!se->vmsd) { /* Old style */ > se->save_state(f, se->opaque); > - return 0; > + return; > } > vmstate_save_state(f,se->vmsd, se->opaque); > - > - return 0; > } > > #define QEMU_VM_FILE_MAGIC 0x5145564d > @@ -1427,6 +1421,20 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se) > #define QEMU_VM_SECTION_FULL 0x04 > #define QEMU_VM_SUBSECTION 0x05 > > +int qemu_savevm_state_blocked(Monitor *mon) > +{ > + SaveStateEntry *se; > + > + QTAILQ_FOREACH(se, &savevm_handlers, entry) { > + if (se->no_migrate) { > + monitor_printf(mon, "state blocked by non-migratable device '%s'\n", > + se->idstr); > + return -EINVAL; > + } > + } > + return 0; > +} > + > int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable, > int shared) > { > @@ -1508,7 +1516,6 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f) > int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f) > { > SaveStateEntry *se; > - int r; > > cpu_synchronize_all_states(); > > @@ -1541,11 +1548,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f) > qemu_put_be32(f, se->instance_id); > qemu_put_be32(f, se->version_id); > > - r = vmstate_save(f, se); > - if (r < 0) { > - monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr); > - return r; > - } > + vmstate_save(f, se); > } > > qemu_put_byte(f, QEMU_VM_EOF); > @@ -1575,6 +1578,11 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f) > saved_vm_running = vm_running; > vm_stop(0); > > + ret = qemu_savevm_state_blocked(mon); > + if (ret < 0) { > + goto out; > + } > + > ret = qemu_savevm_state_begin(mon, f, 0, 0); > if (ret < 0) > goto out; > @@ -1692,6 +1700,11 @@ int qemu_loadvm_state(QEMUFile *f) > unsigned int v; > int ret; > > + ret = qemu_savevm_state_blocked(default_mon); > + if (ret < 0) { > + return ret; > + } > + > v = qemu_get_be32(f); > if (v != QEMU_VM_FILE_MAGIC) > return -EINVAL; > diff --git a/sysemu.h b/sysemu.h > index e728ea1..eefaba5 100644 > --- a/sysemu.h > +++ b/sysemu.h > @@ -75,6 +75,7 @@ void qemu_announce_self(void); > > void main_loop_wait(int nonblocking); > > +int qemu_savevm_state_blocked(Monitor *mon); > int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable, > int shared); > int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f);
On Fri, Jan 07, 2011 at 03:13:25PM -0700, Alex Williamson wrote: > The no_migrate save state flag is currently only checked in the > last phase of migration. This means that we potentially waste > a lot of time and bandwidth with the live state handlers before > we ever check the no_migrate flags. The error message printed > when we catch a non-migratable device doesn't get printed for > a detached migration. And, no_migrate does nothing to prevent > an incoming migration to a target that includes a non-migratable > device. This attempts to fix all of these. > > One notable difference in behavior is that an outgoing migration > now checks for non-migratable devices before ever connecting to > the target system. This means the target will remain listening > rather than exit from failure. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > > v4: > - fix braces noted by Jan > - return error from qemu_savevm_state_blocked rather than fixed EINVAL > at qemu_loadvm_state(), since it'a already using errno values > > v3: > > Daniel, adding you to see if libvirt cares about the difference in > whether the target exits on migration failure as noted above. If the 'migrate' command on the source QEMU returns an error, then libvirt will teardown the target QEMU automatically, so that's not a problem. Regards, Daniel
On Mon, 2011-01-10 at 10:24 +0000, Daniel P. Berrange wrote: > On Fri, Jan 07, 2011 at 03:13:25PM -0700, Alex Williamson wrote: > > The no_migrate save state flag is currently only checked in the > > last phase of migration. This means that we potentially waste > > a lot of time and bandwidth with the live state handlers before > > we ever check the no_migrate flags. The error message printed > > when we catch a non-migratable device doesn't get printed for > > a detached migration. And, no_migrate does nothing to prevent > > an incoming migration to a target that includes a non-migratable > > device. This attempts to fix all of these. > > > > One notable difference in behavior is that an outgoing migration > > now checks for non-migratable devices before ever connecting to > > the target system. This means the target will remain listening > > rather than exit from failure. > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > --- > > > > v4: > > - fix braces noted by Jan > > - return error from qemu_savevm_state_blocked rather than fixed EINVAL > > at qemu_loadvm_state(), since it'a already using errno values > > > > v3: > > > > Daniel, adding you to see if libvirt cares about the difference in > > whether the target exits on migration failure as noted above. > > If the 'migrate' command on the source QEMU returns an error, > then libvirt will teardown the target QEMU automatically, so > that's not a problem. Thanks, that's the way I was hoping it would work. Alex
On Sun, 2011-01-09 at 12:47 +0200, Michael S. Tsirkin wrote: > On Fri, Jan 07, 2011 at 03:13:25PM -0700, Alex Williamson wrote: > > The no_migrate save state flag is currently only checked in the > > last phase of migration. This means that we potentially waste > > a lot of time and bandwidth with the live state handlers before > > we ever check the no_migrate flags. The error message printed > > when we catch a non-migratable device doesn't get printed for > > a detached migration. And, no_migrate does nothing to prevent > > an incoming migration to a target that includes a non-migratable > > device. This attempts to fix all of these. > > > > One notable difference in behavior is that an outgoing migration > > now checks for non-migratable devices before ever connecting to > > the target system. This means the target will remain listening > > rather than exit from failure. > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > Minor nit: > > The API of qemu_savevm_state_blocked is a bit strange. > functions should either return 0/1 values or 0 on success > negative on failure or a bool. > This API asks "is it blocked" and returns -EINVAL to > mean "yes". _blocked is also a bit ambigious: > it seems to imply a temporary condition. But it very well could be a temporary condition. If I have an assigned device, it will block migration/savevm, but removing the device allows it to proceed. > How about we reverse the logic, call the new API > qemu_savevm_state_supported, qemu_savevm_state_enabled, > something like this? > Then you can return 0 if migration is possible, > -1 if not. I tend to like qemu_savevm_state_blocked() as "supported" and "enabled" invoke different ideas for me. I will concede that the errno return value is a little awkward. How about if I make it a bool function instead? Thanks, Alex > > --- > > > > v4: > > - fix braces noted by Jan > > - return error from qemu_savevm_state_blocked rather than fixed EINVAL > > at qemu_loadvm_state(), since it'a already using errno values > > > > v3: > > > > Daniel, adding you to see if libvirt cares about the difference in > > whether the target exits on migration failure as noted above. > > > > Also adding Michael as he's complained about the no_migrate check > > happening so late in the process. > > > > migration.c | 4 ++++ > > savevm.c | 41 +++++++++++++++++++++++++++-------------- > > sysemu.h | 1 + > > 3 files changed, 32 insertions(+), 14 deletions(-) > > > > diff --git a/migration.c b/migration.c > > index e5ba51c..d593b1d 100644 > > --- a/migration.c > > +++ b/migration.c > > @@ -88,6 +88,10 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data) > > return -1; > > } > > > > + if (qemu_savevm_state_blocked(mon)) { > > + return -1; > > + } > > + > > if (strstart(uri, "tcp:", &p)) { > > s = tcp_start_outgoing_migration(mon, p, max_throttle, detach, > > blk, inc); > > diff --git a/savevm.c b/savevm.c > > index 90aa237..34c0d1a 100644 > > --- a/savevm.c > > +++ b/savevm.c > > @@ -1401,19 +1401,13 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id) > > return vmstate_load_state(f, se->vmsd, se->opaque, version_id); > > } > > > > -static int vmstate_save(QEMUFile *f, SaveStateEntry *se) > > +static void vmstate_save(QEMUFile *f, SaveStateEntry *se) > > { > > - if (se->no_migrate) { > > - return -1; > > - } > > - > > if (!se->vmsd) { /* Old style */ > > se->save_state(f, se->opaque); > > - return 0; > > + return; > > } > > vmstate_save_state(f,se->vmsd, se->opaque); > > - > > - return 0; > > } > > > > #define QEMU_VM_FILE_MAGIC 0x5145564d > > @@ -1427,6 +1421,20 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se) > > #define QEMU_VM_SECTION_FULL 0x04 > > #define QEMU_VM_SUBSECTION 0x05 > > > > +int qemu_savevm_state_blocked(Monitor *mon) > > +{ > > + SaveStateEntry *se; > > + > > + QTAILQ_FOREACH(se, &savevm_handlers, entry) { > > + if (se->no_migrate) { > > + monitor_printf(mon, "state blocked by non-migratable device '%s'\n", > > + se->idstr); > > + return -EINVAL; > > + } > > + } > > + return 0; > > +} > > + > > int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable, > > int shared) > > { > > @@ -1508,7 +1516,6 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f) > > int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f) > > { > > SaveStateEntry *se; > > - int r; > > > > cpu_synchronize_all_states(); > > > > @@ -1541,11 +1548,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f) > > qemu_put_be32(f, se->instance_id); > > qemu_put_be32(f, se->version_id); > > > > - r = vmstate_save(f, se); > > - if (r < 0) { > > - monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr); > > - return r; > > - } > > + vmstate_save(f, se); > > } > > > > qemu_put_byte(f, QEMU_VM_EOF); > > @@ -1575,6 +1578,11 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f) > > saved_vm_running = vm_running; > > vm_stop(0); > > > > + ret = qemu_savevm_state_blocked(mon); > > + if (ret < 0) { > > + goto out; > > + } > > + > > ret = qemu_savevm_state_begin(mon, f, 0, 0); > > if (ret < 0) > > goto out; > > @@ -1692,6 +1700,11 @@ int qemu_loadvm_state(QEMUFile *f) > > unsigned int v; > > int ret; > > > > + ret = qemu_savevm_state_blocked(default_mon); > > + if (ret < 0) { > > + return ret; > > + } > > + > > v = qemu_get_be32(f); > > if (v != QEMU_VM_FILE_MAGIC) > > return -EINVAL; > > diff --git a/sysemu.h b/sysemu.h > > index e728ea1..eefaba5 100644 > > --- a/sysemu.h > > +++ b/sysemu.h > > @@ -75,6 +75,7 @@ void qemu_announce_self(void); > > > > void main_loop_wait(int nonblocking); > > > > +int qemu_savevm_state_blocked(Monitor *mon); > > int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable, > > int shared); > > int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f);
On Mon, Jan 10, 2011 at 10:47:58AM -0700, Alex Williamson wrote: > On Sun, 2011-01-09 at 12:47 +0200, Michael S. Tsirkin wrote: > > On Fri, Jan 07, 2011 at 03:13:25PM -0700, Alex Williamson wrote: > > > The no_migrate save state flag is currently only checked in the > > > last phase of migration. This means that we potentially waste > > > a lot of time and bandwidth with the live state handlers before > > > we ever check the no_migrate flags. The error message printed > > > when we catch a non-migratable device doesn't get printed for > > > a detached migration. And, no_migrate does nothing to prevent > > > an incoming migration to a target that includes a non-migratable > > > device. This attempts to fix all of these. > > > > > > One notable difference in behavior is that an outgoing migration > > > now checks for non-migratable devices before ever connecting to > > > the target system. This means the target will remain listening > > > rather than exit from failure. > > > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > > > Minor nit: > > > > The API of qemu_savevm_state_blocked is a bit strange. > > functions should either return 0/1 values or 0 on success > > negative on failure or a bool. > > This API asks "is it blocked" and returns -EINVAL to > > mean "yes". _blocked is also a bit ambigious: > > it seems to imply a temporary condition. > > But it very well could be a temporary condition. If I have an assigned > device, it will block migration/savevm, but removing the device allows > it to proceed. Well to me blocked it means migration will not fail, but will be blocked and unblocked later when device is removed. > > How about we reverse the logic, call the new API > > qemu_savevm_state_supported, qemu_savevm_state_enabled, > > something like this? > > Then you can return 0 if migration is possible, > > -1 if not. > > I tend to like qemu_savevm_state_blocked() as "supported" and "enabled" > invoke different ideas for me. I will concede that the errno return > value is a little awkward. How about if I make it a bool function > instead? Thanks, > > Alex That's better. > > > --- > > > > > > v4: > > > - fix braces noted by Jan > > > - return error from qemu_savevm_state_blocked rather than fixed EINVAL > > > at qemu_loadvm_state(), since it'a already using errno values > > > > > > v3: > > > > > > Daniel, adding you to see if libvirt cares about the difference in > > > whether the target exits on migration failure as noted above. > > > > > > Also adding Michael as he's complained about the no_migrate check > > > happening so late in the process. > > > > > > migration.c | 4 ++++ > > > savevm.c | 41 +++++++++++++++++++++++++++-------------- > > > sysemu.h | 1 + > > > 3 files changed, 32 insertions(+), 14 deletions(-) > > > > > > diff --git a/migration.c b/migration.c > > > index e5ba51c..d593b1d 100644 > > > --- a/migration.c > > > +++ b/migration.c > > > @@ -88,6 +88,10 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data) > > > return -1; > > > } > > > > > > + if (qemu_savevm_state_blocked(mon)) { > > > + return -1; > > > + } > > > + > > > if (strstart(uri, "tcp:", &p)) { > > > s = tcp_start_outgoing_migration(mon, p, max_throttle, detach, > > > blk, inc); > > > diff --git a/savevm.c b/savevm.c > > > index 90aa237..34c0d1a 100644 > > > --- a/savevm.c > > > +++ b/savevm.c > > > @@ -1401,19 +1401,13 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id) > > > return vmstate_load_state(f, se->vmsd, se->opaque, version_id); > > > } > > > > > > -static int vmstate_save(QEMUFile *f, SaveStateEntry *se) > > > +static void vmstate_save(QEMUFile *f, SaveStateEntry *se) > > > { > > > - if (se->no_migrate) { > > > - return -1; > > > - } > > > - > > > if (!se->vmsd) { /* Old style */ > > > se->save_state(f, se->opaque); > > > - return 0; > > > + return; > > > } > > > vmstate_save_state(f,se->vmsd, se->opaque); > > > - > > > - return 0; > > > } > > > > > > #define QEMU_VM_FILE_MAGIC 0x5145564d > > > @@ -1427,6 +1421,20 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se) > > > #define QEMU_VM_SECTION_FULL 0x04 > > > #define QEMU_VM_SUBSECTION 0x05 > > > > > > +int qemu_savevm_state_blocked(Monitor *mon) > > > +{ > > > + SaveStateEntry *se; > > > + > > > + QTAILQ_FOREACH(se, &savevm_handlers, entry) { > > > + if (se->no_migrate) { > > > + monitor_printf(mon, "state blocked by non-migratable device '%s'\n", > > > + se->idstr); > > > + return -EINVAL; > > > + } > > > + } > > > + return 0; > > > +} > > > + > > > int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable, > > > int shared) > > > { > > > @@ -1508,7 +1516,6 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f) > > > int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f) > > > { > > > SaveStateEntry *se; > > > - int r; > > > > > > cpu_synchronize_all_states(); > > > > > > @@ -1541,11 +1548,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f) > > > qemu_put_be32(f, se->instance_id); > > > qemu_put_be32(f, se->version_id); > > > > > > - r = vmstate_save(f, se); > > > - if (r < 0) { > > > - monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr); > > > - return r; > > > - } > > > + vmstate_save(f, se); > > > } > > > > > > qemu_put_byte(f, QEMU_VM_EOF); > > > @@ -1575,6 +1578,11 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f) > > > saved_vm_running = vm_running; > > > vm_stop(0); > > > > > > + ret = qemu_savevm_state_blocked(mon); > > > + if (ret < 0) { > > > + goto out; > > > + } > > > + > > > ret = qemu_savevm_state_begin(mon, f, 0, 0); > > > if (ret < 0) > > > goto out; > > > @@ -1692,6 +1700,11 @@ int qemu_loadvm_state(QEMUFile *f) > > > unsigned int v; > > > int ret; > > > > > > + ret = qemu_savevm_state_blocked(default_mon); > > > + if (ret < 0) { > > > + return ret; > > > + } > > > + > > > v = qemu_get_be32(f); > > > if (v != QEMU_VM_FILE_MAGIC) > > > return -EINVAL; > > > diff --git a/sysemu.h b/sysemu.h > > > index e728ea1..eefaba5 100644 > > > --- a/sysemu.h > > > +++ b/sysemu.h > > > @@ -75,6 +75,7 @@ void qemu_announce_self(void); > > > > > > void main_loop_wait(int nonblocking); > > > > > > +int qemu_savevm_state_blocked(Monitor *mon); > > > int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable, > > > int shared); > > > int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f); > >
diff --git a/migration.c b/migration.c index e5ba51c..d593b1d 100644 --- a/migration.c +++ b/migration.c @@ -88,6 +88,10 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data) return -1; } + if (qemu_savevm_state_blocked(mon)) { + return -1; + } + if (strstart(uri, "tcp:", &p)) { s = tcp_start_outgoing_migration(mon, p, max_throttle, detach, blk, inc); diff --git a/savevm.c b/savevm.c index 90aa237..34c0d1a 100644 --- a/savevm.c +++ b/savevm.c @@ -1401,19 +1401,13 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id) return vmstate_load_state(f, se->vmsd, se->opaque, version_id); } -static int vmstate_save(QEMUFile *f, SaveStateEntry *se) +static void vmstate_save(QEMUFile *f, SaveStateEntry *se) { - if (se->no_migrate) { - return -1; - } - if (!se->vmsd) { /* Old style */ se->save_state(f, se->opaque); - return 0; + return; } vmstate_save_state(f,se->vmsd, se->opaque); - - return 0; } #define QEMU_VM_FILE_MAGIC 0x5145564d @@ -1427,6 +1421,20 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se) #define QEMU_VM_SECTION_FULL 0x04 #define QEMU_VM_SUBSECTION 0x05 +int qemu_savevm_state_blocked(Monitor *mon) +{ + SaveStateEntry *se; + + QTAILQ_FOREACH(se, &savevm_handlers, entry) { + if (se->no_migrate) { + monitor_printf(mon, "state blocked by non-migratable device '%s'\n", + se->idstr); + return -EINVAL; + } + } + return 0; +} + int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable, int shared) { @@ -1508,7 +1516,6 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f) int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f) { SaveStateEntry *se; - int r; cpu_synchronize_all_states(); @@ -1541,11 +1548,7 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f) qemu_put_be32(f, se->instance_id); qemu_put_be32(f, se->version_id); - r = vmstate_save(f, se); - if (r < 0) { - monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr); - return r; - } + vmstate_save(f, se); } qemu_put_byte(f, QEMU_VM_EOF); @@ -1575,6 +1578,11 @@ static int qemu_savevm_state(Monitor *mon, QEMUFile *f) saved_vm_running = vm_running; vm_stop(0); + ret = qemu_savevm_state_blocked(mon); + if (ret < 0) { + goto out; + } + ret = qemu_savevm_state_begin(mon, f, 0, 0); if (ret < 0) goto out; @@ -1692,6 +1700,11 @@ int qemu_loadvm_state(QEMUFile *f) unsigned int v; int ret; + ret = qemu_savevm_state_blocked(default_mon); + if (ret < 0) { + return ret; + } + v = qemu_get_be32(f); if (v != QEMU_VM_FILE_MAGIC) return -EINVAL; diff --git a/sysemu.h b/sysemu.h index e728ea1..eefaba5 100644 --- a/sysemu.h +++ b/sysemu.h @@ -75,6 +75,7 @@ void qemu_announce_self(void); void main_loop_wait(int nonblocking); +int qemu_savevm_state_blocked(Monitor *mon); int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable, int shared); int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f);
The no_migrate save state flag is currently only checked in the last phase of migration. This means that we potentially waste a lot of time and bandwidth with the live state handlers before we ever check the no_migrate flags. The error message printed when we catch a non-migratable device doesn't get printed for a detached migration. And, no_migrate does nothing to prevent an incoming migration to a target that includes a non-migratable device. This attempts to fix all of these. One notable difference in behavior is that an outgoing migration now checks for non-migratable devices before ever connecting to the target system. This means the target will remain listening rather than exit from failure. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- v4: - fix braces noted by Jan - return error from qemu_savevm_state_blocked rather than fixed EINVAL at qemu_loadvm_state(), since it'a already using errno values v3: Daniel, adding you to see if libvirt cares about the difference in whether the target exits on migration failure as noted above. Also adding Michael as he's complained about the no_migrate check happening so late in the process. migration.c | 4 ++++ savevm.c | 41 +++++++++++++++++++++++++++-------------- sysemu.h | 1 + 3 files changed, 32 insertions(+), 14 deletions(-)