Re: [Qemu-block] [PATCH v2] migration: disallow migrate_add_blocker during migration

[Dr. David Alan Gilbert]

* John Snow (address@hidden) 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 <address@hidden>

For the migration code only:
Reviewed-by: Dr. David Alan Gilbert <address@hidden>

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 / address@hidden / Manchester, UK