20.01.2020 16:58, Max Reitz wrote: > On 19.12.19 11:03, Vladimir Sementsov-Ogievskiy wrote: >> 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, let's add a possibility to limit resulting area size, not >> limiting searching area. This will be used in NBD code in further >> commit. (Note that now bdrv_dirty_bitmap_next_dirty_area is unused) >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]> >> --- >> include/block/dirty-bitmap.h | 3 ++- >> include/qemu/hbitmap.h | 25 ++++++++++++--------- >> block/dirty-bitmap.c | 6 +++-- >> tests/test-hbitmap.c | 43 +++++++++++++++++++++++------------- >> util/hbitmap.c | 41 +++++++++++++++++++++------------- >> 5 files changed, 74 insertions(+), 44 deletions(-) > > [...] > >> /** >> 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, >> ; > > These empty statements look a bit weird to me. But they’re > pre-existing, obviously. > >> } >> >> - 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++) >> + { >> ; >> } > > [...] > >> diff --git a/util/hbitmap.c b/util/hbitmap.c >> index d23f4b9678..2a1661ec1d 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); > > You could put this assignment before the if () and then drop the “start” > check from the condition. (But that’s mostly me itching to do > optimizations. I don’t think it’d make the code easier to read.) > > [...] > >> @@ -844,13 +856,12 @@ static void hbitmap_sparse_merge(HBitmap *dst, const >> HBitmap *src) >> int64_t offset = 0; >> int64_t count = src->orig_size; > > These initializations are now unnecessary. I’d drop them because I find > at least the one for @count a tiny bit confusing now. (Because as a > reader, I’d wonder where this value is used.) > > With that done (or maybe not because you disagree):
I agree) > > Reviewed-by: Max Reitz <[email protected]> > >> >> - 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; >> } >> } >> >> > -- Best regards, Vladimir
