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

[Vladimir Sementsov-Ogievskiy]

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 <d...@openvz.org>
CC: Stefan Hajnoczi <stefa...@redhat.com>
CC: Fam Zheng <f...@redhat.com>
CC: Kevin Wolf <kw...@redhat.com>
CC: Max Reitz <mre...@redhat.com>
CC: Jeff Cody <jc...@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;

--
Best regards,
Vladimir