Re: [Qemu-devel] [PATCH 04/13] block: add meta bitmaps

[Fam Zheng <famz@redhat.com>]

On Wed, 11/11 18:16, Vladimir Sementsov-Ogievskiy wrote:
> Meta bitmap is a 'dirty bitmap' for the BdrvDirtyBitmap. It tracks
> changes (set/unset) of this BdrvDirtyBitmap. It is needed for live
> migration of block dirty bitmaps.
> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block.c               | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h |  7 +++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 1171dbb..05307bc 100644
> --- a/block.c
> +++ b/block.c
> @@ -61,9 +61,15 @@
>   * (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().
> + *
> + * Meta bitmap:
> + * Meta bitmap is a 'dirty bitmap' for the BdrvDirtyBitmap. It tracks changes
> + * (set/unset) of this BdrvDirtyBitmap. It is needed for live migration of
> + * block dirty bitmaps.
>   */
>  struct BdrvDirtyBitmap {
>      HBitmap *bitmap;            /* Dirty sector bitmap implementation */
> +    HBitmap *meta_bitmap;       /* Meta bitmap */
>      BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
>      char *name;                 /* Optional non-empty unique ID */
>      int64_t size;               /* Size of the bitmap (Number of sectors) */
> @@ -3145,6 +3151,35 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
>      bitmap->name = NULL;
>  }
>  
> +HBitmap *bdrv_create_meta_bitmap(BdrvDirtyBitmap *bitmap,
> +                                 uint64_t chunk_size)
> +{
> +    uint64_t sector_granularity;
> +
> +    assert((chunk_size & (chunk_size - 1)) == 0);
> +
> +    /* one chunk is corresponding to one bit of the meta bitmap, and each bit
> +     * of the chunk is corresponding to 'bdrv_dirty_bitmap_granularity(bitmap)'
> +     * bytes of the node */
> +    sector_granularity =
> +        (chunk_size * 8 * bdrv_dirty_bitmap_granularity(bitmap))
> +        >> BDRV_SECTOR_BITS;
> +    assert(sector_granularity);
> +
> +    bitmap->meta_bitmap =
> +        hbitmap_alloc(bitmap->size, ctz64(sector_granularity));
> +
> +    return bitmap->meta_bitmap;
> +}
> +
> +void bdrv_release_meta_bitmap(BdrvDirtyBitmap *bitmap)
> +{
> +    if (bitmap->meta_bitmap) {
> +        hbitmap_free(bitmap->meta_bitmap);
> +        bitmap->meta_bitmap = NULL;
> +    }
> +}
> +
>  BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>                                            uint32_t granularity,
>                                            const char *name,
> @@ -3305,6 +3340,9 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>              assert(!bdrv_dirty_bitmap_frozen(bm));
>              QLIST_REMOVE(bitmap, list);
>              hbitmap_free(bitmap->bitmap);
> +            if (bitmap->meta_bitmap) {
> +                hbitmap_free(bitmap->meta_bitmap);
> +            }
>              g_free(bitmap->name);
>              g_free(bitmap);
>              return;
> @@ -3390,6 +3428,9 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>  {
>      assert(bdrv_dirty_bitmap_enabled(bitmap));
>      hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
> +    if (bitmap->meta_bitmap) {
> +        hbitmap_set(bitmap->meta_bitmap, cur_sector, nr_sectors);

This is a bit too much, the bitmap is not necessarily dirty even though
bdrv_set_dirty_bitmap is called, if this range is already dirty. For example
when the guest is repeatedly writing to a hot spot on disk.

Fam

> +    }
>  }
>  
>  void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> @@ -3397,6 +3438,9 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>  {
>      assert(bdrv_dirty_bitmap_enabled(bitmap));
>      hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
> +    if (bitmap->meta_bitmap) {
> +        hbitmap_set(bitmap->meta_bitmap, cur_sector, nr_sectors);
> +    }
>  }
>  
>  void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> diff --git a/include/block/block.h b/include/block/block.h
> index 5b9c2d6..7deb8e8 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -5,6 +5,7 @@
>  #include "qemu-common.h"
>  #include "qemu/option.h"
>  #include "qemu/coroutine.h"
> +#include "qemu/hbitmap.h"
>  #include "block/accounting.h"
>  #include "qapi/qmp/qobject.h"
>  #include "qapi-types.h"
> @@ -511,6 +512,12 @@ void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap *bitmap,
>                                            uint64_t start, uint64_t count);
>  void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>  
> +/* chunk size here is number of bytes of the @bitmap data per one bit of the
> + * meta bitmap being created */
> +HBitmap *bdrv_create_meta_bitmap(BdrvDirtyBitmap *bitmap,
> +                                 uint64_t granularity);
> +void bdrv_release_meta_bitmap(BdrvDirtyBitmap *bitmap);
> +
>  void bdrv_enable_copy_on_read(BlockDriverState *bs);
>  void bdrv_disable_copy_on_read(BlockDriverState *bs);
>  
> -- 
> 2.1.4
> 
>