* [Qemu-devel] [PATCH v3 0/1] migration: disallow migrate_add_blocker during migration
@ 2016-12-09 17:25 John Snow
2016-12-09 17:25 ` [Qemu-devel] [PATCH v3 1/1] " John Snow
2016-12-09 17:50 ` [Qemu-devel] [PATCH v3 0/1] " no-reply
0 siblings, 2 replies; 8+ messages in thread
From: John Snow @ 2016-12-09 17:25 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, qemu-block, mst, jcody, quintela, armbru, groug,
aneesh.kumar, ashijeetacharya, pbonzini, amit.shah,
marcandre.lureau, dgilbert, John Snow
As discussed on-list, this may be a necessary building block for
a run-time flag that prevents us from getting into any situations
where we configure a non-migratable QEMU.
I'd like extra attention to be paid to the virtio-gpu, vhost, and
ivshmem changes as I am not perfectly confident in those.
Sorry for the wide CC distributions.
V3: Rebased from a version I sent about a year ago.
Changed the migration function names a bit,
Changed new calls to migrate_add_blocker.
John Snow (1):
migration: disallow migrate_add_blocker during migration
block/qcow.c | 6 +++++-
block/vdi.c | 6 +++++-
block/vhdx.c | 24 ++++++++++++++----------
block/vmdk.c | 7 ++++++-
block/vpc.c | 10 +++++++---
block/vvfat.c | 20 ++++++++++++--------
hw/9pfs/9p.c | 16 ++++++++++++----
hw/display/virtio-gpu.c | 29 ++++++++++++++++-------------
hw/misc/ivshmem.c | 11 +++++++----
hw/scsi/vhost-scsi.c | 25 +++++++++++++++++++------
hw/virtio/vhost.c | 8 +++++++-
include/migration/migration.h | 6 +++++-
migration/migration.c | 35 +++++++++++++++++++++++++++++++++--
stubs/migr-blocker.c | 3 ++-
target-i386/kvm.c | 16 +++++++++++++---
15 files changed, 163 insertions(+), 59 deletions(-)
--
2.9.3
^ permalink raw reply [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH v3 1/1] migration: disallow migrate_add_blocker during migration 2016-12-09 17:25 [Qemu-devel] [PATCH v3 0/1] migration: disallow migrate_add_blocker during migration John Snow @ 2016-12-09 17:25 ` John Snow 2016-12-09 17:39 ` Dr. David Alan Gilbert 2016-12-12 10:52 ` Greg Kurz 2016-12-09 17:50 ` [Qemu-devel] [PATCH v3 0/1] " no-reply 1 sibling, 2 replies; 8+ messages in thread From: John Snow @ 2016-12-09 17:25 UTC (permalink / raw) To: qemu-devel Cc: kwolf, qemu-block, mst, jcody, quintela, armbru, groug, aneesh.kumar, ashijeetacharya, pbonzini, amit.shah, marcandre.lureau, dgilbert, John Snow If a migration is already in progress and somebody attempts to add a migration blocker, this should rightly fail. Add an errp parameter and a retcode return value to migrate_add_blocker. Signed-off-by: John Snow <jsnow@redhat.com> --- block/qcow.c | 6 +++++- block/vdi.c | 6 +++++- block/vhdx.c | 24 ++++++++++++++---------- block/vmdk.c | 7 ++++++- block/vpc.c | 10 +++++++--- block/vvfat.c | 20 ++++++++++++-------- hw/9pfs/9p.c | 16 ++++++++++++---- hw/display/virtio-gpu.c | 29 ++++++++++++++++------------- hw/misc/ivshmem.c | 11 +++++++---- hw/scsi/vhost-scsi.c | 25 +++++++++++++++++++------ hw/virtio/vhost.c | 8 +++++++- include/migration/migration.h | 6 +++++- migration/migration.c | 35 +++++++++++++++++++++++++++++++++-- stubs/migr-blocker.c | 3 ++- target-i386/kvm.c | 16 +++++++++++++--- 15 files changed, 163 insertions(+), 59 deletions(-) diff --git a/block/qcow.c b/block/qcow.c index 7540f43..11526a1 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -252,7 +252,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, error_setg(&s->migration_blocker, "The qcow format used by node '%s' " "does not support live migration", bdrv_get_device_or_node_name(bs)); - migrate_add_blocker(s->migration_blocker); + ret = migrate_add_blocker(s->migration_blocker, errp); + if (ret < 0) { + error_free(s->migration_blocker); + goto fail; + } qemu_co_mutex_init(&s->lock); return 0; diff --git a/block/vdi.c b/block/vdi.c index 96b78d5..2b56f52 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -471,7 +471,11 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, error_setg(&s->migration_blocker, "The vdi format used by node '%s' " "does not support live migration", bdrv_get_device_or_node_name(bs)); - migrate_add_blocker(s->migration_blocker); + ret = migrate_add_blocker(s->migration_blocker, errp); + if (ret < 0) { + error_free(s->migration_blocker); + goto fail_free_bmap; + } qemu_co_mutex_init(&s->write_lock); diff --git a/block/vhdx.c b/block/vhdx.c index 0ba2f0a..d81941b 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -991,20 +991,24 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags, } } - if (flags & BDRV_O_RDWR) { - ret = vhdx_update_headers(bs, s, false, NULL); - if (ret < 0) { - goto fail; - } - } - - /* TODO: differencing files */ - /* Disable migration when VHDX images are used */ error_setg(&s->migration_blocker, "The vhdx format used by node '%s' " "does not support live migration", bdrv_get_device_or_node_name(bs)); - migrate_add_blocker(s->migration_blocker); + ret = migrate_add_blocker(s->migration_blocker, errp); + if (ret < 0) { + error_free(s->migration_blocker); + goto fail; + } + + if (flags & BDRV_O_RDWR) { + ret = vhdx_update_headers(bs, s, false, NULL); + if (ret < 0) { + goto fail; + } + } + + /* TODO: differencing files */ return 0; fail: diff --git a/block/vmdk.c b/block/vmdk.c index a11c27a..4a45a83 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -976,7 +976,12 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags, error_setg(&s->migration_blocker, "The vmdk format used by node '%s' " "does not support live migration", bdrv_get_device_or_node_name(bs)); - migrate_add_blocker(s->migration_blocker); + ret = migrate_add_blocker(s->migration_blocker, errp); + if (ret < 0) { + error_free(s->migration_blocker); + goto fail; + } + g_free(buf); return 0; diff --git a/block/vpc.c b/block/vpc.c index 8d5886f..299a8c8 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -422,13 +422,17 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, #endif } - qemu_co_mutex_init(&s->lock); - /* Disable migration when VHD images are used */ error_setg(&s->migration_blocker, "The vpc format used by node '%s' " "does not support live migration", bdrv_get_device_or_node_name(bs)); - migrate_add_blocker(s->migration_blocker); + ret = migrate_add_blocker(s->migration_blocker, errp); + if (ret < 0) { + error_free(s->migration_blocker); + goto fail; + } + + qemu_co_mutex_init(&s->lock); return 0; diff --git a/block/vvfat.c b/block/vvfat.c index ded2109..3de3320 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -1185,22 +1185,26 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags, s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; - if (s->first_sectors_number == 0x40) { - init_mbr(s, cyls, heads, secs); - } - - // assert(is_consistent(s)); - qemu_co_mutex_init(&s->lock); - /* Disable migration when vvfat is used rw */ if (s->qcow) { error_setg(&s->migration_blocker, "The vvfat (rw) format used by node '%s' " "does not support live migration", bdrv_get_device_or_node_name(bs)); - migrate_add_blocker(s->migration_blocker); + ret = migrate_add_blocker(s->migration_blocker, errp); + if (ret < 0) { + error_free(s->migration_blocker); + goto fail; + } } + if (s->first_sectors_number == 0x40) { + init_mbr(s, cyls, heads, secs); + } + + // assert(is_consistent(s)); + qemu_co_mutex_init(&s->lock); + ret = 0; fail: qemu_opts_del(opts); diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index faebd91..e72ef43 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -1013,20 +1013,28 @@ static void coroutine_fn v9fs_attach(void *opaque) goto out; } err += offset; - memcpy(&s->root_qid, &qid, sizeof(qid)); - trace_v9fs_attach_return(pdu->tag, pdu->id, - qid.type, qid.version, qid.path); + /* * disable migration if we haven't done already. * attach could get called multiple times for the same export. */ if (!s->migration_blocker) { + int ret; s->root_fid = fid; error_setg(&s->migration_blocker, "Migration is disabled when VirtFS export path '%s' is mounted in the guest using mount_tag '%s'", s->ctx.fs_root ? s->ctx.fs_root : "NULL", s->tag); - migrate_add_blocker(s->migration_blocker); + ret = migrate_add_blocker(s->migration_blocker, NULL); + if (ret < 0) { + err = ret; + clunk_fid(s, fid); + goto out; + } } + + memcpy(&s->root_qid, &qid, sizeof(qid)); + trace_v9fs_attach_return(pdu->tag, pdu->id, + qid.type, qid.version, qid.path); out: put_fid(pdu, fidp); out_nofid: diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index 5f32e1a..6e60b8c 100644 --- a/hw/display/virtio-gpu.c +++ b/hw/display/virtio-gpu.c @@ -1108,14 +1108,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) return; } - g->config_size = sizeof(struct virtio_gpu_config); - g->virtio_config.num_scanouts = g->conf.max_outputs; - virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU, - g->config_size); - - g->req_state[0].width = 1024; - g->req_state[0].height = 768; - g->use_virgl_renderer = false; #if !defined(CONFIG_VIRGL) || defined(HOST_WORDS_BIGENDIAN) have_virgl = false; @@ -1127,6 +1119,22 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) } if (virtio_gpu_virgl_enabled(g->conf)) { + error_setg(&g->migration_blocker, "virgl is not yet migratable"); + if (migrate_add_blocker(g->migration_blocker, errp) < 0) { + error_free(g->migration_blocker); + return; + } + } + + g->config_size = sizeof(struct virtio_gpu_config); + g->virtio_config.num_scanouts = g->conf.max_outputs; + virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU, + g->config_size); + + g->req_state[0].width = 1024; + g->req_state[0].height = 768; + + if (virtio_gpu_virgl_enabled(g->conf)) { /* use larger control queue in 3d mode */ g->ctrl_vq = virtio_add_queue(vdev, 256, virtio_gpu_handle_ctrl_cb); g->cursor_vq = virtio_add_queue(vdev, 16, virtio_gpu_handle_cursor_cb); @@ -1152,11 +1160,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) dpy_gfx_replace_surface(g->scanout[i].con, NULL); } } - - if (virtio_gpu_virgl_enabled(g->conf)) { - error_setg(&g->migration_blocker, "virgl is not yet migratable"); - migrate_add_blocker(g->migration_blocker); - } } static void virtio_gpu_device_unrealize(DeviceState *qdev, Error **errp) diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c index abeaf3d..585cc5b 100644 --- a/hw/misc/ivshmem.c +++ b/hw/misc/ivshmem.c @@ -903,9 +903,6 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp) } } - vmstate_register_ram(s->ivshmem_bar2, DEVICE(s)); - pci_register_bar(PCI_DEVICE(s), 2, attr, s->ivshmem_bar2); - if (s->master == ON_OFF_AUTO_AUTO) { s->master = s->vm_id == 0 ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF; } @@ -913,8 +910,14 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp) if (!ivshmem_is_master(s)) { error_setg(&s->migration_blocker, "Migration is disabled when using feature 'peer mode' in device 'ivshmem'"); - migrate_add_blocker(s->migration_blocker); + if (migrate_add_blocker(s->migration_blocker, errp) < 0) { + error_free(s->migration_blocker); + return; + } } + + vmstate_register_ram(s->ivshmem_bar2, DEVICE(s)); + pci_register_bar(PCI_DEVICE(s), 2, attr, s->ivshmem_bar2); } static void ivshmem_exit(PCIDevice *dev) diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c index 5b26946..ff503d0 100644 --- a/hw/scsi/vhost-scsi.c +++ b/hw/scsi/vhost-scsi.c @@ -238,8 +238,14 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) vhost_dummy_handle_output); if (err != NULL) { error_propagate(errp, err); - close(vhostfd); - return; + goto close_fd; + } + + error_setg(&s->migration_blocker, + "vhost-scsi does not support migration"); + ret = migrate_add_blocker(s->migration_blocker, errp); + if (ret < 0) { + goto free_blocker; } s->dev.nvqs = VHOST_SCSI_VQ_NUM_FIXED + vs->conf.num_queues; @@ -252,7 +258,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) if (ret < 0) { error_setg(errp, "vhost-scsi: vhost initialization failed: %s", strerror(-ret)); - return; + goto free_vqs; } /* At present, channel and lun both are 0 for bootable vhost-scsi disk */ @@ -261,9 +267,16 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) /* Note: we can also get the minimum tpgt from kernel */ s->target = vs->conf.boot_tpgt; - error_setg(&s->migration_blocker, - "vhost-scsi does not support migration"); - migrate_add_blocker(s->migration_blocker); + return; + + free_vqs: + migrate_del_blocker(s->migration_blocker); + g_free(s->dev.vqs); + free_blocker: + error_free(s->migration_blocker); + close_fd: + close(vhostfd); + return; } static void vhost_scsi_unrealize(DeviceState *dev, Error **errp) diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index f7f7023..416fada 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1157,7 +1157,13 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, } if (hdev->migration_blocker != NULL) { - migrate_add_blocker(hdev->migration_blocker); + Error *local_err; + r = migrate_add_blocker(hdev->migration_blocker, &local_err); + if (r < 0) { + error_report_err(local_err); + error_free(hdev->migration_blocker); + goto fail_busyloop; + } } hdev->mem = g_malloc0(offsetof(struct vhost_memory, regions)); diff --git a/include/migration/migration.h b/include/migration/migration.h index c309d23..8a91215 100644 --- a/include/migration/migration.h +++ b/include/migration/migration.h @@ -240,6 +240,7 @@ void remove_migration_state_change_notifier(Notifier *notify); MigrationState *migrate_init(const MigrationParams *params); bool migration_is_blocked(Error **errp); bool migration_in_setup(MigrationState *); +bool migration_is_idle(MigrationState *s); bool migration_has_finished(MigrationState *); bool migration_has_failed(MigrationState *); /* True if outgoing migration has entered postcopy phase */ @@ -284,8 +285,11 @@ int ram_postcopy_incoming_init(MigrationIncomingState *mis); * @migrate_add_blocker - prevent migration from proceeding * * @reason - an error to be returned whenever migration is attempted + * @errp - [out] The reason (if any) we cannot block migration right now. + * + * @returns - 0 on success, -EBUSY on failure, with errp set. */ -void migrate_add_blocker(Error *reason); +int migrate_add_blocker(Error *reason, Error **errp); /** * @migrate_del_blocker - remove a blocking error from migration diff --git a/migration/migration.c b/migration/migration.c index f498ab8..713e012 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1044,6 +1044,31 @@ bool migration_in_postcopy_after_devices(MigrationState *s) return migration_in_postcopy(s) && s->postcopy_after_devices; } +bool migration_is_idle(MigrationState *s) +{ + if (!s) { + s = migrate_get_current(); + } + + switch (s->state) { + case MIGRATION_STATUS_NONE: + case MIGRATION_STATUS_CANCELLED: + case MIGRATION_STATUS_COMPLETED: + case MIGRATION_STATUS_FAILED: + return true; + case MIGRATION_STATUS_SETUP: + case MIGRATION_STATUS_CANCELLING: + case MIGRATION_STATUS_ACTIVE: + case MIGRATION_STATUS_POSTCOPY_ACTIVE: + case MIGRATION_STATUS_COLO: + return false; + case MIGRATION_STATUS__MAX: + g_assert_not_reached(); + } + + return false; +} + MigrationState *migrate_init(const MigrationParams *params) { MigrationState *s = migrate_get_current(); @@ -1086,9 +1111,15 @@ MigrationState *migrate_init(const MigrationParams *params) static GSList *migration_blockers; -void migrate_add_blocker(Error *reason) +int migrate_add_blocker(Error *reason, Error **errp) { - migration_blockers = g_slist_prepend(migration_blockers, reason); + if (migration_is_idle(NULL)) { + migration_blockers = g_slist_prepend(migration_blockers, reason); + return 0; + } + + error_setg(errp, "Cannot block a migration already in-progress"); + return -EBUSY; } void migrate_del_blocker(Error *reason) diff --git a/stubs/migr-blocker.c b/stubs/migr-blocker.c index 8ab3604..a5ba18f 100644 --- a/stubs/migr-blocker.c +++ b/stubs/migr-blocker.c @@ -2,8 +2,9 @@ #include "qemu-common.h" #include "migration/migration.h" -void migrate_add_blocker(Error *reason) +int migrate_add_blocker(Error *reason, Error **errp) { + return 0; } void migrate_del_blocker(Error *reason) diff --git a/target-i386/kvm.c b/target-i386/kvm.c index f62264a..8abb54d 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -961,7 +961,11 @@ int kvm_arch_init_vcpu(CPUState *cs) error_setg(&invtsc_mig_blocker, "State blocked by non-migratable CPU device" " (invtsc flag)"); - migrate_add_blocker(invtsc_mig_blocker); + r = migrate_add_blocker(invtsc_mig_blocker, NULL); + if (r < 0) { + fprintf(stderr, "migrate_add_blocker failed\n"); + goto efail; + } /* for savevm */ vmstate_x86_cpu.unmigratable = 1; } @@ -969,12 +973,12 @@ int kvm_arch_init_vcpu(CPUState *cs) cpuid_data.cpuid.padding = 0; r = kvm_vcpu_ioctl(cs, KVM_SET_CPUID2, &cpuid_data); if (r) { - return r; + goto fail; } r = kvm_arch_set_tsc_khz(cs); if (r < 0) { - return r; + goto fail; } /* vcpu's TSC frequency is either specified by user, or following @@ -1001,6 +1005,12 @@ int kvm_arch_init_vcpu(CPUState *cs) } return 0; + + fail: + migrate_del_blocker(invtsc_mig_blocker); + efail: + error_free(invtsc_mig_blocker); + return r; } void kvm_arch_reset_vcpu(X86CPU *cpu) -- 2.9.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/1] migration: disallow migrate_add_blocker during migration 2016-12-09 17:25 ` [Qemu-devel] [PATCH v3 1/1] " John Snow @ 2016-12-09 17:39 ` Dr. David Alan Gilbert 2016-12-09 21:10 ` John Snow 2016-12-12 10:52 ` Greg Kurz 1 sibling, 1 reply; 8+ messages in thread From: Dr. David Alan Gilbert @ 2016-12-09 17:39 UTC (permalink / raw) To: John Snow Cc: qemu-devel, kwolf, qemu-block, mst, jcody, quintela, armbru, groug, aneesh.kumar, ashijeetacharya, pbonzini, amit.shah, marcandre.lureau * John Snow (jsnow@redhat.com) wrote: > If a migration is already in progress and somebody attempts > to add a migration blocker, this should rightly fail. > > Add an errp parameter and a retcode return value to migrate_add_blocker. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/qcow.c | 6 +++++- > block/vdi.c | 6 +++++- > block/vhdx.c | 24 ++++++++++++++---------- > block/vmdk.c | 7 ++++++- > block/vpc.c | 10 +++++++--- > block/vvfat.c | 20 ++++++++++++-------- > hw/9pfs/9p.c | 16 ++++++++++++---- > hw/display/virtio-gpu.c | 29 ++++++++++++++++------------- > hw/misc/ivshmem.c | 11 +++++++---- > hw/scsi/vhost-scsi.c | 25 +++++++++++++++++++------ > hw/virtio/vhost.c | 8 +++++++- > include/migration/migration.h | 6 +++++- > migration/migration.c | 35 +++++++++++++++++++++++++++++++++-- > stubs/migr-blocker.c | 3 ++- > target-i386/kvm.c | 16 +++++++++++++--- > 15 files changed, 163 insertions(+), 59 deletions(-) > > diff --git a/block/qcow.c b/block/qcow.c > index 7540f43..11526a1 100644 > --- a/block/qcow.c > +++ b/block/qcow.c > @@ -252,7 +252,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, > error_setg(&s->migration_blocker, "The qcow format used by node '%s' " > "does not support live migration", > bdrv_get_device_or_node_name(bs)); > - migrate_add_blocker(s->migration_blocker); > + ret = migrate_add_blocker(s->migration_blocker, errp); > + if (ret < 0) { > + error_free(s->migration_blocker); > + goto fail; > + } > > qemu_co_mutex_init(&s->lock); > return 0; > diff --git a/block/vdi.c b/block/vdi.c > index 96b78d5..2b56f52 100644 > --- a/block/vdi.c > +++ b/block/vdi.c > @@ -471,7 +471,11 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, > error_setg(&s->migration_blocker, "The vdi format used by node '%s' " > "does not support live migration", > bdrv_get_device_or_node_name(bs)); > - migrate_add_blocker(s->migration_blocker); > + ret = migrate_add_blocker(s->migration_blocker, errp); > + if (ret < 0) { > + error_free(s->migration_blocker); > + goto fail_free_bmap; > + } > > qemu_co_mutex_init(&s->write_lock); > > diff --git a/block/vhdx.c b/block/vhdx.c > index 0ba2f0a..d81941b 100644 > --- a/block/vhdx.c > +++ b/block/vhdx.c > @@ -991,20 +991,24 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags, > } > } > > - if (flags & BDRV_O_RDWR) { > - ret = vhdx_update_headers(bs, s, false, NULL); > - if (ret < 0) { > - goto fail; > - } > - } > - > - /* TODO: differencing files */ > - > /* Disable migration when VHDX images are used */ > error_setg(&s->migration_blocker, "The vhdx format used by node '%s' " > "does not support live migration", > bdrv_get_device_or_node_name(bs)); > - migrate_add_blocker(s->migration_blocker); > + ret = migrate_add_blocker(s->migration_blocker, errp); > + if (ret < 0) { > + error_free(s->migration_blocker); > + goto fail; > + } > + > + if (flags & BDRV_O_RDWR) { > + ret = vhdx_update_headers(bs, s, false, NULL); > + if (ret < 0) { > + goto fail; > + } > + } > + > + /* TODO: differencing files */ > > return 0; > fail: > diff --git a/block/vmdk.c b/block/vmdk.c > index a11c27a..4a45a83 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -976,7 +976,12 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags, > error_setg(&s->migration_blocker, "The vmdk format used by node '%s' " > "does not support live migration", > bdrv_get_device_or_node_name(bs)); > - migrate_add_blocker(s->migration_blocker); > + ret = migrate_add_blocker(s->migration_blocker, errp); > + if (ret < 0) { > + error_free(s->migration_blocker); > + goto fail; > + } > + > g_free(buf); > return 0; > > diff --git a/block/vpc.c b/block/vpc.c > index 8d5886f..299a8c8 100644 > --- a/block/vpc.c > +++ b/block/vpc.c > @@ -422,13 +422,17 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > #endif > } > > - qemu_co_mutex_init(&s->lock); > - > /* Disable migration when VHD images are used */ > error_setg(&s->migration_blocker, "The vpc format used by node '%s' " > "does not support live migration", > bdrv_get_device_or_node_name(bs)); > - migrate_add_blocker(s->migration_blocker); > + ret = migrate_add_blocker(s->migration_blocker, errp); > + if (ret < 0) { > + error_free(s->migration_blocker); > + goto fail; > + } > + > + qemu_co_mutex_init(&s->lock); > > return 0; > > diff --git a/block/vvfat.c b/block/vvfat.c > index ded2109..3de3320 100644 > --- a/block/vvfat.c > +++ b/block/vvfat.c > @@ -1185,22 +1185,26 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags, > > s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; > > - if (s->first_sectors_number == 0x40) { > - init_mbr(s, cyls, heads, secs); > - } > - > - // assert(is_consistent(s)); > - qemu_co_mutex_init(&s->lock); > - > /* Disable migration when vvfat is used rw */ > if (s->qcow) { > error_setg(&s->migration_blocker, > "The vvfat (rw) format used by node '%s' " > "does not support live migration", > bdrv_get_device_or_node_name(bs)); > - migrate_add_blocker(s->migration_blocker); > + ret = migrate_add_blocker(s->migration_blocker, errp); > + if (ret < 0) { > + error_free(s->migration_blocker); > + goto fail; > + } > } > > + if (s->first_sectors_number == 0x40) { > + init_mbr(s, cyls, heads, secs); > + } > + > + // assert(is_consistent(s)); > + qemu_co_mutex_init(&s->lock); > + > ret = 0; > fail: > qemu_opts_del(opts); > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index faebd91..e72ef43 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -1013,20 +1013,28 @@ static void coroutine_fn v9fs_attach(void *opaque) > goto out; > } > err += offset; > - memcpy(&s->root_qid, &qid, sizeof(qid)); > - trace_v9fs_attach_return(pdu->tag, pdu->id, > - qid.type, qid.version, qid.path); > + > /* > * disable migration if we haven't done already. > * attach could get called multiple times for the same export. > */ > if (!s->migration_blocker) { > + int ret; > s->root_fid = fid; > error_setg(&s->migration_blocker, > "Migration is disabled when VirtFS export path '%s' is mounted in the guest using mount_tag '%s'", > s->ctx.fs_root ? s->ctx.fs_root : "NULL", s->tag); > - migrate_add_blocker(s->migration_blocker); > + ret = migrate_add_blocker(s->migration_blocker, NULL); > + if (ret < 0) { > + err = ret; > + clunk_fid(s, fid); > + goto out; > + } > } > + > + memcpy(&s->root_qid, &qid, sizeof(qid)); > + trace_v9fs_attach_return(pdu->tag, pdu->id, > + qid.type, qid.version, qid.path); > out: > put_fid(pdu, fidp); > out_nofid: > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > index 5f32e1a..6e60b8c 100644 > --- a/hw/display/virtio-gpu.c > +++ b/hw/display/virtio-gpu.c > @@ -1108,14 +1108,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > return; > } > > - g->config_size = sizeof(struct virtio_gpu_config); > - g->virtio_config.num_scanouts = g->conf.max_outputs; > - virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU, > - g->config_size); > - > - g->req_state[0].width = 1024; > - g->req_state[0].height = 768; > - > g->use_virgl_renderer = false; > #if !defined(CONFIG_VIRGL) || defined(HOST_WORDS_BIGENDIAN) > have_virgl = false; > @@ -1127,6 +1119,22 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > } > > if (virtio_gpu_virgl_enabled(g->conf)) { > + error_setg(&g->migration_blocker, "virgl is not yet migratable"); > + if (migrate_add_blocker(g->migration_blocker, errp) < 0) { > + error_free(g->migration_blocker); > + return; > + } > + } > + > + g->config_size = sizeof(struct virtio_gpu_config); > + g->virtio_config.num_scanouts = g->conf.max_outputs; > + virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU, > + g->config_size); > + > + g->req_state[0].width = 1024; > + g->req_state[0].height = 768; > + > + if (virtio_gpu_virgl_enabled(g->conf)) { > /* use larger control queue in 3d mode */ > g->ctrl_vq = virtio_add_queue(vdev, 256, virtio_gpu_handle_ctrl_cb); > g->cursor_vq = virtio_add_queue(vdev, 16, virtio_gpu_handle_cursor_cb); > @@ -1152,11 +1160,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > dpy_gfx_replace_surface(g->scanout[i].con, NULL); > } > } > - > - if (virtio_gpu_virgl_enabled(g->conf)) { > - error_setg(&g->migration_blocker, "virgl is not yet migratable"); > - migrate_add_blocker(g->migration_blocker); > - } > } > > static void virtio_gpu_device_unrealize(DeviceState *qdev, Error **errp) > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > index abeaf3d..585cc5b 100644 > --- a/hw/misc/ivshmem.c > +++ b/hw/misc/ivshmem.c > @@ -903,9 +903,6 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp) > } > } > > - vmstate_register_ram(s->ivshmem_bar2, DEVICE(s)); > - pci_register_bar(PCI_DEVICE(s), 2, attr, s->ivshmem_bar2); > - > if (s->master == ON_OFF_AUTO_AUTO) { > s->master = s->vm_id == 0 ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF; > } > @@ -913,8 +910,14 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp) > if (!ivshmem_is_master(s)) { > error_setg(&s->migration_blocker, > "Migration is disabled when using feature 'peer mode' in device 'ivshmem'"); > - migrate_add_blocker(s->migration_blocker); > + if (migrate_add_blocker(s->migration_blocker, errp) < 0) { > + error_free(s->migration_blocker); > + return; > + } > } > + > + vmstate_register_ram(s->ivshmem_bar2, DEVICE(s)); > + pci_register_bar(PCI_DEVICE(s), 2, attr, s->ivshmem_bar2); > } > > static void ivshmem_exit(PCIDevice *dev) > diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c > index 5b26946..ff503d0 100644 > --- a/hw/scsi/vhost-scsi.c > +++ b/hw/scsi/vhost-scsi.c > @@ -238,8 +238,14 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) > vhost_dummy_handle_output); > if (err != NULL) { > error_propagate(errp, err); > - close(vhostfd); > - return; > + goto close_fd; > + } > + > + error_setg(&s->migration_blocker, > + "vhost-scsi does not support migration"); > + ret = migrate_add_blocker(s->migration_blocker, errp); > + if (ret < 0) { > + goto free_blocker; > } > > s->dev.nvqs = VHOST_SCSI_VQ_NUM_FIXED + vs->conf.num_queues; > @@ -252,7 +258,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) > if (ret < 0) { > error_setg(errp, "vhost-scsi: vhost initialization failed: %s", > strerror(-ret)); > - return; > + goto free_vqs; > } > > /* At present, channel and lun both are 0 for bootable vhost-scsi disk */ > @@ -261,9 +267,16 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) > /* Note: we can also get the minimum tpgt from kernel */ > s->target = vs->conf.boot_tpgt; > > - error_setg(&s->migration_blocker, > - "vhost-scsi does not support migration"); > - migrate_add_blocker(s->migration_blocker); > + return; > + > + free_vqs: > + migrate_del_blocker(s->migration_blocker); > + g_free(s->dev.vqs); > + free_blocker: > + error_free(s->migration_blocker); > + close_fd: > + close(vhostfd); > + return; > } > > static void vhost_scsi_unrealize(DeviceState *dev, Error **errp) > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index f7f7023..416fada 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -1157,7 +1157,13 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, > } > > if (hdev->migration_blocker != NULL) { > - migrate_add_blocker(hdev->migration_blocker); > + Error *local_err; > + r = migrate_add_blocker(hdev->migration_blocker, &local_err); > + if (r < 0) { > + error_report_err(local_err); > + error_free(hdev->migration_blocker); > + goto fail_busyloop; > + } > } > > hdev->mem = g_malloc0(offsetof(struct vhost_memory, regions)); > diff --git a/include/migration/migration.h b/include/migration/migration.h > index c309d23..8a91215 100644 > --- a/include/migration/migration.h > +++ b/include/migration/migration.h > @@ -240,6 +240,7 @@ void remove_migration_state_change_notifier(Notifier *notify); > MigrationState *migrate_init(const MigrationParams *params); > bool migration_is_blocked(Error **errp); > bool migration_in_setup(MigrationState *); > +bool migration_is_idle(MigrationState *s); > bool migration_has_finished(MigrationState *); > bool migration_has_failed(MigrationState *); > /* True if outgoing migration has entered postcopy phase */ > @@ -284,8 +285,11 @@ int ram_postcopy_incoming_init(MigrationIncomingState *mis); > * @migrate_add_blocker - prevent migration from proceeding > * > * @reason - an error to be returned whenever migration is attempted > + * @errp - [out] The reason (if any) we cannot block migration right now. > + * > + * @returns - 0 on success, -EBUSY on failure, with errp set. > */ > -void migrate_add_blocker(Error *reason); > +int migrate_add_blocker(Error *reason, Error **errp); > > /** > * @migrate_del_blocker - remove a blocking error from migration > diff --git a/migration/migration.c b/migration/migration.c > index f498ab8..713e012 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -1044,6 +1044,31 @@ bool migration_in_postcopy_after_devices(MigrationState *s) > return migration_in_postcopy(s) && s->postcopy_after_devices; > } > > +bool migration_is_idle(MigrationState *s) > +{ > + if (!s) { > + s = migrate_get_current(); > + } > + > + switch (s->state) { > + case MIGRATION_STATUS_NONE: > + case MIGRATION_STATUS_CANCELLED: > + case MIGRATION_STATUS_COMPLETED: > + case MIGRATION_STATUS_FAILED: > + return true; > + case MIGRATION_STATUS_SETUP: > + case MIGRATION_STATUS_CANCELLING: > + case MIGRATION_STATUS_ACTIVE: > + case MIGRATION_STATUS_POSTCOPY_ACTIVE: > + case MIGRATION_STATUS_COLO: > + return false; > + case MIGRATION_STATUS__MAX: > + g_assert_not_reached(); > + } > + > + return false; > +} > + > MigrationState *migrate_init(const MigrationParams *params) > { > MigrationState *s = migrate_get_current(); > @@ -1086,9 +1111,15 @@ MigrationState *migrate_init(const MigrationParams *params) > > static GSList *migration_blockers; > > -void migrate_add_blocker(Error *reason) > +int migrate_add_blocker(Error *reason, Error **errp) > { > - migration_blockers = g_slist_prepend(migration_blockers, reason); > + if (migration_is_idle(NULL)) { > + migration_blockers = g_slist_prepend(migration_blockers, reason); > + return 0; > + } > + > + error_setg(errp, "Cannot block a migration already in-progress"); I wonder if you need to add the 'reason' here as well; otherwise you get this message about blocking a migration but don't know what wanted to block it. So perhaps set errp from reason and then use error_prepend to add that extra message? Dave > + return -EBUSY; > } > > void migrate_del_blocker(Error *reason) > diff --git a/stubs/migr-blocker.c b/stubs/migr-blocker.c > index 8ab3604..a5ba18f 100644 > --- a/stubs/migr-blocker.c > +++ b/stubs/migr-blocker.c > @@ -2,8 +2,9 @@ > #include "qemu-common.h" > #include "migration/migration.h" > > -void migrate_add_blocker(Error *reason) > +int migrate_add_blocker(Error *reason, Error **errp) > { > + return 0; > } > > void migrate_del_blocker(Error *reason) > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > index f62264a..8abb54d 100644 > --- a/target-i386/kvm.c > +++ b/target-i386/kvm.c > @@ -961,7 +961,11 @@ int kvm_arch_init_vcpu(CPUState *cs) > error_setg(&invtsc_mig_blocker, > "State blocked by non-migratable CPU device" > " (invtsc flag)"); > - migrate_add_blocker(invtsc_mig_blocker); > + r = migrate_add_blocker(invtsc_mig_blocker, NULL); > + if (r < 0) { > + fprintf(stderr, "migrate_add_blocker failed\n"); > + goto efail; > + } > /* for savevm */ > vmstate_x86_cpu.unmigratable = 1; > } > @@ -969,12 +973,12 @@ int kvm_arch_init_vcpu(CPUState *cs) > cpuid_data.cpuid.padding = 0; > r = kvm_vcpu_ioctl(cs, KVM_SET_CPUID2, &cpuid_data); > if (r) { > - return r; > + goto fail; > } > > r = kvm_arch_set_tsc_khz(cs); > if (r < 0) { > - return r; > + goto fail; > } > > /* vcpu's TSC frequency is either specified by user, or following > @@ -1001,6 +1005,12 @@ int kvm_arch_init_vcpu(CPUState *cs) > } > > return 0; > + > + fail: > + migrate_del_blocker(invtsc_mig_blocker); > + efail: > + error_free(invtsc_mig_blocker); > + return r; > } > > void kvm_arch_reset_vcpu(X86CPU *cpu) > -- > 2.9.3 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/1] migration: disallow migrate_add_blocker during migration 2016-12-09 17:39 ` Dr. David Alan Gilbert @ 2016-12-09 21:10 ` John Snow 0 siblings, 0 replies; 8+ messages in thread From: John Snow @ 2016-12-09 21:10 UTC (permalink / raw) To: Dr. David Alan Gilbert Cc: qemu-devel, kwolf, qemu-block, mst, jcody, quintela, armbru, groug, aneesh.kumar, ashijeetacharya, pbonzini, amit.shah, marcandre.lureau On 12/09/2016 12:39 PM, Dr. David Alan Gilbert wrote: > * John Snow (jsnow@redhat.com) wrote: >> If a migration is already in progress and somebody attempts >> to add a migration blocker, this should rightly fail. >> >> Add an errp parameter and a retcode return value to migrate_add_blocker. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block/qcow.c | 6 +++++- >> block/vdi.c | 6 +++++- >> block/vhdx.c | 24 ++++++++++++++---------- >> block/vmdk.c | 7 ++++++- >> block/vpc.c | 10 +++++++--- >> block/vvfat.c | 20 ++++++++++++-------- >> hw/9pfs/9p.c | 16 ++++++++++++---- >> hw/display/virtio-gpu.c | 29 ++++++++++++++++------------- >> hw/misc/ivshmem.c | 11 +++++++---- >> hw/scsi/vhost-scsi.c | 25 +++++++++++++++++++------ >> hw/virtio/vhost.c | 8 +++++++- >> include/migration/migration.h | 6 +++++- >> migration/migration.c | 35 +++++++++++++++++++++++++++++++++-- >> stubs/migr-blocker.c | 3 ++- >> target-i386/kvm.c | 16 +++++++++++++--- >> 15 files changed, 163 insertions(+), 59 deletions(-) >> >> diff --git a/block/qcow.c b/block/qcow.c >> index 7540f43..11526a1 100644 >> --- a/block/qcow.c >> +++ b/block/qcow.c >> @@ -252,7 +252,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, >> error_setg(&s->migration_blocker, "The qcow format used by node '%s' " >> "does not support live migration", >> bdrv_get_device_or_node_name(bs)); >> - migrate_add_blocker(s->migration_blocker); >> + ret = migrate_add_blocker(s->migration_blocker, errp); >> + if (ret < 0) { >> + error_free(s->migration_blocker); >> + goto fail; >> + } >> >> qemu_co_mutex_init(&s->lock); >> return 0; >> diff --git a/block/vdi.c b/block/vdi.c >> index 96b78d5..2b56f52 100644 >> --- a/block/vdi.c >> +++ b/block/vdi.c >> @@ -471,7 +471,11 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, >> error_setg(&s->migration_blocker, "The vdi format used by node '%s' " >> "does not support live migration", >> bdrv_get_device_or_node_name(bs)); >> - migrate_add_blocker(s->migration_blocker); >> + ret = migrate_add_blocker(s->migration_blocker, errp); >> + if (ret < 0) { >> + error_free(s->migration_blocker); >> + goto fail_free_bmap; >> + } >> >> qemu_co_mutex_init(&s->write_lock); >> >> diff --git a/block/vhdx.c b/block/vhdx.c >> index 0ba2f0a..d81941b 100644 >> --- a/block/vhdx.c >> +++ b/block/vhdx.c >> @@ -991,20 +991,24 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags, >> } >> } >> >> - if (flags & BDRV_O_RDWR) { >> - ret = vhdx_update_headers(bs, s, false, NULL); >> - if (ret < 0) { >> - goto fail; >> - } >> - } >> - >> - /* TODO: differencing files */ >> - >> /* Disable migration when VHDX images are used */ >> error_setg(&s->migration_blocker, "The vhdx format used by node '%s' " >> "does not support live migration", >> bdrv_get_device_or_node_name(bs)); >> - migrate_add_blocker(s->migration_blocker); >> + ret = migrate_add_blocker(s->migration_blocker, errp); >> + if (ret < 0) { >> + error_free(s->migration_blocker); >> + goto fail; >> + } >> + >> + if (flags & BDRV_O_RDWR) { >> + ret = vhdx_update_headers(bs, s, false, NULL); >> + if (ret < 0) { >> + goto fail; >> + } >> + } >> + >> + /* TODO: differencing files */ >> >> return 0; >> fail: >> diff --git a/block/vmdk.c b/block/vmdk.c >> index a11c27a..4a45a83 100644 >> --- a/block/vmdk.c >> +++ b/block/vmdk.c >> @@ -976,7 +976,12 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags, >> error_setg(&s->migration_blocker, "The vmdk format used by node '%s' " >> "does not support live migration", >> bdrv_get_device_or_node_name(bs)); >> - migrate_add_blocker(s->migration_blocker); >> + ret = migrate_add_blocker(s->migration_blocker, errp); >> + if (ret < 0) { >> + error_free(s->migration_blocker); >> + goto fail; >> + } >> + >> g_free(buf); >> return 0; >> >> diff --git a/block/vpc.c b/block/vpc.c >> index 8d5886f..299a8c8 100644 >> --- a/block/vpc.c >> +++ b/block/vpc.c >> @@ -422,13 +422,17 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, >> #endif >> } >> >> - qemu_co_mutex_init(&s->lock); >> - >> /* Disable migration when VHD images are used */ >> error_setg(&s->migration_blocker, "The vpc format used by node '%s' " >> "does not support live migration", >> bdrv_get_device_or_node_name(bs)); >> - migrate_add_blocker(s->migration_blocker); >> + ret = migrate_add_blocker(s->migration_blocker, errp); >> + if (ret < 0) { >> + error_free(s->migration_blocker); >> + goto fail; >> + } >> + >> + qemu_co_mutex_init(&s->lock); >> >> return 0; >> >> diff --git a/block/vvfat.c b/block/vvfat.c >> index ded2109..3de3320 100644 >> --- a/block/vvfat.c >> +++ b/block/vvfat.c >> @@ -1185,22 +1185,26 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags, >> >> s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; >> >> - if (s->first_sectors_number == 0x40) { >> - init_mbr(s, cyls, heads, secs); >> - } >> - >> - // assert(is_consistent(s)); >> - qemu_co_mutex_init(&s->lock); >> - >> /* Disable migration when vvfat is used rw */ >> if (s->qcow) { >> error_setg(&s->migration_blocker, >> "The vvfat (rw) format used by node '%s' " >> "does not support live migration", >> bdrv_get_device_or_node_name(bs)); >> - migrate_add_blocker(s->migration_blocker); >> + ret = migrate_add_blocker(s->migration_blocker, errp); >> + if (ret < 0) { >> + error_free(s->migration_blocker); >> + goto fail; >> + } >> } >> >> + if (s->first_sectors_number == 0x40) { >> + init_mbr(s, cyls, heads, secs); >> + } >> + >> + // assert(is_consistent(s)); >> + qemu_co_mutex_init(&s->lock); >> + >> ret = 0; >> fail: >> qemu_opts_del(opts); >> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c >> index faebd91..e72ef43 100644 >> --- a/hw/9pfs/9p.c >> +++ b/hw/9pfs/9p.c >> @@ -1013,20 +1013,28 @@ static void coroutine_fn v9fs_attach(void *opaque) >> goto out; >> } >> err += offset; >> - memcpy(&s->root_qid, &qid, sizeof(qid)); >> - trace_v9fs_attach_return(pdu->tag, pdu->id, >> - qid.type, qid.version, qid.path); >> + >> /* >> * disable migration if we haven't done already. >> * attach could get called multiple times for the same export. >> */ >> if (!s->migration_blocker) { >> + int ret; >> s->root_fid = fid; >> error_setg(&s->migration_blocker, >> "Migration is disabled when VirtFS export path '%s' is mounted in the guest using mount_tag '%s'", >> s->ctx.fs_root ? s->ctx.fs_root : "NULL", s->tag); >> - migrate_add_blocker(s->migration_blocker); >> + ret = migrate_add_blocker(s->migration_blocker, NULL); >> + if (ret < 0) { >> + err = ret; >> + clunk_fid(s, fid); >> + goto out; >> + } >> } >> + >> + memcpy(&s->root_qid, &qid, sizeof(qid)); >> + trace_v9fs_attach_return(pdu->tag, pdu->id, >> + qid.type, qid.version, qid.path); >> out: >> put_fid(pdu, fidp); >> out_nofid: >> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c >> index 5f32e1a..6e60b8c 100644 >> --- a/hw/display/virtio-gpu.c >> +++ b/hw/display/virtio-gpu.c >> @@ -1108,14 +1108,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) >> return; >> } >> >> - g->config_size = sizeof(struct virtio_gpu_config); >> - g->virtio_config.num_scanouts = g->conf.max_outputs; >> - virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU, >> - g->config_size); >> - >> - g->req_state[0].width = 1024; >> - g->req_state[0].height = 768; >> - >> g->use_virgl_renderer = false; >> #if !defined(CONFIG_VIRGL) || defined(HOST_WORDS_BIGENDIAN) >> have_virgl = false; >> @@ -1127,6 +1119,22 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) >> } >> >> if (virtio_gpu_virgl_enabled(g->conf)) { >> + error_setg(&g->migration_blocker, "virgl is not yet migratable"); >> + if (migrate_add_blocker(g->migration_blocker, errp) < 0) { >> + error_free(g->migration_blocker); >> + return; >> + } >> + } >> + >> + g->config_size = sizeof(struct virtio_gpu_config); >> + g->virtio_config.num_scanouts = g->conf.max_outputs; >> + virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU, >> + g->config_size); >> + >> + g->req_state[0].width = 1024; >> + g->req_state[0].height = 768; >> + >> + if (virtio_gpu_virgl_enabled(g->conf)) { >> /* use larger control queue in 3d mode */ >> g->ctrl_vq = virtio_add_queue(vdev, 256, virtio_gpu_handle_ctrl_cb); >> g->cursor_vq = virtio_add_queue(vdev, 16, virtio_gpu_handle_cursor_cb); >> @@ -1152,11 +1160,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) >> dpy_gfx_replace_surface(g->scanout[i].con, NULL); >> } >> } >> - >> - if (virtio_gpu_virgl_enabled(g->conf)) { >> - error_setg(&g->migration_blocker, "virgl is not yet migratable"); >> - migrate_add_blocker(g->migration_blocker); >> - } >> } >> >> static void virtio_gpu_device_unrealize(DeviceState *qdev, Error **errp) >> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c >> index abeaf3d..585cc5b 100644 >> --- a/hw/misc/ivshmem.c >> +++ b/hw/misc/ivshmem.c >> @@ -903,9 +903,6 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp) >> } >> } >> >> - vmstate_register_ram(s->ivshmem_bar2, DEVICE(s)); >> - pci_register_bar(PCI_DEVICE(s), 2, attr, s->ivshmem_bar2); >> - >> if (s->master == ON_OFF_AUTO_AUTO) { >> s->master = s->vm_id == 0 ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF; >> } >> @@ -913,8 +910,14 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp) >> if (!ivshmem_is_master(s)) { >> error_setg(&s->migration_blocker, >> "Migration is disabled when using feature 'peer mode' in device 'ivshmem'"); >> - migrate_add_blocker(s->migration_blocker); >> + if (migrate_add_blocker(s->migration_blocker, errp) < 0) { >> + error_free(s->migration_blocker); >> + return; >> + } >> } >> + >> + vmstate_register_ram(s->ivshmem_bar2, DEVICE(s)); >> + pci_register_bar(PCI_DEVICE(s), 2, attr, s->ivshmem_bar2); >> } >> >> static void ivshmem_exit(PCIDevice *dev) >> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c >> index 5b26946..ff503d0 100644 >> --- a/hw/scsi/vhost-scsi.c >> +++ b/hw/scsi/vhost-scsi.c >> @@ -238,8 +238,14 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) >> vhost_dummy_handle_output); >> if (err != NULL) { >> error_propagate(errp, err); >> - close(vhostfd); >> - return; >> + goto close_fd; >> + } >> + >> + error_setg(&s->migration_blocker, >> + "vhost-scsi does not support migration"); >> + ret = migrate_add_blocker(s->migration_blocker, errp); >> + if (ret < 0) { >> + goto free_blocker; >> } >> >> s->dev.nvqs = VHOST_SCSI_VQ_NUM_FIXED + vs->conf.num_queues; >> @@ -252,7 +258,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) >> if (ret < 0) { >> error_setg(errp, "vhost-scsi: vhost initialization failed: %s", >> strerror(-ret)); >> - return; >> + goto free_vqs; >> } >> >> /* At present, channel and lun both are 0 for bootable vhost-scsi disk */ >> @@ -261,9 +267,16 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) >> /* Note: we can also get the minimum tpgt from kernel */ >> s->target = vs->conf.boot_tpgt; >> >> - error_setg(&s->migration_blocker, >> - "vhost-scsi does not support migration"); >> - migrate_add_blocker(s->migration_blocker); >> + return; >> + >> + free_vqs: >> + migrate_del_blocker(s->migration_blocker); >> + g_free(s->dev.vqs); >> + free_blocker: >> + error_free(s->migration_blocker); >> + close_fd: >> + close(vhostfd); >> + return; >> } >> >> static void vhost_scsi_unrealize(DeviceState *dev, Error **errp) >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >> index f7f7023..416fada 100644 >> --- a/hw/virtio/vhost.c >> +++ b/hw/virtio/vhost.c >> @@ -1157,7 +1157,13 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, >> } >> >> if (hdev->migration_blocker != NULL) { >> - migrate_add_blocker(hdev->migration_blocker); >> + Error *local_err; >> + r = migrate_add_blocker(hdev->migration_blocker, &local_err); >> + if (r < 0) { >> + error_report_err(local_err); >> + error_free(hdev->migration_blocker); >> + goto fail_busyloop; >> + } >> } >> >> hdev->mem = g_malloc0(offsetof(struct vhost_memory, regions)); >> diff --git a/include/migration/migration.h b/include/migration/migration.h >> index c309d23..8a91215 100644 >> --- a/include/migration/migration.h >> +++ b/include/migration/migration.h >> @@ -240,6 +240,7 @@ void remove_migration_state_change_notifier(Notifier *notify); >> MigrationState *migrate_init(const MigrationParams *params); >> bool migration_is_blocked(Error **errp); >> bool migration_in_setup(MigrationState *); >> +bool migration_is_idle(MigrationState *s); >> bool migration_has_finished(MigrationState *); >> bool migration_has_failed(MigrationState *); >> /* True if outgoing migration has entered postcopy phase */ >> @@ -284,8 +285,11 @@ int ram_postcopy_incoming_init(MigrationIncomingState *mis); >> * @migrate_add_blocker - prevent migration from proceeding >> * >> * @reason - an error to be returned whenever migration is attempted >> + * @errp - [out] The reason (if any) we cannot block migration right now. >> + * >> + * @returns - 0 on success, -EBUSY on failure, with errp set. >> */ >> -void migrate_add_blocker(Error *reason); >> +int migrate_add_blocker(Error *reason, Error **errp); >> >> /** >> * @migrate_del_blocker - remove a blocking error from migration >> diff --git a/migration/migration.c b/migration/migration.c >> index f498ab8..713e012 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -1044,6 +1044,31 @@ bool migration_in_postcopy_after_devices(MigrationState *s) >> return migration_in_postcopy(s) && s->postcopy_after_devices; >> } >> >> +bool migration_is_idle(MigrationState *s) >> +{ >> + if (!s) { >> + s = migrate_get_current(); >> + } >> + >> + switch (s->state) { >> + case MIGRATION_STATUS_NONE: >> + case MIGRATION_STATUS_CANCELLED: >> + case MIGRATION_STATUS_COMPLETED: >> + case MIGRATION_STATUS_FAILED: >> + return true; >> + case MIGRATION_STATUS_SETUP: >> + case MIGRATION_STATUS_CANCELLING: >> + case MIGRATION_STATUS_ACTIVE: >> + case MIGRATION_STATUS_POSTCOPY_ACTIVE: >> + case MIGRATION_STATUS_COLO: >> + return false; >> + case MIGRATION_STATUS__MAX: >> + g_assert_not_reached(); >> + } >> + >> + return false; >> +} >> + >> MigrationState *migrate_init(const MigrationParams *params) >> { >> MigrationState *s = migrate_get_current(); >> @@ -1086,9 +1111,15 @@ MigrationState *migrate_init(const MigrationParams *params) >> >> static GSList *migration_blockers; >> >> -void migrate_add_blocker(Error *reason) >> +int migrate_add_blocker(Error *reason, Error **errp) >> { >> - migration_blockers = g_slist_prepend(migration_blockers, reason); >> + if (migration_is_idle(NULL)) { >> + migration_blockers = g_slist_prepend(migration_blockers, reason); >> + return 0; >> + } >> + >> + error_setg(errp, "Cannot block a migration already in-progress"); > > I wonder if you need to add the 'reason' here as well; otherwise you get this > message about blocking a migration but don't know what wanted to block it. > So perhaps set errp from reason and then use error_prepend to add that > extra message? > > Dave > We talked about this on the QEMU IRC, but for the benefit of the open list, the problem with this is that the original error messages are written to be displayed in the event of a new migration behind prohibited. e.g. "Sorry, you can't migrate while using 9PFS!" isn't a good error message to display necessarily when 9pfs initialization fails, because it gives the opposite impression (i.e. "B" preventing "A" when in truth it is "A" preventing "B".) We can make all of the migration blockers "cause agnostic" as you suggested on IRC to say that "Sorry, A and B are incompatible!" which will give some idea as to which component is bumping up against migration. One more detail too is that the error message I chose for a migration "in progress" does not sufficiently explain the error case if "COLO" is being used, so a separate error message for that case may be nice. So there's some work to do to make the error messages nicer. If everyone else could look at the technical bits, I can work on spinning a more polished version. Or, Ashijeet, if you want to take this over at any time just let me know. --js >> + return -EBUSY; >> } >> >> void migrate_del_blocker(Error *reason) >> diff --git a/stubs/migr-blocker.c b/stubs/migr-blocker.c >> index 8ab3604..a5ba18f 100644 >> --- a/stubs/migr-blocker.c >> +++ b/stubs/migr-blocker.c >> @@ -2,8 +2,9 @@ >> #include "qemu-common.h" >> #include "migration/migration.h" >> >> -void migrate_add_blocker(Error *reason) >> +int migrate_add_blocker(Error *reason, Error **errp) >> { >> + return 0; >> } >> >> void migrate_del_blocker(Error *reason) >> diff --git a/target-i386/kvm.c b/target-i386/kvm.c >> index f62264a..8abb54d 100644 >> --- a/target-i386/kvm.c >> +++ b/target-i386/kvm.c >> @@ -961,7 +961,11 @@ int kvm_arch_init_vcpu(CPUState *cs) >> error_setg(&invtsc_mig_blocker, >> "State blocked by non-migratable CPU device" >> " (invtsc flag)"); >> - migrate_add_blocker(invtsc_mig_blocker); >> + r = migrate_add_blocker(invtsc_mig_blocker, NULL); >> + if (r < 0) { >> + fprintf(stderr, "migrate_add_blocker failed\n"); >> + goto efail; >> + } >> /* for savevm */ >> vmstate_x86_cpu.unmigratable = 1; >> } >> @@ -969,12 +973,12 @@ int kvm_arch_init_vcpu(CPUState *cs) >> cpuid_data.cpuid.padding = 0; >> r = kvm_vcpu_ioctl(cs, KVM_SET_CPUID2, &cpuid_data); >> if (r) { >> - return r; >> + goto fail; >> } >> >> r = kvm_arch_set_tsc_khz(cs); >> if (r < 0) { >> - return r; >> + goto fail; >> } >> >> /* vcpu's TSC frequency is either specified by user, or following >> @@ -1001,6 +1005,12 @@ int kvm_arch_init_vcpu(CPUState *cs) >> } >> >> return 0; >> + >> + fail: >> + migrate_del_blocker(invtsc_mig_blocker); >> + efail: >> + error_free(invtsc_mig_blocker); >> + return r; >> } >> >> void kvm_arch_reset_vcpu(X86CPU *cpu) >> -- >> 2.9.3 >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/1] migration: disallow migrate_add_blocker during migration 2016-12-09 17:25 ` [Qemu-devel] [PATCH v3 1/1] " John Snow 2016-12-09 17:39 ` Dr. David Alan Gilbert @ 2016-12-12 10:52 ` Greg Kurz 2016-12-12 11:02 ` Dr. David Alan Gilbert 1 sibling, 1 reply; 8+ messages in thread From: Greg Kurz @ 2016-12-12 10:52 UTC (permalink / raw) To: John Snow Cc: qemu-devel, kwolf, qemu-block, mst, jcody, quintela, armbru, aneesh.kumar, ashijeetacharya, pbonzini, amit.shah, marcandre.lureau, dgilbert On Fri, 9 Dec 2016 12:25:47 -0500 John Snow <jsnow@redhat.com> wrote: > If a migration is already in progress and somebody attempts > to add a migration blocker, this should rightly fail. > > Add an errp parameter and a retcode return value to migrate_add_blocker. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/qcow.c | 6 +++++- > block/vdi.c | 6 +++++- > block/vhdx.c | 24 ++++++++++++++---------- > block/vmdk.c | 7 ++++++- > block/vpc.c | 10 +++++++--- > block/vvfat.c | 20 ++++++++++++-------- > hw/9pfs/9p.c | 16 ++++++++++++---- > hw/display/virtio-gpu.c | 29 ++++++++++++++++------------- > hw/misc/ivshmem.c | 11 +++++++---- > hw/scsi/vhost-scsi.c | 25 +++++++++++++++++++------ > hw/virtio/vhost.c | 8 +++++++- > include/migration/migration.h | 6 +++++- > migration/migration.c | 35 +++++++++++++++++++++++++++++++++-- > stubs/migr-blocker.c | 3 ++- > target-i386/kvm.c | 16 +++++++++++++--- > 15 files changed, 163 insertions(+), 59 deletions(-) > > diff --git a/block/qcow.c b/block/qcow.c > index 7540f43..11526a1 100644 > --- a/block/qcow.c > +++ b/block/qcow.c > @@ -252,7 +252,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, > error_setg(&s->migration_blocker, "The qcow format used by node '%s' " > "does not support live migration", > bdrv_get_device_or_node_name(bs)); > - migrate_add_blocker(s->migration_blocker); > + ret = migrate_add_blocker(s->migration_blocker, errp); > + if (ret < 0) { > + error_free(s->migration_blocker); > + goto fail; > + } > > qemu_co_mutex_init(&s->lock); > return 0; > diff --git a/block/vdi.c b/block/vdi.c > index 96b78d5..2b56f52 100644 > --- a/block/vdi.c > +++ b/block/vdi.c > @@ -471,7 +471,11 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, > error_setg(&s->migration_blocker, "The vdi format used by node '%s' " > "does not support live migration", > bdrv_get_device_or_node_name(bs)); > - migrate_add_blocker(s->migration_blocker); > + ret = migrate_add_blocker(s->migration_blocker, errp); > + if (ret < 0) { > + error_free(s->migration_blocker); > + goto fail_free_bmap; > + } > > qemu_co_mutex_init(&s->write_lock); > > diff --git a/block/vhdx.c b/block/vhdx.c > index 0ba2f0a..d81941b 100644 > --- a/block/vhdx.c > +++ b/block/vhdx.c > @@ -991,20 +991,24 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags, > } > } > > - if (flags & BDRV_O_RDWR) { > - ret = vhdx_update_headers(bs, s, false, NULL); > - if (ret < 0) { > - goto fail; > - } > - } > - > - /* TODO: differencing files */ > - > /* Disable migration when VHDX images are used */ > error_setg(&s->migration_blocker, "The vhdx format used by node '%s' " > "does not support live migration", > bdrv_get_device_or_node_name(bs)); > - migrate_add_blocker(s->migration_blocker); > + ret = migrate_add_blocker(s->migration_blocker, errp); > + if (ret < 0) { > + error_free(s->migration_blocker); > + goto fail; > + } > + > + if (flags & BDRV_O_RDWR) { > + ret = vhdx_update_headers(bs, s, false, NULL); > + if (ret < 0) { > + goto fail; > + } > + } > + > + /* TODO: differencing files */ > > return 0; > fail: > diff --git a/block/vmdk.c b/block/vmdk.c > index a11c27a..4a45a83 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -976,7 +976,12 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags, > error_setg(&s->migration_blocker, "The vmdk format used by node '%s' " > "does not support live migration", > bdrv_get_device_or_node_name(bs)); > - migrate_add_blocker(s->migration_blocker); > + ret = migrate_add_blocker(s->migration_blocker, errp); > + if (ret < 0) { > + error_free(s->migration_blocker); > + goto fail; > + } > + > g_free(buf); > return 0; > > diff --git a/block/vpc.c b/block/vpc.c > index 8d5886f..299a8c8 100644 > --- a/block/vpc.c > +++ b/block/vpc.c > @@ -422,13 +422,17 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > #endif > } > > - qemu_co_mutex_init(&s->lock); > - > /* Disable migration when VHD images are used */ > error_setg(&s->migration_blocker, "The vpc format used by node '%s' " > "does not support live migration", > bdrv_get_device_or_node_name(bs)); > - migrate_add_blocker(s->migration_blocker); > + ret = migrate_add_blocker(s->migration_blocker, errp); > + if (ret < 0) { > + error_free(s->migration_blocker); > + goto fail; > + } > + > + qemu_co_mutex_init(&s->lock); > > return 0; > > diff --git a/block/vvfat.c b/block/vvfat.c > index ded2109..3de3320 100644 > --- a/block/vvfat.c > +++ b/block/vvfat.c > @@ -1185,22 +1185,26 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags, > > s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; > > - if (s->first_sectors_number == 0x40) { > - init_mbr(s, cyls, heads, secs); > - } > - > - // assert(is_consistent(s)); > - qemu_co_mutex_init(&s->lock); > - > /* Disable migration when vvfat is used rw */ > if (s->qcow) { > error_setg(&s->migration_blocker, > "The vvfat (rw) format used by node '%s' " > "does not support live migration", > bdrv_get_device_or_node_name(bs)); > - migrate_add_blocker(s->migration_blocker); > + ret = migrate_add_blocker(s->migration_blocker, errp); > + if (ret < 0) { > + error_free(s->migration_blocker); > + goto fail; > + } > } > > + if (s->first_sectors_number == 0x40) { > + init_mbr(s, cyls, heads, secs); > + } > + > + // assert(is_consistent(s)); > + qemu_co_mutex_init(&s->lock); > + > ret = 0; > fail: > qemu_opts_del(opts); > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index faebd91..e72ef43 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -1013,20 +1013,28 @@ static void coroutine_fn v9fs_attach(void *opaque) > goto out; > } > err += offset; > - memcpy(&s->root_qid, &qid, sizeof(qid)); > - trace_v9fs_attach_return(pdu->tag, pdu->id, > - qid.type, qid.version, qid.path); > + > /* > * disable migration if we haven't done already. > * attach could get called multiple times for the same export. > */ > if (!s->migration_blocker) { > + int ret; > s->root_fid = fid; > error_setg(&s->migration_blocker, > "Migration is disabled when VirtFS export path '%s' is mounted in the guest using mount_tag '%s'", > s->ctx.fs_root ? s->ctx.fs_root : "NULL", s->tag); > - migrate_add_blocker(s->migration_blocker); > + ret = migrate_add_blocker(s->migration_blocker, NULL); > + if (ret < 0) { > + err = ret; > + clunk_fid(s, fid); > + goto out; So this would cause mount(2) in the guest to fail with EBUSY if migration is in progress, right ? I guess well configured guests should be able to cope with temporary failures when mounting a network filesystem and retry later. I don't object but I'd like to have some more background... if you could point me to the on-list discussion you're mentioning in the cover letter, that'd be great. :) Thanks. -- Greg > + } > } > + > + memcpy(&s->root_qid, &qid, sizeof(qid)); > + trace_v9fs_attach_return(pdu->tag, pdu->id, > + qid.type, qid.version, qid.path); > out: > put_fid(pdu, fidp); > out_nofid: > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > index 5f32e1a..6e60b8c 100644 > --- a/hw/display/virtio-gpu.c > +++ b/hw/display/virtio-gpu.c > @@ -1108,14 +1108,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > return; > } > > - g->config_size = sizeof(struct virtio_gpu_config); > - g->virtio_config.num_scanouts = g->conf.max_outputs; > - virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU, > - g->config_size); > - > - g->req_state[0].width = 1024; > - g->req_state[0].height = 768; > - > g->use_virgl_renderer = false; > #if !defined(CONFIG_VIRGL) || defined(HOST_WORDS_BIGENDIAN) > have_virgl = false; > @@ -1127,6 +1119,22 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > } > > if (virtio_gpu_virgl_enabled(g->conf)) { > + error_setg(&g->migration_blocker, "virgl is not yet migratable"); > + if (migrate_add_blocker(g->migration_blocker, errp) < 0) { > + error_free(g->migration_blocker); > + return; > + } > + } > + > + g->config_size = sizeof(struct virtio_gpu_config); > + g->virtio_config.num_scanouts = g->conf.max_outputs; > + virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU, > + g->config_size); > + > + g->req_state[0].width = 1024; > + g->req_state[0].height = 768; > + > + if (virtio_gpu_virgl_enabled(g->conf)) { > /* use larger control queue in 3d mode */ > g->ctrl_vq = virtio_add_queue(vdev, 256, virtio_gpu_handle_ctrl_cb); > g->cursor_vq = virtio_add_queue(vdev, 16, virtio_gpu_handle_cursor_cb); > @@ -1152,11 +1160,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > dpy_gfx_replace_surface(g->scanout[i].con, NULL); > } > } > - > - if (virtio_gpu_virgl_enabled(g->conf)) { > - error_setg(&g->migration_blocker, "virgl is not yet migratable"); > - migrate_add_blocker(g->migration_blocker); > - } > } > > static void virtio_gpu_device_unrealize(DeviceState *qdev, Error **errp) > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > index abeaf3d..585cc5b 100644 > --- a/hw/misc/ivshmem.c > +++ b/hw/misc/ivshmem.c > @@ -903,9 +903,6 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp) > } > } > > - vmstate_register_ram(s->ivshmem_bar2, DEVICE(s)); > - pci_register_bar(PCI_DEVICE(s), 2, attr, s->ivshmem_bar2); > - > if (s->master == ON_OFF_AUTO_AUTO) { > s->master = s->vm_id == 0 ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF; > } > @@ -913,8 +910,14 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp) > if (!ivshmem_is_master(s)) { > error_setg(&s->migration_blocker, > "Migration is disabled when using feature 'peer mode' in device 'ivshmem'"); > - migrate_add_blocker(s->migration_blocker); > + if (migrate_add_blocker(s->migration_blocker, errp) < 0) { > + error_free(s->migration_blocker); > + return; > + } > } > + > + vmstate_register_ram(s->ivshmem_bar2, DEVICE(s)); > + pci_register_bar(PCI_DEVICE(s), 2, attr, s->ivshmem_bar2); > } > > static void ivshmem_exit(PCIDevice *dev) > diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c > index 5b26946..ff503d0 100644 > --- a/hw/scsi/vhost-scsi.c > +++ b/hw/scsi/vhost-scsi.c > @@ -238,8 +238,14 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) > vhost_dummy_handle_output); > if (err != NULL) { > error_propagate(errp, err); > - close(vhostfd); > - return; > + goto close_fd; > + } > + > + error_setg(&s->migration_blocker, > + "vhost-scsi does not support migration"); > + ret = migrate_add_blocker(s->migration_blocker, errp); > + if (ret < 0) { > + goto free_blocker; > } > > s->dev.nvqs = VHOST_SCSI_VQ_NUM_FIXED + vs->conf.num_queues; > @@ -252,7 +258,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) > if (ret < 0) { > error_setg(errp, "vhost-scsi: vhost initialization failed: %s", > strerror(-ret)); > - return; > + goto free_vqs; > } > > /* At present, channel and lun both are 0 for bootable vhost-scsi disk */ > @@ -261,9 +267,16 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) > /* Note: we can also get the minimum tpgt from kernel */ > s->target = vs->conf.boot_tpgt; > > - error_setg(&s->migration_blocker, > - "vhost-scsi does not support migration"); > - migrate_add_blocker(s->migration_blocker); > + return; > + > + free_vqs: > + migrate_del_blocker(s->migration_blocker); > + g_free(s->dev.vqs); > + free_blocker: > + error_free(s->migration_blocker); > + close_fd: > + close(vhostfd); > + return; > } > > static void vhost_scsi_unrealize(DeviceState *dev, Error **errp) > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index f7f7023..416fada 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -1157,7 +1157,13 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, > } > > if (hdev->migration_blocker != NULL) { > - migrate_add_blocker(hdev->migration_blocker); > + Error *local_err; > + r = migrate_add_blocker(hdev->migration_blocker, &local_err); > + if (r < 0) { > + error_report_err(local_err); > + error_free(hdev->migration_blocker); > + goto fail_busyloop; > + } > } > > hdev->mem = g_malloc0(offsetof(struct vhost_memory, regions)); > diff --git a/include/migration/migration.h b/include/migration/migration.h > index c309d23..8a91215 100644 > --- a/include/migration/migration.h > +++ b/include/migration/migration.h > @@ -240,6 +240,7 @@ void remove_migration_state_change_notifier(Notifier *notify); > MigrationState *migrate_init(const MigrationParams *params); > bool migration_is_blocked(Error **errp); > bool migration_in_setup(MigrationState *); > +bool migration_is_idle(MigrationState *s); > bool migration_has_finished(MigrationState *); > bool migration_has_failed(MigrationState *); > /* True if outgoing migration has entered postcopy phase */ > @@ -284,8 +285,11 @@ int ram_postcopy_incoming_init(MigrationIncomingState *mis); > * @migrate_add_blocker - prevent migration from proceeding > * > * @reason - an error to be returned whenever migration is attempted > + * @errp - [out] The reason (if any) we cannot block migration right now. > + * > + * @returns - 0 on success, -EBUSY on failure, with errp set. > */ > -void migrate_add_blocker(Error *reason); > +int migrate_add_blocker(Error *reason, Error **errp); > > /** > * @migrate_del_blocker - remove a blocking error from migration > diff --git a/migration/migration.c b/migration/migration.c > index f498ab8..713e012 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -1044,6 +1044,31 @@ bool migration_in_postcopy_after_devices(MigrationState *s) > return migration_in_postcopy(s) && s->postcopy_after_devices; > } > > +bool migration_is_idle(MigrationState *s) > +{ > + if (!s) { > + s = migrate_get_current(); > + } > + > + switch (s->state) { > + case MIGRATION_STATUS_NONE: > + case MIGRATION_STATUS_CANCELLED: > + case MIGRATION_STATUS_COMPLETED: > + case MIGRATION_STATUS_FAILED: > + return true; > + case MIGRATION_STATUS_SETUP: > + case MIGRATION_STATUS_CANCELLING: > + case MIGRATION_STATUS_ACTIVE: > + case MIGRATION_STATUS_POSTCOPY_ACTIVE: > + case MIGRATION_STATUS_COLO: > + return false; > + case MIGRATION_STATUS__MAX: > + g_assert_not_reached(); > + } > + > + return false; > +} > + > MigrationState *migrate_init(const MigrationParams *params) > { > MigrationState *s = migrate_get_current(); > @@ -1086,9 +1111,15 @@ MigrationState *migrate_init(const MigrationParams *params) > > static GSList *migration_blockers; > > -void migrate_add_blocker(Error *reason) > +int migrate_add_blocker(Error *reason, Error **errp) > { > - migration_blockers = g_slist_prepend(migration_blockers, reason); > + if (migration_is_idle(NULL)) { > + migration_blockers = g_slist_prepend(migration_blockers, reason); > + return 0; > + } > + > + error_setg(errp, "Cannot block a migration already in-progress"); > + return -EBUSY; > } > > void migrate_del_blocker(Error *reason) > diff --git a/stubs/migr-blocker.c b/stubs/migr-blocker.c > index 8ab3604..a5ba18f 100644 > --- a/stubs/migr-blocker.c > +++ b/stubs/migr-blocker.c > @@ -2,8 +2,9 @@ > #include "qemu-common.h" > #include "migration/migration.h" > > -void migrate_add_blocker(Error *reason) > +int migrate_add_blocker(Error *reason, Error **errp) > { > + return 0; > } > > void migrate_del_blocker(Error *reason) > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > index f62264a..8abb54d 100644 > --- a/target-i386/kvm.c > +++ b/target-i386/kvm.c > @@ -961,7 +961,11 @@ int kvm_arch_init_vcpu(CPUState *cs) > error_setg(&invtsc_mig_blocker, > "State blocked by non-migratable CPU device" > " (invtsc flag)"); > - migrate_add_blocker(invtsc_mig_blocker); > + r = migrate_add_blocker(invtsc_mig_blocker, NULL); > + if (r < 0) { > + fprintf(stderr, "migrate_add_blocker failed\n"); > + goto efail; > + } > /* for savevm */ > vmstate_x86_cpu.unmigratable = 1; > } > @@ -969,12 +973,12 @@ int kvm_arch_init_vcpu(CPUState *cs) > cpuid_data.cpuid.padding = 0; > r = kvm_vcpu_ioctl(cs, KVM_SET_CPUID2, &cpuid_data); > if (r) { > - return r; > + goto fail; > } > > r = kvm_arch_set_tsc_khz(cs); > if (r < 0) { > - return r; > + goto fail; > } > > /* vcpu's TSC frequency is either specified by user, or following > @@ -1001,6 +1005,12 @@ int kvm_arch_init_vcpu(CPUState *cs) > } > > return 0; > + > + fail: > + migrate_del_blocker(invtsc_mig_blocker); > + efail: > + error_free(invtsc_mig_blocker); > + return r; > } > > void kvm_arch_reset_vcpu(X86CPU *cpu) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/1] migration: disallow migrate_add_blocker during migration 2016-12-12 10:52 ` Greg Kurz @ 2016-12-12 11:02 ` Dr. David Alan Gilbert 2016-12-12 14:56 ` Greg Kurz 0 siblings, 1 reply; 8+ messages in thread From: Dr. David Alan Gilbert @ 2016-12-12 11:02 UTC (permalink / raw) To: Greg Kurz Cc: John Snow, qemu-devel, kwolf, qemu-block, mst, jcody, quintela, armbru, aneesh.kumar, ashijeetacharya, pbonzini, amit.shah, marcandre.lureau * Greg Kurz (groug@kaod.org) wrote: > On Fri, 9 Dec 2016 12:25:47 -0500 > John Snow <jsnow@redhat.com> wrote: > > > If a migration is already in progress and somebody attempts > > to add a migration blocker, this should rightly fail. > > > > Add an errp parameter and a retcode return value to migrate_add_blocker. > > > > Signed-off-by: John Snow <jsnow@redhat.com> > > --- > > block/qcow.c | 6 +++++- > > block/vdi.c | 6 +++++- > > block/vhdx.c | 24 ++++++++++++++---------- > > block/vmdk.c | 7 ++++++- > > block/vpc.c | 10 +++++++--- > > block/vvfat.c | 20 ++++++++++++-------- > > hw/9pfs/9p.c | 16 ++++++++++++---- > > hw/display/virtio-gpu.c | 29 ++++++++++++++++------------- > > hw/misc/ivshmem.c | 11 +++++++---- > > hw/scsi/vhost-scsi.c | 25 +++++++++++++++++++------ > > hw/virtio/vhost.c | 8 +++++++- > > include/migration/migration.h | 6 +++++- > > migration/migration.c | 35 +++++++++++++++++++++++++++++++++-- > > stubs/migr-blocker.c | 3 ++- > > target-i386/kvm.c | 16 +++++++++++++--- > > 15 files changed, 163 insertions(+), 59 deletions(-) > > > > diff --git a/block/qcow.c b/block/qcow.c > > index 7540f43..11526a1 100644 > > --- a/block/qcow.c > > +++ b/block/qcow.c > > @@ -252,7 +252,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, > > error_setg(&s->migration_blocker, "The qcow format used by node '%s' " > > "does not support live migration", > > bdrv_get_device_or_node_name(bs)); > > - migrate_add_blocker(s->migration_blocker); > > + ret = migrate_add_blocker(s->migration_blocker, errp); > > + if (ret < 0) { > > + error_free(s->migration_blocker); > > + goto fail; > > + } > > > > qemu_co_mutex_init(&s->lock); > > return 0; > > diff --git a/block/vdi.c b/block/vdi.c > > index 96b78d5..2b56f52 100644 > > --- a/block/vdi.c > > +++ b/block/vdi.c > > @@ -471,7 +471,11 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, > > error_setg(&s->migration_blocker, "The vdi format used by node '%s' " > > "does not support live migration", > > bdrv_get_device_or_node_name(bs)); > > - migrate_add_blocker(s->migration_blocker); > > + ret = migrate_add_blocker(s->migration_blocker, errp); > > + if (ret < 0) { > > + error_free(s->migration_blocker); > > + goto fail_free_bmap; > > + } > > > > qemu_co_mutex_init(&s->write_lock); > > > > diff --git a/block/vhdx.c b/block/vhdx.c > > index 0ba2f0a..d81941b 100644 > > --- a/block/vhdx.c > > +++ b/block/vhdx.c > > @@ -991,20 +991,24 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags, > > } > > } > > > > - if (flags & BDRV_O_RDWR) { > > - ret = vhdx_update_headers(bs, s, false, NULL); > > - if (ret < 0) { > > - goto fail; > > - } > > - } > > - > > - /* TODO: differencing files */ > > - > > /* Disable migration when VHDX images are used */ > > error_setg(&s->migration_blocker, "The vhdx format used by node '%s' " > > "does not support live migration", > > bdrv_get_device_or_node_name(bs)); > > - migrate_add_blocker(s->migration_blocker); > > + ret = migrate_add_blocker(s->migration_blocker, errp); > > + if (ret < 0) { > > + error_free(s->migration_blocker); > > + goto fail; > > + } > > + > > + if (flags & BDRV_O_RDWR) { > > + ret = vhdx_update_headers(bs, s, false, NULL); > > + if (ret < 0) { > > + goto fail; > > + } > > + } > > + > > + /* TODO: differencing files */ > > > > return 0; > > fail: > > diff --git a/block/vmdk.c b/block/vmdk.c > > index a11c27a..4a45a83 100644 > > --- a/block/vmdk.c > > +++ b/block/vmdk.c > > @@ -976,7 +976,12 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags, > > error_setg(&s->migration_blocker, "The vmdk format used by node '%s' " > > "does not support live migration", > > bdrv_get_device_or_node_name(bs)); > > - migrate_add_blocker(s->migration_blocker); > > + ret = migrate_add_blocker(s->migration_blocker, errp); > > + if (ret < 0) { > > + error_free(s->migration_blocker); > > + goto fail; > > + } > > + > > g_free(buf); > > return 0; > > > > diff --git a/block/vpc.c b/block/vpc.c > > index 8d5886f..299a8c8 100644 > > --- a/block/vpc.c > > +++ b/block/vpc.c > > @@ -422,13 +422,17 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > > #endif > > } > > > > - qemu_co_mutex_init(&s->lock); > > - > > /* Disable migration when VHD images are used */ > > error_setg(&s->migration_blocker, "The vpc format used by node '%s' " > > "does not support live migration", > > bdrv_get_device_or_node_name(bs)); > > - migrate_add_blocker(s->migration_blocker); > > + ret = migrate_add_blocker(s->migration_blocker, errp); > > + if (ret < 0) { > > + error_free(s->migration_blocker); > > + goto fail; > > + } > > + > > + qemu_co_mutex_init(&s->lock); > > > > return 0; > > > > diff --git a/block/vvfat.c b/block/vvfat.c > > index ded2109..3de3320 100644 > > --- a/block/vvfat.c > > +++ b/block/vvfat.c > > @@ -1185,22 +1185,26 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags, > > > > s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; > > > > - if (s->first_sectors_number == 0x40) { > > - init_mbr(s, cyls, heads, secs); > > - } > > - > > - // assert(is_consistent(s)); > > - qemu_co_mutex_init(&s->lock); > > - > > /* Disable migration when vvfat is used rw */ > > if (s->qcow) { > > error_setg(&s->migration_blocker, > > "The vvfat (rw) format used by node '%s' " > > "does not support live migration", > > bdrv_get_device_or_node_name(bs)); > > - migrate_add_blocker(s->migration_blocker); > > + ret = migrate_add_blocker(s->migration_blocker, errp); > > + if (ret < 0) { > > + error_free(s->migration_blocker); > > + goto fail; > > + } > > } > > > > + if (s->first_sectors_number == 0x40) { > > + init_mbr(s, cyls, heads, secs); > > + } > > + > > + // assert(is_consistent(s)); > > + qemu_co_mutex_init(&s->lock); > > + > > ret = 0; > > fail: > > qemu_opts_del(opts); > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > index faebd91..e72ef43 100644 > > --- a/hw/9pfs/9p.c > > +++ b/hw/9pfs/9p.c > > @@ -1013,20 +1013,28 @@ static void coroutine_fn v9fs_attach(void *opaque) > > goto out; > > } > > err += offset; > > - memcpy(&s->root_qid, &qid, sizeof(qid)); > > - trace_v9fs_attach_return(pdu->tag, pdu->id, > > - qid.type, qid.version, qid.path); > > + > > /* > > * disable migration if we haven't done already. > > * attach could get called multiple times for the same export. > > */ > > if (!s->migration_blocker) { > > + int ret; > > s->root_fid = fid; > > error_setg(&s->migration_blocker, > > "Migration is disabled when VirtFS export path '%s' is mounted in the guest using mount_tag '%s'", > > s->ctx.fs_root ? s->ctx.fs_root : "NULL", s->tag); > > - migrate_add_blocker(s->migration_blocker); > > + ret = migrate_add_blocker(s->migration_blocker, NULL); > > + if (ret < 0) { > > + err = ret; > > + clunk_fid(s, fid); > > + goto out; > > So this would cause mount(2) in the guest to fail with EBUSY if > migration is in progress, right ? I guess well configured guests > should be able to cope with temporary failures when mounting > a network filesystem and retry later. > > I don't object but I'd like to have some more background... if > you could point me to the on-list discussion you're mentioning > in the cover letter, that'd be great. :) See the discussion on: https://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg01185.html note, it might be best to special case 9pfs here; it's unusual in that it's unusable in that it always does the migration blocker when in use; everywhere else has the flag in the VMStateDescription or only blocks in specific cases. Dave > Thanks. > > -- > Greg > > > + } > > } > > + > > + memcpy(&s->root_qid, &qid, sizeof(qid)); > > + trace_v9fs_attach_return(pdu->tag, pdu->id, > > + qid.type, qid.version, qid.path); > > out: > > put_fid(pdu, fidp); > > out_nofid: > > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > > index 5f32e1a..6e60b8c 100644 > > --- a/hw/display/virtio-gpu.c > > +++ b/hw/display/virtio-gpu.c > > @@ -1108,14 +1108,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > > return; > > } > > > > - g->config_size = sizeof(struct virtio_gpu_config); > > - g->virtio_config.num_scanouts = g->conf.max_outputs; > > - virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU, > > - g->config_size); > > - > > - g->req_state[0].width = 1024; > > - g->req_state[0].height = 768; > > - > > g->use_virgl_renderer = false; > > #if !defined(CONFIG_VIRGL) || defined(HOST_WORDS_BIGENDIAN) > > have_virgl = false; > > @@ -1127,6 +1119,22 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > > } > > > > if (virtio_gpu_virgl_enabled(g->conf)) { > > + error_setg(&g->migration_blocker, "virgl is not yet migratable"); > > + if (migrate_add_blocker(g->migration_blocker, errp) < 0) { > > + error_free(g->migration_blocker); > > + return; > > + } > > + } > > + > > + g->config_size = sizeof(struct virtio_gpu_config); > > + g->virtio_config.num_scanouts = g->conf.max_outputs; > > + virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU, > > + g->config_size); > > + > > + g->req_state[0].width = 1024; > > + g->req_state[0].height = 768; > > + > > + if (virtio_gpu_virgl_enabled(g->conf)) { > > /* use larger control queue in 3d mode */ > > g->ctrl_vq = virtio_add_queue(vdev, 256, virtio_gpu_handle_ctrl_cb); > > g->cursor_vq = virtio_add_queue(vdev, 16, virtio_gpu_handle_cursor_cb); > > @@ -1152,11 +1160,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > > dpy_gfx_replace_surface(g->scanout[i].con, NULL); > > } > > } > > - > > - if (virtio_gpu_virgl_enabled(g->conf)) { > > - error_setg(&g->migration_blocker, "virgl is not yet migratable"); > > - migrate_add_blocker(g->migration_blocker); > > - } > > } > > > > static void virtio_gpu_device_unrealize(DeviceState *qdev, Error **errp) > > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > > index abeaf3d..585cc5b 100644 > > --- a/hw/misc/ivshmem.c > > +++ b/hw/misc/ivshmem.c > > @@ -903,9 +903,6 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp) > > } > > } > > > > - vmstate_register_ram(s->ivshmem_bar2, DEVICE(s)); > > - pci_register_bar(PCI_DEVICE(s), 2, attr, s->ivshmem_bar2); > > - > > if (s->master == ON_OFF_AUTO_AUTO) { > > s->master = s->vm_id == 0 ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF; > > } > > @@ -913,8 +910,14 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp) > > if (!ivshmem_is_master(s)) { > > error_setg(&s->migration_blocker, > > "Migration is disabled when using feature 'peer mode' in device 'ivshmem'"); > > - migrate_add_blocker(s->migration_blocker); > > + if (migrate_add_blocker(s->migration_blocker, errp) < 0) { > > + error_free(s->migration_blocker); > > + return; > > + } > > } > > + > > + vmstate_register_ram(s->ivshmem_bar2, DEVICE(s)); > > + pci_register_bar(PCI_DEVICE(s), 2, attr, s->ivshmem_bar2); > > } > > > > static void ivshmem_exit(PCIDevice *dev) > > diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c > > index 5b26946..ff503d0 100644 > > --- a/hw/scsi/vhost-scsi.c > > +++ b/hw/scsi/vhost-scsi.c > > @@ -238,8 +238,14 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) > > vhost_dummy_handle_output); > > if (err != NULL) { > > error_propagate(errp, err); > > - close(vhostfd); > > - return; > > + goto close_fd; > > + } > > + > > + error_setg(&s->migration_blocker, > > + "vhost-scsi does not support migration"); > > + ret = migrate_add_blocker(s->migration_blocker, errp); > > + if (ret < 0) { > > + goto free_blocker; > > } > > > > s->dev.nvqs = VHOST_SCSI_VQ_NUM_FIXED + vs->conf.num_queues; > > @@ -252,7 +258,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) > > if (ret < 0) { > > error_setg(errp, "vhost-scsi: vhost initialization failed: %s", > > strerror(-ret)); > > - return; > > + goto free_vqs; > > } > > > > /* At present, channel and lun both are 0 for bootable vhost-scsi disk */ > > @@ -261,9 +267,16 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) > > /* Note: we can also get the minimum tpgt from kernel */ > > s->target = vs->conf.boot_tpgt; > > > > - error_setg(&s->migration_blocker, > > - "vhost-scsi does not support migration"); > > - migrate_add_blocker(s->migration_blocker); > > + return; > > + > > + free_vqs: > > + migrate_del_blocker(s->migration_blocker); > > + g_free(s->dev.vqs); > > + free_blocker: > > + error_free(s->migration_blocker); > > + close_fd: > > + close(vhostfd); > > + return; > > } > > > > static void vhost_scsi_unrealize(DeviceState *dev, Error **errp) > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > index f7f7023..416fada 100644 > > --- a/hw/virtio/vhost.c > > +++ b/hw/virtio/vhost.c > > @@ -1157,7 +1157,13 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, > > } > > > > if (hdev->migration_blocker != NULL) { > > - migrate_add_blocker(hdev->migration_blocker); > > + Error *local_err; > > + r = migrate_add_blocker(hdev->migration_blocker, &local_err); > > + if (r < 0) { > > + error_report_err(local_err); > > + error_free(hdev->migration_blocker); > > + goto fail_busyloop; > > + } > > } > > > > hdev->mem = g_malloc0(offsetof(struct vhost_memory, regions)); > > diff --git a/include/migration/migration.h b/include/migration/migration.h > > index c309d23..8a91215 100644 > > --- a/include/migration/migration.h > > +++ b/include/migration/migration.h > > @@ -240,6 +240,7 @@ void remove_migration_state_change_notifier(Notifier *notify); > > MigrationState *migrate_init(const MigrationParams *params); > > bool migration_is_blocked(Error **errp); > > bool migration_in_setup(MigrationState *); > > +bool migration_is_idle(MigrationState *s); > > bool migration_has_finished(MigrationState *); > > bool migration_has_failed(MigrationState *); > > /* True if outgoing migration has entered postcopy phase */ > > @@ -284,8 +285,11 @@ int ram_postcopy_incoming_init(MigrationIncomingState *mis); > > * @migrate_add_blocker - prevent migration from proceeding > > * > > * @reason - an error to be returned whenever migration is attempted > > + * @errp - [out] The reason (if any) we cannot block migration right now. > > + * > > + * @returns - 0 on success, -EBUSY on failure, with errp set. > > */ > > -void migrate_add_blocker(Error *reason); > > +int migrate_add_blocker(Error *reason, Error **errp); > > > > /** > > * @migrate_del_blocker - remove a blocking error from migration > > diff --git a/migration/migration.c b/migration/migration.c > > index f498ab8..713e012 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -1044,6 +1044,31 @@ bool migration_in_postcopy_after_devices(MigrationState *s) > > return migration_in_postcopy(s) && s->postcopy_after_devices; > > } > > > > +bool migration_is_idle(MigrationState *s) > > +{ > > + if (!s) { > > + s = migrate_get_current(); > > + } > > + > > + switch (s->state) { > > + case MIGRATION_STATUS_NONE: > > + case MIGRATION_STATUS_CANCELLED: > > + case MIGRATION_STATUS_COMPLETED: > > + case MIGRATION_STATUS_FAILED: > > + return true; > > + case MIGRATION_STATUS_SETUP: > > + case MIGRATION_STATUS_CANCELLING: > > + case MIGRATION_STATUS_ACTIVE: > > + case MIGRATION_STATUS_POSTCOPY_ACTIVE: > > + case MIGRATION_STATUS_COLO: > > + return false; > > + case MIGRATION_STATUS__MAX: > > + g_assert_not_reached(); > > + } > > + > > + return false; > > +} > > + > > MigrationState *migrate_init(const MigrationParams *params) > > { > > MigrationState *s = migrate_get_current(); > > @@ -1086,9 +1111,15 @@ MigrationState *migrate_init(const MigrationParams *params) > > > > static GSList *migration_blockers; > > > > -void migrate_add_blocker(Error *reason) > > +int migrate_add_blocker(Error *reason, Error **errp) > > { > > - migration_blockers = g_slist_prepend(migration_blockers, reason); > > + if (migration_is_idle(NULL)) { > > + migration_blockers = g_slist_prepend(migration_blockers, reason); > > + return 0; > > + } > > + > > + error_setg(errp, "Cannot block a migration already in-progress"); > > + return -EBUSY; > > } > > > > void migrate_del_blocker(Error *reason) > > diff --git a/stubs/migr-blocker.c b/stubs/migr-blocker.c > > index 8ab3604..a5ba18f 100644 > > --- a/stubs/migr-blocker.c > > +++ b/stubs/migr-blocker.c > > @@ -2,8 +2,9 @@ > > #include "qemu-common.h" > > #include "migration/migration.h" > > > > -void migrate_add_blocker(Error *reason) > > +int migrate_add_blocker(Error *reason, Error **errp) > > { > > + return 0; > > } > > > > void migrate_del_blocker(Error *reason) > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > > index f62264a..8abb54d 100644 > > --- a/target-i386/kvm.c > > +++ b/target-i386/kvm.c > > @@ -961,7 +961,11 @@ int kvm_arch_init_vcpu(CPUState *cs) > > error_setg(&invtsc_mig_blocker, > > "State blocked by non-migratable CPU device" > > " (invtsc flag)"); > > - migrate_add_blocker(invtsc_mig_blocker); > > + r = migrate_add_blocker(invtsc_mig_blocker, NULL); > > + if (r < 0) { > > + fprintf(stderr, "migrate_add_blocker failed\n"); > > + goto efail; > > + } > > /* for savevm */ > > vmstate_x86_cpu.unmigratable = 1; > > } > > @@ -969,12 +973,12 @@ int kvm_arch_init_vcpu(CPUState *cs) > > cpuid_data.cpuid.padding = 0; > > r = kvm_vcpu_ioctl(cs, KVM_SET_CPUID2, &cpuid_data); > > if (r) { > > - return r; > > + goto fail; > > } > > > > r = kvm_arch_set_tsc_khz(cs); > > if (r < 0) { > > - return r; > > + goto fail; > > } > > > > /* vcpu's TSC frequency is either specified by user, or following > > @@ -1001,6 +1005,12 @@ int kvm_arch_init_vcpu(CPUState *cs) > > } > > > > return 0; > > + > > + fail: > > + migrate_del_blocker(invtsc_mig_blocker); > > + efail: > > + error_free(invtsc_mig_blocker); > > + return r; > > } > > > > void kvm_arch_reset_vcpu(X86CPU *cpu) > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/1] migration: disallow migrate_add_blocker during migration 2016-12-12 11:02 ` Dr. David Alan Gilbert @ 2016-12-12 14:56 ` Greg Kurz 0 siblings, 0 replies; 8+ messages in thread From: Greg Kurz @ 2016-12-12 14:56 UTC (permalink / raw) To: Dr. David Alan Gilbert Cc: John Snow, qemu-devel, kwolf, qemu-block, mst, jcody, quintela, armbru, aneesh.kumar, ashijeetacharya, pbonzini, amit.shah, marcandre.lureau On Mon, 12 Dec 2016 11:02:54 +0000 "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Greg Kurz (groug@kaod.org) wrote: > > On Fri, 9 Dec 2016 12:25:47 -0500 > > John Snow <jsnow@redhat.com> wrote: > > > > > If a migration is already in progress and somebody attempts > > > to add a migration blocker, this should rightly fail. > > > > > > Add an errp parameter and a retcode return value to migrate_add_blocker. > > > > > > Signed-off-by: John Snow <jsnow@redhat.com> > > > --- > > > block/qcow.c | 6 +++++- > > > block/vdi.c | 6 +++++- > > > block/vhdx.c | 24 ++++++++++++++---------- > > > block/vmdk.c | 7 ++++++- > > > block/vpc.c | 10 +++++++--- > > > block/vvfat.c | 20 ++++++++++++-------- > > > hw/9pfs/9p.c | 16 ++++++++++++---- > > > hw/display/virtio-gpu.c | 29 ++++++++++++++++------------- > > > hw/misc/ivshmem.c | 11 +++++++---- > > > hw/scsi/vhost-scsi.c | 25 +++++++++++++++++++------ > > > hw/virtio/vhost.c | 8 +++++++- > > > include/migration/migration.h | 6 +++++- > > > migration/migration.c | 35 +++++++++++++++++++++++++++++++++-- > > > stubs/migr-blocker.c | 3 ++- > > > target-i386/kvm.c | 16 +++++++++++++--- > > > 15 files changed, 163 insertions(+), 59 deletions(-) > > > > > > diff --git a/block/qcow.c b/block/qcow.c > > > index 7540f43..11526a1 100644 > > > --- a/block/qcow.c > > > +++ b/block/qcow.c > > > @@ -252,7 +252,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, > > > error_setg(&s->migration_blocker, "The qcow format used by node '%s' " > > > "does not support live migration", > > > bdrv_get_device_or_node_name(bs)); > > > - migrate_add_blocker(s->migration_blocker); > > > + ret = migrate_add_blocker(s->migration_blocker, errp); > > > + if (ret < 0) { > > > + error_free(s->migration_blocker); > > > + goto fail; > > > + } > > > > > > qemu_co_mutex_init(&s->lock); > > > return 0; > > > diff --git a/block/vdi.c b/block/vdi.c > > > index 96b78d5..2b56f52 100644 > > > --- a/block/vdi.c > > > +++ b/block/vdi.c > > > @@ -471,7 +471,11 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, > > > error_setg(&s->migration_blocker, "The vdi format used by node '%s' " > > > "does not support live migration", > > > bdrv_get_device_or_node_name(bs)); > > > - migrate_add_blocker(s->migration_blocker); > > > + ret = migrate_add_blocker(s->migration_blocker, errp); > > > + if (ret < 0) { > > > + error_free(s->migration_blocker); > > > + goto fail_free_bmap; > > > + } > > > > > > qemu_co_mutex_init(&s->write_lock); > > > > > > diff --git a/block/vhdx.c b/block/vhdx.c > > > index 0ba2f0a..d81941b 100644 > > > --- a/block/vhdx.c > > > +++ b/block/vhdx.c > > > @@ -991,20 +991,24 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags, > > > } > > > } > > > > > > - if (flags & BDRV_O_RDWR) { > > > - ret = vhdx_update_headers(bs, s, false, NULL); > > > - if (ret < 0) { > > > - goto fail; > > > - } > > > - } > > > - > > > - /* TODO: differencing files */ > > > - > > > /* Disable migration when VHDX images are used */ > > > error_setg(&s->migration_blocker, "The vhdx format used by node '%s' " > > > "does not support live migration", > > > bdrv_get_device_or_node_name(bs)); > > > - migrate_add_blocker(s->migration_blocker); > > > + ret = migrate_add_blocker(s->migration_blocker, errp); > > > + if (ret < 0) { > > > + error_free(s->migration_blocker); > > > + goto fail; > > > + } > > > + > > > + if (flags & BDRV_O_RDWR) { > > > + ret = vhdx_update_headers(bs, s, false, NULL); > > > + if (ret < 0) { > > > + goto fail; > > > + } > > > + } > > > + > > > + /* TODO: differencing files */ > > > > > > return 0; > > > fail: > > > diff --git a/block/vmdk.c b/block/vmdk.c > > > index a11c27a..4a45a83 100644 > > > --- a/block/vmdk.c > > > +++ b/block/vmdk.c > > > @@ -976,7 +976,12 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags, > > > error_setg(&s->migration_blocker, "The vmdk format used by node '%s' " > > > "does not support live migration", > > > bdrv_get_device_or_node_name(bs)); > > > - migrate_add_blocker(s->migration_blocker); > > > + ret = migrate_add_blocker(s->migration_blocker, errp); > > > + if (ret < 0) { > > > + error_free(s->migration_blocker); > > > + goto fail; > > > + } > > > + > > > g_free(buf); > > > return 0; > > > > > > diff --git a/block/vpc.c b/block/vpc.c > > > index 8d5886f..299a8c8 100644 > > > --- a/block/vpc.c > > > +++ b/block/vpc.c > > > @@ -422,13 +422,17 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > > > #endif > > > } > > > > > > - qemu_co_mutex_init(&s->lock); > > > - > > > /* Disable migration when VHD images are used */ > > > error_setg(&s->migration_blocker, "The vpc format used by node '%s' " > > > "does not support live migration", > > > bdrv_get_device_or_node_name(bs)); > > > - migrate_add_blocker(s->migration_blocker); > > > + ret = migrate_add_blocker(s->migration_blocker, errp); > > > + if (ret < 0) { > > > + error_free(s->migration_blocker); > > > + goto fail; > > > + } > > > + > > > + qemu_co_mutex_init(&s->lock); > > > > > > return 0; > > > > > > diff --git a/block/vvfat.c b/block/vvfat.c > > > index ded2109..3de3320 100644 > > > --- a/block/vvfat.c > > > +++ b/block/vvfat.c > > > @@ -1185,22 +1185,26 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags, > > > > > > s->sector_count = s->faked_sectors + s->sectors_per_cluster*s->cluster_count; > > > > > > - if (s->first_sectors_number == 0x40) { > > > - init_mbr(s, cyls, heads, secs); > > > - } > > > - > > > - // assert(is_consistent(s)); > > > - qemu_co_mutex_init(&s->lock); > > > - > > > /* Disable migration when vvfat is used rw */ > > > if (s->qcow) { > > > error_setg(&s->migration_blocker, > > > "The vvfat (rw) format used by node '%s' " > > > "does not support live migration", > > > bdrv_get_device_or_node_name(bs)); > > > - migrate_add_blocker(s->migration_blocker); > > > + ret = migrate_add_blocker(s->migration_blocker, errp); > > > + if (ret < 0) { > > > + error_free(s->migration_blocker); > > > + goto fail; > > > + } > > > } > > > > > > + if (s->first_sectors_number == 0x40) { > > > + init_mbr(s, cyls, heads, secs); > > > + } > > > + > > > + // assert(is_consistent(s)); > > > + qemu_co_mutex_init(&s->lock); > > > + > > > ret = 0; > > > fail: > > > qemu_opts_del(opts); > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > > index faebd91..e72ef43 100644 > > > --- a/hw/9pfs/9p.c > > > +++ b/hw/9pfs/9p.c > > > @@ -1013,20 +1013,28 @@ static void coroutine_fn v9fs_attach(void *opaque) > > > goto out; > > > } > > > err += offset; > > > - memcpy(&s->root_qid, &qid, sizeof(qid)); > > > - trace_v9fs_attach_return(pdu->tag, pdu->id, > > > - qid.type, qid.version, qid.path); > > > + > > > /* > > > * disable migration if we haven't done already. > > > * attach could get called multiple times for the same export. > > > */ > > > if (!s->migration_blocker) { > > > + int ret; > > > s->root_fid = fid; > > > error_setg(&s->migration_blocker, > > > "Migration is disabled when VirtFS export path '%s' is mounted in the guest using mount_tag '%s'", > > > s->ctx.fs_root ? s->ctx.fs_root : "NULL", s->tag); > > > - migrate_add_blocker(s->migration_blocker); > > > + ret = migrate_add_blocker(s->migration_blocker, NULL); > > > + if (ret < 0) { > > > + err = ret; > > > + clunk_fid(s, fid); > > > + goto out; > > > > So this would cause mount(2) in the guest to fail with EBUSY if > > migration is in progress, right ? I guess well configured guests > > should be able to cope with temporary failures when mounting > > a network filesystem and retry later. > > > > I don't object but I'd like to have some more background... if > > you could point me to the on-list discussion you're mentioning > > in the cover letter, that'd be great. :) > > See the discussion on: > https://lists.nongnu.org/archive/html/qemu-devel/2016-12/msg01185.html > > note, it might be best to special case 9pfs here; it's unusual > in that it's unusable in that it always does the migration blocker > when in use; everywhere else has the flag in the VMStateDescription > or only blocks in specific cases. > <disclaimer> The succession of 'in that' in the first sentence makes it hard to understand, at least for me... so maybe I don't get it right and what is written below is irrelevant. </disclaimer> virtio-9p-device is only unmigratable when the client has opened a connexion (attach operation) and the associated fid (the root fid) is still referenced. i.e. between mount and umount. This is indeed quite different from always-unmigratable devices, with the VMStateDescription.unmigratable field set (a cooperative guest could unmount the 9p share before migration and remount it afterwards, for example). The migration_add_blocker() call sites covered by this patch seem to be either: - at device realization time - when opening a backend which explicitely doesn't support migration (qcow, vdi...) i.e. when the user starts a migration and then asks to do something that would cause migration to fail... which is also quite different from the 9pfs case where migration_add_blocker() is triggered by the guest. So yes, I agree that 9pfs is a special case... but what would happen if we allow the guest to mount the 9p share if we have a migration in progress ? I' rather have a proper failure than a hang after migration... Makes sense ? > Dave > > > Thanks. > > > > -- > > Greg > > > > > + } > > > } > > > + > > > + memcpy(&s->root_qid, &qid, sizeof(qid)); > > > + trace_v9fs_attach_return(pdu->tag, pdu->id, > > > + qid.type, qid.version, qid.path); > > > out: > > > put_fid(pdu, fidp); > > > out_nofid: > > > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c > > > index 5f32e1a..6e60b8c 100644 > > > --- a/hw/display/virtio-gpu.c > > > +++ b/hw/display/virtio-gpu.c > > > @@ -1108,14 +1108,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > > > return; > > > } > > > > > > - g->config_size = sizeof(struct virtio_gpu_config); > > > - g->virtio_config.num_scanouts = g->conf.max_outputs; > > > - virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU, > > > - g->config_size); > > > - > > > - g->req_state[0].width = 1024; > > > - g->req_state[0].height = 768; > > > - > > > g->use_virgl_renderer = false; > > > #if !defined(CONFIG_VIRGL) || defined(HOST_WORDS_BIGENDIAN) > > > have_virgl = false; > > > @@ -1127,6 +1119,22 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > > > } > > > > > > if (virtio_gpu_virgl_enabled(g->conf)) { > > > + error_setg(&g->migration_blocker, "virgl is not yet migratable"); > > > + if (migrate_add_blocker(g->migration_blocker, errp) < 0) { > > > + error_free(g->migration_blocker); > > > + return; > > > + } > > > + } > > > + > > > + g->config_size = sizeof(struct virtio_gpu_config); > > > + g->virtio_config.num_scanouts = g->conf.max_outputs; > > > + virtio_init(VIRTIO_DEVICE(g), "virtio-gpu", VIRTIO_ID_GPU, > > > + g->config_size); > > > + > > > + g->req_state[0].width = 1024; > > > + g->req_state[0].height = 768; > > > + > > > + if (virtio_gpu_virgl_enabled(g->conf)) { > > > /* use larger control queue in 3d mode */ > > > g->ctrl_vq = virtio_add_queue(vdev, 256, virtio_gpu_handle_ctrl_cb); > > > g->cursor_vq = virtio_add_queue(vdev, 16, virtio_gpu_handle_cursor_cb); > > > @@ -1152,11 +1160,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp) > > > dpy_gfx_replace_surface(g->scanout[i].con, NULL); > > > } > > > } > > > - > > > - if (virtio_gpu_virgl_enabled(g->conf)) { > > > - error_setg(&g->migration_blocker, "virgl is not yet migratable"); > > > - migrate_add_blocker(g->migration_blocker); > > > - } > > > } > > > > > > static void virtio_gpu_device_unrealize(DeviceState *qdev, Error **errp) > > > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > > > index abeaf3d..585cc5b 100644 > > > --- a/hw/misc/ivshmem.c > > > +++ b/hw/misc/ivshmem.c > > > @@ -903,9 +903,6 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp) > > > } > > > } > > > > > > - vmstate_register_ram(s->ivshmem_bar2, DEVICE(s)); > > > - pci_register_bar(PCI_DEVICE(s), 2, attr, s->ivshmem_bar2); > > > - > > > if (s->master == ON_OFF_AUTO_AUTO) { > > > s->master = s->vm_id == 0 ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF; > > > } > > > @@ -913,8 +910,14 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp) > > > if (!ivshmem_is_master(s)) { > > > error_setg(&s->migration_blocker, > > > "Migration is disabled when using feature 'peer mode' in device 'ivshmem'"); > > > - migrate_add_blocker(s->migration_blocker); > > > + if (migrate_add_blocker(s->migration_blocker, errp) < 0) { > > > + error_free(s->migration_blocker); > > > + return; > > > + } > > > } > > > + > > > + vmstate_register_ram(s->ivshmem_bar2, DEVICE(s)); > > > + pci_register_bar(PCI_DEVICE(s), 2, attr, s->ivshmem_bar2); > > > } > > > > > > static void ivshmem_exit(PCIDevice *dev) > > > diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c > > > index 5b26946..ff503d0 100644 > > > --- a/hw/scsi/vhost-scsi.c > > > +++ b/hw/scsi/vhost-scsi.c > > > @@ -238,8 +238,14 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) > > > vhost_dummy_handle_output); > > > if (err != NULL) { > > > error_propagate(errp, err); > > > - close(vhostfd); > > > - return; > > > + goto close_fd; > > > + } > > > + > > > + error_setg(&s->migration_blocker, > > > + "vhost-scsi does not support migration"); > > > + ret = migrate_add_blocker(s->migration_blocker, errp); > > > + if (ret < 0) { > > > + goto free_blocker; > > > } > > > > > > s->dev.nvqs = VHOST_SCSI_VQ_NUM_FIXED + vs->conf.num_queues; > > > @@ -252,7 +258,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) > > > if (ret < 0) { > > > error_setg(errp, "vhost-scsi: vhost initialization failed: %s", > > > strerror(-ret)); > > > - return; > > > + goto free_vqs; > > > } > > > > > > /* At present, channel and lun both are 0 for bootable vhost-scsi disk */ > > > @@ -261,9 +267,16 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) > > > /* Note: we can also get the minimum tpgt from kernel */ > > > s->target = vs->conf.boot_tpgt; > > > > > > - error_setg(&s->migration_blocker, > > > - "vhost-scsi does not support migration"); > > > - migrate_add_blocker(s->migration_blocker); > > > + return; > > > + > > > + free_vqs: > > > + migrate_del_blocker(s->migration_blocker); > > > + g_free(s->dev.vqs); > > > + free_blocker: > > > + error_free(s->migration_blocker); > > > + close_fd: > > > + close(vhostfd); > > > + return; > > > } > > > > > > static void vhost_scsi_unrealize(DeviceState *dev, Error **errp) > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > > index f7f7023..416fada 100644 > > > --- a/hw/virtio/vhost.c > > > +++ b/hw/virtio/vhost.c > > > @@ -1157,7 +1157,13 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, > > > } > > > > > > if (hdev->migration_blocker != NULL) { > > > - migrate_add_blocker(hdev->migration_blocker); > > > + Error *local_err; > > > + r = migrate_add_blocker(hdev->migration_blocker, &local_err); > > > + if (r < 0) { > > > + error_report_err(local_err); > > > + error_free(hdev->migration_blocker); > > > + goto fail_busyloop; > > > + } > > > } > > > > > > hdev->mem = g_malloc0(offsetof(struct vhost_memory, regions)); > > > diff --git a/include/migration/migration.h b/include/migration/migration.h > > > index c309d23..8a91215 100644 > > > --- a/include/migration/migration.h > > > +++ b/include/migration/migration.h > > > @@ -240,6 +240,7 @@ void remove_migration_state_change_notifier(Notifier *notify); > > > MigrationState *migrate_init(const MigrationParams *params); > > > bool migration_is_blocked(Error **errp); > > > bool migration_in_setup(MigrationState *); > > > +bool migration_is_idle(MigrationState *s); > > > bool migration_has_finished(MigrationState *); > > > bool migration_has_failed(MigrationState *); > > > /* True if outgoing migration has entered postcopy phase */ > > > @@ -284,8 +285,11 @@ int ram_postcopy_incoming_init(MigrationIncomingState *mis); > > > * @migrate_add_blocker - prevent migration from proceeding > > > * > > > * @reason - an error to be returned whenever migration is attempted > > > + * @errp - [out] The reason (if any) we cannot block migration right now. > > > + * > > > + * @returns - 0 on success, -EBUSY on failure, with errp set. > > > */ > > > -void migrate_add_blocker(Error *reason); > > > +int migrate_add_blocker(Error *reason, Error **errp); > > > > > > /** > > > * @migrate_del_blocker - remove a blocking error from migration > > > diff --git a/migration/migration.c b/migration/migration.c > > > index f498ab8..713e012 100644 > > > --- a/migration/migration.c > > > +++ b/migration/migration.c > > > @@ -1044,6 +1044,31 @@ bool migration_in_postcopy_after_devices(MigrationState *s) > > > return migration_in_postcopy(s) && s->postcopy_after_devices; > > > } > > > > > > +bool migration_is_idle(MigrationState *s) > > > +{ > > > + if (!s) { > > > + s = migrate_get_current(); > > > + } > > > + > > > + switch (s->state) { > > > + case MIGRATION_STATUS_NONE: > > > + case MIGRATION_STATUS_CANCELLED: > > > + case MIGRATION_STATUS_COMPLETED: > > > + case MIGRATION_STATUS_FAILED: > > > + return true; > > > + case MIGRATION_STATUS_SETUP: > > > + case MIGRATION_STATUS_CANCELLING: > > > + case MIGRATION_STATUS_ACTIVE: > > > + case MIGRATION_STATUS_POSTCOPY_ACTIVE: > > > + case MIGRATION_STATUS_COLO: > > > + return false; > > > + case MIGRATION_STATUS__MAX: > > > + g_assert_not_reached(); > > > + } > > > + > > > + return false; > > > +} > > > + > > > MigrationState *migrate_init(const MigrationParams *params) > > > { > > > MigrationState *s = migrate_get_current(); > > > @@ -1086,9 +1111,15 @@ MigrationState *migrate_init(const MigrationParams *params) > > > > > > static GSList *migration_blockers; > > > > > > -void migrate_add_blocker(Error *reason) > > > +int migrate_add_blocker(Error *reason, Error **errp) > > > { > > > - migration_blockers = g_slist_prepend(migration_blockers, reason); > > > + if (migration_is_idle(NULL)) { > > > + migration_blockers = g_slist_prepend(migration_blockers, reason); > > > + return 0; > > > + } > > > + > > > + error_setg(errp, "Cannot block a migration already in-progress"); > > > + return -EBUSY; > > > } > > > > > > void migrate_del_blocker(Error *reason) > > > diff --git a/stubs/migr-blocker.c b/stubs/migr-blocker.c > > > index 8ab3604..a5ba18f 100644 > > > --- a/stubs/migr-blocker.c > > > +++ b/stubs/migr-blocker.c > > > @@ -2,8 +2,9 @@ > > > #include "qemu-common.h" > > > #include "migration/migration.h" > > > > > > -void migrate_add_blocker(Error *reason) > > > +int migrate_add_blocker(Error *reason, Error **errp) > > > { > > > + return 0; > > > } > > > > > > void migrate_del_blocker(Error *reason) > > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > > > index f62264a..8abb54d 100644 > > > --- a/target-i386/kvm.c > > > +++ b/target-i386/kvm.c > > > @@ -961,7 +961,11 @@ int kvm_arch_init_vcpu(CPUState *cs) > > > error_setg(&invtsc_mig_blocker, > > > "State blocked by non-migratable CPU device" > > > " (invtsc flag)"); > > > - migrate_add_blocker(invtsc_mig_blocker); > > > + r = migrate_add_blocker(invtsc_mig_blocker, NULL); > > > + if (r < 0) { > > > + fprintf(stderr, "migrate_add_blocker failed\n"); > > > + goto efail; > > > + } > > > /* for savevm */ > > > vmstate_x86_cpu.unmigratable = 1; > > > } > > > @@ -969,12 +973,12 @@ int kvm_arch_init_vcpu(CPUState *cs) > > > cpuid_data.cpuid.padding = 0; > > > r = kvm_vcpu_ioctl(cs, KVM_SET_CPUID2, &cpuid_data); > > > if (r) { > > > - return r; > > > + goto fail; > > > } > > > > > > r = kvm_arch_set_tsc_khz(cs); > > > if (r < 0) { > > > - return r; > > > + goto fail; > > > } > > > > > > /* vcpu's TSC frequency is either specified by user, or following > > > @@ -1001,6 +1005,12 @@ int kvm_arch_init_vcpu(CPUState *cs) > > > } > > > > > > return 0; > > > + > > > + fail: > > > + migrate_del_blocker(invtsc_mig_blocker); > > > + efail: > > > + error_free(invtsc_mig_blocker); > > > + return r; > > > } > > > > > > void kvm_arch_reset_vcpu(X86CPU *cpu) > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/1] migration: disallow migrate_add_blocker during migration 2016-12-09 17:25 [Qemu-devel] [PATCH v3 0/1] migration: disallow migrate_add_blocker during migration John Snow 2016-12-09 17:25 ` [Qemu-devel] [PATCH v3 1/1] " John Snow @ 2016-12-09 17:50 ` no-reply 1 sibling, 0 replies; 8+ messages in thread From: no-reply @ 2016-12-09 17:50 UTC (permalink / raw) To: jsnow Cc: famz, qemu-devel, kwolf, qemu-block, quintela, jcody, mst, armbru, groug, aneesh.kumar, ashijeetacharya, marcandre.lureau, amit.shah, pbonzini, dgilbert Hi, Your series seems to have some coding style problems. See output below for more information: Subject: [Qemu-devel] [PATCH v3 0/1] migration: disallow migrate_add_blocker during migration Type: series Message-id: 20161209172547.31550-1-jsnow@redhat.com === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 # Useful git options git config --local diff.renamelimit 0 git config --local diff.renames True commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' bebd3d0 migration: disallow migrate_add_blocker during migration === OUTPUT BEGIN === Checking PATCH 1/1: migration: disallow migrate_add_blocker during migration... ERROR: do not use C99 // comments #158: FILE: block/vvfat.c:1205: + // assert(is_consistent(s)); total: 1 errors, 0 warnings, 394 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@freelists.org ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-12-12 14:57 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-12-09 17:25 [Qemu-devel] [PATCH v3 0/1] migration: disallow migrate_add_blocker during migration John Snow 2016-12-09 17:25 ` [Qemu-devel] [PATCH v3 1/1] " John Snow 2016-12-09 17:39 ` Dr. David Alan Gilbert 2016-12-09 21:10 ` John Snow 2016-12-12 10:52 ` Greg Kurz 2016-12-12 11:02 ` Dr. David Alan Gilbert 2016-12-12 14:56 ` Greg Kurz 2016-12-09 17:50 ` [Qemu-devel] [PATCH v3 0/1] " no-reply
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).