On Fri, 11/20 18:53, Vladimir Sementsov-Ogievskiy wrote:
> On 20.11.2015 12:59, Fam Zheng wrote:
> >"s->bitmap" tracks done sectors, we only check bit states without using any
> >iterator which HBitmap is good for. Switch to "Bitmap" which is simpler and
> >more memory efficient.
> >
> >Meanwhile, rename it to done_bitmap, to reflect the intention.
> >
> >Signed-off-by: Fam Zheng <f...@redhat.com>
> >---
> > block/backup.c | 11 ++++++-----
> > 1 file changed, 6 insertions(+), 5 deletions(-)
> >
> >diff --git a/block/backup.c b/block/backup.c
> >index 3b39119..d408f98 100644
> >--- a/block/backup.c
> >+++ b/block/backup.c
> >@@ -22,6 +22,7 @@
> > #include "qapi/qmp/qerror.h"
> > #include "qemu/ratelimit.h"
> > #include "sysemu/block-backend.h"
> >+#include "qemu/bitmap.h"
> > #define BACKUP_CLUSTER_BITS 16
> > #define BACKUP_CLUSTER_SIZE (1 << BACKUP_CLUSTER_BITS)
> >@@ -47,7 +48,7 @@ typedef struct BackupBlockJob {
> > BlockdevOnError on_target_error;
> > CoRwlock flush_rwlock;
> > uint64_t sectors_read;
> >- HBitmap *bitmap;
> >+ unsigned long *done_bitmap;
> > QLIST_HEAD(, CowRequest) inflight_reqs;
> > } BackupBlockJob;
> >@@ -113,7 +114,7 @@ static int coroutine_fn backup_do_cow(BlockDriverState
> >*bs,
> > cow_request_begin(&cow_request, job, start, end);
> > for (; start < end; start++) {
> >- if (hbitmap_get(job->bitmap, start)) {
> >+ if (test_bit(start, job->done_bitmap)) {
> > trace_backup_do_cow_skip(job, start);
> > continue; /* already copied */
> > }
> >@@ -164,7 +165,7 @@ static int coroutine_fn backup_do_cow(BlockDriverState
> >*bs,
> > goto out;
> > }
> >- hbitmap_set(job->bitmap, start, 1);
> >+ bitmap_set(job->done_bitmap, start, 1);
> > /* Publish progress, guest I/O counts as progress too. Note that
> > the
> > * offset field is an opaque progress value, it is not a disk
> > offset.
> >@@ -394,7 +395,7 @@ static void coroutine_fn backup_run(void *opaque)
> > start = 0;
> > end = DIV_ROUND_UP(job->common.len, BACKUP_CLUSTER_SIZE);
> >- job->bitmap = hbitmap_alloc(end, 0);
> >+ job->done_bitmap = bitmap_new(end);
> > bdrv_set_enable_write_cache(target, true);
> > if (target->blk) {
> >@@ -475,7 +476,7 @@ static void coroutine_fn backup_run(void *opaque)
> > /* wait until pending backup_do_cow() calls have completed */
> > qemu_co_rwlock_wrlock(&job->flush_rwlock);
> > qemu_co_rwlock_unlock(&job->flush_rwlock);
> >- hbitmap_free(job->bitmap);
> >+ g_free(job->done_bitmap);
> > if (target->blk) {
> > blk_iostatus_disable(target->blk);
>
> Isn't it better to use hbitmap iterators here to speed up bitmap handling?
>
> trace_backup_do_cow_skip actually do nothing, so
>
> for (; start < end; start++) {
> if (hbitmap_get(job->bitmap, start)) {
> trace_backup_do_cow_skip(job, start);
> continue; /* already copied */
> }
>
> may efficiently changed by something like:
>
> hbitmap_iter_init(hbi, start);
> while ((start = hbitmap_iter_next(hbi)) != -1) {
>
>
> !! in this case bitmap usage should be reverted, as
> hbitmap_iter_next will skip 0 bits, not 1 ones.
>
This is not really necessary because backup iterates sequentially and only goes
for at most one pass. The bitmap is used for write interception.
Fam
