* [PATCH 01/10] hbitmap: introduce HBITMAP_MAX_ORIG_SIZE
2019-09-30 15:14 [PATCH 00/10] Further bitmaps improvements Vladimir Sementsov-Ogievskiy
@ 2019-09-30 15:14 ` Vladimir Sementsov-Ogievskiy
2019-10-09 15:34 ` Eric Blake
2019-09-30 15:14 ` [PATCH 02/10] hbitmap: move hbitmap_iter_next_word to hbitmap.c Vladimir Sementsov-Ogievskiy
` (8 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-30 15:14 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, fam, vsementsov, qemu-devel, mreitz, den, jsnow
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/qemu/hbitmap.h | 7 +++++++
util/hbitmap.c | 2 ++
2 files changed, 9 insertions(+)
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 1bf944ca3d..82317c5364 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -33,6 +33,13 @@ typedef struct HBitmapIter HBitmapIter;
*/
#define HBITMAP_LEVELS ((HBITMAP_LOG_MAX_SIZE / BITS_PER_LEVEL) + 1)
+/*
+ * We have APIs which returns signed int64_t, to be able to return error.
+ * Therefore we can't handle bitmaps with absolute size larger than
+ * (INT64_MAX+1). Still, keep it INT64_MAX to be a bit safer.
+ */
+#define HBITMAP_MAX_ORIG_SIZE INT64_MAX
+
struct HBitmapIter {
const HBitmap *hb;
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 757d39e360..df192234e3 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -708,6 +708,7 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
HBitmap *hb = g_new0(struct HBitmap, 1);
unsigned i;
+ assert(size <= HBITMAP_MAX_ORIG_SIZE);
hb->orig_size = size;
assert(granularity >= 0 && granularity < 64);
@@ -738,6 +739,7 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
uint64_t num_elements = size;
uint64_t old;
+ assert(size <= HBITMAP_MAX_ORIG_SIZE);
hb->orig_size = size;
/* Size comes in as logical elements, adjust for granularity. */
--
2.21.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 01/10] hbitmap: introduce HBITMAP_MAX_ORIG_SIZE
2019-09-30 15:14 ` [PATCH 01/10] hbitmap: introduce HBITMAP_MAX_ORIG_SIZE Vladimir Sementsov-Ogievskiy
@ 2019-10-09 15:34 ` Eric Blake
2019-10-09 16:04 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 17+ messages in thread
From: Eric Blake @ 2019-10-09 15:34 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, fam, qemu-devel, mreitz, den, jsnow
On 9/30/19 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
A bit light on the commit message for explaining why.
> ---
> include/qemu/hbitmap.h | 7 +++++++
> util/hbitmap.c | 2 ++
> 2 files changed, 9 insertions(+)
>
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index 1bf944ca3d..82317c5364 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -33,6 +33,13 @@ typedef struct HBitmapIter HBitmapIter;
> */
> #define HBITMAP_LEVELS ((HBITMAP_LOG_MAX_SIZE / BITS_PER_LEVEL) + 1)
>
> +/*
> + * We have APIs which returns signed int64_t, to be able to return error.
> + * Therefore we can't handle bitmaps with absolute size larger than
> + * (INT64_MAX+1). Still, keep it INT64_MAX to be a bit safer.
> + */
> +#define HBITMAP_MAX_ORIG_SIZE INT64_MAX
That, and bitmaps represent disk images, but disk images can't exceed
INT64_MAX bytes (thanks to off_t being signed). But does introducing a
new constant really help?
> +
> struct HBitmapIter {
> const HBitmap *hb;
>
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 757d39e360..df192234e3 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -708,6 +708,7 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
> HBitmap *hb = g_new0(struct HBitmap, 1);
> unsigned i;
>
> + assert(size <= HBITMAP_MAX_ORIG_SIZE);
or can we just inline INT64_MAX here?
> hb->orig_size = size;
>
> assert(granularity >= 0 && granularity < 64);
> @@ -738,6 +739,7 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
> uint64_t num_elements = size;
> uint64_t old;
>
> + assert(size <= HBITMAP_MAX_ORIG_SIZE);
> hb->orig_size = size;
>
> /* Size comes in as logical elements, adjust for granularity. */
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 01/10] hbitmap: introduce HBITMAP_MAX_ORIG_SIZE
2019-10-09 15:34 ` Eric Blake
@ 2019-10-09 16:04 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-10-09 16:04 UTC (permalink / raw)
To: Eric Blake, qemu-block; +Cc: kwolf, fam, Denis Lunev, qemu-devel, mreitz, jsnow
09.10.2019 18:34, Eric Blake wrote:
> On 9/30/19 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> A bit light on the commit message for explaining why.
If use just INT64_MAX, the comment below may be move here, accompanied by
your note about disk size limit.
>
>> ---
>> include/qemu/hbitmap.h | 7 +++++++
>> util/hbitmap.c | 2 ++
>> 2 files changed, 9 insertions(+)
>>
>> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
>> index 1bf944ca3d..82317c5364 100644
>> --- a/include/qemu/hbitmap.h
>> +++ b/include/qemu/hbitmap.h
>> @@ -33,6 +33,13 @@ typedef struct HBitmapIter HBitmapIter;
>> */
>> #define HBITMAP_LEVELS ((HBITMAP_LOG_MAX_SIZE / BITS_PER_LEVEL) + 1)
>> +/*
>> + * We have APIs which returns signed int64_t, to be able to return error.
>> + * Therefore we can't handle bitmaps with absolute size larger than
>> + * (INT64_MAX+1). Still, keep it INT64_MAX to be a bit safer.
>> + */
>> +#define HBITMAP_MAX_ORIG_SIZE INT64_MAX
>
> That, and bitmaps represent disk images, but disk images can't exceed INT64_MAX bytes (thanks to off_t being signed). But does introducing a new constant really help?
>
>> +
>> struct HBitmapIter {
>> const HBitmap *hb;
>> diff --git a/util/hbitmap.c b/util/hbitmap.c
>> index 757d39e360..df192234e3 100644
>> --- a/util/hbitmap.c
>> +++ b/util/hbitmap.c
>> @@ -708,6 +708,7 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
>> HBitmap *hb = g_new0(struct HBitmap, 1);
>> unsigned i;
>> + assert(size <= HBITMAP_MAX_ORIG_SIZE);
>
> or can we just inline INT64_MAX here?
OK for me too.
>
>> hb->orig_size = size;
>> assert(granularity >= 0 && granularity < 64);
>> @@ -738,6 +739,7 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
>> uint64_t num_elements = size;
>> uint64_t old;
>> + assert(size <= HBITMAP_MAX_ORIG_SIZE);
>> hb->orig_size = size;
>> /* Size comes in as logical elements, adjust for granularity. */
>>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 02/10] hbitmap: move hbitmap_iter_next_word to hbitmap.c
2019-09-30 15:14 [PATCH 00/10] Further bitmaps improvements Vladimir Sementsov-Ogievskiy
2019-09-30 15:14 ` [PATCH 01/10] hbitmap: introduce HBITMAP_MAX_ORIG_SIZE Vladimir Sementsov-Ogievskiy
@ 2019-09-30 15:14 ` Vladimir Sementsov-Ogievskiy
2019-09-30 15:14 ` [PATCH 03/10] hbitmap: unpublish hbitmap_iter_skip_words Vladimir Sementsov-Ogievskiy
` (7 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-30 15:14 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, fam, vsementsov, qemu-devel, mreitz, den, jsnow
The function is definitely internal (it's not used by third party and
it has complicated interface). Move it to .c file.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/qemu/hbitmap.h | 30 ------------------------------
util/hbitmap.c | 29 +++++++++++++++++++++++++++++
2 files changed, 29 insertions(+), 30 deletions(-)
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 82317c5364..d9a792b067 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -369,34 +369,4 @@ void hbitmap_free_meta(HBitmap *hb);
*/
int64_t hbitmap_iter_next(HBitmapIter *hbi);
-/**
- * hbitmap_iter_next_word:
- * @hbi: HBitmapIter to operate on.
- * @p_cur: Location where to store the next non-zero word.
- *
- * Return the index of the next nonzero word that is set in @hbi's
- * associated HBitmap, and set *p_cur to the content of that word
- * (bits before the index that was passed to hbitmap_iter_init are
- * trimmed on the first call). Return -1, and set *p_cur to zero,
- * if all remaining words are zero.
- */
-static inline size_t hbitmap_iter_next_word(HBitmapIter *hbi, unsigned long *p_cur)
-{
- unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1];
-
- if (cur == 0) {
- cur = hbitmap_iter_skip_words(hbi);
- if (cur == 0) {
- *p_cur = 0;
- return -1;
- }
- }
-
- /* The next call will resume work from the next word. */
- hbi->cur[HBITMAP_LEVELS - 1] = 0;
- *p_cur = cur;
- return hbi->pos;
-}
-
-
#endif
diff --git a/util/hbitmap.c b/util/hbitmap.c
index df192234e3..2799f51b62 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -298,6 +298,35 @@ uint64_t hbitmap_count(const HBitmap *hb)
return hb->count << hb->granularity;
}
+/**
+ * hbitmap_iter_next_word:
+ * @hbi: HBitmapIter to operate on.
+ * @p_cur: Location where to store the next non-zero word.
+ *
+ * Return the index of the next nonzero word that is set in @hbi's
+ * associated HBitmap, and set *p_cur to the content of that word
+ * (bits before the index that was passed to hbitmap_iter_init are
+ * trimmed on the first call). Return -1, and set *p_cur to zero,
+ * if all remaining words are zero.
+ */
+static size_t hbitmap_iter_next_word(HBitmapIter *hbi, unsigned long *p_cur)
+{
+ unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1];
+
+ if (cur == 0) {
+ cur = hbitmap_iter_skip_words(hbi);
+ if (cur == 0) {
+ *p_cur = 0;
+ return -1;
+ }
+ }
+
+ /* The next call will resume work from the next word. */
+ hbi->cur[HBITMAP_LEVELS - 1] = 0;
+ *p_cur = cur;
+ return hbi->pos;
+}
+
/* Count the number of set bits between start and end, not accounting for
* the granularity. Also an example of how to use hbitmap_iter_next_word.
*/
--
2.21.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 03/10] hbitmap: unpublish hbitmap_iter_skip_words
2019-09-30 15:14 [PATCH 00/10] Further bitmaps improvements Vladimir Sementsov-Ogievskiy
2019-09-30 15:14 ` [PATCH 01/10] hbitmap: introduce HBITMAP_MAX_ORIG_SIZE Vladimir Sementsov-Ogievskiy
2019-09-30 15:14 ` [PATCH 02/10] hbitmap: move hbitmap_iter_next_word to hbitmap.c Vladimir Sementsov-Ogievskiy
@ 2019-09-30 15:14 ` Vladimir Sementsov-Ogievskiy
2019-09-30 15:14 ` [PATCH 04/10] hbitmap: drop meta bitmaps as they are unused Vladimir Sementsov-Ogievskiy
` (6 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-30 15:14 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, fam, vsementsov, qemu-devel, mreitz, den, jsnow
Function is internal and even commented as internal. Drop its
definition from .h file.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/qemu/hbitmap.h | 7 -------
util/hbitmap.c | 2 +-
2 files changed, 1 insertion(+), 8 deletions(-)
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index d9a792b067..ba2cdaeaa0 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -304,13 +304,6 @@ void hbitmap_free(HBitmap *hb);
*/
void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first);
-/* hbitmap_iter_skip_words:
- * @hbi: HBitmapIter to operate on.
- *
- * Internal function used by hbitmap_iter_next and hbitmap_iter_next_word.
- */
-unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi);
-
/* hbitmap_next_zero:
*
* Find next not dirty bit within selected range. If not found, return -1.
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 2799f51b62..574eb13ed8 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -104,7 +104,7 @@ struct HBitmap {
/* Advance hbi to the next nonzero word and return it. hbi->pos
* is updated. Returns zero if we reach the end of the bitmap.
*/
-unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
+static unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
{
size_t pos = hbi->pos;
const HBitmap *hb = hbi->hb;
--
2.21.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 04/10] hbitmap: drop meta bitmaps as they are unused
2019-09-30 15:14 [PATCH 00/10] Further bitmaps improvements Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2019-09-30 15:14 ` [PATCH 03/10] hbitmap: unpublish hbitmap_iter_skip_words Vladimir Sementsov-Ogievskiy
@ 2019-09-30 15:14 ` Vladimir Sementsov-Ogievskiy
2019-09-30 15:14 ` [PATCH 05/10] block/dirty-bitmap: switch _next_dirty_area and _next_zero to int64_t Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-30 15:14 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, fam, vsementsov, qemu-devel, mreitz, den, jsnow
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/qemu/hbitmap.h | 21 --------
tests/test-hbitmap.c | 115 -----------------------------------------
util/hbitmap.c | 16 ------
3 files changed, 152 deletions(-)
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index ba2cdaeaa0..cf3dbf8b39 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -332,27 +332,6 @@ int64_t hbitmap_next_zero(const HBitmap *hb, uint64_t start, uint64_t count);
bool hbitmap_next_dirty_area(const HBitmap *hb, uint64_t *start,
uint64_t *count);
-/* hbitmap_create_meta:
- * Create a "meta" hbitmap to track dirtiness of the bits in this HBitmap.
- * The caller owns the created bitmap and must call hbitmap_free_meta(hb) to
- * free it.
- *
- * Currently, we only guarantee that if a bit in the hbitmap is changed it
- * will be reflected in the meta bitmap, but we do not yet guarantee the
- * opposite.
- *
- * @hb: The HBitmap to operate on.
- * @chunk_size: How many bits in @hb does one bit in the meta track.
- */
-HBitmap *hbitmap_create_meta(HBitmap *hb, int chunk_size);
-
-/* hbitmap_free_meta:
- * Free the meta bitmap of @hb.
- *
- * @hb: The HBitmap whose meta bitmap should be freed.
- */
-void hbitmap_free_meta(HBitmap *hb);
-
/**
* hbitmap_iter_next:
* @hbi: HBitmapIter to operate on.
diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index e1f867085f..aeaa0b3f22 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -22,7 +22,6 @@
typedef struct TestHBitmapData {
HBitmap *hb;
- HBitmap *meta;
unsigned long *bits;
size_t size;
size_t old_size;
@@ -94,14 +93,6 @@ static void hbitmap_test_init(TestHBitmapData *data,
}
}
-static void hbitmap_test_init_meta(TestHBitmapData *data,
- uint64_t size, int granularity,
- int meta_chunk)
-{
- hbitmap_test_init(data, size, granularity);
- data->meta = hbitmap_create_meta(data->hb, meta_chunk);
-}
-
static inline size_t hbitmap_test_array_size(size_t bits)
{
size_t n = DIV_ROUND_UP(bits, BITS_PER_LONG);
@@ -144,9 +135,6 @@ static void hbitmap_test_teardown(TestHBitmapData *data,
const void *unused)
{
if (data->hb) {
- if (data->meta) {
- hbitmap_free_meta(data->hb);
- }
hbitmap_free(data->hb);
data->hb = NULL;
}
@@ -648,96 +636,6 @@ static void test_hbitmap_truncate_shrink_large(TestHBitmapData *data,
hbitmap_test_truncate(data, size, -diff, 0);
}
-static void hbitmap_check_meta(TestHBitmapData *data,
- int64_t start, int count)
-{
- int64_t i;
-
- for (i = 0; i < data->size; i++) {
- if (i >= start && i < start + count) {
- g_assert(hbitmap_get(data->meta, i));
- } else {
- g_assert(!hbitmap_get(data->meta, i));
- }
- }
-}
-
-static void hbitmap_test_meta(TestHBitmapData *data,
- int64_t start, int count,
- int64_t check_start, int check_count)
-{
- hbitmap_reset_all(data->hb);
- hbitmap_reset_all(data->meta);
-
- /* Test "unset" -> "unset" will not update meta. */
- hbitmap_reset(data->hb, start, count);
- hbitmap_check_meta(data, 0, 0);
-
- /* Test "unset" -> "set" will update meta */
- hbitmap_set(data->hb, start, count);
- hbitmap_check_meta(data, check_start, check_count);
-
- /* Test "set" -> "set" will not update meta */
- hbitmap_reset_all(data->meta);
- hbitmap_set(data->hb, start, count);
- hbitmap_check_meta(data, 0, 0);
-
- /* Test "set" -> "unset" will update meta */
- hbitmap_reset_all(data->meta);
- hbitmap_reset(data->hb, start, count);
- hbitmap_check_meta(data, check_start, check_count);
-}
-
-static void hbitmap_test_meta_do(TestHBitmapData *data, int chunk_size)
-{
- uint64_t size = chunk_size * 100;
- hbitmap_test_init_meta(data, size, 0, chunk_size);
-
- hbitmap_test_meta(data, 0, 1, 0, chunk_size);
- hbitmap_test_meta(data, 0, chunk_size, 0, chunk_size);
- hbitmap_test_meta(data, chunk_size - 1, 1, 0, chunk_size);
- hbitmap_test_meta(data, chunk_size - 1, 2, 0, chunk_size * 2);
- hbitmap_test_meta(data, chunk_size - 1, chunk_size + 1, 0, chunk_size * 2);
- hbitmap_test_meta(data, chunk_size - 1, chunk_size + 2, 0, chunk_size * 3);
- hbitmap_test_meta(data, 7 * chunk_size - 1, chunk_size + 2,
- 6 * chunk_size, chunk_size * 3);
- hbitmap_test_meta(data, size - 1, 1, size - chunk_size, chunk_size);
- hbitmap_test_meta(data, 0, size, 0, size);
-}
-
-static void test_hbitmap_meta_byte(TestHBitmapData *data, const void *unused)
-{
- hbitmap_test_meta_do(data, BITS_PER_BYTE);
-}
-
-static void test_hbitmap_meta_word(TestHBitmapData *data, const void *unused)
-{
- hbitmap_test_meta_do(data, BITS_PER_LONG);
-}
-
-static void test_hbitmap_meta_sector(TestHBitmapData *data, const void *unused)
-{
- hbitmap_test_meta_do(data, BDRV_SECTOR_SIZE * BITS_PER_BYTE);
-}
-
-/**
- * Create an HBitmap and test set/unset.
- */
-static void test_hbitmap_meta_one(TestHBitmapData *data, const void *unused)
-{
- int i;
- int64_t offsets[] = {
- 0, 1, L1 - 1, L1, L1 + 1, L2 - 1, L2, L2 + 1, L3 - 1, L3, L3 + 1
- };
-
- hbitmap_test_init_meta(data, L3 * 2, 0, 1);
- for (i = 0; i < ARRAY_SIZE(offsets); i++) {
- hbitmap_test_meta(data, offsets[i], 1, offsets[i], 1);
- hbitmap_test_meta(data, offsets[i], L1, offsets[i], L1);
- hbitmap_test_meta(data, offsets[i], L2, offsets[i], L2);
- }
-}
-
static void test_hbitmap_serialize_align(TestHBitmapData *data,
const void *unused)
{
@@ -750,13 +648,6 @@ static void test_hbitmap_serialize_align(TestHBitmapData *data,
g_assert_cmpint(r, ==, 64 << 3);
}
-static void test_hbitmap_meta_zero(TestHBitmapData *data, const void *unused)
-{
- hbitmap_test_init_meta(data, 0, 0, 1);
-
- hbitmap_check_meta(data, 0, 0);
-}
-
static void hbitmap_test_serialize_range(TestHBitmapData *data,
uint8_t *buf, size_t buf_size,
uint64_t pos, uint64_t count)
@@ -1165,12 +1056,6 @@ int main(int argc, char **argv)
hbitmap_test_add("/hbitmap/truncate/shrink/large",
test_hbitmap_truncate_shrink_large);
- hbitmap_test_add("/hbitmap/meta/zero", test_hbitmap_meta_zero);
- hbitmap_test_add("/hbitmap/meta/one", test_hbitmap_meta_one);
- hbitmap_test_add("/hbitmap/meta/byte", test_hbitmap_meta_byte);
- hbitmap_test_add("/hbitmap/meta/word", test_hbitmap_meta_word);
- hbitmap_test_add("/hbitmap/meta/sector", test_hbitmap_meta_sector);
-
hbitmap_test_add("/hbitmap/serialize/align",
test_hbitmap_serialize_align);
hbitmap_test_add("/hbitmap/serialize/basic",
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 574eb13ed8..75d72d7203 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -897,22 +897,6 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result)
return true;
}
-HBitmap *hbitmap_create_meta(HBitmap *hb, int chunk_size)
-{
- assert(!(chunk_size & (chunk_size - 1)));
- assert(!hb->meta);
- hb->meta = hbitmap_alloc(hb->size << hb->granularity,
- hb->granularity + ctz32(chunk_size));
- return hb->meta;
-}
-
-void hbitmap_free_meta(HBitmap *hb)
-{
- assert(hb->meta);
- hbitmap_free(hb->meta);
- hb->meta = NULL;
-}
-
char *hbitmap_sha256(const HBitmap *bitmap, Error **errp)
{
size_t size = bitmap->sizes[HBITMAP_LEVELS - 1] * sizeof(unsigned long);
--
2.21.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 05/10] block/dirty-bitmap: switch _next_dirty_area and _next_zero to int64_t
2019-09-30 15:14 [PATCH 00/10] Further bitmaps improvements Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2019-09-30 15:14 ` [PATCH 04/10] hbitmap: drop meta bitmaps as they are unused Vladimir Sementsov-Ogievskiy
@ 2019-09-30 15:14 ` Vladimir Sementsov-Ogievskiy
2019-09-30 15:14 ` [PATCH 06/10] block/dirty-bitmap: add _next_dirty API Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-30 15:14 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, fam, vsementsov, qemu-devel, mreitz, den, jsnow
We are going to introduce bdrv_dirty_bitmap_next_dirty so that same
variable may be used to store its return value and to be its parameter,
so it would int64_t.
Similarly, we are going to refactor hbitmap_next_dirty_area to use
hbitmap_next_dirty together with hbitmap_next_zero, therefore we want
hbitmap_next_zero parameter type to be int64_t too.
So, for convenience update all parameters of *_next_zero and
*_next_dirty_area to be int64_t.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/block/dirty-bitmap.h | 6 +++---
include/qemu/hbitmap.h | 7 +++----
block/dirty-bitmap.c | 6 +++---
block/mirror.c | 4 ++--
nbd/server.c | 2 +-
tests/test-hbitmap.c | 32 ++++++++++++++++----------------
util/hbitmap.c | 13 ++++++++-----
7 files changed, 36 insertions(+), 34 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 958e7474fb..96d3cc0800 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -103,10 +103,10 @@ for (bitmap = bdrv_dirty_bitmap_first(bs); bitmap; \
bitmap = bdrv_dirty_bitmap_next(bitmap))
char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
-int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset,
- uint64_t bytes);
+int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, int64_t offset,
+ int64_t bytes);
bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
- uint64_t *offset, uint64_t *bytes);
+ int64_t *offset, int64_t *bytes);
BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
Error **errp);
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index cf3dbf8b39..6eb2728a21 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -311,10 +311,10 @@ void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first);
* @hb: The HBitmap to operate on
* @start: The bit to start from.
* @count: Number of bits to proceed. If @start+@count > bitmap size, the whole
- * bitmap is looked through. You can use UINT64_MAX as @count to search up to
+ * bitmap is looked through. You can use INT64_MAX as @count to search up to
* the bitmap end.
*/
-int64_t hbitmap_next_zero(const HBitmap *hb, uint64_t start, uint64_t count);
+int64_t hbitmap_next_zero(const HBitmap *hb, int64_t start, int64_t count);
/* hbitmap_next_dirty_area:
* @hb: The HBitmap to operate on
@@ -329,8 +329,7 @@ int64_t hbitmap_next_zero(const HBitmap *hb, uint64_t start, uint64_t count);
* @offset and @bytes appropriately. Otherwise returns false and leaves @offset
* and @bytes unchanged.
*/
-bool hbitmap_next_dirty_area(const HBitmap *hb, uint64_t *start,
- uint64_t *count);
+bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t *start, int64_t *count);
/**
* hbitmap_iter_next:
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 4bbb251b2c..01135f623e 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -854,14 +854,14 @@ char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp)
return hbitmap_sha256(bitmap->bitmap, errp);
}
-int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset,
- uint64_t bytes)
+int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, int64_t offset,
+ int64_t bytes)
{
return hbitmap_next_zero(bitmap->bitmap, offset, bytes);
}
bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
- uint64_t *offset, uint64_t *bytes)
+ int64_t *offset, int64_t *bytes)
{
return hbitmap_next_dirty_area(bitmap->bitmap, offset, bytes);
}
diff --git a/block/mirror.c b/block/mirror.c
index a6c50caea4..d6b3a898ed 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1182,8 +1182,8 @@ do_sync_target_write(MirrorBlockJob *job, MirrorMethod method,
QEMUIOVector *qiov, int flags)
{
QEMUIOVector target_qiov;
- uint64_t dirty_offset = offset;
- uint64_t dirty_bytes;
+ int64_t dirty_offset = offset;
+ int64_t dirty_bytes;
if (qiov) {
qemu_iovec_init(&target_qiov, qiov->niov);
diff --git a/nbd/server.c b/nbd/server.c
index d8d1e62455..cc0069c15b 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2040,7 +2040,7 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
bool next_dirty = !dirty;
if (dirty) {
- end = bdrv_dirty_bitmap_next_zero(bitmap, begin, UINT64_MAX);
+ end = bdrv_dirty_bitmap_next_zero(bitmap, begin, INT64_MAX);
} else {
bdrv_set_dirty_iter(it, begin);
end = bdrv_dirty_iter_next(it);
diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index aeaa0b3f22..0e1e5c64dd 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -837,7 +837,7 @@ static void test_hbitmap_next_zero_check_range(TestHBitmapData *data,
static void test_hbitmap_next_zero_check(TestHBitmapData *data, int64_t start)
{
- test_hbitmap_next_zero_check_range(data, start, UINT64_MAX);
+ test_hbitmap_next_zero_check_range(data, start, INT64_MAX);
}
static void test_hbitmap_next_zero_do(TestHBitmapData *data, int granularity)
@@ -905,11 +905,11 @@ static void test_hbitmap_next_zero_after_truncate(TestHBitmapData *data,
}
static void test_hbitmap_next_dirty_area_check(TestHBitmapData *data,
- uint64_t offset,
- uint64_t count)
+ int64_t offset,
+ int64_t count)
{
- uint64_t off1, off2;
- uint64_t len1 = 0, len2;
+ int64_t off1, off2;
+ int64_t len1 = 0, len2;
bool ret1, ret2;
int64_t end;
@@ -945,24 +945,24 @@ static void test_hbitmap_next_dirty_area_do(TestHBitmapData *data,
int granularity)
{
hbitmap_test_init(data, L3, granularity);
- test_hbitmap_next_dirty_area_check(data, 0, UINT64_MAX);
+ test_hbitmap_next_dirty_area_check(data, 0, INT64_MAX);
test_hbitmap_next_dirty_area_check(data, 0, 1);
test_hbitmap_next_dirty_area_check(data, L3 - 1, 1);
hbitmap_set(data->hb, L2, 1);
test_hbitmap_next_dirty_area_check(data, 0, 1);
test_hbitmap_next_dirty_area_check(data, 0, L2);
- test_hbitmap_next_dirty_area_check(data, 0, UINT64_MAX);
- test_hbitmap_next_dirty_area_check(data, L2 - 1, UINT64_MAX);
+ test_hbitmap_next_dirty_area_check(data, 0, INT64_MAX);
+ test_hbitmap_next_dirty_area_check(data, L2 - 1, INT64_MAX);
test_hbitmap_next_dirty_area_check(data, L2 - 1, 1);
test_hbitmap_next_dirty_area_check(data, L2 - 1, 2);
test_hbitmap_next_dirty_area_check(data, L2 - 1, 3);
- test_hbitmap_next_dirty_area_check(data, L2, UINT64_MAX);
+ test_hbitmap_next_dirty_area_check(data, L2, INT64_MAX);
test_hbitmap_next_dirty_area_check(data, L2, 1);
test_hbitmap_next_dirty_area_check(data, L2 + 1, 1);
hbitmap_set(data->hb, L2 + 5, L1);
- test_hbitmap_next_dirty_area_check(data, 0, UINT64_MAX);
+ test_hbitmap_next_dirty_area_check(data, 0, INT64_MAX);
test_hbitmap_next_dirty_area_check(data, L2 - 2, 8);
test_hbitmap_next_dirty_area_check(data, L2 + 1, 5);
test_hbitmap_next_dirty_area_check(data, L2 + 1, 3);
@@ -974,16 +974,16 @@ static void test_hbitmap_next_dirty_area_do(TestHBitmapData *data,
test_hbitmap_next_dirty_area_check(data, L2 + 1, 0);
hbitmap_set(data->hb, L2 * 2, L3 - L2 * 2);
- test_hbitmap_next_dirty_area_check(data, 0, UINT64_MAX);
- test_hbitmap_next_dirty_area_check(data, L2, UINT64_MAX);
- test_hbitmap_next_dirty_area_check(data, L2 + 1, UINT64_MAX);
- test_hbitmap_next_dirty_area_check(data, L2 + 5 + L1 - 1, UINT64_MAX);
+ test_hbitmap_next_dirty_area_check(data, 0, INT64_MAX);
+ test_hbitmap_next_dirty_area_check(data, L2, INT64_MAX);
+ test_hbitmap_next_dirty_area_check(data, L2 + 1, INT64_MAX);
+ test_hbitmap_next_dirty_area_check(data, L2 + 5 + L1 - 1, INT64_MAX);
test_hbitmap_next_dirty_area_check(data, L2 + 5 + L1, 5);
test_hbitmap_next_dirty_area_check(data, L2 * 2 - L1, L1 + 1);
test_hbitmap_next_dirty_area_check(data, L2 * 2, L2);
hbitmap_set(data->hb, 0, L3);
- test_hbitmap_next_dirty_area_check(data, 0, UINT64_MAX);
+ test_hbitmap_next_dirty_area_check(data, 0, INT64_MAX);
}
static void test_hbitmap_next_dirty_area_0(TestHBitmapData *data,
@@ -1010,7 +1010,7 @@ static void test_hbitmap_next_dirty_area_after_truncate(TestHBitmapData *data,
hbitmap_test_init(data, L1, 0);
hbitmap_test_truncate_impl(data, L1 * 2);
hbitmap_set(data->hb, L1 + 1, 1);
- test_hbitmap_next_dirty_area_check(data, 0, UINT64_MAX);
+ test_hbitmap_next_dirty_area_check(data, 0, INT64_MAX);
}
int main(int argc, char **argv)
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 75d72d7203..6526d7b6c1 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -193,7 +193,7 @@ void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first)
}
}
-int64_t hbitmap_next_zero(const HBitmap *hb, uint64_t start, uint64_t count)
+int64_t hbitmap_next_zero(const HBitmap *hb, int64_t start, int64_t count)
{
size_t pos = (start >> hb->granularity) >> BITS_PER_LEVEL;
unsigned long *last_lev = hb->levels[HBITMAP_LEVELS - 1];
@@ -202,6 +202,8 @@ int64_t hbitmap_next_zero(const HBitmap *hb, uint64_t start, uint64_t count)
uint64_t end_bit, sz;
int64_t res;
+ assert(start >= 0 && count >= 0);
+
if (start >= hb->orig_size || count == 0) {
return -1;
}
@@ -244,14 +246,15 @@ int64_t hbitmap_next_zero(const HBitmap *hb, uint64_t start, uint64_t count)
return res;
}
-bool hbitmap_next_dirty_area(const HBitmap *hb, uint64_t *start,
- uint64_t *count)
+bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t *start, int64_t *count)
{
HBitmapIter hbi;
int64_t firt_dirty_off, area_end;
uint32_t granularity = 1UL << hb->granularity;
uint64_t end;
+ assert(*start >= 0 && *count >= 0);
+
if (*start >= hb->orig_size || *count == 0) {
return false;
}
@@ -826,8 +829,8 @@ bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b)
*/
static void hbitmap_sparse_merge(HBitmap *dst, const HBitmap *src)
{
- uint64_t offset = 0;
- uint64_t count = src->orig_size;
+ int64_t offset = 0;
+ int64_t count = src->orig_size;
while (hbitmap_next_dirty_area(src, &offset, &count)) {
hbitmap_set(dst, offset, count);
--
2.21.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 06/10] block/dirty-bitmap: add _next_dirty API
2019-09-30 15:14 [PATCH 00/10] Further bitmaps improvements Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2019-09-30 15:14 ` [PATCH 05/10] block/dirty-bitmap: switch _next_dirty_area and _next_zero to int64_t Vladimir Sementsov-Ogievskiy
@ 2019-09-30 15:14 ` Vladimir Sementsov-Ogievskiy
2019-09-30 15:14 ` [PATCH 07/10] block/dirty-bitmap: improve _next_dirty_area API Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-30 15:14 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, fam, vsementsov, qemu-devel, mreitz, den, jsnow
We have bdrv_dirty_bitmap_next_zero, let's add corresponding
bdrv_dirty_bitmap_next_dirty, which is more comfortable to use than
bitmap iterators in some cases.
For test modify test_hbitmap_next_zero_check_range to check both
next_zero and next_dirty and add some new checks.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/block/dirty-bitmap.h | 2 +
include/qemu/hbitmap.h | 13 ++++
block/dirty-bitmap.c | 6 ++
tests/test-hbitmap.c | 130 ++++++++++++++++++++---------------
util/hbitmap.c | 60 ++++++++--------
5 files changed, 126 insertions(+), 85 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 96d3cc0800..333e44e793 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -103,6 +103,8 @@ for (bitmap = bdrv_dirty_bitmap_first(bs); bitmap; \
bitmap = bdrv_dirty_bitmap_next(bitmap))
char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
+int64_t bdrv_dirty_bitmap_next_dirty(BdrvDirtyBitmap *bitmap, int64_t offset,
+ int64_t bytes);
int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, int64_t offset,
int64_t bytes);
bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 6eb2728a21..eeecd33f01 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -304,6 +304,19 @@ void hbitmap_free(HBitmap *hb);
*/
void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first);
+/*
+ * hbitmap_next_dirty:
+ *
+ * Find next dirty bit within selected range. If not found, return -1.
+ *
+ * @hb: The HBitmap to operate on
+ * @start: The bit to start from.
+ * @count: Number of bits to proceed. If @start+@count > bitmap size, the whole
+ * bitmap is looked through. You can use UINT64_MAX as @count to search up to
+ * the bitmap end.
+ */
+int64_t hbitmap_next_dirty(const HBitmap *hb, int64_t start, int64_t count);
+
/* hbitmap_next_zero:
*
* Find next not dirty bit within selected range. If not found, return -1.
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 01135f623e..fe2420b297 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -854,6 +854,12 @@ char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp)
return hbitmap_sha256(bitmap->bitmap, errp);
}
+int64_t bdrv_dirty_bitmap_next_dirty(BdrvDirtyBitmap *bitmap, int64_t offset,
+ int64_t bytes)
+{
+ return hbitmap_next_dirty(bitmap->bitmap, offset, bytes);
+}
+
int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, int64_t offset,
int64_t bytes)
{
diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index 0e1e5c64dd..e3f1b3f361 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -816,92 +816,108 @@ static void test_hbitmap_iter_and_reset(TestHBitmapData *data,
hbitmap_iter_next(&hbi);
}
-static void test_hbitmap_next_zero_check_range(TestHBitmapData *data,
- uint64_t start,
- uint64_t count)
+static void test_hbitmap_next_x_check_range(TestHBitmapData *data,
+ uint64_t start,
+ uint64_t count)
{
- int64_t ret1 = hbitmap_next_zero(data->hb, start, count);
- int64_t ret2 = start;
+ int64_t next_zero = hbitmap_next_zero(data->hb, start, count);
+ int64_t next_dirty = hbitmap_next_dirty(data->hb, start, count);
+ int64_t next;
int64_t end = start >= data->size || data->size - start < count ?
data->size : start + count;
+ bool first_bit = hbitmap_get(data->hb, start);
- for ( ; ret2 < end && hbitmap_get(data->hb, ret2); ret2++) {
+ for (next = start;
+ next < end && hbitmap_get(data->hb, next) == first_bit;
+ next++)
+ {
;
}
- if (ret2 == end) {
- ret2 = -1;
+
+ if (next == end) {
+ next = -1;
}
- g_assert_cmpint(ret1, ==, ret2);
+ g_assert_cmpint(next_dirty, ==, first_bit ? start : next);
+ g_assert_cmpint(next_zero, ==, first_bit ? next : start);
}
-static void test_hbitmap_next_zero_check(TestHBitmapData *data, int64_t start)
+static void test_hbitmap_next_x_check(TestHBitmapData *data, int64_t start)
{
- test_hbitmap_next_zero_check_range(data, start, INT64_MAX);
+ test_hbitmap_next_x_check_range(data, start, INT64_MAX);
}
-static void test_hbitmap_next_zero_do(TestHBitmapData *data, int granularity)
+static void test_hbitmap_next_x_do(TestHBitmapData *data, int granularity)
{
hbitmap_test_init(data, L3, granularity);
- test_hbitmap_next_zero_check(data, 0);
- test_hbitmap_next_zero_check(data, L3 - 1);
- test_hbitmap_next_zero_check_range(data, 0, 1);
- test_hbitmap_next_zero_check_range(data, L3 - 1, 1);
+ test_hbitmap_next_x_check(data, 0);
+ test_hbitmap_next_x_check(data, L3 - 1);
+ test_hbitmap_next_x_check_range(data, 0, 1);
+ test_hbitmap_next_x_check_range(data, L3 - 1, 1);
hbitmap_set(data->hb, L2, 1);
- test_hbitmap_next_zero_check(data, 0);
- test_hbitmap_next_zero_check(data, L2 - 1);
- test_hbitmap_next_zero_check(data, L2);
- test_hbitmap_next_zero_check(data, L2 + 1);
- test_hbitmap_next_zero_check_range(data, 0, 1);
- test_hbitmap_next_zero_check_range(data, 0, L2);
- test_hbitmap_next_zero_check_range(data, L2 - 1, 1);
- test_hbitmap_next_zero_check_range(data, L2 - 1, 2);
- test_hbitmap_next_zero_check_range(data, L2, 1);
- test_hbitmap_next_zero_check_range(data, L2 + 1, 1);
+ test_hbitmap_next_x_check(data, 0);
+ test_hbitmap_next_x_check(data, L2 - 1);
+ test_hbitmap_next_x_check(data, L2);
+ test_hbitmap_next_x_check(data, L2 + 1);
+ test_hbitmap_next_x_check_range(data, 0, 1);
+ test_hbitmap_next_x_check_range(data, 0, L2);
+ test_hbitmap_next_x_check_range(data, L2 - 1, 1);
+ test_hbitmap_next_x_check_range(data, L2 - 1, 2);
+ test_hbitmap_next_x_check_range(data, L2, 1);
+ test_hbitmap_next_x_check_range(data, L2 + 1, 1);
hbitmap_set(data->hb, L2 + 5, L1);
- test_hbitmap_next_zero_check(data, 0);
- test_hbitmap_next_zero_check(data, L2 + 1);
- test_hbitmap_next_zero_check(data, L2 + 2);
- test_hbitmap_next_zero_check(data, L2 + 5);
- test_hbitmap_next_zero_check(data, L2 + L1 - 1);
- test_hbitmap_next_zero_check(data, L2 + L1);
- test_hbitmap_next_zero_check_range(data, L2, 6);
- test_hbitmap_next_zero_check_range(data, L2 + 1, 3);
- test_hbitmap_next_zero_check_range(data, L2 + 4, L1);
- test_hbitmap_next_zero_check_range(data, L2 + 5, L1);
+ test_hbitmap_next_x_check(data, 0);
+ test_hbitmap_next_x_check(data, L2 - L1);
+ test_hbitmap_next_x_check(data, L2 + 1);
+ test_hbitmap_next_x_check(data, L2 + 2);
+ test_hbitmap_next_x_check(data, L2 + 5);
+ test_hbitmap_next_x_check(data, L2 + L1 - 1);
+ test_hbitmap_next_x_check(data, L2 + L1);
+ test_hbitmap_next_x_check(data, L2 + L1 + 1);
+ test_hbitmap_next_x_check_range(data, L2 - 2, L1);
+ test_hbitmap_next_x_check_range(data, L2, 4);
+ test_hbitmap_next_x_check_range(data, L2, 6);
+ test_hbitmap_next_x_check_range(data, L2 + 1, 3);
+ test_hbitmap_next_x_check_range(data, L2 + 4, L1);
+ test_hbitmap_next_x_check_range(data, L2 + 5, L1);
+ test_hbitmap_next_x_check_range(data, L2 + 5 + L1 - 1, 1);
+ test_hbitmap_next_x_check_range(data, L2 + 5 + L1, 1);
+ test_hbitmap_next_x_check_range(data, L2 + 5 + L1 + 1, 1);
hbitmap_set(data->hb, L2 * 2, L3 - L2 * 2);
- test_hbitmap_next_zero_check(data, L2 * 2 - L1);
- test_hbitmap_next_zero_check(data, L2 * 2 - 2);
- test_hbitmap_next_zero_check(data, L2 * 2 - 1);
- test_hbitmap_next_zero_check(data, L2 * 2);
- test_hbitmap_next_zero_check(data, L3 - 1);
- test_hbitmap_next_zero_check_range(data, L2 * 2 - L1, L1 + 1);
- test_hbitmap_next_zero_check_range(data, L2 * 2, L2);
+ test_hbitmap_next_x_check(data, L2 * 2 - L1);
+ test_hbitmap_next_x_check(data, L2 * 2 - 2);
+ test_hbitmap_next_x_check(data, L2 * 2 - 1);
+ test_hbitmap_next_x_check(data, L2 * 2);
+ test_hbitmap_next_x_check(data, L2 * 2 + 1);
+ test_hbitmap_next_x_check(data, L2 * 2 + L1);
+ test_hbitmap_next_x_check(data, L3 - 1);
+ test_hbitmap_next_x_check_range(data, L2 * 2 - L1, L1 + 1);
+ test_hbitmap_next_x_check_range(data, L2 * 2, L2);
hbitmap_set(data->hb, 0, L3);
- test_hbitmap_next_zero_check(data, 0);
+ test_hbitmap_next_x_check(data, 0);
}
-static void test_hbitmap_next_zero_0(TestHBitmapData *data, const void *unused)
+static void test_hbitmap_next_x_0(TestHBitmapData *data, const void *unused)
{
- test_hbitmap_next_zero_do(data, 0);
+ test_hbitmap_next_x_do(data, 0);
}
-static void test_hbitmap_next_zero_4(TestHBitmapData *data, const void *unused)
+static void test_hbitmap_next_x_4(TestHBitmapData *data, const void *unused)
{
- test_hbitmap_next_zero_do(data, 4);
+ test_hbitmap_next_x_do(data, 4);
}
-static void test_hbitmap_next_zero_after_truncate(TestHBitmapData *data,
- const void *unused)
+static void test_hbitmap_next_x_after_truncate(TestHBitmapData *data,
+ const void *unused)
{
hbitmap_test_init(data, L1, 0);
hbitmap_test_truncate_impl(data, L1 * 2);
hbitmap_set(data->hb, 0, L1);
- test_hbitmap_next_zero_check(data, 0);
+ test_hbitmap_next_x_check(data, 0);
}
static void test_hbitmap_next_dirty_area_check(TestHBitmapData *data,
@@ -1068,12 +1084,12 @@ int main(int argc, char **argv)
hbitmap_test_add("/hbitmap/iter/iter_and_reset",
test_hbitmap_iter_and_reset);
- hbitmap_test_add("/hbitmap/next_zero/next_zero_0",
- test_hbitmap_next_zero_0);
- hbitmap_test_add("/hbitmap/next_zero/next_zero_4",
- test_hbitmap_next_zero_4);
- hbitmap_test_add("/hbitmap/next_zero/next_zero_after_truncate",
- test_hbitmap_next_zero_after_truncate);
+ hbitmap_test_add("/hbitmap/next_zero/next_x_0",
+ test_hbitmap_next_x_0);
+ hbitmap_test_add("/hbitmap/next_zero/next_x_4",
+ test_hbitmap_next_x_4);
+ hbitmap_test_add("/hbitmap/next_zero/next_x_after_truncate",
+ test_hbitmap_next_x_after_truncate);
hbitmap_test_add("/hbitmap/next_dirty_area/next_dirty_area_0",
test_hbitmap_next_dirty_area_0);
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 6526d7b6c1..c089020ca3 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -193,6 +193,30 @@ void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first)
}
}
+int64_t hbitmap_next_dirty(const HBitmap *hb, int64_t start, int64_t count)
+{
+ HBitmapIter hbi;
+ int64_t firt_dirty_off;
+ uint64_t end;
+
+ assert(start >= 0 && count >= 0);
+
+ if (start >= hb->orig_size || count == 0) {
+ return -1;
+ }
+
+ end = count > hb->orig_size - start ? hb->orig_size : start + count;
+
+ hbitmap_iter_init(&hbi, hb, start);
+ firt_dirty_off = hbitmap_iter_next(&hbi);
+
+ if (firt_dirty_off < 0 || firt_dirty_off >= end) {
+ return -1;
+ }
+
+ return MAX(start, firt_dirty_off);
+}
+
int64_t hbitmap_next_zero(const HBitmap *hb, int64_t start, int64_t count)
{
size_t pos = (start >> hb->granularity) >> BITS_PER_LEVEL;
@@ -248,40 +272,20 @@ int64_t hbitmap_next_zero(const HBitmap *hb, int64_t start, int64_t count)
bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t *start, int64_t *count)
{
- HBitmapIter hbi;
- int64_t firt_dirty_off, area_end;
- uint32_t granularity = 1UL << hb->granularity;
- uint64_t end;
-
- assert(*start >= 0 && *count >= 0);
-
- if (*start >= hb->orig_size || *count == 0) {
- return false;
- }
-
- end = *count > hb->orig_size - *start ? hb->orig_size : *start + *count;
-
- hbitmap_iter_init(&hbi, hb, *start);
- firt_dirty_off = hbitmap_iter_next(&hbi);
+ int64_t area_start, area_end;
- if (firt_dirty_off < 0 || firt_dirty_off >= end) {
+ area_start = hbitmap_next_dirty(hb, *start, *count);
+ if (area_start < 0) {
return false;
}
- if (firt_dirty_off + granularity >= end) {
- area_end = end;
- } else {
- area_end = hbitmap_next_zero(hb, firt_dirty_off + granularity,
- end - firt_dirty_off - granularity);
- if (area_end < 0) {
- area_end = end;
- }
+ area_end = hbitmap_next_zero(hb, area_start, *start + *count - area_start);
+ if (area_end < 0) {
+ area_end = MIN(hb->orig_size, *start + *count);
}
- if (firt_dirty_off > *start) {
- *start = firt_dirty_off;
- }
- *count = area_end - *start;
+ *start = area_start;
+ *count = area_end - area_start;
return true;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 07/10] block/dirty-bitmap: improve _next_dirty_area API
2019-09-30 15:14 [PATCH 00/10] Further bitmaps improvements Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2019-09-30 15:14 ` [PATCH 06/10] block/dirty-bitmap: add _next_dirty API Vladimir Sementsov-Ogievskiy
@ 2019-09-30 15:14 ` Vladimir Sementsov-Ogievskiy
2019-09-30 15:15 ` [PATCH 08/10] nbd/server: introduce NBDExtentArray Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-30 15:14 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, fam, vsementsov, qemu-devel, mreitz, den, jsnow
Firstly, _next_dirty_area is for scenarios when we may contiguously
search for next dirty area inside some limited region, so it is more
comfortable to specify "end" which should not be recalculated on each
iteration.
Secondly, mirror wants to limit resulting are, and for this thing it
limits @count. But actually mirror don't want to limit the whole region
to search, but only resulting sub-region. Add separate parameter
@max_dirty_count to satisfy such request.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/block/dirty-bitmap.h | 3 ++-
include/qemu/hbitmap.h | 25 ++++++++++++---------
block/dirty-bitmap.c | 6 +++--
block/mirror.c | 11 +++++----
tests/test-hbitmap.c | 43 +++++++++++++++++++++++-------------
util/hbitmap.c | 41 +++++++++++++++++++++-------------
6 files changed, 79 insertions(+), 50 deletions(-)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 333e44e793..a010110709 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -108,7 +108,8 @@ int64_t bdrv_dirty_bitmap_next_dirty(BdrvDirtyBitmap *bitmap, int64_t offset,
int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, int64_t offset,
int64_t bytes);
bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
- int64_t *offset, int64_t *bytes);
+ int64_t start, int64_t end, int64_t max_dirty_count,
+ int64_t *dirty_start, int64_t *dirty_count);
BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
Error **errp);
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index eeecd33f01..99dfcbd9e5 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -331,18 +331,21 @@ int64_t hbitmap_next_zero(const HBitmap *hb, int64_t start, int64_t count);
/* hbitmap_next_dirty_area:
* @hb: The HBitmap to operate on
- * @start: in-out parameter.
- * in: the offset to start from
- * out: (if area found) start of found area
- * @count: in-out parameter.
- * in: length of requested region
- * out: length of found area
- *
- * If dirty area found within [@start, @start + @count), returns true and sets
- * @offset and @bytes appropriately. Otherwise returns false and leaves @offset
- * and @bytes unchanged.
+ * @start: the offset to start from
+ * @end: end of requested area
+ * @max_dirty_count: limit for out parameter dirty_count
+ * @dirty_start: on success: start of found area
+ * @dirty_count: on success: length of found area
+ *
+ * If dirty area found within [@start, @end), returns true and sets
+ * @dirty_start and @dirty_count appropriately. @dirty_count will not exceed
+ * @max_dirty_count.
+ * If dirty area was not found, returns false and leaves @dirty_start and
+ * @dirty_count unchanged.
*/
-bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t *start, int64_t *count);
+bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t start, int64_t end,
+ int64_t max_dirty_count,
+ int64_t *dirty_start, int64_t *dirty_count);
/**
* hbitmap_iter_next:
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index fe2420b297..f1b552b7a2 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -867,9 +867,11 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, int64_t offset,
}
bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
- int64_t *offset, int64_t *bytes)
+ int64_t start, int64_t end, int64_t max_dirty_count,
+ int64_t *dirty_start, int64_t *dirty_count)
{
- return hbitmap_next_dirty_area(bitmap->bitmap, offset, bytes);
+ return hbitmap_next_dirty_area(bitmap->bitmap, start, end, max_dirty_count,
+ dirty_start, dirty_count);
}
/**
diff --git a/block/mirror.c b/block/mirror.c
index d6b3a898ed..c8947a5726 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1190,15 +1190,14 @@ do_sync_target_write(MirrorBlockJob *job, MirrorMethod method,
}
while (true) {
- bool valid_area;
+ bool found;
int ret;
bdrv_dirty_bitmap_lock(job->dirty_bitmap);
- dirty_bytes = MIN(offset + bytes - dirty_offset, INT_MAX);
- valid_area = bdrv_dirty_bitmap_next_dirty_area(job->dirty_bitmap,
- &dirty_offset,
- &dirty_bytes);
- if (!valid_area) {
+ found = bdrv_dirty_bitmap_next_dirty_area(job->dirty_bitmap,
+ dirty_offset, offset + bytes, INT_MAX,
+ &dirty_offset, &dirty_bytes);
+ if (!found) {
bdrv_dirty_bitmap_unlock(job->dirty_bitmap);
break;
}
diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index e3f1b3f361..d75e84a76a 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -920,18 +920,19 @@ static void test_hbitmap_next_x_after_truncate(TestHBitmapData *data,
test_hbitmap_next_x_check(data, 0);
}
-static void test_hbitmap_next_dirty_area_check(TestHBitmapData *data,
- int64_t offset,
- int64_t count)
+static void test_hbitmap_next_dirty_area_check_limited(TestHBitmapData *data,
+ int64_t offset,
+ int64_t count,
+ int64_t max_dirty)
{
int64_t off1, off2;
int64_t len1 = 0, len2;
bool ret1, ret2;
int64_t end;
- off1 = offset;
- len1 = count;
- ret1 = hbitmap_next_dirty_area(data->hb, &off1, &len1);
+ ret1 = hbitmap_next_dirty_area(data->hb,
+ offset, count == INT64_MAX ? INT64_MAX : offset + count, max_dirty,
+ &off1, &len1);
end = offset > data->size || data->size - offset < count ? data->size :
offset + count;
@@ -940,21 +941,25 @@ static void test_hbitmap_next_dirty_area_check(TestHBitmapData *data,
;
}
- for (len2 = 1; off2 + len2 < end && hbitmap_get(data->hb, off2 + len2);
- len2++) {
+ for (len2 = 1; (off2 + len2 < end && len2 < max_dirty &&
+ hbitmap_get(data->hb, off2 + len2)); len2++)
+ {
;
}
ret2 = off2 < end;
- if (!ret2) {
- /* leave unchanged */
- off2 = offset;
- len2 = count;
+ g_assert_cmpint(ret1, ==, ret2);
+
+ if (ret2) {
+ g_assert_cmpint(off1, ==, off2);
+ g_assert_cmpint(len1, ==, len2);
}
+}
- g_assert_cmpint(ret1, ==, ret2);
- g_assert_cmpint(off1, ==, off2);
- g_assert_cmpint(len1, ==, len2);
+static void test_hbitmap_next_dirty_area_check(TestHBitmapData *data,
+ int64_t offset, int64_t count)
+{
+ test_hbitmap_next_dirty_area_check_limited(data, offset, count, INT64_MAX);
}
static void test_hbitmap_next_dirty_area_do(TestHBitmapData *data,
@@ -964,6 +969,7 @@ static void test_hbitmap_next_dirty_area_do(TestHBitmapData *data,
test_hbitmap_next_dirty_area_check(data, 0, INT64_MAX);
test_hbitmap_next_dirty_area_check(data, 0, 1);
test_hbitmap_next_dirty_area_check(data, L3 - 1, 1);
+ test_hbitmap_next_dirty_area_check_limited(data, 0, INT64_MAX, 1);
hbitmap_set(data->hb, L2, 1);
test_hbitmap_next_dirty_area_check(data, 0, 1);
@@ -976,6 +982,8 @@ static void test_hbitmap_next_dirty_area_do(TestHBitmapData *data,
test_hbitmap_next_dirty_area_check(data, L2, INT64_MAX);
test_hbitmap_next_dirty_area_check(data, L2, 1);
test_hbitmap_next_dirty_area_check(data, L2 + 1, 1);
+ test_hbitmap_next_dirty_area_check_limited(data, 0, INT64_MAX, 1);
+ test_hbitmap_next_dirty_area_check_limited(data, L2 - 1, 2, 1);
hbitmap_set(data->hb, L2 + 5, L1);
test_hbitmap_next_dirty_area_check(data, 0, INT64_MAX);
@@ -988,6 +996,8 @@ static void test_hbitmap_next_dirty_area_do(TestHBitmapData *data,
test_hbitmap_next_dirty_area_check(data, L2 + L1, L1);
test_hbitmap_next_dirty_area_check(data, L2, 0);
test_hbitmap_next_dirty_area_check(data, L2 + 1, 0);
+ test_hbitmap_next_dirty_area_check_limited(data, L2 + 3, INT64_MAX, 3);
+ test_hbitmap_next_dirty_area_check_limited(data, L2 + 3, 7, 10);
hbitmap_set(data->hb, L2 * 2, L3 - L2 * 2);
test_hbitmap_next_dirty_area_check(data, 0, INT64_MAX);
@@ -997,6 +1007,9 @@ static void test_hbitmap_next_dirty_area_do(TestHBitmapData *data,
test_hbitmap_next_dirty_area_check(data, L2 + 5 + L1, 5);
test_hbitmap_next_dirty_area_check(data, L2 * 2 - L1, L1 + 1);
test_hbitmap_next_dirty_area_check(data, L2 * 2, L2);
+ test_hbitmap_next_dirty_area_check_limited(data, L2 * 2 + 1, INT64_MAX, 5);
+ test_hbitmap_next_dirty_area_check_limited(data, L2 * 2 + 1, 10, 5);
+ test_hbitmap_next_dirty_area_check_limited(data, L2 * 2 + 1, 2, 5);
hbitmap_set(data->hb, 0, L3);
test_hbitmap_next_dirty_area_check(data, 0, INT64_MAX);
diff --git a/util/hbitmap.c b/util/hbitmap.c
index c089020ca3..77a2d48e7d 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -270,22 +270,34 @@ int64_t hbitmap_next_zero(const HBitmap *hb, int64_t start, int64_t count)
return res;
}
-bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t *start, int64_t *count)
+bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t start, int64_t end,
+ int64_t max_dirty_count,
+ int64_t *dirty_start, int64_t *dirty_count)
{
- int64_t area_start, area_end;
+ int64_t next_zero;
- area_start = hbitmap_next_dirty(hb, *start, *count);
- if (area_start < 0) {
+ assert(start >= 0 && end >= 0 && max_dirty_count > 0);
+
+ if (start >= hb->orig_size || end <= start) {
+ return false;
+ }
+
+ end = MIN(end, hb->orig_size);
+
+ start = hbitmap_next_dirty(hb, start, end - start);
+ if (start < 0) {
return false;
}
- area_end = hbitmap_next_zero(hb, area_start, *start + *count - area_start);
- if (area_end < 0) {
- area_end = MIN(hb->orig_size, *start + *count);
+ end = start + MIN(end - start, max_dirty_count);
+
+ next_zero = hbitmap_next_zero(hb, start, end - start);
+ if (next_zero >= 0) {
+ end = next_zero;
}
- *start = area_start;
- *count = area_end - area_start;
+ *dirty_start = start;
+ *dirty_count = end - start;
return true;
}
@@ -836,13 +848,12 @@ static void hbitmap_sparse_merge(HBitmap *dst, const HBitmap *src)
int64_t offset = 0;
int64_t count = src->orig_size;
- while (hbitmap_next_dirty_area(src, &offset, &count)) {
+ for (offset = 0;
+ hbitmap_next_dirty_area(src, offset, src->orig_size, INT64_MAX,
+ &offset, &count);
+ offset += count)
+ {
hbitmap_set(dst, offset, count);
- offset += count;
- if (offset >= src->orig_size) {
- break;
- }
- count = src->orig_size - offset;
}
}
--
2.21.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 08/10] nbd/server: introduce NBDExtentArray
2019-09-30 15:14 [PATCH 00/10] Further bitmaps improvements Vladimir Sementsov-Ogievskiy
` (6 preceding siblings ...)
2019-09-30 15:14 ` [PATCH 07/10] block/dirty-bitmap: improve _next_dirty_area API Vladimir Sementsov-Ogievskiy
@ 2019-09-30 15:15 ` Vladimir Sementsov-Ogievskiy
2019-10-09 17:02 ` Eric Blake
2019-09-30 15:15 ` [PATCH 09/10] nbd/server: use bdrv_dirty_bitmap_next_dirty_area Vladimir Sementsov-Ogievskiy
2019-09-30 15:15 ` [PATCH 10/10] block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty Vladimir Sementsov-Ogievskiy
9 siblings, 1 reply; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-30 15:15 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, fam, vsementsov, qemu-devel, mreitz, den, jsnow
Introduce NBDExtentArray class, to handle extents list creation in more
controlled way and with less OUT parameters in functions.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
nbd/server.c | 184 +++++++++++++++++++++++++--------------------------
1 file changed, 90 insertions(+), 94 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index cc0069c15b..cc63d8ad21 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1894,27 +1894,63 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
return ret;
}
-/*
- * Populate @extents from block status. Update @bytes to be the actual
- * length encoded (which may be smaller than the original), and update
- * @nb_extents to the number of extents used.
- *
- * Returns zero on success and -errno on bdrv_block_status_above failure.
- */
-static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
- uint64_t *bytes, NBDExtent *extents,
- unsigned int *nb_extents)
+typedef struct NBDExtentArray {
+ NBDExtent *extents;
+ unsigned int nb_alloc;
+ unsigned int count;
+ uint64_t total_length;
+} NBDExtentArray;
+
+static NBDExtentArray *nbd_extent_array_new(unsigned int nb_alloc)
+{
+ NBDExtentArray *ea = g_new0(NBDExtentArray, 1);
+
+ ea->nb_alloc = nb_alloc;
+ ea->extents = g_new(NBDExtent, nb_alloc);
+
+ return ea;
+}
+
+static void nbd_extent_array_free(NBDExtentArray *ea)
+{
+ g_free(ea->extents);
+ g_free(ea);
+}
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free);
+
+static int nbd_extent_array_add(NBDExtentArray *ea,
+ uint32_t length, uint32_t flags)
{
- uint64_t remaining_bytes = *bytes;
- NBDExtent *extent = extents, *extents_end = extents + *nb_extents;
- bool first_extent = true;
+ if (!length) {
+ return 0;
+ }
+
+ /* Extend previous extent if flags are the same */
+ if (ea->count > 0 && flags == ea->extents[ea->count - 1].flags) {
+ ea->extents[ea->count - 1].length += length;
+ ea->total_length += length;
+ return 0;
+ }
- assert(*nb_extents);
- while (remaining_bytes) {
+ if (ea->count >= ea->nb_alloc) {
+ return -1;
+ }
+
+ ea->total_length += length;
+ ea->extents[ea->count] = (NBDExtent) {.length = length, .flags = flags};
+ ea->count++;
+
+ return 0;
+}
+
+static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
+ uint64_t bytes, NBDExtentArray *ea)
+{
+ while (bytes) {
uint32_t flags;
int64_t num;
- int ret = bdrv_block_status_above(bs, NULL, offset, remaining_bytes,
- &num, NULL, NULL);
+ int ret = bdrv_block_status_above(bs, NULL, offset, bytes, &num,
+ NULL, NULL);
if (ret < 0) {
return ret;
@@ -1923,60 +1959,42 @@ static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
flags = (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
(ret & BDRV_BLOCK_ZERO ? NBD_STATE_ZERO : 0);
- if (first_extent) {
- extent->flags = flags;
- extent->length = num;
- first_extent = false;
- } else if (flags == extent->flags) {
- /* extend current extent */
- extent->length += num;
- } else {
- if (extent + 1 == extents_end) {
- break;
- }
-
- /* start new extent */
- extent++;
- extent->flags = flags;
- extent->length = num;
+ if (nbd_extent_array_add(ea, num, flags) < 0) {
+ return 0;
}
- offset += num;
- remaining_bytes -= num;
- }
- extents_end = extent + 1;
-
- for (extent = extents; extent < extents_end; extent++) {
- extent->flags = cpu_to_be32(extent->flags);
- extent->length = cpu_to_be32(extent->length);
+ offset += num;
+ bytes -= num;
}
- *bytes -= remaining_bytes;
- *nb_extents = extents_end - extents;
-
return 0;
}
-/* nbd_co_send_extents
+/*
+ * nbd_co_send_extents
*
- * @length is only for tracing purposes (and may be smaller or larger
- * than the client's original request). @last controls whether
- * NBD_REPLY_FLAG_DONE is sent. @extents should already be in
- * big-endian format.
+ * @last controls whether NBD_REPLY_FLAG_DONE is sent.
*/
static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
- NBDExtent *extents, unsigned int nb_extents,
- uint64_t length, bool last,
- uint32_t context_id, Error **errp)
+ NBDExtentArray *ea,
+ bool last, uint32_t context_id, Error **errp)
{
NBDStructuredMeta chunk;
-
+ size_t len = ea->count * sizeof(ea->extents[0]);
+ g_autofree NBDExtent *extents = g_memdup(ea->extents, len);
+ NBDExtent *extent, *extents_end = extents + ea->count;
struct iovec iov[] = {
{.iov_base = &chunk, .iov_len = sizeof(chunk)},
- {.iov_base = extents, .iov_len = nb_extents * sizeof(extents[0])}
+ {.iov_base = extents, .iov_len = len}
};
- trace_nbd_co_send_extents(handle, nb_extents, context_id, length, last);
+ for (extent = extents; extent < extents_end; extent++) {
+ extent->flags = cpu_to_be32(extent->flags);
+ extent->length = cpu_to_be32(extent->length);
+ }
+
+ trace_nbd_co_send_extents(handle, ea->count, context_id, ea->total_length,
+ last);
set_be_chunk(&chunk.h, last ? NBD_REPLY_FLAG_DONE : 0,
NBD_REPLY_TYPE_BLOCK_STATUS,
handle, sizeof(chunk) - sizeof(chunk.h) + iov[1].iov_len);
@@ -1994,39 +2012,27 @@ static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
{
int ret;
unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
- NBDExtent *extents = g_new(NBDExtent, nb_extents);
- uint64_t final_length = length;
+ g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents);
- ret = blockstatus_to_extents(bs, offset, &final_length, extents,
- &nb_extents);
+ ret = blockstatus_to_extents(bs, offset, length, ea);
if (ret < 0) {
- g_free(extents);
return nbd_co_send_structured_error(
client, handle, -ret, "can't get block status", errp);
}
- ret = nbd_co_send_extents(client, handle, extents, nb_extents,
- final_length, last, context_id, errp);
-
- g_free(extents);
-
- return ret;
+ return nbd_co_send_extents(client, handle, ea, last, context_id, errp);
}
/*
- * Populate @extents from a dirty bitmap. Unless @dont_fragment, the
- * final extent may exceed the original @length. Store in @length the
- * byte length encoded (which may be smaller or larger than the
- * original), and return the number of extents used.
+ * Populate @ea from a dirty bitmap. Unless @dont_fragment, the
+ * final extent may exceed the original @length.
*/
-static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
- uint64_t *length, NBDExtent *extents,
- unsigned int nb_extents,
- bool dont_fragment)
+static void bitmap_to_extents(BdrvDirtyBitmap *bitmap,
+ uint64_t offset, uint64_t length,
+ NBDExtentArray *ea, bool dont_fragment)
{
uint64_t begin = offset, end = offset;
- uint64_t overall_end = offset + *length;
- unsigned int i = 0;
+ uint64_t overall_end = offset + length;
BdrvDirtyBitmapIter *it;
bool dirty;
@@ -2035,8 +2041,7 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
it = bdrv_dirty_iter_new(bitmap);
dirty = bdrv_dirty_bitmap_get_locked(bitmap, offset);
- assert(begin < overall_end && nb_extents);
- while (begin < overall_end && i < nb_extents) {
+ while (begin < overall_end) {
bool next_dirty = !dirty;
if (dirty) {
@@ -2056,9 +2061,10 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
end = overall_end;
}
- extents[i].length = cpu_to_be32(end - begin);
- extents[i].flags = cpu_to_be32(dirty ? NBD_STATE_DIRTY : 0);
- i++;
+ if (nbd_extent_array_add(ea, end - begin,
+ dirty ? NBD_STATE_DIRTY : 0) < 0) {
+ break;
+ }
begin = end;
dirty = next_dirty;
}
@@ -2068,8 +2074,6 @@ static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
bdrv_dirty_bitmap_unlock(bitmap);
assert(offset < end);
- *length = end - offset;
- return i;
}
static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle,
@@ -2077,20 +2081,12 @@ static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle,
uint32_t length, bool dont_fragment, bool last,
uint32_t context_id, Error **errp)
{
- int ret;
unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
- NBDExtent *extents = g_new(NBDExtent, nb_extents);
- uint64_t final_length = length;
+ g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents);
- nb_extents = bitmap_to_extents(bitmap, offset, &final_length, extents,
- nb_extents, dont_fragment);
+ bitmap_to_extents(bitmap, offset, length, ea, dont_fragment);
- ret = nbd_co_send_extents(client, handle, extents, nb_extents,
- final_length, last, context_id, errp);
-
- g_free(extents);
-
- return ret;
+ return nbd_co_send_extents(client, handle, ea, last, context_id, errp);
}
/* nbd_co_receive_request
--
2.21.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 08/10] nbd/server: introduce NBDExtentArray
2019-09-30 15:15 ` [PATCH 08/10] nbd/server: introduce NBDExtentArray Vladimir Sementsov-Ogievskiy
@ 2019-10-09 17:02 ` Eric Blake
2019-10-18 16:07 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 17+ messages in thread
From: Eric Blake @ 2019-10-09 17:02 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, fam, qemu-devel, mreitz, den, jsnow
On 9/30/19 10:15 AM, Vladimir Sementsov-Ogievskiy wrote:
> Introduce NBDExtentArray class, to handle extents list creation in more
> controlled way and with less OUT parameters in functions.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> nbd/server.c | 184 +++++++++++++++++++++++++--------------------------
> 1 file changed, 90 insertions(+), 94 deletions(-)
>
> +static void nbd_extent_array_free(NBDExtentArray *ea)
> +{
> + g_free(ea->extents);
> + g_free(ea);
> +}
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free);
Nice to see this getting more popular :)
> +
> +static int nbd_extent_array_add(NBDExtentArray *ea,
> + uint32_t length, uint32_t flags)
> {
> - assert(*nb_extents);
> - while (remaining_bytes) {
> + if (ea->count >= ea->nb_alloc) {
> + return -1;
> + }
Returning -1 is not a failure in the protocol, just failure to add any
more information to the reply. A function comment might help, but this
looks like a good helper function.
> +static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
> + uint64_t bytes, NBDExtentArray *ea)
> +{
> + while (bytes) {
> uint32_t flags;
> int64_t num;
> - int ret = bdrv_block_status_above(bs, NULL, offset, remaining_bytes,
> - &num, NULL, NULL);
> + int ret = bdrv_block_status_above(bs, NULL, offset, bytes, &num,
> + NULL, NULL);
>
> + if (nbd_extent_array_add(ea, num, flags) < 0) {
> + return 0;
> }
> - offset += num;
> - remaining_bytes -= num;
> - }
>
> - extents_end = extent + 1;
> -
> - for (extent = extents; extent < extents_end; extent++) {
> - extent->flags = cpu_to_be32(extent->flags);
> - extent->length = cpu_to_be32(extent->length);
> + offset += num;
> + bytes -= num;
> }
>
> - *bytes -= remaining_bytes;
> - *nb_extents = extents_end - extents;
> -
> return 0;
Also looks good (return 0 if we populated until we either ran out of
reply space or out of bytes to report on).
> static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
> - NBDExtent *extents, unsigned int nb_extents,
> - uint64_t length, bool last,
> - uint32_t context_id, Error **errp)
> + NBDExtentArray *ea,
> + bool last, uint32_t context_id, Error **errp)
> {
> NBDStructuredMeta chunk;
> -
> + size_t len = ea->count * sizeof(ea->extents[0]);
> + g_autofree NBDExtent *extents = g_memdup(ea->extents, len);
Why do we need memdup here? What's wrong with modifying ea->extents in
place?...
> + NBDExtent *extent, *extents_end = extents + ea->count;
> struct iovec iov[] = {
> {.iov_base = &chunk, .iov_len = sizeof(chunk)},
> - {.iov_base = extents, .iov_len = nb_extents * sizeof(extents[0])}
> + {.iov_base = extents, .iov_len = len}
> };
>
> - trace_nbd_co_send_extents(handle, nb_extents, context_id, length, last);
> + for (extent = extents; extent < extents_end; extent++) {
> + extent->flags = cpu_to_be32(extent->flags);
> + extent->length = cpu_to_be32(extent->length);
> + }
> +
> + trace_nbd_co_send_extents(handle, ea->count, context_id, ea->total_length,
> + last);
> set_be_chunk(&chunk.h, last ? NBD_REPLY_FLAG_DONE : 0,
> NBD_REPLY_TYPE_BLOCK_STATUS,
> handle, sizeof(chunk) - sizeof(chunk.h) + iov[1].iov_len);
> @@ -1994,39 +2012,27 @@ static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
> {
> int ret;
> unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
> - NBDExtent *extents = g_new(NBDExtent, nb_extents);
> - uint64_t final_length = length;
> + g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents);
>
> - ret = blockstatus_to_extents(bs, offset, &final_length, extents,
> - &nb_extents);
> + ret = blockstatus_to_extents(bs, offset, length, ea);
> if (ret < 0) {
> - g_free(extents);
> return nbd_co_send_structured_error(
> client, handle, -ret, "can't get block status", errp);
> }
>
> - ret = nbd_co_send_extents(client, handle, extents, nb_extents,
> - final_length, last, context_id, errp);
> -
> - g_free(extents);
> -
> - return ret;
> + return nbd_co_send_extents(client, handle, ea, last, context_id, errp);
...especially since ea goes out of scope right after the helper function
finishes?
Overall looks like a nice refactoring.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 08/10] nbd/server: introduce NBDExtentArray
2019-10-09 17:02 ` Eric Blake
@ 2019-10-18 16:07 ` Vladimir Sementsov-Ogievskiy
2019-10-18 16:34 ` Eric Blake
0 siblings, 1 reply; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-10-18 16:07 UTC (permalink / raw)
To: Eric Blake, qemu-block; +Cc: kwolf, fam, Denis Lunev, qemu-devel, mreitz, jsnow
09.10.2019 20:02, Eric Blake wrote:
> On 9/30/19 10:15 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Introduce NBDExtentArray class, to handle extents list creation in more
>> controlled way and with less OUT parameters in functions.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> nbd/server.c | 184 +++++++++++++++++++++++++--------------------------
>> 1 file changed, 90 insertions(+), 94 deletions(-)
>>
>
>> +static void nbd_extent_array_free(NBDExtentArray *ea)
>> +{
>> + g_free(ea->extents);
>> + g_free(ea);
>> +}
>> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free);
>
> Nice to see this getting more popular :)
>
>> +
>> +static int nbd_extent_array_add(NBDExtentArray *ea,
>> + uint32_t length, uint32_t flags)
>> {
>
>> - assert(*nb_extents);
>> - while (remaining_bytes) {
>> + if (ea->count >= ea->nb_alloc) {
>> + return -1;
>> + }
>
> Returning -1 is not a failure in the protocol, just failure to add any more information to the reply. A function comment might help, but this looks like a good helper function.
Something like
/*
* Add extent to NBDExtentArray. If extent can't be added (no available space),
* return -1.
*/
above the function?
>
>> +static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
>> + uint64_t bytes, NBDExtentArray *ea)
>> +{
>> + while (bytes) {
>> uint32_t flags;
>> int64_t num;
>> - int ret = bdrv_block_status_above(bs, NULL, offset, remaining_bytes,
>> - &num, NULL, NULL);
>> + int ret = bdrv_block_status_above(bs, NULL, offset, bytes, &num,
>> + NULL, NULL);
>
>> + if (nbd_extent_array_add(ea, num, flags) < 0) {
>> + return 0;
>> }
>> - offset += num;
>> - remaining_bytes -= num;
>> - }
>> - extents_end = extent + 1;
>> -
>> - for (extent = extents; extent < extents_end; extent++) {
>> - extent->flags = cpu_to_be32(extent->flags);
>> - extent->length = cpu_to_be32(extent->length);
>> + offset += num;
>> + bytes -= num;
>> }
>> - *bytes -= remaining_bytes;
>> - *nb_extents = extents_end - extents;
>> -
>> return 0;
>
> Also looks good (return 0 if we populated until we either ran out of reply space or out of bytes to report on).
>
>> static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
>> - NBDExtent *extents, unsigned int nb_extents,
>> - uint64_t length, bool last,
>> - uint32_t context_id, Error **errp)
>> + NBDExtentArray *ea,
>> + bool last, uint32_t context_id, Error **errp)
>> {
>> NBDStructuredMeta chunk;
>> -
>> + size_t len = ea->count * sizeof(ea->extents[0]);
>> + g_autofree NBDExtent *extents = g_memdup(ea->extents, len);
>
> Why do we need memdup here? What's wrong with modifying ea->extents in place?...
To not make ea to be IN-OUT parameter.. I don't like functions with side effects.
It will break the code if at some point we call nbd_co_send_extents twice on same
ea object.
What is the true way? To not memdup, nbd_co_send_extents should consume the whole
ea object..
Seems, g_autoptr attribute can't be used for function parameter, gcc complains:
nbd/server.c:1983:32: error: ‘cleanup’ attribute ignored [-Werror=attributes]
1983 | g_autoptr(NBDExtentArray) ea,
| ^~~~~~~~~
so, is it better
to call nbd_co_send_external(... g_steal_pointer(&ea) ...)
and than in nbd_co_send_external do
g_autoptr(NBDExtentArray) local_ea = ea;
NBDExtent *extents = local_ea->extents;
?
>
>> + NBDExtent *extent, *extents_end = extents + ea->count;
>> struct iovec iov[] = {
>> {.iov_base = &chunk, .iov_len = sizeof(chunk)},
>> - {.iov_base = extents, .iov_len = nb_extents * sizeof(extents[0])}
>> + {.iov_base = extents, .iov_len = len}
>> };
>> - trace_nbd_co_send_extents(handle, nb_extents, context_id, length, last);
>> + for (extent = extents; extent < extents_end; extent++) {
>> + extent->flags = cpu_to_be32(extent->flags);
>> + extent->length = cpu_to_be32(extent->length);
>> + }
>> +
>> + trace_nbd_co_send_extents(handle, ea->count, context_id, ea->total_length,
>> + last);
>> set_be_chunk(&chunk.h, last ? NBD_REPLY_FLAG_DONE : 0,
>> NBD_REPLY_TYPE_BLOCK_STATUS,
>> handle, sizeof(chunk) - sizeof(chunk.h) + iov[1].iov_len);
>> @@ -1994,39 +2012,27 @@ static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
>> {
>> int ret;
>> unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
>> - NBDExtent *extents = g_new(NBDExtent, nb_extents);
>> - uint64_t final_length = length;
>> + g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents);
>> - ret = blockstatus_to_extents(bs, offset, &final_length, extents,
>> - &nb_extents);
>> + ret = blockstatus_to_extents(bs, offset, length, ea);
>> if (ret < 0) {
>> - g_free(extents);
>> return nbd_co_send_structured_error(
>> client, handle, -ret, "can't get block status", errp);
>> }
>> - ret = nbd_co_send_extents(client, handle, extents, nb_extents,
>> - final_length, last, context_id, errp);
>> -
>> - g_free(extents);
>> -
>> - return ret;
>> + return nbd_co_send_extents(client, handle, ea, last, context_id, errp);
>
> ...especially since ea goes out of scope right after the helper function finishes?
>
> Overall looks like a nice refactoring.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 08/10] nbd/server: introduce NBDExtentArray
2019-10-18 16:07 ` Vladimir Sementsov-Ogievskiy
@ 2019-10-18 16:34 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2019-10-18 16:34 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, fam, Denis Lunev, qemu-devel, mreitz, jsnow
On 10/18/19 11:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
>>> - NBDExtent *extents, unsigned int nb_extents,
>>> - uint64_t length, bool last,
>>> - uint32_t context_id, Error **errp)
>>> + NBDExtentArray *ea,
>>> + bool last, uint32_t context_id, Error **errp)
>>> {
>>> NBDStructuredMeta chunk;
>>> -
>>> + size_t len = ea->count * sizeof(ea->extents[0]);
>>> + g_autofree NBDExtent *extents = g_memdup(ea->extents, len);
>>
>> Why do we need memdup here? What's wrong with modifying ea->extents in place?...
>
> To not make ea to be IN-OUT parameter.. I don't like functions with side effects.
> It will break the code if at some point we call nbd_co_send_extents twice on same
> ea object.
>
> What is the true way? To not memdup, nbd_co_send_extents should consume the whole
> ea object..
>
> Seems, g_autoptr attribute can't be used for function parameter, gcc complains:
> nbd/server.c:1983:32: error: ‘cleanup’ attribute ignored [-Werror=attributes]
> 1983 | g_autoptr(NBDExtentArray) ea,
> | ^~~~~~~~~
>
> so, is it better
> to call nbd_co_send_external(... g_steal_pointer(&ea) ...)
>
> and than in nbd_co_send_external do
>
> g_autoptr(NBDExtentArray) local_ea = ea;
> NBDExtent *extents = local_ea->extents;
>
> ?
>
No, that makes it worse. It's that much more confusing to track who is
allocating what and where it gets cleaned up.
I personally don't see the need to avoid jumping through hoops to avoid
an in-out parameter (if we're going to rework code later, we'll notice
that we documented how things are supposed to be used), but if in-out
parameters bother you, then the approach you used, even with an extra
memdup(), is the simplest way to maintain, even if it is not the most
efficient.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 09/10] nbd/server: use bdrv_dirty_bitmap_next_dirty_area
2019-09-30 15:14 [PATCH 00/10] Further bitmaps improvements Vladimir Sementsov-Ogievskiy
` (7 preceding siblings ...)
2019-09-30 15:15 ` [PATCH 08/10] nbd/server: introduce NBDExtentArray Vladimir Sementsov-Ogievskiy
@ 2019-09-30 15:15 ` Vladimir Sementsov-Ogievskiy
2019-10-09 18:26 ` Eric Blake
2019-09-30 15:15 ` [PATCH 10/10] block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty Vladimir Sementsov-Ogievskiy
9 siblings, 1 reply; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-30 15:15 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, fam, vsementsov, qemu-devel, mreitz, den, jsnow
Use bdrv_dirty_bitmap_next_dirty_area for bitmap_to_extents. Since
bdrv_dirty_bitmap_next_dirty_area is very accurate in its interface,
we'll never exceed requested region with last chunk. So, we don't need
dont_fragment, and bitmap_to_extents() interface becomes clean enough
to not require any comment.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
nbd/server.c | 58 ++++++++++++++++------------------------------------
1 file changed, 18 insertions(+), 40 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index cc63d8ad21..edbdb1b6b6 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2023,57 +2023,35 @@ static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
return nbd_co_send_extents(client, handle, ea, last, context_id, errp);
}
-/*
- * Populate @ea from a dirty bitmap. Unless @dont_fragment, the
- * final extent may exceed the original @length.
- */
static void bitmap_to_extents(BdrvDirtyBitmap *bitmap,
uint64_t offset, uint64_t length,
- NBDExtentArray *ea, bool dont_fragment)
+ NBDExtentArray *es)
{
- uint64_t begin = offset, end = offset;
- uint64_t overall_end = offset + length;
- BdrvDirtyBitmapIter *it;
- bool dirty;
+ int64_t start, dirty_start, dirty_count;
+ int64_t end = offset + length;
+ bool full = false;
bdrv_dirty_bitmap_lock(bitmap);
- it = bdrv_dirty_iter_new(bitmap);
- dirty = bdrv_dirty_bitmap_get_locked(bitmap, offset);
-
- while (begin < overall_end) {
- bool next_dirty = !dirty;
-
- if (dirty) {
- end = bdrv_dirty_bitmap_next_zero(bitmap, begin, INT64_MAX);
- } else {
- bdrv_set_dirty_iter(it, begin);
- end = bdrv_dirty_iter_next(it);
- }
- if (end == -1 || end - begin > UINT32_MAX) {
- /* Cap to an aligned value < 4G beyond begin. */
- end = MIN(bdrv_dirty_bitmap_size(bitmap),
- begin + UINT32_MAX + 1 -
- bdrv_dirty_bitmap_granularity(bitmap));
- next_dirty = dirty;
- }
- if (dont_fragment && end > overall_end) {
- end = overall_end;
- }
-
- if (nbd_extent_array_add(ea, end - begin,
- dirty ? NBD_STATE_DIRTY : 0) < 0) {
+ for (start = offset;
+ bdrv_dirty_bitmap_next_dirty_area(bitmap, start, end, INT32_MAX,
+ &dirty_start, &dirty_count);
+ start = dirty_start + dirty_count)
+ {
+ if ((nbd_extent_array_add(es, dirty_start - start, 0) < 0) ||
+ (nbd_extent_array_add(es, dirty_count, NBD_STATE_DIRTY) < 0))
+ {
+ full = true;
break;
}
- begin = end;
- dirty = next_dirty;
}
- bdrv_dirty_iter_free(it);
+ if (!full) {
+ /* last non dirty extent */
+ nbd_extent_array_add(es, end - start, 0);
+ }
bdrv_dirty_bitmap_unlock(bitmap);
-
- assert(offset < end);
}
static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle,
@@ -2084,7 +2062,7 @@ static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle,
unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents);
- bitmap_to_extents(bitmap, offset, length, ea, dont_fragment);
+ bitmap_to_extents(bitmap, offset, length, ea);
return nbd_co_send_extents(client, handle, ea, last, context_id, errp);
}
--
2.21.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 09/10] nbd/server: use bdrv_dirty_bitmap_next_dirty_area
2019-09-30 15:15 ` [PATCH 09/10] nbd/server: use bdrv_dirty_bitmap_next_dirty_area Vladimir Sementsov-Ogievskiy
@ 2019-10-09 18:26 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2019-10-09 18:26 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, fam, qemu-devel, mreitz, den, jsnow
On 9/30/19 10:15 AM, Vladimir Sementsov-Ogievskiy wrote:
> Use bdrv_dirty_bitmap_next_dirty_area for bitmap_to_extents. Since
> bdrv_dirty_bitmap_next_dirty_area is very accurate in its interface,
> we'll never exceed requested region with last chunk. So, we don't need
> dont_fragment, and bitmap_to_extents() interface becomes clean enough
> to not require any comment.
Comments are a useful style, even if functions seem trivial.
When req_one is in effect, we have to stop at the requested length.
When req_one is not in effect, the NBD spec does not require us to stop
until the next change in extent status, but also does not force us to
continue past. So this change is fine from the protocol standpoint.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> nbd/server.c | 58 ++++++++++++++++------------------------------------
> 1 file changed, 18 insertions(+), 40 deletions(-)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index cc63d8ad21..edbdb1b6b6 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -2023,57 +2023,35 @@ static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
> return nbd_co_send_extents(client, handle, ea, last, context_id, errp);
> }
>
> -/*
> - * Populate @ea from a dirty bitmap. Unless @dont_fragment, the
> - * final extent may exceed the original @length.
> - */
I would have kept the first sentence, and dropped only the second.
> static void bitmap_to_extents(BdrvDirtyBitmap *bitmap,
> uint64_t offset, uint64_t length,
> - NBDExtentArray *ea, bool dont_fragment)
> + NBDExtentArray *es)
> {
> - uint64_t begin = offset, end = offset;
> - uint64_t overall_end = offset + length;
> - BdrvDirtyBitmapIter *it;
> - bool dirty;
> + int64_t start, dirty_start, dirty_count;
> + int64_t end = offset + length;
> + bool full = false;
>
> bdrv_dirty_bitmap_lock(bitmap);
> + for (start = offset;
> + bdrv_dirty_bitmap_next_dirty_area(bitmap, start, end, INT32_MAX,
> + &dirty_start, &dirty_count);
> + start = dirty_start + dirty_count)
> + {
> + if ((nbd_extent_array_add(es, dirty_start - start, 0) < 0) ||
> + (nbd_extent_array_add(es, dirty_count, NBD_STATE_DIRTY) < 0))
As long as bdrv_dirty_bitmap_next_dirty_area works correctly, this
should work regardless of whether start is dirty or clean (if dirty, the
first call will be a 0-length no-op).
> + {
> + full = true;
> break;
> }
> - begin = end;
> - dirty = next_dirty;
> }
>
> - bdrv_dirty_iter_free(it);
> + if (!full) {
> + /* last non dirty extent */
> + nbd_extent_array_add(es, end - start, 0);
> + }
Losing the possibility of reporting beyond the end of the original
request (when req_one is not in force) is not fatal (it might make some
clients less efficient when walking the entire disk, but qemu as a
client isn't currently taking advantage of NBD's permission to return
extra length).
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 10/10] block/qcow2-bitmap: use bdrv_dirty_bitmap_next_dirty
2019-09-30 15:14 [PATCH 00/10] Further bitmaps improvements Vladimir Sementsov-Ogievskiy
` (8 preceding siblings ...)
2019-09-30 15:15 ` [PATCH 09/10] nbd/server: use bdrv_dirty_bitmap_next_dirty_area Vladimir Sementsov-Ogievskiy
@ 2019-09-30 15:15 ` Vladimir Sementsov-Ogievskiy
9 siblings, 0 replies; 17+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-30 15:15 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, fam, vsementsov, qemu-devel, mreitz, den, jsnow
store_bitmap_data() loop does bdrv_set_dirty_iter() on each iteration,
which means that we actually don't need iterator itself and we can use
simpler bitmap API.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2-bitmap.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 98294a7696..09d892638d 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1260,7 +1260,6 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
uint8_t *buf = NULL;
- BdrvDirtyBitmapIter *dbi;
uint64_t *tb;
uint64_t tb_size =
size_to_clusters(s,
@@ -1279,12 +1278,14 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
return NULL;
}
- dbi = bdrv_dirty_iter_new(bitmap);
buf = g_malloc(s->cluster_size);
limit = bytes_covered_by_bitmap_cluster(s, bitmap);
assert(DIV_ROUND_UP(bm_size, limit) == tb_size);
- while ((offset = bdrv_dirty_iter_next(dbi)) >= 0) {
+ offset = 0;
+ while ((offset = bdrv_dirty_bitmap_next_dirty(bitmap, offset, INT64_MAX))
+ >= 0)
+ {
uint64_t cluster = offset / limit;
uint64_t end, write_size;
int64_t off;
@@ -1331,19 +1332,17 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
break;
}
- bdrv_set_dirty_iter(dbi, end);
+ offset = end;
}
*bitmap_table_size = tb_size;
g_free(buf);
- bdrv_dirty_iter_free(dbi);
return tb;
fail:
clear_bitmap_table(bs, tb, tb_size);
g_free(buf);
- bdrv_dirty_iter_free(dbi);
g_free(tb);
return NULL;
--
2.21.0
^ permalink raw reply related [flat|nested] 17+ messages in thread