Re: [Qemu-devel] [PATCH v4 06/15] block: Hide HBitmap in block dirty bitmap interface

From: Max Reitz <mreitz@redhat.com>
To: Fam Zheng <famz@redhat.com>, qemu-devel@nongnu.org
Cc: kwolf@redhat.com,
	Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	jsnow@redhat.com, qemu-block@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v4 06/15] block: Hide HBitmap in block dirty bitmap interface
Date: Fri, 11 Mar 2016 15:27:51 +0100	[thread overview]
Message-ID: <56E2D5E7.4000105@redhat.com> (raw)
In-Reply-To: <56E2CE1D.4080502@redhat.com>

[-- Attachment #1.1: Type: text/plain, Size: 6095 bytes --]

On 11.03.2016 14:54, Max Reitz wrote:
> On 08.03.2016 05:44, 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.c.
> 
> It's block/dirty-bitmap.c now. :-)
> 
>> 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               | 15 +++++++++------
>>  include/block/dirty-bitmap.h |  7 +++++--
>>  include/qemu/typedefs.h      |  1 +
>>  5 files changed, 56 insertions(+), 20 deletions(-)
>>
> 
> [...]
> 
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 556e1d1..16f73b2 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
> 
> [...]
> 
>> @@ -373,12 +401,11 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
>>  }
>>  
>>  /**
>> - * Advance an HBitmapIter to an arbitrary offset.
>> + * Advance an BdrvDirtyBitmapIter to an arbitrary offset.
> 
> This should be "a BdrvDirtyBitmapIter".
> 
>>   */
>> -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 9635fa8..87a5a86 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
> 
> [...]
> 
>> @@ -304,10 +304,11 @@ 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_dirty_iter_free(s->dbi);
>> +        s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap, 0);
> 
> Works, but wouldn't bdrv_set_dirty_iter(s->dbi, 0); suffice?
> 
>> +        sector_num = bdrv_dirty_iter_next(s->dbi);
>>          trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap));
>>          assert(sector_num >= 0);
>>      }
>> @@ -330,7 +331,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>>              mirror_wait_for_io(s);
>>              /* Now retry.  */
>>          } else {
>> -            hbitmap_next = hbitmap_iter_next(&s->hbi);
>> +            hbitmap_next = bdrv_dirty_iter_next(s->dbi);
> 
> It would make sense to change this variable's name from hbitmap_next to
> e.g. bdrv_dirty_next or bs_next_dirty.
> 
>>              assert(hbitmap_next == next_sector);
>>              nb_chunks++;
>>          }
>> @@ -577,7 +578,8 @@ static void coroutine_fn mirror_run(void *opaque)
>>          }
>>      }
>>  
>> -    bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
>> +    bdrv_dirty_iter_free(s->dbi);
>> +    s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap, 0);
> 
> Again, bdrv_set_dirty_iter(s->dbi, 0); should work just fine.

OK, here it wouldn't because s->dbi is still NULL. Why do we need the
bdrv_dirty_iter_free(), then? It wasn't present in v3.

Max

> Since this patch is technically correct, I could still imagine giving an
> R-b, but I'm really reluctant to do so because of the
> bdrv_dirty_iter_free()/bdrv_dirty_iter_new() pairs.
> 
> Max
> 
>>      for (;;) {
>>          uint64_t delay_ns = 0;
>>          int64_t cnt;
>> @@ -688,6 +690,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);
>>      if (s->target->blk) {
>>          blk_iostatus_disable(s->target->blk);
>> 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 fd039e0..7c39f0f 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;
>>
> 
> 

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]