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