Re: [PATCH v5 1/4] blockcopy: add more XML for state tracking

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]