bitmap: Update count after a merge
PATCH v2 1/1 bitmap: Update count after a merge
From: Eric Blake <[email protected]>
We need an accurate count of the number of bits set in a bitmap after a merge. In particular, since the merge operation short-circuits a merge from an empty source, if you have bitmaps A, B, and C where B started empty, then merge C into B, and B into A, an inaccurate count meant that A did not get the contents of C.
In the worst case, we may falsely regard the bitmap as empty when it has had new writes merged into it.
---
v2: based off of Eric's cover letter, now rebased properly
on top of the jsnow/bitmaps staging branch to use the correct bitmap target (result).
util/hbitmap.c | 3 +++ 1 file changed, 3 insertions(+)
util/hbitmap.c +3 -0
@@ -759,6 +759,9 @@ 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; }
Signature · 1 line
2.14.4
Quoted 8 lines · John Snow
From: Eric Blake <[email protected]>
We need an accurate count of the number of bits set in a bitmap after a merge. In particular, since the merge operation short-circuits a merge from an empty source, if you have bitmaps A, B, and C where B started empty, then merge C into B, and B into A, an inaccurate count meant that A did not get the contents of C.
Quoted 7 lines
In the worst case, we may falsely regard the bitmap as empty when it has had new writes merged into it.
Fixes be58721dbSigned-off-by Eric Blake <[email protected]>Signed-off-by John Snow <[email protected]>
Hm, I rememberd:
commit 3260cdfffbf00f33923f5f9f6bef45932d7ac28b
Author: Liang Li <[email protected]>
Date: Wed Feb 7 11:35:49 2018 -0500
hbitmap: fix missing restore count when finish deserialization
The .count of HBitmap is forgot to set in function hbitmap_deserialize_finish, let's set it to the right value.
- I always forget to update this field.. We definitely should add some
generic check on it somewhere, at least in tests.
Quoted 24 lines
---
v2: based off of Eric's cover letter, now rebased properly
on top of the jsnow/bitmaps staging branch to use the correct bitmap target (result).util/hbitmap.c | 3 +++ 1 file changed, 3 insertions(+)util/hbitmap.c +3
-0@@ -759,6 +759,9 @@ 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; }
Signature · 2 lines
Best regards, Vladimir
Quoted 24 lines · Vladimir Sementsov-Ogievskiy
Quoted 8 lines · John Snow
From: Eric Blake <[email protected]>
We need an accurate count of the number of bits set in a bitmap after a merge. In particular, since the merge operation short-circuits a merge from an empty source, if you have bitmaps A, B, and C where B started empty, then merge C into B, and B into A, an inaccurate count meant that A did not get the contents of C.
Quoted 7 lines
In the worst case, we may falsely regard the bitmap as empty when it has had new writes merged into it.
Fixes be58721dbSigned-off-by Eric Blake <[email protected]>Signed-off-by John Snow <[email protected]>Reviewed-by Vladimir Sementsov-Ogievskiy <[email protected]>Hm, I rememberd:
commit3260cdfffbf00f33923f5f9f6bef45932d7ac28b
Author: Liang Li <[email protected]>
Date: Wed Feb 7 11:35:49 2018 -0500
Quoted 8 lines
hbitmap: fix missing restore count when finish deserialization
The .count of HBitmap is forgot to set in function
hbitmap_deserialize_finish, let's set it to the right value.- I always forget to update this field.. We definitely should add some
generic check on it somewhere, at least in tests.
My suggestion (in another thread) was to enhance x-debug-block-dirty-bitmap-sha256 to include 'count' alongside the checksum, to make it easier to write such tests.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Quoted 31 lines · Eric Blake
Quoted 30 lines · Vladimir Sementsov-Ogievskiy
Quoted 8 lines · John Snow
From: Eric Blake <[email protected]>
We need an accurate count of the number of bits set in a bitmap after a merge. In particular, since the merge operation short-circuits a merge from an empty source, if you have bitmaps A, B, and C where B started empty, then merge C into B, and B into A, an inaccurate count meant that A did not get the contents of C.
Quoted 7 lines
In the worst case, we may falsely regard the bitmap as empty when it has had new writes merged into it.
Fixes be58721dbSigned-off-by Eric Blake <[email protected]>Signed-off-by John Snow <[email protected]>Reviewed-by Vladimir Sementsov-Ogievskiy <[email protected]>Hm, I rememberd:
commit3260cdfffbf00f33923f5f9f6bef45932d7ac28b
Author: Liang Li <[email protected]>
Date: Wed Feb 7 11:35:49 2018 -0500hbitmap: fix missing restore count when finish deserialization
The .count of HBitmap is forgot to set in function
hbitmap_deserialize_finish, let's set it to the right value.
Quoted 7 lines
Quoted 2 lines
- I always forget to update this field.. We definitely should add some generic check on it somewhere, at least in tests.
My suggestion (in another thread) was to enhance x-debug-block-dirty-bitmap-sha256 to include 'count' alongside the checksum, to make it easier to write such tests.
I suppose this is in preference to query-block? It would make it easier.
--js
Quoted 9 lines · John Snow
Quoted 7 lines
Quoted 2 lines
- I always forget to update this field.. We definitely should add some
generic check on it somewhere, at least in tests.My suggestion (in another thread) was to enhance x-debug-block-dirty-bitmap-sha256 to include 'count' alongside the checksum, to make it easier to write such tests.
I suppose this is in preference to query-block? It would make it easier.
Well, query-block remains the canonical way for users to access the count, while x-debug-block-dirty-bitmap-sha256 is for debugging only (basically, for our unit tests, as no one outside of qemu will get much use out of it). But yes, writing a test that uses bitmaps on an unconnected BDS is easier than a test that has to use a named device, since query-block won't list nodes that aren't connected to a named device, and requires a lot more parsing to get at the actual bitmap information in comparison to the debug command directly telling you about a single bitmap.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Quoted 13 lines · Eric Blake
Quoted 10 lines · John Snow
Quoted 8 lines
Quoted 3 lines
- I always forget to update this field.. We definitely should add some
generic check on it somewhere, at least in tests.My suggestion (in another thread) was to enhance x-debug-block-dirty-bitmap-sha256 to include 'count' alongside the checksum, to make it easier to write such tests.
I suppose this is in preference to query-block? It would make it easier.
Quoted 10 lines
Well, query-block remains the canonical way for users to access the count, while x-debug-block-dirty-bitmap-sha256 is for debugging only (basically, for our unit tests, as no one outside of qemu will get much use out of it). But yes, writing a test that uses bitmaps on an unconnected BDS is easier than a test that has to use a named device, since query-block won't list nodes that aren't connected to a named device, and requires a lot more parsing to get at the actual bitmap information in comparison to the debug command directly telling you about a single bitmap.
I think the best place is bitmap unit test, which is tests/test-bitmaps, and we don't need additional api
Signature · 2 lines
Best regards, Vladimir
John Snow
AppliedQuoted 31 lines · John Snow
From: Eric Blake <[email protected]>
We need an accurate count of the number of bits set in a bitmap after a merge. In particular, since the merge operation short-circuits a merge from an empty source, if you have bitmaps A, B, and C where B started empty, then merge C into B, and B into A, an inaccurate count meant that A did not get the contents of C.
In the worst case, we may falsely regard the bitmap as empty when it has had new writes merged into it.
Fixes be58721dbSigned-off-by Eric Blake <[email protected]>Signed-off-by John Snow <[email protected]>---
v2: based off of Eric's cover letter, now rebased properly
on top of the jsnow/bitmaps staging branch to use the correct bitmap target (result).util/hbitmap.c | 3 +++ 1 file changed, 3 insertions(+)util/hbitmap.c +0
-0
util/hbitmap.c bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result) · line 767
Quoted 9 lines
util/hbitmap.c +3
-0@@ -759,6 +759,9 @@ 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; }
Tests on the way, but for now...
Thanks, applied to my bitmaps tree:
https://github.com/jnsnow/qemu/commits/bitmaps
https://github.com/jnsnow/qemu.git
--js