qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 5/6] qapi: add block-dirty-bitmap-merge


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v2 5/6] qapi: add block-dirty-bitmap-merge
Date: Thu, 20 Sep 2018 14:40:18 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0

[reviving an old patch]

On 1/16/18 6:54 AM, Vladimir Sementsov-Ogievskiy wrote:
Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
  qapi/block-core.json         | 38 ++++++++++++++++++++++++++++++++++++++
  include/block/dirty-bitmap.h |  2 ++
  block/dirty-bitmap.c         | 18 ++++++++++++++++++
  blockdev.c                   | 30 ++++++++++++++++++++++++++++++
  4 files changed, 88 insertions(+)


We've already hit a couple issues with this patch mentioned in other threads:

diff --git a/qapi/block-core.json b/qapi/block-core.json
index b3851ee760..9f9cfa0a44 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1604,6 +1604,20 @@
              '*persistent': 'bool', '*autoload': 'bool' } }
##
+# @BlockDirtyBitmapMerge:
+#
+# @node: name of device/node which the bitmap is tracking
+#
+# @dst_name: name of the destination dirty bitmap
+#
+# @src_name: name of the source dirty bitmap

Naming choice (prefer - over _):
https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg02352.html

+++ b/block/dirty-bitmap.c
@@ -699,3 +699,21 @@ char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap 
*bitmap, Error **errp)
  {
      return hbitmap_sha256(bitmap->bitmap, errp);
  }
+
+void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
+                             Error **errp)
+{
+    /* only bitmaps from one bds are supported */
+    assert(dest->mutex == src->mutex);
+
+    qemu_mutex_lock(dest->mutex);
+
+    assert(bdrv_dirty_bitmap_enabled(dest));

Merging to a disabled bitmap is desirable:
https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg02358.html

+    assert(!bdrv_dirty_bitmap_readonly(dest));
+
+    if (!hbitmap_merge(dest->bitmap, src->bitmap)) {

And here's another one I ran into. hbitmap_merge() does NOT properly update hbitmap_count(), which means statistics about the number of bits set in the bitmap, as visible through QMP "query-block", are inaccurate after a merge.

Check out hbitmap_set() and hbitmap_reset(), which carefully recompute hb->count using hb_count_between() as part of flipping bits. But since hbitmap_merge() fails to recompute bits, various other aspects of the code go haywire (for example, code that uses hbitmap_empty() or hbitmap_count()==0 to short-circuit operations [and hbitmap_merge() is one such caller] makes the wrong choice on a bitmap that started life empty, and had a non-empty bitmap merged in, because hb->count is still 0).

I think that x-debug-block-dirty-bitmap-sha256 should be enhanced to output count as well as the checksum of the bitmap contents, and then that we need iotests coverage of x-block-dirty-bitmap-merge, complete with a check that the count is properly updated. iotests 165, 169, 176, 199 all perform bitmap checksum validations, but so far none of them perform a bitmap merge. Be careful in writing any such test that we don't re-introduce any sensitivities on 32- vs. 64-bit or endianness in our checksum computation (see commit 2807746f for comparison).

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



reply via email to

[Prev in Thread] Current Thread [Next in Thread]