Re: [Qemu-devel] [PATCH 08/20] mirror: allow customizing the granularity

[Eric Blake]

On 12/12/2012 06:46 AM, Paolo Bonzini wrote:
> The desired granularity may be very different depending on the kind of
> operation (e.g. continuous replication vs. collapse-to-raw) and whether
> the VM is expected to perform lots of I/O while mirroring is in progress.
> 
> Allow the user to customize it, while providing a sane default so that
> in general there will be no extra allocated space in the target compared
> to the source.

Might be worth mentioning that this configuration still has to be done
prior to starting the job (ie. it's a one-time initialization, not
something that is run-time tweakable).

> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  block/mirror.c   | 50 ++++++++++++++++++++++++++++++++------------------
>  block_int.h      |  3 ++-
>  blockdev.c       | 15 ++++++++++++++-
>  hmp.c            |  2 +-
>  qapi-schema.json |  8 +++++++-
>  qmp-commands.hx  |  8 +++++++-
>  6 files changed, 63 insertions(+), 23 deletions(-)

> @@ -330,7 +329,7 @@ static BlockJobType mirror_job_type = {
>  };
>  
>  void mirror_start(BlockDriverState *bs, BlockDriverState *target,
> -                  int64_t speed, MirrorSyncMode mode,
> +                  int64_t speed, int64_t granularity, MirrorSyncMode mode,
>                    BlockdevOnError on_source_error,
>                    BlockdevOnError on_target_error,
>                    BlockDriverCompletionFunc *cb,
> @@ -338,6 +337,20 @@ void mirror_start(BlockDriverState *bs, BlockDriverState 
> *target,
>  {
>      MirrorBlockJob *s;
>  
> +    if (granularity == 0) {
> +        /* Choose the default granularity based on the target file's cluster
> +         * size, clamped between 4k and 64k.  */
> +        BlockDriverInfo bdi;
> +        if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != 0) {
> +            granularity = MAX(4096, bdi.cluster_size);
> +            granularity = MIN(65536, granularity);

You clamp granularity to sane bounds when defaulting...

> +        } else {
> +            granularity = 65536;
> +        }
> +    }
> +
> +    assert((granularity & (granularity - 1)) == 0);

...but don't do any checking other than power-of-two on user input.  Can
the user request a granularity that makes no sense, such as something
less than 512 or huge like 1G?  Or for that matter, is it even a problem
if the user requests a granularity larger than the target bdi.cluster_size?

> @@ -1217,6 +1218,17 @@ void qmp_drive_mirror(const char *device, const char 
> *target,
>      if (!has_mode) {
>          mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
>      }
> +    if (!has_granularity) {
> +        granularity = 0;
> +    }
> +    if (granularity != 0 && (granularity < 512 || granularity > 1048576 * 
> 64)) {

That answers part of my question - you clamp between 512 and 64M, which
is a much wider range than the defaults you end up choosing.

> +++ b/qapi-schema.json
> @@ -1636,6 +1636,11 @@
>  #        (all the disk, only the sectors allocated in the topmost image, or
>  #        only new I/O).
>  #
> +# @granularity: #optional granularity of the dirty bitmap, default is 64K
> +#               if the image format doesn't have clusters, 4K if the clusters
> +#               are smaller than that, else the cluster size.  Must be a
> +#               power of 2 between 512 and 64M.

Maybe mention that this attribute was added in 1.4.  (Hmm, now I have to
decide how to expose this attribute via libvirt.)

> @@ -971,6 +973,10 @@ Arguments:
>  - "on-target-error": the action to take on an error on the target
>    (BlockdevOnError, default 'report')
>  
> +The default value of the granularity is, if the image format defines
> +a cluster size, the cluster size or 4096, whichever is larger.  If it
> +does not define a cluster size, the default value of the granularity
> +is 65536.

That doesn't quite cover the fact that you clamp granularity to 64k if
the image format has a cluster size larger than 64k.

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

signature.asc
Description: OpenPGP digital signature