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

[John Snow <jsnow@redhat.com>]

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

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