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

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

[Denis V. Lunev]

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.

Though this change requires some preparations in bdrv_get_block_status_above()
performed in the patch (1).

Changes from v1:
- fixed assert in 041 test case (added patch 1)
- fixed commit message
- fixed status check to be on the safe side

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>

Denis V. Lunev (2):
  block: sync bdrv_co_get_block_status_above() with
    bdrv_is_allocated_above()
  mirror: fix improperly filled copy_bitmap for mirror block job

 block/io.c     | 26 ++++++++++++++++++++------
 block/mirror.c | 18 ++++++++++++------
 2 files changed, 32 insertions(+), 12 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] block: sync bdrv_co_get_block_status_above() with bdrv_is_allocated_above()

[Denis V. Lunev]

They should work very similar, covering same areas if backing store is
shorter than the image. This change is necessary for the followup patch
switching to bdrv_get_block_status_above() in mirror to avoid assert
in check_block.

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/io.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/block/io.c b/block/io.c
index 420944d..0422123 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1745,14 +1745,28 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
 
     assert(bs != base);
     for (p = bs; p != base; p = backing_bs(p)) {
-        ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file);
-        if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) {
-            break;
+        int sc;
+        ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, &sc, file);
+        if (ret < 0) {
+            return ret;
+        } else if (ret & BDRV_BLOCK_ALLOCATED) {
+            *pnum = sc;
+            return ret;
+        }
+
+        /*
+         * [sector_num, nb_sectors] is unallocated on top but intermediate
+         * might have
+         *
+         * [sector_num+x, nr_sectors] allocated.
+         */
+        if (nb_sectors > sc &&
+            (p == bs || sector_num + sc < p->total_sectors)) {
+            nb_sectors = sc;
         }
-        /* [sector_num, pnum] unallocated on this layer, which could be only
-         * the first part of [sector_num, nb_sectors].  */
-        nb_sectors = MIN(nb_sectors, *pnum);
     }
+
+    *pnum = nb_sectors;
     return ret;
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] block: sync bdrv_co_get_block_status_above() with bdrv_is_allocated_above()

[Vladimir Sementsov-Ogievskiy]

On 09.09.2016 15:31, Denis V. Lunev wrote:
> They should work very similar, covering same areas if backing store is
> shorter than the image. This change is necessary for the followup patch
> switching to bdrv_get_block_status_above() in mirror to avoid assert
> in check_block.
>
> 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/io.c | 26 ++++++++++++++++++++------
>   1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index 420944d..0422123 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1745,14 +1745,28 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
>   
>       assert(bs != base);
>       for (p = bs; p != base; p = backing_bs(p)) {
> -        ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file);
> -        if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) {
> -            break;
> +        int sc;
> +        ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, &sc, file);
> +        if (ret < 0) {
> +            return ret;
> +        } else if (ret & BDRV_BLOCK_ALLOCATED) {
> +            *pnum = sc;
> +            return ret;
> +        }
> +
> +        /*
> +         * [sector_num, nb_sectors] is unallocated on top but intermediate
> +         * might have
> +         *
> +         * [sector_num+x, nr_sectors] allocated.
> +         */

this comment is unrelated here, as you reduce nb_sectors (used in 
bdrv_co_get_block_status() above) in the following "if"

> +        if (nb_sectors > sc &&
> +            (p == bs || sector_num + sc < p->total_sectors)) {
> +            nb_sectors = sc;
>           }
> -        /* [sector_num, pnum] unallocated on this layer, which could be only
> -         * the first part of [sector_num, nb_sectors].  */
> -        nb_sectors = MIN(nb_sectors, *pnum);
>       }
> +
> +    *pnum = nb_sectors;
>       return ret;
>   }
>   


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 1/2] block: sync bdrv_co_get_block_status_above() with bdrv_is_allocated_above()

[Roman Kagan]

On Fri, Sep 09, 2016 at 03:31:47PM +0300, Denis V. Lunev wrote:
> They should work very similar, covering same areas if backing store is
> shorter than the image. This change is necessary for the followup patch
> switching to bdrv_get_block_status_above() in mirror to avoid assert
> in check_block.

I wonder why bdrv_is_allocated_above has to be a separate function
rather than a trivial wrapper around bdrv_get_block_status_above() (like
bdrv_is_allocated() is over bdrv_get_block_status())?

> 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/io.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 420944d..0422123 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1745,14 +1745,28 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,
>  
>      assert(bs != base);
>      for (p = bs; p != base; p = backing_bs(p)) {
> -        ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file);
> -        if (ret < 0 || ret & BDRV_BLOCK_ALLOCATED) {
> -            break;
> +        int sc;
> +        ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, &sc, file);
> +        if (ret < 0) {
> +            return ret;
> +        } else if (ret & BDRV_BLOCK_ALLOCATED) {
> +            *pnum = sc;
> +            return ret;
> +        }
> +
> +        /*
> +         * [sector_num, nb_sectors] is unallocated on top but intermediate
> +         * might have
> +         *
> +         * [sector_num+x, nr_sectors] allocated.
> +         */
> +        if (nb_sectors > sc &&
> +            (p == bs || sector_num + sc < p->total_sectors)) {
> +            nb_sectors = sc;
>          }
> -        /* [sector_num, pnum] unallocated on this layer, which could be only
> -         * the first part of [sector_num, nb_sectors].  */
> -        nb_sectors = MIN(nb_sectors, *pnum);
>      }
> +
> +    *pnum = nb_sectors;
>      return ret;

IIUC in the chain image->backing_1->backing_2, where size(image) >
size(backing_1) and size(backing_1) < size(backing_2), if the status of
blocks beyond size(backing_1) is requested we'll start falling through
to backing_2.  I'm not certain this is desirable.  (And yes, this is
already the case in bdrv_is_allocated_above).

Roman.

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

[Denis V. Lunev]

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 | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index e0b3f41..da55375 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -548,14 +548,15 @@ 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;
 
+    alloc_mask = BDRV_BLOCK_ALLOCATED;
     if (base == NULL && !bdrv_has_zero_init(target_bs)) {
         if (!bdrv_can_write_zeroes_with_unmap(target_bs)) {
             bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, end);
@@ -583,6 +584,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
         }
 
         mirror_drain(s);
+
+        alloc_mask = BDRV_BLOCK_DATA;
     }
 
     /* First part, loop on the sectors and initialize the dirty bitmap.  */
@@ -590,6 +593,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 +602,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;
-- 
2.7.4

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1558 bytes --]

On 09/09/2016 07:31 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 | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 

> @@ -597,13 +602,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);

Eventually, we should probably fix bdrv_get_block_status_above() to be
byte-based, but that's not a problem with this patch.

Looks okay to me, but I haven't thought closely enough about potential
corner cases to feel comfortable with giving R-b yet...


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Jeff Cody]

On Fri, Sep 09, 2016 at 03:31:48PM +0300, 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 | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index e0b3f41..da55375 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -548,14 +548,15 @@ 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;
>  
> +    alloc_mask = BDRV_BLOCK_ALLOCATED;
>      if (base == NULL && !bdrv_has_zero_init(target_bs)) {
>          if (!bdrv_can_write_zeroes_with_unmap(target_bs)) {
>              bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, end);
> @@ -583,6 +584,8 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>          }
>  
>          mirror_drain(s);
> +
> +        alloc_mask = BDRV_BLOCK_DATA;

What about when base == NULL, and bdrv_has_zero_init(target_bs) == true?  In
that case we also know the target image is zeroed, but this does not take
advantage of that.

>      }
>  
>      /* First part, loop on the sectors and initialize the dirty bitmap.  */
> @@ -590,6 +593,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 +602,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;

-Jeff