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

2015-11-23 Thread Wen Congyang 
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 
> ---
>  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(_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.

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(>flush_rwlock);
>  qemu_co_rwlock_unlock(>flush_rwlock);
> -hbitmap_free(job->bitmap);
> +g_free(job->done_bitmap);
>  
>  if (target->blk) {
>  blk_iostatus_disable(target->blk);
>

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

2015-11-23 Thread Wen Congyang 
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 
>>> ---
>>>  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(_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(>flush_rwlock);
>>>  qemu_co_rwlock_unlock(>flush_rwlock);
>>> -hbitmap_free(job->bitmap);
>>> +g_free(job->done_bitmap);
>>>  
>>>  if (target->blk) {
>>>  blk_iostatus_disable(target->blk);
>>>
>>
> .
>

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

2015-11-23 Thread Fam Zheng 
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 
> >>> ---
> >>>  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(_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

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

2015-11-23 Thread Fam Zheng 
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 
> > ---
> >  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(_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.

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(>flush_rwlock);
> >  qemu_co_rwlock_unlock(>flush_rwlock);
> > -hbitmap_free(job->bitmap);
> > +g_free(job->done_bitmap);
> >  
> >  if (target->blk) {
> >  blk_iostatus_disable(target->blk);
> > 
>

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

2015-11-23 Thread Wen Congyang 
On 11/23/2015 05:55 PM, Fam Zheng wrote:
> 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 
> ---
>  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(_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.

I don't test it. It is just a suggestion.

Thanks
Wen Congyang

> 
> Fam
> .
>

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

2015-11-23 Thread Paolo Bonzini 


On 23/11/2015 10:55, Fam Zheng wrote:
>>> 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.

Substantially.  It's also documented:

/*
 * Also the following operations apply to bitmaps.
 *
 * set_bit(bit, addr)   *addr |= bit
 * clear_bit(bit, addr) *addr &= ~bit
 * change_bit(bit, addr)*addr ^= bit
 * test_bit(bit, addr)  Is bit set in *addr?
 * test_and_set_bit(bit, addr)  Set bit and return old value
 * test_and_clear_bit(bit, addr)Clear bit and return old value
 * test_and_change_bit(bit, addr)   Change bit and return old value
 * find_first_zero_bit(addr, nbits) Position first zero bit in *addr
 * find_first_bit(addr, nbits)  Position first set bit in *addr
 * find_next_zero_bit(addr, nbits, bit) Position next zero bit in *addr >= bit
 * find_next_bit(addr, nbits, bit)  Position next set bit in *addr >= bit
 */

Paolo

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

2015-11-23 Thread John Snow 


On 11/20/2015 04:59 AM, 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(_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(>flush_rwlock);
>  qemu_co_rwlock_unlock(>flush_rwlock);
> -hbitmap_free(job->bitmap);
> +g_free(job->done_bitmap);
>  
>  if (target->blk) {
>  blk_iostatus_disable(target->blk);
> 

Replacing the bitmap_set with set_bit, in response to Paolo's comment:

Reviewed-by: John Snow

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

2015-11-22 Thread Fam Zheng 
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(_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(>flush_rwlock);
> >  qemu_co_rwlock_unlock(>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

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

2015-11-20 Thread Fam Zheng 
"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(_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(>flush_rwlock);
 qemu_co_rwlock_unlock(>flush_rwlock);
-hbitmap_free(job->bitmap);
+g_free(job->done_bitmap);
 
 if (target->blk) {
 blk_iostatus_disable(target->blk);
-- 
2.4.3

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

2015-11-20 Thread Vladimir Sementsov-Ogievskiy 

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(_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(>flush_rwlock);
  qemu_co_rwlock_unlock(>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.