Re: [Qemu-devel] [PATCH v5 09/21] block: Add bitmap successors

[Eric Blake <eblake@redhat.com>]

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

On 04/08/2015 04:19 PM, John Snow wrote:
> A bitmap successor is an anonymous BdrvDirtyBitmap that is intended to
> be created just prior to a sensitive operation (e.g. Incremental Backup)
> that can either succeed or fail, but during the course of which we still
> want a bitmap tracking writes.
> 
> On creating a successor, we "freeze" the parent bitmap which prevents
> its deletion, enabling, anonymization, or creating a bitmap with the
> same name.
> 
> On success, the parent bitmap can "abdicate" responsibility to the
> successor, which will inherit its name. The successor will have been
> tracking writes during the course of the backup operation. The parent
> will be safely deleted.
> 
> On failure, we can "reclaim" the successor from the parent, unifying
> them such that the resulting bitmap describes all writes occurring since
> the last successful backup, for instance. Reclamation will thaw the
> parent, but not explicitly re-enable it.
> 
> BdrvDirtyBitmap operations that target a single bitmap are protected
> by assertions that the bitmap is not frozen and/or disabled.
> 
> BdrvDirtyBitmap operations that target a group of bitmaps, such as
> bdrv_{set,reset}_dirty will ignore frozen/disabled drives with a
> conditional instead.
> 
> QMP transactions that enable/disable bitmaps have extra error checking
> surrounding them that prevent modifying bitmaps that are frozen.

Is this last paragraph stale now?

> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block.c               | 104 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  blockdev.c            |   7 ++++
>  include/block/block.h |  10 +++++
>  qapi/block-core.json  |   1 +
>  4 files changed, 121 insertions(+), 1 deletion(-)
> 

> +/**
> + * A BdrvDirtyBitmap can be in three possible states:
> + * (1) successor is false and disabled is false: full r/w mode
> + * (2) successor is false and disabled is true: read only mode ("disabled")
> + * (3) successor is set: frozen mode.
> + *     A frozen bitmap cannot be renamed, deleted, anonymized, cleared, set,
> + *     or enabled. A frozen bitmap can only abdicate() or reclaim().
> + */
>  struct BdrvDirtyBitmap {
>      HBitmap *bitmap;
> +    BdrvDirtyBitmap *successor;

'successor is false' reads awkwardly given that it is not a bool; maybe
'successor is NULL' is better?

>  
> +bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
> +{
> +    return bitmap->successor;

Good thing C99 requires conversion of pointer to bool to work :)


> +
> +/**
> + * For a bitmap with a successor, yield our name to the successor,
> + * Delete the old bitmap, and return a handle to the new bitmap.

Sentences with capitals after comma, Even with a line break, Look weird.
(s/Delete/delete/)


> +
> +/**
> + * In cases of failure where we can no longer safely delete the parent,
> + * We may wish to re-join the parent and child/successor.

and again (s/We/we/)

Grammar fixes are minor, so:
Reviewed-by: Eric Blake <eblake@redhat.com>

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