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 <[email protected]> > --- > 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.
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
