Re: [Qemu-devel] [PATCH v12 06/17] block: Add bitmap successors

[Max Reitz <mreitz@redhat.com>]

On 2015-02-09 at 20:35, 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.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block.c               | 104 +++++++++++++++++++++++++++++++++++++++++++++++++-
>   blockdev.c            |  22 +++++++++++
>   include/block/block.h |  10 +++++
>   qapi/block-core.json  |   3 ++
>   4 files changed, 138 insertions(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index 8d63375..8d84ace 100644
> --- a/block.c
> +++ b/block.c
> @@ -51,8 +51,17 @@
>   #include <windows.h>
>   #endif
>   
> +/**
> + * 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;
>       char *name;
>       bool disabled;
>       QLIST_ENTRY(BdrvDirtyBitmap) list;
> @@ -5383,6 +5392,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
>   
>   void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>   {
> +    assert(!bdrv_dirty_bitmap_frozen(bitmap));
>       g_free(bitmap->name);
>       bitmap->name = NULL;
>   }
> @@ -5418,9 +5428,98 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>       return bitmap;
>   }
>   
> +bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
> +{
> +    return bitmap->successor;
> +}
> +
>   bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
>   {
> -    return !bitmap->disabled;
> +    return !(bitmap->disabled || bitmap->successor);
> +}
> +
> +/**
> + * Create a successor bitmap destined to replace this bitmap after an operation.
> + * Requires that the bitmap is not frozen and has no successor.
> + */
> +int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
> +                                       BdrvDirtyBitmap *bitmap, Error **errp)
> +{
> +    uint64_t granularity;
> +    BdrvDirtyBitmap *child;
> +
> +    if (bdrv_dirty_bitmap_frozen(bitmap)) {
> +        error_setg(errp, "Cannot create a successor for a bitmap that is "
> +                   "currently frozen.");

Hm, I seem to remember Markus saying something about full stops at the 
end of error messages...

> +        return -1;
> +    }
> +    assert(!bitmap->successor);
> +
> +    /* Create an anonymous successor */
> +    granularity = bdrv_dirty_bitmap_granularity(bitmap);
> +    child = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
> +    if (!child) {
> +        return -1;
> +    }
> +
> +    /* Successor will be on or off based on our current state. */
> +    child->disabled = bitmap->disabled;
> +
> +    /* Install the successor and freeze the parent */
> +    bitmap->successor = child;
> +    return 0;
> +}
> +
> +/**
> + * For a bitmap with a successor, yield our name to the successor,
> + * Delete the old bitmap, and return a handle to the new bitmap.
> + */
> +BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
> +                                            BdrvDirtyBitmap *bitmap,
> +                                            Error **errp)
> +{
> +    char *name;
> +    BdrvDirtyBitmap *successor = bitmap->successor;
> +
> +    if (successor == NULL) {
> +        error_setg(errp, "Cannot relinquish control if "
> +                   "there's no successor present.\n");

So while I'm unsure about full stops, I am pretty sure that we don't 
really want newlines.

With the full stops and newlines removed (in all error_setg() calls in 
this patch):

Reviewed-by: Max Reitz <mreitz@redhat.com>