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@xxxxxxxxxx> > --- > 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list