Re: [Qemu-block] [PATCH v22 06/30] block/dirty-bitmap: add deserialize_ones func

[Eric Blake]

On 06/28/2017 07:05 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add bdrv_dirty_bitmap_deserialize_ones() function, which is needed for
> qcow2 bitmap loading, to handle unallocated bitmap parts, marked as
> all-ones.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Reviewed-by: Kevin Wolf <address@hidden>
> Reviewed-by: John Snow <address@hidden>
> ---
>  block/dirty-bitmap.c         |  7 +++++++
>  include/block/dirty-bitmap.h |  3 +++
>  include/qemu/hbitmap.h       | 15 +++++++++++++++
>  util/hbitmap.c               | 17 +++++++++++++++++
>  4 files changed, 42 insertions(+)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index df0110cf9f..f502c45a70 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -586,6 +586,13 @@ void 
> bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap *bitmap,
>      hbitmap_deserialize_zeroes(bitmap->bitmap, start, count, finish);
>  }
>  
> +void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap,
> +                                        uint64_t start, uint64_t count,
> +                                        bool finish)
> +{
> +    hbitmap_deserialize_ones(bitmap->bitmap, start, count, finish);
> +}

What unit is 'count' in?

/me reads the code, finding serialization_chunk()

Oh, it's in terms of the underlying bitmap->bitmap->granularity
(currently the number of sectors).  Eww - I'd rather we name the
parameter 'bytes' rather than 'count', and then scale it appropriately
when passing to the underlying bitmap->bitmap, rather than making the
user guess what scale to use.

But as you are just copying from the existing deserialize_zeroes right
above, I'd rather keep the semantics the same between the two, so my
series will take care of fixing that up, and your series can go in as-is
for this patch.

> +++ b/include/qemu/hbitmap.h
> @@ -229,6 +229,21 @@ void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t 
> start, uint64_t count,
>                                  bool finish);
>  
>  /**
> + * hbitmap_deserialize_ones
> + * @hb: HBitmap to operate on.
> + * @start: First bit to restore.
> + * @count: Number of bits to restore.

This part is accurate (the dirty-bitmap is using an underlying bitmap
with "one bit per sector" before my series, afterwards it will be "one
bit per byte", remembering that hbitmap really stores only one bit per
granularity multiple of the underlying unit), if incomplete (the code
asserts that things are aligned, but doesn't document that the caller
must pass in aligned values); but again, that's matching the
pre-existing deserialize_zeroes code.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature