From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37072) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a0kpl-0005zD-SH for qemu-devel@nongnu.org; Mon, 23 Nov 2015 01:42:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a0kpk-0007CI-Rb for qemu-devel@nongnu.org; Mon, 23 Nov 2015 01:42:41 -0500 Date: Mon, 23 Nov 2015 14:42:31 +0800 From: Fam Zheng Message-ID: <20151123064231.GA14351@ad.usersys.redhat.com> References: <1448013593-14282-1-git-send-email-famz@redhat.com> <1448013593-14282-2-git-send-email-famz@redhat.com> <564F4215.7050006@virtuozzo.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <564F4215.7050006@virtuozzo.com> 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: Vladimir Sementsov-Ogievskiy Cc: Kevin Wolf , qemu-block@nongnu.org, Jeff Cody , qemu-devel@nongnu.org, mreitz@redhat.com, pbonzini@redhat.com, jsnow@redhat.com 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 > >--- > > 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