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'
>
> because of abuse of QERR_INVALID_PARAMETER (which is supposed to
> name the parameter that was given a bad value, rather than the
> value passed to some other parameter). I don't see that a
> capability check will help, so we'll just live with it (and I'll
> send a patch to get qemu 2.2 to give a nicer message).
>
> * src/qemu/qemu_monitor.h (qemuMonitorDriveMirror): Add
> parameters.
> * src/qemu/qemu_monitor.c (qemuMonitorDriveMirror): Likewise.
> * src/qemu/qemu_monitor_json.h (qemuMonitorJSONDriveMirror):
> Likewise.
> * src/qemu/qemu_monitor_json.c (qemuMonitorJSONDriveMirror):
> Likewise.
> * src/qemu/qemu_driver.c (qemuDomainBlockCopyCommon): Likewise.
> (qemuDomainBlockRebase, qemuDomainBlockCopy): Adjust callers.
> * src/qemu/qemu_migration.c (qemuMigrationDriveMirror): Likewise.
> * tests/qemumonitorjsontest.c (qemuMonitorJSONDriveMirror): Likewise.
>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
>
> 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.
>
> src/qemu/qemu_driver.c | 18 +++++-------------
> src/qemu/qemu_migration.c | 2 +-
> src/qemu/qemu_monitor.c | 8 +++++---
> src/qemu/qemu_monitor.h | 2 ++
> src/qemu/qemu_monitor_json.c | 3 +++
> src/qemu/qemu_monitor_json.h | 2 ++
> tests/qemumonitorjsontest.c | 2 +-
> 7 files changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 4876617..d43d257 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -15281,6 +15281,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr *vmptr,
> const char *path,
> virStorageSourcePtr dest,
> unsigned long bandwidth,
> + int granularity,
> + int buf_size,
int ??
> unsigned int flags)
> {
> virDomainObjPtr vm = *vmptr;
> @@ -15421,7 +15423,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr *vmptr,
> /* Actually start the mirroring */
> qemuDomainObjEnterMonitor(driver, vm);
> ret = qemuMonitorDriveMirror(priv->mon, device, dest->path, format,
> - bandwidth, flags);
> + bandwidth, granularity, buf_size, flags);
> virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0);
> qemuDomainObjExitMonitor(driver, vm);
> if (ret < 0) {
> @@ -15498,7 +15500,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base,
> * and VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT map to the same value
> * as for block copy. */
> ret = qemuDomainBlockCopyCommon(&vm, dom->conn, path, dest,
> - bandwidth, flags);
> + bandwidth, 0, 0, flags);
>
> cleanup:
> if (vm)
> @@ -15561,23 +15563,13 @@ qemuDomainBlockCopy(virDomainPtr dom, const char *disk, const char *destxml,
> buf_size = params[i].value.ui;
> }
> }
> - if (granularity) {
> - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> - _("granularity tuning not supported yet"));
> - goto cleanup;
> - }
> - if (buf_size) {
> - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> - _("buffer size tuning not supported yet"));
> - goto cleanup;
> - }
>
> if (!(dest = virDomainDiskDefSourceParse(destxml, vm->def, driver->xmlopt,
> VIR_DOMAIN_XML_INACTIVE)))
> goto cleanup;
>
> ret = qemuDomainBlockCopyCommon(&vm, dom->conn, disk, dest,
> - bandwidth, flags);
> + bandwidth, granularity, buf_size, flags);
both granularity and bufsize are unsigned here!
>
> cleanup:
> virStorageSourceFree(dest);
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 9cfb77e..111f6af 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1279,7 +1279,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
> QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
> goto error;
> mon_ret = qemuMonitorDriveMirror(priv->mon, diskAlias, nbd_dest,
> - NULL, speed, mirror_flags);
> + NULL, speed, 0, 0, mirror_flags);
> qemuDomainObjExitMonitor(driver, vm);
>
> if (mon_ret < 0)
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index d5ba08d..0332091 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3182,14 +3182,16 @@ int
> qemuMonitorDriveMirror(qemuMonitorPtr mon,
> const char *device, const char *file,
> const char *format, unsigned long bandwidth,
> + unsigned int granularity, unsigned int buf_size,
> unsigned int flags)
> {
> int ret = -1;
> unsigned long long speed;
>
> VIR_DEBUG("mon=%p, device=%s, file=%s, format=%s, bandwidth=%ld, "
> - "flags=%x",
> - mon, device, file, NULLSTR(format), bandwidth, flags);
> + "granularity=%#x, buf_size=%#x, flags=%x",
Clever way to check the power-of-2-ness, although bufsize would be beter
off with %u.
> + mon, device, file, NULLSTR(format), bandwidth, granularity,
> + buf_size, flags);
>
> /* Convert bandwidth MiB to bytes - unfortunately the JSON QMP protocol is
> * limited to LLONG_MAX also for unsigned values */
> @@ -3204,7 +3206,7 @@ qemuMonitorDriveMirror(qemuMonitorPtr mon,
>
> if (mon->json)
> ret = qemuMonitorJSONDriveMirror(mon, device, file, format, speed,
> - flags);
> + granularity, buf_size, flags);
> else
> virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> _("drive-mirror requires JSON monitor"));
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 4fd6f01..9da7ee4 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -650,6 +650,8 @@ int qemuMonitorDriveMirror(qemuMonitorPtr mon,
> const char *file,
> const char *format,
> unsigned long bandwidth,
> + unsigned int granularity,
> + unsigned int buf_size,
> unsigned int flags)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
> int qemuMonitorDrivePivot(qemuMonitorPtr mon,
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 62e7d5d..dcbb693 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -3397,6 +3397,7 @@ int
> qemuMonitorJSONDriveMirror(qemuMonitorPtr mon,
> const char *device, const char *file,
> const char *format, unsigned long long speed,
> + unsigned int granularity, unsigned int buf_size,
> unsigned int flags)
> {
> int ret = -1;
> @@ -3409,6 +3410,8 @@ qemuMonitorJSONDriveMirror(qemuMonitorPtr mon,
> "s:device", device,
> "s:target", file,
> "U:speed", speed,
> + "z:granularity", granularity,
> + "z:buf-size", buf_size,
z is for signed values. both are unsigned. I think you wanted to use a
'p' formatter.
> "s:sync", shallow ? "top" : "full",
> "s:mode", reuse ? "existing" : "absolute-paths",
> "S:format", format,
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index d8c9308..cd331db 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -249,6 +249,8 @@ int qemuMonitorJSONDriveMirror(qemuMonitorPtr mon,
> const char *file,
> const char *format,
> unsigned long long speed,
> + unsigned int granularity,
> + unsigned int buf_size,
> unsigned int flags)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
> int qemuMonitorJSONDrivePivot(qemuMonitorPtr mon,
> diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> index e3fb4f7..afbf13a 100644
> --- a/tests/qemumonitorjsontest.c
> +++ b/tests/qemumonitorjsontest.c
> @@ -1176,7 +1176,7 @@ GEN_TEST_FUNC(qemuMonitorJSONRemoveNetdev, "net0")
> GEN_TEST_FUNC(qemuMonitorJSONDelDevice, "ide0")
> GEN_TEST_FUNC(qemuMonitorJSONAddDevice, "some_dummy_devicestr")
> GEN_TEST_FUNC(qemuMonitorJSONSetDrivePassphrase, "vda", "secret_passhprase")
> -GEN_TEST_FUNC(qemuMonitorJSONDriveMirror, "vdb", "/foo/bar", NULL, 1024,
> +GEN_TEST_FUNC(qemuMonitorJSONDriveMirror, "vdb", "/foo/bar", NULL, 1024, 0, 0,
> VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)
> GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", "/foo/bar1", "/foo/bar2", NULL, 1024)
> GEN_TEST_FUNC(qemuMonitorJSONDrivePivot, "vdb", NULL, NULL)
>
There are a few singedness problems in this patch. Although trivial
enough to grant an
ACK if you fix the issues above.
Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list