Re: [Qemu-devel] [PATCH v13 03/17] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[Eric Blake]

On 02/13/2015 03:08 PM, John Snow wrote:
> The new command pair is added to manage user created dirty bitmap. The

s/manage/manage a/

> dirty bitmap's name is mandatory and must be unique for the same device,
> but different devices can have bitmaps with the same names.
> 
> The granularity is an optional field. If it is not specified, we will
> choose a default granularity based on the cluster size if available,
> clamped to between 4K and 64K to mirror how the 'mirror' code was
> already choosing granularity. If we do not have cluster size info
> available, we choose 64K. This code has been factored out into a helper
> shared with block/mirror.

The drive-mirror command in block-core.json documents that it supports a
granularity between 512 and 64M, not 64k.  Is there a bug here?  See
more below.

> 
> This patch also introduces the 'block_dirty_bitmap_lookup' helper,
> which takes a device name and a dirty bitmap name and validates the
> lookup, returning NULL and setting errp if there is a problem with
> either field. This helper will be re-used in future patches in this
> series.
> 
> The types added to block-core.json will be re-used in future patches
> in this series, see:
> 'qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}'
> 
> Signed-off-by: John Snow <address@hidden>
> ---
>  block.c               |  20 ++++++++++
>  block/mirror.c        |  10 +----
>  blockdev.c            | 100 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h |   1 +
>  qapi/block-core.json  |  55 +++++++++++++++++++++++++++
>  qmp-commands.hx       |  51 +++++++++++++++++++++++++
>  6 files changed, 228 insertions(+), 9 deletions(-)
>  

> +
> +    if (has_granularity) {
> +        if (granularity < 512 || !is_power_of_2(granularity)) {
> +            error_setg(errp, "Granularity must be power of 2 "
> +                             "and at least 512");
> +            goto out;
> +        }
> +    } else {
> +        /* Default to cluster size, if available: */
> +        granularity = bdrv_get_default_bitmap_granularity(bs);
> +    }

This enforces a minimum granularity, but should we also be enforcing a
maximum?  That is, if the user does not provide granularity, we default
between 4k and 64k based on cluster size, but if the user DOES provide
granularity, we allow as small as 512, but as large as the user can
pass.  Should we enforce a maximum of 64M?


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

signature.asc
Description: OpenPGP digital signature