On 08/24/14 05:32, Eric Blake wrote: > 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 > will have to do overflow detection on 32-bit platforms. > > * 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@xxxxxxxxxx> > --- > include/libvirt/libvirt.h.in | 57 +++++++++++++++++++-- > src/driver.h | 11 +++- > src/libvirt.c | 118 ++++++++++++++++++++++++++++++++++++++++++- > src/libvirt_public.syms | 5 ++ > 4 files changed, 184 insertions(+), 7 deletions(-) > > diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in > index 47ea695..89c8e63 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,53 @@ 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 MiB/s and ullong? thats an awful lot of speed. Shouldn't we go with KiB/s? This might benefit slower networks. (although it may never converge there) > + * the hypervisor may restrict the set of valid values to a smaller > + * range). 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 at which the copy operation recognizes dirty pages, I'd rather say "dirty blocks". Pages might indicate RAM memory pages. > + * as an unsigned int, and must be a power of 2. > + */ > +#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 can be in flight between source and destination, as > + * an unsigned int. In bytes? > + */ > +#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 +4877,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..99f1dc1 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,118 @@ 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 @bandwidth parameter @bandwidth is now provided as a typed param. > + * affects how fast the source is pulled 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. > + * > + * 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) Wow, XML, typed params and flags. Now that's future proof! :) > +{ > + 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); > + if (params) > + virCheckPositiveArgGoto(nparams, error); > + else > + virCheckZeroArgGoto(nparams, 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; One of us will have to rebase. I've recently posted a series that adds API too :) > +} LIBVIRT_1.2.7; > + > # .... define new API here using predicted next version number .... > Apart from a few DOC problems the API looks fine to me and should be fairly future proof. ACK to the design (once docs are fixed). Peter P.S.: I've run out of time to review the rest of this, but this should be good enough to merge the rest a bit later.
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list