20.09.2018 22:40, Eric Blake wrote: > [reviving an old patch] > > On 1/16/18 6:54 AM, Vladimir Sementsov-Ogievskiy wrote: >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >> --- >> 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). >
or at least we should check count in tests/test-hbitmap.. About bitmap api: can we now switch naming style from '_' to '-' and drop x- prefixes? Or we are waiting for your libvirt API proposal acceptance? -- Best regards, Vladimir
