Re: [Qemu-devel] [PATCH 05/12] dirty-bitmap: Set iterator start by offset, not sector

[John Snow]

On 04/12/2017 01:49 PM, Eric Blake wrote:
> All callers to bdrv_dirty_iter_new() passed 0 for their initial
> starting point, drop that parameter.
> 
> All callers to bdrv_set_dirty_iter() were scaling an offset to
> a sector number; move the scaling to occur internally to dirty
> bitmap code instead.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  include/block/dirty-bitmap.h | 5 ++---
>  block/backup.c               | 5 ++---
>  block/dirty-bitmap.c         | 9 ++++-----
>  block/mirror.c               | 4 ++--
>  4 files changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index a83979d..efcec60 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -41,11 +41,10 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>  void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>                               int64_t cur_sector, int64_t nr_sectors);
>  BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
> -BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
> -                                         uint64_t first_sector);
> +BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap);
>  void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
>  int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
> -void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);
> +void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t offset);
>  int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
>  int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
>  void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
> diff --git a/block/backup.c b/block/backup.c
> index 63ca208..efa4896 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -372,7 +372,7 @@ static int coroutine_fn 
> backup_run_incremental(BackupBlockJob *job)
> 
>      granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
>      clusters_per_iter = MAX((granularity / job->cluster_size), 1);
> -    dbi = bdrv_dirty_iter_new(job->sync_bitmap, 0);
> +    dbi = bdrv_dirty_iter_new(job->sync_bitmap);
> 
>      /* Find the next dirty sector(s) */
>      while ((offset = bdrv_dirty_iter_next(dbi) * BDRV_SECTOR_SIZE) >= 0) {
> @@ -403,8 +403,7 @@ static int coroutine_fn 
> backup_run_incremental(BackupBlockJob *job)
>          /* If the bitmap granularity is smaller than the backup granularity,
>           * we need to advance the iterator pointer to the next cluster. */
>          if (granularity < job->cluster_size) {
> -            bdrv_set_dirty_iter(dbi,
> -                                cluster * job->cluster_size / 
> BDRV_SECTOR_SIZE);
> +            bdrv_set_dirty_iter(dbi, cluster * job->cluster_size);
>          }
> 
>          last_cluster = cluster - 1;
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index a413df1..3fb4871 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -367,11 +367,10 @@ uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap 
> *bitmap)
>      return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
>  }
> 
> -BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
> -                                         uint64_t first_sector)
> +BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap)
>  {
>      BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
> -    hbitmap_iter_init(&iter->hbi, bitmap->bitmap, first_sector);
> +    hbitmap_iter_init(&iter->hbi, bitmap->bitmap, 0);
>      iter->bitmap = bitmap;
>      bitmap->active_iterators++;
>      return iter;
> @@ -488,9 +487,9 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t 
> cur_sector,
>  /**
>   * Advance a BdrvDirtyBitmapIter to an arbitrary offset.
>   */
> -void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t sector_num)
> +void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t offset)
>  {
> -    hbitmap_iter_init(&iter->hbi, iter->hbi.hb, sector_num);
> +    hbitmap_iter_init(&iter->hbi, iter->hbi.hb, offset >> BDRV_SECTOR_BITS);
>  }
> 
>  int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
> diff --git a/block/mirror.c b/block/mirror.c
> index c92335a..7c1d6bf 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -370,7 +370,7 @@ static uint64_t coroutine_fn 
> mirror_iteration(MirrorBlockJob *s)
>          next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
>          if (next_dirty > next_offset || next_dirty < 0) {
>              /* The bitmap iterator's cache is stale, refresh it */
> -            bdrv_set_dirty_iter(s->dbi, next_offset >> BDRV_SECTOR_BITS);
> +            bdrv_set_dirty_iter(s->dbi, next_offset);
>              next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
>          }
>          assert(next_dirty == next_offset);
> @@ -779,7 +779,7 @@ static void coroutine_fn mirror_run(void *opaque)
>      }
> 
>      assert(!s->dbi);
> -    s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap, 0);
> +    s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap);
>      for (;;) {
>          uint64_t delay_ns = 0;
>          int64_t cnt, delta;
> 

Reviewed-by: John Snow <address@hidden>