Re: [Qemu-block] [Qemu-devel] [PATCH 08/12] dirty-bitmap: Change bdrv_get_dirty() to take bytes

[John Snow]

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 <address@hidden>
> ---
>  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 <address@hidden>