Re: [Qemu-devel] [PATCH 3/9] mirror: optimize dirty bitmap filling in mirror_run a bit

2016-06-15 Thread Eric Blake
On 06/15/2016 02:41 AM, Denis V. Lunev wrote:
> On 06/15/2016 05:36 AM, Eric Blake wrote:
>> On 06/14/2016 09:25 AM, Denis V. Lunev wrote:
>>> There is no need to scan allocation tables if we have mark_all_dirty
>>> flag
>>> set. Just mark it all dirty.
>>>

>>>   int ret, n;
>>> end = s->bdev_length / BDRV_SECTOR_SIZE;
>>>   +if (base == NULL && !bdrv_has_zero_init(target_bs)) {
>>> +bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, end);
>> Won't work as written.  'end' is 64 bits, but bdrv_set_dirty_bitmap() is
>> limited to a 32-bit sector count.  Might be first worth updating
>> bdrv_set_dirty_bitmap() and friends to be byte-based rather than
>> sector-based (but still tracking a sector per bit, obviously), as well
>> as expand it to operate on 64-bit sizes rather than 32-bit.
> very nice catch! thank you
> 

[meta-comment - your reply style is a bit hard to read. It's best to
include a blank line both before and after any text you write when
replying in context, as the extra spacing calls visual attention to your
reply rather than being part of a wall of text]

>> I'm also worried slightly that the existing code repeated things in a
>> loop, and therefore had pause points every iteration and could thus
>> remain responsive to an early cancel.  But doing the entire operation in
>> one chunk (assuming you fix bitmap code to handle a 64-bit size) may end
>> up running for so long without interruption that you lose the benefits
>> of an early interruption that you have by virtue of a 32-bit limit.
>>
> I do not think that this should be worried actually. We just perform memset
> inside for not that big area (1 Tb disk will have 2 Mb dirty area
> bitmap) under
> default parameters.
> 

Okay, so the real slowdown in the loop was the rest of the code
(checking backing status in bdrv_is_allocated_above() and not in
actually writing to the bitmap (bdrv_set_dirty_bitmap()).

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature


Re: [Qemu-devel] [PATCH 3/9] mirror: optimize dirty bitmap filling in mirror_run a bit

2016-06-15 Thread Denis V. Lunev

On 06/15/2016 05:36 AM, Eric Blake wrote:

On 06/14/2016 09:25 AM, Denis V. Lunev wrote:

There is no need to scan allocation tables if we have mark_all_dirty flag
set. Just mark it all dirty.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Vladimir Sementsov-Ogievskiy
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Jeff Cody 
CC: Eric Blake 
---
  block/mirror.c | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 797659d..c7b3639 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -507,12 +507,16 @@ static int mirror_dirty_init(MirrorBlockJob *s)
  BlockDriverState *base = s->base;
  BlockDriverState *bs = blk_bs(s->common.blk);
  BlockDriverState *target_bs = blk_bs(s->target);
-bool mark_all_dirty = base == NULL && !bdrv_has_zero_init(target_bs);
  uint64_t last_pause_ns;
  int ret, n;
  
  end = s->bdev_length / BDRV_SECTOR_SIZE;
  
+if (base == NULL && !bdrv_has_zero_init(target_bs)) {

+bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, end);

Won't work as written.  'end' is 64 bits, but bdrv_set_dirty_bitmap() is
limited to a 32-bit sector count.  Might be first worth updating
bdrv_set_dirty_bitmap() and friends to be byte-based rather than
sector-based (but still tracking a sector per bit, obviously), as well
as expand it to operate on 64-bit sizes rather than 32-bit.

very nice catch! thank you


I'm also worried slightly that the existing code repeated things in a
loop, and therefore had pause points every iteration and could thus
remain responsive to an early cancel.  But doing the entire operation in
one chunk (assuming you fix bitmap code to handle a 64-bit size) may end
up running for so long without interruption that you lose the benefits
of an early interruption that you have by virtue of a 32-bit limit.


I do not think that this should be worried actually. We just perform memset
inside for not that big area (1 Tb disk will have 2 Mb dirty area 
bitmap) under

default parameters.



Re: [Qemu-devel] [PATCH 3/9] mirror: optimize dirty bitmap filling in mirror_run a bit

2016-06-14 Thread Eric Blake
On 06/14/2016 09:25 AM, Denis V. Lunev wrote:
> There is no need to scan allocation tables if we have mark_all_dirty flag
> set. Just mark it all dirty.
> 
> Signed-off-by: Denis V. Lunev 
> Reviewed-by: Vladimir Sementsov-Ogievskiy
> CC: Stefan Hajnoczi 
> CC: Fam Zheng 
> CC: Kevin Wolf 
> CC: Max Reitz 
> CC: Jeff Cody 
> CC: Eric Blake 
> ---
>  block/mirror.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 797659d..c7b3639 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -507,12 +507,16 @@ static int mirror_dirty_init(MirrorBlockJob *s)
>  BlockDriverState *base = s->base;
>  BlockDriverState *bs = blk_bs(s->common.blk);
>  BlockDriverState *target_bs = blk_bs(s->target);
> -bool mark_all_dirty = base == NULL && !bdrv_has_zero_init(target_bs);
>  uint64_t last_pause_ns;
>  int ret, n;
>  
>  end = s->bdev_length / BDRV_SECTOR_SIZE;
>  
> +if (base == NULL && !bdrv_has_zero_init(target_bs)) {
> +bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, end);

Won't work as written.  'end' is 64 bits, but bdrv_set_dirty_bitmap() is
limited to a 32-bit sector count.  Might be first worth updating
bdrv_set_dirty_bitmap() and friends to be byte-based rather than
sector-based (but still tracking a sector per bit, obviously), as well
as expand it to operate on 64-bit sizes rather than 32-bit.

I'm also worried slightly that the existing code repeated things in a
loop, and therefore had pause points every iteration and could thus
remain responsive to an early cancel.  But doing the entire operation in
one chunk (assuming you fix bitmap code to handle a 64-bit size) may end
up running for so long without interruption that you lose the benefits
of an early interruption that you have by virtue of a 32-bit limit.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature


[Qemu-devel] [PATCH 3/9] mirror: optimize dirty bitmap filling in mirror_run a bit

2016-06-14 Thread Denis V. Lunev
There is no need to scan allocation tables if we have mark_all_dirty flag
set. Just mark it all dirty.

Signed-off-by: Denis V. Lunev 
Reviewed-by: Vladimir Sementsov-Ogievskiy
CC: Stefan Hajnoczi 
CC: Fam Zheng 
CC: Kevin Wolf 
CC: Max Reitz 
CC: Jeff Cody 
CC: Eric Blake 
---
 block/mirror.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 797659d..c7b3639 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -507,12 +507,16 @@ static int mirror_dirty_init(MirrorBlockJob *s)
 BlockDriverState *base = s->base;
 BlockDriverState *bs = blk_bs(s->common.blk);
 BlockDriverState *target_bs = blk_bs(s->target);
-bool mark_all_dirty = base == NULL && !bdrv_has_zero_init(target_bs);
 uint64_t last_pause_ns;
 int ret, n;
 
 end = s->bdev_length / BDRV_SECTOR_SIZE;
 
+if (base == NULL && !bdrv_has_zero_init(target_bs)) {
+bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, end);
+return 0;
+}
+
 last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
 
 /* First part, loop on the sectors and initialize the dirty bitmap.  */
@@ -537,7 +541,7 @@ static int mirror_dirty_init(MirrorBlockJob *s)
 }
 
 assert(n > 0);
-if (ret == 1 || mark_all_dirty) {
+if (ret == 1) {
 bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n);
 }
 sector_num += n;
-- 
2.5.0