Re: [Qemu-devel] [PATCH v11 08/13] block: Add bitmap successors

[Fam Zheng]

On Mon, 01/12 11:31, 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.

If we compare this with image snapshot and overlay, it fits the current backing
chain model very well.  Given that we're on the way to persistent dirty bitmap,
which will probably be read/written with general block.c code, I'm wondering if
it will be any better to reuse the block layer backing file code to handle
"successor" logic?

Also with the frozen feature built in dirty bitmaps, is it okay to drop
"enabled" state?

I think there are three states for a bitmap:

1) Active, no successor (similar to an read-write top image)
2) Frozen, no successor (similar to an read-only top image)
3) Frozen, with successor (similar to an read-only backing file, with an
   overlap)

Admittedly, more code is needed than this patch, in order to glue hbitmap and
block layer together, but it would probably make live migration of dirty bitmap
very easy, but I'm not sure without a closer look.

> 
> Signed-off-by: John Snow <address@hidden>
> ---
>  block.c               | 123 
> ++++++++++++++++++++++++++++++++++++++++++++++++--
>  blockdev.c            |  14 ++++++
>  include/block/block.h |  10 ++++
>  3 files changed, 144 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index bd4b449..3f33b9d 100644
> --- a/block.c
> +++ b/block.c
> @@ -53,10 +53,12 @@
>  
>  struct BdrvDirtyBitmap {
>      HBitmap *bitmap;
> +    BdrvDirtyBitmap *successor;
>      int64_t size;
>      int64_t granularity;
>      char *name;
>      bool enabled;
> +    bool frozen;
>      QLIST_ENTRY(BdrvDirtyBitmap) list;
>  };
>  
> @@ -5342,6 +5344,7 @@ BdrvDirtyBitmap 
> *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
>  
>  void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap 
> *bitmap)
>  {
> +    assert(!bitmap->frozen);
>      g_free(bitmap->name);
>      bitmap->name = NULL;
>  }
> @@ -5379,9 +5382,114 @@ BdrvDirtyBitmap 
> *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>      return bitmap;
>  }
>  
> +/**
> +  * A frozen bitmap cannot be renamed, deleted, cleared,
> +  * or set. A frozen bitmap can only abdicate, reclaim,
> +  * or thaw.
> +  */
> +static void bdrv_freeze_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> +{
> +    bitmap->frozen = true;
> +}
> +
> +static void bdrv_thaw_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> +{
> +    bitmap->frozen = false;
> +}
> +
> +bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
> +{
> +    return bitmap->frozen;
> +}
> +
>  bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
>  {
> -    return bitmap->enabled;
> +    return bitmap->enabled && !bitmap->frozen;

An indication that "enabled" could be replaced by "frozen". Otherwise it looks
confusing to me.

> +}
> +
> +/**
> + * 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;
> +
> +    if (bdrv_dirty_bitmap_frozen(bitmap)) {
> +        error_setg(errp,
> +                   "Cannot create a successor for a bitmap currently 
> in-use.");
> +        return -1;
> +    } else if (bitmap->successor) {
> +        error_setg(errp, "Cannot create a successor for a bitmap that "
> +                   "already has one.");
> +        return -1;
> +    }
> +
> +    /* Create an anonymous successor */
> +    granularity = bdrv_dirty_bitmap_granularity(bs, bitmap);
> +    bitmap->successor = bdrv_create_dirty_bitmap(bs, granularity, NULL, 
> errp);
> +    if (!bitmap->successor) {
> +        return -1;
> +    }
> +
> +    /* Successor will be on or off based on our current state.
> +     * Parent will be disabled and frozen. */
> +    bitmap->successor->enabled = bitmap->enabled;
> +    bdrv_disable_dirty_bitmap(bitmap);
> +    bdrv_freeze_dirty_bitmap(bitmap);
> +    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");
> +        return NULL;
> +    }
> +
> +    name = bitmap->name;
> +    bitmap->name = NULL;
> +    successor->name = name;
> +
> +    bdrv_thaw_dirty_bitmap(bitmap);
> +    bdrv_release_dirty_bitmap(bs, bitmap);
> +
> +    return successor;
> +}
> +
> +/**
> + * In cases of failure where we can no longer safely delete the parent,
> + * We may wish to re-join the parent and child/successor.
> + * The merged parent will be un-frozen, but not explicitly re-enabled.
> + */
> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> +                                           BdrvDirtyBitmap *parent,
> +                                           Error **errp)
> +{
> +    BdrvDirtyBitmap *successor = parent->successor;
> +    if (!successor) {
> +        error_setg(errp, "Cannot reclaim a successor when none is 
> present.\n");
> +        return NULL;
> +    }
> +
> +    hbitmap_merge(parent->bitmap, successor->bitmap);
> +    bdrv_release_dirty_bitmap(bs, successor);
> +
> +    bdrv_thaw_dirty_bitmap(parent);
> +    parent->successor = NULL;
> +
> +    return parent;
>  }
>  
>  void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> @@ -5389,6 +5497,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, 
> BdrvDirtyBitmap *bitmap)
>      BdrvDirtyBitmap *bm, *next;
>      QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
>          if (bm == bitmap) {
> +            assert(!bm->frozen);
>              QLIST_REMOVE(bitmap, list);
>              hbitmap_free(bitmap->bitmap);
>              g_free(bitmap->name);
> @@ -5405,6 +5514,7 @@ void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>  
>  void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>  {
> +    assert(!bitmap->frozen);
>      bitmap->enabled = true;
>  }
>  
> @@ -5483,7 +5593,9 @@ void bdrv_set_dirty_bitmap(BlockDriverState *bs, 
> BdrvDirtyBitmap *bitmap,
>  void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>                               int64_t cur_sector, int nr_sectors)
>  {
> -    hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
> +    if (!bitmap->frozen) {

Probably just assert it's not frozen?

> +        hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
> +    }
>  }
>  
>  /**
> @@ -5492,7 +5604,9 @@ void bdrv_reset_dirty_bitmap(BlockDriverState *bs, 
> BdrvDirtyBitmap *bitmap,
>   */
>  void bdrv_clear_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>  {
> -    hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
> +    if (!bitmap->frozen) {
> +        hbitmap_reset(bitmap->bitmap, 0, bitmap->size);

The same: probably just assert it's not frozen?

> +    }
>  }
>  
>  static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> @@ -5512,6 +5626,9 @@ static void bdrv_reset_dirty(BlockDriverState *bs, 
> int64_t cur_sector,
>  {
>      BdrvDirtyBitmap *bitmap;
>      QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> +        if (bitmap->frozen) {
> +            continue;
> +        }
>          hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
>      }
>  }
> diff --git a/blockdev.c b/blockdev.c
> index 118cb6c..8dde72a 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1988,9 +1988,16 @@ void qmp_block_dirty_bitmap_remove(const char 
> *node_ref, const char *name,
>      aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(aio_context);
>  
> +    if (bdrv_dirty_bitmap_frozen(bitmap)) {
> +        error_setg(errp,
> +                   "Bitmap %s is currently frozen and cannot be removed 
> yet.\n",
> +                   name);
> +        goto out;
> +    }
>      bdrv_dirty_bitmap_make_anon(bs, bitmap);
>      bdrv_release_dirty_bitmap(bs, bitmap);
>  
> + out:
>      aio_context_release(aio_context);
>  }
>  
> @@ -2009,8 +2016,15 @@ void qmp_block_dirty_bitmap_enable(const char 
> *node_ref, const char *name,
>      aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(aio_context);
>  
> +    if (bdrv_dirty_bitmap_frozen(bitmap)) {
> +        error_setg(errp, "Bitmap %s is frozen and cannot be enabled yet.\n",
> +                   name);
> +        goto out;
> +    }
> +
>      bdrv_enable_dirty_bitmap(bitmap);
>  
> + out:
>      aio_context_release(aio_context);
>  }
>  
> diff --git a/include/block/block.h b/include/block/block.h
> index a5bc249..6138544 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -433,6 +433,15 @@ BdrvDirtyBitmap 
> *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>                                            int granularity,
>                                            const char *name,
>                                            Error **errp);
> +int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
> +                                       BdrvDirtyBitmap *bitmap,
> +                                       Error **errp);
> +BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
> +                                            BdrvDirtyBitmap *bitmap,
> +                                            Error **errp);
> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> +                                           BdrvDirtyBitmap *bitmap,
> +                                           Error **errp);
>  BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
>                                          const char *name);
>  void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap 
> *bitmap);
> @@ -444,6 +453,7 @@ uint64_t 
> bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
>  uint64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
>                                        BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
> +bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
>  int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t 
> sector);
>  void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>                             int64_t cur_sector, int nr_sectors);
> -- 
> 1.9.3
>