[08/12] dirty-bitmap: Change bdrv_get_dirty() to take bytes

Half the callers were already scaling bytes to sectors; the other
half can eventually be simplified to use byte iteration.  Both
callers were already using the result as a bool, so make that
explicit.  Making the change also makes it easier for a future
dirty-bitmap patch to offload scaling over to the internal hbitmap.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/dirty-bitmap.h | 4 ++--
 block/dirty-bitmap.c         | 8 ++++----
 block/mirror.c               | 3 +--
 migration/block.c            | 3 +--
 4 files changed, 8 insertions(+), 10 deletions(-)

On 04/12/2017 01:49 PM, Eric Blake wrote:
> Half the callers were already scaling bytes to sectors; the other
> half can eventually be simplified to use byte iteration.  Both
> callers were already using the result as a bool, so make that
> explicit.  Making the change also makes it easier for a future
> dirty-bitmap patch to offload scaling over to the internal hbitmap.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/dirty-bitmap.h | 4 ++--
>  block/dirty-bitmap.c         | 8 ++++----
>  block/mirror.c               | 3 +--
>  migration/block.c            | 3 +--
>  4 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index efcec60..b8434e5 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -34,8 +34,8 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
>  const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap);
>  DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
> -int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> -                   int64_t sector);
> +bool bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> +                    int64_t offset);
>  void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>                             int64_t cur_sector, int64_t nr_sectors);
>  void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index e3c2e34..c8100d2 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -332,13 +332,13 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>      return list;
>  }
> 
> -int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> -                   int64_t sector)
> +bool bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> +                    int64_t offset)
>  {
>      if (bitmap) {
> -        return hbitmap_get(bitmap->bitmap, sector);
> +        return hbitmap_get(bitmap->bitmap, offset >> BDRV_SECTOR_BITS);
>      } else {
> -        return 0;
> +        return false;
>      }
>  }
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 1b98a77..1e2e655 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -359,8 +359,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>          int64_t next_offset = offset + nb_chunks * s->granularity;
>          int64_t next_chunk = next_offset / s->granularity;
>          if (next_offset >= s->bdev_length ||
> -            !bdrv_get_dirty(source, s->dirty_bitmap,
> -                            next_offset >> BDRV_SECTOR_BITS)) {
> +            !bdrv_get_dirty(source, s->dirty_bitmap, next_offset)) {
>              break;
>          }
>          if (test_bit(next_chunk, s->in_flight_bitmap)) {
> diff --git a/migration/block.c b/migration/block.c
> index 3daa5c7..9e21aeb 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -537,8 +537,7 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
>          } else {
>              blk_mig_unlock();
>          }
> -        if (bdrv_get_dirty(bs, bmds->dirty_bitmap, sector)) {
> -
> +        if (bdrv_get_dirty(bs, bmds->dirty_bitmap, sector * BDRV_SECTOR_SIZE)) {

This one is a little weirder now though, isn't it? We're asking for the
dirty status of a single byte, technically. In practice, the scaling
factor will always cover the entire sector, but it reads a lot jankier now.

>              if (total_sectors - sector < BDRV_SECTORS_PER_DIRTY_CHUNK) {
>                  nr_sectors = total_sectors - sector;
>              } else {
> 

Oh well, it was always janky...

Reviewed-by: John Snow <jsnow@redhat.com>

On 04/12/2017 07:25 PM, John Snow wrote:

>> +++ b/migration/block.c
>> @@ -537,8 +537,7 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
>>          } else {
>>              blk_mig_unlock();
>>          }
>> -        if (bdrv_get_dirty(bs, bmds->dirty_bitmap, sector)) {
>> -
>> +        if (bdrv_get_dirty(bs, bmds->dirty_bitmap, sector * BDRV_SECTOR_SIZE)) {
> 
> This one is a little weirder now though, isn't it? We're asking for the
> dirty status of a single byte, technically. In practice, the scaling
> factor will always cover the entire sector, but it reads a lot jankier now.
> 
>>              if (total_sectors - sector < BDRV_SECTORS_PER_DIRTY_CHUNK) {
>>                  nr_sectors = total_sectors - sector;
>>              } else {
>>
> 
> Oh well, it was always janky...

True. Think of it as "is the granularity (which may be a sector, a
cluster, or even some other size) that contains 'offset' dirty?".  I
really think migration/block.c will be easier to read once converted to
byte operations everywhere, but have not yet tackled it (it was hard
enough tackling mirror, backup, commit, and stream in parallel for the
previous series).

> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> 

Thanks for the reviews, by the way, even if the prerequisite patches
still haven't been fully reviewed.