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

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

[alexander.ivanov]

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

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.

Alexander Ivanov (3):
  parallels: Add checking and repairing duplicate offsets in BAT
  parallels: Let duplicates repairing pass without unwanted messages
  iotests, parallels: Add a test for duplicated clusters

 block/parallels.c                             | 112 ++++++++++++++++--
 tests/qemu-iotests/314                        |  88 ++++++++++++++
 tests/qemu-iotests/314.out                    |  36 ++++++
 .../parallels-2-duplicated-cluster.bz2        | Bin 0 -> 148 bytes
 4 files changed, 227 insertions(+), 9 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 1/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 quest 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 | 93 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 90 insertions(+), 3 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index a229c06f25..6a82942f38 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),
@@ -419,9 +424,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
                                            BdrvCheckMode fix)
 {
     BDRVParallelsState *s = bs->opaque;
-    int64_t size, prev_off, high_off;
-    int ret;
-    uint32_t i;
+    QEMUIOVector qiov;
+    int64_t size, prev_off, high_off, sector_num;
+    int ret, n;
+    uint32_t i, idx_host, *reversed_bat;
+    int64_t *cluster_buf;
     bool flush_bat = false;
 
     size = bdrv_getlength(bs->file->bs);
@@ -443,8 +450,31 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
     }
 
     res->bfi.total_clusters = s->bat_size;
+    res->bfi.allocated_clusters = 0;
     res->bfi.compressed_clusters = 0; /* compression is not supported */
 
+    cluster_buf = g_malloc(s->cluster_size);
+    qemu_iovec_init(&qiov, 0);
+    qemu_iovec_add(&qiov, cluster_buf, s->cluster_size);
+
+    /*
+     * Make a reversed BAT. The table has the same size as BAT.
+     * Initially the table is filled with REVERSED_BAT_UNTOUCHED values.
+     * A position in the table is defined by a host index
+     * (a number of a cluster in the data area):
+     *     index = (cluster_offset - data_area_offset) / cluster_size
+     * In the main loop fill the table with guest indexes
+     * (a number of entry in BAT).
+     * Before this, check if the relevant entry in the reversed table
+     * is REVERSED_BAT_UNTOUCHED. If that's not true, a guest index was
+     * written to the reversed table on a previous step.
+     * It means there is a duplicate offset.
+     */
+    reversed_bat = g_malloc(s->bat_size * sizeof(uint32_t));
+    for (i = 0; i < s->bat_size; i++) {
+        reversed_bat[i] = REVERSED_BAT_UNTOUCHED;
+    }
+
     high_off = 0;
     prev_off = 0;
     for (i = 0; i < s->bat_size; i++) {
@@ -468,6 +498,59 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
             }
         }
 
+        /* Checking bat entry uniqueness. */
+        idx_host = HOST_CLUSTER_INDEX(s, off);
+        if (reversed_bat[idx_host] != REVERSED_BAT_UNTOUCHED) {
+            /* 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) {
+                /* copy data to a new 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;
+                }
+
+                s->bat_bitmap[i] = 0;
+
+                sector_num = (i * s->cluster_size) >> BDRV_SECTOR_BITS;
+                off = allocate_clusters(bs, sector_num, s->tracks, &n);
+                if (off < 0) {
+                    res->check_errors++;
+                    goto out;
+                }
+                off <<= BDRV_SECTOR_BITS;
+
+                /* off is new and we should repair idx_host accordingly. */
+                idx_host = HOST_CLUSTER_INDEX(s, off);
+
+                ret = bdrv_co_pwritev(bs->file, off, s->cluster_size, &qiov, 0);
+                if (ret < 0) {
+                    res->check_errors++;
+                    goto out;
+                }
+
+                size = bdrv_getlength(bs->file->bs);
+                if (size < 0) {
+                    res->check_errors++;
+                    ret = size;
+                    goto out;
+                }
+
+                res->corruptions_fixed++;
+                flush_bat = true;
+            }
+        }
+        reversed_bat[idx_host] = i;
+
         res->bfi.allocated_clusters++;
         if (off > high_off) {
             high_off = off;
@@ -477,6 +560,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
             res->bfi.fragmented_clusters++;
         }
         prev_off = off;
+
     }
 
     ret = 0;
@@ -515,6 +599,9 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
     }
 
 out:
+    qemu_iovec_destroy(&qiov);
+    g_free(cluster_buf);
+    g_free(reversed_bat);
     qemu_co_mutex_unlock(&s->lock);
     return ret;
 }
-- 
2.34.1

[PATCH 2/3] parallels: Let duplicates repairing pass without unwanted messages

[alexander.ivanov]

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

When duplicates are repaired a new space area is allocated
and further leak check considers it as a leak.
Let fix it without printing any messages.

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

diff --git a/block/parallels.c b/block/parallels.c
index 6a82942f38..1f56ce26e4 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -429,7 +429,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
     int ret, n;
     uint32_t i, idx_host, *reversed_bat;
     int64_t *cluster_buf;
-    bool flush_bat = false;
+    bool flush_bat = false, truncate_silently = false;
 
     size = bdrv_getlength(bs->file->bs);
     if (size < 0) {
@@ -547,6 +547,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
 
                 res->corruptions_fixed++;
                 flush_bat = true;
+                truncate_silently = true;
             }
         }
         reversed_bat[idx_host] = i;
@@ -576,10 +577,13 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
     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 (!truncate_silently) {
+            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;
 
@@ -594,7 +598,10 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
                 res->check_errors++;
                 goto out;
             }
-            res->leaks_fixed += count;
+
+            if (!truncate_silently) {
+                res->leaks_fixed += count;
+            }
         }
     }
 
-- 
2.34.1

[PATCH 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                        |  88 ++++++++++++++++++
 tests/qemu-iotests/314.out                    |  36 +++++++
 .../parallels-2-duplicated-cluster.bz2        | Bin 0 -> 148 bytes
 3 files changed, 124 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..fdf47f86d4
--- /dev/null
+++ b/tests/qemu-iotests/314
@@ -0,0 +1,88 @@
+#!/usr/bin/env bash
+# group: rw auto quick
+#
+# Test qemu-img check on duplicated clusters
+#
+# Copyright (C) 2009 Red Hat, Inc.
+#
+# 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
+owner=natalia.kuzmina@openvz.org
+
+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"
+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 0/3] Check and repair duplicated clusters in parallels images

[Denis V. Lunev]

On 04.08.2022 16:51, alexander.ivanov@virtuozzo.com wrote:
> From: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>
> 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.
>
> Alexander Ivanov (3):
>    parallels: Add checking and repairing duplicate offsets in BAT
>    parallels: Let duplicates repairing pass without unwanted messages
>    iotests, parallels: Add a test for duplicated clusters
>
>   block/parallels.c                             | 112 ++++++++++++++++--
>   tests/qemu-iotests/314                        |  88 ++++++++++++++
>   tests/qemu-iotests/314.out                    |  36 ++++++
>   .../parallels-2-duplicated-cluster.bz2        | Bin 0 -> 148 bytes
>   4 files changed, 227 insertions(+), 9 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
>
the series itself should be marked as v2 indicating that this is based on
the original work of Natalia. Natalia should be in CC list.

Also list of CC persons is too small, pls run

iris ~/src/qemu $ ./scripts/get_maintainer.pl 
0001-parallels-Add-checking-and-repairing-duplicate-offse.patch
Stefan Hajnoczi <stefanha@redhat.com> (supporter:parallels)
"Denis V. Lunev" <den@openvz.org> (supporter:parallels)
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> 
(supporter:parallels)
Kevin Wolf <kwolf@redhat.com> (supporter:Block layer core)
Hanna Reitz <hreitz@redhat.com> (supporter:Block layer core)
qemu-block@nongnu.org (open list:parallels)
qemu-devel@nongnu.org (open list:All patches CC here)
iris ~/src/qemu $

Additionally, you should list of changes from previous version
at least in the main message.

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

[Denis V. Lunev]

On 04.08.2022 16:51, alexander.ivanov@virtuozzo.com wrote:
> From: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>
> There could be corruptions in the image file:
> two quest 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 | 93 +++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 90 insertions(+), 3 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index a229c06f25..6a82942f38 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),
> @@ -419,9 +424,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>                                              BdrvCheckMode fix)
>   {
>       BDRVParallelsState *s = bs->opaque;
> -    int64_t size, prev_off, high_off;
> -    int ret;
> -    uint32_t i;
> +    QEMUIOVector qiov;
> +    int64_t size, prev_off, high_off, sector_num;
> +    int ret, n;
> +    uint32_t i, idx_host, *reversed_bat;
> +    int64_t *cluster_buf;
>       bool flush_bat = false;
>   
>       size = bdrv_getlength(bs->file->bs);
> @@ -443,8 +450,31 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>       }
>   
>       res->bfi.total_clusters = s->bat_size;
> +    res->bfi.allocated_clusters = 0;
>       res->bfi.compressed_clusters = 0; /* compression is not supported */
>   
> +    cluster_buf = g_malloc(s->cluster_size);
> +    qemu_iovec_init(&qiov, 0);
> +    qemu_iovec_add(&qiov, cluster_buf, s->cluster_size);
> +
> +    /*
> +     * Make a reversed BAT. The table has the same size as BAT.
I would better use different wording. We need to define why the table is 
used
and what is idea behind it.

"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.
> +     * A position in the table is defined by a host index
> +     * (a number of a cluster in the data area):
> +     *     index = (cluster_offset - data_area_offset) / cluster_size
> +     * In the main loop fill the table with guest indexes
^^ indices
> +     * (a number of entry in BAT).
> +     * Before this, check if the relevant entry in the reversed table
> +     * is REVERSED_BAT_UNTOUCHED. If that's not true, a guest index was
> +     * written to the reversed table on a previous step.
> +     * It means there is a duplicate offset.
> +     */
> +    reversed_bat = g_malloc(s->bat_size * sizeof(uint32_t));
> +    for (i = 0; i < s->bat_size; i++) {
> +        reversed_bat[i] = REVERSED_BAT_UNTOUCHED;
> +    }
> +
>       high_off = 0;
>       prev_off = 0;
>       for (i = 0; i < s->bat_size; i++) {
> @@ -468,6 +498,59 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>               }
>           }
>   
> +        /* Checking bat entry uniqueness. */
> +        idx_host = HOST_CLUSTER_INDEX(s, off);
> +        if (reversed_bat[idx_host] != REVERSED_BAT_UNTOUCHED) {
> +            /* 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) {
> +                /* copy data to a new 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;
> +                }
> +
I'd add a comment here, this is a bit tricky thing, we have discussed this
verbally a lot.

> +                s->bat_bitmap[i] = 0;
> +
> +                sector_num = (i * s->cluster_size) >> BDRV_SECTOR_BITS;
> +                off = allocate_clusters(bs, sector_num, s->tracks, &n);
Should we do sanity check one more time? Fatal if that I believe.

> +                if (off < 0) {
> +                    res->check_errors++;
> +                    goto out;
> +                }
> +                off <<= BDRV_SECTOR_BITS;
> +
> +                /* off is new and we should repair idx_host accordingly. */
> +                idx_host = HOST_CLUSTER_INDEX(s, off);
> +
> +                ret = bdrv_co_pwritev(bs->file, off, s->cluster_size, &qiov, 0);
> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    goto out;
> +                }
> +
> +                size = bdrv_getlength(bs->file->bs);
> +                if (size < 0) {
> +                    res->check_errors++;
> +                    ret = size;
> +                    goto out;
> +                }
> +
> +                res->corruptions_fixed++;
> +                flush_bat = true;
> +            }
> +        }
> +        reversed_bat[idx_host] = i;
> +
>           res->bfi.allocated_clusters++;
>           if (off > high_off) {
>               high_off = off;
> @@ -477,6 +560,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>               res->bfi.fragmented_clusters++;
>           }
>           prev_off = off;
> +
please no unrelated changes

>       }
>   
>       ret = 0;
> @@ -515,6 +599,9 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>       }
>   
>   out:
> +    qemu_iovec_destroy(&qiov);
> +    g_free(cluster_buf);
> +    g_free(reversed_bat);
>       qemu_co_mutex_unlock(&s->lock);
>       return ret;
>   }

Re: [PATCH 2/3] parallels: Let duplicates repairing pass without unwanted messages

[Denis V. Lunev]

On 04.08.2022 16:51, alexander.ivanov@virtuozzo.com wrote:
> From: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>
> When duplicates are repaired a new space area is allocated
> and further leak check considers it as a leak.
> Let fix it without printing any messages.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 19 +++++++++++++------
>   1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 6a82942f38..1f56ce26e4 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -429,7 +429,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>       int ret, n;
>       uint32_t i, idx_host, *reversed_bat;
>       int64_t *cluster_buf;
> -    bool flush_bat = false;
> +    bool flush_bat = false, truncate_silently = false;
>   
>       size = bdrv_getlength(bs->file->bs);
>       if (size < 0) {
> @@ -547,6 +547,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>   
>                   res->corruptions_fixed++;
>                   flush_bat = true;
> +                truncate_silently = true;
>               }
>           }
>           reversed_bat[idx_host] = i;
> @@ -576,10 +577,13 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>       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 (!truncate_silently) {
> +            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;
>   
> @@ -594,7 +598,10 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>                   res->check_errors++;
>                   goto out;
>               }
> -            res->leaks_fixed += count;
> +
> +            if (!truncate_silently) {
> +                res->leaks_fixed += count;
> +            }
>           }
>       }
>   
This looks a little bit cumbersome. Yes, we need to make a check that we 
do not have
leaked at the end space, but it would be beneficial to make this check 
before we
have written to the image, in the other case we will lose that information.

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

[Denis V. Lunev]

On 04.08.2022 16:52, 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                        |  88 ++++++++++++++++++
>   tests/qemu-iotests/314.out                    |  36 +++++++
>   .../parallels-2-duplicated-cluster.bz2        | Bin 0 -> 148 bytes
>   3 files changed, 124 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..fdf47f86d4
> --- /dev/null
> +++ b/tests/qemu-iotests/314
> @@ -0,0 +1,88 @@
> +#!/usr/bin/env bash
> +# group: rw auto quick
> +#
> +# Test qemu-img check on duplicated clusters
for parallels format

> +#
> +# Copyright (C) 2009 Red Hat, Inc.
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
> +owner=natalia.kuzmina@openvz.org
should be either you or me as a maintener, not Natalia

I believe that we should create a place-holder for more upcoming repair 
tests,
we will add more and more. They are better to cover more and more cases.

> +
> +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"
> +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
>

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

[Denis V. Lunev]

On 04.08.2022 17:01, Denis V. Lunev wrote:
> On 04.08.2022 16:51, alexander.ivanov@virtuozzo.com wrote:
>> From: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>
>> 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.
>>
>> Alexander Ivanov (3):
>>    parallels: Add checking and repairing duplicate offsets in BAT
>>    parallels: Let duplicates repairing pass without unwanted messages
>>    iotests, parallels: Add a test for duplicated clusters
>>
>>   block/parallels.c                             | 112 ++++++++++++++++--
>>   tests/qemu-iotests/314                        |  88 ++++++++++++++
>>   tests/qemu-iotests/314.out                    |  36 ++++++
>>   .../parallels-2-duplicated-cluster.bz2        | Bin 0 -> 148 bytes
>>   4 files changed, 227 insertions(+), 9 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
>>
> the series itself should be marked as v2 indicating that this is based on
> the original work of Natalia. Natalia should be in CC list.
>
> Also list of CC persons is too small, pls run
>
> iris ~/src/qemu $ ./scripts/get_maintainer.pl 
> 0001-parallels-Add-checking-and-repairing-duplicate-offse.patch
> Stefan Hajnoczi <stefanha@redhat.com> (supporter:parallels)
> "Denis V. Lunev" <den@openvz.org> (supporter:parallels)
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> 
> (supporter:parallels)
> Kevin Wolf <kwolf@redhat.com> (supporter:Block layer core)
> Hanna Reitz <hreitz@redhat.com> (supporter:Block layer core)
> qemu-block@nongnu.org (open list:parallels)
> qemu-devel@nongnu.org (open list:All patches CC here)
> iris ~/src/qemu $
>
> Additionally, you should list of changes from previous version
> at least in the main message.
and generic note, to think.

We will add more and more checks. Should we start thinking on a better
code structure in parallels_co_check, f.e. we could performs each
check in a separate loop in separate helper and calculate statisctics
once all checks were done.

This is just a quick note, but I think it worth to keep this in mind.

Den