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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

On 28.06.2016 18:36, Vladimir Sementsov-Ogievskiy wrote:
> On 03.06.2016 07:32, 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 <famz@redhat.com>
>> ---
>>   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);
>>               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)
>
> can you use const BdrvDirtyBitmap * pointer ? To allow iterating 
> through const bitmap, in store_to_file function foe ex.

I see, no, because of 'bitmap->active_iterators++'...

>
>>   {
>> -    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