From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57151) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZzoNg-0000bE-8D for qemu-devel@nongnu.org; Fri, 20 Nov 2015 11:17:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZzoNb-0006Po-8K for qemu-devel@nongnu.org; Fri, 20 Nov 2015 11:17:48 -0500 Message-ID: <564F4215.7050006@virtuozzo.com> Date: Fri, 20 Nov 2015 18:53:57 +0300 From: Vladimir Sementsov-Ogievskiy MIME-Version: 1.0 References: <1448013593-14282-1-git-send-email-famz@redhat.com> <1448013593-14282-2-git-send-email-famz@redhat.com> In-Reply-To: <1448013593-14282-2-git-send-email-famz@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH for 2.6 1/3] backup: Use Bitmap to replace "s->bitmap" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , qemu-block@nongnu.org, Jeff Cody , mreitz@redhat.com, pbonzini@redhat.com, jsnow@redhat.com 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 > --- > 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. -- Best regards, Vladimir * now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.