11:25 a.m.
Block job handling violates our usage of domain jobs and changes disk source
definition behind our back.
Peter Krempa (3):
qemu: process: Export qemuProcessFindDomainDiskByAlias
qemu: event: Don't fiddle with disk backing trees without a job
qemu: Disallow concurrent block jobs on a single disk
src/conf/domain_conf.h | 1 +
src/qemu/qemu_domain.c | 23 +++++++
src/qemu/qemu_domain.h | 4 ++
src/qemu/qemu_driver.c | 170 +++++++++++++++++++++++++++++++++++++++++++-----
src/qemu/qemu_process.c | 131 +++++++------------------------------
src/qemu/qemu_process.h | 3 +
6 files changed, 211 insertions(+), 121 deletions(-)
--
2.2.2
11:25 a.m.
New subject: [libvirt] [PATCH 1/3] qemu: process: Export qemuProcessFindDomainDiskByAlias
---
src/qemu/qemu_process.c | 2 +-
src/qemu/qemu_process.h | 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index d1f089d..28c3c27 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -390,7 +390,7 @@ qemuProcessFindDomainDiskByPath(virDomainObjPtr vm,
return NULL;
}
-static virDomainDiskDefPtr
+virDomainDiskDefPtr
qemuProcessFindDomainDiskByAlias(virDomainObjPtr vm,
const char *alias)
{
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 5c70803..3c04179 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -112,4 +112,7 @@ int qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
virDomainGraphicsDefPtr graphics,
bool allocate);
+virDomainDiskDefPtr qemuProcessFindDomainDiskByAlias(virDomainObjPtr vm,
+ const char *alias);
+
#endif /* __QEMU_PROCESS_H__ */
--
2.2.2
1:03 p.m.
New subject: [libvirt] [PATCH 1/3] qemu: process: Export qemuProcessFindDomainDiskByAlias
On 03/13/2015 10:25 AM, Peter Krempa wrote:
---
src/qemu/qemu_process.c | 2 +-
src/qemu/qemu_process.h | 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:25 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: event: Don't fiddle with disk backing trees without a job
Surprisingly we did not grab a VM job when a block job finished and we'd
happily rewrite the backing chain data. This made it possible to crash
libvirt when queueing two backing chains tightly and other badness.
To fix it, add yet another handler to the helper thread that handles
monitor events that require a job.
---
src/qemu/qemu_domain.h | 2 +
src/qemu/qemu_driver.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_process.c | 129 ++++++++-----------------------------------
3 files changed, 168 insertions(+), 105 deletions(-)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index fe3e2b1..a7ebb47 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -196,6 +196,7 @@ typedef enum {
QEMU_PROCESS_EVENT_DEVICE_DELETED,
QEMU_PROCESS_EVENT_NIC_RX_FILTER_CHANGED,
QEMU_PROCESS_EVENT_SERIAL_CHANGED,
+ QEMU_PROCESS_EVENT_BLOCK_JOB,
QEMU_PROCESS_EVENT_LAST
} qemuProcessEventType;
@@ -204,6 +205,7 @@ struct qemuProcessEvent {
virDomainObjPtr vm;
qemuProcessEventType eventType;
int action;
+ int status;
void *data;
};
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b3263ac..1aed55f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4448,6 +4448,141 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
}
+static void
+processBlockJobEvent(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ char *diskAlias,
+ int type,
+ int status)
+{
+ virObjectEventPtr event = NULL;
+ virObjectEventPtr event2 = NULL;
+ const char *path;
+ virDomainDiskDefPtr disk;
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ virDomainDiskDefPtr persistDisk = NULL;
+ bool save = false;
+
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+ goto cleanup;
+
+ if (!virDomainObjIsActive(vm)) {
+ VIR_DEBUG("Domain is not running");
+ goto endjob;
+ }
+
+ disk = qemuProcessFindDomainDiskByAlias(vm, diskAlias);
+
+ if (disk) {
+ /* Have to generate two variants of the event for old vs. new
+ * client callbacks */
+ if (type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT &&
+ disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT)
+ type = disk->mirrorJob;
+ path = virDomainDiskGetSource(disk);
+ event = virDomainEventBlockJobNewFromObj(vm, path, type, status);
+ event2 = virDomainEventBlockJob2NewFromObj(vm, disk->dst, type,
+ status);
+
+ /* If we completed a block pull or commit, then update the XML
+ * to match. */
+ switch ((virConnectDomainEventBlockJobStatus) status) {
+ case VIR_DOMAIN_BLOCK_JOB_COMPLETED:
+ if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT) {
+ if (vm->newDef) {
+ int indx = virDomainDiskIndexByName(vm->newDef, disk->dst,
+ false);
+ virStorageSourcePtr copy = NULL;
+
+ if (indx >= 0) {
+ persistDisk = vm->newDef->disks[indx];
+ copy = virStorageSourceCopy(disk->mirror, false);
+ if (virStorageSourceInitChainElement(copy,
+ persistDisk->src,
+ true) < 0) {
+ VIR_WARN("Unable to update persistent definition "
+ "on vm %s after block job",
+ vm->def->name);
+ virStorageSourceFree(copy);
+ copy = NULL;
+ persistDisk = NULL;
+ }
+ }
+ if (copy) {
+ virStorageSourceFree(persistDisk->src);
+ persistDisk->src = copy;
+ }
+ }
+
+ /* XXX We want to revoke security labels and disk
+ * lease, as well as audit that revocation, before
+ * dropping the original source. But it gets tricky
+ * if both source and mirror share common backing
+ * files (we want to only revoke the non-shared
+ * portion of the chain); so for now, we leak the
+ * access to the original. */
+ virStorageSourceFree(disk->src);
+ disk->src = disk->mirror;
+ } else {
+ virStorageSourceFree(disk->mirror);
+ }
+
+ /* Recompute the cached backing chain to match our
+ * updates. Better would be storing the chain ourselves
+ * rather than reprobing, but we haven't quite completed
+ * that conversion to use our XML tracking. */
+ disk->mirror = NULL;
+ save = disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
+ disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
+ disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
+ ignore_value(qemuDomainDetermineDiskChain(driver, vm, disk,
+ true, true));
+ break;
+
+ case VIR_DOMAIN_BLOCK_JOB_READY:
+ disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
+ save = true;
+ break;
+
+ case VIR_DOMAIN_BLOCK_JOB_FAILED:
+ case VIR_DOMAIN_BLOCK_JOB_CANCELED:
+ virStorageSourceFree(disk->mirror);
+ disk->mirror = NULL;
+ disk->mirrorState = status == VIR_DOMAIN_BLOCK_JOB_FAILED ?
+ VIR_DOMAIN_DISK_MIRROR_STATE_ABORT : VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
+ disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
+ save = true;
+ break;
+
+ case VIR_DOMAIN_BLOCK_JOB_LAST:
+ break;
+ }
+ }
+
+ if (save) {
+ if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
+ VIR_WARN("Unable to save status on vm %s after block job",
+ vm->def->name);
+ if (persistDisk && virDomainSaveConfig(cfg->configDir,
+ vm->newDef) < 0)
+ VIR_WARN("Unable to update persistent definition on vm %s "
+ "after block job", vm->def->name);
+ }
+ virObjectUnlock(vm);
+ virObjectUnref(cfg);
+
+ if (event)
+ qemuDomainEventQueue(driver, event);
+ if (event2)
+ qemuDomainEventQueue(driver, event2);
+
+ endjob:
+ qemuDomainObjEndJob(driver, vm);
+ cleanup:
+ VIR_FREE(diskAlias);
+}
+
+
static void qemuProcessEventHandler(void *data, void *opaque)
{
struct qemuProcessEvent *processEvent = data;
@@ -4474,6 +4609,13 @@ static void qemuProcessEventHandler(void *data, void *opaque)
case QEMU_PROCESS_EVENT_SERIAL_CHANGED:
processSerialChangedEvent(driver, vm, processEvent->data,
processEvent->action);
+ break;
+ case QEMU_PROCESS_EVENT_BLOCK_JOB:
+ processBlockJobEvent(driver, vm,
+ processEvent->data,
+ processEvent->action,
+ processEvent->status);
+ break;
case QEMU_PROCESS_EVENT_LAST:
break;
}
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 28c3c27..b841e8d 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1017,123 +1017,42 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
void *opaque)
{
virQEMUDriverPtr driver = opaque;
- virObjectEventPtr event = NULL;
- virObjectEventPtr event2 = NULL;
- const char *path;
- virDomainDiskDefPtr disk;
- virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
- virDomainDiskDefPtr persistDisk = NULL;
- bool save = false;
+ struct qemuProcessEvent *processEvent = NULL;
+ char *data;
virObjectLock(vm);
- disk = qemuProcessFindDomainDiskByAlias(vm, diskAlias);
- if (disk) {
- /* Have to generate two variants of the event for old vs. new
- * client callbacks */
- if (type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT &&
- disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT)
- type = disk->mirrorJob;
- path = virDomainDiskGetSource(disk);
- event = virDomainEventBlockJobNewFromObj(vm, path, type, status);
- event2 = virDomainEventBlockJob2NewFromObj(vm, disk->dst, type,
- status);
-
- /* If we completed a block pull or commit, then update the XML
- * to match. */
- switch ((virConnectDomainEventBlockJobStatus) status) {
- case VIR_DOMAIN_BLOCK_JOB_COMPLETED:
- if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT) {
- if (vm->newDef) {
- int indx = virDomainDiskIndexByName(vm->newDef, disk->dst,
- false);
- virStorageSourcePtr copy = NULL;
-
- if (indx >= 0) {
- persistDisk = vm->newDef->disks[indx];
- copy = virStorageSourceCopy(disk->mirror, false);
- if (virStorageSourceInitChainElement(copy,
- persistDisk->src,
- true) < 0) {
- VIR_WARN("Unable to update persistent definition "
- "on vm %s after block job",
- vm->def->name);
- virStorageSourceFree(copy);
- copy = NULL;
- persistDisk = NULL;
- }
- }
- if (copy) {
- virStorageSourceFree(persistDisk->src);
- persistDisk->src = copy;
- }
- }
-
- /* XXX We want to revoke security labels and disk
- * lease, as well as audit that revocation, before
- * dropping the original source. But it gets tricky
- * if both source and mirror share common backing
- * files (we want to only revoke the non-shared
- * portion of the chain); so for now, we leak the
- * access to the original. */
- virStorageSourceFree(disk->src);
- disk->src = disk->mirror;
- } else {
- virStorageSourceFree(disk->mirror);
- }
+ VIR_DEBUG("Block job for device %s (domain: %p,%s) type %d status %d",
+ diskAlias, vm, vm->def->name, type, status);
- /* Recompute the cached backing chain to match our
- * updates. Better would be storing the chain ourselves
- * rather than reprobing, but we haven't quite completed
- * that conversion to use our XML tracking. */
- disk->mirror = NULL;
- save = disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
- disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
- disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
- ignore_value(qemuDomainDetermineDiskChain(driver, vm, disk,
- true, true));
- break;
-
- case VIR_DOMAIN_BLOCK_JOB_READY:
- disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
- save = true;
- break;
+ if (VIR_ALLOC(processEvent) < 0)
+ goto error;
- case VIR_DOMAIN_BLOCK_JOB_FAILED:
- case VIR_DOMAIN_BLOCK_JOB_CANCELED:
- virStorageSourceFree(disk->mirror);
- disk->mirror = NULL;
- disk->mirrorState = status == VIR_DOMAIN_BLOCK_JOB_FAILED ?
- VIR_DOMAIN_DISK_MIRROR_STATE_ABORT : VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
- disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
- save = true;
- break;
+ processEvent->eventType = QEMU_PROCESS_EVENT_BLOCK_JOB;
+ if (VIR_STRDUP(data, diskAlias) < 0)
+ goto error;
+ processEvent->data = data;
+ processEvent->vm = vm;
+ processEvent->action = type;
+ processEvent->status = status;
- case VIR_DOMAIN_BLOCK_JOB_LAST:
- break;
- }
+ virObjectRef(vm);
+ if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) {
+ ignore_value(virObjectUnref(vm));
+ goto error;
}
- if (save) {
- if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
- VIR_WARN("Unable to save status on vm %s after block job",
- vm->def->name);
- if (persistDisk && virDomainSaveConfig(cfg->configDir,
- vm->newDef) < 0)
- VIR_WARN("Unable to update persistent definition on vm %s "
- "after block job", vm->def->name);
- }
+ cleanup:
virObjectUnlock(vm);
- virObjectUnref(cfg);
-
- if (event)
- qemuDomainEventQueue(driver, event);
- if (event2)
- qemuDomainEventQueue(driver, event2);
-
return 0;
+ error:
+ if (processEvent)
+ VIR_FREE(processEvent->data);
+ VIR_FREE(processEvent);
+ goto cleanup;
}
+
static int
qemuProcessHandleGraphics(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
virDomainObjPtr vm,
--
2.2.2
2:31 p.m.
New subject: [libvirt] [PATCH 2/3] qemu: event: Don't fiddle with disk backing trees without a job
On 03/13/2015 10:25 AM, Peter Krempa wrote:
Surprisingly we did not grab a VM job when a block job finished and
we'd
happily rewrite the backing chain data. This made it possible to crash
libvirt when queueing two backing chains tightly and other badness.
My fault for violating the rule of 'no VM modifications without a job'.
Thanks for finding and fixing this.
To fix it, add yet another handler to the helper thread that handles
monitor events that require a job.
---
src/qemu/qemu_domain.h | 2 +
src/qemu/qemu_driver.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_process.c | 129 ++++++++-----------------------------------
3 files changed, 168 insertions(+), 105 deletions(-)
+processBlockJobEvent(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ char *diskAlias,
+ int type,
+ int status)
+{
+ virObjectEventPtr event = NULL;
+ virObjectEventPtr event2 = NULL;
+ const char *path;
+ virDomainDiskDefPtr disk;
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ virDomainDiskDefPtr persistDisk = NULL;
+ bool save = false;
+
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+ goto cleanup;
+
+ if (!virDomainObjIsActive(vm)) {
+ VIR_DEBUG("Domain is not running");
+ goto endjob;
+ }
Hmm. I suspect we could hit the situation where the pivot after active
block commit or block copy happens just before the guest quits, and thus
where we fail to update the XML to record that the pivot happened and
instead leave the XML in its old state. Probably not the end of the
world (at that point, the amount of work done by the guest after pivot
is not much, so restarting the guest from the unpivoted disk is
hopefully not inconsistent); except that a guest shutting down might
make the small modification of marking a file system clean that turns
out to have a large impact on how the guest next starts if it starts
from the wrong disk. I don't know if we can do any better, and at a
minimum this is a strict improvement over what we had before, so I'm not
going to reject the patch because of it, but it is food for thought.
[Actually, we probably need the notion of persistent bitmaps, which
looks like it might hit qemu 2.4, before we can guarantee that we KNOW
when a pivot job completed or failed, and thus always update the XML
correctly, based on the state of the persistent bitmap file]
virObjectLock(vm);
- disk = qemuProcessFindDomainDiskByAlias(vm, diskAlias);
- if (disk) {
The old code was locking the VM, but modifying without a job.
+ processEvent->eventType = QEMU_PROCESS_EVENT_BLOCK_JOB;
+ if (VIR_STRDUP(data, diskAlias) < 0)
+ goto error;
+ processEvent->data = data;
+ processEvent->vm = vm;
+ processEvent->action = type;
+ processEvent->status = status;
- case VIR_DOMAIN_BLOCK_JOB_LAST:
- break;
- }
+ virObjectRef(vm);
+ if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) {
+ ignore_value(virObjectUnref(vm));
+ goto error;
The new code is now throwing things over the fence to a helper thread,
so that blocking while waiting for the job is no longer holding up
monitor interactions. Looks correct to me.
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:35 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: event: Don't fiddle with disk backing trees without a job
On Fri, 13 Mar 2015, Peter Krempa wrote:
Surprisingly we did not grab a VM job when a block job finished and
we'd
happily rewrite the backing chain data. This made it possible to crash
libvirt when queueing two backing chains tightly and other badness.
To fix it, add yet another handler to the helper thread that handles
monitor events that require a job.
Hi Peter,
Unfortunately, I think this change has broken disk mirroring during
migration. When a disk mirror is ready (or aborted), processBlockJobEvent
blocks when beginning its MODIFY job because of the ongoing MIGRATION_OUT
async job.
I'd like to solve this by making use of blockJobSync from within
qemuMigrationDriveMirror. This is also going to fix another bug where
libvirt doesn't wait for QEMU to complete the drive mirroring before
migrating a VM (leading to disk corruption if there's ongoing writes!)
The plan would be to move qemuBlockJobEventProcess into a separate
qemu_blockjob.c, so it can be used from both qemu_driver and
qemu_migration, along with a few helper functions to set up blockJobSync
and to wait for it to be notified. Do you think this would be a suitable
approach?
- Michael
3:29 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: event: Don't fiddle with disk backing trees without a job
On 08.04.2015 08:35, Michael Chapman wrote:
On Fri, 13 Mar 2015, Peter Krempa wrote:
> Surprisingly we did not grab a VM job when a block job finished and we'd
> happily rewrite the backing chain data. This made it possible to crash
> libvirt when queueing two backing chains tightly and other badness.
>
> To fix it, add yet another handler to the helper thread that handles
> monitor events that require a job.
Hi Peter,
Unfortunately, I think this change has broken disk mirroring during
migration. When a disk mirror is ready (or aborted),
processBlockJobEvent blocks when beginning its MODIFY job because of the
ongoing MIGRATION_OUT async job.
Yes, I've already told Peter in person a week ago.
I'd like to solve this by making use of blockJobSync from within
qemuMigrationDriveMirror. This is also going to fix another bug where
libvirt doesn't wait for QEMU to complete the drive mirroring before
migrating a VM (leading to disk corruption if there's ongoing writes!)
The plan would be to move qemuBlockJobEventProcess into a separate
qemu_blockjob.c, so it can be used from both qemu_driver and
qemu_migration, along with a few helper functions to set up blockJobSync
and to wait for it to be notified. Do you think this would be a suitable
approach?
Sounds good to me.
Michal
11:25 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: Disallow concurrent block jobs on a single disk
While qemu may be prepared to do this libvirt is not. Forbid the block
ops until we fix our code.
---
src/conf/domain_conf.h | 1 +
src/qemu/qemu_domain.c | 23 +++++++++++++++++++++++
src/qemu/qemu_domain.h | 2 ++
src/qemu/qemu_driver.c | 28 +++++++++++++---------------
4 files changed, 39 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index ea463cb..6f2df46 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -664,6 +664,7 @@ struct _virDomainDiskDef {
int tray_status; /* enum virDomainDiskTray */
int removable; /* enum virTristateSwitch */
+ bool blockjob;
virStorageSourcePtr mirror;
int mirrorState; /* enum virDomainDiskMirrorState */
int mirrorJob; /* virDomainBlockJobType */
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index d8a2087..ff4307b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2756,6 +2756,29 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
return ret;
}
+
+bool
+qemuDomainDiskBlockJobIsActive(virDomainDiskDefPtr disk)
+{
+ if (disk->mirror) {
+ virReportError(VIR_ERR_BLOCK_COPY_ACTIVE,
+ _("disk '%s' already in active block job"),
+ disk->dst);
+
+ return true;
+ }
+
+ if (disk->blockjob) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("disk '%s' already in active block job"),
+ disk->dst);
+ return true;
+ }
+
+ return false;
+}
+
+
int
qemuDomainUpdateDeviceList(virQEMUDriverPtr driver,
virDomainObjPtr vm,
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index a7ebb47..41e075b 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -414,6 +414,8 @@ int qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4);
+bool qemuDomainDiskBlockJobIsActive(virDomainDiskDefPtr disk);
+
void qemuDomObjEndAPI(virDomainObjPtr *vm);
#endif /* __QEMU_DOMAIN_H__ */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1aed55f..864ee50 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4537,6 +4537,7 @@ processBlockJobEvent(virQEMUDriverPtr driver,
disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
ignore_value(qemuDomainDetermineDiskChain(driver, vm, disk,
true, true));
+ disk->blockjob = false;
break;
case VIR_DOMAIN_BLOCK_JOB_READY:
@@ -4552,6 +4553,7 @@ processBlockJobEvent(virQEMUDriverPtr driver,
VIR_DOMAIN_DISK_MIRROR_STATE_ABORT : VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
save = true;
+ disk->blockjob = false;
break;
case VIR_DOMAIN_BLOCK_JOB_LAST:
@@ -16133,6 +16135,7 @@ qemuDomainBlockPivot(virConnectPtr conn,
disk->mirror = NULL;
disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
+ disk->blockjob = false;
}
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
ret = -1;
@@ -16233,12 +16236,9 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
goto endjob;
disk = vm->def->disks[idx];
- if (mode == BLOCK_JOB_PULL && disk->mirror) {
- virReportError(VIR_ERR_BLOCK_COPY_ACTIVE,
- _("disk '%s' already in active block job"),
- disk->dst);
+ if (mode == BLOCK_JOB_PULL && qemuDomainDiskBlockJobIsActive(disk))
goto endjob;
- }
+
if (mode == BLOCK_JOB_ABORT) {
if ((flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT) &&
!(async && disk->mirror)) {
@@ -16322,6 +16322,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
if (mode == BLOCK_JOB_ABORT && disk->mirror)
disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
goto endjob;
+ } else if (mode == BLOCK_JOB_PULL) {
+ disk->blockjob = true;
}
waitjob:
@@ -16573,12 +16575,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
if (!device)
goto endjob;
disk = vm->def->disks[idx];
- if (disk->mirror) {
- virReportError(VIR_ERR_BLOCK_COPY_ACTIVE,
- _("disk '%s' already in active block job"),
- disk->dst);
+ if (qemuDomainDiskBlockJobIsActive(disk))
goto endjob;
- }
if (!(virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_MIRROR) &&
virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC))) {
@@ -16699,6 +16697,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
disk->mirror = mirror;
mirror = NULL;
disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY;
+ disk->blockjob = true;
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
VIR_WARN("Unable to save status on vm %s after state change",
@@ -16960,12 +16959,9 @@ qemuDomainBlockCommit(virDomainPtr dom,
disk->dst);
goto endjob;
}
- if (disk->mirror) {
- virReportError(VIR_ERR_BLOCK_COPY_ACTIVE,
- _("disk '%s' already in active block job"),
- disk->dst);
+
+ if (qemuDomainDiskBlockJobIsActive(disk))
goto endjob;
- }
if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0)
goto endjob;
@@ -17089,6 +17085,8 @@ qemuDomainBlockCommit(virDomainPtr dom,
goto endjob;
}
+ disk->blockjob = true;
+
if (mirror) {
if (ret == 0) {
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
--
2.2.2
2:36 p.m.
New subject: [libvirt] [PATCH 3/3] qemu: Disallow concurrent block jobs on a single disk
On 03/13/2015 10:25 AM, Peter Krempa wrote:
While qemu may be prepared to do this libvirt is not. Forbid the
block
ops until we fix our code.
---
src/conf/domain_conf.h | 1 +
src/qemu/qemu_domain.c | 23 +++++++++++++++++++++++
src/qemu/qemu_domain.h | 2 ++
src/qemu/qemu_driver.c | 28 +++++++++++++---------------
4 files changed, 39 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index ea463cb..6f2df46 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -664,6 +664,7 @@ struct _virDomainDiskDef {
int tray_status; /* enum virDomainDiskTray */
int removable; /* enum virTristateSwitch */
+ bool blockjob;
Might be worth a FIXME comment acknowledging that a bool will be
insufficient when we update our code to support parallel jobs.
+
+bool
+qemuDomainDiskBlockJobIsActive(virDomainDiskDefPtr disk)
+{
+ if (disk->mirror) {
+ virReportError(VIR_ERR_BLOCK_COPY_ACTIVE,
+ _("disk '%s' already in active block job"),
+ disk->dst);
+
+ return true;
+ }
+
+ if (disk->blockjob) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("disk '%s' already in active block job"),
+ disk->dst);
+ return true;
+ }
Shorter as 'if (disk->mirror || disk->blockjob)', up to you if you want
to compress the common code.
ACK. Looks like you have correctly locked out blockpull, blockcopy, and
blockcommit as the three jobs that are all mutually exclusive according
to our current abilities.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:19 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: Disallow concurrent block jobs on a single disk
On Fri, Mar 13, 2015 at 13:36:13 -0600, Eric Blake wrote:
On 03/13/2015 10:25 AM, Peter Krempa wrote:
> While qemu may be prepared to do this libvirt is not. Forbid the block
> ops until we fix our code.
> ---
> src/conf/domain_conf.h | 1 +
> src/qemu/qemu_domain.c | 23 +++++++++++++++++++++++
> src/qemu/qemu_domain.h | 2 ++
> src/qemu/qemu_driver.c | 28 +++++++++++++---------------
> 4 files changed, 39 insertions(+), 15 deletions(-)
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index ea463cb..6f2df46 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -664,6 +664,7 @@ struct _virDomainDiskDef {
> int tray_status; /* enum virDomainDiskTray */
> int removable; /* enum virTristateSwitch */
>
> + bool blockjob;
Might be worth a FIXME comment acknowledging that a bool will be
insufficient when we update our code to support parallel jobs.
Ok, I'll add a note.
> +
> +bool
> +qemuDomainDiskBlockJobIsActive(virDomainDiskDefPtr disk)
> +{
> + if (disk->mirror) {
> + virReportError(VIR_ERR_BLOCK_COPY_ACTIVE,
> + _("disk '%s' already in active block
job"),
> + disk->dst);
> +
> + return true;
> + }
> +
> + if (disk->blockjob) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("disk '%s' already in active block
job"),
> + disk->dst);
> + return true;
> + }
Shorter as 'if (disk->mirror || disk->blockjob)', up to you if you want
to compress the common code.
The errors have different error codes and I wanted to avoid a ternary
operator.
ACK. Looks like you have correctly locked out blockpull, blockcopy, and
blockcommit as the three jobs that are all mutually exclusive according
to our current abilities.
Peter
12:24 p.m.
On 03/13/2015 10:25 AM, Peter Krempa wrote:
Block job handling violates our usage of domain jobs and changes disk
source
definition behind our back.
Peter Krempa (3):
qemu: process: Export qemuProcessFindDomainDiskByAlias
qemu: event: Don't fiddle with disk backing trees without a job
qemu: Disallow concurrent block jobs on a single disk
On description alone, this should help solve the crash that Shanzhi
observed:
https://www.redhat.com/archives/libvir-list/2015-March/msg00656.html
src/conf/domain_conf.h | 1 +
src/qemu/qemu_domain.c | 23 +++++++
src/qemu/qemu_domain.h | 4 ++
src/qemu/qemu_driver.c | 170 +++++++++++++++++++++++++++++++++++++++++++-----
src/qemu/qemu_process.c | 131 +++++++------------------------------
src/qemu/qemu_process.h | 3 +
6 files changed, 211 insertions(+), 121 deletions(-)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
6 a.m.
On Fri, Mar 13, 2015 at 11:24:25 -0600, Eric Blake wrote:
On 03/13/2015 10:25 AM, Peter Krempa wrote:
> Block job handling violates our usage of domain jobs and changes disk source
> definition behind our back.
>
> Peter Krempa (3):
> qemu: process: Export qemuProcessFindDomainDiskByAlias
> qemu: event: Don't fiddle with disk backing trees without a job
> qemu: Disallow concurrent block jobs on a single disk
On description alone, this should help solve the crash that Shanzhi
observed:
https://www.redhat.com/archives/libvir-list/2015-March/msg00656.html
I've pushed this series. While my suggestion to check also the length of
the backing chain reported by qemu would fix that crash as a symptom,
this fixes the reason why it happened.
Peter
3518
days inactive
3544
days old
11 comments
4 participants
participants (4)
-
Eric Blake -
Michael Chapman -
Michal Privoznik -
Peter Krempa