Re: [Qemu-devel] [PATCH v6 16/18] qcow2: Switch store_bitmap_data() to byte-based iteration

[Kevin Wolf <kwolf@redhat.com>]

Am 30.08.2017 um 23:05 hat Eric Blake geschrieben:
> Now that we have adjusted the majority of the calls this function
> makes to be byte-based, it is easier to read the code if it makes
> passes over the image using bytes rather than sectors.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> ---
> v5: no change
> v4: new patch
> ---
>  block/qcow2-bitmap.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index b807298484..63d845e35f 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1072,10 +1072,9 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
>  {
>      int ret;
>      BDRVQcow2State *s = bs->opaque;
> -    int64_t sector;
> -    uint64_t limit, sbc;
> +    int64_t offset;
> +    uint64_t limit;
>      uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
> -    uint64_t bm_sectors = DIV_ROUND_UP(bm_size, BDRV_SECTOR_SIZE);
>      const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
>      uint8_t *buf = NULL;
>      BdrvDirtyBitmapIter *dbi;
> @@ -1100,18 +1099,17 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
>      dbi = bdrv_dirty_iter_new(bitmap);
>      buf = g_malloc(s->cluster_size);
>      limit = bytes_covered_by_bitmap_cluster(s, bitmap);
> -    sbc = limit >> BDRV_SECTOR_BITS;
>      assert(DIV_ROUND_UP(bm_size, limit) == tb_size);
> 
> -    while ((sector = bdrv_dirty_iter_next(dbi) >> BDRV_SECTOR_BITS) != -1) {
> -        uint64_t cluster = sector / sbc;
> +    while ((offset = bdrv_dirty_iter_next(dbi)) != -1) {

Don't you have to multiply both sides of the equation? This would be
offset != -512, which points out that the previous patch to convert
bdrv_dirty_iter_next() to byte-based gave it a really awkward interface.

> +        uint64_t cluster = offset / limit;
>          uint64_t end, write_size;
>          int64_t off;
> 
> -        sector = cluster * sbc;
> -        end = MIN(bm_sectors, sector + sbc);
> -        write_size = bdrv_dirty_bitmap_serialization_size(bitmap,
> -            sector * BDRV_SECTOR_SIZE, (end - sector) * BDRV_SECTOR_SIZE);
> +        offset = cluster * limit;

You just had cluster = offset / limit, so in other words, align down
offset? If so, this is how it should be written.

Kevin