This commit (finally) adds the virDomainBlockCopy API, with the intent that it will provide more power to the existing 'virsh blockcopy' command. 'virsh blockcopy' was first added in Apr 2012 (v0.9.12), which corresponds to the upstream qemu 1.2 timeframe. It was done as a hack on top of the existing virDomainBlockRebase() API call, for two reasons: 1) it was targetting a feature that landed first in downstream RHEL qemu, but had not stabilized in upstream qemu at the time (and indeed, 'drive-mirror' only landed upstream in qemu 1.3 with slight differences to the first RHEL attempt, and later gained further parameters like granularity and buf-size that are also worth exposing), and 2) extending an existing API allowed it to be backported without worrying about bumping .so versions. A virDomainBlockCopy() API was proposed at that time [1], but we decided not to accept it into libvirt until after upstream qemu stabilized, and it ended up getting scrapped. Whether or not RHEL should have attempted adding a new feature without getting it upstream first is a debate that can be held another day; but enough time has now elapsed that we are ready to do the interface cleanly. [1] https://www.redhat.com/archives/libvir-list/2012-April/msg00768.html Delaying the creation of a clean API until now has also had a benefit: we've only recently learned of a shortcoming in the original design, namely, that it is unable to target a network destination (such as a gluster volume) because it hard-coded the assumption that the destination is a local file name. Because of all the refactoring we've done to add virStorageSourcePtr, we are in a better position to declare an API that parses XML describing a host storage source as the copy destination, which was not possible had we implemented virDomainBlockCopy as it had been originally envisioned. At least I had the foresight to create 'virsh blockcopy' as a separate command at the UI level (commit 1f06c00) rather than leaking the underlying API overload of virDomainBlockRebase onto shell users. A note on the bandwidth option: virTypedParameters intentionally lacks unsigned long (since variable-width interaction between mixed 32- vs. 64-bit client/server setups is nasty), but we have to deal with the fact that we are interacting with existing older code that mistakenly chose unsigned long bandwidth at a point before we decided to prohibit it in all new API. The typed parameter is therefore unsigned long long, and the implementation (in a later patch) will have to do overflow detection on 32-bit platforms. (Actually, qemu treats bandwidth as bytes/second, while we document it as megabytes/second, so we already do overflow detection even on 64-bit hosts to prohibit anything larger than LLONG_MAX >> 20). * include/libvirt/libvirt.h.in (virDomainBlockCopy): New API. (virDomainBlockJobType, virConnectDomainEventBlockJobStatus): Update related documentation. * src/libvirt.c (virDomainBlockCopy): Implement it. * src/libvirt_public.syms (LIBVIRT_1.2.8): Export it. * src/driver.h (_virDriver): New driver callback. Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt.h.in | 61 ++++++++++++++++++++-- src/driver.h | 11 +++- src/libvirt.c | 120 ++++++++++++++++++++++++++++++++++++++++++- src/libvirt_public.syms | 5 ++ 4 files changed, 190 insertions(+), 7 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 47ea695..ebed373 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2518,16 +2518,16 @@ typedef enum { * flags), job ends on completion */ VIR_DOMAIN_BLOCK_JOB_TYPE_COPY = 2, - /* Block Copy (virDomainBlockRebase with flags), job exists as - * long as mirroring is active */ + /* Block Copy (virDomainBlockCopy, or virDomainBlockRebase with + * flags), job exists as long as mirroring is active */ VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT = 3, /* Block Commit (virDomainBlockCommit without flags), job ends on * completion */ VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT = 4, - /* Active Block Commit (virDomainBlockCommit with flags), job - * exists as long as sync is active */ + /* Active Block Commit (virDomainBlockCommit with flags), job exists + * as long as sync is active */ #ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_BLOCK_JOB_TYPE_LAST @@ -2597,6 +2597,57 @@ int virDomainBlockRebase(virDomainPtr dom, const char *disk, unsigned int flags); /** + * virDomainBlockCopyFlags: + * + * Flags available for virDomainBlockCopy(). + */ +typedef enum { + VIR_DOMAIN_BLOCK_COPY_SHALLOW = 1 << 0, /* Limit copy to top of source + backing chain */ + VIR_DOMAIN_BLOCK_COPY_REUSE_EXT = 1 << 1, /* Reuse existing external + file for a copy */ +} virDomainBlockCopyFlags; + +/** + * VIR_DOMAIN_BLOCK_COPY_BANDWIDTH: + * Macro for the virDomainBlockCopy bandwidth tunable: it represents + * the maximum bandwidth (in MiB/s) used while getting the copy + * operation into the mirrored phase, with a type of ullong (although + * the hypervisor may restrict the set of valid values to a smaller + * range). Specifying 0 is the same as omitting this parameter, to + * request the hypervisor default. Some hypervisors may lack support + * for this parameter, while still allowing a subsequent change of + * bandwidth via virDomainBlockJobSetSpeed(). The actual speed can + * be determined with virDomainGetBlockJobInfo(). + */ +#define VIR_DOMAIN_BLOCK_COPY_BANDWIDTH "bandwidth" + +/** + * VIR_DOMAIN_BLOCK_COPY_GRANULARITY: + * Macro for the virDomainBlockCopy granularity tunable: it represents + * the granularity in bytes at which the copy operation recognizes dirty + * blocks that need copying, as an unsigned int, and must be a power of + * two. Specifying 0 is the same as omitting this parameter, to request + * the hypervisor default. + */ +#define VIR_DOMAIN_BLOCK_COPY_GRANULARITY "granularity" + +/** + * VIR_DOMAIN_BLOCK_COPY_BUF_SIZE: + * Macro for the virDomainBlockCopy buffer size tunable: it represents + * how much data in bytes can be in flight between source and destination, + * as an unsigned int. Specifying 0 is the same as omitting this parameter, + * to request the hypervisor default. + */ +#define VIR_DOMAIN_BLOCK_COPY_BUF_SIZE "buf-size" + +int virDomainBlockCopy(virDomainPtr dom, const char *disk, + const char *destxml, + virTypedParameterPtr params, + int nparams, + unsigned int flags); + +/** * virDomainBlockCommitFlags: * * Flags available for virDomainBlockCommit(). @@ -4830,7 +4881,7 @@ typedef void (*virConnectDomainEventGraphicsCallback)(virConnectPtr conn, * virConnectDomainEventBlockJobStatus: * * Tracks status of a virDomainBlockPull(), virDomainBlockRebase(), - * or virDomainBlockCommit() operation + * virDomainBlockCopy(), or virDomainBlockCommit() operation */ typedef enum { VIR_DOMAIN_BLOCK_JOB_COMPLETED = 0, diff --git a/src/driver.h b/src/driver.h index ba7c1fc..14933a7 100644 --- a/src/driver.h +++ b/src/driver.h @@ -2,7 +2,7 @@ * driver.h: description of the set of interfaces provided by a * entry point to the virtualization engine * - * Copyright (C) 2006-2013 Red Hat, Inc. + * Copyright (C) 2006-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -1009,6 +1009,14 @@ typedef int unsigned int flags); typedef int +(*virDrvDomainBlockCopy)(virDomainPtr dom, + const char *path, + const char *destxml, + virTypedParameterPtr params, + int nparams, + unsigned int flags); + +typedef int (*virDrvDomainBlockCommit)(virDomainPtr dom, const char *disk, const char *base, @@ -1382,6 +1390,7 @@ struct _virDriver { virDrvDomainBlockJobSetSpeed domainBlockJobSetSpeed; virDrvDomainBlockPull domainBlockPull; virDrvDomainBlockRebase domainBlockRebase; + virDrvDomainBlockCopy domainBlockCopy; virDrvDomainBlockCommit domainBlockCommit; virDrvConnectSetKeepAlive connectSetKeepAlive; virDrvConnectIsAlive connectIsAlive; diff --git a/src/libvirt.c b/src/libvirt.c index 8349261..d2707ab 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -19924,7 +19924,11 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, * The actual speed can be determined with virDomainGetBlockJobInfo(). * * When @base is NULL and @flags is 0, this is identical to - * virDomainBlockPull(). + * virDomainBlockPull(). When @flags contains VIR_DOMAIN_BLOCK_REBASE_COPY, + * this command is shorthand for virDomainBlockCopy() where the destination + * has file type, @bandwidth is passed as a typed parameter, and the flags + * control whether the destination format is raw, identical to the source, + * or probed from the reused file. * * Returns 0 if the operation has started, -1 on failure. */ @@ -19975,6 +19979,120 @@ virDomainBlockRebase(virDomainPtr dom, const char *disk, /** + * virDomainBlockCopy: + * @dom: pointer to domain object + * @disk: path to the block device, or device shorthand + * @destxml: XML description of the copy destination + * @params: Pointer to block copy parameter objects, or NULL + * @nparams: Number of block copy parameters (this value can be the same or + * less than the number of parameters supported) + * @flags: bitwise-OR of virDomainBlockCopyFlags + * + * Copy the guest-visible contents of a disk image to a new file described + * by @destxml. The destination XML has a top-level element of <disk>, and + * resembles what is used when hot-plugging a disk via virDomainAttachDevice(), + * except that only sub-elements related to describing the new host resource + * are necessary (sub-elements related to the guest view, such as <target>, + * are ignored). It is strongly recommended to include a <driver type='...'/> + * format designation for the destination, to avoid the potential of any + * security problem that might be caused by probing a file for its format. + * + * This command starts a long-running copy. By default, the copy will pull + * the entire source chain into the destination file, but if @flags also + * contains VIR_DOMAIN_BLOCK_COPY_SHALLOW, then only the top of the source + * chain will be copied (the source and destination have a common backing + * file). The format of the destination file is controlled by the <driver> + * sub-element of the XML. The destination will be created unless the + * VIR_DOMAIN_BLOCK_COPY_REUSE_EXT flag is present stating that the file + * was pre-created with the correct format and metadata and sufficient + * size to hold the copy. In case the VIR_DOMAIN_BLOCK_COPY_SHALLOW flag + * is used the pre-created file has to exhibit the same guest visible contents + * as the backing file of the original image. This allows a management app to + * pre-create files with relative backing file names, rather than the default + * of absolute backing file names. + * + * A copy job has two parts; in the first phase, the source is copied into + * the destination, and the job can only be canceled by reverting to the + * source file; progress in this phase can be tracked via the + * virDomainBlockJobInfo() command, with a job type of + * VIR_DOMAIN_BLOCK_JOB_TYPE_COPY. The job transitions to the second + * phase when the job info states cur == end, and remains alive to mirror + * all further changes to both source and destination. The user must + * call virDomainBlockJobAbort() to end the mirroring while choosing + * whether to revert to source or pivot to the destination. An event is + * issued when the job ends, and depending on the hypervisor, an event may + * also be issued when the job transitions from pulling to mirroring. If + * the job is aborted, a new job will have to start over from the beginning + * of the first phase. + * + * Some hypervisors will restrict certain actions, such as virDomainSave() + * or virDomainDetachDevice(), while a copy job is active; they may + * also restrict a copy job to transient domains. + * + * The @disk parameter is either an unambiguous source name of the + * block device (the <source file='...'/> sub-element, such as + * "/path/to/image"), or the device target shorthand (the + * <target dev='...'/> sub-element, such as "vda"). Valid names + * can be found by calling virDomainGetXMLDesc() and inspecting + * elements within //domain/devices/disk. + * + * The @params and @nparams arguments can be used to set hypervisor-specific + * tuning parameters, such as maximum bandwidth or granularity. For a + * parameter that the hypervisor understands, explicitly specifying 0 + * behaves the same as omitting the parameter, to use the hypervisor + * default; however, omitting a parameter is less likely to fail. + * + * This command is a superset of the older virDomainBlockRebase() when used + * with the VIR_DOMAIN_BLOCK_REBASE_COPY flag, and offers better control + * over the destination format, the ability to copy to a destination that + * is not a local file, and the possibility of additional tuning parameters. + * + * Returns 0 if the operation has started, -1 on failure. + */ +int +virDomainBlockCopy(virDomainPtr dom, const char *disk, + const char *destxml, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(dom, + "disk=%s, destxml=%s, params=%p, nparams=%d, flags=%x", + disk, destxml, params, nparams, flags); + VIR_TYPED_PARAMS_DEBUG(params, nparams); + + virResetLastError(); + + virCheckDomainReturn(dom, -1); + conn = dom->conn; + + virCheckReadOnlyGoto(conn->flags, error); + virCheckNonNullArgGoto(disk, error); + virCheckNonNullArgGoto(destxml, error); + virCheckNonNegativeArgGoto(nparams, error); + if (nparams) + virCheckNonNullArgGoto(params, error); + + if (conn->driver->domainBlockCopy) { + int ret; + ret = conn->driver->domainBlockCopy(dom, disk, destxml, + params, nparams, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(dom->conn); + return -1; +} + + +/** * virDomainBlockCommit: * @dom: pointer to domain object * @disk: path to the block device, or device shorthand diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 9f4016a..c3f3bd0 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -670,4 +670,9 @@ LIBVIRT_1.2.7 { virConnectGetDomainCapabilities; } LIBVIRT_1.2.6; +LIBVIRT_1.2.8 { + global: + virDomainBlockCopy; +} LIBVIRT_1.2.7; + # .... define new API here using predicted next version number .... -- 1.9.3

Expose the new power of virDomainBlockCopy through virsh. Continue to use the older API where possible, for maximum compatibility. The command now requires either --dest (with optional --format), to directly describe the file destination, or --xml, to name a file that contains an XML description such as: <disk type='network'> <driver type='raw'/> <source protocol='gluster' name='vol1/img'> <host name='red'/> </source> </disk> Non-zero option parameters are converted into virTypedParameters, and if anything requires the new API, the command can synthesize appropriate XML even if the --dest option was used instead of --xml. The existing --raw flag remains for back-compat, but the preferred spelling is now --format=raw, since the new API now allows us to specify all formats rather than just a boolean raw to suppress probing. I hope I did justice in describing the effects of granularity and buf-size on how they get passed through to qemu. * tools/virsh-domain.c (cmdBlockCopy): Add new options --xml, --granularity, --buf-size, --format. Make --raw an alias for --format=raw. Call new API if new parameters are in use. * tools/virsh.pod (blockcopy): Document new options. Signed-off-by: Eric Blake <eblake@redhat.com> --- tools/virsh-domain.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++----- tools/virsh.pod | 35 +++++++++------ 2 files changed, 134 insertions(+), 25 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index fb9c009..a42858a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1793,11 +1793,10 @@ static const vshCmdOptDef opts_block_copy[] = { {.name = "path", .type = VSH_OT_DATA, .flags = VSH_OFLAG_REQ, - .help = N_("fully-qualified path of disk") + .help = N_("fully-qualified path of source disk") }, {.name = "dest", .type = VSH_OT_DATA, - .flags = VSH_OFLAG_REQ, .help = N_("path of the copy to create") }, {.name = "bandwidth", @@ -1813,8 +1812,8 @@ static const vshCmdOptDef opts_block_copy[] = { .help = N_("reuse existing destination") }, {.name = "raw", - .type = VSH_OT_BOOL, - .help = N_("use raw destination file") + .type = VSH_OT_ALIAS, + .help = "format=raw" }, {.name = "wait", .type = VSH_OT_BOOL, @@ -1840,6 +1839,22 @@ static const vshCmdOptDef opts_block_copy[] = { .type = VSH_OT_BOOL, .help = N_("with --wait, don't wait for cancel to finish") }, + {.name = "xml", + .type = VSH_OT_DATA, + .help = N_("filename containing XML description of the copy destination") + }, + {.name = "format", + .type = VSH_OT_DATA, + .help = N_("format of the destination file") + }, + {.name = "granularity", + .type = VSH_OT_INT, + .help = N_("power-of-two granularity to use during the copy") + }, + {.name = "buf-size", + .type = VSH_OT_INT, + .help = N_("maximum amount of in-flight data during the copy") + }, {.name = NULL} }; @@ -1847,9 +1862,12 @@ static bool cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; - const char *dest; + const char *dest = NULL; + const char *format = NULL; unsigned long bandwidth = 0; - unsigned int flags = VIR_DOMAIN_BLOCK_REBASE_COPY; + unsigned int granularity = 0; + unsigned int buf_size = 0; + unsigned int flags = 0; bool ret = false; bool blocking = vshCommandOptBool(cmd, "wait"); bool verbose = vshCommandOptBool(cmd, "verbose"); @@ -1864,6 +1882,22 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) const char *path = NULL; bool quit = false; int abort_flags = 0; + const char *xml = NULL; + char *xmlstr = NULL; + virTypedParameterPtr params = NULL; + int nparams = 0; + + if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) + return false; + if (vshCommandOptString(cmd, "dest", &dest) < 0) + goto cleanup; + if (vshCommandOptString(cmd, "xml", &xml) < 0) + return false; + if (vshCommandOptString(cmd, "format", &format) < 0) + return false; + + VSH_EXCLUSIVE_OPTIONS_VAR(dest, xml); + VSH_EXCLUSIVE_OPTIONS_VAR(format, xml); if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) return false; @@ -1901,18 +1935,82 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) vshError(ctl, "%s", _("bandwidth must be a number")); goto cleanup; } + if (vshCommandOptUIntWrap(cmd, "granularity", &granularity) < 0) { + vshError(ctl, "%s", _("granularity must be a number")); + goto cleanup; + } + if (vshCommandOptUIntWrap(cmd, "buf-size", &buf_size) < 0) { + vshError(ctl, "%s", _("buf-size must be a number")); + goto cleanup; + } + if (xml) { + if (virFileReadAll(xml, 8192, &xmlstr) < 0) { + vshReportError(ctl); + goto cleanup; + } + } else if (!dest) { + vshError(ctl, "%s", _("need either --dest or --xml")); + goto cleanup; + } + + /* Exploit that VIR_DOMAIN_BLOCK_REBASE_* and + * VIR_DOMAIN_BLOCK_COPY_* flags have the same values. */ if (vshCommandOptBool(cmd, "shallow")) flags |= VIR_DOMAIN_BLOCK_REBASE_SHALLOW; if (vshCommandOptBool(cmd, "reuse-external")) flags |= VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT; - if (vshCommandOptBool(cmd, "raw")) - flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_RAW; - if (vshCommandOptStringReq(ctl, cmd, "dest", &dest) < 0) - goto cleanup; - if (virDomainBlockRebase(dom, path, dest, bandwidth, flags) < 0) - goto cleanup; + if (granularity || buf_size || (format && STRNEQ(format, "raw")) || xml) { + /* New API */ + if (bandwidth || granularity || buf_size) { + params = vshCalloc(ctl, 3, sizeof(*params)); + /* bandwidth is ulong, but the typed parameter is ullong; + * use addition to ensure correct vararg typing */ + if (bandwidth && + virTypedParameterAssign(¶ms[nparams++], + VIR_DOMAIN_BLOCK_COPY_BANDWIDTH, + VIR_TYPED_PARAM_ULLONG, + bandwidth + 0ULL) < 0) + goto cleanup; + if (granularity && + virTypedParameterAssign(¶ms[nparams++], + VIR_DOMAIN_BLOCK_COPY_GRANULARITY, + VIR_TYPED_PARAM_UINT, + granularity) < 0) + goto cleanup; + if (buf_size && + virTypedParameterAssign(¶ms[nparams++], + VIR_DOMAIN_BLOCK_COPY_BUF_SIZE, + VIR_TYPED_PARAM_UINT, + buf_size) < 0) + goto cleanup; + } + + if (!xmlstr) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + virBufferAddLit(&buf, "<disk type='file'>\n"); + virBufferAdjustIndent(&buf, 2); + virBufferEscapeString(&buf, "<source file='%s'/>\n", dest); + virBufferEscapeString(&buf, "<driver type='%s'/>\n", format); + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</disk>\n"); + if (virBufferCheckError(&buf) < 0) + goto cleanup; + xmlstr = virBufferContentAndReset(&buf); + } + + if (virDomainBlockCopy(dom, path, xmlstr, params, nparams, flags) < 0) + goto cleanup; + } else { + /* Old API */ + flags |= VIR_DOMAIN_BLOCK_REBASE_COPY; + if (STREQ_NULLABLE(format, "raw")) + flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_RAW; + + if (virDomainBlockRebase(dom, path, dest, bandwidth, flags) < 0) + goto cleanup; + } if (!blocking) { vshPrint(ctl, "%s", _("Block Copy started")); @@ -1983,6 +2081,8 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: + VIR_FREE(xmlstr); + virTypedParamsFree(params, nparams); if (dom) virDomainFree(dom); if (blocking) diff --git a/tools/virsh.pod b/tools/virsh.pod index e0dfd13..32c5ff9 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -873,27 +873,29 @@ value is interpreted as an unsigned long long value or essentially unlimited. The hypervisor can choose whether to reject the value or convert it to the maximum value allowed. -=item B<blockcopy> I<domain> I<path> I<dest> [I<bandwidth>] [I<--shallow>] -[I<--reuse-external>] [I<--raw>] [I<--wait> [I<--async>] [I<--verbose>]] +=item B<blockcopy> I<domain> I<path> { I<dest> [I<format>] | I<xml> } +[I<--shallow>] [I<--reuse-external>] [I<--wait> [I<--async>] [I<--verbose>]] [{I<--pivot> | I<--finish>}] [I<--timeout> B<seconds>] +[I<bandwidth>] [I<granularity>] [I<buf-size>] -Copy a disk backing image chain to I<dest>. By default, this command -flattens the entire chain; but if I<--shallow> is specified, the copy -shares the backing chain. +Copy a disk backing image chain to a destination. Either I<dest> as +the destination file name, or I<xml> as the name of an XML file containing +a top-level <disk> element describing the destination, must be present. +Additionally, if I<dest> is given, I<format> should be specified to declare +the format of the destination (if I<format> is omitted, then libvirt +will reuse the format of the source, or with I<--reuse-external> will +be forced to probe the destination format, which could be a potential +security hole). The command supports I<--raw> as a boolean flag synonym for +I<--format=raw>. By default, this command flattens the entire chain; but +if I<--shallow> is specified, the copy shares the backing chain. -If I<--reuse-external> is specified, then I<dest> must exist and have +If I<--reuse-external> is specified, then the destination must exist and have sufficient space to hold the copy. If I<--shallow> is used in conjunction with I<--reuse-external> then the pre-created image must have guest visible contents identical to guest visible contents of the backing file of the original image. This may be used to modify the backing file names on the destination. -The format of the destination is determined by the first match in the -following list: if I<--raw> is specified, it will be raw; if -I<--reuse-external> is specified, the existing destination is probed -for a format; and in all other cases, the destination format will -match the source format. - By default, the copy job runs in the background, and consists of two phases. Initially, the job must copy all data from the source, and during this phase, the job can only be canceled to revert back to the @@ -917,7 +919,14 @@ I<path> specifies fully-qualified path of the disk. I<bandwidth> specifies copying bandwidth limit in MiB/s. Specifying a negative value is interpreted as an unsigned long long value or essentially unlimited. The hypervisor can choose whether to reject the value or -convert it to the maximum value allowed. +convert it to the maximum value allowed. Specifying I<granularity> +allows fine-tuning of the granularity that will be copied when a dirty +page is detected; larger values trigger less I/O overhead but may end +up copying more data overall (the default value is usually correct); +this value must be a power of two. Specifying I<buf-size> will control +how much data can be simultaneously in-flight during the copy; larger +values use more memory but may allow faster completion (the default +value is usually correct). =item B<blockpull> I<domain> I<path> [I<bandwidth>] [I<base>] [I<--wait> [I<--verbose>] [I<--timeout> B<seconds>] [I<--async>]] -- 1.9.3

The new blockcopy API wants to reuse only a subset of the disk hotplug parser - namely, we only care about the embedded virStorageSourcePtr inside a <disk> XML. Strange as it may seem, it was easier to just parse an entire disk definition, then throw away everything but the embedded source, than it was to disentangle the source parsing code from the rest of the overall disk parsing function. All that I needed was a couple of tweaks and a new internal flag that determines whether the normally-mandatory target element can be gracefully skipped. While adding the new flag, I noticed we had a verify() that was incomplete after several recent internal flag additions; move that up higher in the code to make it harder to forget to modify on the next flag addition. * src/conf/domain_conf.h (virDomainDiskSourceParse): New prototype. * src/conf/domain_conf.c (VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE): New flag. (virDomainDiskDefParseXML): Honor flag to make target optional. (virDomainDiskSourceParse): New function. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 147 +++++++++++++++++++++++++++++++---------------- src/conf/domain_conf.h | 4 ++ src/libvirt_private.syms | 1 + 3 files changed, 103 insertions(+), 49 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dd512ca..2ee2af0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -89,19 +89,36 @@ struct _virDomainXMLOption { /* Private flags used internally by virDomainSaveStatus and - * virDomainLoadStatus. */ + * virDomainLoadStatus, in addition to the public virDomainXMLFlags. */ typedef enum { /* dump internal domain status information */ - VIR_DOMAIN_XML_INTERNAL_STATUS = (1<<16), + VIR_DOMAIN_XML_INTERNAL_STATUS = 1 << 16, /* dump/parse <actual> element */ - VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET = (1<<17), + VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET = 1 << 17, /* dump/parse original states of host PCI device */ - VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES = (1<<18), - VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM = (1<<19), - VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT = (1<<20), - VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST = (1<<21), + VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES = 1 << 18, + VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM = 1 << 19, + VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT = 1 << 20, + VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST = 1 << 21, + /* parse only source half of <disk> */ + VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE = 1 << 22, } virDomainXMLInternalFlags; +#define DUMPXML_FLAGS \ + (VIR_DOMAIN_XML_SECURE | \ + VIR_DOMAIN_XML_INACTIVE | \ + VIR_DOMAIN_XML_UPDATE_CPU | \ + VIR_DOMAIN_XML_MIGRATABLE) + +verify(((VIR_DOMAIN_XML_INTERNAL_STATUS | + VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | + VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES | + VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM | + VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT | + VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST | + VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE) + & DUMPXML_FLAGS) == 0); + VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST, "custom-argv", "custom-monitor", @@ -5818,9 +5835,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, * to indicate no media present. LUN is for raw access CD-ROMs * that are not attached to a physical device presently */ if (source == NULL && def->src->hosts == NULL && !def->src->srcpool && - def->device != VIR_DOMAIN_DISK_DEVICE_CDROM && - def->device != VIR_DOMAIN_DISK_DEVICE_LUN && - def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) { + (def->device == VIR_DOMAIN_DISK_DEVICE_DISK || + (flags & VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE))) { virReportError(VIR_ERR_NO_SOURCE, target ? "%s" : NULL, target); goto error; @@ -5840,7 +5856,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, ctxt->node = saved_node; } - if (target == NULL) { + if (target == NULL && !(flags & VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE)) { if (def->src->srcpool) { char *tmp; if (virAsprintf(&tmp, "pool = '%s', volume = '%s'", @@ -5855,27 +5871,29 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && - !STRPREFIX(target, "fd")) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid floppy device name: %s"), target); - goto error; - } + if (!(flags & VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE)) { + if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && + !STRPREFIX(target, "fd")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid floppy device name: %s"), target); + goto error; + } - /* Force CDROM to be listed as read only */ - if (def->device == VIR_DOMAIN_DISK_DEVICE_CDROM) - def->src->readonly = true; + /* Force CDROM to be listed as read only */ + if (def->device == VIR_DOMAIN_DISK_DEVICE_CDROM) + def->src->readonly = true; - if ((def->device == VIR_DOMAIN_DISK_DEVICE_DISK || - def->device == VIR_DOMAIN_DISK_DEVICE_LUN) && - !STRPREFIX((const char *)target, "hd") && - !STRPREFIX((const char *)target, "sd") && - !STRPREFIX((const char *)target, "vd") && - !STRPREFIX((const char *)target, "xvd") && - !STRPREFIX((const char *)target, "ubd")) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid harddisk device name: %s"), target); - goto error; + if ((def->device == VIR_DOMAIN_DISK_DEVICE_DISK || + def->device == VIR_DOMAIN_DISK_DEVICE_LUN) && + !STRPREFIX((const char *)target, "hd") && + !STRPREFIX((const char *)target, "sd") && + !STRPREFIX((const char *)target, "vd") && + !STRPREFIX((const char *)target, "xvd") && + !STRPREFIX((const char *)target, "ubd")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid harddisk device name: %s"), target); + goto error; + } } if (snapshot) { @@ -5929,7 +5947,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } else { if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) { def->bus = VIR_DOMAIN_DISK_BUS_FDC; - } else { + } else if (!(flags & VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE)) { if (STRPREFIX(target, "hd")) def->bus = VIR_DOMAIN_DISK_BUS_IDE; else if (STRPREFIX(target, "sd")) @@ -6155,12 +6173,14 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } } - if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE - && virDomainDiskDefAssignAddress(xmlopt, def) < 0) - goto error; + if (!(flags & VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE)) { + if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE + && virDomainDiskDefAssignAddress(xmlopt, def) < 0) + goto error; - if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0) - goto error; + if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0) + goto error; + } cleanup: VIR_FREE(bus); @@ -10359,7 +10379,7 @@ virDomainDeviceDefParse(const char *xmlStr, } /* callback to fill driver specific device aspects */ - if (virDomainDeviceDefPostParse(dev, def, caps, xmlopt) < 0) + if (virDomainDeviceDefPostParse(dev, def, caps, xmlopt) < 0) goto error; cleanup: @@ -10373,6 +10393,47 @@ virDomainDeviceDefParse(const char *xmlStr, } +virStorageSourcePtr +virDomainDiskDefSourceParse(const char *xmlStr, + const virDomainDef *def, + virDomainXMLOptionPtr xmlopt, + unsigned int flags) +{ + xmlDocPtr xml; + xmlNodePtr node; + xmlXPathContextPtr ctxt = NULL; + virDomainDiskDefPtr disk = NULL; + virStorageSourcePtr ret = NULL; + + if (!(xml = virXMLParseStringCtxt(xmlStr, _("(disk_definition)"), &ctxt))) + goto cleanup; + node = ctxt->node; + + if (!xmlStrEqual(node->name, BAD_CAST "disk")) { + virReportError(VIR_ERR_XML_ERROR, + _("expecting root element of 'disk', not '%s'"), + node->name); + goto cleanup; + } + + flags |= VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE; + if (!(disk = virDomainDiskDefParseXML(xmlopt, node, ctxt, + NULL, def->seclabels, + def->nseclabels, + flags))) + goto cleanup; + + ret = disk->src; + disk->src = NULL; + + cleanup: + virDomainDiskDefFree(disk); + xmlFreeDoc(xml); + xmlXPathFreeContext(ctxt); + return ret; +} + + static const char * virDomainChrTargetTypeToString(int deviceType, int targetType) @@ -17726,18 +17787,6 @@ virDomainHugepagesFormat(virBufferPtr buf, } -#define DUMPXML_FLAGS \ - (VIR_DOMAIN_XML_SECURE | \ - VIR_DOMAIN_XML_INACTIVE | \ - VIR_DOMAIN_XML_UPDATE_CPU | \ - VIR_DOMAIN_XML_MIGRATABLE) - -verify(((VIR_DOMAIN_XML_INTERNAL_STATUS | - VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | - VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES | - VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST) - & DUMPXML_FLAGS) == 0); - static bool virDomainDefHasCapabilitiesFeatures(virDomainDefPtr def) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index aead903..512d097 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2294,6 +2294,10 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(const char *xmlStr, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, unsigned int flags); +virStorageSourcePtr virDomainDiskDefSourceParse(const char *xmlStr, + const virDomainDef *def, + virDomainXMLOptionPtr xmlopt, + unsigned int flags); virDomainDefPtr virDomainDefParseString(const char *xmlStr, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6b9ee21..1195208 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -222,6 +222,7 @@ virDomainDiskDefAssignAddress; virDomainDiskDefForeachPath; virDomainDiskDefFree; virDomainDiskDefNew; +virDomainDiskDefSourceParse; virDomainDiskDeviceTypeToString; virDomainDiskDiscardTypeToString; virDomainDiskErrorPolicyTypeFromString; -- 1.9.3

Upstream qemu 1.4 added some drive-mirror tunables not present when it was first introduced in 1.3. Management apps may want to set these in some cases (for example, without tuning granularity down to sector size, a copy may end up occupying more bytes than the original because an entire cluster is copied even when only a sector within the cluster is dirty, although tuning it down results in more CPU time to do the copy). I haven't personally needed to use the parameters, but since they exist, and since the new API supports virTypedParams, we might as well expose them. Since the tuning parameters aren't often used, and omitted from the QMP command when unspecified, I think it is safe to rely on qemu 1.3 to issue an error about them being unsupported, rather than trying to create a new capability bit in libvirt. Meanwhile, all versions of qemu from 1.4 to 2.1 have a bug where a bad granularity (such as non-power-of-2) gives a poor message: error: internal error: unable to execute QEMU command 'drive-mirror': Invalid parameter 'drive-virtio-disk0' because of abuse of QERR_INVALID_PARAMETER (which is supposed to name the parameter that was given a bad value, rather than the value passed to some other parameter). I don't see that a capability check will help, so we'll just live with it (and I'll send a patch to get qemu 2.2 to give a nicer message). * src/qemu/qemu_monitor.h (qemuMonitorDriveMirror): Add parameters. * src/qemu/qemu_monitor.c (qemuMonitorDriveMirror): Likewise. * src/qemu/qemu_monitor_json.h (qemuMonitorJSONDriveMirror): Likewise. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONDriveMirror): Likewise. * src/qemu/qemu_driver.c (qemuDomainBlockCopyCommon): Likewise. (qemuDomainBlockRebase, qemuDomainBlockCopy): Adjust callers. * src/qemu/qemu_migration.c (qemuMigrationDriveMirror): Likewise. * tests/qemumonitorjsontest.c (qemuMonitorJSONDriveMirror): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- Maybe I need to tweak libvirt's handler to report a nicer message if qemu reports an invalid parameter, since playing the qemu message verbatim isn't very informative. src/qemu/qemu_driver.c | 18 +++++------------- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_monitor.c | 8 +++++--- src/qemu/qemu_monitor.h | 2 ++ src/qemu/qemu_monitor_json.c | 3 +++ src/qemu/qemu_monitor_json.h | 2 ++ tests/qemumonitorjsontest.c | 2 +- 7 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4876617..d43d257 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15281,6 +15281,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr *vmptr, const char *path, virStorageSourcePtr dest, unsigned long bandwidth, + int granularity, + int buf_size, unsigned int flags) { virDomainObjPtr vm = *vmptr; @@ -15421,7 +15423,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr *vmptr, /* Actually start the mirroring */ qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorDriveMirror(priv->mon, device, dest->path, format, - bandwidth, flags); + bandwidth, granularity, buf_size, flags); virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0); qemuDomainObjExitMonitor(driver, vm); if (ret < 0) { @@ -15498,7 +15500,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, * and VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT map to the same value * as for block copy. */ ret = qemuDomainBlockCopyCommon(&vm, dom->conn, path, dest, - bandwidth, flags); + bandwidth, 0, 0, flags); cleanup: if (vm) @@ -15561,23 +15563,13 @@ qemuDomainBlockCopy(virDomainPtr dom, const char *disk, const char *destxml, buf_size = params[i].value.ui; } } - if (granularity) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("granularity tuning not supported yet")); - goto cleanup; - } - if (buf_size) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("buffer size tuning not supported yet")); - goto cleanup; - } if (!(dest = virDomainDiskDefSourceParse(destxml, vm->def, driver->xmlopt, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; ret = qemuDomainBlockCopyCommon(&vm, dom->conn, disk, dest, - bandwidth, flags); + bandwidth, granularity, buf_size, flags); cleanup: virStorageSourceFree(dest); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9cfb77e..111f6af 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1279,7 +1279,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) goto error; mon_ret = qemuMonitorDriveMirror(priv->mon, diskAlias, nbd_dest, - NULL, speed, mirror_flags); + NULL, speed, 0, 0, mirror_flags); qemuDomainObjExitMonitor(driver, vm); if (mon_ret < 0) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index d5ba08d..0332091 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3182,14 +3182,16 @@ int qemuMonitorDriveMirror(qemuMonitorPtr mon, const char *device, const char *file, const char *format, unsigned long bandwidth, + unsigned int granularity, unsigned int buf_size, unsigned int flags) { int ret = -1; unsigned long long speed; VIR_DEBUG("mon=%p, device=%s, file=%s, format=%s, bandwidth=%ld, " - "flags=%x", - mon, device, file, NULLSTR(format), bandwidth, flags); + "granularity=%#x, buf_size=%#x, flags=%x", + mon, device, file, NULLSTR(format), bandwidth, granularity, + buf_size, flags); /* Convert bandwidth MiB to bytes - unfortunately the JSON QMP protocol is * limited to LLONG_MAX also for unsigned values */ @@ -3204,7 +3206,7 @@ qemuMonitorDriveMirror(qemuMonitorPtr mon, if (mon->json) ret = qemuMonitorJSONDriveMirror(mon, device, file, format, speed, - flags); + granularity, buf_size, flags); else virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("drive-mirror requires JSON monitor")); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 4fd6f01..9da7ee4 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -650,6 +650,8 @@ int qemuMonitorDriveMirror(qemuMonitorPtr mon, const char *file, const char *format, unsigned long bandwidth, + unsigned int granularity, + unsigned int buf_size, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int qemuMonitorDrivePivot(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 62e7d5d..dcbb693 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3397,6 +3397,7 @@ int qemuMonitorJSONDriveMirror(qemuMonitorPtr mon, const char *device, const char *file, const char *format, unsigned long long speed, + unsigned int granularity, unsigned int buf_size, unsigned int flags) { int ret = -1; @@ -3409,6 +3410,8 @@ qemuMonitorJSONDriveMirror(qemuMonitorPtr mon, "s:device", device, "s:target", file, "U:speed", speed, + "z:granularity", granularity, + "z:buf-size", buf_size, "s:sync", shallow ? "top" : "full", "s:mode", reuse ? "existing" : "absolute-paths", "S:format", format, diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index d8c9308..cd331db 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -249,6 +249,8 @@ int qemuMonitorJSONDriveMirror(qemuMonitorPtr mon, const char *file, const char *format, unsigned long long speed, + unsigned int granularity, + unsigned int buf_size, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index e3fb4f7..afbf13a 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1176,7 +1176,7 @@ GEN_TEST_FUNC(qemuMonitorJSONRemoveNetdev, "net0") GEN_TEST_FUNC(qemuMonitorJSONDelDevice, "ide0") GEN_TEST_FUNC(qemuMonitorJSONAddDevice, "some_dummy_devicestr") GEN_TEST_FUNC(qemuMonitorJSONSetDrivePassphrase, "vda", "secret_passhprase") -GEN_TEST_FUNC(qemuMonitorJSONDriveMirror, "vdb", "/foo/bar", NULL, 1024, +GEN_TEST_FUNC(qemuMonitorJSONDriveMirror, "vdb", "/foo/bar", NULL, 1024, 0, 0, VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT) GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", "/foo/bar1", "/foo/bar2", NULL, 1024) GEN_TEST_FUNC(qemuMonitorJSONDrivePivot, "vdb", NULL, NULL) -- 1.9.3