[libvirt] [PATCH v2 1/8] blockcopy: virDomainBlockCopy with XML destination, typed params

Tue Aug 26 14:39:51 UTC 2014

On 08/26/14 13:21, 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
> (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 at 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

I'll re-iterate here. MiB/s seems a pretty big granularity with a lot of
headroom in the big numbers (2^64 MiB/s networks won't be around for a
while) but not enough options at the small end where we have 1 MiB/s, 2
MiB/s and nothing between.

Regarding your comment virDomainBlockJobSetSpeed specifies default unit
in MiB/s:

1) this is a new API so we can choose an arbitrary unit (not that it
would be nice)

2) for virDomainBlockJobSetSpeed we can add a flag for the smaller
granularity.

3) 2^32 KiB/s is also plenty for today's standard: I'd like to have a
4TiB/s network/disk drive.

Anyways, this is bikeshedding, this API can get a new flag too.

> + * 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"

You've fixed all the problems with docs I've pointed out and I don't
have any additional comments.

ACK

Peter

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140826/76e0fbca/attachment-0001.sig>