[PATCH v2 0/3] Check and repair duplicated clusters in parallels images

[PATCH v2 0/3] Check and repair duplicated clusters in parallels images

[alexander.ivanov]

From: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

We will add more and more checks of images so we need to reorganize the code.
Put each check to a separate helper function with a separate loop.
Add two helpers: truncate_file() and sync_header(). They will be used
in multiple functions.

Parallels image file can be corrupted this way: two guest memory areas
refer to the same host memory area (duplicated offsets in BAT).
qemu-img check copies data from duplicated cluster to the new cluster and
writes new corresponding offset to BAT instead of duplicated one.

Test 314 uses sample corrupted image parallels-2-duplicated-cluster.bz2.
Reading from duplicated offset and from original offset returns the same
data. After repairing changing either of these blocks of data
does not affect another one.

v2 changes:
 * Split parallels_co_check() to separate functions.
 * Move buffer allocation outside the loop.
 * Replace work with internals by zeroing BAT entries and
   allocate_clusters() calling.
 * Make reverse table unsigned and replace -1 by 0xFFFFFFFF.
 * Some minor fixes.
 * Add more detailed comments.

Alexander Ivanov (3):
  parallels: Put the image checks in separate functions
  parallels: Add checking and repairing duplicate offsets in BAT
  iotests, parallels: Add a test for duplicated clusters

 block/parallels.c                             | 315 +++++++++++++++---
 tests/qemu-iotests/314                        |  89 +++++
 tests/qemu-iotests/314.out                    |  36 ++
 .../parallels-2-duplicated-cluster.bz2        | Bin 0 -> 148 bytes
 4 files changed, 393 insertions(+), 47 deletions(-)
 create mode 100755 tests/qemu-iotests/314
 create mode 100644 tests/qemu-iotests/314.out
 create mode 100644 tests/qemu-iotests/sample_images/parallels-2-duplicated-cluster.bz2

-- 
2.34.1

[PATCH v2 1/3] parallels: Put the image checks in separate functions

[alexander.ivanov]

From: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

We will add more and more checks of images so we need to reorganize the code.
Put each check to a separate helper function with a separate loop.
Add two helpers: truncate_file() and sync_header(). They will be used
in multiple functions.

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

diff --git a/block/parallels.c b/block/parallels.c
index a229c06f25..a0eb7ec3c3 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -413,24 +413,41 @@ static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
     return ret;
 }
 
-
-static int coroutine_fn parallels_co_check(BlockDriverState *bs,
-                                           BdrvCheckResult *res,
-                                           BdrvCheckMode fix)
+static int sync_header(BlockDriverState *bs, BdrvCheckResult *res)
 {
     BDRVParallelsState *s = bs->opaque;
-    int64_t size, prev_off, high_off;
     int ret;
-    uint32_t i;
-    bool flush_bat = false;
 
-    size = bdrv_getlength(bs->file->bs);
-    if (size < 0) {
+    ret = bdrv_co_pwrite_sync(bs->file, 0, s->header_size, s->header, 0);
+    if (ret < 0) {
         res->check_errors++;
-        return size;
     }
+    return ret;
+}
+
+static int truncate_file(BlockDriverState *bs, int64_t size)
+{
+    Error *local_err = NULL;
+    int ret;
+
+    /*
+     * In order to really repair the image, we must shrink it.
+     * That means we have to pass exact=true.
+     */
+    ret = bdrv_truncate(bs->file, size, true,
+                        PREALLOC_MODE_OFF, 0, &local_err);
+    if (ret < 0) {
+        error_report_err(local_err);
+    }
+    return ret;
+}
+
+static void check_unclean(BlockDriverState *bs,
+                          BdrvCheckResult *res,
+                          BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
 
-    qemu_co_mutex_lock(&s->lock);
     if (s->header_unclean) {
         fprintf(stderr, "%s image was not closed correctly\n",
                 fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
@@ -441,78 +458,147 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
             s->header_unclean = false;
         }
     }
+}
 
-    res->bfi.total_clusters = s->bat_size;
-    res->bfi.compressed_clusters = 0; /* compression is not supported */
+static int check_cluster_outside_image(BlockDriverState *bs,
+                                       BdrvCheckResult *res,
+                                       BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int64_t size;
+    uint32_t i;
+    bool sync = false;
+
+    size = bdrv_getlength(bs->file->bs);
+    if (size < 0) {
+        res->check_errors++;
+        return size;
+    }
 
-    high_off = 0;
-    prev_off = 0;
     for (i = 0; i < s->bat_size; i++) {
-        int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS;
-        if (off == 0) {
-            prev_off = 0;
-            continue;
-        }
-
-        /* cluster outside the image */
-        if (off > size) {
+        if ((bat2sect(s, i) << BDRV_SECTOR_BITS) > size) {
             fprintf(stderr, "%s cluster %u is outside image\n",
                     fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
             res->corruptions++;
             if (fix & BDRV_FIX_ERRORS) {
-                prev_off = 0;
                 s->bat_bitmap[i] = 0;
                 res->corruptions_fixed++;
-                flush_bat = true;
-                continue;
+                sync = true;
             }
         }
+    }
 
-        res->bfi.allocated_clusters++;
-        if (off > high_off) {
-            high_off = off;
+    if (sync) {
+        return sync_header(bs, res);
+    }
+
+    return 0;
+}
+
+static void check_fragmentation(BlockDriverState *bs,
+                                BdrvCheckResult *res,
+                                BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int64_t off, prev_off;
+    uint32_t i;
+
+    prev_off = 0;
+    for (i = 0; i < s->bat_size; i++) {
+        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+        if (off == 0) {
+            prev_off = 0;
+            continue;
         }
-
         if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
             res->bfi.fragmented_clusters++;
         }
         prev_off = off;
     }
+}
 
-    ret = 0;
-    if (flush_bat) {
-        ret = bdrv_co_pwrite_sync(bs->file, 0, s->header_size, s->header, 0);
-        if (ret < 0) {
-            res->check_errors++;
-            goto out;
+static int check_leak(BlockDriverState *bs,
+                      BdrvCheckResult *res,
+                      BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int64_t size, off, high_off, count;
+    uint32_t i;
+
+    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;
+        if (off > high_off) {
+            high_off = off;
         }
     }
 
     res->image_end_offset = high_off + s->cluster_size;
     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;
+            int ret;
 
-            /*
-             * In order to really repair the image, we must shrink it.
-             * That means we have to pass exact=true.
-             */
-            ret = bdrv_truncate(bs->file, res->image_end_offset, true,
-                                PREALLOC_MODE_OFF, 0, &local_err);
+            ret = truncate_file(bs, res->image_end_offset);
             if (ret < 0) {
-                error_report_err(local_err);
-                res->check_errors++;
-                goto out;
+                return ret;
             }
             res->leaks_fixed += count;
         }
     }
+    return 0;
+}
+
+static void collect_statistics(BlockDriverState *bs, BdrvCheckResult *res)
+{
+    BDRVParallelsState *s = bs->opaque;
+    uint32_t i;
+
+    res->bfi.total_clusters = s->bat_size;
+    res->bfi.compressed_clusters = 0; /* compression is not supported */
+    res->bfi.allocated_clusters = 0;
+
+    for (i = 0; i < s->bat_size; i++) {
+        if ((bat2sect(s, i) << BDRV_SECTOR_BITS) > 0) {
+            res->bfi.allocated_clusters++;
+        }
+    }
+}
+
+static int coroutine_fn parallels_co_check(BlockDriverState *bs,
+                                           BdrvCheckResult *res,
+                                           BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int ret;
+
+    qemu_co_mutex_lock(&s->lock);
+
+    check_unclean(bs, res, fix);
+
+    ret = check_cluster_outside_image(bs, res, fix);
+    if (ret != 0) {
+        goto out;
+    }
+
+    check_fragmentation(bs, res, fix);
+
+    ret = check_leak(bs, res, fix);
+    if (ret != 0) {
+        goto out;
+    }
+
+    collect_statistics(bs, res);
 
 out:
     qemu_co_mutex_unlock(&s->lock);
-- 
2.34.1

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

[alexander.ivanov]

From: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

There could be corruptions in the image file:
two guest memory areas refer to the same host cluster.

If a duplicate offset is found fix it by copying the content
of the referred cluster to a new allocated cluster and
replace one of the two referring entries by the new cluster offset.

Signed-off-by: Natalia Kuzmina <natalia.kuzmina@openvz.org>
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 135 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index a0eb7ec3c3..73264b4bd1 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -64,6 +64,11 @@ static QEnumLookup prealloc_mode_lookup = {
 #define PARALLELS_OPT_PREALLOC_MODE     "prealloc-mode"
 #define PARALLELS_OPT_PREALLOC_SIZE     "prealloc-size"
 
+#define REVERSED_BAT_UNTOUCHED  0xFFFFFFFF
+
+#define HOST_CLUSTER_INDEX(s, off) \
+    ((off - ((s->header->data_off) << BDRV_SECTOR_BITS)) / (s->cluster_size))
+
 static QemuOptsList parallels_runtime_opts = {
     .name = "parallels",
     .head = QTAILQ_HEAD_INITIALIZER(parallels_runtime_opts.head),
@@ -559,6 +564,131 @@ static int check_leak(BlockDriverState *bs,
     return 0;
 }
 
+static int check_duplicate(BlockDriverState *bs,
+                           BdrvCheckResult *res,
+                           BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    QEMUIOVector qiov;
+    int64_t off, high_off, size, sector_num;
+    uint32_t i, idx_host;
+    int ret = 0, n;
+    g_autofree uint32_t *reversed_bat = NULL;
+    g_autofree int64_t *cluster_buf = NULL;
+    bool sync_and_truncate = false;
+
+    /*
+     * Make a reversed BAT.
+     * Each cluster in the host file could represent only one cluster
+     * from the guest point of view. Reversed BAT provides mapping of that type.
+     * Initially the table is filled with REVERSED_BAT_UNTOUCHED values.
+     */
+    reversed_bat = g_malloc(s->bat_size * sizeof(uint32_t));
+    for (i = 0; i < s->bat_size; i++) {
+        reversed_bat[i] = REVERSED_BAT_UNTOUCHED;
+    }
+
+    cluster_buf = g_malloc(s->cluster_size);
+    qemu_iovec_init(&qiov, 0);
+    qemu_iovec_add(&qiov, cluster_buf, s->cluster_size);
+
+    high_off = 0;
+    for (i = 0; i < s->bat_size; i++) {
+        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+        if (off == 0) {
+            continue;
+        }
+
+        if (off > high_off) {
+            high_off = off;
+        }
+
+        idx_host = HOST_CLUSTER_INDEX(s, off);
+        if (idx_host >= s->bat_size) {
+            res->check_errors++;
+            goto out;
+        }
+
+        if (reversed_bat[idx_host] != REVERSED_BAT_UNTOUCHED) {
+            /*
+             * We have alreade written a guest cluster index for this
+             * host cluster. It means there is a duplicated offset in BAT.
+             */
+            fprintf(stderr,
+                    "%s BAT offset in entry %u duplicates offset in entry %u\n",
+                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR",
+                    i, reversed_bat[idx_host]);
+            res->corruptions++;
+
+            if (fix & BDRV_FIX_ERRORS) {
+                /*
+                 * Write zero to the current BAT entry, allocate a new cluster
+                 * for the relevant guest offset and copy the host cluster
+                 * to the new allocated cluster.
+                 * In this way mwe will have two identical clusters and two
+                 * BAT entries with the offsets of these clusters.
+                 */
+                s->bat_bitmap[i] = 0;
+
+                sector_num = bat2sect(s, reversed_bat[idx_host]);
+                ret = bdrv_pread(bs->file, sector_num << BDRV_SECTOR_BITS,
+                                 s->cluster_size, cluster_buf, 0);
+                if (ret < 0) {
+                    res->check_errors++;
+                    goto out;
+                }
+
+                sector_num = (i * s->cluster_size) >> BDRV_SECTOR_BITS;
+                off = allocate_clusters(bs, sector_num, s->tracks, &n);
+                if (off < 0) {
+                    res->check_errors++;
+                    ret = off;
+                    goto out;
+                }
+                off <<= BDRV_SECTOR_BITS;
+
+                ret = bdrv_co_pwritev(bs->file, off, s->cluster_size, &qiov, 0);
+                if (ret < 0) {
+                    res->check_errors++;
+                    goto out;
+                }
+
+                /* off is new and we should repair idx_host accordingly. */
+                idx_host = HOST_CLUSTER_INDEX(s, off);
+                res->corruptions_fixed++;
+                sync_and_truncate = true;
+            }
+        }
+        reversed_bat[idx_host] = i;
+    }
+
+    if (sync_and_truncate) {
+        ret = sync_header(bs, res);
+        if (ret < 0) {
+            goto out;
+        }
+
+        size = bdrv_getlength(bs->file->bs);
+        if (size < 0) {
+            res->check_errors++;
+            ret = size;
+            goto out;
+        }
+
+        res->image_end_offset = high_off + s->cluster_size;
+        if (size > res->image_end_offset) {
+            ret = truncate_file(bs, res->image_end_offset);
+            if (ret < 0) {
+                goto out;
+            }
+        }
+    }
+
+out:
+    qemu_iovec_destroy(&qiov);
+    return ret;
+}
+
 static void collect_statistics(BlockDriverState *bs, BdrvCheckResult *res)
 {
     BDRVParallelsState *s = bs->opaque;
@@ -598,6 +728,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
         goto out;
     }
 
+    ret = check_duplicate(bs, res, fix);
+    if (ret != 0) {
+        goto out;
+    }
+
     collect_statistics(bs, res);
 
 out:
-- 
2.34.1

[PATCH v2 3/3] iotests, parallels: Add a test for duplicated clusters

[alexander.ivanov]

From: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

Check if original and duplicated offsets refer to the same cluster.
Repair the image and check that writing to a referred cluster
doesn't affects another referred cluster.

Signed-off-by: Natalia Kuzmina <natalia.kuzmina@openvz.org>
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 tests/qemu-iotests/314                        |  89 ++++++++++++++++++
 tests/qemu-iotests/314.out                    |  36 +++++++
 .../parallels-2-duplicated-cluster.bz2        | Bin 0 -> 148 bytes
 3 files changed, 125 insertions(+)
 create mode 100755 tests/qemu-iotests/314
 create mode 100644 tests/qemu-iotests/314.out
 create mode 100644 tests/qemu-iotests/sample_images/parallels-2-duplicated-cluster.bz2

diff --git a/tests/qemu-iotests/314 b/tests/qemu-iotests/314
new file mode 100755
index 0000000000..79b4d3a749
--- /dev/null
+++ b/tests/qemu-iotests/314
@@ -0,0 +1,89 @@
+#!/usr/bin/env bash
+# group: rw auto quick
+#
+# Test qemu-img check on duplicated clusters
+#
+# Copyright (c) 2022 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator natalia.kuzmina@openvz.org
+
+owner=alexander.ivanov@virtuozzo.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1    # failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+
+_supported_fmt parallels
+_supported_proto file
+_supported_os Linux
+
+echo
+echo "using sample corrupted image"
+echo
+_use_sample_img parallels-2-duplicated-cluster.bz2
+
+CLUSTER_SIZE=65536
+
+#read one cluster from original offset
+$QEMU_IO -c "read -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG" | \
+    _filter_qemu_io
+#read from duplicated offset (data must be the same as on original offset)
+$QEMU_IO -c "read -P 0x11 $((4 * CLUSTER_SIZE)) $CLUSTER_SIZE" "$TEST_IMG" | \
+    _filter_qemu_io
+#change data from original offset
+$QEMU_IO -c "write -P 0x55 0 $CLUSTER_SIZE" "$TEST_IMG" | \
+    _filter_qemu_io
+#read from duplicated offset (data must be the same as on original offset)
+$QEMU_IO -c "read -P 0x55 $((4 * CLUSTER_SIZE)) $CLUSTER_SIZE" "$TEST_IMG" | \
+    _filter_qemu_io
+echo
+echo "check and repair the image for parallels format"
+echo
+_check_test_img -r all
+echo
+
+#read one cluster from original offset
+$QEMU_IO -c "read -P 0x55 0 $CLUSTER_SIZE" "$TEST_IMG" | \
+    _filter_qemu_io
+#read copied data from new offset
+$QEMU_IO -c "read -P 0x55 $((4 * CLUSTER_SIZE)) $CLUSTER_SIZE" "$TEST_IMG" | \
+    _filter_qemu_io
+#change data from original offset
+$QEMU_IO -c "write -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG" | \
+    _filter_qemu_io
+#read from new offset (fail, now this data was left unchanged)
+$QEMU_IO -c "read -P 0x11 $((4 * CLUSTER_SIZE)) $CLUSTER_SIZE" "$TEST_IMG" | \
+    _filter_qemu_io
+
+echo
+echo
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/314.out b/tests/qemu-iotests/314.out
new file mode 100644
index 0000000000..c36022c407
--- /dev/null
+++ b/tests/qemu-iotests/314.out
@@ -0,0 +1,36 @@
+QA output created by 314
+
+using sample corrupted image
+
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 262144
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 262144
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+check and repair the image
+
+Repairing BAT offset in entry 4 duplicates offset in entry 0
+The following inconsistencies were found and repaired:
+
+    0 leaked clusters
+    1 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
+
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65536/65536 bytes at offset 262144
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Pattern verification failed at offset 262144, 65536 bytes
+read 65536/65536 bytes at offset 262144
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+
+*** done
diff --git a/tests/qemu-iotests/sample_images/parallels-2-duplicated-cluster.bz2 b/tests/qemu-iotests/sample_images/parallels-2-duplicated-cluster.bz2
new file mode 100644
index 0000000000000000000000000000000000000000..ee8f0149b5ecffc4fdc5e2c0cf45b731610378af
GIT binary patch
literal 148
zcmZ>Y%CIzaj8qGboOfsS0tPO%`U`(O5*Pv)I2hO&I2yDPt~od`068263_Exd7-leV
zwiz(^Ft8k0sa3TsBZG0}Vv}35zt?O%VET5A+3Q2o4%bdpm~pLC^&`WR2CW6$VGH;&
vm{u|@;OhXBE0|U>d|v){U)AOQJ)h70iu-<&;S?CYW~db}a<vGU0CEKYoE$uo

literal 0
HcmV?d00001

-- 
2.34.1

Re: [PATCH v2 1/3] parallels: Put the image checks in separate functions

[Vladimir Sementsov-Ogievskiy]

On 8/5/22 18:47, alexander.ivanov@virtuozzo.com wrote:
> From: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

A bit strange to see this. Is your name set up correctly in ~/.gitconfig ?

> 
> We will add more and more checks of images so we need to reorganize the code.
> Put each check to a separate helper function with a separate loop.
> Add two helpers: truncate_file() and sync_header(). They will be used
> in multiple functions.

It would be a lot simpler to review if you split out one check_ function per patch :)

> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---

[..]

> -            /*
> -             * In order to really repair the image, we must shrink it.
> -             * That means we have to pass exact=true.
> -             */
> -            ret = bdrv_truncate(bs->file, res->image_end_offset, true,
> -                                PREALLOC_MODE_OFF, 0, &local_err);
> +            ret = truncate_file(bs, res->image_end_offset);
>               if (ret < 0) {
> -                error_report_err(local_err);
> -                res->check_errors++;

Why you remove check_errors++ ? It's an error of truncate_file counted.
With it kept here:

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

> -                goto out;
> +                return ret;
>               }
>               res->leaks_fixed += count;
>           }

[..]

> +static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> +                                           BdrvCheckResult *res,
> +                                           BdrvCheckMode fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    int ret;
> +
> +    qemu_co_mutex_lock(&s->lock);

If touch this anyway, I'd move to QEMU_LOCK_GUARD().

> +
> +    check_unclean(bs, res, fix);
> +
> +    ret = check_cluster_outside_image(bs, res, fix);
> +    if (ret != 0) {
> +        goto out;
> +    }
> +
> +    check_fragmentation(bs, res, fix);
> +
> +    ret = check_leak(bs, res, fix);
> +    if (ret != 0) {
> +        goto out;
> +    }
> +
> +    collect_statistics(bs, res);

probably, chack_fragmentation() should be part of collect_statistics() (called from it, or just merged into same loop, as it fills same res->bfi)

Also while reviewing I noted that when FIX_ERRORS is not enabled we count broken clusters (with off > size) as allocated, and also they participate in fragmentation statistics. That's preexisting still..


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

On 8/5/22 18:47, alexander.ivanov@virtuozzo.com wrote:
> From: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> 
> There could be corruptions in the image file:
> two guest memory areas refer to the same host cluster.

Is that written in parallels spec (docs/interop/parallels.txt)?

Hmm yes: "- the value must be unique among all BAT entries".

> 
> If a duplicate offset is found fix it by copying the content
> of the referred cluster to a new allocated cluster and
> replace one of the two referring entries by the new cluster offset.
> 
> Signed-off-by: Natalia Kuzmina <natalia.kuzmina@openvz.org>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 135 insertions(+)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index a0eb7ec3c3..73264b4bd1 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -64,6 +64,11 @@ static QEnumLookup prealloc_mode_lookup = {
>   #define PARALLELS_OPT_PREALLOC_MODE     "prealloc-mode"
>   #define PARALLELS_OPT_PREALLOC_SIZE     "prealloc-size"
>   
> +#define REVERSED_BAT_UNTOUCHED  0xFFFFFFFF
> +
> +#define HOST_CLUSTER_INDEX(s, off) \
> +    ((off - ((s->header->data_off) << BDRV_SECTOR_BITS)) / (s->cluster_size))

Let it be a static function, not a macro.

> +
>   static QemuOptsList parallels_runtime_opts = {
>       .name = "parallels",
>       .head = QTAILQ_HEAD_INITIALIZER(parallels_runtime_opts.head),
> @@ -559,6 +564,131 @@ static int check_leak(BlockDriverState *bs,
>       return 0;
>   }
>   
> +static int check_duplicate(BlockDriverState *bs,
> +                           BdrvCheckResult *res,
> +                           BdrvCheckMode fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    QEMUIOVector qiov;
> +    int64_t off, high_off, size, sector_num;
> +    uint32_t i, idx_host;
> +    int ret = 0, n;
> +    g_autofree uint32_t *reversed_bat = NULL;
> +    g_autofree int64_t *cluster_buf = NULL;
> +    bool sync_and_truncate = false;
> +
> +    /*
> +     * Make a reversed BAT.
> +     * Each cluster in the host file could represent only one cluster
> +     * from the guest point of view. Reversed BAT provides mapping of that type.
> +     * Initially the table is filled with REVERSED_BAT_UNTOUCHED values.
> +     */
> +    reversed_bat = g_malloc(s->bat_size * sizeof(uint32_t));

Hmm. Why size of reversed_bat equal to bat_size? Couldn't host file size be larger than that?

Seems that we want calculate the highest offset first, and then allocate corresponding table.

Also, here we probably allocate too much memory. Better use g_try_malloc and clean error-out instead of crash.

> +    for (i = 0; i < s->bat_size; i++) {
> +        reversed_bat[i] = REVERSED_BAT_UNTOUCHED;
> +    }
> +
> +    cluster_buf = g_malloc(s->cluster_size);
> +    qemu_iovec_init(&qiov, 0);
> +    qemu_iovec_add(&qiov, cluster_buf, s->cluster_size);
> +
> +    high_off = 0;
> +    for (i = 0; i < s->bat_size; i++) {
> +        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> +        if (off == 0) {
> +            continue;
> +        }
> +
> +        if (off > high_off) {
> +            high_off = off;
> +        }
> +
> +        idx_host = HOST_CLUSTER_INDEX(s, off);
> +        if (idx_host >= s->bat_size) {
> +            res->check_errors++;

As I understand, check_errors is mostly for IO errors during the check.

If it's incorrect for parallels format to have such cluster, we want res->corruptions++ here instead.

But is it really incorrect, what the spec say?

> +            goto out;
> +        }
> +
> +        if (reversed_bat[idx_host] != REVERSED_BAT_UNTOUCHED) {
> +            /*
> +             * We have alreade written a guest cluster index for this

already

> +             * host cluster. It means there is a duplicated offset in BAT.
> +             */
> +            fprintf(stderr,
> +                    "%s BAT offset in entry %u duplicates offset in entry %u\n",
> +                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR",
> +                    i, reversed_bat[idx_host]);
> +            res->corruptions++;
> +
> +            if (fix & BDRV_FIX_ERRORS) {
> +                /*
> +                 * Write zero to the current BAT entry, allocate a new cluster
> +                 * for the relevant guest offset and copy the host cluster
> +                 * to the new allocated cluster.
> +                 * In this way mwe will have two identical clusters and two
> +                 * BAT entries with the offsets of these clusters.
> +                 */
> +                s->bat_bitmap[i] = 0;

I don't understand that. So you just remove that guest cluster. Shouldn't we instead set it to new allocated off?

> +
> +                sector_num = bat2sect(s, reversed_bat[idx_host]);
> +                ret = bdrv_pread(bs->file, sector_num << BDRV_SECTOR_BITS,
> +                                 s->cluster_size, cluster_buf, 0);
> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    goto out;
> +                }
> +
> +                sector_num = (i * s->cluster_size) >> BDRV_SECTOR_BITS;
> +                off = allocate_clusters(bs, sector_num, s->tracks, &n);

you probably want to update high_off here

> +                if (off < 0) {
> +                    res->check_errors++;
> +                    ret = off;
> +                    goto out;
> +                }
> +                off <<= BDRV_SECTOR_BITS;
> +
> +                ret = bdrv_co_pwritev(bs->file, off, s->cluster_size, &qiov, 0);
> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    goto out;
> +                }
> +
> +                /* off is new and we should repair idx_host accordingly. */
> +                idx_host = HOST_CLUSTER_INDEX(s, off);
> +                res->corruptions_fixed++;
> +                sync_and_truncate = true;
> +            }
> +        }
> +        reversed_bat[idx_host] = i;
> +    }
> +
> +    if (sync_and_truncate) {
> +        ret = sync_header(bs, res);
> +        if (ret < 0) {
> +            goto out;
> +        }
> +
> +        size = bdrv_getlength(bs->file->bs);
> +        if (size < 0) {
> +            res->check_errors++;
> +            ret = size;
> +            goto out;
> +        }
> +
> +        res->image_end_offset = high_off + s->cluster_size;
> +        if (size > res->image_end_offset) {
> +            ret = truncate_file(bs, res->image_end_offset);

that's already done in check_leak, why we need it again?

> +            if (ret < 0) {
> +                goto out;
> +            }
> +        }
> +    }
> +
> +out:
> +    qemu_iovec_destroy(&qiov);
> +    return ret;
> +}
> +
>   static void collect_statistics(BlockDriverState *bs, BdrvCheckResult *res)
>   {
>       BDRVParallelsState *s = bs->opaque;
> @@ -598,6 +728,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>           goto out;
>       }
>   
> +    ret = check_duplicate(bs, res, fix);
> +    if (ret != 0) {
> +        goto out;
> +    }
> +
>       collect_statistics(bs, res);
>   
>   out:


-- 
Best regards,
Vladimir

Re: [PATCH v2 3/3] iotests, parallels: Add a test for duplicated clusters

[Vladimir Sementsov-Ogievskiy]

On 8/5/22 18:47, alexander.ivanov@virtuozzo.com wrote:
> From: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> 
> Check if original and duplicated offsets refer to the same cluster.
> Repair the image and check that writing to a referred cluster
> doesn't affects another referred cluster.
> 
> Signed-off-by: Natalia Kuzmina <natalia.kuzmina@openvz.org>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   tests/qemu-iotests/314                        |  89 ++++++++++++++++++

New tests should be added to tests/qemu-iotests/tests with good file names.

>   tests/qemu-iotests/314.out                    |  36 +++++++
>   .../parallels-2-duplicated-cluster.bz2        | Bin 0 -> 148 bytes
>   3 files changed, 125 insertions(+)
>   create mode 100755 tests/qemu-iotests/314
>   create mode 100644 tests/qemu-iotests/314.out
>   create mode 100644 tests/qemu-iotests/sample_images/parallels-2-duplicated-cluster.bz2
> 
> diff --git a/tests/qemu-iotests/314 b/tests/qemu-iotests/314
> new file mode 100755
> index 0000000000..79b4d3a749
> --- /dev/null
> +++ b/tests/qemu-iotests/314
> @@ -0,0 +1,89 @@
> +#!/usr/bin/env bash
> +# group: rw auto quick
> +#

[..]

> +
> +#read one cluster from original offset
> +$QEMU_IO -c "read -P 0x55 0 $CLUSTER_SIZE" "$TEST_IMG" | \
> +    _filter_qemu_io
> +#read copied data from new offset
> +$QEMU_IO -c "read -P 0x55 $((4 * CLUSTER_SIZE)) $CLUSTER_SIZE" "$TEST_IMG" | \
> +    _filter_qemu_io
> +#change data from original offset
> +$QEMU_IO -c "write -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG" | \
> +    _filter_qemu_io
> +#read from new offset (fail, now this data was left unchanged)
> +$QEMU_IO -c "read -P 0x11 $((4 * CLUSTER_SIZE)) $CLUSTER_SIZE" "$TEST_IMG" | \
> +    _filter_qemu_io

Maybe, add also read -P 0x55 for 4th cluster, to be sure that it's really unchanged.
And maybe read 0x11 for 0th cluster, to be sure that it's really rewritten.

> +
> +echo
> +echo
> +# success, all done
> +echo "*** done"
> +rm -f $seq.full
> +status=0

[..]

> zcmZ>Y%CIzaj8qGboOfsS0tPO%`U`(O5*Pv)I2hO&I2yDPt~od`068263_Exd7-leV
> zwiz(^Ft8k0sa3TsBZG0}Vv}35zt?O%VET5A+3Q2o4%bdpm~pLC^&`WR2CW6$VGH;&
> vm{u|@;OhXBE0|U>d|v){U)AOQJ)h70iu-<&;S?CYW~db}a<vGU0CEKYoE$uo
> 
> literal 0
> HcmV?d00001
> 

With at least new test renamed and moved to tests/qemu-iotests/tests:

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

-- 
Best regards,
Vladimir

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

[Denis V. Lunev]

On 06.08.2022 22:45, Vladimir Sementsov-Ogievskiy wrote:
> On 8/5/22 18:47, alexander.ivanov@virtuozzo.com wrote:
>> From: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>
>> There could be corruptions in the image file:
>> two guest memory areas refer to the same host cluster.
>
> Is that written in parallels spec (docs/interop/parallels.txt)?
>
> Hmm yes: "- the value must be unique among all BAT entries".
>
>>
>> If a duplicate offset is found fix it by copying the content
>> of the referred cluster to a new allocated cluster and
>> replace one of the two referring entries by the new cluster offset.
>>
>> Signed-off-by: Natalia Kuzmina <natalia.kuzmina@openvz.org>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>>   block/parallels.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 135 insertions(+)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index a0eb7ec3c3..73264b4bd1 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -64,6 +64,11 @@ static QEnumLookup prealloc_mode_lookup = {
>>   #define PARALLELS_OPT_PREALLOC_MODE     "prealloc-mode"
>>   #define PARALLELS_OPT_PREALLOC_SIZE     "prealloc-size"
>>   +#define REVERSED_BAT_UNTOUCHED  0xFFFFFFFF
>> +
>> +#define HOST_CLUSTER_INDEX(s, off) \
>> +    ((off - ((s->header->data_off) << BDRV_SECTOR_BITS)) / 
>> (s->cluster_size))
>
> Let it be a static function, not a macro.
>
>> +
>>   static QemuOptsList parallels_runtime_opts = {
>>       .name = "parallels",
>>       .head = QTAILQ_HEAD_INITIALIZER(parallels_runtime_opts.head),
>> @@ -559,6 +564,131 @@ static int check_leak(BlockDriverState *bs,
>>       return 0;
>>   }
>>   +static int check_duplicate(BlockDriverState *bs,
>> +                           BdrvCheckResult *res,
>> +                           BdrvCheckMode fix)
>> +{
>> +    BDRVParallelsState *s = bs->opaque;
>> +    QEMUIOVector qiov;
>> +    int64_t off, high_off, size, sector_num;
>> +    uint32_t i, idx_host;
>> +    int ret = 0, n;
>> +    g_autofree uint32_t *reversed_bat = NULL;
>> +    g_autofree int64_t *cluster_buf = NULL;
>> +    bool sync_and_truncate = false;
>> +
>> +    /*
>> +     * Make a reversed BAT.
>> +     * Each cluster in the host file could represent only one cluster
>> +     * from the guest point of view. Reversed BAT provides mapping 
>> of that type.
>> +     * Initially the table is filled with REVERSED_BAT_UNTOUCHED 
>> values.
>> +     */
>> +    reversed_bat = g_malloc(s->bat_size * sizeof(uint32_t));
>
> Hmm. Why size of reversed_bat equal to bat_size? Couldn't host file 
> size be larger than that?
>
> Seems that we want calculate the highest offset first, and then 
> allocate corresponding table.
>
> Also, here we probably allocate too much memory. Better use 
> g_try_malloc and clean error-out instead of crash.
>

This is incorrect. reversed_bat has __different__ size than BAT.
Technically we should take into account real file length and
use it as a size. Even if all blocks are allocated, the size of
the file is __greater__ than size of the payload by BAT + header
sizes.

But this is even trickier. The length could grow once we
copy duplicated clusters.

Also we are fucked up a bit with this way a little bit. We can have
first cluster just after the BAT (not aligned to cluster) or
with a padding, i.e. aligned to cluster size. This means that
data_off could be zero and non-zero.

I have missed this 3 times already :(

>> +    for (i = 0; i < s->bat_size; i++) {
>> +        reversed_bat[i] = REVERSED_BAT_UNTOUCHED;
>> +    }
>> +
>> +    cluster_buf = g_malloc(s->cluster_size);
>> +    qemu_iovec_init(&qiov, 0);
>> +    qemu_iovec_add(&qiov, cluster_buf, s->cluster_size);
>> +
>> +    high_off = 0;
>> +    for (i = 0; i < s->bat_size; i++) {
>> +        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
>> +        if (off == 0) {
>> +            continue;
>> +        }
>> +
>> +        if (off > high_off) {
>> +            high_off = off;
>> +        }
>> +
>> +        idx_host = HOST_CLUSTER_INDEX(s, off);
>> +        if (idx_host >= s->bat_size) {
>> +            res->check_errors++;
>
> As I understand, check_errors is mostly for IO errors during the check.
>
> If it's incorrect for parallels format to have such cluster, we want 
> res->corruptions++ here instead.
>
> But is it really incorrect, what the spec say?
>
Common sense says that if BAT points outside of the real
host file that cluster is really not existing and should be
read as zeroes. This is what and how we are fixing here.


>> +            goto out;
>> +        }
>> +
>> +        if (reversed_bat[idx_host] != REVERSED_BAT_UNTOUCHED) {
>> +            /*
>> +             * We have alreade written a guest cluster index for this
>
> already
>
>> +             * host cluster. It means there is a duplicated offset 
>> in BAT.
>> +             */
>> +            fprintf(stderr,
>> +                    "%s BAT offset in entry %u duplicates offset in 
>> entry %u\n",
>> +                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR",
>> +                    i, reversed_bat[idx_host]);
>> +            res->corruptions++;
>> +
>> +            if (fix & BDRV_FIX_ERRORS) {
>> +                /*
>> +                 * Write zero to the current BAT entry, allocate a 
>> new cluster
>> +                 * for the relevant guest offset and copy the host 
>> cluster
>> +                 * to the new allocated cluster.
>> +                 * In this way mwe will have two identical clusters 
>> and two
>> +                 * BAT entries with the offsets of these clusters.
>> +                 */
>> +                s->bat_bitmap[i] = 0;
>
> I don't understand that. So you just remove that guest cluster. 
> Shouldn't we instead set it to new allocated off?
>
we will write it again below and new offset will be allocated
properly within alloc_cluster()

>> +
>> +                sector_num = bat2sect(s, reversed_bat[idx_host]);
>> +                ret = bdrv_pread(bs->file, sector_num << 
>> BDRV_SECTOR_BITS,
>> +                                 s->cluster_size, cluster_buf, 0);
>> +                if (ret < 0) {
>> +                    res->check_errors++;
>> +                    goto out;
>> +                }
>> +
>> +                sector_num = (i * s->cluster_size) >> BDRV_SECTOR_BITS;
>> +                off = allocate_clusters(bs, sector_num, s->tracks, &n);
>
> you probably want to update high_off here
>
statistics should be calculated after all fixes. That should be more 
correct.


>> +                if (off < 0) {
>> +                    res->check_errors++;
>> +                    ret = off;
>> +                    goto out;
>> +                }
>> +                off <<= BDRV_SECTOR_BITS;
>> +
>> +                ret = bdrv_co_pwritev(bs->file, off, 
>> s->cluster_size, &qiov, 0);
>> +                if (ret < 0) {
>> +                    res->check_errors++;
>> +                    goto out;
>> +                }
>> +
>> +                /* off is new and we should repair idx_host 
>> accordingly. */
>> +                idx_host = HOST_CLUSTER_INDEX(s, off);
>> +                res->corruptions_fixed++;
>> +                sync_and_truncate = true;
>> +            }
>> +        }
>> +        reversed_bat[idx_host] = i;
>> +    }
>> +
>> +    if (sync_and_truncate) {
>> +        ret = sync_header(bs, res);
>> +        if (ret < 0) {
>> +            goto out;
>> +        }
>> +
>> +        size = bdrv_getlength(bs->file->bs);
>> +        if (size < 0) {
>> +            res->check_errors++;
>> +            ret = size;
>> +            goto out;
>> +        }
>> +
>> +        res->image_end_offset = high_off + s->cluster_size;
>> +        if (size > res->image_end_offset) {
>> +            ret = truncate_file(bs, res->image_end_offset);
>
> that's already done in check_leak, why we need it again?
>
>> +            if (ret < 0) {
>> +                goto out;
>> +            }
>> +        }
>> +    }
>> +
>> +out:
>> +    qemu_iovec_destroy(&qiov);
>> +    return ret;
>> +}
>> +
>>   static void collect_statistics(BlockDriverState *bs, 
>> BdrvCheckResult *res)
>>   {
>>       BDRVParallelsState *s = bs->opaque;
>> @@ -598,6 +728,11 @@ static int coroutine_fn 
>> parallels_co_check(BlockDriverState *bs,
>>           goto out;
>>       }
>>   +    ret = check_duplicate(bs, res, fix);
>> +    if (ret != 0) {
>> +        goto out;
>> +    }
>> +
>>       collect_statistics(bs, res);
>>     out:
>
>