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

2016-07-13 Thread Max Reitz 
On 13.07.2016 09:57, Vladimir Sementsov-Ogievskiy wrote:
> 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 
>>> ---
>>>   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(-)
>>>

[...]

>>> @@ -224,6 +231,7 @@ static void
>>> bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
>>>   BdrvDirtyBitmap *bm, *next;
>>>   QLIST_FOREACH_SAFE(bm, >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.

The function is not called "bdrv_release_dirty_bitmap()", though, but
"bdrv_do_release_matching_dirty_bitmap()". The "do" means that it's an
internal function, called only by bdrv_release_dirty_bitmap() (aha!) and
bdrv_release_named_dirty_bitmaps(); and the "matching" means that if you
pass NULL, it'll release all bitmaps.

What we could do is make bdrv_release_dirty_bitmap() a no-op if NULL is
passed, or add an assertion that the argument is not NULL. I'd be fine
with either, but I don't think bdrv_do_release_matching_dirty_bitmap()
needs to be adjusted.

Max



signature.asc
Description: OpenPGP digital signature

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

2016-07-13 Thread 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 
---
  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, );
+dbi = bdrv_dirty_iter_new(job->sync_bitmap, 0);
  
  /* Find the next dirty sector(s) */

-while ((sector = hbitmap_iter_next()) != -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, _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(, 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, >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, >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

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

2016-07-12 Thread John Snow 


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 
> ---
>  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, );
> +dbi = bdrv_dirty_iter_new(job->sync_bitmap, 0);
>  
>  /* Find the next dirty sector(s) */
> -while ((sector = hbitmap_iter_next()) != -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, _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(, 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, >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, >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.

Fixing up in my local branch.

--js

>