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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7671 bytes --]

On 02/09/2015 06:35 PM, John Snow wrote:
> The new command pair is added to manage user created dirty bitmap. The
> dirty bitmap's name is mandatory and must be unique for the same device,
> but different devices can have bitmaps with the same names.

I haven't been following this series closely (I know I should be doing
that, though).  Is the bitmap associated with the BDS (a host resource,
independent of which device(s) are currently viewing that content) or
with the BlockBackend (only one bitmap namespace per device)?  I'm a bit
worried that we will WANT to have bitmaps associated with BDS (if we
don't already) because of image fleecing.  That is, if we start with:

base <- mid <- active

and request an image fleecing operation, we want:

base <- mid <- active
            \- overlay

where overlay serves the NBD that sees the point in time. If we then
allow a block-commit, then writes to 'mid' containing the content from
'active' will trigger another write to 'overlay' with the pre-modified
contents, so that the NBD fleecing operation doesn't see any changes.
If we then migrate, it means we need multiple bitmaps: the map for the
commit of active into mid (how much remains to be committed), and the
map for mid to overlay (how much of mid has been changed since the
point-in-time overlay was created).

By associating bitmaps with a device (a BB), rather than a BDS, you may
be artificially limiting which operations can be performed.  On the
other hand, if you associate with a BDS, and we improve things to allow
arbitrary refactoring relationships where a BDS can be in more than one
tree at once, it starts to be hard to prove that bitmap names won't be
duplicated.

Am I overthinking something here, or are we okay limiting bitmap names
to just the BB device, rather than a BDS?

> 
> 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.
> 
> 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: Fam Zheng <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  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(-)
> 

> +++ b/blockdev.c
> @@ -1173,6 +1173,48 @@ out_aio_context:
>      return NULL;
>  }
>  
> +/**
> + * Return a dirty bitmap (if present), after validating
> + * the node reference and bitmap names. Returns NULL on error,
> + * including when the BDS and/or bitmap is not found.
> + */
> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
> +                                                  const char *name,
> +                                                  BlockDriverState **pbs,
> +                                                  Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +
> +    if (!node) {
> +        error_setg(errp, "Node cannot be NULL");
> +        return NULL;
> +    }

Node tends to be the term we use for BDS, rather than for device BB.

> +    if (!name) {
> +        error_setg(errp, "Bitmap name cannot be NULL");
> +        return NULL;
> +    }
> +
> +    bs = bdrv_lookup_bs(node, node, NULL);
> +    if (!bs) {
> +        error_setg(errp, "Node '%s' not found", node);
> +        return NULL;
> +    }
> +
> +    /* If caller provided a BDS*, provide the result of that lookup, too. */
> +    if (pbs) {
> +        *pbs = bs;
> +    }
> +
> +    bitmap = bdrv_find_dirty_bitmap(bs, name);

and this seems to say that bitmap names are per-BDS, not per-device.

> +    if (!bitmap) {
> +        error_setg(errp, "Dirty bitmap '%s' not found", name);
> +        return NULL;
> +    }
> +
> +    return bitmap;
> +}
> +
>  /* New and old BlockDriverState structs for atomic group operations */
> +++ b/qapi/block-core.json
> @@ -959,6 +959,61 @@
>              '*on-target-error': 'BlockdevOnError' } }
>  
>  ##
> +# @BlockDirtyBitmap
> +#
> +# @node: name of device/node which the bitmap is tracking
> +#
> +# @name: name of the dirty bitmap
> +#
> +# Since 2.3
> +##
> +{ 'type': 'BlockDirtyBitmap',
> +  'data': { 'node': 'str', 'name': 'str' } }

This naming implies that bitmap is a per-BDS option, but that as a
convenience, we allow a device name (BB) as shorthand for the top-level
BDS associated with the BB.  I can live with that.

> +++ b/qmp-commands.hx
> @@ -1244,6 +1244,57 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "block-dirty-bitmap-add",
> +        .args_type  = "node:B,name:s,granularity:i?",
> +        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_add,
> +    },
> +    {
> +        .name       = "block-dirty-bitmap-remove",
> +        .args_type  = "node:B,name:s",
> +        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove,
> +    },

I don't know if it is worth interleaving these declarations...

> +
> +SQMP
> +
> +block-dirty-bitmap-add
> +----------------------
> +Since 2.3
> +
> +Create a dirty bitmap with a name on the device, and start tracking the writes.
> +
> +Arguments:
> +
> +- "node": device/node on which to create dirty bitmap (json-string)
> +- "name": name of the new dirty bitmap (json-string)
> +- "granularity": granularity to track writes with (int, optional)
> +
> +Example:
> +
> +-> { "execute": "block-dirty-bitmap-add", "arguments": { "node": "drive0",
> +                                                   "name": "bitmap0" } }
> +<- { "return": {} }
> +
> +block-dirty-bitmap-remove
> +-------------------------
> +Since 2.3

...to align with their examples. I don't see much else grouping in this
file.

> +
> +Stop write tracking and remove the dirty bitmap that was created with
> +block-dirty-bitmap-add.
> +
> +Arguments:
> +
> +- "node": device/node on which to remove dirty bitmap (json-string)
> +- "name": name of the dirty bitmap to remove (json-string)
> +
> +Example:
> +
> +-> { "execute": "block-dirty-bitmap-remove", "arguments": { "node": "drive0",
> +                                                      "name": "bitmap0" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "blockdev-snapshot-sync",
>          .args_type  = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?",
>          .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,
> 

The interface seems okay; I'm not sure if there are any tweaks we need
to the commit message or documentation.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]