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

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

[Alexander Ivanov]

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

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().

v5:
3: Fixed a byteorder bug, fixed zero-length image handling and fixed uint32
   truncation.

v4:
2,5: Rebased.

v3:
2: Added (size >= res->image_end_offset) assert and changed the comment in
   parallels_get_leak_size(). Changed error printing and leaks fixing order.
3: Removed highest_offset() helper, instead image_end_offset field is used.
5: Moved highest_offset() code to parallels_open() - now it is used only in
   this function. Fixed data_end update condition. Fixed a leak of
   s->migration_blocker.

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 | 295 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 232 insertions(+), 63 deletions(-)

-- 
2.34.1

[PATCH v5 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 7c263d5085..1ec98c722b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -861,9 +861,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 v5 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 | 86 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 61 insertions(+), 25 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 1ec98c722b..64850b9655 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -482,43 +482,79 @@ parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res,
     return 0;
 }
 
+static int64_t parallels_get_leak_size(BlockDriverState *bs,
+                                       BdrvCheckResult *res)
+{
+    int64_t size;
+
+    size = bdrv_getlength(bs->file->bs);
+    if (size < 0) {
+        return size;
+    }
+
+    /*
+     * Before any usage of this function, image_end_offset has to be set to the
+     * the highest offset in the BAT, excluding out-of-image offsets.
+     */
+    assert(size >= res->image_end_offset);
+
+    return 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 coroutine_fn GRAPH_RDLOCK
 parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
                      BdrvCheckMode fix)
 {
     BDRVParallelsState *s = bs->opaque;
-    int64_t size;
+    int64_t count, leak_size;
     int ret;
 
-    size = bdrv_getlength(bs->file->bs);
-    if (size < 0) {
+    leak_size = parallels_get_leak_size(bs, res);
+    if (leak_size < 0) {
         res->check_errors++;
-        return size;
+        return leak_size;
+    }
+    if (leak_size == 0) {
+        return 0;
     }
 
-    if (size > res->image_end_offset) {
-        int64_t count;
-        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;
+    count = DIV_ROUND_UP(leak_size, s->cluster_size);
+    res->leaks += count;
+    fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
+            fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_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);
-                res->check_errors++;
-                return ret;
-            }
-            res->leaks_fixed += count;
+    if (fix & BDRV_FIX_LEAKS) {
+        ret = parallels_fix_leak(bs, res);
+        if (ret < 0) {
+            return ret;
         }
+        res->leaks_fixed += count;
     }
 
     return 0;
-- 
2.34.1

[PATCH v5 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() helper 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 | 138 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 133 insertions(+), 5 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 64850b9655..9fa1f93973 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -136,6 +136,12 @@ 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 -= le32_to_cpu(s->header->data_off) << BDRV_SECTOR_BITS;
+    return off / s->cluster_size;
+}
+
 static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
                             int nb_sectors, int *pnum)
 {
@@ -533,7 +539,6 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
 {
     BDRVParallelsState *s = bs->opaque;
     int64_t count, leak_size;
-    int ret;
 
     leak_size = parallels_get_leak_size(bs, res);
     if (leak_size < 0) {
@@ -550,16 +555,127 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
             fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size);
 
     if (fix & BDRV_FIX_LEAKS) {
-        ret = parallels_fix_leak(bs, res);
-        if (ret < 0) {
-            return ret;
-        }
         res->leaks_fixed += count;
     }
 
     return 0;
 }
 
+static int parallels_check_duplicate(BlockDriverState *bs,
+                                     BdrvCheckResult *res,
+                                     BdrvCheckMode *fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    QEMUIOVector qiov;
+    int64_t off, sector;
+    unsigned long *bitmap;
+    uint32_t i, bitmap_size, cluster_index;
+    int n, ret = 0;
+    uint64_t *buf = NULL;
+
+    /*
+     * 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, res->image_end_offset);
+    if (bitmap_size == 0) {
+        return 0;
+    }
+
+    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 * (int64_t)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;
+
+                ret = bdrv_co_pwritev(bs->file, off, s->cluster_size, &qiov, 0);
+                if (ret < 0) {
+                    res->check_errors++;
+                    goto out;
+                }
+
+                if (off + s->cluster_size > res->image_end_offset) {
+                    res->image_end_offset = off + s->cluster_size;
+                }
+
+                /*
+                 * 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++;
+            }
+        } 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)
@@ -611,7 +727,19 @@ parallels_co_check(BlockDriverState *bs, BdrvCheckResult *res,
             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) {
+            ret = parallels_fix_leak(bs, res);
+            if (ret < 0) {
+                return ret;
+            }
+        }
     }
 
     ret = bdrv_co_flush(bs);
-- 
2.34.1

[PATCH v5 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>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 block/parallels.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 9fa1f93973..d64e8007d5 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"
 
@@ -436,8 +437,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 */
@@ -464,8 +465,8 @@ parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res,
     for (i = 0; i < s->bat_size; i++) {
         off = bat2sect(s, i) << BDRV_SECTOR_BITS;
         if (off + s->cluster_size > 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);
@@ -551,8 +552,8 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
 
     count = DIV_ROUND_UP(leak_size, s->cluster_size);
     res->leaks += count;
-    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);
 
     if (fix & BDRV_FIX_LEAKS) {
         res->leaks_fixed += count;
@@ -603,9 +604,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 v5 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 | 65 +++++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 30 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index d64e8007d5..7bbd5cb112 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -962,7 +962,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVParallelsState *s = bs->opaque;
     ParallelsHeader ph;
     int ret, size, i;
-    int64_t file_nb_sectors;
+    int64_t file_nb_sectors, sector;
     QemuOpts *opts = NULL;
     Error *local_err = NULL;
     char *buf;
@@ -1039,35 +1039,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_nb_sectors) {
-            if (flags & BDRV_O_CHECK) {
-                continue;
-            }
-            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
-                       "is larger than file size (%" PRIi64 ")",
-                       off << BDRV_SECTOR_BITS, i,
-                       file_nb_sectors << BDRV_SECTOR_BITS);
-            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;
@@ -1130,6 +1101,40 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
     qemu_co_mutex_init(&s->lock);
+
+    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
+        s->header_unclean = true;
+    }
+
+    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;
+        }
+    }
+
+    /*
+     * We don't repair the image here if it's opened for checks. Also we don't
+     * want to change inactive images and can't change readonly images.
+     */
+    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_nb_sectors || s->header_unclean) {
+        BdrvCheckResult res;
+        ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
+        if (ret < 0) {
+            error_free(s->migration_blocker);
+            error_setg_errno(errp, -ret, "Could not repair corrupted image");
+            goto fail;
+        }
+    }
+
     return 0;
 
 fail_format:
-- 
2.34.1

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

[Hanna Czenczek]

On 29.05.23 17:15, 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 | 86 +++++++++++++++++++++++++++++++++--------------
>   1 file changed, 61 insertions(+), 25 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 1ec98c722b..64850b9655 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -482,43 +482,79 @@ parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res,
>       return 0;
>   }
>   
> +static int64_t parallels_get_leak_size(BlockDriverState *bs,
> +                                       BdrvCheckResult *res)
> +{
> +    int64_t size;
> +
> +    size = bdrv_getlength(bs->file->bs);
> +    if (size < 0) {
> +        return size;
> +    }
> +
> +    /*
> +     * Before any usage of this function, image_end_offset has to be set to the
> +     * the highest offset in the BAT, excluding out-of-image offsets.
> +     */
> +    assert(size >= res->image_end_offset);

If `high_off == 0` in parallels_check_outside_image(), it will use 
s->data_end to determine image_end_offset, which is originally read from 
the image header.  I don’t see any place where we ensure that 
`s->data_end <= bdrv_getlength(bs->file->bs)`, so can we be certain the 
assertion holds even in that case?

> +
> +    return 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 coroutine_fn GRAPH_RDLOCK
>   parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
>                        BdrvCheckMode fix)
>   {
>       BDRVParallelsState *s = bs->opaque;
> -    int64_t size;
> +    int64_t count, leak_size;
>       int ret;
>   
> -    size = bdrv_getlength(bs->file->bs);
> -    if (size < 0) {
> +    leak_size = parallels_get_leak_size(bs, res);
> +    if (leak_size < 0) {
>           res->check_errors++;
> -        return size;
> +        return leak_size;
> +    }
> +    if (leak_size == 0) {
> +        return 0;
>       }
>   
> -    if (size > res->image_end_offset) {
> -        int64_t count;
> -        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;
> +    count = DIV_ROUND_UP(leak_size, s->cluster_size);
> +    res->leaks += count;
> +    fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
> +            fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_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);
> -                res->check_errors++;
> -                return ret;
> -            }
> -            res->leaks_fixed += count;
> +    if (fix & BDRV_FIX_LEAKS) {
> +        ret = parallels_fix_leak(bs, res);
> +        if (ret < 0) {
> +            return ret;

We used to increment res->check_errors here – should we still do that?

Hanna

>           }
> +        res->leaks_fixed += count;
>       }
>   
>       return 0;

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

[Hanna Czenczek]

On 29.05.23 17:15, 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() helper 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.

I’m not really a fan of splitting parallels_fix_leak() in this way. One 
problem is that parallels_check_leak() still increments leaks_fixed, 
even though it cannot know whether that will succeed. Would it be a 
problem to move parallels_check_leak() after parallels_check_duplicate()?

> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 138 ++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 133 insertions(+), 5 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 64850b9655..9fa1f93973 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -136,6 +136,12 @@ 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 -= le32_to_cpu(s->header->data_off) << BDRV_SECTOR_BITS;
> +    return off / s->cluster_size;
> +}
> +
>   static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
>                               int nb_sectors, int *pnum)
>   {
> @@ -533,7 +539,6 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
>   {
>       BDRVParallelsState *s = bs->opaque;
>       int64_t count, leak_size;
> -    int ret;
>   
>       leak_size = parallels_get_leak_size(bs, res);
>       if (leak_size < 0) {
> @@ -550,16 +555,127 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
>               fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size);
>   
>       if (fix & BDRV_FIX_LEAKS) {
> -        ret = parallels_fix_leak(bs, res);
> -        if (ret < 0) {
> -            return ret;
> -        }
>           res->leaks_fixed += count;
>       }
>   
>       return 0;
>   }
>   
> +static int parallels_check_duplicate(BlockDriverState *bs,
> +                                     BdrvCheckResult *res,
> +                                     BdrvCheckMode *fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    QEMUIOVector qiov;
> +    int64_t off, sector;
> +    unsigned long *bitmap;
> +    uint32_t i, bitmap_size, cluster_index;
> +    int n, ret = 0;
> +    uint64_t *buf = NULL;
> +
> +    /*
> +     * 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, res->image_end_offset);
> +    if (bitmap_size == 0) {
> +        return 0;
> +    }
> +
> +    bitmap = bitmap_new(bitmap_size);
> +
> +    buf = qemu_memalign(4096, s->cluster_size);

There is qemu_blockalign(), which actually uses the BDS’s memory 
alignment, so should be a better fit.

> +    qemu_iovec_init(&qiov, 0);
> +    qemu_iovec_add(&qiov, buf, s->cluster_size);

I don’t think this is necessary, there is bdrv_co_pwrite() that takes a 
buffer.  OTOH, if you want to keep this, you could replace the 
bdrv_co_pread() call by bdrv_co_preadv().

> +
> +    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)) {

I understand we’ve already ensured that image_end_offset (which 
determines the bitmap size) is large enough, and so this can’t overflow, 
but I could sleep better if there was an `assert(cluster_index < 
bitmap_size);` before this `test_bit()`.

> +            /* 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);

As far as I understand, this will modify the image content when read.  
Can we perhaps revert this change if there’s an error in bdrv_co_pread() 
or allocate_clusters()?  I understand that a double allocation is a bad 
corruption, but if we can’t fix it because of some unexpected error, I 
feel like it’s better to still keep the image in the same state as 
before rather than having effectively destroyed the data in the 
respective cluster, so users can at least try to fix it by copying it.

> +
> +                ret = bdrv_co_pread(bs->file, off, s->cluster_size, buf, 0);
> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    goto out;
> +                }
> +
> +                sector = (i * (int64_t)s->cluster_size) >> BDRV_SECTOR_BITS;

Both for my own understanding and to maybe suggest a simplification: 
This is the same as `sector = i * (int64_t)s->tracks`, right?

Hanna

> +                sector = allocate_clusters(bs, sector, s->tracks, &n);
> +                if (sector < 0) {
> +                    res->check_errors++;
> +                    ret = sector;
> +                    goto out;
> +                }

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

[Hanna Czenczek]

On 29.05.23 17:14, Alexander Ivanov wrote:
> 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(-)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

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

[Hanna Czenczek]

On 29.05.23 17:15, 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.

I understand stderr counts as part of the VM log, doesn’t it?  I thought 
stderr is generally logged, and naïvely, it seems like the better fit to 
me, because it conveys more urgency than the standard log (which, 
judging from its callers, looks mostly like a debug log).

Hanna

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

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

[Hanna Czenczek]

On 29.05.23 17:15, 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 | 65 +++++++++++++++++++++++++----------------------
>   1 file changed, 35 insertions(+), 30 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index d64e8007d5..7bbd5cb112 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c

[...]

> @@ -1130,6 +1101,40 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>           goto fail;
>       }
>       qemu_co_mutex_init(&s->lock);
> +
> +    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> +        s->header_unclean = true;
> +    }
> +
> +    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;
> +        }
> +    }
> +
> +    /*
> +     * We don't repair the image here if it's opened for checks. Also we don't
> +     * want to change inactive images and can't change readonly images.
> +     */
> +    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_nb_sectors || s->header_unclean) {
> +        BdrvCheckResult res;
> +        ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
> +        if (ret < 0) {

Should we also verify that res->corruptions == res->corruptions_fixed && 
res->check_errors == 0?

> +            error_free(s->migration_blocker);

I’d move this clean-up to a new error path below, then we could even 
reuse that where migrate_add_blocker() fails.

Anyway, not wrong as-is, just suggestion, so:

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

> +            error_setg_errno(errp, -ret, "Could not repair corrupted image");
> +            goto fail;
> +        }
> +    }
> +
>       return 0;
>   
>   fail_format:

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

[Alexander Ivanov]

On 6/2/23 16:08, Hanna Czenczek wrote:
> On 29.05.23 17:15, 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 | 86 +++++++++++++++++++++++++++++++++--------------
>>   1 file changed, 61 insertions(+), 25 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 1ec98c722b..64850b9655 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -482,43 +482,79 @@ parallels_check_outside_image(BlockDriverState 
>> *bs, BdrvCheckResult *res,
>>       return 0;
>>   }
>>   +static int64_t parallels_get_leak_size(BlockDriverState *bs,
>> +                                       BdrvCheckResult *res)
>> +{
>> +    int64_t size;
>> +
>> +    size = bdrv_getlength(bs->file->bs);
>> +    if (size < 0) {
>> +        return size;
>> +    }
>> +
>> +    /*
>> +     * Before any usage of this function, image_end_offset has to be 
>> set to the
>> +     * the highest offset in the BAT, excluding out-of-image offsets.
>> +     */
>> +    assert(size >= res->image_end_offset);
>
> If `high_off == 0` in parallels_check_outside_image(), it will use 
> s->data_end to determine image_end_offset, which is originally read 
> from the image header.  I don’t see any place where we ensure that 
> `s->data_end <= bdrv_getlength(bs->file->bs)`, so can we be certain 
> the assertion holds even in that case?
Will add s->data_end > file_nb_sectors check to parallels_open(). Thank you.
>
>> +
>> +    return 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 coroutine_fn GRAPH_RDLOCK
>>   parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
>>                        BdrvCheckMode fix)
>>   {
>>       BDRVParallelsState *s = bs->opaque;
>> -    int64_t size;
>> +    int64_t count, leak_size;
>>       int ret;
>>   -    size = bdrv_getlength(bs->file->bs);
>> -    if (size < 0) {
>> +    leak_size = parallels_get_leak_size(bs, res);
>> +    if (leak_size < 0) {
>>           res->check_errors++;
>> -        return size;
>> +        return leak_size;
>> +    }
>> +    if (leak_size == 0) {
>> +        return 0;
>>       }
>>   -    if (size > res->image_end_offset) {
>> -        int64_t count;
>> -        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;
>> +    count = DIV_ROUND_UP(leak_size, s->cluster_size);
>> +    res->leaks += count;
>> +    fprintf(stderr, "%s space leaked at the end of the image %" 
>> PRId64 "\n",
>> +            fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_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);
>> -                res->check_errors++;
>> -                return ret;
>> -            }
>> -            res->leaks_fixed += count;
>> +    if (fix & BDRV_FIX_LEAKS) {
>> +        ret = parallels_fix_leak(bs, res);
>> +        if (ret < 0) {
>> +            return ret;
>
> We used to increment res->check_errors here – should we still do that?
>
> Hanna
>
>>           }
>> +        res->leaks_fixed += count;
>>       }
>>         return 0;
>

-- 
Best regards,
Alexander Ivanov

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

[Alexander Ivanov]

On 6/2/23 16:43, Hanna Czenczek wrote:
> On 29.05.23 17:15, 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() helper 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.
>
> I’m not really a fan of splitting parallels_fix_leak() in this way. 
> One problem is that parallels_check_leak() still increments 
> leaks_fixed, even though it cannot know whether that will succeed. 
> Would it be a problem to move parallels_check_leak() after 
> parallels_check_duplicate()?
Will get rid of this splitting and add an argument to 
parallels_check_leak() for opportunity to call this function without 
changing leaks and fixed_leaks fields in res. So we can just call it 
from parallels_check_duplicate().
>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>>   block/parallels.c | 138 ++++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 133 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 64850b9655..9fa1f93973 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -136,6 +136,12 @@ 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 -= le32_to_cpu(s->header->data_off) << BDRV_SECTOR_BITS;
>> +    return off / s->cluster_size;
>> +}
>> +
>>   static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
>>                               int nb_sectors, int *pnum)
>>   {
>> @@ -533,7 +539,6 @@ parallels_check_leak(BlockDriverState *bs, 
>> BdrvCheckResult *res,
>>   {
>>       BDRVParallelsState *s = bs->opaque;
>>       int64_t count, leak_size;
>> -    int ret;
>>         leak_size = parallels_get_leak_size(bs, res);
>>       if (leak_size < 0) {
>> @@ -550,16 +555,127 @@ parallels_check_leak(BlockDriverState *bs, 
>> BdrvCheckResult *res,
>>               fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size);
>>         if (fix & BDRV_FIX_LEAKS) {
>> -        ret = parallels_fix_leak(bs, res);
>> -        if (ret < 0) {
>> -            return ret;
>> -        }
>>           res->leaks_fixed += count;
>>       }
>>         return 0;
>>   }
>>   +static int parallels_check_duplicate(BlockDriverState *bs,
>> +                                     BdrvCheckResult *res,
>> +                                     BdrvCheckMode *fix)
>> +{
>> +    BDRVParallelsState *s = bs->opaque;
>> +    QEMUIOVector qiov;
>> +    int64_t off, sector;
>> +    unsigned long *bitmap;
>> +    uint32_t i, bitmap_size, cluster_index;
>> +    int n, ret = 0;
>> +    uint64_t *buf = NULL;
>> +
>> +    /*
>> +     * 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, res->image_end_offset);
>> +    if (bitmap_size == 0) {
>> +        return 0;
>> +    }
>> +
>> +    bitmap = bitmap_new(bitmap_size);
>> +
>> +    buf = qemu_memalign(4096, s->cluster_size);
>
> There is qemu_blockalign(), which actually uses the BDS’s memory 
> alignment, so should be a better fit.
+
>
>> +    qemu_iovec_init(&qiov, 0);
>> +    qemu_iovec_add(&qiov, buf, s->cluster_size);
>
> I don’t think this is necessary, there is bdrv_co_pwrite() that takes 
> a buffer.  OTOH, if you want to keep this, you could replace the 
> bdrv_co_pread() call by bdrv_co_preadv().
+
>
>> +
>> +    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)) {
>
> I understand we’ve already ensured that image_end_offset (which 
> determines the bitmap size) is large enough, and so this can’t 
> overflow, but I could sleep better if there was an 
> `assert(cluster_index < bitmap_size);` before this `test_bit()`.
+
>> +            /* 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);
>
> As far as I understand, this will modify the image content when read.  
> Can we perhaps revert this change if there’s an error in 
> bdrv_co_pread() or allocate_clusters()?  I understand that a double 
> allocation is a bad corruption, but if we can’t fix it because of some 
> unexpected error, I feel like it’s better to still keep the image in 
> the same state as before rather than having effectively destroyed the 
> data in the respective cluster, so users can at least try to fix it by 
> copying it.
+
>
>> +
>> +                ret = bdrv_co_pread(bs->file, off, s->cluster_size, 
>> buf, 0);
>> +                if (ret < 0) {
>> +                    res->check_errors++;
>> +                    goto out;
>> +                }
>> +
>> +                sector = (i * (int64_t)s->cluster_size) >> 
>> BDRV_SECTOR_BITS;
>
> Both for my own understanding and to maybe suggest a simplification: 
> This is the same as `sector = i * (int64_t)s->tracks`, right?
Yes, you are right, will replace.
>
> Hanna
>
>> +                sector = allocate_clusters(bs, sector, s->tracks, &n);
>> +                if (sector < 0) {
>> +                    res->check_errors++;
>> +                    ret = sector;
>> +                    goto out;
>> +                }
>

-- 
Best regards,
Alexander Ivanov

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

[Alexander Ivanov]

On 6/2/23 16:48, Hanna Czenczek wrote:
> On 29.05.23 17:15, 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.
>
> I understand stderr counts as part of the VM log, doesn’t it?  I 
> thought stderr is generally logged, and naïvely, it seems like the 
> better fit to me, because it conveys more urgency than the standard 
> log (which, judging from its callers, looks mostly like a debug log).
>
> Hanna
We want to add some image checks to parallels_open(). It means that it 
will be used not only in qemu-img and we may lose these messages if a 
log file is specified. Stderr is not duplicated to the log file.
>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> Reviewed-by: Denis V. Lunev <den@openvz.org>
>> ---
>>   block/parallels.c | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>

-- 
Best regards,
Alexander Ivanov

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

[Peter Maydell]

On Mon, 29 May 2023 at 16:16, Alexander Ivanov
<alexander.ivanov@virtuozzo.com> 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>
> Reviewed-by: Denis V. Lunev <den@openvz.org>
> ---
>  block/parallels.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 9fa1f93973..d64e8007d5 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"
>
> @@ -436,8 +437,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");

Generally speaking you shouldn't directly call qemu_log().
Instead call qemu_log_mask() and pass it the relevant
QEMU_LOG_* constant for whichever kind of -d debug logging
this is. The raw qemu_log() function is for the odd case
of expensive logging where you want to say
 if (log category foo enabled) {
     do expensive calculation;
     qemu_log(result1);
     qemu_log(result2);
 }

But this doesn't look like the kind of logging we would usually
do with qemu_log(). Notably, the default for the qemu_log machinery
is to not output anything at all: it's only enabled if the user
explicitly turns on debug logging with a -d option.

You probably want warn_report() here.

thanks
-- PMM

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

[Alexander Ivanov]

On 6/2/23 16:59, Hanna Czenczek wrote:
> On 29.05.23 17:15, 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 | 65 +++++++++++++++++++++++++----------------------
>>   1 file changed, 35 insertions(+), 30 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index d64e8007d5..7bbd5cb112 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>
> [...]
>
>> @@ -1130,6 +1101,40 @@ static int parallels_open(BlockDriverState 
>> *bs, QDict *options, int flags,
>>           goto fail;
>>       }
>>       qemu_co_mutex_init(&s->lock);
>> +
>> +    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
>> +        s->header_unclean = true;
>> +    }
>> +
>> +    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;
>> +        }
>> +    }
>> +
>> +    /*
>> +     * We don't repair the image here if it's opened for checks. 
>> Also we don't
>> +     * want to change inactive images and can't change readonly images.
>> +     */
>> +    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_nb_sectors || s->header_unclean) {
>> +        BdrvCheckResult res;
>> +        ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
>> +        if (ret < 0) {
>
> Should we also verify that res->corruptions == res->corruptions_fixed 
> && res->check_errors == 0?
If ret == 0 there must be res->check_errors == 0 and res->corruptions == 
res->corruptions_fixed.
>
>> + error_free(s->migration_blocker);
>
> I’d move this clean-up to a new error path below, then we could even 
> reuse that where migrate_add_blocker() fails.
Is this guaranteed that s->migration_blocker is NULL at the function 
parallels_open() beginning? If so it could be easy to move the clean-up,
otherwise it could lead to code complication.
>
> Anyway, not wrong as-is, just suggestion, so:
>
> Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
>
>> +            error_setg_errno(errp, -ret, "Could not repair corrupted 
>> image");
>> +            goto fail;
>> +        }
>> +    }
>> +
>>       return 0;
>>     fail_format:
>

-- 
Best regards,
Alexander Ivanov

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

[Hanna Czenczek]

On 09.06.23 15:21, Alexander Ivanov wrote:
>
>
> On 6/2/23 16:59, Hanna Czenczek wrote:
>> On 29.05.23 17:15, 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 | 65 
>>> +++++++++++++++++++++++++----------------------
>>>   1 file changed, 35 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/block/parallels.c b/block/parallels.c
>>> index d64e8007d5..7bbd5cb112 100644
>>> --- a/block/parallels.c
>>> +++ b/block/parallels.c
>>
>> [...]
>>
>>> @@ -1130,6 +1101,40 @@ static int parallels_open(BlockDriverState 
>>> *bs, QDict *options, int flags,
>>>           goto fail;
>>>       }
>>>       qemu_co_mutex_init(&s->lock);
>>> +
>>> +    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
>>> +        s->header_unclean = true;
>>> +    }
>>> +
>>> +    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;
>>> +        }
>>> +    }
>>> +
>>> +    /*
>>> +     * We don't repair the image here if it's opened for checks. 
>>> Also we don't
>>> +     * want to change inactive images and can't change readonly 
>>> images.
>>> +     */
>>> +    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_nb_sectors || s->header_unclean) {
>>> +        BdrvCheckResult res;
>>> +        ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
>>> +        if (ret < 0) {
>>
>> Should we also verify that res->corruptions == res->corruptions_fixed 
>> && res->check_errors == 0?
> If ret == 0 there must be res->check_errors == 0 and res->corruptions 
> == res->corruptions_fixed.

OK.

>>
>>> + error_free(s->migration_blocker);
>>
>> I’d move this clean-up to a new error path below, then we could even 
>> reuse that where migrate_add_blocker() fails.
> Is this guaranteed that s->migration_blocker is NULL at the function 
> parallels_open() beginning? If so it could be easy to move the clean-up,
> otherwise it could lead to code complication.

Three answers here:

First, I just realized that we probably need to undo the 
migrate_add_blocker() call, too, i.e. call migrate_del_blocker() here.

Second, I’m pretty sure that s->migration_blocker must be NULL before 
the error_setg(&s->migration_blocker) call, because error_setg() asserts 
that the *errp passed to it is NULL.

Third, I meant to add a new path e.g.:

```
fail_blocker:
     error_free(s->migration_blocker);
fail_format:
[...]
```

And then use `goto fail_blocker;` here and in the migrate_add_blocker() 
error path, so it shouldn’t really matter whether s->migration_blocker 
is NULL before the error_setg() call.  But then again, I think the 
probably necessary migrate_del_blocker() call complicates things further.

Hanna

>>
>> Anyway, not wrong as-is, just suggestion, so:
>>
>> Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
>>
>>> +            error_setg_errno(errp, -ret, "Could not repair 
>>> corrupted image");
>>> +            goto fail;
>>> +        }
>>> +    }
>>> +
>>>       return 0;
>>>     fail_format:
>>
>

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

[Alexander Ivanov]

On 6/9/23 15:41, Hanna Czenczek wrote:
> On 09.06.23 15:21, Alexander Ivanov wrote:
>>
>>
>> On 6/2/23 16:59, Hanna Czenczek wrote:
>>> On 29.05.23 17:15, 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 | 65 
>>>> +++++++++++++++++++++++++----------------------
>>>>   1 file changed, 35 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/block/parallels.c b/block/parallels.c
>>>> index d64e8007d5..7bbd5cb112 100644
>>>> --- a/block/parallels.c
>>>> +++ b/block/parallels.c
>>>
>>> [...]
>>>
>>>> @@ -1130,6 +1101,40 @@ static int parallels_open(BlockDriverState 
>>>> *bs, QDict *options, int flags,
>>>>           goto fail;
>>>>       }
>>>>       qemu_co_mutex_init(&s->lock);
>>>> +
>>>> +    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
>>>> +        s->header_unclean = true;
>>>> +    }
>>>> +
>>>> +    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;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * We don't repair the image here if it's opened for checks. 
>>>> Also we don't
>>>> +     * want to change inactive images and can't change readonly 
>>>> images.
>>>> +     */
>>>> +    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_nb_sectors || s->header_unclean) {
>>>> +        BdrvCheckResult res;
>>>> +        ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
>>>> +        if (ret < 0) {
>>>
>>> Should we also verify that res->corruptions == 
>>> res->corruptions_fixed && res->check_errors == 0?
>> If ret == 0 there must be res->check_errors == 0 and res->corruptions 
>> == res->corruptions_fixed.
>
> OK.
>
>>>
>>>> + error_free(s->migration_blocker);
>>>
>>> I’d move this clean-up to a new error path below, then we could even 
>>> reuse that where migrate_add_blocker() fails.
>> Is this guaranteed that s->migration_blocker is NULL at the function 
>> parallels_open() beginning? If so it could be easy to move the clean-up,
>> otherwise it could lead to code complication.
>
> Three answers here:
>
> First, I just realized that we probably need to undo the 
> migrate_add_blocker() call, too, i.e. call migrate_del_blocker() here.
>
> Second, I’m pretty sure that s->migration_blocker must be NULL before 
> the error_setg(&s->migration_blocker) call, because error_setg() 
> asserts that the *errp passed to it is NULL.
>
> Third, I meant to add a new path e.g.:
>
> ```
> fail_blocker:
>     error_free(s->migration_blocker);
> fail_format:
> [...]
> ```
>
> And then use `goto fail_blocker;` here and in the 
> migrate_add_blocker() error path, so it shouldn’t really matter 
> whether s->migration_blocker is NULL before the error_setg() call.  
> But then again, I think the probably necessary migrate_del_blocker() 
> call complicates things further.
>
> Hanna
Do we need to run the rest part of the parallels_close() code?

     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);
     }

     g_free(s->bat_dirty_bmap);

If so, maybe it would be better to call parallels_close()?

>>>
>>> Anyway, not wrong as-is, just suggestion, so:
>>>
>>> Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
>>>
>>>> +            error_setg_errno(errp, -ret, "Could not repair 
>>>> corrupted image");
>>>> +            goto fail;
>>>> +        }
>>>> +    }
>>>> +
>>>>       return 0;
>>>>     fail_format:
>>>
>>
>

-- 
Best regards,
Alexander Ivanov