Re: [libvirt] [PATCH v3 03/18] blockjob: split out block info driver handling
On 09/04/2014 09:11 AM, Peter Krempa wrote:
> modify command. Technically, there is one case where getting
> block job info can modify domain XML - we do snooping to see if
> a 2-phase job has transitioned into the second phase, for an
> optimization in the case of old qemu that lacked an event for
> the transition. But I think that optimization is safe; and if
> it proves to be a problem, we could use the difference between
> QEMU_CAPS_BLOCKJOB_{ASYNC,SYNC} to determine whether we even
> need snooping, and request a modifying job in that case.
>
> +
> + if (info->type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT &&
> + disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT)
> + info->type = disk->mirrorJob;
> +
> + /* Snoop block copy operations, so future cancel operations can
> + * avoid checking if pivot is safe. Save the change to XML, but
> + * we can ignore failure because it is only an optimization. */
> + if (ret == 1 && disk->mirror &&
> + info->cur == info->end && !disk->mirrorState) {
> + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +
> + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
Atomicity is guaranteed by holding the domain lock here. We should
document that the mirrorState field can change even when the _MODIFY job
is held though.
Otherwise I don't see a problem currently with this as long as it is
stated somewhere. Possibly even in a comment here.
ACK
Here's what I squashed in.
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index e7bd504..fb13169 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -15289,7 +15289,9 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom,
const char *path,
/* Snoop block copy operations, so future cancel operations can
* avoid checking if pivot is safe. Save the change to XML, but
- * we can ignore failure because it is only an optimization. */
+ * we can ignore failure because it is only an optimization. We
+ * hold the vm lock, so modifying the in-memory representation
+ * even though we are a query rather than a modify job, is safe. */
if (ret == 1 && disk->mirror &&
info->cur == info->end && !disk->mirrorState) {
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 539 bytes)