[PATCH 0/3] Repair duplicated clusters in parallels image

[PATCH 0/3] Repair duplicated clusters in parallels image

[Natalia Kuzmina]

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.

Natalia Kuzmina (3):
  qemu-img check: fixing duplicated clusters for parallels format
  iotests: 314 test on duplicated clusters (parallels format)
  docs: parallels image format supports consistency checks

 block/parallels.c                             |  66 ++++++++++++-
 docs/tools/qemu-img.rst                       |   2 +-
 tests/qemu-iotests/314                        |  88 ++++++++++++++++++
 tests/qemu-iotests/314.out                    |  36 +++++++
 .../parallels-2-duplicated-cluster.bz2        | Bin 0 -> 148 bytes
 5 files changed, 189 insertions(+), 3 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.25.1

[PATCH 1/3] qemu-img check: fixing duplicated clusters for parallels format

[Natalia Kuzmina]

Let qemu-img check fix corruption in the image file: two
guest memory areas refer to the same host memory area
(duplicated offsets in BAT).

Signed-off-by: Natalia Kuzmina <natalia.kuzmina@openvz.org>
---
 block/parallels.c | 66 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 64 insertions(+), 2 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 6ebad2a2bb..6a73933d45 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -418,9 +418,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
                                            BdrvCheckMode fix)
 {
     BDRVParallelsState *s = bs->opaque;
-    int64_t size, prev_off, high_off;
+    int64_t size, prev_off, high_off, idx_host, sector_num;
     int ret;
     uint32_t i;
+    int64_t *buf;
+    int *reversed_bat;
     bool flush_bat = false;
 
     size = bdrv_getlength(bs->file->bs);
@@ -442,8 +444,14 @@ 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 */
 
+    reversed_bat = g_malloc(s->bat_size * sizeof(int));
+    for (i = 0; i < s->bat_size; i++) {
+        reversed_bat[i] = -1;
+    }
+
     high_off = 0;
     prev_off = 0;
     for (i = 0; i < s->bat_size; i++) {
@@ -453,6 +461,59 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
             continue;
         }
 
+        /* checking bat entry uniqueness */
+        idx_host = (off - ((s->header->data_off) << BDRV_SECTOR_BITS))
+            / (s->cluster_size);
+        if (reversed_bat[idx_host] != -1) { /* duplicated cluster */
+            fprintf(stderr, "%s cluster %u is duplicated (with cluster %u)\n",
+                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR",
+                    i, reversed_bat[idx_host]);
+            res->corruptions++;
+            res->bfi.allocated_clusters--; /* not to count this cluster twice */
+            if (fix & BDRV_FIX_ERRORS) {
+                /* copy data to new cluster */
+                sector_num = bat2sect(s, reversed_bat[idx_host]);
+                buf = g_malloc(s->cluster_size);
+                ret = bdrv_pread(bs->file, sector_num << BDRV_SECTOR_BITS,
+                                 buf, s->cluster_size);
+                if (ret < 0) {
+                    res->check_errors++;
+                    g_free(buf);
+                    goto out;
+                }
+
+                ret = bdrv_pwrite(bs->file, s->data_end << BDRV_SECTOR_BITS,
+                                  buf, s->cluster_size);
+                if (ret < 0) {
+                    res->check_errors++;
+                    g_free(buf);
+                    goto out;
+                }
+
+                s->bat_bitmap[i] = cpu_to_le32(s->data_end / s->off_multiplier);
+                s->data_end += s->tracks;
+                bitmap_set(s->bat_dirty_bmap,
+                           bat_entry_off(i) / s->bat_dirty_block, 1);
+                g_free(buf);
+
+                res->corruptions_fixed++;
+                flush_bat = true;
+
+                /* these values are invalid after repairing */
+                off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+                idx_host = (off - ((s->header->data_off) << BDRV_SECTOR_BITS))
+                    / (s->cluster_size);
+                size = bdrv_getlength(bs->file->bs);
+                if (size < 0) {
+                    res->check_errors++;
+                    ret = size;
+                    goto out;
+                }
+            }
+        }
+
+        reversed_bat[idx_host] = i;
+
         /* cluster outside the image */
         if (off > size) {
             fprintf(stderr, "%s cluster %u is outside image\n",
@@ -472,7 +533,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
             high_off = off;
         }
 
-        if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
+        if (prev_off != 0 && (off - prev_off) % s->cluster_size != 0) {
             res->bfi.fragmented_clusters++;
         }
         prev_off = off;
@@ -514,6 +575,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
     }
 
 out:
+    g_free(reversed_bat);
     qemu_co_mutex_unlock(&s->lock);
     return ret;
 }
-- 
2.25.1

[PATCH 2/3] iotests: 314 test on duplicated clusters (parallels format)

[Natalia Kuzmina]

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.

Signed-off-by: Natalia Kuzmina <natalia.kuzmina@openvz.org>
---
 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..167b75d1af
--- /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..efb138e6a1
--- /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 cluster 4 is duplicated (with cluster 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.25.1

[PATCH 3/3] docs: parallels image format supports consistency checks

[Natalia Kuzmina]

Add parallels to list of formats that support consistency
checks by qemu-img check.

Signed-off-by: Natalia Kuzmina <natalia.kuzmina@openvz.org>
---
 docs/tools/qemu-img.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index 8885ea11cf..14e98df34f 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -332,7 +332,7 @@ Command description:
   ``-r all`` fixes all kinds of errors, with a higher risk of choosing the
   wrong fix or hiding corruption that has already occurred.
 
-  Only the formats ``qcow2``, ``qed`` and ``vdi`` support
+  Only the formats ``qcow2``, ``qed``, ``vdi`` and ``parallels`` support
   consistency checks.
 
   In case the image does not have any inconsistencies, check exits with ``0``.
-- 
2.25.1

Re: [PATCH 3/3] docs: parallels image format supports consistency checks

[Denis V. Lunev]

On 18.04.2022 14:04, Natalia Kuzmina wrote:
> Add parallels to list of formats that support consistency
> checks by qemu-img check.
>
> Signed-off-by: Natalia Kuzmina <natalia.kuzmina@openvz.org>
> ---
>   docs/tools/qemu-img.rst | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index 8885ea11cf..14e98df34f 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -332,7 +332,7 @@ Command description:
>     ``-r all`` fixes all kinds of errors, with a higher risk of choosing the
>     wrong fix or hiding corruption that has already occurred.
>   
> -  Only the formats ``qcow2``, ``qed`` and ``vdi`` support
> +  Only the formats ``qcow2``, ``qed``, ``vdi`` and ``parallels`` support
>     consistency checks.
>   
>     In case the image does not have any inconsistencies, check exits with ``0``.
This one has already been landed mainstream

https://marc.info/?l=qemu-devel&m=164994729129548&w=4

Den

Re: [PATCH 1/3] qemu-img check: fixing duplicated clusters for parallels format

[Denis V. Lunev]

On 18.04.2022 14:04, Natalia Kuzmina wrote:
> Let qemu-img check fix corruption in the image file: two
> guest memory areas refer to the same host memory area
> (duplicated offsets in BAT).
The code below requires big fat comment, what is reversed BAT,
why it is needed and how it helps us to detect the corruption.
You have spent more than a month writing the code. It would
be very good to spend 1 hour to write detailed comment.

> Signed-off-by: Natalia Kuzmina <natalia.kuzmina@openvz.org>
> ---
>   block/parallels.c | 66 +++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 64 insertions(+), 2 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 6ebad2a2bb..6a73933d45 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -418,9 +418,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>                                              BdrvCheckMode fix)
>   {
>       BDRVParallelsState *s = bs->opaque;
> -    int64_t size, prev_off, high_off;
> +    int64_t size, prev_off, high_off, idx_host, sector_num;
>       int ret;
>       uint32_t i;
> +    int64_t *buf;
> +    int *reversed_bat;
>       bool flush_bat = false;
>   
>       size = bdrv_getlength(bs->file->bs);
> @@ -442,8 +444,14 @@ 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 */
>   
> +    reversed_bat = g_malloc(s->bat_size * sizeof(int));
> +    for (i = 0; i < s->bat_size; i++) {
> +        reversed_bat[i] = -1;
> +    }
> +
>       high_off = 0;
>       prev_off = 0;
>       for (i = 0; i < s->bat_size; i++) {
> @@ -453,6 +461,59 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>               continue;
>           }
>   
> +        /* checking bat entry uniqueness */
> +        idx_host = (off - ((s->header->data_off) << BDRV_SECTOR_BITS))
> +            / (s->cluster_size);

> +        if (reversed_bat[idx_host] != -1) { /* duplicated cluster */
> +            fprintf(stderr, "%s cluster %u is duplicated (with cluster %u)\n",
> +                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR",
> +                    i, reversed_bat[idx_host]);
> +            res->corruptions++;
> +            res->bfi.allocated_clusters--; /* not to count this cluster twice */
that seems wrong. Duplicated cluster after the fix would be present
twice, with one location and with another one.
If this is correct, detailed explanation is needed.

> +            if (fix & BDRV_FIX_ERRORS) {
> +                /* copy data to new cluster */
> +                sector_num = bat2sect(s, reversed_bat[idx_host]);
> +                buf = g_malloc(s->cluster_size);
why not to allocate this buffer once at the beginning of the function?
Error handling would be simplear or use approach below.

> +                ret = bdrv_pread(bs->file, sector_num << BDRV_SECTOR_BITS,
> +                                 buf, s->cluster_size);
> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    g_free(buf);
> +                    goto out;
> +                }
> +

would it be sane to just zero BAT entry and call
                                   bdrv_pwritev(bs, sector_num << 
BDRV_SECTOR_BITS,  buf, s->cluster_size);
and avoid error prone dances with the internals?
Once the operation is done you could start the operation for the same 
block from the beginning.

> +                ret = bdrv_pwrite(bs->file, s->data_end << BDRV_SECTOR_BITS,
> +                                  buf, s->cluster_size);
> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    g_free(buf);
> +                    goto out;
> +                }
> +
> +                s->bat_bitmap[i] = cpu_to_le32(s->data_end / s->off_multiplier);
> +                s->data_end += s->tracks;
> +                bitmap_set(s->bat_dirty_bmap,
> +                           bat_entry_off(i) / s->bat_dirty_block, 1);
> +                g_free(buf);
> +
> +                res->corruptions_fixed++;
> +                flush_bat = true;
> +
> +                /* these values are invalid after repairing */
> +                off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> +                idx_host = (off - ((s->header->data_off) << BDRV_SECTOR_BITS))
> +                    / (s->cluster_size);
> +                size = bdrv_getlength(bs->file->bs);
> +                if (size < 0) {
> +                    res->check_errors++;
> +                    ret = size;
> +                    goto out;
> +                }
> +            }
> +        }
> +
> +        reversed_bat[idx_host] = i;
> +
>           /* cluster outside the image */
>           if (off > size) {
>               fprintf(stderr, "%s cluster %u is outside image\n",
> @@ -472,7 +533,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>               high_off = off;
>           }
>   
> -        if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
> +        if (prev_off != 0 && (off - prev_off) % s->cluster_size != 0) {
>               res->bfi.fragmented_clusters++;
>           }
for me this change is not a part of the commit. Should it be present here
or be moved into the separate commit?

>           prev_off = off;
> @@ -514,6 +575,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>       }
>   
>   out:
> +    g_free(reversed_bat);
>       qemu_co_mutex_unlock(&s->lock);
>       return ret;
>   }

Re: [PATCH 2/3] iotests: 314 test on duplicated clusters (parallels format)

[Stefan Hajnoczi]

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

On Mon, Apr 18, 2022 at 02:04:29PM +0300, Natalia Kuzmina wrote:
> 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.
> 
> Signed-off-by: Natalia Kuzmina <natalia.kuzmina@openvz.org>
> ---
>  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

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]