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

[John Snow]

Missed you by several seconds, I submitted a v13 fixup to cover Max's 
comments and had wrongly assumed I wouldn't be hearing anything else at 
5PM on a Friday :)

On 02/13/2015 05:24 PM, Eric Blake wrote:
> 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

Currently: I am intending to allow users to attach them to any old BDS, 
per-node. However, they are currently only /useful/ if you attach them 
to the root, since that's what Drive Backup is going to operate on.

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

Not my intention to create an artificial limitation, just an implicit 
one: There are currently no users of named BdrvDirtyBitmaps that act 
per-node.

You may review this series under this premise and yell at me if I have 
diverged from this assumption.

> > 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 <address@hidden>
> > 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(-)
>
> > +++ 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.

So far so good.

> > +    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.

We're still on the same page.

> > +    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.

It sounds like I am not going to the mental facility after all. This is 
what I wanted.

> > +++ 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...

I can demingle them if I must; I just carried forth the convention in 
the original patches I received and nobody has mentioned it yet.

> > +
> > +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.

OK, if a v14 is needed for other reasons I will decouple the examples 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.