[v3,2/2] mirror: fix improperly filled copy_bitmap for mirror block job
diff mbox

Message ID 1473957290-13382-3-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev Sept. 15, 2016, 4:34 p.m. UTC
bdrv_is_allocated_above() returns true in the case even for completel
zeroed areas as BDRV_BLOCK_ALLOCATED flag is set in both cases.

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 if possible.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Fam Zheng <famz@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
CC: Jeff Cody <jcody@redhat.com>
---
 block/mirror.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Comments

Eric Blake Sept. 15, 2016, 5:19 p.m. UTC | #1
On 09/15/2016 11:34 AM, Denis V. Lunev wrote:
> bdrv_is_allocated_above() returns true in the case even for completel

s/completel/completely/

> zeroed areas as BDRV_BLOCK_ALLOCATED flag is set in both cases.
> 
> 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 if possible.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Jeff Cody <jcody@redhat.com>
> ---
>  block/mirror.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
Vladimir Sementsov-Ogievskiy Sept. 23, 2016, 11 a.m. UTC | #2
On 15.09.2016 19:34, Denis V. Lunev wrote:
> bdrv_is_allocated_above() returns true in the case even for completel
> zeroed areas as BDRV_BLOCK_ALLOCATED flag is set in both cases.
>
> 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 if possible.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> CC: Jeff Cody <jcody@redhat.com>
> ---
>   block/mirror.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index e0b3f41..2710f62 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -548,11 +548,11 @@ static void mirror_throttle(MirrorBlockJob *s)
>   
>   static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>   {
> -    int64_t sector_num, end;
> +    int64_t sector_num, end, alloc_mask;
>       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;
>   
> @@ -585,11 +585,15 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>           mirror_drain(s);
>       }
>   
> +    alloc_mask = base == NULL ? BDRV_BLOCK_ALLOCATED : BDRV_BLOCK_DATA;

should not s/==/!= ?

> +
>       /* First part, loop on the sectors and initialize the dirty bitmap.  */
>       for (sector_num = 0; sector_num < end; ) {
>           /* 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 +601,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 & alloc_mask) {
>               bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n);
>           }
>           sector_num += n;

Patch
diff mbox

diff --git a/block/mirror.c b/block/mirror.c
index e0b3f41..2710f62 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -548,11 +548,11 @@  static void mirror_throttle(MirrorBlockJob *s)
 
 static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
 {
-    int64_t sector_num, end;
+    int64_t sector_num, end, alloc_mask;
     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;
 
@@ -585,11 +585,15 @@  static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
         mirror_drain(s);
     }
 
+    alloc_mask = base == NULL ? BDRV_BLOCK_ALLOCATED : BDRV_BLOCK_DATA;
+
     /* First part, loop on the sectors and initialize the dirty bitmap.  */
     for (sector_num = 0; sector_num < end; ) {
         /* 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 +601,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 & alloc_mask) {
             bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n);
         }
         sector_num += n;