[Qemu-devel] [PATCH 1/1] mirror: fix improperly filled copy_bitmap for mirror block job

[Qemu-devel] [PATCH 1/1] mirror: fix improperly filled copy_bitmap for mirror block job

[Denis V. Lunev]

bdrv_is_allocated_above() returns true in the case if qcow2 even for
completely zeroed areas as BDRV_BLOCK_ALLOCATED flag is set in both
cases. Though we have completely zeroed out the image just above or it was
already zeroed.

The patch stops using bdrv_is_allocated_above() wrapper and switches to
bdrv_get_block_status_above() to distinguish zeroed areas and areas with
data to avoid extra IO operations, which could be VERY slow.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
---
 block/mirror.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index e0b3f41..87edbd8 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -552,7 +552,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
     BlockDriverState *base = s->base;
     BlockDriverState *bs = blk_bs(s->common.blk);
     BlockDriverState *target_bs = blk_bs(s->target);
-    int ret, n;
+    int n;
 
     end = s->bdev_length / BDRV_SECTOR_SIZE;
 
@@ -590,6 +590,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
         /* Just to make sure we are not exceeding int limit. */
         int nb_sectors = MIN(INT_MAX >> BDRV_SECTOR_BITS,
                              end - sector_num);
+        int64_t status;
+        BlockDriverState *file;
 
         mirror_throttle(s);
 
@@ -597,13 +599,14 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
             return 0;
         }
 
-        ret = bdrv_is_allocated_above(bs, base, sector_num, nb_sectors, &n);
-        if (ret < 0) {
-            return ret;
+        status = bdrv_get_block_status_above(bs, base, sector_num,
+                                             nb_sectors, &n, &file);
+        if (status < 0) {
+            return status;
         }
 
         assert(n > 0);
-        if (ret == 1) {
+        if (status & BDRV_BLOCK_DATA) {
             bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n);
         }
         sector_num += n;
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/1] mirror: fix improperly filled copy_bitmap for mirror block job

[Jeff Cody]

On Tue, Sep 06, 2016 at 06:11:19PM +0300, Denis V. Lunev wrote:
> bdrv_is_allocated_above() returns true in the case if qcow2 even for
> completely zeroed areas as BDRV_BLOCK_ALLOCATED flag is set in both
> cases.

Hi Denis,

Not just qcow2.  BDRV_BLOCK_ALLOCATED for a layer means the content of the
block is defined by that layer (even if that is a zero block, that doesn't
mean it is unallocated).  This applies to all image formats.

> Though we have completely zeroed out the image just above or it was
> already zeroed.

This is only true if we are in 'sync = full' mode.

> The patch stops using bdrv_is_allocated_above() wrapper and switches to
> bdrv_get_block_status_above() to distinguish zeroed areas and areas with
> data to avoid extra IO operations, which could be VERY slow.

For 'sync = top', we need to make sure to appropriately allocate the sector,
even if it is BDRV_BLOCK_ZERO.

> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
>  block/mirror.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index e0b3f41..87edbd8 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -552,7 +552,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>      BlockDriverState *base = s->base;
>      BlockDriverState *bs = blk_bs(s->common.blk);
>      BlockDriverState *target_bs = blk_bs(s->target);
> -    int ret, n;
> +    int n;
>  
>      end = s->bdev_length / BDRV_SECTOR_SIZE;
>  
> @@ -590,6 +590,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>          /* Just to make sure we are not exceeding int limit. */
>          int nb_sectors = MIN(INT_MAX >> BDRV_SECTOR_BITS,
>                               end - sector_num);
> +        int64_t status;
> +        BlockDriverState *file;
>  
>          mirror_throttle(s);
>  
> @@ -597,13 +599,14 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>              return 0;
>          }
>  
> -        ret = bdrv_is_allocated_above(bs, base, sector_num, nb_sectors, &n);
> -        if (ret < 0) {
> -            return ret;
> +        status = bdrv_get_block_status_above(bs, base, sector_num,
> +                                             nb_sectors, &n, &file);
> +        if (status < 0) {
> +            return status;
>          }
>  
>          assert(n > 0);
> -        if (ret == 1) {
> +        if (status & BDRV_BLOCK_DATA) {

I think this patch would work if this was changed to something like this:

         mark_dirty = base == NULL ? status & BDRV_BLOCK_DATA :     /* 'full' */
                                     status & BDRV_BLOCK_ALLOCATED;

         if (mark_dirty) {


Thanks,
Jeff

>              bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n);
>          }



>          sector_num += n;
> -- 
> 2.7.4
> 

Re: [Qemu-devel] [PATCH 1/1] mirror: fix improperly filled copy_bitmap for mirror block job

[Denis V. Lunev]

On 09/07/2016 09:15 AM, Jeff Cody wrote:
> On Tue, Sep 06, 2016 at 06:11:19PM +0300, Denis V. Lunev wrote:
>> bdrv_is_allocated_above() returns true in the case if qcow2 even for
>> completely zeroed areas as BDRV_BLOCK_ALLOCATED flag is set in both
>> cases.
> Hi Denis,
>
> Not just qcow2.  BDRV_BLOCK_ALLOCATED for a layer means the content of the
> block is defined by that layer (even if that is a zero block, that doesn't
> mean it is unallocated).  This applies to all image formats.
>
>> Though we have completely zeroed out the image just above or it was
>> already zeroed.
> This is only true if we are in 'sync = full' mode.
>
>> The patch stops using bdrv_is_allocated_above() wrapper and switches to
>> bdrv_get_block_status_above() to distinguish zeroed areas and areas with
>> data to avoid extra IO operations, which could be VERY slow.
> For 'sync = top', we need to make sure to appropriately allocate the sector,
> even if it is BDRV_BLOCK_ZERO.
>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Jeff Cody <jcody@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/mirror.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index e0b3f41..87edbd8 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -552,7 +552,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>>      BlockDriverState *base = s->base;
>>      BlockDriverState *bs = blk_bs(s->common.blk);
>>      BlockDriverState *target_bs = blk_bs(s->target);
>> -    int ret, n;
>> +    int n;
>>  
>>      end = s->bdev_length / BDRV_SECTOR_SIZE;
>>  
>> @@ -590,6 +590,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>>          /* Just to make sure we are not exceeding int limit. */
>>          int nb_sectors = MIN(INT_MAX >> BDRV_SECTOR_BITS,
>>                               end - sector_num);
>> +        int64_t status;
>> +        BlockDriverState *file;
>>  
>>          mirror_throttle(s);
>>  
>> @@ -597,13 +599,14 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>>              return 0;
>>          }
>>  
>> -        ret = bdrv_is_allocated_above(bs, base, sector_num, nb_sectors, &n);
>> -        if (ret < 0) {
>> -            return ret;
>> +        status = bdrv_get_block_status_above(bs, base, sector_num,
>> +                                             nb_sectors, &n, &file);
>> +        if (status < 0) {
>> +            return status;
>>          }
>>  
>>          assert(n > 0);
>> -        if (ret == 1) {
>> +        if (status & BDRV_BLOCK_DATA) {
> I think this patch would work if this was changed to something like this:
>
>          mark_dirty = base == NULL ? status & BDRV_BLOCK_DATA :     /* 'full' */
>                                      status & BDRV_BLOCK_ALLOCATED;
>
>          if (mark_dirty) {
reasonable. I have doubts with the code but was unable to
formulate. Thank you for clarifications. Will respin.

Den