[PATCH v2 00/20] parallels: Add full dirty bitmap support

[PATCH v2 00/20] parallels: Add full dirty bitmap support

[Alexander Ivanov]

Parallels format driver:
* make some preparation
* add dirty bitmap saving
* make dirty bitmap RW
* fix broken checks
* refactor leak check
* add parallels format support to several tests

You could find these patches in my repo:
https://github.com/AlexanderIvanov-Virtuozzo/qemu/tree/parallels-v2

v2:
1: New patch to fix double free error.
4: Fixed clusters leaks.
15: Fixed (end_off != s->used_bmap_size) handling in parallels_truncate_unused_clusters().
16,17: Changed the sequence of the patches - in this way we have correct leaks check.

Alexander Ivanov (20):
  parallels: Set s->used_bmap to NULL in parallels_free_used_bitmap()
  parallels: Move inactivation code to a separate function
  parallels: Add mark_unused() helper
  parallels: Move host clusters allocation to a separate function
  parallels: Set data_end value in parallels_check_leak()
  parallels: Recreate used bitmap in parallels_check_leak()
  parallels: Add a note about used bitmap in parallels_check_duplicate()
  parallels: Create used bitmap even if checks needed
  parallels: Make mark_used() and mark_unused() global functions
  parallels: Add dirty bitmaps saving
  parallels: Let image extensions work in RW mode
  parallels: Handle L1 entries equal to one
  parallels: Make a loaded dirty bitmap persistent
  parallels: Reverse a conditional in parallels_check_leak() to reduce
    indents
  parallels: Truncate images on the last used cluster
  parallels: Check unused clusters in parallels_check_leak()
  parallels: Remove unnecessary data_end field
  tests: Add parallels images support to test 165
  tests: Turned on 256, 299, 304 and block-status-cache for parallels
    format
  tests: Add parallels format support to image-fleecing

 block/parallels-ext.c                       | 182 +++++++++-
 block/parallels.c                           | 361 ++++++++++++--------
 block/parallels.h                           |  15 +-
 tests/qemu-iotests/165                      |  40 ++-
 tests/qemu-iotests/256                      |   2 +-
 tests/qemu-iotests/299                      |   2 +-
 tests/qemu-iotests/304                      |   2 +-
 tests/qemu-iotests/tests/block-status-cache |   2 +-
 tests/qemu-iotests/tests/image-fleecing     |  13 +-
 9 files changed, 447 insertions(+), 172 deletions(-)

-- 
2.34.1

[PATCH v2 01/20] parallels: Set s->used_bmap to NULL in parallels_free_used_bitmap()

[Alexander Ivanov]

After used bitmap freeng s->used_bmap points to the freed memory. If we try
to free used bitmap one more time it leads to double free error.

Set s->used_bmap to NULL to exclude double free error.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/parallels.c b/block/parallels.c
index 6b46623241..ba1fdde259 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -244,6 +244,7 @@ static void parallels_free_used_bitmap(BlockDriverState *bs)
 {
     BDRVParallelsState *s = bs->opaque;
     s->used_bmap_size = 0;
+    s->used_bmap = NULL;
     g_free(s->used_bmap);
 }
 
-- 
2.34.1

Re: [PATCH v2 01/20] parallels: Set s->used_bmap to NULL in parallels_free_used_bitmap()

[Mike Maslenkin]

On Thu, Oct 19, 2023 at 4:06 PM Alexander Ivanov
<alexander.ivanov@virtuozzo.com> wrote:
>
> After used bitmap freeng s->used_bmap points to the freed memory. If we try
> to free used bitmap one more time it leads to double free error.
>
> Set s->used_bmap to NULL to exclude double free error.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>  block/parallels.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 6b46623241..ba1fdde259 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -244,6 +244,7 @@ static void parallels_free_used_bitmap(BlockDriverState *bs)
>  {
>      BDRVParallelsState *s = bs->opaque;
>      s->used_bmap_size = 0;
> +    s->used_bmap = NULL;
>      g_free(s->used_bmap);
>  }

Shouldn't it be added after g_free() call?

Regards,
Mike.

Re: [PATCH v2 01/20] parallels: Set s->used_bmap to NULL in parallels_free_used_bitmap()

[Alexander Ivanov]

On 10/21/23 12:40, Mike Maslenkin wrote:
> On Thu, Oct 19, 2023 at 4:06 PM Alexander Ivanov
> <alexander.ivanov@virtuozzo.com> wrote:
>> After used bitmap freeng s->used_bmap points to the freed memory. If we try
>> to free used bitmap one more time it leads to double free error.
>>
>> Set s->used_bmap to NULL to exclude double free error.
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>>   block/parallels.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 6b46623241..ba1fdde259 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -244,6 +244,7 @@ static void parallels_free_used_bitmap(BlockDriverState *bs)
>>   {
>>       BDRVParallelsState *s = bs->opaque;
>>       s->used_bmap_size = 0;
>> +    s->used_bmap = NULL;
>>       g_free(s->used_bmap);
>>   }
> Shouldn't it be added after g_free() call?
OMG, of course.
>
> Regards,
> Mike.

-- 
Best regards,
Alexander Ivanov

[PATCH v2 02/20] parallels: Move inactivation code to a separate function

[Alexander Ivanov]

We are going to add parallels image extensions storage and need a separate
function for inactivation code.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index ba1fdde259..7eb92f87ab 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1430,18 +1430,27 @@ fail:
     return ret;
 }
 
+static int parallels_inactivate(BlockDriverState *bs)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int ret;
+
+    s->header->inuse = 0;
+    parallels_update_header(bs);
+
+    /* errors are ignored, so we might as well pass exact=true */
+    ret = bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, true,
+                        PREALLOC_MODE_OFF, 0, NULL);
+
+    return ret;
+}
 
 static void parallels_close(BlockDriverState *bs)
 {
     BDRVParallelsState *s = bs->opaque;
 
     if ((bs->open_flags & BDRV_O_RDWR) && !(bs->open_flags & BDRV_O_INACTIVE)) {
-        s->header->inuse = 0;
-        parallels_update_header(bs);
-
-        /* errors are ignored, so we might as well pass exact=true */
-        bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, true,
-                      PREALLOC_MODE_OFF, 0, NULL);
+        parallels_inactivate(bs);
     }
 
     parallels_free_used_bitmap(bs);
@@ -1481,6 +1490,7 @@ static BlockDriver bdrv_parallels = {
     .bdrv_co_check              = parallels_co_check,
     .bdrv_co_pdiscard           = parallels_co_pdiscard,
     .bdrv_co_pwrite_zeroes      = parallels_co_pwrite_zeroes,
+    .bdrv_inactivate            = parallels_inactivate,
 };
 
 static void bdrv_parallels_init(void)
-- 
2.34.1

[PATCH v2 03/20] parallels: Add mark_unused() helper

[Alexander Ivanov]

Add a helper to set unused areas in used bitmap.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 7eb92f87ab..6a4e3945c6 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -195,6 +195,23 @@ static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
     return 0;
 }
 
+static int mark_unused(BlockDriverState *bs, unsigned long *bitmap,
+                       uint32_t bitmap_size, int64_t off, uint32_t count)
+{
+    BDRVParallelsState *s = bs->opaque;
+    uint32_t cluster_index = host_cluster_index(s, off);
+    unsigned long next_unused;
+    if (cluster_index + count > bitmap_size) {
+        return -E2BIG;
+    }
+    next_unused = find_next_zero_bit(bitmap, bitmap_size, cluster_index);
+    if (next_unused < cluster_index + count) {
+        return -EINVAL;
+    }
+    bitmap_clear(bitmap, cluster_index, count);
+    return 0;
+}
+
 /*
  * Collect used bitmap. The image can contain errors, we should fill the
  * bitmap anyway, as much as we can. This information will be used for
-- 
2.34.1

[PATCH v2 04/20] parallels: Move host clusters allocation to a separate function

[Alexander Ivanov]

For parallels images extensions we need to allocate host clusters
without any connection to BAT. Move host clusters allocation code to
allocate_host_clusters().

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 126 ++++++++++++++++++++++++----------------------
 block/parallels.h |   4 ++
 2 files changed, 71 insertions(+), 59 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 6a4e3945c6..ce97387e3e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -265,58 +265,31 @@ static void parallels_free_used_bitmap(BlockDriverState *bs)
     g_free(s->used_bmap);
 }
 
-static int64_t coroutine_fn GRAPH_RDLOCK
-allocate_clusters(BlockDriverState *bs, int64_t sector_num,
-                  int nb_sectors, int *pnum)
+int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
+                                         int64_t *clusters)
 {
-    int ret = 0;
     BDRVParallelsState *s = bs->opaque;
-    int64_t i, pos, idx, to_allocate, first_free, host_off;
-
-    pos = block_status(s, sector_num, nb_sectors, pnum);
-    if (pos > 0) {
-        return pos;
-    }
-
-    idx = sector_num / s->tracks;
-    to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx;
-
-    /*
-     * This function is called only by parallels_co_writev(), which will never
-     * pass a sector_num at or beyond the end of the image (because the block
-     * layer never passes such a sector_num to that function). Therefore, idx
-     * is always below s->bat_size.
-     * block_status() will limit *pnum so that sector_num + *pnum will not
-     * exceed the image end. Therefore, idx + to_allocate cannot exceed
-     * s->bat_size.
-     * Note that s->bat_size is an unsigned int, therefore idx + to_allocate
-     * will always fit into a uint32_t.
-     */
-    assert(idx < s->bat_size && idx + to_allocate <= s->bat_size);
+    int64_t first_free, next_used, host_off, prealloc_clusters;
+    int64_t bytes, prealloc_bytes;
+    uint32_t new_usedsize;
+    int ret = 0;
 
     first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
     if (first_free == s->used_bmap_size) {
-        uint32_t new_usedsize;
-        int64_t bytes = to_allocate * s->cluster_size;
-        bytes += s->prealloc_size * BDRV_SECTOR_SIZE;
-
         host_off = s->data_end * BDRV_SECTOR_SIZE;
+        prealloc_clusters = *clusters + s->prealloc_size / s->tracks;
+        bytes = *clusters * s->cluster_size;
+        prealloc_bytes = prealloc_clusters * s->cluster_size;
 
-        /*
-         * We require the expanded size to read back as zero. If the
-         * user permitted truncation, we try that; but if it fails, we
-         * force the safer-but-slower fallocate.
-         */
         if (s->prealloc_mode == PRL_PREALLOC_MODE_TRUNCATE) {
-            ret = bdrv_co_truncate(bs->file, host_off + bytes,
-                                   false, PREALLOC_MODE_OFF,
-                                   BDRV_REQ_ZERO_WRITE, NULL);
+            ret = bdrv_truncate(bs->file, host_off + prealloc_bytes, false,
+                                PREALLOC_MODE_OFF, BDRV_REQ_ZERO_WRITE, NULL);
             if (ret == -ENOTSUP) {
                 s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
             }
         }
         if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
-            ret = bdrv_co_pwrite_zeroes(bs->file, host_off, bytes, 0);
+            ret = bdrv_pwrite_zeroes(bs->file, host_off, prealloc_bytes, 0);
         }
         if (ret < 0) {
             return ret;
@@ -326,15 +299,15 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
         s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
                                           new_usedsize);
         s->used_bmap_size = new_usedsize;
+        if (host_off + bytes > s->data_end * BDRV_SECTOR_SIZE) {
+            s->data_end = (host_off + bytes) / BDRV_SECTOR_SIZE;
+        }
     } else {
-        int64_t next_used;
         next_used = find_next_bit(s->used_bmap, s->used_bmap_size, first_free);
 
         /* Not enough continuous clusters in the middle, adjust the size */
-        if (next_used - first_free < to_allocate) {
-            to_allocate = next_used - first_free;
-            *pnum = (idx + to_allocate) * s->tracks - sector_num;
-        }
+        *clusters = MIN(*clusters, next_used - first_free);
+        bytes = *clusters * s->cluster_size;
 
         host_off = s->data_start * BDRV_SECTOR_SIZE;
         host_off += first_free * s->cluster_size;
@@ -346,14 +319,58 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
          */
         if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE &&
                 host_off < s->data_end * BDRV_SECTOR_SIZE) {
-            ret = bdrv_co_pwrite_zeroes(bs->file, host_off,
-                                        s->cluster_size * to_allocate, 0);
+            ret = bdrv_pwrite_zeroes(bs->file, host_off, bytes, 0);
             if (ret < 0) {
                 return ret;
             }
         }
     }
 
+    ret = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, *clusters);
+    if (ret < 0) {
+        /* Image consistency is broken. Alarm! */
+        return ret;
+    }
+
+    return host_off;
+}
+
+static int64_t coroutine_fn GRAPH_RDLOCK
+allocate_clusters(BlockDriverState *bs, int64_t sector_num,
+                  int nb_sectors, int *pnum)
+{
+    int ret = 0;
+    BDRVParallelsState *s = bs->opaque;
+    int64_t i, pos, idx, to_allocate, host_off;
+
+    pos = block_status(s, sector_num, nb_sectors, pnum);
+    if (pos > 0) {
+        return pos;
+    }
+
+    idx = sector_num / s->tracks;
+    to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx;
+
+    /*
+     * This function is called only by parallels_co_writev(), which will never
+     * pass a sector_num at or beyond the end of the image (because the block
+     * layer never passes such a sector_num to that function). Therefore, idx
+     * is always below s->bat_size.
+     * block_status() will limit *pnum so that sector_num + *pnum will not
+     * exceed the image end. Therefore, idx + to_allocate cannot exceed
+     * s->bat_size.
+     * Note that s->bat_size is an unsigned int, therefore idx + to_allocate
+     * will always fit into a uint32_t.
+     */
+    assert(idx < s->bat_size && idx + to_allocate <= s->bat_size);
+
+    host_off = parallels_allocate_host_clusters(bs, &to_allocate);
+    if (host_off < 0) {
+        return host_off;
+    }
+
+    *pnum = MIN(*pnum, (idx + to_allocate) * s->tracks - sector_num);
+
     /*
      * Try to read from backing to fill empty clusters
      * FIXME: 1. previous write_zeroes may be redundant
@@ -370,32 +387,23 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
 
         ret = bdrv_co_pread(bs->backing, idx * s->tracks * BDRV_SECTOR_SIZE,
                             nb_cow_bytes, buf, 0);
-        if (ret < 0) {
-            qemu_vfree(buf);
-            return ret;
+        if (ret == 0) {
+            ret = bdrv_co_pwrite(bs->file, host_off, nb_cow_bytes, buf, 0);
         }
 
-        ret = bdrv_co_pwrite(bs->file, s->data_end * BDRV_SECTOR_SIZE,
-                             nb_cow_bytes, buf, 0);
         qemu_vfree(buf);
         if (ret < 0) {
+            mark_unused(bs, s->used_bmap, s->used_bmap_size,
+                        host_off, to_allocate);
             return ret;
         }
     }
 
-    ret = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, to_allocate);
-    if (ret < 0) {
-        /* Image consistency is broken. Alarm! */
-        return ret;
-    }
     for (i = 0; i < to_allocate; i++) {
         parallels_set_bat_entry(s, idx + i,
                 host_off / BDRV_SECTOR_SIZE / s->off_multiplier);
         host_off += s->cluster_size;
     }
-    if (host_off > s->data_end * BDRV_SECTOR_SIZE) {
-        s->data_end = host_off / BDRV_SECTOR_SIZE;
-    }
 
     return bat2sect(s, idx) + sector_num % s->tracks;
 }
diff --git a/block/parallels.h b/block/parallels.h
index 6b199443cf..3e4f397502 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -90,7 +90,11 @@ typedef struct BDRVParallelsState {
     Error *migration_blocker;
 } BDRVParallelsState;
 
+int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
+                                         int64_t *clusters);
+
 int parallels_read_format_extension(BlockDriverState *bs,
                                     int64_t ext_off, Error **errp);
 
+
 #endif
-- 
2.34.1

[PATCH v2 05/20] parallels: Set data_end value in parallels_check_leak()

[Alexander Ivanov]

In parallels_check_leak() we change file size but don't correct data_end
field of BDRVParallelsState structure. Fix it.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/parallels.c b/block/parallels.c
index ce97387e3e..ba9fc46279 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -806,6 +806,7 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
                 res->check_errors++;
                 return ret;
             }
+            s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
             if (explicit) {
                 res->leaks_fixed += count;
             }
-- 
2.34.1

[PATCH v2 06/20] parallels: Recreate used bitmap in parallels_check_leak()

[Alexander Ivanov]

In parallels_check_leak() file can be truncated. In this case the used
bitmap would not comply to the file. Recreate the bitmap after file
truncation.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index ba9fc46279..dad2bf67cf 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -807,6 +807,14 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
                 return ret;
             }
             s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
+
+            parallels_free_used_bitmap(bs);
+            ret = parallels_fill_used_bitmap(bs);
+            if (ret == -ENOMEM) {
+                res->check_errors++;
+                return ret;
+            }
+
             if (explicit) {
                 res->leaks_fixed += count;
             }
-- 
2.34.1

[PATCH v2 07/20] parallels: Add a note about used bitmap in parallels_check_duplicate()

[Alexander Ivanov]

In parallels_check_duplicate() We use a bitmap for duplication detection.
This bitmap is not related to used_bmap field in BDRVParallelsState. Add
a comment about it to avoid confusion.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/parallels.c b/block/parallels.c
index dad2bf67cf..dd8ec925f3 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -837,7 +837,10 @@ parallels_check_duplicate(BlockDriverState *bs, BdrvCheckResult *res,
     bool fixed = false;
 
     /*
-     * Create a bitmap of used clusters.
+     * Create a bitmap of used clusters. Please note that this bitmap is not
+     * related to used_bmap field in BDRVParallelsState and is created only for
+     * local usage.
+     *
      * If a bit is set, there is a BAT entry pointing to this cluster.
      * Loop through the BAT entries, check bits relevant to an entry offset.
      * If bit is set, this entry is duplicated. Otherwise set the bit.
-- 
2.34.1

[PATCH v2 08/20] parallels: Create used bitmap even if checks needed

[Alexander Ivanov]

All the checks were fixed to work with used bitmap. Create used bitmap in
parallels_open() even if need_check is true.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index dd8ec925f3..a22ab7f2fc 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1422,13 +1422,11 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     }
     need_check = need_check || s->data_end > file_nb_sectors;
 
-    if (!need_check) {
-        ret = parallels_fill_used_bitmap(bs);
-        if (ret == -ENOMEM) {
-            goto fail;
-        }
-        need_check = need_check || ret < 0; /* These are correctable errors */
+    ret = parallels_fill_used_bitmap(bs);
+    if (ret == -ENOMEM) {
+        goto fail;
     }
+    need_check = need_check || ret < 0; /* These are correctable errors */
 
     /*
      * We don't repair the image here if it's opened for checks. Also we don't
-- 
2.34.1

[PATCH v2 09/20] parallels: Make mark_used() and mark_unused() global functions

[Alexander Ivanov]

We will need these functions in parallels-ext.c too. Let them be global
functions parallels_mark_used() and parallels_mark_unused().

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 22 ++++++++++++----------
 block/parallels.h |  5 +++++
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index a22ab7f2fc..2ee2b42038 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -178,8 +178,8 @@ static void parallels_set_bat_entry(BDRVParallelsState *s,
     bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block, 1);
 }
 
-static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
-                     uint32_t bitmap_size, int64_t off, uint32_t count)
+int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
+                        uint32_t bitmap_size, int64_t off, uint32_t count)
 {
     BDRVParallelsState *s = bs->opaque;
     uint32_t cluster_index = host_cluster_index(s, off);
@@ -195,8 +195,8 @@ static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
     return 0;
 }
 
-static int mark_unused(BlockDriverState *bs, unsigned long *bitmap,
-                       uint32_t bitmap_size, int64_t off, uint32_t count)
+int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
+                          uint32_t bitmap_size, int64_t off, uint32_t count)
 {
     BDRVParallelsState *s = bs->opaque;
     uint32_t cluster_index = host_cluster_index(s, off);
@@ -249,7 +249,8 @@ static int parallels_fill_used_bitmap(BlockDriverState *bs)
             continue;
         }
 
-        err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, 1);
+        err2 = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
+                                   host_off, 1);
         if (err2 < 0 && err == 0) {
             err = err2;
         }
@@ -326,7 +327,8 @@ int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
         }
     }
 
-    ret = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, *clusters);
+    ret = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
+                              host_off, *clusters);
     if (ret < 0) {
         /* Image consistency is broken. Alarm! */
         return ret;
@@ -393,8 +395,8 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
 
         qemu_vfree(buf);
         if (ret < 0) {
-            mark_unused(bs, s->used_bmap, s->used_bmap_size,
-                        host_off, to_allocate);
+            parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
+                                  host_off, to_allocate);
             return ret;
         }
     }
@@ -868,7 +870,7 @@ parallels_check_duplicate(BlockDriverState *bs, BdrvCheckResult *res,
             continue;
         }
 
-        ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
+        ret = parallels_mark_used(bs, bitmap, bitmap_size, host_off, 1);
         assert(ret != -E2BIG);
         if (ret == 0) {
             continue;
@@ -928,7 +930,7 @@ parallels_check_duplicate(BlockDriverState *bs, BdrvCheckResult *res,
          * considered, and the bitmap size doesn't change. This specifically
          * means that -E2BIG is OK.
          */
-        ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
+        ret = parallels_mark_used(bs, bitmap, bitmap_size, host_off, 1);
         if (ret == -EBUSY) {
             res->check_errors++;
             goto out_repair_bat;
diff --git a/block/parallels.h b/block/parallels.h
index 3e4f397502..4e7aa6b80f 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -90,6 +90,11 @@ typedef struct BDRVParallelsState {
     Error *migration_blocker;
 } BDRVParallelsState;
 
+int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
+                        uint32_t bitmap_size, int64_t off, uint32_t count);
+int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
+                          uint32_t bitmap_size, int64_t off, uint32_t count);
+
 int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
                                          int64_t *clusters);
 
-- 
2.34.1

Re: [PATCH v2 09/20] parallels: Make mark_used() and mark_unused() global functions

[Mike Maslenkin]

On Thu, Oct 19, 2023 at 5:23 PM Alexander Ivanov
<alexander.ivanov@virtuozzo.com> wrote:
>
> We will need these functions in parallels-ext.c too. Let them be global
> functions parallels_mark_used() and parallels_mark_unused().
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>  block/parallels.c | 22 ++++++++++++----------
>  block/parallels.h |  5 +++++
>  2 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index a22ab7f2fc..2ee2b42038 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -178,8 +178,8 @@ static void parallels_set_bat_entry(BDRVParallelsState *s,
>      bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block, 1);
>  }
>
> -static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
> -                     uint32_t bitmap_size, int64_t off, uint32_t count)
> +int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
> +                        uint32_t bitmap_size, int64_t off, uint32_t count)
>  {
>      BDRVParallelsState *s = bs->opaque;
>      uint32_t cluster_index = host_cluster_index(s, off);
> @@ -195,8 +195,8 @@ static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
>      return 0;
>  }
>
> -static int mark_unused(BlockDriverState *bs, unsigned long *bitmap,
> -                       uint32_t bitmap_size, int64_t off, uint32_t count)
> +int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
> +                          uint32_t bitmap_size, int64_t off, uint32_t count)
>  {
>      BDRVParallelsState *s = bs->opaque;
>      uint32_t cluster_index = host_cluster_index(s, off);
> @@ -249,7 +249,8 @@ static int parallels_fill_used_bitmap(BlockDriverState *bs)
>              continue;
>          }
>
> -        err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, 1);
> +        err2 = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
> +                                   host_off, 1);
>          if (err2 < 0 && err == 0) {
>              err = err2;
>          }
> @@ -326,7 +327,8 @@ int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
>          }
>      }
>
> -    ret = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, *clusters);
> +    ret = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
> +                              host_off, *clusters);
>      if (ret < 0) {
>          /* Image consistency is broken. Alarm! */
>          return ret;
> @@ -393,8 +395,8 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
>
>          qemu_vfree(buf);
>          if (ret < 0) {
> -            mark_unused(bs, s->used_bmap, s->used_bmap_size,
> -                        host_off, to_allocate);
> +            parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
> +                                  host_off, to_allocate);
>              return ret;
>          }
>      }
> @@ -868,7 +870,7 @@ parallels_check_duplicate(BlockDriverState *bs, BdrvCheckResult *res,
>              continue;
>          }
>
> -        ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
> +        ret = parallels_mark_used(bs, bitmap, bitmap_size, host_off, 1);
>          assert(ret != -E2BIG);
>          if (ret == 0) {
>              continue;
> @@ -928,7 +930,7 @@ parallels_check_duplicate(BlockDriverState *bs, BdrvCheckResult *res,
>           * considered, and the bitmap size doesn't change. This specifically
>           * means that -E2BIG is OK.
>           */
> -        ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
> +        ret = parallels_mark_used(bs, bitmap, bitmap_size, host_off, 1);
>          if (ret == -EBUSY) {
>              res->check_errors++;
>              goto out_repair_bat;
> diff --git a/block/parallels.h b/block/parallels.h
> index 3e4f397502..4e7aa6b80f 100644
> --- a/block/parallels.h
> +++ b/block/parallels.h
> @@ -90,6 +90,11 @@ typedef struct BDRVParallelsState {
>      Error *migration_blocker;
>  } BDRVParallelsState;
>
> +int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
> +                        uint32_t bitmap_size, int64_t off, uint32_t count);
> +int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
> +                          uint32_t bitmap_size, int64_t off, uint32_t count);
> +
>  int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
>                                           int64_t *clusters);
>
> --
> 2.34.1
>
>

Just a note: parallels_mark_unused() could be initially declared as
global just because after patch 3/20 there can be compilation warning:
warning: unused function 'mark_unused' [-Wunused-function]
:)

I do not have strong opinion about how to avoid such compilation
warning in the middle of the patch series.
The simplest and straightforward way is to declare this function as
static in patch 4.

I do not have any other objections for the series except misplaced
NULL assignment.

Regards,
Mike.

Re: [PATCH v2 09/20] parallels: Make mark_used() and mark_unused() global functions

[Alexander Ivanov]

On 10/21/23 15:29, Mike Maslenkin wrote:
> On Thu, Oct 19, 2023 at 5:23 PM Alexander Ivanov
> <alexander.ivanov@virtuozzo.com> wrote:
>> We will need these functions in parallels-ext.c too. Let them be global
>> functions parallels_mark_used() and parallels_mark_unused().
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>>   block/parallels.c | 22 ++++++++++++----------
>>   block/parallels.h |  5 +++++
>>   2 files changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index a22ab7f2fc..2ee2b42038 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -178,8 +178,8 @@ static void parallels_set_bat_entry(BDRVParallelsState *s,
>>       bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block, 1);
>>   }
>>
>> -static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
>> -                     uint32_t bitmap_size, int64_t off, uint32_t count)
>> +int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
>> +                        uint32_t bitmap_size, int64_t off, uint32_t count)
>>   {
>>       BDRVParallelsState *s = bs->opaque;
>>       uint32_t cluster_index = host_cluster_index(s, off);
>> @@ -195,8 +195,8 @@ static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
>>       return 0;
>>   }
>>
>> -static int mark_unused(BlockDriverState *bs, unsigned long *bitmap,
>> -                       uint32_t bitmap_size, int64_t off, uint32_t count)
>> +int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
>> +                          uint32_t bitmap_size, int64_t off, uint32_t count)
>>   {
>>       BDRVParallelsState *s = bs->opaque;
>>       uint32_t cluster_index = host_cluster_index(s, off);
>> @@ -249,7 +249,8 @@ static int parallels_fill_used_bitmap(BlockDriverState *bs)
>>               continue;
>>           }
>>
>> -        err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, 1);
>> +        err2 = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
>> +                                   host_off, 1);
>>           if (err2 < 0 && err == 0) {
>>               err = err2;
>>           }
>> @@ -326,7 +327,8 @@ int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
>>           }
>>       }
>>
>> -    ret = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, *clusters);
>> +    ret = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
>> +                              host_off, *clusters);
>>       if (ret < 0) {
>>           /* Image consistency is broken. Alarm! */
>>           return ret;
>> @@ -393,8 +395,8 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
>>
>>           qemu_vfree(buf);
>>           if (ret < 0) {
>> -            mark_unused(bs, s->used_bmap, s->used_bmap_size,
>> -                        host_off, to_allocate);
>> +            parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
>> +                                  host_off, to_allocate);
>>               return ret;
>>           }
>>       }
>> @@ -868,7 +870,7 @@ parallels_check_duplicate(BlockDriverState *bs, BdrvCheckResult *res,
>>               continue;
>>           }
>>
>> -        ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
>> +        ret = parallels_mark_used(bs, bitmap, bitmap_size, host_off, 1);
>>           assert(ret != -E2BIG);
>>           if (ret == 0) {
>>               continue;
>> @@ -928,7 +930,7 @@ parallels_check_duplicate(BlockDriverState *bs, BdrvCheckResult *res,
>>            * considered, and the bitmap size doesn't change. This specifically
>>            * means that -E2BIG is OK.
>>            */
>> -        ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
>> +        ret = parallels_mark_used(bs, bitmap, bitmap_size, host_off, 1);
>>           if (ret == -EBUSY) {
>>               res->check_errors++;
>>               goto out_repair_bat;
>> diff --git a/block/parallels.h b/block/parallels.h
>> index 3e4f397502..4e7aa6b80f 100644
>> --- a/block/parallels.h
>> +++ b/block/parallels.h
>> @@ -90,6 +90,11 @@ typedef struct BDRVParallelsState {
>>       Error *migration_blocker;
>>   } BDRVParallelsState;
>>
>> +int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
>> +                        uint32_t bitmap_size, int64_t off, uint32_t count);
>> +int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
>> +                          uint32_t bitmap_size, int64_t off, uint32_t count);
>> +
>>   int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
>>                                            int64_t *clusters);
>>
>> --
>> 2.34.1
>>
>>
> Just a note: parallels_mark_unused() could be initially declared as
> global just because after patch 3/20 there can be compilation warning:
> warning: unused function 'mark_unused' [-Wunused-function]
> :)
>
> I do not have strong opinion about how to avoid such compilation
> warning in the middle of the patch series.
> The simplest and straightforward way is to declare this function as
> static in patch 4.
Will pay attention, thanks.
>
> I do not have any other objections for the series except misplaced
> NULL assignment.
>
> Regards,
> Mike.

-- 
Best regards,
Alexander Ivanov

[PATCH v2 10/20] parallels: Add dirty bitmaps saving

[Alexander Ivanov]

Now dirty bitmaps can be loaded but there is no their saving. Add code for
dirty bitmap storage.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels-ext.c | 167 ++++++++++++++++++++++++++++++++++++++++++
 block/parallels.c     |  16 +++-
 block/parallels.h     |   5 ++
 3 files changed, 186 insertions(+), 2 deletions(-)

diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index 8a109f005a..0a632a2331 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -24,6 +24,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "block/block-io.h"
 #include "block/block_int.h"
@@ -301,3 +302,169 @@ out:
 
     return ret;
 }
+
+static void parallels_save_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+                                  uint8_t **buf, int *buf_size)
+{
+    BDRVParallelsState *s = bs->opaque;
+    ParallelsFeatureHeader *fh;
+    ParallelsDirtyBitmapFeature *bh;
+    uint64_t *l1_table, l1_size, granularity, limit;
+    int64_t bm_size, ser_size, offset, buf_used;
+    int64_t alloc_size = 1;
+    const char *name;
+    uint8_t *bm_buf;
+    QemuUUID uuid;
+    int ret = 0;
+
+    if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
+        bdrv_dirty_bitmap_inconsistent(bitmap)) {
+        return;
+    }
+
+    bm_size = bdrv_dirty_bitmap_size(bitmap);
+    granularity = bdrv_dirty_bitmap_granularity(bitmap);
+    limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap);
+    ser_size = bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size);
+    l1_size = DIV_ROUND_UP(ser_size, s->cluster_size);
+
+    buf_used = l1_size * 8 + sizeof(*fh) + sizeof(*bh);
+    /* Check if there is enough space for the final section */
+    if (*buf_size - buf_used < sizeof(*fh)) {
+        return;
+    }
+
+    name = bdrv_dirty_bitmap_name(bitmap);
+    ret = qemu_uuid_parse(name, &uuid);
+    if (ret < 0) {
+        error_report("Can't save dirty bitmap: ID parsing error: '%s'", name);
+        return;
+    }
+
+    fh = (ParallelsFeatureHeader *)*buf;
+    bh = (ParallelsDirtyBitmapFeature *)(*buf + sizeof(*fh));
+    l1_table = (uint64_t *)((uint8_t *)bh + sizeof(*bh));
+
+    fh->magic = cpu_to_le64(PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC);
+    fh->data_size = cpu_to_le32(l1_size * 8 + sizeof(*bh));
+
+    bh->l1_size = cpu_to_le32(l1_size);
+    bh->size = cpu_to_le64(bm_size >> BDRV_SECTOR_BITS);
+    bh->granularity = cpu_to_le32(granularity >> BDRV_SECTOR_BITS);
+    memcpy(bh->id, &uuid, sizeof(uuid));
+
+    bm_buf = qemu_blockalign(bs, s->cluster_size);
+
+    offset = 0;
+    while ((offset = bdrv_dirty_bitmap_next_dirty(bitmap, offset, bm_size)) >= 0) {
+        uint64_t idx = offset / limit;
+        int64_t cluster_off, end, write_size;
+
+        offset = QEMU_ALIGN_DOWN(offset, limit);
+        end = MIN(bm_size, offset + limit);
+        write_size = bdrv_dirty_bitmap_serialization_size(bitmap, offset,
+                                                          end - offset);
+        assert(write_size <= s->cluster_size);
+
+        bdrv_dirty_bitmap_serialize_part(bitmap, bm_buf, offset, end - offset);
+        if (write_size < s->cluster_size) {
+            memset(bm_buf + write_size, 0, s->cluster_size - write_size);
+        }
+
+        cluster_off = parallels_allocate_host_clusters(bs, &alloc_size);
+        if (cluster_off <= 0) {
+            goto end;
+        }
+
+        ret = bdrv_pwrite(bs->file, cluster_off, s->cluster_size, bm_buf, 0);
+        if (ret < 0) {
+            memset(&fh->magic, 0, sizeof(fh->magic));
+            parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
+                                  cluster_off, 1);
+            goto end;
+        }
+
+        l1_table[idx] = cpu_to_le64(cluster_off >> BDRV_SECTOR_BITS);
+        offset = end;
+    }
+
+    *buf_size -= buf_used;
+    *buf += buf_used;
+
+end:
+    qemu_vfree(bm_buf);
+}
+
+void parallels_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+                                              Error **errp)
+{
+    BDRVParallelsState *s = bs->opaque;
+    BdrvDirtyBitmap *bitmap;
+    ParallelsFormatExtensionHeader *eh;
+    int remaining = s->cluster_size;
+    uint8_t *buf, *pos;
+    int64_t header_off, alloc_size = 1;
+    g_autofree uint8_t *hash = NULL;
+    size_t hash_len = 0;
+    int ret;
+
+    s->header->ext_off = 0;
+
+    if (!bdrv_has_named_bitmaps(bs)) {
+        return;
+    }
+
+    buf = qemu_blockalign0(bs, s->cluster_size);
+
+    eh = (ParallelsFormatExtensionHeader *)buf;
+    pos = buf + sizeof(*eh);
+
+    eh->magic = cpu_to_le64(PARALLELS_FORMAT_EXTENSION_MAGIC);
+
+    FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
+        parallels_save_bitmap(bs, bitmap, &pos, &remaining);
+    }
+
+    header_off = parallels_allocate_host_clusters(bs, &alloc_size);
+    if (header_off < 0) {
+        error_report("Can't save dirty bitmap: cluster allocation error");
+        ret = header_off;
+        goto end;
+    }
+
+    ret = qcrypto_hash_bytes(QCRYPTO_HASH_ALG_MD5,
+                             (const char *)(buf + sizeof(*eh)),
+                             s->cluster_size - sizeof(*eh),
+                             &hash, &hash_len, errp);
+    if (ret < 0 || hash_len != sizeof(eh->check_sum)) {
+        error_report("Can't save dirty bitmap: hash error");
+        ret = -EINVAL;
+        goto end;
+    }
+    memcpy(eh->check_sum, hash, hash_len);
+
+    ret = bdrv_pwrite(bs->file, header_off, s->cluster_size, buf, 0);
+    if (ret < 0) {
+        error_report("Can't save dirty bitmap: IO error");
+        parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
+                              header_off, 1);
+        goto end;
+    }
+
+    s->header->ext_off = cpu_to_le64(header_off / BDRV_SECTOR_SIZE);
+end:
+    qemu_vfree(buf);
+}
+
+bool coroutine_fn parallels_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
+                                                          const char *name,
+                                                          uint32_t granularity,
+                                                          Error **errp)
+{
+    if (bdrv_find_dirty_bitmap(bs, name)) {
+        error_setg(errp, "Bitmap already exists: %s", name);
+        return false;
+    }
+
+    return true;
+}
diff --git a/block/parallels.c b/block/parallels.c
index 2ee2b42038..bb1e765ec8 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1470,14 +1470,25 @@ fail:
 static int parallels_inactivate(BlockDriverState *bs)
 {
     BDRVParallelsState *s = bs->opaque;
+    Error *err = NULL;
     int ret;
 
+    parallels_store_persistent_dirty_bitmaps(bs, &err);
+    if (err != NULL) {
+        error_reportf_err(err, "Lost persistent bitmaps during "
+                          "inactivation of node '%s': ",
+                          bdrv_get_device_or_node_name(bs));
+    }
+
     s->header->inuse = 0;
     parallels_update_header(bs);
 
     /* errors are ignored, so we might as well pass exact=true */
-    ret = bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, true,
-                        PREALLOC_MODE_OFF, 0, NULL);
+    ret = bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS,
+                        true, PREALLOC_MODE_OFF, 0, NULL);
+    if (ret < 0) {
+        error_report("Failed to truncate image: %s", strerror(-ret));
+    }
 
     return ret;
 }
@@ -1528,6 +1539,7 @@ static BlockDriver bdrv_parallels = {
     .bdrv_co_pdiscard           = parallels_co_pdiscard,
     .bdrv_co_pwrite_zeroes      = parallels_co_pwrite_zeroes,
     .bdrv_inactivate            = parallels_inactivate,
+    .bdrv_co_can_store_new_dirty_bitmap = parallels_co_can_store_new_dirty_bitmap,
 };
 
 static void bdrv_parallels_init(void)
diff --git a/block/parallels.h b/block/parallels.h
index 4e7aa6b80f..18b4f8068e 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -100,6 +100,11 @@ int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
 
 int parallels_read_format_extension(BlockDriverState *bs,
                                     int64_t ext_off, Error **errp);
+void parallels_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+                                              Error **errp);
+bool coroutine_fn
+parallels_co_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
+                                        uint32_t granularity, Error **errp);
 
 
 #endif
-- 
2.34.1

Re: [PATCH v2 10/20] parallels: Add dirty bitmaps saving

[Mike Maslenkin]

On Thu, Oct 19, 2023 at 4:05 PM Alexander Ivanov
<alexander.ivanov@virtuozzo.com> wrote:
>
> Now dirty bitmaps can be loaded but there is no their saving. Add code for
> dirty bitmap storage.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>  block/parallels-ext.c | 167 ++++++++++++++++++++++++++++++++++++++++++
>  block/parallels.c     |  16 +++-
>  block/parallels.h     |   5 ++
>  3 files changed, 186 insertions(+), 2 deletions(-)
>
> diff --git a/block/parallels-ext.c b/block/parallels-ext.c
> index 8a109f005a..0a632a2331 100644
> --- a/block/parallels-ext.c
> +++ b/block/parallels-ext.c
> @@ -24,6 +24,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "block/block-io.h"
>  #include "block/block_int.h"
> @@ -301,3 +302,169 @@ out:
>
>      return ret;
>  }
> +
> +static void parallels_save_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
> +                                  uint8_t **buf, int *buf_size)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    ParallelsFeatureHeader *fh;
> +    ParallelsDirtyBitmapFeature *bh;
> +    uint64_t *l1_table, l1_size, granularity, limit;
> +    int64_t bm_size, ser_size, offset, buf_used;
> +    int64_t alloc_size = 1;
> +    const char *name;
> +    uint8_t *bm_buf;
> +    QemuUUID uuid;
> +    int ret = 0;
> +
> +    if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
> +        bdrv_dirty_bitmap_inconsistent(bitmap)) {
> +        return;
> +    }
> +
> +    bm_size = bdrv_dirty_bitmap_size(bitmap);
> +    granularity = bdrv_dirty_bitmap_granularity(bitmap);
> +    limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap);
> +    ser_size = bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size);
> +    l1_size = DIV_ROUND_UP(ser_size, s->cluster_size);
> +
> +    buf_used = l1_size * 8 + sizeof(*fh) + sizeof(*bh);
> +    /* Check if there is enough space for the final section */
> +    if (*buf_size - buf_used < sizeof(*fh)) {
> +        return;
> +    }
> +
> +    name = bdrv_dirty_bitmap_name(bitmap);
> +    ret = qemu_uuid_parse(name, &uuid);
> +    if (ret < 0) {
> +        error_report("Can't save dirty bitmap: ID parsing error: '%s'", name);
> +        return;
> +    }
> +
> +    fh = (ParallelsFeatureHeader *)*buf;
> +    bh = (ParallelsDirtyBitmapFeature *)(*buf + sizeof(*fh));
> +    l1_table = (uint64_t *)((uint8_t *)bh + sizeof(*bh));
> +
> +    fh->magic = cpu_to_le64(PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC);
> +    fh->data_size = cpu_to_le32(l1_size * 8 + sizeof(*bh));
> +
> +    bh->l1_size = cpu_to_le32(l1_size);
> +    bh->size = cpu_to_le64(bm_size >> BDRV_SECTOR_BITS);
> +    bh->granularity = cpu_to_le32(granularity >> BDRV_SECTOR_BITS);
> +    memcpy(bh->id, &uuid, sizeof(uuid));
> +
> +    bm_buf = qemu_blockalign(bs, s->cluster_size);
> +
> +    offset = 0;
> +    while ((offset = bdrv_dirty_bitmap_next_dirty(bitmap, offset, bm_size)) >= 0) {
> +        uint64_t idx = offset / limit;
> +        int64_t cluster_off, end, write_size;
> +
> +        offset = QEMU_ALIGN_DOWN(offset, limit);
> +        end = MIN(bm_size, offset + limit);
> +        write_size = bdrv_dirty_bitmap_serialization_size(bitmap, offset,
> +                                                          end - offset);
> +        assert(write_size <= s->cluster_size);
> +
> +        bdrv_dirty_bitmap_serialize_part(bitmap, bm_buf, offset, end - offset);
> +        if (write_size < s->cluster_size) {
> +            memset(bm_buf + write_size, 0, s->cluster_size - write_size);
> +        }
> +
> +        cluster_off = parallels_allocate_host_clusters(bs, &alloc_size);
> +        if (cluster_off <= 0) {
> +            goto end;
> +        }
> +
> +        ret = bdrv_pwrite(bs->file, cluster_off, s->cluster_size, bm_buf, 0);
> +        if (ret < 0) {
> +            memset(&fh->magic, 0, sizeof(fh->magic));
> +            parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
> +                                  cluster_off, 1);
> +            goto end;
> +        }
> +
> +        l1_table[idx] = cpu_to_le64(cluster_off >> BDRV_SECTOR_BITS);
> +        offset = end;
> +    }
> +
> +    *buf_size -= buf_used;
> +    *buf += buf_used;
> +
> +end:
> +    qemu_vfree(bm_buf);
> +}
> +
> +void parallels_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> +                                              Error **errp)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    BdrvDirtyBitmap *bitmap;
> +    ParallelsFormatExtensionHeader *eh;
> +    int remaining = s->cluster_size;
> +    uint8_t *buf, *pos;
> +    int64_t header_off, alloc_size = 1;
> +    g_autofree uint8_t *hash = NULL;
> +    size_t hash_len = 0;
> +    int ret;
> +
> +    s->header->ext_off = 0;
> +
> +    if (!bdrv_has_named_bitmaps(bs)) {
> +        return;
> +    }
> +
> +    buf = qemu_blockalign0(bs, s->cluster_size);
> +
> +    eh = (ParallelsFormatExtensionHeader *)buf;
> +    pos = buf + sizeof(*eh);
> +
> +    eh->magic = cpu_to_le64(PARALLELS_FORMAT_EXTENSION_MAGIC);
> +
> +    FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
> +        parallels_save_bitmap(bs, bitmap, &pos, &remaining);
> +    }
> +
> +    header_off = parallels_allocate_host_clusters(bs, &alloc_size);
> +    if (header_off < 0) {
> +        error_report("Can't save dirty bitmap: cluster allocation error");
> +        ret = header_off;
> +        goto end;
> +    }
> +
> +    ret = qcrypto_hash_bytes(QCRYPTO_HASH_ALG_MD5,
> +                             (const char *)(buf + sizeof(*eh)),
> +                             s->cluster_size - sizeof(*eh),
> +                             &hash, &hash_len, errp);
> +    if (ret < 0 || hash_len != sizeof(eh->check_sum)) {
> +        error_report("Can't save dirty bitmap: hash error");
> +        ret = -EINVAL;
> +        goto end;
> +    }
> +    memcpy(eh->check_sum, hash, hash_len);
> +
> +    ret = bdrv_pwrite(bs->file, header_off, s->cluster_size, buf, 0);
> +    if (ret < 0) {
> +        error_report("Can't save dirty bitmap: IO error");
> +        parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
> +                              header_off, 1);
> +        goto end;
> +    }
> +
> +    s->header->ext_off = cpu_to_le64(header_off / BDRV_SECTOR_SIZE);
> +end:
> +    qemu_vfree(buf);
> +}
> +
> +bool coroutine_fn parallels_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
> +                                                          const char *name,
> +                                                          uint32_t granularity,
> +                                                          Error **errp)
> +{
> +    if (bdrv_find_dirty_bitmap(bs, name)) {
> +        error_setg(errp, "Bitmap already exists: %s", name);
> +        return false;
> +    }
> +
> +    return true;
> +}
> diff --git a/block/parallels.c b/block/parallels.c
> index 2ee2b42038..bb1e765ec8 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -1470,14 +1470,25 @@ fail:
>  static int parallels_inactivate(BlockDriverState *bs)
>  {
>      BDRVParallelsState *s = bs->opaque;
> +    Error *err = NULL;
>      int ret;
>
> +    parallels_store_persistent_dirty_bitmaps(bs, &err);
> +    if (err != NULL) {
> +        error_reportf_err(err, "Lost persistent bitmaps during "
> +                          "inactivation of node '%s': ",
> +                          bdrv_get_device_or_node_name(bs));
> +    }
> +
>      s->header->inuse = 0;
>      parallels_update_header(bs);
>

Recently bdrv_get_device_or_node_name() calls were guarded by
bdrv_graph_rd{,un}lock_main_loop.
There are no annotations for parallels_close() or
parallels_inactivate(), so may be it should be done in the same way?

Re: [PATCH v2 10/20] parallels: Add dirty bitmaps saving

[Alexander Ivanov]

On 10/21/23 12:40, Mike Maslenkin wrote:
> On Thu, Oct 19, 2023 at 4:05 PM Alexander Ivanov
> <alexander.ivanov@virtuozzo.com> wrote:
>> Now dirty bitmaps can be loaded but there is no their saving. Add code for
>> dirty bitmap storage.
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>>   block/parallels-ext.c | 167 ++++++++++++++++++++++++++++++++++++++++++
>>   block/parallels.c     |  16 +++-
>>   block/parallels.h     |   5 ++
>>   3 files changed, 186 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/parallels-ext.c b/block/parallels-ext.c
>> index 8a109f005a..0a632a2331 100644
>> --- a/block/parallels-ext.c
>> +++ b/block/parallels-ext.c
>> @@ -24,6 +24,7 @@
>>    */
>>
>>   #include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>>   #include "qapi/error.h"
>>   #include "block/block-io.h"
>>   #include "block/block_int.h"
>> @@ -301,3 +302,169 @@ out:
>>
>>       return ret;
>>   }
>> +
>> +static void parallels_save_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
>> +                                  uint8_t **buf, int *buf_size)
>> +{
>> +    BDRVParallelsState *s = bs->opaque;
>> +    ParallelsFeatureHeader *fh;
>> +    ParallelsDirtyBitmapFeature *bh;
>> +    uint64_t *l1_table, l1_size, granularity, limit;
>> +    int64_t bm_size, ser_size, offset, buf_used;
>> +    int64_t alloc_size = 1;
>> +    const char *name;
>> +    uint8_t *bm_buf;
>> +    QemuUUID uuid;
>> +    int ret = 0;
>> +
>> +    if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
>> +        bdrv_dirty_bitmap_inconsistent(bitmap)) {
>> +        return;
>> +    }
>> +
>> +    bm_size = bdrv_dirty_bitmap_size(bitmap);
>> +    granularity = bdrv_dirty_bitmap_granularity(bitmap);
>> +    limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap);
>> +    ser_size = bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size);
>> +    l1_size = DIV_ROUND_UP(ser_size, s->cluster_size);
>> +
>> +    buf_used = l1_size * 8 + sizeof(*fh) + sizeof(*bh);
>> +    /* Check if there is enough space for the final section */
>> +    if (*buf_size - buf_used < sizeof(*fh)) {
>> +        return;
>> +    }
>> +
>> +    name = bdrv_dirty_bitmap_name(bitmap);
>> +    ret = qemu_uuid_parse(name, &uuid);
>> +    if (ret < 0) {
>> +        error_report("Can't save dirty bitmap: ID parsing error: '%s'", name);
>> +        return;
>> +    }
>> +
>> +    fh = (ParallelsFeatureHeader *)*buf;
>> +    bh = (ParallelsDirtyBitmapFeature *)(*buf + sizeof(*fh));
>> +    l1_table = (uint64_t *)((uint8_t *)bh + sizeof(*bh));
>> +
>> +    fh->magic = cpu_to_le64(PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC);
>> +    fh->data_size = cpu_to_le32(l1_size * 8 + sizeof(*bh));
>> +
>> +    bh->l1_size = cpu_to_le32(l1_size);
>> +    bh->size = cpu_to_le64(bm_size >> BDRV_SECTOR_BITS);
>> +    bh->granularity = cpu_to_le32(granularity >> BDRV_SECTOR_BITS);
>> +    memcpy(bh->id, &uuid, sizeof(uuid));
>> +
>> +    bm_buf = qemu_blockalign(bs, s->cluster_size);
>> +
>> +    offset = 0;
>> +    while ((offset = bdrv_dirty_bitmap_next_dirty(bitmap, offset, bm_size)) >= 0) {
>> +        uint64_t idx = offset / limit;
>> +        int64_t cluster_off, end, write_size;
>> +
>> +        offset = QEMU_ALIGN_DOWN(offset, limit);
>> +        end = MIN(bm_size, offset + limit);
>> +        write_size = bdrv_dirty_bitmap_serialization_size(bitmap, offset,
>> +                                                          end - offset);
>> +        assert(write_size <= s->cluster_size);
>> +
>> +        bdrv_dirty_bitmap_serialize_part(bitmap, bm_buf, offset, end - offset);
>> +        if (write_size < s->cluster_size) {
>> +            memset(bm_buf + write_size, 0, s->cluster_size - write_size);
>> +        }
>> +
>> +        cluster_off = parallels_allocate_host_clusters(bs, &alloc_size);
>> +        if (cluster_off <= 0) {
>> +            goto end;
>> +        }
>> +
>> +        ret = bdrv_pwrite(bs->file, cluster_off, s->cluster_size, bm_buf, 0);
>> +        if (ret < 0) {
>> +            memset(&fh->magic, 0, sizeof(fh->magic));
>> +            parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
>> +                                  cluster_off, 1);
>> +            goto end;
>> +        }
>> +
>> +        l1_table[idx] = cpu_to_le64(cluster_off >> BDRV_SECTOR_BITS);
>> +        offset = end;
>> +    }
>> +
>> +    *buf_size -= buf_used;
>> +    *buf += buf_used;
>> +
>> +end:
>> +    qemu_vfree(bm_buf);
>> +}
>> +
>> +void parallels_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>> +                                              Error **errp)
>> +{
>> +    BDRVParallelsState *s = bs->opaque;
>> +    BdrvDirtyBitmap *bitmap;
>> +    ParallelsFormatExtensionHeader *eh;
>> +    int remaining = s->cluster_size;
>> +    uint8_t *buf, *pos;
>> +    int64_t header_off, alloc_size = 1;
>> +    g_autofree uint8_t *hash = NULL;
>> +    size_t hash_len = 0;
>> +    int ret;
>> +
>> +    s->header->ext_off = 0;
>> +
>> +    if (!bdrv_has_named_bitmaps(bs)) {
>> +        return;
>> +    }
>> +
>> +    buf = qemu_blockalign0(bs, s->cluster_size);
>> +
>> +    eh = (ParallelsFormatExtensionHeader *)buf;
>> +    pos = buf + sizeof(*eh);
>> +
>> +    eh->magic = cpu_to_le64(PARALLELS_FORMAT_EXTENSION_MAGIC);
>> +
>> +    FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
>> +        parallels_save_bitmap(bs, bitmap, &pos, &remaining);
>> +    }
>> +
>> +    header_off = parallels_allocate_host_clusters(bs, &alloc_size);
>> +    if (header_off < 0) {
>> +        error_report("Can't save dirty bitmap: cluster allocation error");
>> +        ret = header_off;
>> +        goto end;
>> +    }
>> +
>> +    ret = qcrypto_hash_bytes(QCRYPTO_HASH_ALG_MD5,
>> +                             (const char *)(buf + sizeof(*eh)),
>> +                             s->cluster_size - sizeof(*eh),
>> +                             &hash, &hash_len, errp);
>> +    if (ret < 0 || hash_len != sizeof(eh->check_sum)) {
>> +        error_report("Can't save dirty bitmap: hash error");
>> +        ret = -EINVAL;
>> +        goto end;
>> +    }
>> +    memcpy(eh->check_sum, hash, hash_len);
>> +
>> +    ret = bdrv_pwrite(bs->file, header_off, s->cluster_size, buf, 0);
>> +    if (ret < 0) {
>> +        error_report("Can't save dirty bitmap: IO error");
>> +        parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
>> +                              header_off, 1);
>> +        goto end;
>> +    }
>> +
>> +    s->header->ext_off = cpu_to_le64(header_off / BDRV_SECTOR_SIZE);
>> +end:
>> +    qemu_vfree(buf);
>> +}
>> +
>> +bool coroutine_fn parallels_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
>> +                                                          const char *name,
>> +                                                          uint32_t granularity,
>> +                                                          Error **errp)
>> +{
>> +    if (bdrv_find_dirty_bitmap(bs, name)) {
>> +        error_setg(errp, "Bitmap already exists: %s", name);
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 2ee2b42038..bb1e765ec8 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -1470,14 +1470,25 @@ fail:
>>   static int parallels_inactivate(BlockDriverState *bs)
>>   {
>>       BDRVParallelsState *s = bs->opaque;
>> +    Error *err = NULL;
>>       int ret;
>>
>> +    parallels_store_persistent_dirty_bitmaps(bs, &err);
>> +    if (err != NULL) {
>> +        error_reportf_err(err, "Lost persistent bitmaps during "
>> +                          "inactivation of node '%s': ",
>> +                          bdrv_get_device_or_node_name(bs));
>> +    }
>> +
>>       s->header->inuse = 0;
>>       parallels_update_header(bs);
>>
> Recently bdrv_get_device_or_node_name() calls were guarded by
> bdrv_graph_rd{,un}lock_main_loop.
> There are no annotations for parallels_close() or
> parallels_inactivate(), so may be it should be done in the same way?

Thank you, will take a look.

-- 
Best regards,
Alexander Ivanov

[PATCH v2 11/20] parallels: Let image extensions work in RW mode

[Alexander Ivanov]

Now we support extensions saving and can let to work with them in
read-write mode.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels-ext.c |  4 ----
 block/parallels.c     | 17 ++++-------------
 2 files changed, 4 insertions(+), 17 deletions(-)

diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index 0a632a2331..a0de4f1b07 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -177,10 +177,6 @@ static BdrvDirtyBitmap *parallels_load_bitmap(BlockDriverState *bs,
         return NULL;
     }
 
-    /* We support format extension only for RO parallels images. */
-    assert(!(bs->open_flags & BDRV_O_RDWR));
-    bdrv_dirty_bitmap_set_readonly(bitmap, true);
-
     return bitmap;
 }
 
diff --git a/block/parallels.c b/block/parallels.c
index bb1e765ec8..2198eff21e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1374,19 +1374,10 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     if (ph.ext_off) {
-        if (flags & BDRV_O_RDWR) {
-            /*
-             * It's unsafe to open image RW if there is an extension (as we
-             * don't support it). But parallels driver in QEMU historically
-             * ignores the extension, so print warning and don't care.
-             */
-            warn_report("Format Extension ignored in RW mode");
-        } else {
-            ret = parallels_read_format_extension(
-                    bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
-            if (ret < 0) {
-                goto fail;
-            }
+        ret = parallels_read_format_extension(
+                bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
+        if (ret < 0) {
+            goto fail;
         }
     }
 
-- 
2.34.1

[PATCH v2 12/20] parallels: Handle L1 entries equal to one

[Alexander Ivanov]

If all the bits in a dirty bitmap cluster are ones, the cluster shouldn't
be written. Instead the corresponding L1 entry should be set to 1.

Check if all bits in a memory region are ones and set 1 to L1 entries
corresponding clusters filled with ones.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels-ext.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index a0de4f1b07..ebda6b0a01 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -354,7 +354,7 @@ static void parallels_save_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
     offset = 0;
     while ((offset = bdrv_dirty_bitmap_next_dirty(bitmap, offset, bm_size)) >= 0) {
         uint64_t idx = offset / limit;
-        int64_t cluster_off, end, write_size;
+        int64_t cluster_off, end, write_size, first_zero;
 
         offset = QEMU_ALIGN_DOWN(offset, limit);
         end = MIN(bm_size, offset + limit);
@@ -367,6 +367,16 @@ static void parallels_save_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
             memset(bm_buf + write_size, 0, s->cluster_size - write_size);
         }
 
+        first_zero = bdrv_dirty_bitmap_next_zero(bitmap, offset, bm_size);
+        if (first_zero < 0) {
+            goto end;
+        }
+        if (first_zero - offset >= s->cluster_size) {
+            l1_table[idx] = 1;
+            offset = end;
+            continue;
+        }
+
         cluster_off = parallels_allocate_host_clusters(bs, &alloc_size);
         if (cluster_off <= 0) {
             goto end;
-- 
2.34.1

[PATCH v2 13/20] parallels: Make a loaded dirty bitmap persistent

[Alexander Ivanov]

After bitmap loading the bitmap is not persistent and is removed on image
saving. Set bitmap persistence to true.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels-ext.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index ebda6b0a01..bb4478c350 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -256,6 +256,7 @@ static int parallels_parse_format_extension(BlockDriverState *bs,
             if (!bitmap) {
                 goto fail;
             }
+            bdrv_dirty_bitmap_set_persistence(bitmap, true);
             bitmaps = g_slist_append(bitmaps, bitmap);
             break;
 
-- 
2.34.1

[PATCH v2 14/20] parallels: Reverse a conditional in parallels_check_leak() to reduce indents

[Alexander Ivanov]

Let the function return a success code if a file size is not bigger than
image_end_offset. Thus we can decrease indents in the next code block.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 72 +++++++++++++++++++++++------------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 2198eff21e..2d54459087 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -775,7 +775,7 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
                      BdrvCheckMode fix, bool explicit)
 {
     BDRVParallelsState *s = bs->opaque;
-    int64_t size;
+    int64_t size, count;
     int ret;
 
     size = bdrv_co_getlength(bs->file->bs);
@@ -783,43 +783,43 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
         res->check_errors++;
         return size;
     }
+    if (size <= res->image_end_offset) {
+        return 0;
+    }
+
+    count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
+    if (explicit) {
+        fprintf(stderr,
+                "%s space leaked at the end of the image %" PRId64 "\n",
+                fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
+                size - res->image_end_offset);
+        res->leaks += count;
+    }
+    if (fix & BDRV_FIX_LEAKS) {
+        Error *local_err = NULL;
+
+        /*
+         * In order to really repair the image, we must shrink it.
+         * That means we have to pass exact=true.
+         */
+        ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
+                               PREALLOC_MODE_OFF, 0, &local_err);
+        if (ret < 0) {
+            error_report_err(local_err);
+            res->check_errors++;
+            return ret;
+        }
+        s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
+
+        parallels_free_used_bitmap(bs);
+        ret = parallels_fill_used_bitmap(bs);
+        if (ret == -ENOMEM) {
+            res->check_errors++;
+            return ret;
+        }
 
-    if (size > res->image_end_offset) {
-        int64_t count;
-        count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
         if (explicit) {
-            fprintf(stderr,
-                    "%s space leaked at the end of the image %" PRId64 "\n",
-                    fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
-                    size - res->image_end_offset);
-            res->leaks += count;
-        }
-        if (fix & BDRV_FIX_LEAKS) {
-            Error *local_err = NULL;
-
-            /*
-             * In order to really repair the image, we must shrink it.
-             * That means we have to pass exact=true.
-             */
-            ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
-                                   PREALLOC_MODE_OFF, 0, &local_err);
-            if (ret < 0) {
-                error_report_err(local_err);
-                res->check_errors++;
-                return ret;
-            }
-            s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
-
-            parallels_free_used_bitmap(bs);
-            ret = parallels_fill_used_bitmap(bs);
-            if (ret == -ENOMEM) {
-                res->check_errors++;
-                return ret;
-            }
-
-            if (explicit) {
-                res->leaks_fixed += count;
-            }
+            res->leaks_fixed += count;
         }
     }
 
-- 
2.34.1

[PATCH v2 15/20] parallels: Truncate images on the last used cluster

[Alexander Ivanov]

On an image closing there can be unused clusters in the end of the image.
Truncate these clusters and update data_end field.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 2d54459087..c7db2a9562 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1458,6 +1458,23 @@ fail:
     return ret;
 }
 
+static int parallels_truncate_unused_clusters(BlockDriverState *bs)
+{
+    BDRVParallelsState *s = bs->opaque;
+    uint64_t end_off = 0;
+    if (s->used_bmap_size > 0) {
+        end_off = find_last_bit(s->used_bmap, s->used_bmap_size);
+        if (end_off == s->used_bmap_size) {
+            end_off = 0;
+        } else {
+            end_off = (end_off + 1) * s->cluster_size;
+        }
+    }
+    end_off += s->data_start * BDRV_SECTOR_SIZE;
+    s->data_end = end_off / BDRV_SECTOR_SIZE;
+    return bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
+}
+
 static int parallels_inactivate(BlockDriverState *bs)
 {
     BDRVParallelsState *s = bs->opaque;
@@ -1475,8 +1492,7 @@ static int parallels_inactivate(BlockDriverState *bs)
     parallels_update_header(bs);
 
     /* errors are ignored, so we might as well pass exact=true */
-    ret = bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS,
-                        true, PREALLOC_MODE_OFF, 0, NULL);
+    ret = parallels_truncate_unused_clusters(bs);
     if (ret < 0) {
         error_report("Failed to truncate image: %s", strerror(-ret));
     }
-- 
2.34.1

[PATCH v2 16/20] parallels: Check unused clusters in parallels_check_leak()

[Alexander Ivanov]

Since we have used bitmap, leak check is useless. Transform
parallels_truncate_unused_clusters() to parallels_check_unused_clusters()
helper and use it in leak check.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 121 +++++++++++++++++++++++++---------------------
 1 file changed, 67 insertions(+), 54 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index c7db2a9562..b1100dc65c 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -770,57 +770,87 @@ parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res,
     return 0;
 }
 
+static int64_t parallels_check_unused_clusters(BlockDriverState *bs,
+                                               bool truncate)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int64_t leak, file_size, end_off = 0;
+    int ret;
+
+    file_size = bdrv_getlength(bs->file->bs);
+    if (file_size < 0) {
+        return file_size;
+    }
+
+    if (s->used_bmap_size > 0) {
+        end_off = find_last_bit(s->used_bmap, s->used_bmap_size);
+        if (end_off == s->used_bmap_size) {
+            end_off = 0;
+        } else {
+            end_off = (end_off + 1) * s->cluster_size;
+        }
+    }
+
+    end_off += s->data_start * BDRV_SECTOR_SIZE;
+    leak = file_size - end_off;
+    if (leak < 0) {
+        return -EINVAL;
+    }
+    if (!truncate || leak == 0) {
+        return leak;
+    }
+
+    ret = bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
+    if (ret) {
+        return ret;
+    }
+
+    s->data_end = end_off / BDRV_SECTOR_SIZE;
+
+    parallels_free_used_bitmap(bs);
+    ret = parallels_fill_used_bitmap(bs);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return leak;
+}
+
 static int coroutine_fn GRAPH_RDLOCK
 parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
                      BdrvCheckMode fix, bool explicit)
 {
     BDRVParallelsState *s = bs->opaque;
-    int64_t size, count;
-    int ret;
+    int64_t leak, count, size;
+
+    leak = parallels_check_unused_clusters(bs, fix & BDRV_FIX_LEAKS);
+    if (leak < 0) {
+        res->check_errors++;
+        return leak;
+    }
+    if (leak == 0) {
+        return 0;
+    }
 
     size = bdrv_co_getlength(bs->file->bs);
     if (size < 0) {
         res->check_errors++;
         return size;
     }
-    if (size <= res->image_end_offset) {
+    res->image_end_offset = size;
+
+    if (!explicit) {
         return 0;
     }
 
-    count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
-    if (explicit) {
-        fprintf(stderr,
-                "%s space leaked at the end of the image %" PRId64 "\n",
-                fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
-                size - res->image_end_offset);
-        res->leaks += count;
-    }
+    count = DIV_ROUND_UP(leak, s->cluster_size);
+    fprintf(stderr,
+            "%s space leaked at the end of the image %" PRId64 "\n",
+            fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak);
+    res->leaks += count;
+
     if (fix & BDRV_FIX_LEAKS) {
-        Error *local_err = NULL;
-
-        /*
-         * In order to really repair the image, we must shrink it.
-         * That means we have to pass exact=true.
-         */
-        ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
-                               PREALLOC_MODE_OFF, 0, &local_err);
-        if (ret < 0) {
-            error_report_err(local_err);
-            res->check_errors++;
-            return ret;
-        }
-        s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
-
-        parallels_free_used_bitmap(bs);
-        ret = parallels_fill_used_bitmap(bs);
-        if (ret == -ENOMEM) {
-            res->check_errors++;
-            return ret;
-        }
-
-        if (explicit) {
-            res->leaks_fixed += count;
-        }
+        res->leaks_fixed += count;
     }
 
     return 0;
@@ -1458,23 +1488,6 @@ fail:
     return ret;
 }
 
-static int parallels_truncate_unused_clusters(BlockDriverState *bs)
-{
-    BDRVParallelsState *s = bs->opaque;
-    uint64_t end_off = 0;
-    if (s->used_bmap_size > 0) {
-        end_off = find_last_bit(s->used_bmap, s->used_bmap_size);
-        if (end_off == s->used_bmap_size) {
-            end_off = 0;
-        } else {
-            end_off = (end_off + 1) * s->cluster_size;
-        }
-    }
-    end_off += s->data_start * BDRV_SECTOR_SIZE;
-    s->data_end = end_off / BDRV_SECTOR_SIZE;
-    return bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
-}
-
 static int parallels_inactivate(BlockDriverState *bs)
 {
     BDRVParallelsState *s = bs->opaque;
@@ -1492,7 +1505,7 @@ static int parallels_inactivate(BlockDriverState *bs)
     parallels_update_header(bs);
 
     /* errors are ignored, so we might as well pass exact=true */
-    ret = parallels_truncate_unused_clusters(bs);
+    ret = parallels_check_unused_clusters(bs, true);
     if (ret < 0) {
         error_report("Failed to truncate image: %s", strerror(-ret));
     }
-- 
2.34.1

[PATCH v2 17/20] parallels: Remove unnecessary data_end field

[Alexander Ivanov]

Since we have used bitmap, field data_end in BDRVParallelsState is
redundant and can be removed.

Add parallels_data_end() helper and remove data_end handling.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 33 +++++++++++++--------------------
 block/parallels.h |  1 -
 2 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index b1100dc65c..fcea44276a 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -266,6 +266,13 @@ static void parallels_free_used_bitmap(BlockDriverState *bs)
     g_free(s->used_bmap);
 }
 
+static int64_t parallels_data_end(BDRVParallelsState *s)
+{
+    int64_t data_end = s->data_start * BDRV_SECTOR_SIZE;
+    data_end += s->used_bmap_size * s->cluster_size;
+    return data_end;
+}
+
 int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
                                          int64_t *clusters)
 {
@@ -277,7 +284,7 @@ int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
 
     first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
     if (first_free == s->used_bmap_size) {
-        host_off = s->data_end * BDRV_SECTOR_SIZE;
+        host_off = parallels_data_end(s);
         prealloc_clusters = *clusters + s->prealloc_size / s->tracks;
         bytes = *clusters * s->cluster_size;
         prealloc_bytes = prealloc_clusters * s->cluster_size;
@@ -300,9 +307,6 @@ int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
         s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
                                           new_usedsize);
         s->used_bmap_size = new_usedsize;
-        if (host_off + bytes > s->data_end * BDRV_SECTOR_SIZE) {
-            s->data_end = (host_off + bytes) / BDRV_SECTOR_SIZE;
-        }
     } else {
         next_used = find_next_bit(s->used_bmap, s->used_bmap_size, first_free);
 
@@ -318,8 +322,7 @@ int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
          * branch. In the other case we are likely re-using hole. Preallocate
          * the space if required by the prealloc_mode.
          */
-        if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE &&
-                host_off < s->data_end * BDRV_SECTOR_SIZE) {
+        if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
             ret = bdrv_pwrite_zeroes(bs->file, host_off, bytes, 0);
             if (ret < 0) {
                 return ret;
@@ -760,13 +763,7 @@ parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res,
         }
     }
 
-    if (high_off == 0) {
-        res->image_end_offset = s->data_end << BDRV_SECTOR_BITS;
-    } else {
-        res->image_end_offset = high_off + s->cluster_size;
-        s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
-    }
-
+    res->image_end_offset = parallels_data_end(s);
     return 0;
 }
 
@@ -805,8 +802,6 @@ static int64_t parallels_check_unused_clusters(BlockDriverState *bs,
         return ret;
     }
 
-    s->data_end = end_off / BDRV_SECTOR_SIZE;
-
     parallels_free_used_bitmap(bs);
     ret = parallels_fill_used_bitmap(bs);
     if (ret < 0) {
@@ -1394,8 +1389,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     s->data_start = data_start;
-    s->data_end = s->data_start;
-    if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
+    if (s->data_start < (s->header_size >> BDRV_SECTOR_BITS)) {
         /*
          * There is not enough unused space to fit to block align between BAT
          * and actual data. We can't avoid read-modify-write...
@@ -1439,11 +1433,10 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
 
     for (i = 0; i < s->bat_size; i++) {
         sector = bat2sect(s, i);
-        if (sector + s->tracks > s->data_end) {
-            s->data_end = sector + s->tracks;
+        if (sector + s->tracks > file_nb_sectors) {
+            need_check = true;
         }
     }
-    need_check = need_check || s->data_end > file_nb_sectors;
 
     ret = parallels_fill_used_bitmap(bs);
     if (ret == -ENOMEM) {
diff --git a/block/parallels.h b/block/parallels.h
index 18b4f8068e..a6a048d890 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -79,7 +79,6 @@ typedef struct BDRVParallelsState {
     unsigned int bat_size;
 
     int64_t  data_start;
-    int64_t  data_end;
     uint64_t prealloc_size;
     ParallelsPreallocMode prealloc_mode;
 
-- 
2.34.1

[PATCH v2 18/20] tests: Add parallels images support to test 165

[Alexander Ivanov]

Use a different bitmap name for parallels images because their has own ID
format, and can't contain an arbitrary string.

Replace image reopen by shutdown/launch VM because parallels images doesn't
support reopen.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 tests/qemu-iotests/165 | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
index b24907a62f..f732db257c 100755
--- a/tests/qemu-iotests/165
+++ b/tests/qemu-iotests/165
@@ -38,6 +38,10 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
 
     def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, disk, str(disk_size))
+        if iotests.imgfmt == 'parallels':
+            self.bitmap_name = '00000000-0000-0000-0000-000000000000'
+        else:
+            self.bitmap_name = 'bitmap0'
 
     def tearDown(self):
         os.remove(disk)
@@ -50,12 +54,12 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
 
     def getSha256(self):
         result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
-                             node='drive0', name='bitmap0')
+                             node='drive0', name=self.bitmap_name)
         return result['return']['sha256']
 
     def checkBitmap(self, sha256):
         result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
-                             node='drive0', name='bitmap0')
+                             node='drive0', name=self.bitmap_name)
         self.assert_qmp(result, 'return/sha256', sha256);
 
     def writeRegions(self, regions):
@@ -65,7 +69,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
 
     def qmpAddBitmap(self):
         self.vm.qmp('block-dirty-bitmap-add', node='drive0',
-                    name='bitmap0', persistent=True)
+                    name=self.bitmap_name, persistent=True)
 
     def test_persistent(self):
         self.vm = self.mkVm()
@@ -117,7 +121,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
         assert sha256_1 != sha256_2 # Otherwise, it's not very interesting.
 
         self.vm.cmd('block-dirty-bitmap-clear', node='drive0',
-                    name='bitmap0')
+                    name=self.bitmap_name)
 
         # Start with regions1
 
@@ -135,16 +139,22 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
         self.writeRegions(regions2)
         assert sha256_1 == self.getSha256()
 
-        # Reopen to RW
-        self.vm.cmd('blockdev-reopen', options=[{
-            'node-name': 'node0',
-            'driver': iotests.imgfmt,
-            'file': {
-                'driver': 'file',
-                'filename': disk
-            },
-            'read-only': False
-        }])
+        if iotests.imgfmt == 'parallels':
+            # parallels doesn't support reopen
+            self.vm.shutdown()
+            self.vm = self.mkVm()
+            self.vm.launch()
+        else:
+            # Reopen to RW
+            self.vm.cmd('blockdev-reopen', options=[{
+                'node-name': 'node0',
+                'driver': iotests.imgfmt,
+                'file': {
+                    'driver': 'file',
+                    'filename': disk
+                },
+                'read-only': False
+            }])
 
         # Check that bitmap is reopened to RW and we can write to it.
         self.writeRegions(regions2)
@@ -154,6 +164,6 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
 
 
 if __name__ == '__main__':
-    iotests.main(supported_fmts=['qcow2'],
+    iotests.main(supported_fmts=['qcow2', 'parallels'],
                  supported_protocols=['file'],
                  unsupported_imgopts=['compat'])
-- 
2.34.1

[PATCH v2 19/20] tests: Turned on 256, 299, 304 and block-status-cache for parallels format

[Alexander Ivanov]

These tests pass with parallels format. Add parallels to supporting
formats for these tests.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 tests/qemu-iotests/256                      | 2 +-
 tests/qemu-iotests/299                      | 2 +-
 tests/qemu-iotests/304                      | 2 +-
 tests/qemu-iotests/tests/block-status-cache | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/256 b/tests/qemu-iotests/256
index f34af6cef7..1a4c9c6885 100755
--- a/tests/qemu-iotests/256
+++ b/tests/qemu-iotests/256
@@ -26,7 +26,7 @@ from iotests import log
 
 iotests.verify_virtio_scsi_pci_or_ccw()
 
-iotests.script_initialize(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2', 'parallels'])
 size = 64 * 1024 * 1024
 
 with iotests.FilePath('img0') as img0_path, \
diff --git a/tests/qemu-iotests/299 b/tests/qemu-iotests/299
index a7122941fd..d8c4399446 100755
--- a/tests/qemu-iotests/299
+++ b/tests/qemu-iotests/299
@@ -23,7 +23,7 @@ import iotests
 
 # The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
 iotests.script_initialize(
-    supported_fmts=['qcow2'],
+    supported_fmts=['qcow2', 'parallels'],
 )
 
 nbd_sock = iotests.file_path('nbd.sock', base_dir=iotests.sock_dir)
diff --git a/tests/qemu-iotests/304 b/tests/qemu-iotests/304
index 198f282087..1ebf999930 100755
--- a/tests/qemu-iotests/304
+++ b/tests/qemu-iotests/304
@@ -23,7 +23,7 @@
 import iotests
 from iotests import qemu_img_create, qemu_img_log, file_path
 
-iotests.script_initialize(supported_fmts=['qcow2'],
+iotests.script_initialize(supported_fmts=['qcow2', 'parallels'],
                           supported_protocols=['file'])
 
 test_img = file_path('test.qcow2')
diff --git a/tests/qemu-iotests/tests/block-status-cache b/tests/qemu-iotests/tests/block-status-cache
index 5a7bc2c149..ade3d5b169 100755
--- a/tests/qemu-iotests/tests/block-status-cache
+++ b/tests/qemu-iotests/tests/block-status-cache
@@ -131,5 +131,5 @@ class TestBscWithNbd(iotests.QMPTestCase):
 if __name__ == '__main__':
     # The block-status cache only works on the protocol layer, so to test it,
     # we can only use the raw format
-    iotests.main(supported_fmts=['raw'],
+    iotests.main(supported_fmts=['raw', 'parallels'],
                  supported_protocols=['file'])
-- 
2.34.1

[PATCH v2 20/20] tests: Add parallels format support to image-fleecing

[Alexander Ivanov]

Use a different bitmap name for parallels images because their has own ID
format, and can't contain an arbitrary string.

Replace hardcoded 'qcow2' format to iotests.imgfmt.

Add 'parallels' to supported formats.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 tests/qemu-iotests/tests/image-fleecing | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/tests/image-fleecing b/tests/qemu-iotests/tests/image-fleecing
index 5e3b2c7e46..dd940b7203 100755
--- a/tests/qemu-iotests/tests/image-fleecing
+++ b/tests/qemu-iotests/tests/image-fleecing
@@ -28,7 +28,7 @@ import iotests
 from iotests import log, qemu_img, qemu_io
 
 iotests.script_initialize(
-    supported_fmts=['qcow2'],
+    supported_fmts=['qcow2', 'parallels'],
     supported_platforms=['linux'],
     required_fmts=['copy-before-write'],
     unsupported_imgopts=['compat']
@@ -61,12 +61,17 @@ def do_test(vm, use_cbw, use_snapshot_access_filter, base_img_path,
     if push_backup:
         assert use_cbw
 
+    if iotests.imgfmt == 'parallels':
+        bitmap_name = '00000000-0000-0000-0000-000000000000'
+    else:
+        bitmap_name = 'bitmap0'
+
     log('--- Setting up images ---')
     log('')
 
     qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M')
     if bitmap:
-        qemu_img('bitmap', '--add', base_img_path, 'bitmap0')
+        qemu_img('bitmap', '--add', base_img_path, bitmap_name)
 
     if use_snapshot_access_filter:
         assert use_cbw
@@ -75,7 +80,7 @@ def do_test(vm, use_cbw, use_snapshot_access_filter, base_img_path,
         qemu_img('create', '-f', 'qcow2', fleece_img_path, '64M')
 
     if push_backup:
-        qemu_img('create', '-f', 'qcow2', target_img_path, '64M')
+        qemu_img('create', '-f', iotests.imgfmt, target_img_path, '64M')
 
     for p in patterns:
         qemu_io('-f', iotests.imgfmt,
@@ -130,7 +135,7 @@ def do_test(vm, use_cbw, use_snapshot_access_filter, base_img_path,
         }
 
         if bitmap:
-            fl_cbw['bitmap'] = {'node': src_node, 'name': 'bitmap0'}
+            fl_cbw['bitmap'] = {'node': src_node, 'name': bitmap_name}
 
         log(vm.qmp('blockdev-add', fl_cbw))
 
-- 
2.34.1