Re: [Qemu-devel] [PATCH RFC v3 05/14] block: add meta bitmaps

[John Snow <jsnow@redhat.com>]

On 02/18/2015 09:00 AM, 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.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
> ---
>   block.c               | 40 ++++++++++++++++++++++++++++++++++++++++
>   include/block/block.h |  5 +++++
>   2 files changed, 45 insertions(+)
>
> diff --git a/block.c b/block.c
> index a127fd2..aaa08b8 100644
> --- a/block.c
> +++ b/block.c
> @@ -58,9 +58,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) */
> @@ -5398,6 +5404,31 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
>       bitmap->name = NULL;
>   }
>
> +HBitmap *bdrv_create_meta_bitmap(BdrvDirtyBitmap *bitmap,
> +                                        uint64_t granularity)
> +{
> +    uint64_t sector_granularity;
> +
> +    assert((granularity & (granularity - 1)) == 0);
> +
> +    granularity *= 8 * bdrv_dirty_bitmap_granularity(bitmap);
> +    sector_granularity = granularity >> BDRV_SECTOR_BITS;
> +    assert(sector_granularity);
> +

The maths here could use a comment, I think.

the "granularity" field here actually describes the desired 
serialization buffer size; e.g. CHUNK_SIZE (1 << 20) or 1 MiB. This 
parameter should be renamed to explain what it's actually for. Something 
like "chunk_size" and a comment explaining that it is in bytes.

...

That said, let's talk about the default chunk size you're using in 
correlation with this function.

a CHUNK_SIZE of 1MiB here is going to lead us to, if we have a bitmap 
with the default granularity of 128 sectors\64KiB bytes, a granularity 
for the meta_bitmap of one billion sectors (1 << 30) or 512GiB.

That's going to be bigger than most drives entirely, which will 
generally lead us to only using a single "chunk" per drive. Which means 
we won't really get a lot of mileage out of the bulk/dirty phases most 
of the time.

It's wild to think about that the first 1,000,000,000 sectors or 
512,000,000,000 bytes will all be represented by the first single bit in 
this bitmap. If a single hair on the drive changes, we resend the 
_entire_ bitmap, possibly over and over again. Will we ever make 
progress? Should we investigate a smaller chunk size?

Here's some quick mappings of chunk size (bytes) to effective 
meta_bitmap byte granularities, assuming the meta_bitmap is tracking a 
bitmap with the default granularity of 64KiB:

(1 << 20) 1MiB   -- 512GiB  // This is too high of a granularity
(1 << 17) 128KiB --  64GiB
(1 << 15) 32KiB  --  16GiB
(1 << 11) 2KiB   --   1GiB
(1 << 10) 1KiB   -- 512MiB
(1 << 9)  512B   -- 256MiB
(1 << 8)  256B   -- 128MiB
(1 << 5)  32 B   --  16MiB  // This is too small of a chunk size.
(1 << 1)   1 B   --   1MiB

We want to make the chunk sends efficient, but we also want to make sure 
that the dirty phase doesn't resend more data than it needs to, so we 
need to strike a balance here, no?

I think arguments could be made for most granularities between 128MiB 
through 1GiB. Anything outside of that is too lopsided, IMO.

What are your thoughts on this?

> +    bitmap->meta_bitmap =
> +        hbitmap_alloc(bitmap->size, ffsll(sector_granularity) - 1);
> +
> +    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,
> @@ -5532,6 +5563,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;
> @@ -5659,6 +5693,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);
> +    }
>   }
>
>   void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> @@ -5666,6 +5703,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 c6a928d..f2c62f6 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -4,6 +4,7 @@
>   #include "block/aio.h"
>   #include "qemu-common.h"
>   #include "qemu/option.h"
> +#include "qemu/hbitmap.h"
>   #include "block/coroutine.h"
>   #include "block/accounting.h"
>   #include "qapi/qmp/qobject.h"
> @@ -487,6 +488,10 @@ void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap *bitmap,
>                                             uint64_t start, uint64_t count);
>   void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>
> +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);
>
>