On Mon, 11/23 17:24, Wen Congyang wrote:
> On 11/23/2015 05:19 PM, Fam Zheng wrote:
> > On Mon, 11/23 17:01, Wen Congyang wrote:
> >> On 11/20/2015 05:59 PM, 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);
> >>
> >> You can use set_bit() here.
> >
> > Why? I think bitmap_set is a better match with bitmap_new below.
>
> set_bit() is quicker than bitmap_set() if you only set one bit.
>
How much quicker is it? This doesn't sound convincing enough for me to lose the
readability.
Fam
