[libvirt] [PATCH 0/4] qemu: clean up qemuDomainBlockJobAbort/Pivot (blockdev-add saga)
Peter Krempa (4): qemu: Always save status XML in qemuDomainBlockJobAbort qemu: Move shareable disk check for block copy qemu: Remove unused 'cfg' qemuDomainBlockPivot qemu: Use data in qemuBlockJobDataPtr instead of re-generating job name src/qemu/qemu_driver.c | 46 +++++++++++++++--------------------------- 1 file changed, 16 insertions(+), 30 deletions(-) -- 2.20.1
For copy and active commit jobs we record the state of the mirror so that we can recover. The status XML was not saved in case of qemuDomainBlockPivot due to an oversight. Save the XML always when invoking qemuDomainBlockJobAbort even if the job is not currently tracking any state. This will change later and also this is not a particularly hot code path. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2485dd93eb..e422a44b2e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17311,7 +17311,6 @@ qemuDomainBlockJobAbort(virDomainPtr dom, char *device = NULL; virDomainDiskDefPtr disk = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - bool save = false; bool pivot = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT); bool async = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC); qemuBlockJobDataPtr job = NULL; @@ -17363,10 +17362,8 @@ qemuDomainBlockJobAbort(virDomainPtr dom, if ((ret = qemuDomainBlockPivot(driver, vm, device, disk)) < 0) goto endjob; } else { - if (disk->mirror) { + if (disk->mirror) disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ABORT; - save = true; - } qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), device); @@ -17382,11 +17379,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom, } } - /* If we have made changes to XML due to a copy job, make a best - * effort to save it now. But we can ignore failure, since there - * will be further changes when the event marks completion. */ - if (save) - ignore_value(virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps)); + ignore_value(virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps)); /* * With the ABORT_ASYNC flag we don't need to do anything, the event will -- 2.20.1
On Thu, Jan 31, 2019 at 02:43:47PM +0100, Peter Krempa wrote:
For copy and active commit jobs we record the state of the mirror so that we can recover. The status XML was not saved in case of qemuDomainBlockPivot due to an oversight.
Save the XML always when invoking qemuDomainBlockJobAbort even if the job is not currently tracking any state. This will change later and also this is not a particularly hot code path.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The writing to an image actually starts when the copy job is initiated, so checking this at the time of the pivot operation is too late. Move the check to qemuDomainBlockCopyCommon. Note that modern qemu would have prevented two writers with qcow2 so the slim possibility of a job started with libvirtd without this patch missing the check is not really worth worrying about. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e422a44b2e..6674db7d20 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17140,16 +17140,6 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, goto cleanup; } - /* When pivoting to a shareable disk we need to make sure that the disk can - * be safely shared, since block copy might have changed the format. */ - if (disk->src->shared && !disk->src->readonly && - !qemuBlockStorageSourceSupportsConcurrentAccess(disk->mirror)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("can't pivot a shared disk to a storage volume not " - "supporting sharing")); - goto cleanup; - } - /* Attempt the pivot. Record the attempt now, to prevent duplicate * attempts; but the actual disk change will be made when emitting * the event. @@ -17780,6 +17770,16 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, } } + /* When copying a shareable disk we need to make sure that the disk can + * be safely shared, since block copy may change the format. */ + if (disk->src->shared && !disk->src->readonly && + !qemuBlockStorageSourceSupportsConcurrentAccess(mirror)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("can't pivot a shared disk to a storage volume not " + "supporting sharing")); + goto endjob; + } + /* pre-create the image file */ if (!reuse) { if (virStorageFileCreate(mirror) < 0) { -- 2.20.1
On Thu, Jan 31, 2019 at 02:43:48PM +0100, Peter Krempa wrote:
The writing to an image actually starts when the copy job is initiated, so checking this at the time of the pivot operation is too late.
Move the check to qemuDomainBlockCopyCommon. Note that modern qemu would have prevented two writers with qcow2 so the slim possibility of a job started with libvirtd without this patch missing the check is not really worth worrying about.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6674db7d20..c80d126276 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17124,7 +17124,6 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); if (!disk->mirror) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -17165,7 +17164,6 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, } cleanup: - virObjectUnref(cfg); return ret; } -- 2.20.1
On Thu, Jan 31, 2019 at 02:43:49PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 2 -- 1 file changed, 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
qemuDomainBlockPivot and qemuDomainBlockJobAbort need the job name for cancelling or pivoting but were generating it locally instead of accessing the existing copy in the job data structure. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c80d126276..f790ffdf36 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17119,7 +17119,7 @@ qemuDomainOpenChannel(virDomainPtr dom, static int qemuDomainBlockPivot(virQEMUDriverPtr driver, virDomainObjPtr vm, - const char *device, + qemuBlockJobDataPtr job, virDomainDiskDefPtr disk) { int ret = -1; @@ -17151,7 +17151,7 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, * overall return value. */ disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT; qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorDrivePivot(priv->mon, device); + ret = qemuMonitorDrivePivot(priv->mon, job->name); if (qemuDomainObjExitMonitor(driver, vm) < 0) { ret = -1; goto cleanup; @@ -17296,7 +17296,6 @@ qemuDomainBlockJobAbort(virDomainPtr dom, unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; - char *device = NULL; virDomainDiskDefPtr disk = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); bool pivot = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT); @@ -17326,9 +17325,6 @@ qemuDomainBlockJobAbort(virDomainPtr dom, if (!(disk = qemuDomainDiskByName(vm->def, path))) goto endjob; - if (!(device = qemuAliasDiskDriveFromDisk(disk))) - goto endjob; - if (!(job = qemuBlockJobDiskGetJob(disk))) { virReportError(VIR_ERR_INVALID_ARG, _("disk %s does not have an active block job"), disk->dst); @@ -17347,14 +17343,14 @@ qemuDomainBlockJobAbort(virDomainPtr dom, qemuBlockJobSyncBegin(job); if (pivot) { - if ((ret = qemuDomainBlockPivot(driver, vm, device, disk)) < 0) + if ((ret = qemuDomainBlockPivot(driver, vm, job, disk)) < 0) goto endjob; } else { if (disk->mirror) disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ABORT; qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), device); + ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), job->name); if (qemuDomainObjExitMonitor(driver, vm) < 0) { ret = -1; goto endjob; @@ -17394,7 +17390,6 @@ qemuDomainBlockJobAbort(virDomainPtr dom, cleanup: virObjectUnref(job); virObjectUnref(cfg); - VIR_FREE(device); virDomainObjEndAPI(&vm); return ret; } -- 2.20.1
On Thu, Jan 31, 2019 at 02:43:50PM +0100, Peter Krempa wrote:
qemuDomainBlockPivot and qemuDomainBlockJobAbort need the job name for cancelling or pivoting but were generating it locally instead of accessing the existing copy in the job data structure.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa