From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: kwolf@redhat.com, qemu-block@nongnu.org, quintela@redhat.com,
jcody@redhat.com, qemu-devel@nongnu.org, amit.shah@redhat.com
Subject: Re: [Qemu-devel] [PATCH v2] migration: disallow migrate_add_blocker during migration
Date: Fri, 9 Oct 2015 18:55:23 +0100 [thread overview]
Message-ID: <20151009175523.GJ2702@work-vm> (raw)
In-Reply-To: <1443632849-10940-1-git-send-email-jsnow@redhat.com>
* 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.
>
> This is part one of two for a solution to prohibit e.g. block jobs
> from running concurrently with migration.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
For the migration code only:
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Dave
> ---
> block/qcow.c | 6 +++++-
> block/vdi.c | 6 +++++-
> block/vhdx.c | 6 +++++-
> block/vmdk.c | 7 ++++++-
> block/vpc.c | 10 +++++++---
> block/vvfat.c | 20 ++++++++++++--------
> hw/9pfs/virtio-9p.c | 16 ++++++++++++----
> hw/misc/ivshmem.c | 5 ++++-
> hw/scsi/vhost-scsi.c | 25 +++++++++++++++++++------
> hw/virtio/vhost.c | 35 +++++++++++++++++++++++------------
> include/migration/migration.h | 6 +++++-
> migration/migration.c | 32 ++++++++++++++++++++++++++++----
> stubs/migr-blocker.c | 3 ++-
> target-i386/kvm.c | 16 +++++++++++++---
> 14 files changed, 146 insertions(+), 47 deletions(-)
>
> diff --git a/block/qcow.c b/block/qcow.c
> index 6e35db1..fbb498f 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -236,7 +236,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 062a654..1635ab1 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -505,7 +505,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 d3bb1bd..097c707 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1005,7 +1005,11 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
> 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;
> + }
>
> return 0;
> fail:
> diff --git a/block/vmdk.c b/block/vmdk.c
> index be0d640..36ff6f4 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -951,7 +951,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 2b3b518..f3dd677 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -325,13 +325,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 7ddc962..bdc1297 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1191,22 +1191,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/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index f972731..67dc626 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -978,20 +978,28 @@ static void v9fs_attach(void *opaque)
> clunk_fid(s, fid);
> goto out;
> }
> - err += offset;
> - 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;
> + }
> }
> +
> + err += offset;
> + 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/misc/ivshmem.c b/hw/misc/ivshmem.c
> index cc76989..859e844 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -740,7 +740,10 @@ static int pci_ivshmem_init(PCIDevice *dev)
> if (s->role_val == IVSHMEM_PEER) {
> 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, NULL) < 0) {
> + error_report("Unable to prohibit migration during ivshmem init");
> + exit(1);
> + }
> }
>
> pci_conf = dev->config;
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index fb7983d..46ab197 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -239,8 +239,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;
> @@ -253,7 +259,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 */
> @@ -262,9 +268,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 c0ed5b2..0de7a01 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -926,13 +926,25 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> goto fail;
> }
>
> - for (i = 0; i < hdev->nvqs; ++i) {
> - r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i);
> - if (r < 0) {
> - goto fail_vq;
> - }
> - }
> hdev->features = features;
> + hdev->migration_blocker = NULL;
> +
> + if (!(hdev->features & (0x1ULL << VHOST_F_LOG_ALL))) {
> + error_setg(&hdev->migration_blocker,
> + "Migration disabled: vhost lacks VHOST_F_LOG_ALL feature.");
> + r = migrate_add_blocker(hdev->migration_blocker, NULL);
> + if (r < 0) {
> + errno = -r;
> + goto fail_mig;
> + }
> + }
> +
> + for (i = 0; i < hdev->nvqs; ++i) {
> + r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i);
> + if (r < 0) {
> + goto fail_vq;
> + }
> + }
>
> hdev->memory_listener = (MemoryListener) {
> .begin = vhost_begin,
> @@ -949,12 +961,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> .eventfd_del = vhost_eventfd_del,
> .priority = 10
> };
> - hdev->migration_blocker = NULL;
> - if (!(hdev->features & (0x1ULL << VHOST_F_LOG_ALL))) {
> - error_setg(&hdev->migration_blocker,
> - "Migration disabled: vhost lacks VHOST_F_LOG_ALL feature.");
> - migrate_add_blocker(hdev->migration_blocker);
> - }
> +
> hdev->mem = g_malloc0(offsetof(struct vhost_memory, regions));
> hdev->n_mem_sections = 0;
> hdev->mem_sections = NULL;
> @@ -965,10 +972,14 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> hdev->memory_changed = false;
> memory_listener_register(&hdev->memory_listener, &address_space_memory);
> return 0;
> +
> fail_vq:
> while (--i >= 0) {
> vhost_virtqueue_cleanup(hdev->vqs + i);
> }
> + migrate_del_blocker(hdev->migration_blocker);
> +fail_mig:
> + error_free(hdev->migration_blocker);
> fail:
> r = -errno;
> hdev->vhost_ops->vhost_backend_cleanup(hdev);
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 8334621..fc18956 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -117,6 +117,7 @@ int migrate_fd_close(MigrationState *s);
> void add_migration_state_change_notifier(Notifier *notify);
> void remove_migration_state_change_notifier(Notifier *notify);
> bool migration_in_setup(MigrationState *);
> +bool migration_has_started(MigrationState *s);
> bool migration_has_finished(MigrationState *);
> bool migration_has_failed(MigrationState *);
> MigrationState *migrate_get_current(void);
> @@ -150,8 +151,11 @@ void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
> * @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 e48dd13..897ae40 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -632,6 +632,26 @@ bool migration_has_failed(MigrationState *s)
> s->state == MIGRATION_STATUS_FAILED);
> }
>
> +bool migration_has_started(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 false;
> + case MIGRATION_STATUS_SETUP:
> + case MIGRATION_STATUS_CANCELLING:
> + case MIGRATION_STATUS_ACTIVE:
> + default:
> + return true;
> + }
> +}
> +
> static MigrationState *migrate_init(const MigrationParams *params)
> {
> MigrationState *s = migrate_get_current();
> @@ -667,9 +687,15 @@ static MigrationState *migrate_init(const MigrationParams *params)
>
> static GSList *migration_blockers;
>
> -void migrate_add_blocker(Error *reason)
> +int migrate_add_blocker(Error *reason, Error **errp)
> {
> + if (migration_has_started(NULL)) {
> + error_setg(errp, "Cannot block a migration already in-progress");
> + return -EBUSY;
> + }
> +
> migration_blockers = g_slist_prepend(migration_blockers, reason);
> + return 0;
> }
>
> void migrate_del_blocker(Error *reason)
> @@ -712,9 +738,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> params.blk = has_blk && blk;
> params.shared = has_inc && inc;
>
> - if (s->state == MIGRATION_STATUS_ACTIVE ||
> - s->state == MIGRATION_STATUS_SETUP ||
> - s->state == MIGRATION_STATUS_CANCELLING) {
> + if (migration_has_started(s)) {
> error_setg(errp, QERR_MIGRATION_ACTIVE);
> return;
> }
> diff --git a/stubs/migr-blocker.c b/stubs/migr-blocker.c
> index 300df6e..06812bd 100644
> --- a/stubs/migr-blocker.c
> +++ b/stubs/migr-blocker.c
> @@ -1,8 +1,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 7b0ba17..f45baa5 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -731,7 +731,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 fail_mig;
> + }
> /* for savevm */
> vmstate_x86_cpu.unmigratable = 1;
> }
> @@ -739,7 +743,7 @@ 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_ioctl;
> }
>
> r = kvm_check_extension(cs->kvm_state, KVM_CAP_TSC_CONTROL);
> @@ -747,7 +751,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
> r = kvm_vcpu_ioctl(cs, KVM_SET_TSC_KHZ, env->tsc_khz);
> if (r < 0) {
> fprintf(stderr, "KVM_SET_TSC_KHZ failed\n");
> - return r;
> + goto fail_ioctl;
> }
> }
>
> @@ -760,6 +764,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
> }
>
> return 0;
> +
> + fail_ioctl:
> + migrate_del_blocker(invtsc_mig_blocker);
> + fail_mig:
> + error_free(invtsc_mig_blocker);
> + return r;
> }
>
> void kvm_arch_reset_vcpu(X86CPU *cpu)
> --
> 2.4.3
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
next prev parent reply other threads:[~2015-10-09 17:55 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-30 17:07 [Qemu-devel] [PATCH v2] migration: disallow migrate_add_blocker during migration John Snow
2015-10-09 17:55 ` Dr. David Alan Gilbert [this message]
2015-10-09 19:42 ` Eric Blake
2015-10-09 19:53 ` John Snow
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20151009175523.GJ2702@work-vm \
--to=dgilbert@redhat.com \
--cc=amit.shah@redhat.com \
--cc=jcody@redhat.com \
--cc=jsnow@redhat.com \
--cc=kwolf@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=quintela@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.