[v4,14/17] qcow2: Switch load_bitmap_data() to byte-based iteration

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>

---
v4: new patch
---
 block/qcow2-bitmap.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

On 07/03/2017 11:10 AM, Eric Blake wrote:
> 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>

Vladimir, look good to you?

--js

03.07.2017 18:10, Eric Blake wrote:
> 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>
>
> ---
> v4: new patch
> ---
>   block/qcow2-bitmap.c | 22 ++++++++--------------
>   1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 280960a..1de2ab5 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -291,9 +291,8 @@ static int load_bitmap_data(BlockDriverState *bs,
>   {
>       int ret = 0;
>       BDRVQcow2State *s = bs->opaque;
> -    uint64_t sector, limit, sbc;
> +    uint64_t offset, limit;
>       uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
> -    uint64_t bm_sectors = DIV_ROUND_UP(bm_size, BDRV_SECTOR_SIZE);
>       uint8_t *buf = NULL;
>       uint64_t i, tab_size =
>               size_to_clusters(s,
> @@ -305,32 +304,27 @@ static int load_bitmap_data(BlockDriverState *bs,
>
>       buf = g_malloc(s->cluster_size);
>       limit = bytes_covered_by_bitmap_cluster(s, bitmap);
> -    sbc = limit >> BDRV_SECTOR_BITS;
> -    for (i = 0, sector = 0; i < tab_size; ++i, sector += sbc) {
> -        uint64_t count = MIN(bm_sectors - sector, sbc);
> +    for (i = 0, offset = 0; i < tab_size; ++i, offset += limit) {
> +        uint64_t count = MIN(bm_size - offset, limit);
>           uint64_t entry = bitmap_table[i];
> -        uint64_t offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
> +        uint64_t data_offset = entry & BME_TABLE_ENTRY_OFFSET_MASK;
>
>           assert(check_table_entry(entry, s->cluster_size) == 0);
>
> -        if (offset == 0) {
> +        if (data_offset == 0) {
>               if (entry & BME_TABLE_ENTRY_FLAG_ALL_ONES) {
> -                bdrv_dirty_bitmap_deserialize_ones(bitmap,
> -                                                   sector * BDRV_SECTOR_SIZE,
> -                                                   count * BDRV_SECTOR_SIZE,
> +                bdrv_dirty_bitmap_deserialize_ones(bitmap, offset, count,

The change is that now 3rd parameter may not be aligned by sectors, but 
looks like it should be ok.

Reviewed-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>



>                                                      false);
>               } else {
>                   /* No need to deserialize zeros because the dirty bitmap is
>                    * already cleared */
>               }
>           } else {
> -            ret = bdrv_pread(bs->file, offset, buf, s->cluster_size);
> +            ret = bdrv_pread(bs->file, data_offset, buf, s->cluster_size);
>               if (ret < 0) {
>                   goto finish;
>               }
> -            bdrv_dirty_bitmap_deserialize_part(bitmap, buf,
> -                                               sector * BDRV_SECTOR_SIZE,
> -                                               count * BDRV_SECTOR_SIZE,
> +            bdrv_dirty_bitmap_deserialize_part(bitmap, buf, offset, count,
>                                                  false);
>           }
>       }

On 07/13/2017 04:16 AM, Vladimir Sementsov-Ogievskiy wrote:
> 03.07.2017 18:10, Eric Blake wrote:
>> 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>
>>
>> ---
>> v4: new patch
>> ---

>>
>> -        if (offset == 0) {
>> +        if (data_offset == 0) {
>>               if (entry & BME_TABLE_ENTRY_FLAG_ALL_ONES) {
>> -                bdrv_dirty_bitmap_deserialize_ones(bitmap,
>> -                                                   sector *
>> BDRV_SECTOR_SIZE,
>> -                                                   count *
>> BDRV_SECTOR_SIZE,
>> +                bdrv_dirty_bitmap_deserialize_ones(bitmap, offset,
>> count,
> 
> The change is that now 3rd parameter may not be aligned by sectors, but
> looks like it should be ok.

Correct - the dirty bitmap automatically widens any non-aligned input
into granularity.

> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
> 

I posted a v5 prior to this email, if you want to add your Reviewed-by
on any patches there (I think this patch went unchanged into that
revision, but scripts don't always pick up R-b given on a previous
version of a patch).