On 05/17/2017 06:59 AM, Juan Quintela wrote: > Eric Blake <ebl...@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 <ebl...@redhat.com> >> Reviewed-by: John Snow <js...@redhat.com> > > Reviewed-by: Juan Quintela <quint...@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
signature.asc
Description: OpenPGP digital signature
