[PATCH v2 0/5] parallels: Add duplication check, repair at open, fix bugs

[PATCH v2 0/5] parallels: Add duplication check, repair at open, fix bugs

[Alexander Ivanov]

Fix incorrect data end calculation in parallels_open().

Split image leak handling to separate check and fix helpers.

Add checking and repairing duplicate offsets in BAT

Replace fprintf() by qemu_log().

Image repairing in parallels_open().

v2:
2: Moved outsude parallels_check_leak() 2 helpers:
   parallels_get_leak_size() and parallels_fix_leak().
   
3: Used highest_offset() helper in parallels_check_leak(). Fixed a typo.
   Added comments. Replaced g_malloc() call by qemu_memalign(). Replaced
   bdrv_pread() call by bdrv_co_pread(). Got rid of keeping bytes and
   sectors in the same variable. Added setting the bitmap of the used
   clusters for a new allocated cluster if it isn't out of the bitmap.
   Moved the leak fix to the end of all the checks. Removed a dependence
   on image format for the duplicate check.
   
4 (old): Merged this patch to the previous.
4 (former 5): Fixed formatting.
5 (former 6): Fixed comments. Added O_INACTIVE check in the condition.
              Replaced inuse detection by header_unclean checking.
              Replaced playing with corutines by bdrv_check() usage.

Alexander Ivanov (5):
  parallels: Incorrect data end calculation in parallels_open()
  parallels: Split image leak handling to separate check and fix helpers
  parallels: Add checking and repairing duplicate offsets in BAT
  parallels: Replace fprintf by qemu_log in check
  parallels: Image repairing in parallels_open()

 block/parallels.c | 321 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 247 insertions(+), 74 deletions(-)

-- 
2.34.1

[PATCH v2 1/5] parallels: Incorrect data end calculation in parallels_open()

[Alexander Ivanov]

The BDRVParallelsState structure contains data_end field that is measured
in sectors. In parallels_open() initially this field is set by data_off
field from parallels image header.

According to the parallels format documentation, data_off field contains
an offset, in sectors, from the start of the file to the start of the
data area. For "WithoutFreeSpace" images: if data_off is zero, the offset
is calculated as the end of the BAT table plus some padding to ensure
sector size alignment.

The parallels_open() function has code for handling zero value in
data_off, but in the result data_end contains the offset in bytes.

Replace the alignment to sector size by division by sector size and fix
the comparision with s->header_size.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 block/parallels.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index eda3fb558d..ed2cf27abc 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -862,9 +862,9 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     }
     s->data_end = le32_to_cpu(ph.data_off);
     if (s->data_end == 0) {
-        s->data_end = ROUND_UP(bat_entry_off(s->bat_size), BDRV_SECTOR_SIZE);
+        s->data_end = DIV_ROUND_UP(size, BDRV_SECTOR_SIZE);
     }
-    if (s->data_end < s->header_size) {
+    if (s->data_end < (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... */
         s->header_size = size;
-- 
2.34.1

[PATCH v2 2/5] parallels: Split image leak handling to separate check and fix helpers

[Alexander Ivanov]

We need to fix leak after deduplication in the next patch. Move leak
fixing to a separate helper parallels_fix_leak() and add
parallels_get_leak_size() helper wich used in parallels_fix_leak() and
parallels_check_leak().

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

diff --git a/block/parallels.c b/block/parallels.c
index ed2cf27abc..da1e75096c 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -475,21 +475,53 @@ static int parallels_check_outside_image(BlockDriverState *bs,
     return 0;
 }
 
+static int64_t parallels_get_leak_size(BlockDriverState *bs,
+                                       BdrvCheckResult *res)
+{
+    int64_t size;
+    size = bdrv_getlength(bs->file->bs);
+    /*
+     * Before any usage of this function out-of-image corruption has been
+     * fixed. If the function returns a negative value, it means an error.
+     */
+    return (size < 0) ? size : (size - res->image_end_offset);
+}
+
+static int parallels_fix_leak(BlockDriverState *bs,
+                              BdrvCheckResult *res)
+{
+    Error *local_err = NULL;
+    int64_t size;
+    int ret;
+
+    size = parallels_get_leak_size(bs, res);
+    if (size <= 0) {
+        return size;
+    }
+
+    /*
+     * 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);
+        return ret;
+    }
+
+    return 0;
+}
+
 static int parallels_check_leak(BlockDriverState *bs,
                                 BdrvCheckResult *res,
                                 BdrvCheckMode fix)
 {
     BDRVParallelsState *s = bs->opaque;
-    int64_t size, off, high_off, count;
+    int64_t off, high_off, count, leak_size;
     uint32_t i;
     int ret;
 
-    size = bdrv_getlength(bs->file->bs);
-    if (size < 0) {
-        res->check_errors++;
-        return size;
-    }
-
     high_off = 0;
     for (i = 0; i < s->bat_size; i++) {
         off = bat2sect(s, i) << BDRV_SECTOR_BITS;
@@ -499,30 +531,32 @@ static int parallels_check_leak(BlockDriverState *bs,
     }
 
     res->image_end_offset = high_off + s->cluster_size;
-    if (size > res->image_end_offset) {
-        count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
-        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;
-            }
-            res->leaks_fixed += count;
+    leak_size = parallels_get_leak_size(bs, res);
+    if (leak_size < 0) {
+        res->check_errors++;
+        return leak_size;
+    }
+    if (leak_size == 0) {
+        return 0;
+    }
+
+    if (fix & BDRV_FIX_LEAKS) {
+        ret = parallels_fix_leak(bs, res);
+        if (ret < 0) {
+            return ret;
         }
     }
 
+    count = DIV_ROUND_UP(leak_size, s->cluster_size);
+    fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
+            fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size);
+
+    res->leaks += count;
+    if (fix & BDRV_FIX_LEAKS) {
+        res->leaks_fixed += count;
+    }
+
     return 0;
 }
 
-- 
2.34.1

[PATCH v2 3/5] parallels: Add checking and repairing duplicate offsets in BAT

[Alexander Ivanov]

Cluster offsets must be unique among all the BAT entries. Find duplicate
offsets in the BAT and fix it by copying the content of the relevant
cluster to a newly allocated cluster and set the new cluster offset to the
duplicated entry.

Add host_cluster_index() and highest_offset() helpers to deduplicate the
code.

Move parallels_fix_leak() call to parallels_co_check() to fix both types
of leak: real corruption and a leak produced by allocate_clusters()
during deduplication.

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

diff --git a/block/parallels.c b/block/parallels.c
index da1e75096c..73e992875a 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -136,6 +136,26 @@ static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
     return MIN(nb_sectors, ret);
 }
 
+static uint32_t host_cluster_index(BDRVParallelsState *s, int64_t off)
+{
+    off -= s->header->data_off << BDRV_SECTOR_BITS;
+    return off / s->cluster_size;
+}
+
+static int64_t highest_offset(BDRVParallelsState *s)
+{
+    int64_t off, high_off = 0;
+    int i;
+
+    for (i = 0; i < s->bat_size; i++) {
+        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+        if (off > high_off) {
+            high_off = off;
+        }
+    }
+    return high_off;
+}
+
 static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
                             int nb_sectors, int *pnum)
 {
@@ -518,17 +538,9 @@ static int parallels_check_leak(BlockDriverState *bs,
                                 BdrvCheckMode fix)
 {
     BDRVParallelsState *s = bs->opaque;
-    int64_t off, high_off, count, leak_size;
-    uint32_t i;
-    int ret;
+    int64_t high_off, count, leak_size;
 
-    high_off = 0;
-    for (i = 0; i < s->bat_size; i++) {
-        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
-        if (off > high_off) {
-            high_off = off;
-        }
-    }
+    high_off = highest_offset(s);
 
     res->image_end_offset = high_off + s->cluster_size;
 
@@ -541,13 +553,6 @@ static int parallels_check_leak(BlockDriverState *bs,
         return 0;
     }
 
-    if (fix & BDRV_FIX_LEAKS) {
-        ret = parallels_fix_leak(bs, res);
-        if (ret < 0) {
-            return ret;
-        }
-    }
-
     count = DIV_ROUND_UP(leak_size, s->cluster_size);
     fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
             fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size);
@@ -560,6 +565,122 @@ static int parallels_check_leak(BlockDriverState *bs,
     return 0;
 }
 
+static int parallels_check_duplicate(BlockDriverState *bs,
+                                     BdrvCheckResult *res,
+                                     BdrvCheckMode *fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    QEMUIOVector qiov;
+    int64_t off, high_off, sector;
+    unsigned long *bitmap;
+    uint32_t i, bitmap_size, cluster_index;
+    int n, ret = 0;
+    uint64_t *buf = NULL;
+
+    high_off = highest_offset(s);
+    if (high_off == 0) {
+        return 0;
+    }
+
+    /*
+     * Create a bitmap of used clusters.
+     * 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.
+     *
+     * We shouldn't worry about newly allocated clusters outside the image
+     * because they are created higher then any existing cluster pointed by
+     * a BAT entry.
+     */
+    bitmap_size = host_cluster_index(s, high_off) + 1;
+    bitmap = bitmap_new(bitmap_size);
+
+    buf = qemu_memalign(4096, s->cluster_size);
+    qemu_iovec_init(&qiov, 0);
+    qemu_iovec_add(&qiov, buf, s->cluster_size);
+
+    for (i = 0; i < s->bat_size; i++) {
+        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+        if (off == 0) {
+            continue;
+        }
+
+        cluster_index = host_cluster_index(s, off);
+        if (test_bit(cluster_index, bitmap)) {
+            /* this cluster duplicates another one */
+            fprintf(stderr,
+                    "%s duplicate offset in BAT entry %u\n",
+                    *fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
+
+            res->corruptions++;
+
+            if (*fix & BDRV_FIX_ERRORS) {
+                /*
+                 * Reset the entry and allocate a new cluster
+                 * for the relevant guest offset. In this way we let
+                 * the lower layer to place the new cluster properly.
+                 * Copy the original cluster to the allocated one.
+                 */
+                parallels_set_bat_entry(s, i, 0);
+
+                ret = bdrv_co_pread(bs->file, off, s->cluster_size, buf, 0);
+                if (ret < 0) {
+                    res->check_errors++;
+                    goto out;
+                }
+
+                sector = (i * s->cluster_size) >> BDRV_SECTOR_BITS;
+                sector = allocate_clusters(bs, sector, s->tracks, &n);
+                if (sector < 0) {
+                    res->check_errors++;
+                    ret = sector;
+                    goto out;
+                }
+                off = sector << BDRV_SECTOR_BITS;
+                if (off > high_off) {
+                    high_off = off;
+                }
+
+                ret = bdrv_co_pwritev(bs->file, off, s->cluster_size, &qiov, 0);
+                if (ret < 0) {
+                    res->check_errors++;
+                    goto out;
+                }
+
+                /*
+                 * In the future allocate_cluster() will reuse holed offsets
+                 * inside the image. Keep the used clusters bitmap content
+                 * consistent for the new allocated clusters too.
+                 *
+                 * Note, clusters allocated outside the current image are not
+                 * considered, and the bitmap size doesn't change.
+                 */
+                cluster_index = host_cluster_index(s, off);
+                if (cluster_index < bitmap_size) {
+                    bitmap_set(bitmap, cluster_index, 1);
+                }
+
+                /*
+                 * When new clusters are allocated, file size increases by
+                 * 128 Mb blocks. We need to truncate the file to the right
+                 * size. Let the leak fix code make its job.
+                 */
+                *fix |= BDRV_FIX_LEAKS;
+                res->corruptions_fixed++;
+            }
+            res->image_end_offset = high_off + s->cluster_size;
+        } else {
+            bitmap_set(bitmap, cluster_index, 1);
+        }
+    }
+
+out:
+    qemu_iovec_destroy(&qiov);
+    g_free(buf);
+    g_free(bitmap);
+    return ret;
+}
+
 static void parallels_collect_statistics(BlockDriverState *bs,
                                          BdrvCheckResult *res,
                                          BdrvCheckMode fix)
@@ -608,7 +729,20 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
             return ret;
         }
 
+        ret = parallels_check_duplicate(bs, res, &fix);
+        if (ret < 0) {
+            return ret;
+        }
+
         parallels_collect_statistics(bs, res, fix);
+
+        if (fix & BDRV_FIX_LEAKS &&
+            (res->corruptions_fixed || res->leaks_fixed)) {
+            ret = parallels_fix_leak(bs, res);
+            if (ret < 0) {
+                return ret;
+            }
+        }
     }
 
     ret = bdrv_co_flush(bs);
-- 
2.34.1

[PATCH v2 4/5] parallels: Replace fprintf by qemu_log in check

[Alexander Ivanov]

If the check is called during normal work, tracking of the check must be
present in VM logs to have some clues if something going wrong with user's
data.

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

diff --git a/block/parallels.c b/block/parallels.c
index 73e992875a..5c9568f197 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -42,6 +42,7 @@
 #include "qemu/bswap.h"
 #include "qemu/bitmap.h"
 #include "qemu/memalign.h"
+#include "qemu/log-for-trace.h"
 #include "migration/blocker.h"
 #include "parallels.h"
 
@@ -448,8 +449,8 @@ static void parallels_check_unclean(BlockDriverState *bs,
         return;
     }
 
-    fprintf(stderr, "%s image was not closed correctly\n",
-            fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
+    qemu_log("%s image was not closed correctly\n",
+             fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
     res->corruptions++;
     if (fix & BDRV_FIX_ERRORS) {
         /* parallels_close will do the job right */
@@ -476,8 +477,8 @@ static int parallels_check_outside_image(BlockDriverState *bs,
     for (i = 0; i < s->bat_size; i++) {
         off = bat2sect(s, i) << BDRV_SECTOR_BITS;
         if (off >= size) {
-            fprintf(stderr, "%s cluster %u is outside image\n",
-                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
+            qemu_log("%s cluster %u is outside image\n",
+                     fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
             res->corruptions++;
             if (fix & BDRV_FIX_ERRORS) {
                 parallels_set_bat_entry(s, i, 0);
@@ -554,8 +555,8 @@ static int parallels_check_leak(BlockDriverState *bs,
     }
 
     count = DIV_ROUND_UP(leak_size, s->cluster_size);
-    fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
-            fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size);
+    qemu_log("%s space leaked at the end of the image %" PRId64 "\n",
+             fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size);
 
     res->leaks += count;
     if (fix & BDRV_FIX_LEAKS) {
@@ -608,9 +609,8 @@ static int parallels_check_duplicate(BlockDriverState *bs,
         cluster_index = host_cluster_index(s, off);
         if (test_bit(cluster_index, bitmap)) {
             /* this cluster duplicates another one */
-            fprintf(stderr,
-                    "%s duplicate offset in BAT entry %u\n",
-                    *fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
+            qemu_log("%s duplicate offset in BAT entry %u\n",
+                     *fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
 
             res->corruptions++;
 
-- 
2.34.1

[PATCH v2 5/5] parallels: Image repairing in parallels_open()

[Alexander Ivanov]

Repair an image at opening if the image is unclean or out-of-image
corruption was detected.

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

diff --git a/block/parallels.c b/block/parallels.c
index 5c9568f197..74f6d00ffb 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -753,7 +753,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
     return ret;
 }
 
-
 static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
                                             Error **errp)
 {
@@ -965,8 +964,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
 {
     BDRVParallelsState *s = bs->opaque;
     ParallelsHeader ph;
-    int ret, size, i;
-    int64_t file_size;
+    int ret, size;
+    int64_t file_size, high_off;
     QemuOpts *opts = NULL;
     Error *local_err = NULL;
     char *buf;
@@ -1044,34 +1043,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     }
     s->bat_bitmap = (uint32_t *)(s->header + 1);
 
-    for (i = 0; i < s->bat_size; i++) {
-        int64_t off = bat2sect(s, i);
-        if (off >= file_size) {
-            if (flags & BDRV_O_CHECK) {
-                continue;
-            }
-            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
-                       "is larger than file size (%" PRIi64 ")",
-                       off, i, file_size);
-            ret = -EINVAL;
-            goto fail;
-        }
-        if (off >= s->data_end) {
-            s->data_end = off + s->tracks;
-        }
-    }
-
-    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
-        /* Image was not closed correctly. The check is mandatory */
-        s->header_unclean = true;
-        if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_CHECK)) {
-            error_setg(errp, "parallels: Image was not closed correctly; "
-                       "cannot be opened read/write");
-            ret = -EACCES;
-            goto fail;
-        }
-    }
-
     opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
     if (!opts) {
         goto fail_options;
@@ -1133,7 +1104,41 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         error_free(s->migration_blocker);
         goto fail;
     }
+
     qemu_co_mutex_init(&s->lock);
+
+    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
+        s->header_unclean = true;
+    }
+
+    high_off = highest_offset(s) >> BDRV_SECTOR_BITS;
+    if (high_off >= s->data_end) {
+        s->data_end = high_off + s->tracks;
+    }
+
+    /*
+     * We don't repair the image here if it is opened for checks and
+     * shouldn't change the image if BDRV_O_RDWR or BDRV_O_INACTIVE
+     * flag is present.
+     */
+    if ((flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) || !(flags & BDRV_O_RDWR)) {
+        return 0;
+    }
+
+    /*
+     * Repair the image if it's dirty or
+     * out-of-image corruption was detected.
+     */
+    if (s->data_end > file_size || s->header_unclean) {
+        BdrvCheckResult res;
+        ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret,
+                             "Could not repair corrupted image");
+            goto fail;
+        }
+    }
+
     return 0;
 
 fail_format:
-- 
2.34.1

Re: [PATCH v2 0/5] parallels: Add duplication check, repair at open, fix bugs

[Alexander Ivanov]

This patchset should be applied on the top of *[PATCH v8 00/11] 
parallels: Refactor the code of images checks and fix a bug*

On 12.01.2023 16:01, Alexander Ivanov wrote:
> Fix incorrect data end calculation in parallels_open().
>
> Split image leak handling to separate check and fix helpers.
>
> Add checking and repairing duplicate offsets in BAT
>
> Replace fprintf() by qemu_log().
>
> Image repairing in parallels_open().
>
> v2:
> 2: Moved outsude parallels_check_leak() 2 helpers:
>     parallels_get_leak_size() and parallels_fix_leak().
>     
> 3: Used highest_offset() helper in parallels_check_leak(). Fixed a typo.
>     Added comments. Replaced g_malloc() call by qemu_memalign(). Replaced
>     bdrv_pread() call by bdrv_co_pread(). Got rid of keeping bytes and
>     sectors in the same variable. Added setting the bitmap of the used
>     clusters for a new allocated cluster if it isn't out of the bitmap.
>     Moved the leak fix to the end of all the checks. Removed a dependence
>     on image format for the duplicate check.
>     
> 4 (old): Merged this patch to the previous.
> 4 (former 5): Fixed formatting.
> 5 (former 6): Fixed comments. Added O_INACTIVE check in the condition.
>                Replaced inuse detection by header_unclean checking.
>                Replaced playing with corutines by bdrv_check() usage.
>
> Alexander Ivanov (5):
>    parallels: Incorrect data end calculation in parallels_open()
>    parallels: Split image leak handling to separate check and fix helpers
>    parallels: Add checking and repairing duplicate offsets in BAT
>    parallels: Replace fprintf by qemu_log in check
>    parallels: Image repairing in parallels_open()
>
>   block/parallels.c | 321 +++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 247 insertions(+), 74 deletions(-)
>

Re: [PATCH v2 2/5] parallels: Split image leak handling to separate check and fix helpers

[Denis V. Lunev]

On 1/12/23 16:01, Alexander Ivanov wrote:
> We need to fix leak after deduplication in the next patch. Move leak
> fixing to a separate helper parallels_fix_leak() and add
> parallels_get_leak_size() helper wich used in parallels_fix_leak() and
> parallels_check_leak().
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 88 ++++++++++++++++++++++++++++++++---------------
>   1 file changed, 61 insertions(+), 27 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index ed2cf27abc..da1e75096c 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -475,21 +475,53 @@ static int parallels_check_outside_image(BlockDriverState *bs,
>       return 0;
>   }
>   
> +static int64_t parallels_get_leak_size(BlockDriverState *bs,
> +                                       BdrvCheckResult *res)
> +{
> +    int64_t size;
> +    size = bdrv_getlength(bs->file->bs);
> +    /*
> +     * Before any usage of this function out-of-image corruption has been
> +     * fixed. If the function returns a negative value, it means an error.
> +     */
> +    return (size < 0) ? size : (size - res->image_end_offset);
I'd better use normal 'if' here and will add an assert that
'size' >= 'res->image_end_offset' on success path.

> +}
> +
> +static int parallels_fix_leak(BlockDriverState *bs,
> +                              BdrvCheckResult *res)
> +{
> +    Error *local_err = NULL;
> +    int64_t size;
> +    int ret;
> +
> +    size = parallels_get_leak_size(bs, res);
> +    if (size <= 0) {
> +        return size;
> +    }
> +
> +    /*
> +     * 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);
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> +
>   static int parallels_check_leak(BlockDriverState *bs,
>                                   BdrvCheckResult *res,
>                                   BdrvCheckMode fix)
>   {
>       BDRVParallelsState *s = bs->opaque;
> -    int64_t size, off, high_off, count;
> +    int64_t off, high_off, count, leak_size;
>       uint32_t i;
>       int ret;
>   
> -    size = bdrv_getlength(bs->file->bs);
> -    if (size < 0) {
> -        res->check_errors++;
> -        return size;
> -    }
> -
>       high_off = 0;
>       for (i = 0; i < s->bat_size; i++) {
>           off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> @@ -499,30 +531,32 @@ static int parallels_check_leak(BlockDriverState *bs,
>       }
>   
>       res->image_end_offset = high_off + s->cluster_size;
> -    if (size > res->image_end_offset) {
> -        count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
> -        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;
> -            }
> -            res->leaks_fixed += count;
> +    leak_size = parallels_get_leak_size(bs, res);
> +    if (leak_size < 0) {
> +        res->check_errors++;
> +        return leak_size;
> +    }
> +    if (leak_size == 0) {
> +        return 0;
> +    }
> +
> +    if (fix & BDRV_FIX_LEAKS) {
> +        ret = parallels_fix_leak(bs, res);

you have changed semantics a bit - with a error here the message
that we do get leak will be lost. I think the code is to be restructured
a bit here:
* move printing above this if
* fill leaks_fixed inside this if on the success path
This will be much straight forward

Den

> +        if (ret < 0) {
> +            return ret;
>           }
>       }
>   
> +    count = DIV_ROUND_UP(leak_size, s->cluster_size);
> +    fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
> +            fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size);
> +
> +    res->leaks += count;
> +    if (fix & BDRV_FIX_LEAKS) {
> +        res->leaks_fixed += count;
> +    }
> +
>       return 0;
>   }
>   

Re: [PATCH v2 3/5] parallels: Add checking and repairing duplicate offsets in BAT

[Denis V. Lunev]

On 1/12/23 16:01, Alexander Ivanov wrote:
> Cluster offsets must be unique among all the BAT entries. Find duplicate
> offsets in the BAT and fix it by copying the content of the relevant
> cluster to a newly allocated cluster and set the new cluster offset to the
> duplicated entry.
>
> Add host_cluster_index() and highest_offset() helpers to deduplicate the
> code.
>
> Move parallels_fix_leak() call to parallels_co_check() to fix both types
> of leak: real corruption and a leak produced by allocate_clusters()
> during deduplication.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 168 +++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 151 insertions(+), 17 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index da1e75096c..73e992875a 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -136,6 +136,26 @@ static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
>       return MIN(nb_sectors, ret);
>   }
>   
> +static uint32_t host_cluster_index(BDRVParallelsState *s, int64_t off)
> +{
> +    off -= s->header->data_off << BDRV_SECTOR_BITS;
> +    return off / s->cluster_size;
> +}
> +
> +static int64_t highest_offset(BDRVParallelsState *s)
> +{
> +    int64_t off, high_off = 0;
> +    int i;
> +
> +    for (i = 0; i < s->bat_size; i++) {
> +        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> +        if (off > high_off) {
> +            high_off = off;
> +        }
> +    }
> +    return high_off;
> +}
> +
>   static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
>                               int nb_sectors, int *pnum)
>   {
> @@ -518,17 +538,9 @@ static int parallels_check_leak(BlockDriverState *bs,
>                                   BdrvCheckMode fix)
>   {
>       BDRVParallelsState *s = bs->opaque;
> -    int64_t off, high_off, count, leak_size;
> -    uint32_t i;
> -    int ret;
> +    int64_t high_off, count, leak_size;
>   
> -    high_off = 0;
> -    for (i = 0; i < s->bat_size; i++) {
> -        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> -        if (off > high_off) {
> -            high_off = off;
> -        }
> -    }
> +    high_off = highest_offset(s);
>   
>       res->image_end_offset = high_off + s->cluster_size;
>   
> @@ -541,13 +553,6 @@ static int parallels_check_leak(BlockDriverState *bs,
>           return 0;
>       }
>   
> -    if (fix & BDRV_FIX_LEAKS) {
> -        ret = parallels_fix_leak(bs, res);
> -        if (ret < 0) {
> -            return ret;
> -        }
> -    }
> -
>       count = DIV_ROUND_UP(leak_size, s->cluster_size);
>       fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
>               fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size);
> @@ -560,6 +565,122 @@ static int parallels_check_leak(BlockDriverState *bs,
>       return 0;
>   }
>   
> +static int parallels_check_duplicate(BlockDriverState *bs,
> +                                     BdrvCheckResult *res,
> +                                     BdrvCheckMode *fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    QEMUIOVector qiov;
> +    int64_t off, high_off, sector;
> +    unsigned long *bitmap;
> +    uint32_t i, bitmap_size, cluster_index;
> +    int n, ret = 0;
> +    uint64_t *buf = NULL;
> +
> +    high_off = highest_offset(s);
> +    if (high_off == 0) {
> +        return 0;
> +    }
> +
> +    /*
> +     * Create a bitmap of used clusters.
> +     * 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.
> +     *
> +     * We shouldn't worry about newly allocated clusters outside the image
> +     * because they are created higher then any existing cluster pointed by
> +     * a BAT entry.
> +     */
> +    bitmap_size = host_cluster_index(s, high_off) + 1;
> +    bitmap = bitmap_new(bitmap_size);
> +
> +    buf = qemu_memalign(4096, s->cluster_size);
> +    qemu_iovec_init(&qiov, 0);
> +    qemu_iovec_add(&qiov, buf, s->cluster_size);
> +
> +    for (i = 0; i < s->bat_size; i++) {
> +        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> +        if (off == 0) {
> +            continue;
> +        }
> +
> +        cluster_index = host_cluster_index(s, off);
> +        if (test_bit(cluster_index, bitmap)) {
> +            /* this cluster duplicates another one */
> +            fprintf(stderr,
> +                    "%s duplicate offset in BAT entry %u\n",
> +                    *fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
> +
> +            res->corruptions++;
> +
> +            if (*fix & BDRV_FIX_ERRORS) {
> +                /*
> +                 * Reset the entry and allocate a new cluster
> +                 * for the relevant guest offset. In this way we let
> +                 * the lower layer to place the new cluster properly.
> +                 * Copy the original cluster to the allocated one.
> +                 */
> +                parallels_set_bat_entry(s, i, 0);
> +
> +                ret = bdrv_co_pread(bs->file, off, s->cluster_size, buf, 0);
> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    goto out;
> +                }
> +
> +                sector = (i * s->cluster_size) >> BDRV_SECTOR_BITS;
> +                sector = allocate_clusters(bs, sector, s->tracks, &n);
> +                if (sector < 0) {
> +                    res->check_errors++;
> +                    ret = sector;
> +                    goto out;
> +                }
> +                off = sector << BDRV_SECTOR_BITS;
> +                if (off > high_off) {
> +                    high_off = off;
> +                }
> +
> +                ret = bdrv_co_pwritev(bs->file, off, s->cluster_size, &qiov, 0);
> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    goto out;
> +                }
> +
> +                /*
> +                 * In the future allocate_cluster() will reuse holed offsets
> +                 * inside the image. Keep the used clusters bitmap content
> +                 * consistent for the new allocated clusters too.
> +                 *
> +                 * Note, clusters allocated outside the current image are not
> +                 * considered, and the bitmap size doesn't change.
> +                 */
> +                cluster_index = host_cluster_index(s, off);
> +                if (cluster_index < bitmap_size) {
> +                    bitmap_set(bitmap, cluster_index, 1);
> +                }
> +
> +                /*
> +                 * When new clusters are allocated, file size increases by
> +                 * 128 Mb blocks. We need to truncate the file to the right
> +                 * size. Let the leak fix code make its job.
> +                 */
> +                *fix |= BDRV_FIX_LEAKS;
> +                res->corruptions_fixed++;
> +            }
> +            res->image_end_offset = high_off + s->cluster_size;
> +        } else {
> +            bitmap_set(bitmap, cluster_index, 1);
> +        }
> +    }
> +
> +out:
> +    qemu_iovec_destroy(&qiov);
> +    g_free(buf);
> +    g_free(bitmap);
> +    return ret;
> +}
> +
>   static void parallels_collect_statistics(BlockDriverState *bs,
>                                            BdrvCheckResult *res,
>                                            BdrvCheckMode fix)
> @@ -608,7 +729,20 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>               return ret;
>           }
>   
> +        ret = parallels_check_duplicate(bs, res, &fix);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +
>           parallels_collect_statistics(bs, res, fix);
> +
> +        if (fix & BDRV_FIX_LEAKS &&
> +            (res->corruptions_fixed || res->leaks_fixed)) {
> +            ret = parallels_fix_leak(bs, res);
> +            if (ret < 0) {
> +                return ret;
> +            }
> +        }
>       }
>   
>       ret = bdrv_co_flush(bs);
I would be more happy if this patch will be split - helpers creation
is better to be separated from functional changes.

Den

Re: [PATCH v2 4/5] parallels: Replace fprintf by qemu_log in check

[Denis V. Lunev]

On 1/12/23 16:01, Alexander Ivanov wrote:
> If the check is called during normal work, tracking of the check must be
> present in VM logs to have some clues if something going wrong with user's
> data.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 73e992875a..5c9568f197 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -42,6 +42,7 @@
>   #include "qemu/bswap.h"
>   #include "qemu/bitmap.h"
>   #include "qemu/memalign.h"
> +#include "qemu/log-for-trace.h"
>   #include "migration/blocker.h"
>   #include "parallels.h"
>   
> @@ -448,8 +449,8 @@ static void parallels_check_unclean(BlockDriverState *bs,
>           return;
>       }
>   
> -    fprintf(stderr, "%s image was not closed correctly\n",
> -            fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
> +    qemu_log("%s image was not closed correctly\n",
> +             fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
>       res->corruptions++;
>       if (fix & BDRV_FIX_ERRORS) {
>           /* parallels_close will do the job right */
> @@ -476,8 +477,8 @@ static int parallels_check_outside_image(BlockDriverState *bs,
>       for (i = 0; i < s->bat_size; i++) {
>           off = bat2sect(s, i) << BDRV_SECTOR_BITS;
>           if (off >= size) {
> -            fprintf(stderr, "%s cluster %u is outside image\n",
> -                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
> +            qemu_log("%s cluster %u is outside image\n",
> +                     fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
>               res->corruptions++;
>               if (fix & BDRV_FIX_ERRORS) {
>                   parallels_set_bat_entry(s, i, 0);
> @@ -554,8 +555,8 @@ static int parallels_check_leak(BlockDriverState *bs,
>       }
>   
>       count = DIV_ROUND_UP(leak_size, s->cluster_size);
> -    fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
> -            fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size);
> +    qemu_log("%s space leaked at the end of the image %" PRId64 "\n",
> +             fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size);
>   
>       res->leaks += count;
>       if (fix & BDRV_FIX_LEAKS) {
> @@ -608,9 +609,8 @@ static int parallels_check_duplicate(BlockDriverState *bs,
>           cluster_index = host_cluster_index(s, off);
>           if (test_bit(cluster_index, bitmap)) {
>               /* this cluster duplicates another one */
> -            fprintf(stderr,
> -                    "%s duplicate offset in BAT entry %u\n",
> -                    *fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
> +            qemu_log("%s duplicate offset in BAT entry %u\n",
> +                     *fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
>   
>               res->corruptions++;
>   
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH v2 5/5] parallels: Image repairing in parallels_open()

[Denis V. Lunev]

On 1/12/23 16:01, Alexander Ivanov wrote:
> Repair an image at opening if the image is unclean or out-of-image
> corruption was detected.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 67 +++++++++++++++++++++++++----------------------
>   1 file changed, 36 insertions(+), 31 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 5c9568f197..74f6d00ffb 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -753,7 +753,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>       return ret;
>   }
>   
> -
>   static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
>                                               Error **errp)
>   {
> @@ -965,8 +964,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>   {
>       BDRVParallelsState *s = bs->opaque;
>       ParallelsHeader ph;
> -    int ret, size, i;
> -    int64_t file_size;
> +    int ret, size;
> +    int64_t file_size, high_off;
>       QemuOpts *opts = NULL;
>       Error *local_err = NULL;
>       char *buf;
> @@ -1044,34 +1043,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>       }
>       s->bat_bitmap = (uint32_t *)(s->header + 1);
>   
> -    for (i = 0; i < s->bat_size; i++) {
> -        int64_t off = bat2sect(s, i);
> -        if (off >= file_size) {
> -            if (flags & BDRV_O_CHECK) {
> -                continue;
> -            }
> -            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
> -                       "is larger than file size (%" PRIi64 ")",
> -                       off, i, file_size);
> -            ret = -EINVAL;
> -            goto fail;
> -        }
> -        if (off >= s->data_end) {
> -            s->data_end = off + s->tracks;
> -        }
> -    }
> -
> -    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> -        /* Image was not closed correctly. The check is mandatory */
> -        s->header_unclean = true;
> -        if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_CHECK)) {
> -            error_setg(errp, "parallels: Image was not closed correctly; "
> -                       "cannot be opened read/write");
> -            ret = -EACCES;
> -            goto fail;
> -        }
> -    }
> -
>       opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
>       if (!opts) {
>           goto fail_options;
> @@ -1133,7 +1104,41 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>           error_free(s->migration_blocker);
>           goto fail;
>       }
> +
>       qemu_co_mutex_init(&s->lock);
> +
> +    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> +        s->header_unclean = true;
> +    }
> +
> +    high_off = highest_offset(s) >> BDRV_SECTOR_BITS;
> +    if (high_off >= s->data_end) {
high_off + s->cluster_size I believe in the logic of the previous 
patchset :)
may be should be separated.

> +        s->data_end = high_off + s->tracks;
> +    }
> +
> +    /*
> +     * We don't repair the image here if it is opened for checks and
> +     * shouldn't change the image if BDRV_O_RDWR or BDRV_O_INACTIVE
> +     * flag is present.
> +     */
> +    if ((flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) || !(flags & BDRV_O_RDWR)) {
> +        return 0;
> +    }
> +
> +    /*
> +     * Repair the image if it's dirty or
> +     * out-of-image corruption was detected.
> +     */
> +    if (s->data_end > file_size || s->header_unclean) {
> +        BdrvCheckResult res;
> +        ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret,
> +                             "Could not repair corrupted image");
> +            goto fail;
> +        }
> +    }
> +
>       return 0;
>   
>   fail_format:

Re: [PATCH v2 5/5] parallels: Image repairing in parallels_open()

[Denis V. Lunev]

On 1/12/23 16:01, Alexander Ivanov wrote:
> Repair an image at opening if the image is unclean or out-of-image
> corruption was detected.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 67 +++++++++++++++++++++++++----------------------
>   1 file changed, 36 insertions(+), 31 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 5c9568f197..74f6d00ffb 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -753,7 +753,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>       return ret;
>   }
>   
> -
>   static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
>                                               Error **errp)
>   {
> @@ -965,8 +964,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>   {
>       BDRVParallelsState *s = bs->opaque;
>       ParallelsHeader ph;
> -    int ret, size, i;
> -    int64_t file_size;
> +    int ret, size;
> +    int64_t file_size, high_off;
>       QemuOpts *opts = NULL;
>       Error *local_err = NULL;
>       char *buf;
> @@ -1044,34 +1043,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>       }
>       s->bat_bitmap = (uint32_t *)(s->header + 1);
>   
> -    for (i = 0; i < s->bat_size; i++) {
> -        int64_t off = bat2sect(s, i);
> -        if (off >= file_size) {
> -            if (flags & BDRV_O_CHECK) {
> -                continue;
> -            }
> -            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
> -                       "is larger than file size (%" PRIi64 ")",
> -                       off, i, file_size);
> -            ret = -EINVAL;
> -            goto fail;
> -        }
> -        if (off >= s->data_end) {
> -            s->data_end = off + s->tracks;
> -        }
> -    }
> -
> -    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> -        /* Image was not closed correctly. The check is mandatory */
> -        s->header_unclean = true;
> -        if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_CHECK)) {
> -            error_setg(errp, "parallels: Image was not closed correctly; "
> -                       "cannot be opened read/write");
> -            ret = -EACCES;
> -            goto fail;
> -        }
> -    }
> -
>       opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
>       if (!opts) {
>           goto fail_options;
> @@ -1133,7 +1104,41 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>           error_free(s->migration_blocker);
>           goto fail;
>       }
> +
>       qemu_co_mutex_init(&s->lock);
> +
> +    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> +        s->header_unclean = true;
> +    }
> +
> +    high_off = highest_offset(s) >> BDRV_SECTOR_BITS;
> +    if (high_off >= s->data_end) {
> +        s->data_end = high_off + s->tracks;
> +    }
> +
> +    /*
> +     * We don't repair the image here if it is opened for checks and
> +     * shouldn't change the image if BDRV_O_RDWR or BDRV_O_INACTIVE
> +     * flag is present.
> +     */
> +    if ((flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) || !(flags & BDRV_O_RDWR)) {
> +        return 0;
> +    }
> +
> +    /*
> +     * Repair the image if it's dirty or
> +     * out-of-image corruption was detected.
> +     */
> +    if (s->data_end > file_size || s->header_unclean) {
> +        BdrvCheckResult res;
> +        ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret,
> +                             "Could not repair corrupted image");
> +            goto fail;
This leaks s->migration_blocker. Please see error path above, i.e.

     /* Disable migration until bdrv_activate method is added */
     error_setg(&s->migration_blocker, "The Parallels format used by 
node '%s' "
                "does not support live migration",
                bdrv_get_device_or_node_name(bs));
     ret = migrate_add_blocker(s->migration_blocker, errp);
     if (ret < 0) {
         error_free(s->migration_blocker); <--------------------
         goto fail;
     }

Thanks a lot for Mike Maslenkin for the note.

Den


> +        }
> +    }
> +
>       return 0;
>   
>   fail_format: