Re: [Qemu-devel] [PATCH v2 09/12] dirty-bitmap: Change bdrv_[re]set_dirty_bitmap() to use bytes

[Eric Blake <eblake@redhat.com>]

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

On 05/17/2017 06:59 AM, Juan Quintela wrote:
> Eric Blake <eblake@redhat.com> wrote:
>> Some of the callers were already scaling bytes to sectors; others
>> can be easily converted to pass byte offsets, all in our shift
>> towards a consistent byte interface everywhere.  Making the change
>> will also make it easier to write the hold-out callers to use byte
>> rather than sectors for their iterations; it also makes it easier
>> for a future dirty-bitmap patch to offload scaling over to the
>> internal hbitmap.  Although all callers happen to pass
>> sector-aligned values, make the internal scaling robust to any
>> sub-sector requests.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: John Snow <jsnow@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> 
>> +
>>      assert(bdrv_dirty_bitmap_enabled(bitmap));
>> -    hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
>> +    hbitmap_set(bitmap->bitmap, offset >> BDRV_SECTOR_BITS,
>> +                end_sector - (offset >> BDRV_SECTOR_BITS));
> 
> It is just me, or we use:
> 
> bytes = sectors * BDRV_SECTOR_SIZE
> or
> bytes = sectors << BDRV_SECTOR_BITS

BDRV_SECTOR_SIZE is ULL, while BDRV_SECTOR_BITS is not. Multiplying by
SIZE guarantees a 64-bit answer, but shifting by bits will give only a
32-bit answer if the left hand side of << is not already 64 bits (I've
had bugs in earlier conversion patches where I used << but should have
used *).  So for scaling up, my recent round of patches have been
favoring * (ideally, since BDRV_SECTOR_SIZE is a power-of-2 constant,
the compiler optimizes it into a shift anyways instead of wasting time
on a multiply).

For scaling down, '/ SIZE' and '>> BITS' are equivalent, but I prefer
the >> form.

> 
> and the same from bytes to sectors (no, this patch is consistent,
> but just looking at the file ....)

The real end-goal is to update the file to track the entire process with
bytes everywhere (and skip the intermediate representation in sectors),
which will reduce the number of scaling operations needed in the first
place.  migration/block.c isn't quite there yet, but we'll get there.

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


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