On 07/29/14 06:20, Eric Blake wrote:
> Doing a blockcopy operation across a libvirtd restart is not very
> robust at the moment. In particular, we are clearing the <mirror>
> element prior to telling qemu to finish the job; and thanks to the
> ability to request async completion, the user can easily regain
> control prior to qemu actually finishing the effort, and they should
> be able to poll the domain XML to see if the job is still going.
>
> A future patch will fix things to actually wait until qemu is done
> before modifying the XML to reflect the job completion. But since
> qemu issues identical BLOCK_JOB_COMPLETE events regardless of whether
> the job was cancelled (kept the original disk) or completed (pivoted
> to the new disk), we have to track which of the two operations were
> used to end the job. Furthermore, we'd like to avoid attempts to
> end a job where we are already waiting on an earlier request to qemu
> to end the job. Likewise, if we miss the qemu event (perhaps because
> it arrived during a libvirtd restart), we still need enough state
> recorded to be able to determine how to modify the domain XML once
> we reconnect to qemu and manually learn whether the job still exists.
>
> Although this patch doesn't actually fix the problem, it is a
> preliminary step that makes it possible to track whether a job
> has already begun steps towards completion.
>
> * src/conf/domain_conf.h (virDomainDiskMirror): New enum.
> (_virDomainDiskDef): Convert bool mirroring to new enum.
> * src/conf/domain_conf.c (virDomainDiskDefParseXML)
> (virDomainDiskDefFormat): Handle new values.
> * src/qemu/qemu_process.c (qemuProcessHandleBlockJob): Adjust
> client.
> * src/qemu/qemu_driver.c (qemuDomainBlockPivot)
> (qemuDomainBlockJobImpl): Likewise.
> * docs/schemas/domaincommon.rng (diskMirror): Expose new values.
> * docs/formatdomain.html.in (elementsDisks): Document it.
> * tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml: Test it.
>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> docs/formatdomain.html.in | 10 +++++++---
> docs/schemas/domaincommon.rng | 6 +++++-
> src/conf/domain_conf.c | 23 +++++++++++++++++++---
> src/conf/domain_conf.h | 13 +++++++++++-
> src/qemu/qemu_driver.c | 12 +++++------
> src/qemu/qemu_process.c | 4 ++--
> .../qemuxml2argvdata/qemuxml2argv-disk-mirror.xml | 4 ++++
> 7 files changed, 56 insertions(+), 16 deletions(-)
>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index f6f697c..389c8c9 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4284,7 +4284,11 @@
> </choice>
> <optional>
> <attribute name='ready'>
> - <value>yes</value>
> + <choice>
> + <value>yes</value>
> + <value>abort</value>
Seems reasonable to store the state of the final step here.
> + <value>pivot</value>
> + </choice>
> </attribute>
> </optional>
> </element>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 910f6e2..1c8f8cf 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -747,6 +747,12 @@ VIR_ENUM_IMPL(virDomainDiskDiscard, VIR_DOMAIN_DISK_DISCARD_LAST,
> "unmap",
> "ignore")
>
> +VIR_ENUM_IMPL(virDomainDiskMirror, VIR_DOMAIN_DISK_MIRROR_LAST,
> + "default",
> + "yes",
> + "abort",
> + "pivot")
> +
> #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE
> #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE
>
> @@ -5482,7 +5488,14 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
> }
> ready = virXMLPropString(cur, "ready");
> if (ready) {
> - def->mirroring = true;
> + def->mirrorState = virDomainDiskMirrorTypeFromString(ready);
> + if (def->mirrorState < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("unknown mirror ready state %s"),
> + ready);
> + VIR_FREE(ready);
> + goto error;
> + }
> VIR_FREE(ready);
> }
> } else if (xmlStrEqual(cur->name, BAD_CAST "auth")) {
> @@ -15273,8 +15286,12 @@ virDomainDiskDefFormat(virBufferPtr buf,
> virBufferEscapeString(buf, " file='%s'", def->mirror->path);
> virBufferEscapeString(buf, " format='%s'", formatStr);
> }
> - if (def->mirroring)
> - virBufferAddLit(buf, " ready='yes'");
> + if (def->mirrorState) {
> + const char *mirror;
> +
> + mirror = virDomainDiskMirrorTypeToString(def->mirrorState);
> + virBufferEscapeString(buf, " ready='%s'", mirror);
> + }
> virBufferAddLit(buf, ">\n");
> virBufferAdjustIndent(buf, 2);
> virBufferEscapeString(buf, "<format type='%s'/>\n", formatStr);
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index b988b17..bc8966d 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -610,6 +610,16 @@ struct _virDomainBlockIoTuneInfo {
> typedef virDomainBlockIoTuneInfo *virDomainBlockIoTuneInfoPtr;
>
>
> +typedef enum {
> + VIR_DOMAIN_DISK_MIRROR_DEFAULT = 0, /* No job, or job still not synced */
I'd go with _NONE here and ...
> + VIR_DOMAIN_DISK_MIRROR_READY, /* Job in second phase */
> + VIR_DOMAIN_DISK_MIRROR_ABORT, /* Job aborted, waiting for event */
> + VIR_DOMAIN_DISK_MIRROR_PIVOT, /* Job pivoted, waiting for event */
> +
> + VIR_DOMAIN_DISK_MIRROR_LAST
> +} virDomainDiskMirror;
... as this describes the ready state of the mirroring operation I'd go
with virDomainDiskMirrorState and rename those fields accordingly.
Without that it's not clear what the enum describes.
> +
> +
> /* Stores the virtual disk configuration */
> struct _virDomainDiskDef {
> virStorageSourcePtr src; /* non-NULL. XXX Allow NULL for empty cdrom? */
> @@ -621,7 +631,7 @@ struct _virDomainDiskDef {
> int removable; /* enum virTristateSwitch */
>
> virStorageSourcePtr mirror;
> - bool mirroring;
> + int mirrorState; /* enum virDomainDiskMirror */
>
> struct {
> unsigned int cylinders;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 704ba39..acc9ef0 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -14846,7 +14846,7 @@ qemuDomainBlockPivot(virConnectPtr conn,
> format = virStorageFileFormatTypeToString(disk->mirror->format);
>
> /* Probe the status, if needed. */
> - if (!disk->mirroring) {
> + if (!disk->mirrorState) {
> qemuDomainObjEnterMonitor(driver, vm);
> rc = qemuMonitorBlockJob(priv->mon, device, NULL, NULL, 0, &info,
> BLOCK_JOB_INFO, true);
> @@ -14860,10 +14860,10 @@ qemuDomainBlockPivot(virConnectPtr conn,
> }
> if (rc == 1 && info.cur == info.end &&
> info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY)
> - disk->mirroring = true;
> + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_READY;
> }
>
> - if (!disk->mirroring) {
> + if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_READY) {
> virReportError(VIR_ERR_BLOCK_COPY_ACTIVE,
> _("disk '%s' not ready for pivot yet"),
> disk->dst);
> @@ -14939,7 +14939,7 @@ qemuDomainBlockPivot(virConnectPtr conn,
> }
>
> disk->mirror = NULL;
> - disk->mirroring = false;
> + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_DEFAULT;
>
> cleanup:
> /* revert to original disk def on failure */
> @@ -15096,7 +15096,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
> * avoid checking if pivot is safe. */
> if (mode == BLOCK_JOB_INFO && ret == 1 && disk->mirror &&
> info->cur == info->end && info->type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY)
> - disk->mirroring = true;
> + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_READY;
>
> /* A successful block job cancelation stops any mirroring. */
> if (mode == BLOCK_JOB_ABORT && disk->mirror) {
> @@ -15104,7 +15104,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
> * the mirror, and audit that fact, before dropping things. */
> virStorageSourceFree(disk->mirror);
> disk->mirror = NULL;
> - disk->mirroring = false;
> + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_DEFAULT;
> }
>
> waitjob:
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 3d10732..73ad27d 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1040,11 +1040,11 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> qemuDomainDetermineDiskChain(driver, vm, disk, true);
> if (disk->mirror && type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) {
> if (status == VIR_DOMAIN_BLOCK_JOB_READY) {
> - disk->mirroring = true;
> + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_READY;
> } else if (status == VIR_DOMAIN_BLOCK_JOB_FAILED) {
> virStorageSourceFree(disk->mirror);
> disk->mirror = NULL;
> - disk->mirroring = false;
> + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_DEFAULT;
> }
> }
> }
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml
> index 72b03c9..fa7af31 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-mirror.xml
> @@ -42,6 +42,10 @@
> <disk type='file' device='disk'>
> <source file='/tmp/logs.img'/>
> <backingStore/>
> + <mirror type='file' file='/tmp/logcopy.img' format='qcow2' ready='abort'>
> + <format type='qcow2'/>
> + <source file='/tmp/logcopy.img'/>
> + </mirror>
> <target dev='vdb' bus='virtio'/>
> </disk>
> <controller type='usb' index='0'/>
>
Otherwise looks good. ACK if you change the enum name and default value
name.
Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list