On Mon, Apr 09, 2012 at 21:52:17 -0600, Eric Blake wrote:
> For now, disk migration via block copy job is not implemented. But
> when we do implement it, we have to deal with the fact that qemu does
> not provide an easy way to re-start a qemu process with mirroring
> still intact (it _might_ be possible by using qemu -S then an
> initial 'drive-mirror' with disk reuse before starting the domain,
> but that gets hairy). Even something like 'virDomainSave' becomes
> hairy, if you realize the implications that 'virDomainRestore' would
> be stuck with recreating the same mirror layout.
>
> But if we step back and look at the bigger picture, we realize that
> the initial client of live storage migration via disk mirroring is
> oVirt, which always uses transient domains, and that if a transient
> domain is destroyed while a mirror exists, oVirt can easily restart
> the storage migration by creating a new domain that visits just the
> source storage, with no loss in data.
>
> We can make life a lot easier by being cowards, and forbidding
> certain operations on a domain. This patch guarantees that we
> never get in a state where we would have to restart a domain with
> a mirroring block copy, by preventing saves, snapshots, hot
> unplug of a disk in use, and conversion to a persistent domain.
>
> * src/conf/domain_conf.h (virDomainHasDiskMirror): New prototype.
> * src/conf/domain_conf.c (virDomainHasDiskMirror): New function.
> * src/libvirt_private.syms (domain_conf.h): Export it.
> * src/qemu/qemu_driver.c (qemuDomainSaveInternal)
> (qemuDomainSnapshotCreateXML, qemuDomainRevertToSnapshot)
> (qemuDomainBlockJobImpl, qemudDomainDefine): Prevent dangerous
> actions while block copy is already in action.
> * src/qemu/qemu_hotplug.c (qemuDomainDetachDiskDevice): Likewise.
> ---
> src/conf/domain_conf.c | 12 ++++++++++++
> src/conf/domain_conf.h | 1 +
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_driver.c | 31 ++++++++++++++++++++++++++++++-
> src/qemu/qemu_hotplug.c | 7 +++++++
> 5 files changed, 51 insertions(+), 1 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 8899653..3aa6861 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -7183,6 +7183,18 @@ virDomainDiskRemoveByName(virDomainDefPtr def, const char *name)
> return virDomainDiskRemove(def, i);
> }
>
> +/* Return true if VM has at least one disk involved in a current block
> + * copy job (that is, with a <mirror> element in the disk xml). */
> +bool
> +virDomainHasDiskMirror(virDomainObjPtr vm)
> +{
> + int i;
> + for (i = 0; i < vm->def->ndisks; i++)
> + if (vm->def->disks[i]->mirror)
> + return true;
> + return false;
> +}
> +
> int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net)
> {
> if (VIR_REALLOC_N(def->nets, def->nnets + 1) < 0)
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index abc953d..77c501c 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1954,6 +1954,7 @@ virDomainDiskDefPtr
> virDomainDiskRemove(virDomainDefPtr def, size_t i);
> virDomainDiskDefPtr
> virDomainDiskRemoveByName(virDomainDefPtr def, const char *name);
> +bool virDomainHasDiskMirror(virDomainObjPtr vm);
>
> int virDomainNetIndexByMac(virDomainDefPtr def, const unsigned char *mac);
> int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index a90f8a0..570940d 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -354,6 +354,7 @@ virDomainGraphicsSpiceZlibCompressionTypeFromString;
> virDomainGraphicsSpiceZlibCompressionTypeToString;
> virDomainGraphicsTypeFromString;
> virDomainGraphicsTypeToString;
> +virDomainHasDiskMirror;
> virDomainHostdevDefAlloc;
> virDomainHostdevDefClear;
> virDomainHostdevDefFree;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 425d340..b0937eb 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -2558,6 +2558,11 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom,
> "%s", _("domain is marked for auto destroy"));
> goto cleanup;
> }
> + if (virDomainHasDiskMirror(vm)) {
> + qemuReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s",
> + _("domain has active block copy job"));
> + goto cleanup;
> + }
>
> memset(&header, 0, sizeof(header));
> memcpy(header.magic, QEMUD_SAVE_PARTIAL, sizeof(header.magic));
> @@ -4947,6 +4952,12 @@ static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) {
> goto cleanup;
> }
> def = NULL;
> + if (virDomainHasDiskMirror(vm)) {
> + qemuReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s",
> + _("domain has active block copy job"));
> + virDomainObjAssignDef(vm, NULL, false);
> + goto cleanup;
> + }
> vm->persistent = 1;
I think it would be better to do this check a bit earlier in the process to
avoid the need to undo virDomainObjAssignDef().
> if (virDomainSaveConfig(driver->configDir,
> @@ -10262,6 +10273,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
> "%s", _("domain is marked for auto destroy"));
> goto cleanup;
> }
> + if (virDomainHasDiskMirror(vm)) {
> + qemuReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s",
> + _("domain has active block copy job"));
> + goto cleanup;
> + }
> +
> if (!vm->persistent && (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) {
> qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> _("cannot halt after transient domain snapshot"));
> @@ -10869,6 +10886,11 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> _("no domain with matching uuid '%s'"), uuidstr);
> goto cleanup;
> }
> + if (virDomainHasDiskMirror(vm)) {
> + qemuReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s",
> + _("domain has active block copy job"));
> + goto cleanup;
> + }
>
> snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name);
> if (!snap) {
> @@ -11607,6 +11629,7 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base,
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> char *device = NULL;
> int ret = -1;
> + int idx;
>
> qemuDriverLock(driver);
> virUUIDFormat(dom->uuid, uuidstr);
> @@ -11617,7 +11640,7 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base,
> goto cleanup;
> }
>
> - device = qemuDiskPathToAlias(vm, path, NULL);
> + device = qemuDiskPathToAlias(vm, path, &idx);
> if (!device) {
> goto cleanup;
> }
> @@ -11633,6 +11656,12 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base,
> "this QEMU binary"));
> goto cleanup;
> }
> + if (mode == BLOCK_JOB_PULL && vm->def->disks[idx]->mirror) {
> + qemuReportError(VIR_ERR_BLOCK_COPY_ACTIVE,
> + _("disk '%s' already in active block copy job"),
> + vm->def->disks[idx]->dst);
> + goto cleanup;
> + }
>
> if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0)
> goto cleanup;
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 857b980..98fa8f8 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1721,6 +1721,13 @@ int qemuDomainDetachDiskDevice(struct qemud_driver *driver,
>
> detach = vm->def->disks[i];
>
> + if (detach->mirror) {
> + qemuReportError(VIR_ERR_BLOCK_COPY_ACTIVE,
> + _("disk '%s' is in an active block copy job"),
> + detach->dst);
> + goto cleanup;
> + }
> +
> if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) {
> if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) != 0) {
> qemuReportError(VIR_ERR_INTERNAL_ERROR,
OK
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list