[PATCH v4 00/21] parallels: Add full dirty bitmap support

[PATCH v4 00/21] 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-v4

v4:
4: A new patch with limitation of search in parallels_mark_used.
5: Previously 4. Search is limited to (cluster_index + count).
6: Previously 5. Added GRAPH_RDLOCK annotation, added a note in the commit
   message.
11: Previously 10. Added GRAPH_RDLOCK annotation.
16-18: Added GRAPH_RDLOCK annotations.

v3:
1: Fixed the order of g_free() and s->used_bmap = NULL.
3,4: Made mark_used() a global function before mark_unused() addition. In
     this way we can avoid compilation warnings.
5-9: Patches shifted.
11: Added GRAPH_RDLOCK annotation to parallels_inactivate(). Guard
    parallels_close() with GRAPH_RDLOCK_GUARD_MAINLOOP().
12-21: Patches shifted.

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 (21):
  parallels: Set s->used_bmap to NULL in parallels_free_used_bitmap()
  parallels: Move inactivation code to a separate function
  parallels: Make mark_used() a global function
  parallels: Limit search in parallels_mark_used to the last marked
    claster
  parallels: Add parallels_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: 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                       | 183 +++++++++-
 block/parallels.c                           | 371 ++++++++++++--------
 block/parallels.h                           |  14 +-
 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, 453 insertions(+), 176 deletions(-)

-- 
2.40.1

[PATCH v4 01/21] 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>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 block/parallels.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/parallels.c b/block/parallels.c
index 9205a0864f..072b1efd78 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -245,6 +245,7 @@ static void parallels_free_used_bitmap(BlockDriverState *bs)
     BDRVParallelsState *s = bs->opaque;
     s->used_bmap_size = 0;
     g_free(s->used_bmap);
+    s->used_bmap = NULL;
 }
 
 static int64_t coroutine_fn GRAPH_RDLOCK
-- 
2.40.1

[PATCH v4 02/21] 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>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 block/parallels.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 072b1efd78..992362ce29 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1424,6 +1424,20 @@ fail:
     return ret;
 }
 
+static int GRAPH_RDLOCK 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)
 {
@@ -1432,12 +1446,7 @@ static void parallels_close(BlockDriverState *bs)
     GRAPH_RDLOCK_GUARD_MAINLOOP();
 
     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);
@@ -1476,6 +1485,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.40.1

[PATCH v4 03/21] parallels: Make mark_used() a global function

[Alexander Ivanov]

We will need this function and a function for marking unused clusters (will
be added in the next patch) in parallels-ext.c too. Let it be a global
function parallels_mark_used().

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 block/parallels.c | 14 ++++++++------
 block/parallels.h |  3 +++
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 992362ce29..ae524f1820 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);
@@ -232,7 +232,8 @@ static int GRAPH_RDLOCK 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;
         }
@@ -366,7 +367,8 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
         }
     }
 
-    ret = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, to_allocate);
+    ret = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
+                              host_off, to_allocate);
     if (ret < 0) {
         /* Image consistency is broken. Alarm! */
         return ret;
@@ -827,7 +829,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;
@@ -887,7 +889,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 423b2ad727..68077416b1 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -90,6 +90,9 @@ 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 GRAPH_RDLOCK
 parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
                                 Error **errp);
-- 
2.40.1

[PATCH v4 04/21] parallels: Limit search in parallels_mark_used to the last marked claster

[Alexander Ivanov]

There is no necessity to search to the end of the bitmap. Limit the search
area as cluster_index + count.

Add cluster_end variable to avoid its calculation in a few places.

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

diff --git a/block/parallels.c b/block/parallels.c
index ae524f1820..4470519656 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -182,13 +182,14 @@ 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);
+    uint32_t cluster_end, cluster_index = host_cluster_index(s, off);
     unsigned long next_used;
-    if (cluster_index + count > bitmap_size) {
+    cluster_end = cluster_index + count;
+    if (cluster_end > bitmap_size) {
         return -E2BIG;
     }
-    next_used = find_next_bit(bitmap, bitmap_size, cluster_index);
-    if (next_used < cluster_index + count) {
+    next_used = find_next_bit(bitmap, cluster_end, cluster_index);
+    if (next_used < cluster_end) {
         return -EBUSY;
     }
     bitmap_set(bitmap, cluster_index, count);
-- 
2.40.1

[PATCH v4 05/21] parallels: Add parallels_mark_unused() helper

[Alexander Ivanov]

Add a helper to set unused areas in the used bitmap.

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

diff --git a/block/parallels.c b/block/parallels.c
index 4470519656..13726fb3d5 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -196,6 +196,24 @@ int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
     return 0;
 }
 
+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_end, cluster_index = host_cluster_index(s, off);
+    unsigned long next_unused;
+    cluster_end = cluster_index + count;
+    if (cluster_end > bitmap_size) {
+        return -E2BIG;
+    }
+    next_unused = find_next_zero_bit(bitmap, cluster_end, cluster_index);
+    if (next_unused < cluster_end) {
+        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
diff --git a/block/parallels.h b/block/parallels.h
index 68077416b1..02b857b4a4 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -92,6 +92,8 @@ typedef struct 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);
 
 int GRAPH_RDLOCK
 parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
-- 
2.40.1

[PATCH v4 06/21] 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
parallels_allocate_host_clusters().

This function can be called not only from coroutines so all the
*_co_* functions were replaced by corresponding wrappers.

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

diff --git a/block/parallels.c b/block/parallels.c
index 13726fb3d5..658902ae51 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -268,58 +268,31 @@ static void parallels_free_used_bitmap(BlockDriverState *bs)
     s->used_bmap = NULL;
 }
 
-static int64_t coroutine_fn GRAPH_RDLOCK
-allocate_clusters(BlockDriverState *bs, int64_t sector_num,
-                  int nb_sectors, int *pnum)
+int64_t GRAPH_RDLOCK 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;
@@ -329,15 +302,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;
@@ -349,14 +322,59 @@ 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 = parallels_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
@@ -373,33 +391,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) {
+            parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
+                                  host_off, to_allocate);
             return ret;
         }
     }
 
-    ret = parallels_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 02b857b4a4..493c89e976 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -95,6 +95,9 @@ int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
 int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
                           uint32_t bitmap_size, int64_t off, uint32_t count);
 
+int64_t GRAPH_RDLOCK parallels_allocate_host_clusters(BlockDriverState *bs,
+                                                      int64_t *clusters);
+
 int GRAPH_RDLOCK
 parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
                                 Error **errp);
-- 
2.40.1

[PATCH v4 07/21] 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 658902ae51..8a6e2ba7ee 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.40.1

[PATCH v4 08/21] 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 8a6e2ba7ee..04c114f696 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.40.1

[PATCH v4 09/21] 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 04c114f696..0ae06ec0b1 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.40.1

[PATCH v4 10/21] 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 0ae06ec0b1..f38dd2309c 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1421,13 +1421,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.40.1

[PATCH v4 11/21] 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 | 168 ++++++++++++++++++++++++++++++++++++++++++
 block/parallels.c     |  16 +++-
 block/parallels.h     |   5 ++
 3 files changed, 187 insertions(+), 2 deletions(-)

diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index b4e14c88f2..c83d1ea393 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"
@@ -300,3 +301,170 @@ out:
 
     return ret;
 }
+
+static void GRAPH_RDLOCK 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 GRAPH_RDLOCK
+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 f38dd2309c..a49922c6a7 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1466,14 +1466,25 @@ fail:
 static int GRAPH_RDLOCK 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;
 }
@@ -1525,6 +1536,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 493c89e976..9db4f5c908 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -101,5 +101,10 @@ int64_t GRAPH_RDLOCK parallels_allocate_host_clusters(BlockDriverState *bs,
 int GRAPH_RDLOCK
 parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
                                 Error **errp);
+void GRAPH_RDLOCK
+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.40.1

[PATCH v4 12/21] 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 c83d1ea393..195b01b109 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -175,10 +175,6 @@ parallels_load_bitmap(BlockDriverState *bs, uint8_t *data, size_t data_size,
         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 a49922c6a7..d5d87984cf 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.40.1

[PATCH v4 13/21] 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 195b01b109..033ca3ec3a 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -354,7 +354,7 @@ static void GRAPH_RDLOCK parallels_save_bitmap(BlockDriverState *bs,
     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 GRAPH_RDLOCK parallels_save_bitmap(BlockDriverState *bs,
             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.40.1

[PATCH v4 14/21] 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 033ca3ec3a..2a7ff6e35b 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -255,6 +255,7 @@ parallels_parse_format_extension(BlockDriverState *bs, uint8_t *ext_cluster,
             if (!bitmap) {
                 goto fail;
             }
+            bdrv_dirty_bitmap_set_persistence(bitmap, true);
             bitmaps = g_slist_append(bitmaps, bitmap);
             break;
 
-- 
2.40.1

[PATCH v4 15/21] 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 d5d87984cf..fb7bc5e555 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -773,7 +773,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);
@@ -781,43 +781,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.40.1

[PATCH v4 16/21] 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 fb7bc5e555..136865d53e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1454,6 +1454,23 @@ fail:
     return ret;
 }
 
+static int GRAPH_RDLOCK 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 GRAPH_RDLOCK parallels_inactivate(BlockDriverState *bs)
 {
     BDRVParallelsState *s = bs->opaque;
@@ -1471,8 +1488,7 @@ static int GRAPH_RDLOCK 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.40.1

[PATCH v4 17/21] 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 136865d53e..5ed58826bb 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -768,57 +768,87 @@ parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res,
     return 0;
 }
 
+static int64_t GRAPH_RDLOCK
+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;
@@ -1454,23 +1484,6 @@ fail:
     return ret;
 }
 
-static int GRAPH_RDLOCK 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 GRAPH_RDLOCK parallels_inactivate(BlockDriverState *bs)
 {
     BDRVParallelsState *s = bs->opaque;
@@ -1488,7 +1501,7 @@ static int GRAPH_RDLOCK 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.40.1

[PATCH v4 18/21] 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 5ed58826bb..2803119699 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -268,6 +268,13 @@ static void parallels_free_used_bitmap(BlockDriverState *bs)
     s->used_bmap = NULL;
 }
 
+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 GRAPH_RDLOCK parallels_allocate_host_clusters(BlockDriverState *bs,
                                                       int64_t *clusters)
 {
@@ -279,7 +286,7 @@ int64_t GRAPH_RDLOCK 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;
@@ -302,9 +309,6 @@ int64_t GRAPH_RDLOCK 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);
 
@@ -320,8 +324,7 @@ int64_t GRAPH_RDLOCK 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;
@@ -758,13 +761,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;
 }
 
@@ -803,8 +800,6 @@ parallels_check_unused_clusters(BlockDriverState *bs, bool truncate)
         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...
@@ -1436,11 +1430,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 9db4f5c908..b494d93139 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.40.1

[PATCH v4 19/21] 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.40.1

[PATCH v4 20/21] 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.40.1

[PATCH v4 21/21] 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.40.1

Re: [PATCH v4 21/21] tests: Add parallels format support to image-fleecing

[Vladimir Sementsov-Ogievskiy]

On 28.12.23 13:12, Alexander Ivanov wrote:
> 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>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

Re: [PATCH v4 04/21] parallels: Limit search in parallels_mark_used to the last marked claster

[Denis V. Lunev]

On 12/28/23 11:12, Alexander Ivanov wrote:
> There is no necessity to search to the end of the bitmap. Limit the search
> area as cluster_index + count.
>
> Add cluster_end variable to avoid its calculation in a few places.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index ae524f1820..4470519656 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -182,13 +182,14 @@ 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);
> +    uint32_t cluster_end, cluster_index = host_cluster_index(s, off);
>       unsigned long next_used;
> -    if (cluster_index + count > bitmap_size) {
> +    cluster_end = cluster_index + count;
> +    if (cluster_end > bitmap_size) {
>           return -E2BIG;
>       }
> -    next_used = find_next_bit(bitmap, bitmap_size, cluster_index);
> -    if (next_used < cluster_index + count) {
> +    next_used = find_next_bit(bitmap, cluster_end, cluster_index);
> +    if (next_used < cluster_end) {
>           return -EBUSY;
>       }
>       bitmap_set(bitmap, cluster_index, count);
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH v4 05/21] parallels: Add parallels_mark_unused() helper

[Denis V. Lunev]

On 12/28/23 11:12, Alexander Ivanov wrote:
> Add a helper to set unused areas in the used bitmap.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 18 ++++++++++++++++++
>   block/parallels.h |  2 ++
>   2 files changed, 20 insertions(+)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 4470519656..13726fb3d5 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -196,6 +196,24 @@ int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
>       return 0;
>   }
>   
> +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_end, cluster_index = host_cluster_index(s, off);
> +    unsigned long next_unused;
> +    cluster_end = cluster_index + count;
> +    if (cluster_end > bitmap_size) {
> +        return -E2BIG;
> +    }
> +    next_unused = find_next_zero_bit(bitmap, cluster_end, cluster_index);
> +    if (next_unused < cluster_end) {
> +        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
> diff --git a/block/parallels.h b/block/parallels.h
> index 68077416b1..02b857b4a4 100644
> --- a/block/parallels.h
> +++ b/block/parallels.h
> @@ -92,6 +92,8 @@ typedef struct 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);
>   
>   int GRAPH_RDLOCK
>   parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH v4 06/21] parallels: Move host clusters allocation to a separate function

[Denis V. Lunev]

On 12/28/23 11:12, Alexander Ivanov wrote:
> For parallels images extensions we need to allocate host clusters
> without any connection to BAT. Move host clusters allocation code to
> parallels_allocate_host_clusters().
>
> This function can be called not only from coroutines so all the
> *_co_* functions were replaced by corresponding wrappers.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 128 ++++++++++++++++++++++++----------------------
>   block/parallels.h |   3 ++
>   2 files changed, 71 insertions(+), 60 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 13726fb3d5..658902ae51 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -268,58 +268,31 @@ static void parallels_free_used_bitmap(BlockDriverState *bs)
>       s->used_bmap = NULL;
>   }
>   
> -static int64_t coroutine_fn GRAPH_RDLOCK
> -allocate_clusters(BlockDriverState *bs, int64_t sector_num,
> -                  int nb_sectors, int *pnum)
> +int64_t GRAPH_RDLOCK 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.
> -         */
This comment seems useful. I'd better keep it as is.

For now (if we will not have a re-submission, I'll have
the comment back in the code). If the code will be
resubmitted, please add it back.

>           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;
> @@ -329,15 +302,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;
> @@ -349,14 +322,59 @@ 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 = parallels_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
> @@ -373,33 +391,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) {
> +            parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
> +                                  host_off, to_allocate);
>               return ret;
>           }
>       }
>   
> -    ret = parallels_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 02b857b4a4..493c89e976 100644
> --- a/block/parallels.h
> +++ b/block/parallels.h
> @@ -95,6 +95,9 @@ int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
>   int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
>                             uint32_t bitmap_size, int64_t off, uint32_t count);
>   
> +int64_t GRAPH_RDLOCK parallels_allocate_host_clusters(BlockDriverState *bs,
> +                                                      int64_t *clusters);
> +
>   int GRAPH_RDLOCK
>   parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
>                                   Error **errp);
There is a difference in between what we have had before
this patch and after. On a error originally we have had
data_end unchanged, now it points to a wrong location.

May be this would be mitigated later, but I'd prefer
to have data_end updated in mark_unused. That would make
a lot of sense.

Anyway, with data_end dropped at the end of the series,
this would not worth efforts. Thus this is fine.

With a note about comment,

Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH v4 07/21] parallels: Set data_end value in parallels_check_leak()

[Denis V. Lunev]

On 12/28/23 11:12, Alexander Ivanov wrote:
> 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 658902ae51..8a6e2ba7ee 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;
>               }
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH v4 08/21] parallels: Recreate used bitmap in parallels_check_leak()

[Denis V. Lunev]

On 12/28/23 11:12, Alexander Ivanov wrote:
> 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 8a6e2ba7ee..04c114f696 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;
>               }
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH v4 09/21] parallels: Add a note about used bitmap in parallels_check_duplicate()

[Denis V. Lunev]

On 12/28/23 11:12, Alexander Ivanov wrote:
> 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 04c114f696..0ae06ec0b1 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.
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH v4 10/21] parallels: Create used bitmap even if checks needed

[Denis V. Lunev]

On 12/28/23 11:12, Alexander Ivanov wrote:
> 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 0ae06ec0b1..f38dd2309c 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -1421,13 +1421,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
Why do we need it? Most likely we will have to recreate it
on a error. If there is some sense - we need a real motivation
why do we need used bitmap.

Here, at this point, we have already detected that there is
something very bad happened and we can have too long file
or something like that.

Re: [PATCH v4 12/21] parallels: Let image extensions work in RW mode

[Denis V. Lunev]

On 12/28/23 11:12, Alexander Ivanov wrote:
> 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 c83d1ea393..195b01b109 100644
> --- a/block/parallels-ext.c
> +++ b/block/parallels-ext.c
> @@ -175,10 +175,6 @@ parallels_load_bitmap(BlockDriverState *bs, uint8_t *data, size_t data_size,
>           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 a49922c6a7..d5d87984cf 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;
>           }
>       }
>   
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH v4 11/21] parallels: Add dirty bitmaps saving

[Denis V. Lunev]

On 12/28/23 11:12, Alexander Ivanov 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 | 168 ++++++++++++++++++++++++++++++++++++++++++
>   block/parallels.c     |  16 +++-
>   block/parallels.h     |   5 ++
>   3 files changed, 187 insertions(+), 2 deletions(-)
>
> diff --git a/block/parallels-ext.c b/block/parallels-ext.c
> index b4e14c88f2..c83d1ea393 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"
> @@ -300,3 +301,170 @@ out:
>   
>       return ret;
>   }
> +
> +static void GRAPH_RDLOCK parallels_save_bitmap(BlockDriverState *bs,
> +                                               BdrvDirtyBitmap *bitmap,
> +                                               uint8_t **buf, int *buf_size)
Do we need a error?

> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    ParallelsFeatureHeader *fh;
> +    ParallelsDirtyBitmapFeature *bh;
> +    uint64_t *l1_table, l1_size, granularity, limit;
I would say that 'limit' here means 'bits_in_cluster'

We are writing the new code and I would prefer if we
would have bits, bytes, clusters, sectors etc are
clearly seen in variable names. It is quite complex
to track various measurables.

> +    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);
As far as I can see, bdrv_dirty_bitmap_serialization_size() returns bytes.
That is correct. Thus multiplying it by 8 seems fatal mistake.

I am also quite unsure that we should roundup to the cluster, that
will occupy more clusters than needed. Can you please take a look
here
https://src.openvz.org/users/ibazhitov/repos/ploop/browse/tools/ploop-cbt.c

> +    /* 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));
bh = fh + 1 ?
> +    l1_table = (uint64_t *)((uint8_t *)bh + sizeof(*bh));
l1_table = bh + 1 ?
> +
> +    fh->magic = cpu_to_le64(PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC);
> +    fh->data_size = cpu_to_le32(l1_size * 8 + sizeof(*bh));
I am quite concerned here. Please compare with

int save_dirty_bitmap(int devfd, struct delta *delta, off_t offset,
         void *buf, __u32 *size, void *or_data, writer_fn wr,
         void *data)
{
     int ret = 0;
     struct ploop_pvd_header *vh;
     size_t block_size;
     __u64 bits, bytes, *p;
     __u32 byte_granularity;
     void *block;
     struct ploop_pvd_dirty_bitmap_raw *raw = (struct 
ploop_pvd_dirty_bitmap_raw *)buf;
     char x[50];

     vh = (struct ploop_pvd_header *)delta->hdr0;

     /* granularity and uuid */
     if ((ret = cbt_get_dirty_bitmap_metadata(devfd, raw->m_Id, 
&raw->m_Granularity)))
         return ret;
     raw->m_Granularity /= SECTOR_SIZE;

     block_size = vh->m_Sectors * SECTOR_SIZE;
     if (p_memalign((void **)&block, 4096, block_size))
         return SYSEXIT_MALLOC;

     raw->m_Size = vh->m_SizeInSectors_v2;

     byte_granularity = raw->m_Granularity * SECTOR_SIZE;
     bits = ((raw->m_Size + raw->m_Granularity - 1) / raw->m_Granularity);
     bytes = (bits + 7) >> 3;
     raw->m_L1Size = (bytes + block_size - 1) / block_size;

which means that the header is rotten. In that case can you pls
take a look why this has not been caught by tests?

> +
> +    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);
That is incomplete. You have to clean all clusters inside of the
extension.

> +            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 GRAPH_RDLOCK
> +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 f38dd2309c..a49922c6a7 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -1466,14 +1466,25 @@ fail:
>   static int GRAPH_RDLOCK parallels_inactivate(BlockDriverState *bs)
>   {
>       BDRVParallelsState *s = bs->opaque;
> +    Error *err = NULL;
>       int ret;
>   
> +    parallels_store_persistent_dirty_bitmaps(bs, &err);
> +    if (err != NULL) {
For me this looks quite odd, indirect check for error. Not good

> +        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;
>   }
> @@ -1525,6 +1536,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 493c89e976..9db4f5c908 100644
> --- a/block/parallels.h
> +++ b/block/parallels.h
> @@ -101,5 +101,10 @@ int64_t GRAPH_RDLOCK parallels_allocate_host_clusters(BlockDriverState *bs,
>   int GRAPH_RDLOCK
>   parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
>                                   Error **errp);
> +void GRAPH_RDLOCK
> +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

Re: [PATCH v4 12/21] parallels: Let image extensions work in RW mode

[Denis V. Lunev]

On 1/16/24 15:45, Denis V. Lunev wrote:
> On 12/28/23 11:12, Alexander Ivanov wrote:
>> 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 c83d1ea393..195b01b109 100644
>> --- a/block/parallels-ext.c
>> +++ b/block/parallels-ext.c
>> @@ -175,10 +175,6 @@ parallels_load_bitmap(BlockDriverState *bs, 
>> uint8_t *data, size_t data_size,
>>           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 a49922c6a7..d5d87984cf 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;
>>           }
>>       }
> Reviewed-by: Denis V. Lunev <den@openvz.org>
This patch also deserves a note, what will happen with
format extensions clusters. According to the current
policy, we have only transient extensions, i.e.
CBT. Cluster allocation mechanism will reuse these
clusters as they are not marked as used.
Thus we should either set format extension offset
in the header to 0 or perform any other correct
measures to properly handle this.

It should also be noted, that on any QEMU crash
appropriate format extensions are to be properly
treated. We could not make them RW until this would
not be addressed as we could easily mess up with
trashed metadata.

Re: [PATCH v4 12/21] parallels: Let image extensions work in RW mode

[Denis V. Lunev]

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

On 12/28/23 11:12, Alexander Ivanov wrote:
> 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 c83d1ea393..195b01b109 100644
> --- a/block/parallels-ext.c
> +++ b/block/parallels-ext.c
> @@ -175,10 +175,6 @@ parallels_load_bitmap(BlockDriverState *bs, uint8_t *data, size_t data_size,
>           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 a49922c6a7..d5d87984cf 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;
>           }
>       }
>   
something like attached should be taken into the account.
Though the destiny of cluster with old
extension offset requires some thinking.

I would say that it could be marked as used on read.
Anyway, this requires at least detailed thinking.

[-- Attachment #2: 0001-parallels-drop-dirty-bitmap-data-if-the-image-was-no.patch --]
[-- Type: text/x-patch, Size: 1259 bytes --]

From 2f70166ef640304726d5dfcee3e906b0ba1676dd Mon Sep 17 00:00:00 2001
From: "Denis V. Lunev" <den@openvz.org>
Date: Thu, 18 Jan 2024 13:29:56 +0100
Subject: [PATCH 1/1] parallels: drop dirty bitmap data if the image was not
 properly closed

This data is obsolete.

The approach is exactly the same like we use with QCOW2.

Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 block/parallels-ext.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index c83d1ea393..54e8bb66a6 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -255,6 +255,14 @@ parallels_parse_format_extension(BlockDriverState *bs, uint8_t *ext_cluster,
             return 0;
 
         case PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC:
+            if (s->header_unclean) {
+                /*
+                 * The image was not closed correctly and thus dirty bitmap
+                 * data inside the image is considered as incorrect and thus
+                 * it should be dropper, exactly like we do for QCOW2.
+                 */
+                break;
+            }
             bitmap = parallels_load_bitmap(bs, pos, fh.data_size, errp);
             if (!bitmap) {
                 goto fail;
-- 
2.34.1

Re: [PATCH v4 13/21] parallels: Handle L1 entries equal to one

[Denis V. Lunev]

On 12/28/23 11:12, Alexander Ivanov wrote:
> 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 195b01b109..033ca3ec3a 100644
> --- a/block/parallels-ext.c
> +++ b/block/parallels-ext.c
> @@ -354,7 +354,7 @@ static void GRAPH_RDLOCK parallels_save_bitmap(BlockDriverState *bs,
>       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 GRAPH_RDLOCK parallels_save_bitmap(BlockDriverState *bs,
>               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;
That is not enough. We should handle all-one and all-zeroes according
to the spec and all-zeroes would be much more common.

Re: [PATCH v4 14/21] parallels: Make a loaded dirty bitmap persistent

[Denis V. Lunev]

On 12/28/23 11:12, Alexander Ivanov wrote:
> 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 033ca3ec3a..2a7ff6e35b 100644
> --- a/block/parallels-ext.c
> +++ b/block/parallels-ext.c
> @@ -255,6 +255,7 @@ parallels_parse_format_extension(BlockDriverState *bs, uint8_t *ext_cluster,
>               if (!bitmap) {
>                   goto fail;
>               }
> +            bdrv_dirty_bitmap_set_persistence(bitmap, true);
>               bitmaps = g_slist_append(bitmaps, bitmap);
>               break;
>   
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH v4 15/21] parallels: Reverse a conditional in parallels_check_leak() to reduce indents

[Denis V. Lunev]

On 12/28/23 11:12, Alexander Ivanov wrote:
> 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 d5d87984cf..fb7bc5e555 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -773,7 +773,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);
> @@ -781,43 +781,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;
>           }
>       }
>   
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH v4 16/21] parallels: Truncate images on the last used cluster

[Denis V. Lunev]

On 12/28/23 11:12, Alexander Ivanov wrote:
> 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 fb7bc5e555..136865d53e 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -1454,6 +1454,23 @@ fail:
>       return ret;
>   }
>   
> +static int GRAPH_RDLOCK 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 GRAPH_RDLOCK parallels_inactivate(BlockDriverState *bs)
>   {
>       BDRVParallelsState *s = bs->opaque;
> @@ -1471,8 +1488,7 @@ static int GRAPH_RDLOCK 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));
>       }
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH v4 17/21] parallels: Check unused clusters in parallels_check_leak()

[Denis V. Lunev]

On 12/28/23 11:12, Alexander Ivanov wrote:
> 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 136865d53e..5ed58826bb 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -768,57 +768,87 @@ parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res,
>       return 0;
>   }
>   
> +static int64_t GRAPH_RDLOCK
> +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;
> @@ -1454,23 +1484,6 @@ fail:
>       return ret;
>   }
>   
> -static int GRAPH_RDLOCK 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 GRAPH_RDLOCK parallels_inactivate(BlockDriverState *bs)
>   {
>       BDRVParallelsState *s = bs->opaque;
> @@ -1488,7 +1501,7 @@ static int GRAPH_RDLOCK 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));
>       }
This particular patch does not look good.

You are deleting the stuff you have just added (in the previous patch)
and you add lengthy operation (recreate used bitmap) on the image close,
which is better to be finished first.

I would say that the concept should be somehow reworked or the
whole patch is to be dropped.

Re: [PATCH v4 18/21] parallels: Remove unnecessary data_end field

[Denis V. Lunev]

On 12/28/23 11:12, Alexander Ivanov wrote:
> 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 5ed58826bb..2803119699 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -268,6 +268,13 @@ static void parallels_free_used_bitmap(BlockDriverState *bs)
>       s->used_bmap = NULL;
>   }
>   
> +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 GRAPH_RDLOCK parallels_allocate_host_clusters(BlockDriverState *bs,
>                                                         int64_t *clusters)
>   {
> @@ -279,7 +286,7 @@ int64_t GRAPH_RDLOCK 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;
> @@ -302,9 +309,6 @@ int64_t GRAPH_RDLOCK 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);
>   
> @@ -320,8 +324,7 @@ int64_t GRAPH_RDLOCK 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) {
This seems wrong. The check whether the offset is in a tail area
or not has been deleted. This looks incorrect.

> +        if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
>               ret = bdrv_pwrite_zeroes(bs->file, host_off, bytes, 0);
>               if (ret < 0) {
>                   return ret;
> @@ -758,13 +761,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;
>   }
>   
> @@ -803,8 +800,6 @@ parallels_check_unused_clusters(BlockDriverState *bs, bool truncate)
>           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...
> @@ -1436,11 +1430,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;
break;
>           }
>       }
> -    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 9db4f5c908..b494d93139 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;
>   

Re: [PATCH v4 00/21] parallels: Add full dirty bitmap support

[Denis V. Lunev]

On 12/28/23 11:12, Alexander Ivanov wrote:
> 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-v4
>
> v4:
> 4: A new patch with limitation of search in parallels_mark_used.
> 5: Previously 4. Search is limited to (cluster_index + count).
> 6: Previously 5. Added GRAPH_RDLOCK annotation, added a note in the commit
>     message.
> 11: Previously 10. Added GRAPH_RDLOCK annotation.
> 16-18: Added GRAPH_RDLOCK annotations.
>
> v3:
> 1: Fixed the order of g_free() and s->used_bmap = NULL.
> 3,4: Made mark_used() a global function before mark_unused() addition. In
>       this way we can avoid compilation warnings.
> 5-9: Patches shifted.
> 11: Added GRAPH_RDLOCK annotation to parallels_inactivate(). Guard
>      parallels_close() with GRAPH_RDLOCK_GUARD_MAINLOOP().
> 12-21: Patches shifted.
>
> 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 (21):
>    parallels: Set s->used_bmap to NULL in parallels_free_used_bitmap()
>    parallels: Move inactivation code to a separate function
>    parallels: Make mark_used() a global function
>    parallels: Limit search in parallels_mark_used to the last marked
>      claster
>    parallels: Add parallels_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: 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                       | 183 +++++++++-
>   block/parallels.c                           | 371 ++++++++++++--------
>   block/parallels.h                           |  14 +-
>   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, 453 insertions(+), 176 deletions(-)
>
I would also say that if we add parallels extension in
read-write mode, we should also add appropriates clauses
into parallels_check.

Den

Re: [PATCH v4 11/21] parallels: Add dirty bitmaps saving

[Alexander Ivanov]

On 1/18/24 14:27, Denis V. Lunev wrote:
> On 12/28/23 11:12, Alexander Ivanov 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 | 168 ++++++++++++++++++++++++++++++++++++++++++
>>   block/parallels.c     |  16 +++-
>>   block/parallels.h     |   5 ++
>>   3 files changed, 187 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/parallels-ext.c b/block/parallels-ext.c
>> index b4e14c88f2..c83d1ea393 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"
>> @@ -300,3 +301,170 @@ out:
>>         return ret;
>>   }
>> +
>> +static void GRAPH_RDLOCK parallels_save_bitmap(BlockDriverState *bs,
>> +                                               BdrvDirtyBitmap *bitmap,
>> +                                               uint8_t **buf, int 
>> *buf_size)
> Do we need a error?
I think no. We save bitmaps in a loop. If some bitmap has a problem it 
would be better
just to  print an error message and try to save other bitmaps.
>
>> +{
>> +    BDRVParallelsState *s = bs->opaque;
>> +    ParallelsFeatureHeader *fh;
>> +    ParallelsDirtyBitmapFeature *bh;
>> +    uint64_t *l1_table, l1_size, granularity, limit;
> I would say that 'limit' here means 'bits_in_cluster'
>
> We are writing the new code and I would prefer if we
> would have bits, bytes, clusters, sectors etc are
> clearly seen in variable names. It is quite complex
> to track various measurables.
OK
>> +    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);
> As far as I can see, bdrv_dirty_bitmap_serialization_size() returns 
> bytes.
> That is correct. Thus multiplying it by 8 seems fatal mistake.
l1_size is amount of entries in l1_table. Every entry has 8 bytes size 
and corresponds
to one cluster containing a part of a bitmap.
> I am also quite unsure that we should roundup to the cluster, that
> will occupy more clusters than needed. Can you please take a look
> here
> https://src.openvz.org/users/ibazhitov/repos/ploop/browse/tools/ploop-cbt.c 
>
Why shouldn't we? We need to have l1_table which should be able to contain
all the bitmap data. Note: there is not a ROUND_UP(), but DIV_ROUND_UP().
>> +    /* 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));
> bh = fh + 1 ?
>> +    l1_table = (uint64_t *)((uint8_t *)bh + sizeof(*bh));
> l1_table = bh + 1 ?
Yes, it's better.
>> +
>> +    fh->magic = cpu_to_le64(PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC);
>> +    fh->data_size = cpu_to_le32(l1_size * 8 + sizeof(*bh));
> I am quite concerned here. Please compare with
>
> int save_dirty_bitmap(int devfd, struct delta *delta, off_t offset,
>         void *buf, __u32 *size, void *or_data, writer_fn wr,
>         void *data)
> {
>     int ret = 0;
>     struct ploop_pvd_header *vh;
>     size_t block_size;
>     __u64 bits, bytes, *p;
>     __u32 byte_granularity;
>     void *block;
>     struct ploop_pvd_dirty_bitmap_raw *raw = (struct 
> ploop_pvd_dirty_bitmap_raw *)buf;
>     char x[50];
>
>     vh = (struct ploop_pvd_header *)delta->hdr0;
>
>     /* granularity and uuid */
>     if ((ret = cbt_get_dirty_bitmap_metadata(devfd, raw->m_Id, 
> &raw->m_Granularity)))
>         return ret;
>     raw->m_Granularity /= SECTOR_SIZE;
>
>     block_size = vh->m_Sectors * SECTOR_SIZE;
>     if (p_memalign((void **)&block, 4096, block_size))
>         return SYSEXIT_MALLOC;
>
>     raw->m_Size = vh->m_SizeInSectors_v2;
>
>     byte_granularity = raw->m_Granularity * SECTOR_SIZE;
>     bits = ((raw->m_Size + raw->m_Granularity - 1) / raw->m_Granularity);
>     bytes = (bits + 7) >> 3;
>     raw->m_L1Size = (bytes + block_size - 1) / block_size;
>
> which means that the header is rotten. In that case can you pls
> take a look why this has not been caught by tests?
It looks the same if block_size is cluster size in bytes.
bytes - bitmap size in bytes
raw->m_L1Size - bitmap size in clusters

In my code fh->data_size is l1_table size (8 bytes entry for each bitmap 
cluster) plus
dirty bitmap feature header.
>
>> +
>> +    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);
> That is incomplete. You have to clean all clusters inside of the
> extension.
OK
>
>> +            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 GRAPH_RDLOCK
>> +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 f38dd2309c..a49922c6a7 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -1466,14 +1466,25 @@ fail:
>>   static int GRAPH_RDLOCK parallels_inactivate(BlockDriverState *bs)
>>   {
>>       BDRVParallelsState *s = bs->opaque;
>> +    Error *err = NULL;
>>       int ret;
>>   +    parallels_store_persistent_dirty_bitmaps(bs, &err);
>> +    if (err != NULL) {
> For me this looks quite odd, indirect check for error. Not good
Will add return value;
>
>> +        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;
>>   }
>> @@ -1525,6 +1536,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 493c89e976..9db4f5c908 100644
>> --- a/block/parallels.h
>> +++ b/block/parallels.h
>> @@ -101,5 +101,10 @@ int64_t GRAPH_RDLOCK 
>> parallels_allocate_host_clusters(BlockDriverState *bs,
>>   int GRAPH_RDLOCK
>>   parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
>>                                   Error **errp);
>> +void GRAPH_RDLOCK
>> +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
>

-- 
Best regards,
Alexander Ivanov

Re: [PATCH v4 12/21] parallels: Let image extensions work in RW mode

[Alexander Ivanov]

On 1/18/24 14:31, Denis V. Lunev wrote:
> On 1/16/24 15:45, Denis V. Lunev wrote:
>> On 12/28/23 11:12, Alexander Ivanov wrote:
>>> 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 c83d1ea393..195b01b109 100644
>>> --- a/block/parallels-ext.c
>>> +++ b/block/parallels-ext.c
>>> @@ -175,10 +175,6 @@ parallels_load_bitmap(BlockDriverState *bs, 
>>> uint8_t *data, size_t data_size,
>>>           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 a49922c6a7..d5d87984cf 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;
>>>           }
>>>       }
>> Reviewed-by: Denis V. Lunev <den@openvz.org>
> This patch also deserves a note, what will happen with
> format extensions clusters. According to the current
> policy, we have only transient extensions, i.e.
> CBT. Cluster allocation mechanism will reuse these
> clusters as they are not marked as used.
> Thus we should either set format extension offset
> in the header to 0 or perform any other correct
> measures to properly handle this.
Yes, all the clusters used by extensions are marked as unused
on loading. In further work they can be reallocated for other
purposes.
Agree that we need to set ext_off to zero.
>
> It should also be noted, that on any QEMU crash
> appropriate format extensions are to be properly
> treated. We could not make them RW until this would
> not be addressed as we could easily mess up with
> trashed metadata.
If QEMU crashes after extensions loading there will be
zero in the ext_off field and an inappropriate dirty bitmap
will be ignored.

-- 
Best regards,
Alexander Ivanov

Re: [PATCH v4 12/21] parallels: Let image extensions work in RW mode

[Denis V. Lunev]

On 2/28/24 11:25, Alexander Ivanov wrote:
>
>
> On 1/18/24 14:31, Denis V. Lunev wrote:
>> On 1/16/24 15:45, Denis V. Lunev wrote:
>>> On 12/28/23 11:12, Alexander Ivanov wrote:
>>>> 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 c83d1ea393..195b01b109 100644
>>>> --- a/block/parallels-ext.c
>>>> +++ b/block/parallels-ext.c
>>>> @@ -175,10 +175,6 @@ parallels_load_bitmap(BlockDriverState *bs, 
>>>> uint8_t *data, size_t data_size,
>>>>           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 a49922c6a7..d5d87984cf 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;
>>>>           }
>>>>       }
>>> Reviewed-by: Denis V. Lunev <den@openvz.org>
>> This patch also deserves a note, what will happen with
>> format extensions clusters. According to the current
>> policy, we have only transient extensions, i.e.
>> CBT. Cluster allocation mechanism will reuse these
>> clusters as they are not marked as used.
>> Thus we should either set format extension offset
>> in the header to 0 or perform any other correct
>> measures to properly handle this.
> Yes, all the clusters used by extensions are marked as unused
> on loading. In further work they can be reallocated for other
> purposes.
> Agree that we need to set ext_off to zero.
>>
>> It should also be noted, that on any QEMU crash
>> appropriate format extensions are to be properly
>> treated. We could not make them RW until this would
>> not be addressed as we could easily mess up with
>> trashed metadata.
> If QEMU crashes after extensions loading there will be
> zero in the ext_off field and an inappropriate dirty bitmap
> will be ignored.
>
That should be considered as a minimal kludge.
Normally we should mark format extension cluster
as used and nullify references from the bitmaps
there.

Anyway, even if the ext_off is not zero and
bitmaps are present, bitmaps loading over
unclean image should not be performed. They should
be considered outdated.

Den

Re: [PATCH v4 13/21] parallels: Handle L1 entries equal to one

[Alexander Ivanov]

On 1/18/24 14:37, Denis V. Lunev wrote:
> On 12/28/23 11:12, Alexander Ivanov wrote:
>> 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 195b01b109..033ca3ec3a 100644
>> --- a/block/parallels-ext.c
>> +++ b/block/parallels-ext.c
>> @@ -354,7 +354,7 @@ static void GRAPH_RDLOCK 
>> parallels_save_bitmap(BlockDriverState *bs,
>>       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 GRAPH_RDLOCK 
>> parallels_save_bitmap(BlockDriverState *bs,
>>               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;
> That is not enough. We should handle all-one and all-zeroes according
> to the spec and all-zeroes would be much more common.
Buffer for extensions contains zeroes before handling (it was allocated with
qemu_blockalign0). We skip all  all-zeroes l1 entries and the stay zeroed.

-- 
Best regards,
Alexander Ivanov