On 2/15/22 20:53, Vladimir Sementsov-Ogievskiy wrote:
We have too much logic to simply check that bitmaps are of the same
size. Let's just define that hbitmap_merge() and
bdrv_dirty_bitmap_merge_internal() require their argument bitmaps be of
same size, this simplifies things.
Let's look through the callers:
For backup_init_bcs_bitmap() we already assert that merge can't fail.
In bdrv_reclaim_dirty_bitmap_locked() we gracefully handle the error
that can't happen: successor always has same size as its parent, drop
this logic.
In bdrv_merge_dirty_bitmap() we already has assertion and separate
check. Make the check explicit and improve error message.
Signed-off-by: Vladimir Sementsov-Ogievskiy<vsement...@virtuozzo.com>
---
include/block/block_int.h | 2 +-
include/qemu/hbitmap.h | 15 ++-------------
block/backup.c | 6 ++----
block/dirty-bitmap.c | 25 ++++++++++---------------
util/hbitmap.c | 25 +++++++------------------
5 files changed, 22 insertions(+), 51 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 27008cfb22..cc40b6363d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1382,7 +1382,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t offset,
int64_t bytes);
void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup);
-bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
+void bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
const BdrvDirtyBitmap *src,
HBitmap **backup, bool lock);
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 5e71b6d6f7..4dc1c6ad14 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -76,20 +76,9 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size);
*
* Store result of merging @a and @b into @result.
* @result is allowed to be equal to @a or @b.
- *
- * Return true if the merge was successful,
- * false if it was not attempted.
+ * All bitmaps must have same size.
*/
-bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result);
-
-/**
- * hbitmap_can_merge:
- *
- * hbitmap_can_merge(a, b) && hbitmap_can_merge(a, result) is sufficient and
- * necessary for hbitmap_merge will not fail.
- *
- */
-bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b);
+void hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result);
/**
* hbitmap_empty:
diff --git a/block/backup.c b/block/backup.c
index 21d5983779..fb3d4b0e13 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -228,15 +228,13 @@ out:
static void backup_init_bcs_bitmap(BackupBlockJob *job)
{
- bool ret;
uint64_t estimate;
BdrvDirtyBitmap *bcs_bitmap = block_copy_dirty_bitmap(job->bcs);
if (job->sync_mode == MIRROR_SYNC_MODE_BITMAP) {
bdrv_clear_dirty_bitmap(bcs_bitmap, NULL);
- ret = bdrv_dirty_bitmap_merge_internal(bcs_bitmap, job->sync_bitmap,
- NULL, true);
- assert(ret);
+ bdrv_dirty_bitmap_merge_internal(bcs_bitmap, job->sync_bitmap, NULL,
+ true);
} else if (job->sync_mode == MIRROR_SYNC_MODE_TOP) {
/*
* We can't hog the coroutine to initialize this thoroughly.
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index d16b96ee62..9d803fcda3 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -309,10 +309,7 @@ BdrvDirtyBitmap
*bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *parent,
return NULL;
}
- if (!hbitmap_merge(parent->bitmap, successor->bitmap, parent->bitmap)) {
- error_setg(errp, "Merging of parent and successor bitmap failed");
- return NULL;
- }
+ hbitmap_merge(parent->bitmap, successor->bitmap, parent->bitmap);
parent->disabled = successor->disabled;
parent->busy = false;
@@ -899,13 +896,14 @@ bool bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const
BdrvDirtyBitmap *src,
goto out;
}
- if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) {
- error_setg(errp, "Bitmaps are incompatible and can't be merged");
+ if (bdrv_dirty_bitmap_size(src) != bdrv_dirty_bitmap_size(dest)) {
+ error_setg(errp, "Bitmaps are of different sizes (destination size is
%"
+ PRId64 ", source size is %" PRId64 ") and can't be merged",
+ bdrv_dirty_bitmap_size(dest), bdrv_dirty_bitmap_size(src));
goto out;
}
- ret = bdrv_dirty_bitmap_merge_internal(dest, src, backup, false);
- assert(ret);
+ bdrv_dirty_bitmap_merge_internal(dest, src, backup, false);
out:
bdrv_dirty_bitmaps_unlock(dest->bs);
@@ -919,18 +917,17 @@ out:
/**
* bdrv_dirty_bitmap_merge_internal: merge src into dest.
* Does NOT check bitmap permissions; not suitable for use as public API.
+ * @dest, @src and @backup (if not NULL) must have same size.
*
* @backup: If provided, make a copy of dest here prior to merge.
* @lock: If true, lock and unlock bitmaps on the way in/out.
* returns true if the merge succeeded; false if unattempted.
*/
There is no return value after this change.
-bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
+void bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
const BdrvDirtyBitmap *src,
HBitmap **backup,
bool lock)
{
- bool ret;
-
assert(!bdrv_dirty_bitmap_readonly(dest));
assert(!bdrv_dirty_bitmap_inconsistent(dest));
assert(!bdrv_dirty_bitmap_inconsistent(src));
@@ -945,9 +942,9 @@ bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
if (backup) {
*backup = dest->bitmap;
dest->bitmap = hbitmap_alloc(dest->size,
hbitmap_granularity(*backup));
- ret = hbitmap_merge(*backup, src->bitmap, dest->bitmap);
+ hbitmap_merge(*backup, src->bitmap, dest->bitmap);
} else {
- ret = hbitmap_merge(dest->bitmap, src->bitmap, dest->bitmap);
+ hbitmap_merge(dest->bitmap, src->bitmap, dest->bitmap);
}
if (lock) {
@@ -956,6 +953,4 @@ bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
bdrv_dirty_bitmaps_unlock(src->bs);
}
}
-
- return ret;
}
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 305b894a63..d0aaf205ed 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -840,11 +840,6 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
}
}
-bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b)
-{
- return (a->orig_size == b->orig_size);
-}
-
/**
* hbitmap_sparse_merge: performs dst = dst | src
* works with differing granularities.
@@ -868,28 +863,24 @@ static void hbitmap_sparse_merge(HBitmap *dst, const
HBitmap *src)
* Given HBitmaps A and B, let R := A (BITOR) B.
* Bitmaps A and B will not be modified,
* except when bitmap R is an alias of A or B.
- *
- * @return true if the merge was successful,
- * false if it was not attempted.
+ * Bitmaps must have same size.
*/
-bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
+void hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
{
int i;
uint64_t j;
- if (!hbitmap_can_merge(a, b) || !hbitmap_can_merge(a, result)) {
- return false;
- }
- assert(hbitmap_can_merge(b, result));
+ assert(a->orig_size == result->orig_size);
+ assert(b->orig_size == result->orig_size);
When hbitmap_merge() is called it is impossible to know if 'a' and 'b' have
the same orig_size. Of course there is workaround as it is shown using
bdrv_dirty_bitmap_size. However one function for checking looks to me better
than workaround. May be I missed the idea so I don't insist on it moreover.
if ((!hbitmap_count(a) && result == b) ||
(!hbitmap_count(b) && result == a)) {
- return true;
+ return;
}
if (!hbitmap_count(a) && !hbitmap_count(b)) {
hbitmap_reset_all(result);
- return true;
+ return;
}
if (a->granularity != b->granularity) {
@@ -902,7 +893,7 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b,
HBitmap *result)
if (b != result) {
hbitmap_sparse_merge(result, b);
}
- return true;
+ return;
}
/* This merge is O(size), as BITS_PER_LONG and HBITMAP_LEVELS are constant.
@@ -918,8 +909,6 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b,
HBitmap *result)
/* Recompute the dirty count */
result->count = hb_count_between(result, 0, result->size - 1);
-
- return true;
}
char *hbitmap_sha256(const HBitmap *bitmap, Error **errp)
With changed description:
Reviewed-by: Nikita Lapshin<nikita.laps...@virtuozzo.com>
