Message ID | 20180814121443.33114-6-vsementsov@virtuozzo.com |
---|---|
State | New |
Headers | show |
Series | dirty-bitmap: rewrite bdrv_dirty_iter_next_area | expand |
On 08/14/2018 08:14 AM, Vladimir Sementsov-Ogievskiy wrote: > Use bdrv_dirty_bitmap_next_dirty_area() instead of > bdrv_dirty_iter_next_area(), because of the following problems of > bdrv_dirty_iter_next_area(): > > 1. Using HBitmap iterators we should carefully handle unaligned offset, > as first call to hbitmap_iter_next() may return a value less than > original offset (actually, it will be original offset rounded down to > bitmap granularity). This handling is not done in > do_sync_target_write(). > > 2. bdrv_dirty_iter_next_area() handles unaligned max_offset > incorrectly: > > look at the code: > if (max_offset == iter->bitmap->size) { > /* If max_offset points to the image end, round it up by the > * bitmap granularity */ > gran_max_offset = ROUND_UP(max_offset, granularity); > } else { > gran_max_offset = max_offset; > } > > ret = hbitmap_iter_next(&iter->hbi, false); > if (ret < 0 || ret + granularity > gran_max_offset) { > return false; > } > > and assume that max_offset != iter->bitmap->size but still unaligned. > if 0 < ret < max_offset we found dirty area, but the function can > return false in this case (if ret + granularity > max_offset). > > 3. bdrv_dirty_iter_next_area() uses inefficient loop to find the end of > the dirty area. Let's use more efficient hbitmap_next_zero instead > (bdrv_dirty_bitmap_next_dirty_area() do so) > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/mirror.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/block/mirror.c b/block/mirror.c > index b48c3f8cf5..d2806812c8 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -1167,25 +1167,22 @@ static void do_sync_target_write(MirrorBlockJob *job, MirrorMethod method, > uint64_t offset, uint64_t bytes, > QEMUIOVector *qiov, int flags) > { > - BdrvDirtyBitmapIter *iter; > QEMUIOVector target_qiov; > - uint64_t dirty_offset; > - int dirty_bytes; > + uint64_t dirty_offset = offset; > + uint64_t dirty_bytes; > > if (qiov) { > qemu_iovec_init(&target_qiov, qiov->niov); > } > > - iter = bdrv_dirty_iter_new(job->dirty_bitmap); > - bdrv_set_dirty_iter(iter, offset); > - > while (true) { > bool valid_area; > int ret; > > bdrv_dirty_bitmap_lock(job->dirty_bitmap); > - valid_area = bdrv_dirty_iter_next_area(iter, offset + bytes, > - &dirty_offset, &dirty_bytes); > + valid_area = bdrv_dirty_bitmap_next_dirty_area( > + job->dirty_bitmap, &dirty_offset, > + MIN(offset + bytes, dirty_offset + INT_MAX), &dirty_bytes); Looks correct, though the invocation looks harder to read now. Might be a sign that changing the signature is a good idea.... which I think you're planning to do. ACK. > if (!valid_area) { > bdrv_dirty_bitmap_unlock(job->dirty_bitmap); > break; > @@ -1241,9 +1238,10 @@ static void do_sync_target_write(MirrorBlockJob *job, MirrorMethod method, > break; > } > } > + > + dirty_offset += dirty_bytes; > } > > - bdrv_dirty_iter_free(iter); > if (qiov) { > qemu_iovec_destroy(&target_qiov); > } >
diff --git a/block/mirror.c b/block/mirror.c index b48c3f8cf5..d2806812c8 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1167,25 +1167,22 @@ static void do_sync_target_write(MirrorBlockJob *job, MirrorMethod method, uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags) { - BdrvDirtyBitmapIter *iter; QEMUIOVector target_qiov; - uint64_t dirty_offset; - int dirty_bytes; + uint64_t dirty_offset = offset; + uint64_t dirty_bytes; if (qiov) { qemu_iovec_init(&target_qiov, qiov->niov); } - iter = bdrv_dirty_iter_new(job->dirty_bitmap); - bdrv_set_dirty_iter(iter, offset); - while (true) { bool valid_area; int ret; bdrv_dirty_bitmap_lock(job->dirty_bitmap); - valid_area = bdrv_dirty_iter_next_area(iter, offset + bytes, - &dirty_offset, &dirty_bytes); + valid_area = bdrv_dirty_bitmap_next_dirty_area( + job->dirty_bitmap, &dirty_offset, + MIN(offset + bytes, dirty_offset + INT_MAX), &dirty_bytes); if (!valid_area) { bdrv_dirty_bitmap_unlock(job->dirty_bitmap); break; @@ -1241,9 +1238,10 @@ static void do_sync_target_write(MirrorBlockJob *job, MirrorMethod method, break; } } + + dirty_offset += dirty_bytes; } - bdrv_dirty_iter_free(iter); if (qiov) { qemu_iovec_destroy(&target_qiov); }
Use bdrv_dirty_bitmap_next_dirty_area() instead of bdrv_dirty_iter_next_area(), because of the following problems of bdrv_dirty_iter_next_area(): 1. Using HBitmap iterators we should carefully handle unaligned offset, as first call to hbitmap_iter_next() may return a value less than original offset (actually, it will be original offset rounded down to bitmap granularity). This handling is not done in do_sync_target_write(). 2. bdrv_dirty_iter_next_area() handles unaligned max_offset incorrectly: look at the code: if (max_offset == iter->bitmap->size) { /* If max_offset points to the image end, round it up by the * bitmap granularity */ gran_max_offset = ROUND_UP(max_offset, granularity); } else { gran_max_offset = max_offset; } ret = hbitmap_iter_next(&iter->hbi, false); if (ret < 0 || ret + granularity > gran_max_offset) { return false; } and assume that max_offset != iter->bitmap->size but still unaligned. if 0 < ret < max_offset we found dirty area, but the function can return false in this case (if ret + granularity > max_offset). 3. bdrv_dirty_iter_next_area() uses inefficient loop to find the end of the dirty area. Let's use more efficient hbitmap_next_zero instead (bdrv_dirty_bitmap_next_dirty_area() do so) Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/mirror.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)