[libvirt] [PATCH] qemu: reduce file padding requirements

[Daniel P. Berrange]

On Wed, Jun 09, 2010 at 08:33:49PM -0600, Eric Blake wrote:
> Followup to https://bugzilla.redhat.com/show_bug.cgi?id=599091,
> commit 20206a4b, to reduce disk waste in padding.
> 
> * src/qemu/qemu_monitor.h (QEMU_MONITOR_MIGRATE_TO_FILE_BS): Drop
> back to 512.
> (QEMU_MONITOR_MIGRATE_TO_FILE_TRANSFER_SIZE): New macro.
> * src/qemu/qemu_driver.c (qemudDomainSaveFlag): Update comment.
> * src/qemu/qemu_monitor_text.c (qemuMonitorTextMigrateToFile): Use
> two invocations of dd to output non-aligned large blocks.
> * src/qemu/qemu_monitor_json.c (qemuMonitorJSONMigrateToFile):
> Likewise.
> ---
> 
> This should result in much less wasted space (but I don't have a good
> environment set up for testing migration at the moment; Laine, would
> you mind testing this patch, since you wrote the last one?).
> 
> Also, should we bump QEMU_MONITOR_MIGRATE_TO_FILE_BS to 4k, for the
> benefit of newer disks that have 4k blocks?
> 
>  src/qemu/qemu_driver.c       |    7 -------
>  src/qemu/qemu_monitor.h      |   12 ++++++++----
>  src/qemu/qemu_monitor_json.c |   13 +++++++++----
>  src/qemu/qemu_monitor_text.c |   13 +++++++++----
>  4 files changed, 26 insertions(+), 19 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index bc8dcfa..4edf5ec 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5000,13 +5000,6 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path,
>       * we need to ensure there's a 512 byte boundary. Unfortunately
>       * we don't have an explicit offset in the header, so we fake
>       * it by padding the XML string with NULLs.
> -     *
> -     * XXX: This means there will be (QEMU_MONITOR_MIGRATE_TO_FILE_BS
> -     *      - strlen(xml)) bytes of wastage in each file.
> -     *      Unfortunately, a large BS is needed for reasonable
> -     *      performance. It would be nice to find a replacement for dd
> -     *      that could specify the start offset in bytes rather than
> -     *      blocks, to eliminate this waste.
>       */
>      if (offset % QEMU_MONITOR_MIGRATE_TO_FILE_BS) {
>          unsigned long long pad =
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index e03b4df..8391ebd 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1,7 +1,7 @@
>  /*
>   * qemu_monitor.h: interaction with QEMU monitor console
>   *
> - * Copyright (C) 2006-2009 Red Hat, Inc.
> + * Copyright (C) 2006-2010 Red Hat, Inc.
>   * Copyright (C) 2006 Daniel P. Berrange
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -261,10 +261,14 @@ int qemuMonitorMigrateToCommand(qemuMonitorPtr mon,
>                                  unsigned int background,
>                                  const char * const *argv);
> 
> -/* In general, a larger BS means better domain save performance,
> - * at the expense of a larger resulting file - see qemu_driver.c
> +/* In general, BS is the smallest fundamental block size we can use to
> + * access a block device; everything must be aligned to a multiple of
> + * this.  However, operating on blocks this small is painfully slow,
> + * so we also have a transfer size that is larger but only aligned to
> + * the smaller block size.
>   */
> -# define QEMU_MONITOR_MIGRATE_TO_FILE_BS (1024llu * 1024)
> +# define QEMU_MONITOR_MIGRATE_TO_FILE_BS 512llu
> +# define QEMU_MONITOR_MIGRATE_TO_FILE_TRANSFER_SIZE (1024llu * 1024)
> 
>  int qemuMonitorMigrateToFile(qemuMonitorPtr mon,
>                               unsigned int background,
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index e89ad43..5e6cedd 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1695,10 +1695,15 @@ int qemuMonitorJSONMigrateToFile(qemuMonitorPtr mon,
>          goto cleanup;
>      }
> 
> -    if (virAsprintf(&dest, "exec:%s | dd of=%s bs=%llu seek=%llu",
> -                    argstr, safe_target,
> -                    QEMU_MONITOR_MIGRATE_TO_FILE_BS,
> -                    offset / QEMU_MONITOR_MIGRATE_TO_FILE_BS) < 0) {
> +    /* Two dd processes, sharing the same stdout, are necessary to
> +     * allow starting at an alignment of 512, but without wasting
> +     * padding to get to the larger alignment useful for speed.  */
> +    if (virAsprintf(&dest, "exec:%s | { dd bs=%llu seek=%llu if=/dev/null && "
> +                    "dd bs=%llu; } >%s",

Does this work with block devices as the target ?  We previously
switched from cat>> to dd, because it didn't work correctly with block
devs.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|