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

[John Snow]

On 02/16/2015 03:22 PM, Eric Blake wrote:
> 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.

qmp_drive_mirror DOES clamp the user-specified value to [512, 64M]. It 
does also allow '0', which gets passed through to mirror_start_job, 
which chooses a default based on the cluster_size, clamped to [512, 64K].

> > 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?

Not a bug as such, though not a purposeful decision on my part either.

I don't have a concrete reason to prohibit what kinds of granularities 
the user can supply, but when it came to picking a default I decided to 
imitate what mirror was already doing, which I explain above.

The existing code is basically: "Try to pick a sane default if the user 
didn't give us one, but allow the user to choose something bananas if 
they want to. Maybe they know better than we do."