[libvirt] [PATCH 0/4] qemu: always use blockdev non-shared storage migration with blockdev (blockdev-add saga)
Once we enable blockdev there's no need to stick to drive mirror for non-shared-storage migration. This patchset applies on top of the external snapshot branch: https://www.redhat.com/archives/libvir-list/2019-September/msg00082.html Peter Krempa (4): qemu: migration: Refactor cleanup in qemuMigrationSrcNBDStorageCopyBlockdev qemu: migration: Refactor cleanup in qemuMigrationSrcNBDStorageCopyDriveMirror qemu: migration: Refactor cleanup in qemuMigrationSrcNBDStorageCopy qemu: migration: Switch to blockdev mode for non-shared storage migration src/qemu/qemu_migration.c | 69 ++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 40 deletions(-) -- 2.21.0
Remove the cleanup label as it's empty. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index e387deb497..1a557851bb 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -796,50 +796,49 @@ qemuMigrationSrcNBDStorageCopyBlockdev(virQEMUDriverPtr driver, VIR_AUTOPTR(qemuBlockStorageSourceAttachData) data = NULL; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); int mon_ret = 0; - int ret = -1; VIR_AUTOUNREF(virStorageSourcePtr) copysrc = NULL; VIR_DEBUG("starting blockdev mirror for disk=%s to host=%s", diskAlias, host); if (!(copysrc = virStorageSourceNew())) - goto cleanup; + return -1; copysrc->type = VIR_STORAGE_TYPE_NETWORK; copysrc->protocol = VIR_STORAGE_NET_PROTOCOL_NBD; copysrc->format = VIR_STORAGE_FILE_RAW; if (!(copysrc->backingStore = virStorageSourceNew())) - goto cleanup; + return -1; if (VIR_STRDUP(copysrc->path, diskAlias) < 0) - goto cleanup; + return -1; if (VIR_ALLOC_N(copysrc->hosts, 1) < 0) - goto cleanup; + return -1; copysrc->nhosts = 1; copysrc->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; copysrc->hosts->port = port; if (VIR_STRDUP(copysrc->hosts->name, host) < 0) - goto cleanup; + return -1; if (VIR_STRDUP(copysrc->tlsAlias, tlsAlias) < 0) - goto cleanup; + return -1; if (virAsprintf(©src->nodestorage, "migration-%s-storage", disk->dst) < 0 || virAsprintf(©src->nodeformat, "migration-%s-format", disk->dst) < 0) - goto cleanup; + return -1; /* Migration via blockdev-mirror was supported sooner than the auto-read-only * feature was added to qemu */ if (!(data = qemuBlockStorageSourceAttachPrepareBlockdev(copysrc, copysrc->backingStore, false))) - goto cleanup; + return -1; if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) - goto cleanup; + return -1; mon_ret = qemuBlockStorageSourceAttachApply(qemuDomainGetMonitor(vm), data); @@ -852,14 +851,11 @@ qemuMigrationSrcNBDStorageCopyBlockdev(virQEMUDriverPtr driver, qemuBlockStorageSourceAttachRollback(qemuDomainGetMonitor(vm), data); if (qemuDomainObjExitMonitor(driver, vm) < 0 || mon_ret < 0) - goto cleanup; + return -1; VIR_STEAL_PTR(diskPriv->migrSource, copysrc); - ret = 0; - - cleanup: - return ret; + return 0; } -- 2.21.0
Use VIR_AUTOFREE and remove the cleanup label. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 1a557851bb..82625b2261 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -868,36 +868,31 @@ qemuMigrationSrcNBDStorageCopyDriveMirror(virQEMUDriverPtr driver, unsigned long long mirror_speed, bool mirror_shallow) { - char *nbd_dest = NULL; + VIR_AUTOFREE(char *) nbd_dest = NULL; int mon_ret; - int ret = -1; if (strchr(host, ':')) { if (virAsprintf(&nbd_dest, "nbd:[%s]:%d:exportname=%s", host, port, diskAlias) < 0) - goto cleanup; + return -1; } else { if (virAsprintf(&nbd_dest, "nbd:%s:%d:exportname=%s", host, port, diskAlias) < 0) - goto cleanup; + return -1; } if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) - goto cleanup; + return -1; mon_ret = qemuMonitorDriveMirror(qemuDomainGetMonitor(vm), diskAlias, nbd_dest, "raw", mirror_speed, 0, 0, mirror_shallow, true); if (qemuDomainObjExitMonitor(driver, vm) < 0 || mon_ret < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - VIR_FREE(nbd_dest); - return ret; + return 0; } -- 2.21.0
On 9/4/19 10:39 AM, Peter Krempa wrote:
Use VIR_AUTOFREE and remove the cleanup label.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 1a557851bb..82625b2261 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -868,36 +868,31 @@ qemuMigrationSrcNBDStorageCopyDriveMirror(virQEMUDriverPtr driver, unsigned long long mirror_speed, bool mirror_shallow) { - char *nbd_dest = NULL; + VIR_AUTOFREE(char *) nbd_dest = NULL; int mon_ret; - int ret = -1;
if (strchr(host, ':')) { if (virAsprintf(&nbd_dest, "nbd:[%s]:%d:exportname=%s", host, port, diskAlias) < 0)
Should we prefer "nbd://[%s]:%d/%s" here, now that the NBD URI spec is available (and since qemu supports that for quite some time now)? https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md
- goto cleanup; + return -1; } else { if (virAsprintf(&nbd_dest, "nbd:%s:%d:exportname=%s",
and similar here? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Wed, Sep 04, 2019 at 12:01:27 -0500, Eric Blake wrote:
On 9/4/19 10:39 AM, Peter Krempa wrote:
Use VIR_AUTOFREE and remove the cleanup label.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 1a557851bb..82625b2261 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -868,36 +868,31 @@ qemuMigrationSrcNBDStorageCopyDriveMirror(virQEMUDriverPtr driver, unsigned long long mirror_speed, bool mirror_shallow) { - char *nbd_dest = NULL; + VIR_AUTOFREE(char *) nbd_dest = NULL; int mon_ret; - int ret = -1;
if (strchr(host, ':')) { if (virAsprintf(&nbd_dest, "nbd:[%s]:%d:exportname=%s", host, port, diskAlias) < 0)
Should we prefer "nbd://[%s]:%d/%s" here, now that the NBD URI spec is available (and since qemu supports that for quite some time now)? https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md
That commit is literally 2 days old. Also this code is executed on all supported qemus (starting from 1.5.3) so you'd need to be more specific on the 'quite some time now'. Additionally this patchset actually stops calling this code path when blockdev is used so I'm not going to invest into investigating whether the URI format is really supported on ancient qemus if it's not going to be used in the future anyways.
Use VIR_AUTOUNREF and remove the cleanup label. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 82625b2261..c13b21a0bb 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -983,13 +983,12 @@ qemuMigrationSrcNBDStorageCopy(virQEMUDriverPtr driver, unsigned int flags) { qemuDomainObjPrivatePtr priv = vm->privateData; - int ret = -1; int port; size_t i; unsigned long long mirror_speed = speed; bool mirror_shallow = *migrate_flags & QEMU_MONITOR_MIGRATE_NON_SHARED_INC; int rv; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); VIR_DEBUG("Starting drive mirrors for domain %s", vm->def->name); @@ -997,7 +996,7 @@ qemuMigrationSrcNBDStorageCopy(virQEMUDriverPtr driver, virReportError(VIR_ERR_OVERFLOW, _("bandwidth must be less than %llu"), LLONG_MAX >> 20); - goto cleanup; + return -1; } mirror_speed <<= 20; @@ -1015,34 +1014,34 @@ qemuMigrationSrcNBDStorageCopy(virQEMUDriverPtr driver, if (qemuMigrationSrcNBDStorageCopyOne(driver, vm, disk, host, port, mirror_speed, mirror_shallow, tlsAlias, flags) < 0) - goto cleanup; + return -1; if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) { VIR_WARN("Failed to save status on vm %s", vm->def->name); - goto cleanup; + return -1; } } while ((rv = qemuMigrationSrcNBDStorageCopyReady(vm, QEMU_ASYNC_JOB_MIGRATION_OUT)) != 1) { if (rv < 0) - goto cleanup; + return -1; if (priv->job.abortJob) { priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_CANCELED; virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"), qemuDomainAsyncJobTypeToString(priv->job.asyncJob), _("canceled by client")); - goto cleanup; + return -1; } if (dconn && virConnectIsAlive(dconn) <= 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Lost connection to destination host")); - goto cleanup; + return -1; } if (virDomainObjWait(vm) < 0) - goto cleanup; + return -1; } qemuMigrationSrcFetchMirrorStats(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT, @@ -1051,11 +1050,8 @@ qemuMigrationSrcNBDStorageCopy(virQEMUDriverPtr driver, /* Okay, all disks are ready. Modify migrate_flags */ *migrate_flags &= ~(QEMU_MONITOR_MIGRATE_NON_SHARED_DISK | QEMU_MONITOR_MIGRATE_NON_SHARED_INC); - ret = 0; - cleanup: - virObjectUnref(cfg); - return ret; + return 0; } -- 2.21.0
When blockdev is used we always should use the blockdev mode for non-shared storage migration. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index c13b21a0bb..6ff956b1f5 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -907,6 +907,7 @@ qemuMigrationSrcNBDStorageCopyOne(virQEMUDriverPtr driver, const char *tlsAlias, unsigned int flags) { + qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); qemuBlockJobDataPtr job = NULL; char *diskAlias = NULL; @@ -921,7 +922,8 @@ qemuMigrationSrcNBDStorageCopyOne(virQEMUDriverPtr driver, qemuBlockJobSyncBegin(job); - if (flags & VIR_MIGRATE_TLS) { + if (flags & VIR_MIGRATE_TLS || + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) { rc = qemuMigrationSrcNBDStorageCopyBlockdev(driver, vm, disk, diskAlias, host, port, -- 2.21.0
On 9/4/19 10:39 AM, Peter Krempa wrote:
Once we enable blockdev there's no need to stick to drive mirror for non-shared-storage migration.
This patchset applies on top of the external snapshot branch: https://www.redhat.com/archives/libvir-list/2019-September/msg00082.html
Peter Krempa (4): qemu: migration: Refactor cleanup in qemuMigrationSrcNBDStorageCopyBlockdev qemu: migration: Refactor cleanup in qemuMigrationSrcNBDStorageCopyDriveMirror qemu: migration: Refactor cleanup in qemuMigrationSrcNBDStorageCopy qemu: migration: Switch to blockdev mode for non-shared storage migration
In spite of my question on patch 2, series: Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa