[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 redhat com>
> ---
> 
> 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]