Re: [PATCH v3 07/19] block/dirty-bitmap: introduce bdrv_dirty_bitmap_status()

[Nikta Lapshin <nikita.lapshin@virtuozzo.com>]

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

On 12/22/21 20:40, Vladimir Sementsov-Ogievskiy wrote:

> Add a convenient function similar with bdrv_block_status() to get
> status of dirty bitmap.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
> ---
>   include/block/dirty-bitmap.h |  2 ++
>   include/qemu/hbitmap.h       | 11 +++++++++++
>   block/dirty-bitmap.c         |  6 ++++++
>   util/hbitmap.c               | 36 ++++++++++++++++++++++++++++++++++++
>   4 files changed, 55 insertions(+)
>
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index f95d350b70..2ae7dc3d1d 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -115,6 +115,8 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, int64_t offset,
>   bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
>           int64_t start, int64_t end, int64_t max_dirty_count,
>           int64_t *dirty_start, int64_t *dirty_count);
> +void bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap, int64_t offset,
> +                              int64_t bytes, bool *is_dirty, int64_t *count);
>   BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
>                                                     Error **errp);
>   
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index 5e71b6d6f7..845fda12db 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -340,6 +340,17 @@ bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t start, int64_t end,
>                                int64_t max_dirty_count,
>                                int64_t *dirty_start, int64_t *dirty_count);
>   
> +/*
> + * bdrv_dirty_bitmap_status:
> + * @hb: The HBitmap to operate on
> + * @start: the offset to start from
> + * @end: end of requested area
> + * @is_dirty: is bitmap dirty at @offset
> + * @pnum: how many bits has same value starting from @offset
> + */
> +void hbitmap_status(const HBitmap *hb, int64_t offset, int64_t bytes,
> +                    bool *is_dirty, int64_t *pnum);
> +

I think description should be changed, there is no start and no end
arguments in function.

>   /**
>    * hbitmap_iter_next:
>    * @hbi: HBitmapIter to operate on.
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 94a0276833..e4a836749a 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -875,6 +875,12 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
>                                      dirty_start, dirty_count);
>   }
>   
> +void bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap, int64_t offset,
> +                              int64_t bytes, bool *is_dirty, int64_t *count)
> +{
> +    hbitmap_status(bitmap->bitmap, offset, bytes, is_dirty, count);
> +}
> +
>   /**
>    * bdrv_merge_dirty_bitmap: merge src into dest.
>    * Ensures permissions on bitmaps are reasonable; use for public API.
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 305b894a63..ae8d0eb4d2 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -301,6 +301,42 @@ bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t start, int64_t end,
>       return true;
>   }
>   
> +void hbitmap_status(const HBitmap *hb, int64_t start, int64_t count,
> +                    bool *is_dirty, int64_t *pnum)
> +{
> +    int64_t next_dirty, next_zero;
> +
> +    assert(start >= 0);
> +    assert(count > 0);
> +    assert(start + count <= hb->orig_size);
> +
> +    next_dirty = hbitmap_next_dirty(hb, start, count);
> +    if (next_dirty == -1) {
> +        *pnum = count;
> +        *is_dirty = false;
> +        return;
> +    }
> +
> +    if (next_dirty > start) {
> +        *pnum = next_dirty - start;
> +        *is_dirty = false;
> +        return;
> +    }
> +
> +    assert(next_dirty == start);
> +
> +    next_zero = hbitmap_next_zero(hb, start, count);
> +    if (next_zero == -1) {
> +        *pnum = count;
> +        *is_dirty = true;
> +        return;
> +    }
> +
> +    assert(next_zero > start);
> +    *pnum = next_zero - start;
> +    *is_dirty = false;
> +}
> +

This function finds if this bitmap is dirty and also counts first bits.
I don't think that this is a problem, but may be it should be divided?

>   bool hbitmap_empty(const HBitmap *hb)
>   {
>       return hb->count == 0;

With corrected description
Reviewed-by: Nikita Lapshin<nikita.lapshin@virtuozzo.com>

[-- Attachment #2: Type: text/html, Size: 5239 bytes --]