Re: [Qemu-devel] [PATCH for 2.6 1/3] backup: Use Bitmap to replace "s->bitmap"

[Wen Congyang <wency@cn.fujitsu.com>]

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 <famz@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.

Thanks
Wen Congyang

> 
> Fam
> 
>>
>> Thanks
>> Wen Congyang
>>
>>>  
>>>          /* 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);
>>>
>>
> .
>