Re: [Qemu-devel] [PATCH v5 04/10] block: Support meta dirty bitmap

[Max Reitz <mreitz@redhat.com>]

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

On 03.06.2016 06:32, Fam Zheng wrote:
> The added group of operations enables tracking of the changed bits in
> the dirty bitmap.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/dirty-bitmap.c         | 52 ++++++++++++++++++++++++++++++++++++++++++++
>  include/block/dirty-bitmap.h |  9 ++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 628b77c..9c53c56 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -38,6 +38,7 @@
>   */
>  struct BdrvDirtyBitmap {
>      HBitmap *bitmap;            /* Dirty sector bitmap implementation */
> +    HBitmap *meta;              /* Meta dirty 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) */
> @@ -103,6 +104,56 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>      return bitmap;
>  }
>  
> +/* bdrv_create_meta_dirty_bitmap
> + *
> + * Create a meta dirty bitmap that tracks the changes of bits in @bitmap. I.e.
> + * when a dirty status bit in @bitmap is changed (either from reset to set or
> + * the other way around), its respective meta dirty bitmap bit will be marked
> + * dirty as well.

Not wrong, but I'd like a note here that this is not an
when-and-only-when relationship, i.e. that bits in the meta bitmap may
be set even without the tracked bits in the dirty bitmap having changed.

Maybe this should be mentioned somewhere in patch 2, too. Or maybe only
in patch 2.

> + *
> + * @bitmap: the block dirty bitmap for which to create a meta dirty bitmap.
> + * @chunk_size: how many bytes of bitmap data does each bit in the meta bitmap
> + * track.
> + */
> +void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> +                                   int chunk_size)
> +{
> +    assert(!bitmap->meta);
> +    bitmap->meta = hbitmap_create_meta(bitmap->bitmap,
> +                                       chunk_size * BITS_PER_BYTE);
> +}
> +
> +void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> +{
> +    assert(bitmap->meta);
> +    hbitmap_free_meta(bitmap->bitmap);
> +    bitmap->meta = NULL;
> +}
> +
> +int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
> +                               BdrvDirtyBitmap *bitmap, int64_t sector,
> +                               int nb_sectors)
> +{
> +    uint64_t i;
> +    int gran = bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
> +
> +    /* To optimize: we can make hbitmap to internally check the range in a
> +     * coarse level, or at least do it word by word. */

We could also multiply gran by the granularity of the meta bitmap. Each
bit of the meta bitmap tracks at least eight bits of the dirty bitmap,
so we're calling hbitmap_get() at least eight times as often as
necessary here.

Or we just use int gran = hbitmap_granularity(bitmap->meta);.

Not exactly wrong, though, so:

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

> +    for (i = sector; i < sector + nb_sectors; i += gran) {
> +        if (hbitmap_get(bitmap->meta, i)) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]