Re: [Qemu-devel] [PATCH 3/3] block: per caller dirty bitmap

[Fam Zheng <famz@redhat.com>]

On 10/30/2013 03:49 PM, Paolo Bonzini wrote:
> Il 30/10/2013 08:08, Fam Zheng ha scritto:
>> Previously a BlockDriverState has only one dirty bitmap, so only one
>> caller (e.g. a block job) can keep track of writing. This changes the
>> dirty bitmap to a list and creates one HBitmap for each caller, the
>> lifecycle is managed with these new functions:
>>
>>      bdrv_create_dirty_bitmap
>>      bdrv_release_dirty_bitmap
>>
>> In place of this one:
>>
>>      bdrv_set_dirty_tracking
>>
>> An HBitmap pointer argument is added to these functions, since each
>> caller has its own dirty bitmap:
>>
>>      bdrv_get_dirty
>>      bdrv_dirty_iter_init
>>      bdrv_get_dirty_count
>>
>> While bdrv_set_dirty and bdrv_reset_dirty prototypes unchanged but
>> internally walk the list of all dirty bitmaps and set them one by one.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>   block-migration.c         | 22 ++++++++++----
>>   block.c                   | 74 ++++++++++++++++++++++++++---------------------
>>   block/mirror.c            | 23 ++++++++-------
>>   block/qapi.c              |  8 -----
>>   include/block/block.h     | 11 ++++---
>>   include/block/block_int.h |  2 +-
>>   6 files changed, 78 insertions(+), 62 deletions(-)
>>
>> diff --git a/block-migration.c b/block-migration.c
>> index daf9ec1..08df056 100644
>> --- a/block-migration.c
>> +++ b/block-migration.c
>> @@ -58,6 +58,7 @@ typedef struct BlkMigDevState {
>>       /* Protected by block migration lock.  */
>>       unsigned long *aio_bitmap;
>>       int64_t completed_sectors;
>> +    HBitmap *dirty_bitmap;
>>   } BlkMigDevState;
>>   
>>   typedef struct BlkMigBlock {
>> @@ -309,12 +310,21 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
>>   
>>   /* Called with iothread lock taken.  */
>>   
>> -static void set_dirty_tracking(int enable)
>> +static void set_dirty_tracking(void)
>>   {
>>       BlkMigDevState *bmds;
>>   
>>       QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
>> -        bdrv_set_dirty_tracking(bmds->bs, enable ? BLOCK_SIZE : 0);
>> +        bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE);
>> +    }
>> +}
>> +
>> +static void unset_dirty_tracking(void)
>> +{
>> +    BlkMigDevState *bmds;
>> +
>> +    QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
>> +        bdrv_release_dirty_bitmap(bmds->bs, bmds->dirty_bitmap);
>>       }
>>   }
>>   
>> @@ -432,7 +442,7 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
>>           } else {
>>               blk_mig_unlock();
>>           }
>> -        if (bdrv_get_dirty(bmds->bs, sector)) {
>> +        if (bdrv_get_dirty(bmds->bs, bmds->dirty_bitmap, sector)) {
>>   
>>               if (total_sectors - sector < BDRV_SECTORS_PER_DIRTY_CHUNK) {
>>                   nr_sectors = total_sectors - sector;
>> @@ -554,7 +564,7 @@ static int64_t get_remaining_dirty(void)
>>       int64_t dirty = 0;
>>   
>>       QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
>> -        dirty += bdrv_get_dirty_count(bmds->bs);
>> +        dirty += bdrv_get_dirty_count(bmds->bs, bmds->dirty_bitmap);
>>       }
>>   
>>       return dirty << BDRV_SECTOR_BITS;
>> @@ -569,7 +579,7 @@ static void blk_mig_cleanup(void)
>>   
>>       bdrv_drain_all();
>>   
>> -    set_dirty_tracking(0);
>> +    unset_dirty_tracking();
>>   
>>       blk_mig_lock();
>>       while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
>> @@ -604,7 +614,7 @@ static int block_save_setup(QEMUFile *f, void *opaque)
>>       init_blk_migration(f);
>>   
>>       /* start track dirty blocks */
>> -    set_dirty_tracking(1);
>> +    set_dirty_tracking();
>>       qemu_mutex_unlock_iothread();
>>   
>>       ret = flush_blks(f);
>> diff --git a/block.c b/block.c
>> index fd05a80..9975428 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -323,6 +323,7 @@ BlockDriverState *bdrv_new(const char *device_name)
>>       BlockDriverState *bs;
>>   
>>       bs = g_malloc0(sizeof(BlockDriverState));
>> +    QLIST_INIT(&bs->dirty_bitmaps);
>>       pstrcpy(bs->device_name, sizeof(bs->device_name), device_name);
>>       if (device_name[0] != '\0') {
>>           QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
>> @@ -1614,7 +1615,7 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
>>       bs_dest->iostatus           = bs_src->iostatus;
>>   
>>       /* dirty bitmap */
>> -    bs_dest->dirty_bitmap       = bs_src->dirty_bitmap;
>> +    bs_dest->dirty_bitmaps      = bs_src->dirty_bitmaps;
>>   
>>       /* reference count */
>>       bs_dest->refcnt             = bs_src->refcnt;
>> @@ -1647,7 +1648,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
>>   
>>       /* bs_new must be anonymous and shouldn't have anything fancy enabled */
>>       assert(bs_new->device_name[0] == '\0');
>> -    assert(bs_new->dirty_bitmap == NULL);
>> +    assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
>>       assert(bs_new->job == NULL);
>>       assert(bs_new->dev == NULL);
>>       assert(bs_new->in_use == 0);
>> @@ -1708,6 +1709,7 @@ static void bdrv_delete(BlockDriverState *bs)
>>       assert(!bs->job);
>>       assert(!bs->in_use);
>>       assert(!bs->refcnt);
>> +    assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>>   
>>       bdrv_close(bs);
>>   
>> @@ -2784,9 +2786,7 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs,
>>           ret = bdrv_co_flush(bs);
>>       }
>>   
>> -    if (bs->dirty_bitmap) {
>>           bdrv_set_dirty(bs, sector_num, nb_sectors);
>> -    }
>>   
>>       if (bs->wr_highest_sector < sector_num + nb_sectors - 1) {
>>           bs->wr_highest_sector = sector_num + nb_sectors - 1;
>> @@ -3321,7 +3321,7 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>>       if (bdrv_check_request(bs, sector_num, nb_sectors))
>>           return -EIO;
>>   
>> -    assert(!bs->dirty_bitmap);
>> +    assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>>   
>>       return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
>>   }
>> @@ -4181,9 +4181,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>>           return -EROFS;
>>       }
>>   
>> -    if (bs->dirty_bitmap) {
>> -        bdrv_reset_dirty(bs, sector_num, nb_sectors);
>> -    }
>> +    bdrv_reset_dirty(bs, sector_num, nb_sectors);
>>   
>>       /* Do nothing if disabled.  */
>>       if (!(bs->open_flags & BDRV_O_UNMAP)) {
>> @@ -4345,58 +4343,68 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov)
>>       return true;
>>   }
>>   
>> -void bdrv_set_dirty_tracking(BlockDriverState *bs, int granularity)
>> +HBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity)
>>   {
>>       int64_t bitmap_size;
>> +    HBitmap *bitmap;
>>   
>>       assert((granularity & (granularity - 1)) == 0);
>>   
>> -    if (granularity) {
>> -        granularity >>= BDRV_SECTOR_BITS;
>> -        assert(!bs->dirty_bitmap);
>> -        bitmap_size = (bdrv_getlength(bs) >> BDRV_SECTOR_BITS);
>> -        bs->dirty_bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
>> -    } else {
>> -        if (bs->dirty_bitmap) {
>> -            hbitmap_free(bs->dirty_bitmap);
>> -            bs->dirty_bitmap = NULL;
>> +    granularity >>= BDRV_SECTOR_BITS;
>> +    assert(granularity);
>> +    bitmap_size = (bdrv_getlength(bs) >> BDRV_SECTOR_BITS);
>> +    bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
>> +    QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
>> +    return bitmap;
>> +}
>> +
>> +void bdrv_release_dirty_bitmap(BlockDriverState *bs, HBitmap *bitmap)
>> +{
>> +    HBitmap *bm, *next;
>> +    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
>> +        if (bm == bitmap) {
>> +            QLIST_REMOVE(bitmap, list);
>> +            hbitmap_free(bitmap);
>> +            return;
>>           }
>>       }
>>   }
>>   
>> -int bdrv_get_dirty(BlockDriverState *bs, int64_t sector)
>> +int bdrv_get_dirty(BlockDriverState *bs, HBitmap *bitmap, int64_t sector)
>>   {
>> -    if (bs->dirty_bitmap) {
>> -        return hbitmap_get(bs->dirty_bitmap, sector);
>> +    if (bitmap) {
>> +        return hbitmap_get(bitmap, sector);
>>       } else {
>>           return 0;
>>       }
>>   }
>>   
>> -void bdrv_dirty_iter_init(BlockDriverState *bs, HBitmapIter *hbi)
>> +void bdrv_dirty_iter_init(BlockDriverState *bs,
>> +                          HBitmap *bitmap, HBitmapIter *hbi)
>>   {
>> -    hbitmap_iter_init(hbi, bs->dirty_bitmap, 0);
>> +    hbitmap_iter_init(hbi, bitmap, 0);
>>   }
>>   
>>   void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
>>                       int nr_sectors)
>>   {
>> -    hbitmap_set(bs->dirty_bitmap, cur_sector, nr_sectors);
>> +    HBitmap *bitmap;
>> +    QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
>> +        hbitmap_set(bitmap, cur_sector, nr_sectors);
>> +    }
>>   }
>>   
>> -void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
>> -                      int nr_sectors)
>> +void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors)
>>   {
>> -    hbitmap_reset(bs->dirty_bitmap, cur_sector, nr_sectors);
>> +    HBitmap *bitmap;
>> +    QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
>> +        hbitmap_reset(bitmap, cur_sector, nr_sectors);
>> +    }
>>   }
> I think callers outside block.c should only call
> hbitmap_set/hbitmap_reset; resetting is typically done before processing
> sectors and setting after an error (both of which happen privately to
> each task).
>
> Thus you probably should add a fourth patch which makes
> bdrv_(re)set_dirty static and remove
> bdrv_get_dirty/bdrv_dirty_iter_init/bdrv_get_dirty_count.
I like the idea of adding a wrapper struct (will be BdrvDirtyBitmap) to 
HBitmap so that patch 1 is not needed, and HBitmap becomes (almost) 
internal to block.c.

But I'm not sure removing 
bdrv_get_dirty/bdrv_dirty_iter_init/bdrv_get_dirty_count is good, as we 
are exposing BdrvDirtyBitmap, we should also provide operations on it, 
instead of let callers to handle HBitmap pointer inside. In other words, 
I prefer to define BdrvDirtyBitmap structure in block.c and only put a 
type declaration in header.

Fam