[libvirt] [PATCH v2 8/8] blockcopy: add qemu implementation of new tunables

[Eric Blake]

On 08/26/2014 09:54 AM, Peter Krempa wrote:
> On 08/26/14 13:21, Eric Blake wrote:
>> 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'

>> 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.

This 'internal error' is too easy to trigger; for v3, I'm trying to
generate a nicer error message.

>> @@ -15281,6 +15281,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr *vmptr,
>>                            const char *path,
>>                            virStorageSourcePtr dest,
>>                            unsigned long bandwidth,
>> +                          int granularity,
>> +                          int buf_size,
> 
> int ??

I know granularity is capped (qemu chokes if it is larger than 64M), but
buf_size appears to have no limit in qemu.  Hmm, maybe that means I
should tweak the public API to have the virTypedParameter for buf_size
be unsigned long long, before we bake it in too small.  And yes, I'm
inconsistent on signed/unsigned (evidence of several iterations of this
patch).

> There are a few singedness problems in this patch. Although trivial
> enough to grant an
> 
> ACK if you fix the issues above.

At this point, I'd feel better getting a v3 reviewed.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

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