[libvirt] [PATCH 0/8] qemu: Fix non-shared storage migration over NBD (blockdev-add sequels)
I put some blockdev awareness into the non-shared storage migration NBD code but it was not complete. Seeing it there mislead me thinking that it's done. Fix it now. Peter Krempa (8): qemu: migration: Simplify handling of 'diskAlias' when adding NBD exports qemu: migration: Properly export backend for NBD storage migration qemu: migration: Access job name from job struct qemu: migration: Simplify cleanup in qemuMigrationSrcNBDCopyCancelOne qemu: blockjob: Allow NULL 'mirror' for block copy jobs due to migration qemu: migration: Split out setup of the migration target qemu: migration: Mention disk target rather than the drive name in debug msg qemu: migration: Properly setup mirror for blockdev configurations src/qemu/qemu_blockjob.c | 11 +++- src/qemu/qemu_migration.c | 115 ++++++++++++++++++++++++-------------- 2 files changed, 80 insertions(+), 46 deletions(-) -- 2.23.0
Declare the variable inside the loop with automatic clearing. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index dabdda2715..af22dfb48d 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -377,7 +377,6 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; unsigned short port = 0; - char *diskAlias = NULL; size_t i; virStorageNetHostDef server = { .name = (char *)listenAddr, /* cast away const */ @@ -392,6 +391,7 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; + g_autofree char *diskAlias = NULL; /* check whether disk should be migrated */ if (!qemuMigrationAnyCopyDisk(disk, nmigrate_disks, migrate_disks)) @@ -404,7 +404,6 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, goto cleanup; } - VIR_FREE(diskAlias); if (!(diskAlias = qemuAliasDiskDriveFromDisk(disk))) goto cleanup; @@ -433,7 +432,6 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, ret = 0; cleanup: - VIR_FREE(diskAlias); if (ret < 0 && nbdPort == 0) virPortAllocatorRelease(port); return ret; -- 2.23.0
On 12/6/19 12:09 PM, Peter Krempa wrote:
Declare the variable inside the loop with automatic clearing.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
With -blockdev we must use the nodename as the export but we must keep the name of the export as it was before to ensure compatiblity. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index af22dfb48d..2416dbe432 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -392,6 +392,8 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; g_autofree char *diskAlias = NULL; + const char *exportname = NULL; + const char *devicename = NULL; /* check whether disk should be migrated */ if (!qemuMigrationAnyCopyDisk(disk, nmigrate_disks, migrate_disks)) @@ -407,6 +409,14 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, if (!(diskAlias = qemuAliasDiskDriveFromDisk(disk))) goto cleanup; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) { + exportname = diskAlias; + devicename = disk->src->nodeformat; + } else { + exportname = NULL; + devicename = diskAlias; + } + if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_MIGRATION_IN) < 0) goto cleanup; @@ -422,7 +432,7 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, goto exit_monitor; } - if (qemuMonitorNBDServerAdd(priv->mon, diskAlias, NULL, true, NULL) < 0) + if (qemuMonitorNBDServerAdd(priv->mon, devicename, exportname, true, NULL) < 0) goto exit_monitor; if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; -- 2.23.0
On 12/6/19 12:09 PM, Peter Krempa wrote:
With -blockdev we must use the nodename as the export but we must keep the name of the export as it was before to ensure compatiblity.
compatibility Makes sense - this is code on the destination, but we may be migrating storage from an older libvirt that wasn't using blockdev, so we must export the same name as expected by that older client (our goal is to allow migration as a way to upgrade from a -device command line to a -blockdev command line, without any guest downtime) Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
qemuMigrationSrcNBDCopyCancelOne uses the block job data structure but generated it's own job name rather than taking it from the block job data. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 2416dbe432..b14a1bcd15 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -649,7 +649,6 @@ qemuMigrationSrcNBDCopyCancelOne(virQEMUDriverPtr driver, qemuDomainAsyncJob asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; - char *diskAlias = NULL; int ret = -1; int status; int rv; @@ -668,13 +667,10 @@ qemuMigrationSrcNBDCopyCancelOne(virQEMUDriverPtr driver, goto cleanup; } - if (!(diskAlias = qemuAliasDiskDriveFromDisk(disk))) - return -1; - if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto cleanup; - rv = qemuMonitorBlockJobCancel(priv->mon, diskAlias); + rv = qemuMonitorBlockJobCancel(priv->mon, job->name); if (qemuDomainObjExitMonitor(driver, vm) < 0 || rv < 0) goto cleanup; @@ -682,7 +678,6 @@ qemuMigrationSrcNBDCopyCancelOne(virQEMUDriverPtr driver, ret = 0; cleanup: - VIR_FREE(diskAlias); return ret; } -- 2.23.0
On 12/6/19 12:09 PM, Peter Krempa wrote:
qemuMigrationSrcNBDCopyCancelOne uses the block job data structure but generated it's own job name rather than taking it from the block job data.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Now that the cleanup section does not exist remove the label. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b14a1bcd15..9062be38ab 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -649,7 +649,6 @@ qemuMigrationSrcNBDCopyCancelOne(virQEMUDriverPtr driver, qemuDomainAsyncJob asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; - int ret = -1; int status; int rv; @@ -659,26 +658,22 @@ qemuMigrationSrcNBDCopyCancelOne(virQEMUDriverPtr driver, case VIR_DOMAIN_BLOCK_JOB_CANCELED: if (failNoJob) { qemuMigrationNBDReportMirrorError(job, disk->dst); - goto cleanup; + return -1; } G_GNUC_FALLTHROUGH; case VIR_DOMAIN_BLOCK_JOB_COMPLETED: - ret = 1; - goto cleanup; + return 1; } if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) - goto cleanup; + return -1; rv = qemuMonitorBlockJobCancel(priv->mon, job->name); if (qemuDomainObjExitMonitor(driver, vm) < 0 || rv < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - return ret; + return 0; } -- 2.23.0
On 12/6/19 12:09 PM, Peter Krempa wrote:
Now that the cleanup section does not exist remove the label.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
The non-shared-storage migration tracks the storage source used explicitly in the migration data so we must allow for processing of the block job which has NULL mirror as the mirror will not be populated. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index baa79ea80c..2773acc990 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1139,7 +1139,9 @@ qemuBlockJobProcessEventConcludedCopyPivot(virQEMUDriverPtr driver, { VIR_DEBUG("copy job '%s' on VM '%s' pivoted", job->name, vm->def->name); - if (!job->disk) + /* mirror may be NULL for copy job corresponding to migragion */ + if (!job->disk || + !job->disk->mirror) return; /* for shallow copy without reusing external image the user can either not @@ -1166,7 +1168,9 @@ qemuBlockJobProcessEventConcludedCopyAbort(virQEMUDriverPtr driver, { VIR_DEBUG("copy job '%s' on VM '%s' aborted", job->name, vm->def->name); - if (!job->disk) + /* mirror may be NULL for copy job corresponding to migragion */ + if (!job->disk || + !job->disk->mirror) return; qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, job->disk->mirror); @@ -1383,7 +1387,8 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver, break; case QEMU_BLOCKJOB_STATE_READY: - if (job->disk && job->disk->mirror) { + /* mirror may be NULL for copy job corresponding to migragion */ + if (job->disk) { job->disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; qemuBlockJobEmitEvents(driver, vm, job->disk, job->type, job->newstate); } -- 2.23.0
On 12/6/19 12:09 PM, Peter Krempa wrote:
The non-shared-storage migration tracks the storage source used explicitly in the migration data so we must allow for processing of the block job which has NULL mirror as the mirror will not be populated.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index baa79ea80c..2773acc990 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1139,7 +1139,9 @@ qemuBlockJobProcessEventConcludedCopyPivot(virQEMUDriverPtr driver, { VIR_DEBUG("copy job '%s' on VM '%s' pivoted", job->name, vm->def->name);
- if (!job->disk) + /* mirror may be NULL for copy job corresponding to migragion */
migration (3 times) With typos fixed, Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Separate out allocation of the virStorageSource corresponding to the target NBD export of the migration. As part of the splitout we allocate the export name explicitly as that one must not change regardless whether blockdev is used or not to provide compatibility. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 55 +++++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9062be38ab..8bf23e9f30 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -776,38 +776,28 @@ qemuMigrationSrcNBDCopyCancel(virQEMUDriverPtr driver, } -static int -qemuMigrationSrcNBDStorageCopyBlockdev(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDiskDefPtr disk, - const char *diskAlias, - const char *host, - int port, - unsigned long long mirror_speed, - unsigned int mirror_shallow, - const char *tlsAlias) +static virStorageSourcePtr +qemuMigrationSrcNBDStorageCopyBlockdevPrepareSource(virDomainDiskDefPtr disk, + const char *host, + int port, + const char *tlsAlias) { - g_autoptr(qemuBlockStorageSourceAttachData) data = NULL; - qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - int mon_ret = 0; g_autoptr(virStorageSource) copysrc = NULL; - VIR_DEBUG("starting blockdev mirror for disk=%s to host=%s", diskAlias, host); - if (!(copysrc = virStorageSourceNew())) - return -1; + return NULL; copysrc->type = VIR_STORAGE_TYPE_NETWORK; copysrc->protocol = VIR_STORAGE_NET_PROTOCOL_NBD; copysrc->format = VIR_STORAGE_FILE_RAW; if (!(copysrc->backingStore = virStorageSourceNew())) - return -1; + return NULL; - copysrc->path = g_strdup(diskAlias); + if (!(copysrc->path = qemuAliasDiskDriveFromDisk(disk))) + return NULL; - if (VIR_ALLOC_N(copysrc->hosts, 1) < 0) - return -1; + copysrc->hosts = g_new0(virStorageNetHostDef, 1); copysrc->nhosts = 1; copysrc->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; @@ -819,6 +809,31 @@ qemuMigrationSrcNBDStorageCopyBlockdev(virQEMUDriverPtr driver, copysrc->nodestorage = g_strdup_printf("migration-%s-storage", disk->dst); copysrc->nodeformat = g_strdup_printf("migration-%s-format", disk->dst); + return g_steal_pointer(©src); +} + + +static int +qemuMigrationSrcNBDStorageCopyBlockdev(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr disk, + const char *diskAlias, + const char *host, + int port, + unsigned long long mirror_speed, + unsigned int mirror_shallow, + const char *tlsAlias) +{ + g_autoptr(qemuBlockStorageSourceAttachData) data = NULL; + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + int mon_ret = 0; + g_autoptr(virStorageSource) copysrc = NULL; + + VIR_DEBUG("starting blockdev mirror for disk=%s to host=%s", diskAlias, host); + + if (!(copysrc = qemuMigrationSrcNBDStorageCopyBlockdevPrepareSource(disk, host, port, tlsAlias))) + return -1; + /* Migration via blockdev-mirror was supported sooner than the auto-read-only * feature was added to qemu */ if (!(data = qemuBlockStorageSourceAttachPrepareBlockdev(copysrc, -- 2.23.0
On 12/6/19 12:09 PM, Peter Krempa wrote:
Separate out allocation of the virStorageSource corresponding to the target NBD export of the migration.
As part of the splitout we allocate the export name explicitly as that one must not change regardless whether blockdev is used or not to provide compatibility.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 55 +++++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 20 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 8bf23e9f30..27023c94b1 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -829,7 +829,7 @@ qemuMigrationSrcNBDStorageCopyBlockdev(virQEMUDriverPtr driver, int mon_ret = 0; g_autoptr(virStorageSource) copysrc = NULL; - VIR_DEBUG("starting blockdev mirror for disk=%s to host=%s", diskAlias, host); + VIR_DEBUG("starting blockdev mirror for disk=%s to host=%s", disk->dst, host); if (!(copysrc = qemuMigrationSrcNBDStorageCopyBlockdevPrepareSource(disk, host, port, tlsAlias))) return -1; -- 2.23.0
On 12/6/19 12:09 PM, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 8bf23e9f30..27023c94b1 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -829,7 +829,7 @@ qemuMigrationSrcNBDStorageCopyBlockdev(virQEMUDriverPtr driver, int mon_ret = 0; g_autoptr(virStorageSource) copysrc = NULL;
- VIR_DEBUG("starting blockdev mirror for disk=%s to host=%s", diskAlias, host); + VIR_DEBUG("starting blockdev mirror for disk=%s to host=%s", disk->dst, host);
if (!(copysrc = qemuMigrationSrcNBDStorageCopyBlockdevPrepareSource(disk, host, port, tlsAlias))) return -1;
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
With blockdev we need to refer to the nodename of the disk source image as the source argument for the blockdev-mirror operation while still keeping the old job name. With blockdev we must also persist the job in qemu. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 27023c94b1..6dfcd5a642 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -817,7 +817,9 @@ static int qemuMigrationSrcNBDStorageCopyBlockdev(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, - const char *diskAlias, + const char *jobname, + const char *sourcename, + bool persistjob, const char *host, int port, unsigned long long mirror_speed, @@ -848,8 +850,8 @@ qemuMigrationSrcNBDStorageCopyBlockdev(virQEMUDriverPtr driver, mon_ret = qemuBlockStorageSourceAttachApply(qemuDomainGetMonitor(vm), data); if (mon_ret == 0) - mon_ret = qemuMonitorBlockdevMirror(qemuDomainGetMonitor(vm), NULL, false, - diskAlias, copysrc->nodeformat, + mon_ret = qemuMonitorBlockdevMirror(qemuDomainGetMonitor(vm), jobname, persistjob, + sourcename, copysrc->nodeformat, mirror_speed, 0, 0, mirror_shallow); if (mon_ret != 0) @@ -914,6 +916,9 @@ qemuMigrationSrcNBDStorageCopyOne(virQEMUDriverPtr driver, qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); qemuBlockJobDataPtr job = NULL; char *diskAlias = NULL; + const char *jobname = NULL; + const char *sourcename = NULL; + bool persistjob = false; int rc; int ret = -1; @@ -923,12 +928,23 @@ qemuMigrationSrcNBDStorageCopyOne(virQEMUDriverPtr driver, if (!(job = qemuBlockJobDiskNew(vm, disk, QEMU_BLOCKJOB_TYPE_COPY, diskAlias))) goto cleanup; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) { + jobname = diskAlias; + sourcename = disk->src->nodeformat; + persistjob = true; + } else { + jobname = NULL; + sourcename = diskAlias; + persistjob = false; + } + qemuBlockJobSyncBegin(job); if (flags & VIR_MIGRATE_TLS || virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV)) { rc = qemuMigrationSrcNBDStorageCopyBlockdev(driver, vm, - disk, diskAlias, + disk, jobname, + sourcename, persistjob, host, port, mirror_speed, mirror_shallow, -- 2.23.0
On 12/6/19 12:09 PM, Peter Krempa wrote:
With blockdev we need to refer to the nodename of the disk source image as the source argument for the blockdev-mirror operation while still keeping the old job name. With blockdev we must also persist the job in qemu.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)
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