Re: [Qemu-block] [Qemu-devel] [PATCH v5 01/10] block: Hide HBitmap in block dirty bitmap interface

[Vladimir Sementsov-Ogievskiy]

On 13.07.2016 01:49, John Snow wrote:
> On 06/03/2016 12:32 AM, Fam Zheng wrote:
> > HBitmap is an implementation detail of block dirty bitmap that should be hidden
> > from users. Introduce a BdrvDirtyBitmapIter to encapsulate the underlying
> > HBitmapIter.
> >
> > A small difference in the interface is, before, an HBitmapIter is initialized
> > in place, now the new BdrvDirtyBitmapIter must be dynamically allocated because
> > the structure definition is in block/dirty-bitmap.c.
> >
> > Two current users are converted too.
> >
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> >   block/backup.c               | 14 ++++++++------
> >   block/dirty-bitmap.c         | 39 +++++++++++++++++++++++++++++++++------
> >   block/mirror.c               | 24 +++++++++++++-----------
> >   include/block/dirty-bitmap.h |  7 +++++--
> >   include/qemu/typedefs.h      |  1 +
> >   5 files changed, 60 insertions(+), 25 deletions(-)
> >
> > diff --git a/block/backup.c b/block/backup.c
> > index feeb9f8..ac7ca45 100644
> > --- a/block/backup.c
> > +++ b/block/backup.c
> > @@ -317,14 +317,14 @@ static int coroutine_fn 
> > backup_run_incremental(BackupBlockJob *job)
> >       int64_t end;
> >       int64_t last_cluster = -1;
> >       int64_t sectors_per_cluster = cluster_size_sectors(job);
> > -    HBitmapIter hbi;
> > +    BdrvDirtyBitmapIter *dbi;
> >   
> >       granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
> >       clusters_per_iter = MAX((granularity / job->cluster_size), 1);
> > -    bdrv_dirty_iter_init(job->sync_bitmap, &hbi);
> > +    dbi = bdrv_dirty_iter_new(job->sync_bitmap, 0);
> >   
> >       /* Find the next dirty sector(s) */
> > -    while ((sector = hbitmap_iter_next(&hbi)) != -1) {
> > +    while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
> >           cluster = sector / sectors_per_cluster;
> >   
> >           /* Fake progress updates for any clusters we skipped */
> > @@ -336,7 +336,7 @@ static int coroutine_fn 
> > backup_run_incremental(BackupBlockJob *job)
> >           for (end = cluster + clusters_per_iter; cluster < end; cluster++) {
> >               do {
> >                   if (yield_and_check(job)) {
> > -                    return ret;
> > +                    goto out;
> >                   }
> >                   ret = backup_do_cow(job, cluster * sectors_per_cluster,
> >                                       sectors_per_cluster, &error_is_read,
> > @@ -344,7 +344,7 @@ static int coroutine_fn 
> > backup_run_incremental(BackupBlockJob *job)
> >                   if ((ret < 0) &&
> >                       backup_error_action(job, error_is_read, -ret) ==
> >                       BLOCK_ERROR_ACTION_REPORT) {
> > -                    return ret;
> > +                    goto out;
> >                   }
> >               } while (ret < 0);
> >           }
> > @@ -352,7 +352,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(&hbi, cluster * sectors_per_cluster);
> > +            bdrv_set_dirty_iter(dbi, cluster * sectors_per_cluster);
> >           }
> >   
> >           last_cluster = cluster - 1;
> > @@ -364,6 +364,8 @@ static int coroutine_fn 
> > backup_run_incremental(BackupBlockJob *job)
> >           job->common.offset += ((end - last_cluster - 1) * job->cluster_size);
> >       }
> >   
> > +out:
> > +    bdrv_dirty_iter_free(dbi);
> >       return ret;
> >   }
> >   
> > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> > index 4902ca5..ec073ee 100644
> > --- a/block/dirty-bitmap.c
> > +++ b/block/dirty-bitmap.c
> > @@ -42,9 +42,15 @@ struct BdrvDirtyBitmap {
> >       char *name;                 /* Optional non-empty unique ID */
> >       int64_t size;               /* Size of the bitmap (Number of sectors) */
> >       bool disabled;              /* Bitmap is read-only */
> > +    int active_iterators;       /* How many iterators are active */
> >       QLIST_ENTRY(BdrvDirtyBitmap) list;
> >   };
> >   
> > +struct BdrvDirtyBitmapIter {
> > +    HBitmapIter hbi;
> > +    BdrvDirtyBitmap *bitmap;
> > +};
> > +
> >   BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char 
> > *name)
> >   {
> >       BdrvDirtyBitmap *bm;
> > @@ -212,6 +218,7 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
> >   
> >       QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> >           assert(!bdrv_dirty_bitmap_frozen(bitmap));
> > +        assert(!bitmap->active_iterators);
> >           hbitmap_truncate(bitmap->bitmap, size);
> >           bitmap->size = size;
> >       }
> > @@ -224,6 +231,7 @@ static void 
> > bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
> >       BdrvDirtyBitmap *bm, *next;
> >       QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
> >           if ((!bitmap || bm == bitmap) && (!only_named || bm->name)) {
> > +            assert(!bitmap->active_iterators);
> No good -- this function is also used to clear out all named bitmaps
> belonging to a BDS, so 'bitmap' may be NULL, so this assertion is bad.

Just a note about this. I do not like this hidden behaviour of 
release_bitmap, as it more native for freeing functions to do nothing 
with NULL pointer.. g_free(NULL) do not free all allocations))).. If 
someone agrees, this may be better to be changed.

> Fixing up in my local branch.
>
> --js
>
> >               assert(!bdrv_dirty_bitmap_frozen(bm));
> >               QLIST_REMOVE(bm, list);
> >               hbitmap_free(bm->bitmap);
> > @@ -320,9 +328,29 @@ uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap 
> > *bitmap)
> >       return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
> >   }
> >   
> > -void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
> > +BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
> > +                                         uint64_t first_sector)
> >   {
> > -    hbitmap_iter_init(hbi, bitmap->bitmap, 0);
> > +    BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
> > +    hbitmap_iter_init(&iter->hbi, bitmap->bitmap, first_sector);
> > +    iter->bitmap = bitmap;
> > +    bitmap->active_iterators++;
> > +    return iter;
> > +}
> > +
> > +void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)
> > +{
> > +    if (!iter) {
> > +        return;
> > +    }
> > +    assert(iter->bitmap->active_iterators > 0);
> > +    iter->bitmap->active_iterators--;
> > +    g_free(iter);
> > +}
> > +
> > +int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
> > +{
> > +    return hbitmap_iter_next(&iter->hbi);
> >   }
> >   
> >   void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> > @@ -373,12 +401,11 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t 
> > cur_sector,
> >   }
> >   
> >   /**
> > - * Advance an HBitmapIter to an arbitrary offset.
> > + * Advance a BdrvDirtyBitmapIter to an arbitrary offset.
> >    */
> > -void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
> > +void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t sector_num)
> >   {
> > -    assert(hbi->hb);
> > -    hbitmap_iter_init(hbi, hbi->hb, offset);
> > +    hbitmap_iter_init(&iter->hbi, iter->hbi.hb, sector_num);
> >   }
> >   
> >   int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
> > diff --git a/block/mirror.c b/block/mirror.c
> > index 80fd3c7..1d90aa5 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -52,7 +52,7 @@ typedef struct MirrorBlockJob {
> >       int64_t bdev_length;
> >       unsigned long *cow_bitmap;
> >       BdrvDirtyBitmap *dirty_bitmap;
> > -    HBitmapIter hbi;
> > +    BdrvDirtyBitmapIter *dbi;
> >       uint8_t *buf;
> >       QSIMPLEQ_HEAD(, MirrorBuffer) buf_free;
> >       int buf_free_count;
> > @@ -317,10 +317,10 @@ static uint64_t coroutine_fn 
> > mirror_iteration(MirrorBlockJob *s)
> >       int64_t end = s->bdev_length / BDRV_SECTOR_SIZE;
> >       int sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
> >   
> > -    sector_num = hbitmap_iter_next(&s->hbi);
> > +    sector_num = bdrv_dirty_iter_next(s->dbi);
> >       if (sector_num < 0) {
> > -        bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
> > -        sector_num = hbitmap_iter_next(&s->hbi);
> > +        bdrv_set_dirty_iter(s->dbi, 0);
> > +        sector_num = bdrv_dirty_iter_next(s->dbi);
> >           trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap));
> >           assert(sector_num >= 0);
> >       }
> > @@ -334,7 +334,7 @@ static uint64_t coroutine_fn 
> > mirror_iteration(MirrorBlockJob *s)
> >       /* Find the number of consective dirty chunks following the first dirty
> >        * one, and wait for in flight requests in them. */
> >       while (nb_chunks * sectors_per_chunk < (s->buf_size >> BDRV_SECTOR_BITS)) 
> > {
> > -        int64_t hbitmap_next;
> > +        int64_t next_dirty;
> >           int64_t next_sector = sector_num + nb_chunks * sectors_per_chunk;
> >           int64_t next_chunk = next_sector / sectors_per_chunk;
> >           if (next_sector >= end ||
> > @@ -345,13 +345,13 @@ static uint64_t coroutine_fn 
> > mirror_iteration(MirrorBlockJob *s)
> >               break;
> >           }
> >   
> > -        hbitmap_next = hbitmap_iter_next(&s->hbi);
> > -        if (hbitmap_next > next_sector || hbitmap_next < 0) {
> > +        next_dirty = bdrv_dirty_iter_next(s->dbi);
> > +        if (next_dirty > next_sector || next_dirty < 0) {
> >               /* The bitmap iterator's cache is stale, refresh it */
> > -            bdrv_set_dirty_iter(&s->hbi, next_sector);
> > -            hbitmap_next = hbitmap_iter_next(&s->hbi);
> > +            bdrv_set_dirty_iter(s->dbi, next_sector);
> > +            next_dirty = bdrv_dirty_iter_next(s->dbi);
> >           }
> > -        assert(hbitmap_next == next_sector);
> > +        assert(next_dirty == next_sector);
> >           nb_chunks++;
> >       }
> >   
> > @@ -601,7 +601,8 @@ static void coroutine_fn mirror_run(void *opaque)
> >           }
> >       }
> >   
> > -    bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
> > +    assert(!s->dbi);
> > +    s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap, 0);
> >       for (;;) {
> >           uint64_t delay_ns = 0;
> >           int64_t cnt;
> > @@ -712,6 +713,7 @@ immediate_exit:
> >       qemu_vfree(s->buf);
> >       g_free(s->cow_bitmap);
> >       g_free(s->in_flight_bitmap);
> > +    bdrv_dirty_iter_free(s->dbi);
> >       bdrv_release_dirty_bitmap(bs, s->dirty_bitmap);
> >   
> >       data = g_malloc(sizeof(*data));
> > diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> > index 80afe60..2ea601b 100644
> > --- a/include/block/dirty-bitmap.h
> > +++ b/include/block/dirty-bitmap.h
> > @@ -36,8 +36,11 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> >                              int64_t cur_sector, int nr_sectors);
> >   void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> >                                int64_t cur_sector, int nr_sectors);
> > -void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
> > -void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
> > +BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
> > +                                         uint64_t first_sector);
> > +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);
> >   int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
> >   void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
> >   
> > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> > index b113fcf..1b8c30a 100644
> > --- a/include/qemu/typedefs.h
> > +++ b/include/qemu/typedefs.h
> > @@ -11,6 +11,7 @@ typedef struct AioContext AioContext;
> >   typedef struct AllwinnerAHCIState AllwinnerAHCIState;
> >   typedef struct AudioState AudioState;
> >   typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
> > +typedef struct BdrvDirtyBitmapIter BdrvDirtyBitmapIter;
> >   typedef struct BlockBackend BlockBackend;
> >   typedef struct BlockBackendRootState BlockBackendRootState;
> >   typedef struct BlockDriverState BlockDriverState;


--
Best regards,
Vladimir