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

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

[Alexander Ivanov]

Fix incorrect data end calculation in parallels_open().

Check if data_end greater than the file size.

Add change_info argument to parallels_check_leak().

Add checking and repairing duplicate offsets in BAT

Image repairing in parallels_open().

v7:
3: Renamed "change_info" argument to "explicit", move info printing under
   explicit condition.
4: Different patch. Add data_start field to BDRVParallelsState for future
   host_cluster_index() function.
5: Prevously was 4th. Used s->data_start instead of s->header->data_off.
   Add bitmap size increasing if file length is not cluster aligned. Revert
   a couple conditions for better readability. Changed sectors calculation
   for better code transparency. Replaced sector variable by host_sector and
   guest_sector. Renamed off to host_off. Moved out_repare_bat: below return
   to get jumps on error path only.
6: Prevously was 5th.
7: New patch. Replace bdrv_getlength() by bdrv_co_getlength() in
   parallels_check_outside_image() because it is used in coroutine context.
8: New patch. Add data_off check.

v6:
2: Different patch. Refused to split image leak handling. Instead there is a
   patch with a data_end check.
3: Different patch. There is a patch with change_info argument.
4: Removed changing fprintf by qemu_log from this patchset. Previously 3rd
   patch became 4th. Replaced qemu_memalign() by qemu_blockalign(). Got
   rid of iovecs, replaced bdrv_co_pwritev() by bdrv_co_pwrite(). Added
   assert(cluster_index < bitmap_size). Now BAT changes are reverted if
   there was an error in the cluster copying process. Simplified a sector
   calculation.
5: Moved header magic check to the appropriate place. Added a
   migrate_del_blocker() call and s->bat_dirty_bmap freeing on error.

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

v4:
2,5: Rebased.

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

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


Alexander Ivanov (8):
  parallels: Incorrect data end calculation in parallels_open()
  parallels: Check if data_end greater than the file size
  parallels: Add "explicit" argument to parallels_check_leak()
  parallels: Add data_start field to BDRVParallelsState
  parallels: Add checking and repairing duplicate offsets in BAT
  parallels: Image repairing in parallels_open()
  parallels: Use bdrv_co_getlength() in parallels_check_outside_image()
  parallels: Add data_off check

 block/parallels.c | 317 +++++++++++++++++++++++++++++++++++++++-------
 block/parallels.h |   1 +
 2 files changed, 274 insertions(+), 44 deletions(-)

-- 
2.34.1

[PATCH v7 1/8] parallels: Incorrect data end calculation in parallels_open()

[Alexander Ivanov]

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

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

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

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

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

diff --git a/block/parallels.c b/block/parallels.c
index 18e34aef28..86bc3bfcb8 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -861,9 +861,9 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     }
     s->data_end = le32_to_cpu(ph.data_off);
     if (s->data_end == 0) {
-        s->data_end = ROUND_UP(bat_entry_off(s->bat_size), BDRV_SECTOR_SIZE);
+        s->data_end = DIV_ROUND_UP(size, BDRV_SECTOR_SIZE);
     }
-    if (s->data_end < s->header_size) {
+    if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
         /* there is not enough unused space to fit to block align between BAT
            and actual data. We can't avoid read-modify-write... */
         s->header_size = size;
-- 
2.34.1

[PATCH v7 2/8] parallels: Check if data_end greater than the file size

[Alexander Ivanov]

Initially data_end is set to the data_off image header field and must not be
greater than the file size.

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

diff --git a/block/parallels.c b/block/parallels.c
index 86bc3bfcb8..40a26908db 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -868,6 +868,11 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
            and actual data. We can't avoid read-modify-write... */
         s->header_size = size;
     }
+    if (s->data_end > file_nb_sectors) {
+        error_setg(errp, "Invalid image: incorrect data_off field");
+        ret = -EINVAL;
+        goto fail;
+    }
 
     ret = bdrv_pread(bs->file, 0, s->header_size, s->header, 0);
     if (ret < 0) {
-- 
2.34.1

Re: [PATCH v7 2/8] parallels: Check if data_end greater than the file size

[Denis V. Lunev]

On 7/1/23 12:07, Alexander Ivanov wrote:
> Initially data_end is set to the data_off image header field and must not be
should be truncated to 74 symbols

> greater than the file size.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 86bc3bfcb8..40a26908db 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -868,6 +868,11 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>              and actual data. We can't avoid read-modify-write... */
>           s->header_size = size;
>       }
> +    if (s->data_end > file_nb_sectors) {
> +        error_setg(errp, "Invalid image: incorrect data_off field");
> +        ret = -EINVAL;
> +        goto fail;
> +    }
>   
>       ret = bdrv_pread(bs->file, 0, s->header_size, s->header, 0);
>       if (ret < 0) {
With comment line truncated to 74 chars:

Reviewed-by: Denis V. Lunev <den@openvz.org>

[PATCH v7 3/8] parallels: Add "explicit" argument to parallels_check_leak()

[Alexander Ivanov]

In the on of the next patches we need to repair leaks without changing
leaks and leaks_fixed info in res. Also we don't want to print any warning
about leaks. Add "explicit" argument to skip info changing if the argument
is false.

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

diff --git a/block/parallels.c b/block/parallels.c
index 40a26908db..3cff25e3a4 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -484,7 +484,7 @@ parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res,
 
 static int coroutine_fn GRAPH_RDLOCK
 parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
-                     BdrvCheckMode fix)
+                     BdrvCheckMode fix, bool explicit)
 {
     BDRVParallelsState *s = bs->opaque;
     int64_t size;
@@ -499,10 +499,13 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
     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 (explicit) {
+            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;
 
@@ -517,7 +520,9 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
                 res->check_errors++;
                 return ret;
             }
-            res->leaks_fixed += count;
+            if (explicit) {
+                res->leaks_fixed += count;
+            }
         }
     }
 
@@ -570,7 +575,7 @@ parallels_co_check(BlockDriverState *bs, BdrvCheckResult *res,
             return ret;
         }
 
-        ret = parallels_check_leak(bs, res, fix);
+        ret = parallels_check_leak(bs, res, fix, true);
         if (ret < 0) {
             return ret;
         }
-- 
2.34.1

Re: [PATCH v7 3/8] parallels: Add "explicit" argument to parallels_check_leak()

[Denis V. Lunev]

On 7/1/23 12:07, Alexander Ivanov wrote:
> In the on of the next patches we need to repair leaks without changing
> leaks and leaks_fixed info in res. Also we don't want to print any warning
> about leaks. Add "explicit" argument to skip info changing if the argument
> is false.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 19 ++++++++++++-------
>   1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 40a26908db..3cff25e3a4 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -484,7 +484,7 @@ parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res,
>   
>   static int coroutine_fn GRAPH_RDLOCK
>   parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
> -                     BdrvCheckMode fix)
> +                     BdrvCheckMode fix, bool explicit)
>   {
>       BDRVParallelsState *s = bs->opaque;
>       int64_t size;
> @@ -499,10 +499,13 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
>       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 (explicit) {
> +            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;
>   
> @@ -517,7 +520,9 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
>                   res->check_errors++;
>                   return ret;
>               }
> -            res->leaks_fixed += count;
> +            if (explicit) {
> +                res->leaks_fixed += count;
> +            }
>           }
>       }
>   
> @@ -570,7 +575,7 @@ parallels_co_check(BlockDriverState *bs, BdrvCheckResult *res,
>               return ret;
>           }
>   
> -        ret = parallels_check_leak(bs, res, fix);
> +        ret = parallels_check_leak(bs, res, fix, true);
>           if (ret < 0) {
>               return ret;
>           }
Reviewed-by: Denis V. Lunev <den@openvz.org>

[PATCH v7 4/8] parallels: Add data_start field to BDRVParallelsState

[Alexander Ivanov]

In the next patch we will need the offset of the data area for host cluster
index calculation. Add this field and setting up code.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 7 ++++---
 block/parallels.h | 1 +
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 3cff25e3a4..374c9d17eb 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -864,10 +864,11 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         ret = -ENOMEM;
         goto fail;
     }
-    s->data_end = le32_to_cpu(ph.data_off);
-    if (s->data_end == 0) {
-        s->data_end = DIV_ROUND_UP(size, BDRV_SECTOR_SIZE);
+    s->data_start = le32_to_cpu(ph.data_off);
+    if (s->data_start == 0) {
+        s->data_start = DIV_ROUND_UP(size, BDRV_SECTOR_SIZE);
     }
+    s->data_end = s->data_start;
     if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
         /* there is not enough unused space to fit to block align between BAT
            and actual data. We can't avoid read-modify-write... */
diff --git a/block/parallels.h b/block/parallels.h
index f22f43f988..4e53e9572d 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -75,6 +75,7 @@ typedef struct BDRVParallelsState {
     uint32_t *bat_bitmap;
     unsigned int bat_size;
 
+    int64_t  data_start;
     int64_t  data_end;
     uint64_t prealloc_size;
     ParallelsPreallocMode prealloc_mode;
-- 
2.34.1

Re: [PATCH v7 4/8] parallels: Add data_start field to BDRVParallelsState

[Denis V. Lunev]

On 7/1/23 12:07, Alexander Ivanov wrote:
> In the next patch we will need the offset of the data area for host cluster
> index calculation. Add this field and setting up code.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 7 ++++---
>   block/parallels.h | 1 +
>   2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 3cff25e3a4..374c9d17eb 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -864,10 +864,11 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>           ret = -ENOMEM;
>           goto fail;
>       }
> -    s->data_end = le32_to_cpu(ph.data_off);
> -    if (s->data_end == 0) {
> -        s->data_end = DIV_ROUND_UP(size, BDRV_SECTOR_SIZE);
> +    s->data_start = le32_to_cpu(ph.data_off);
> +    if (s->data_start == 0) {
> +        s->data_start = DIV_ROUND_UP(size, BDRV_SECTOR_SIZE);
>       }
> +    s->data_end = s->data_start;
>       if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
>           /* there is not enough unused space to fit to block align between BAT
>              and actual data. We can't avoid read-modify-write... */
> diff --git a/block/parallels.h b/block/parallels.h
> index f22f43f988..4e53e9572d 100644
> --- a/block/parallels.h
> +++ b/block/parallels.h
> @@ -75,6 +75,7 @@ typedef struct BDRVParallelsState {
>       uint32_t *bat_bitmap;
>       unsigned int bat_size;
>   
> +    int64_t  data_start;
>       int64_t  data_end;
>       uint64_t prealloc_size;
>       ParallelsPreallocMode prealloc_mode;
Reviewed-by: Denis V. Lunev <den@openvz.org>

[PATCH v7 5/8] parallels: Add checking and repairing duplicate offsets in BAT

[Alexander Ivanov]

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

Add host_cluster_index() helper to deduplicate the code.

When new clusters are allocated, the file size increases by 128 Mb. Call
parallels_check_leak() to fix this leak.

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

diff --git a/block/parallels.c b/block/parallels.c
index 374c9d17eb..0f207c4b32 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -136,6 +136,12 @@ static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
     return MIN(nb_sectors, ret);
 }
 
+static uint32_t host_cluster_index(BDRVParallelsState *s, int64_t off)
+{
+    off -= s->data_start << BDRV_SECTOR_BITS;
+    return off / s->cluster_size;
+}
+
 static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
                             int nb_sectors, int *pnum)
 {
@@ -529,6 +535,139 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
     return 0;
 }
 
+static int coroutine_fn GRAPH_RDLOCK
+parallels_check_duplicate(BlockDriverState *bs, BdrvCheckResult *res,
+                          BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int64_t host_off, host_sector, guest_sector;
+    unsigned long *bitmap;
+    uint32_t i, bitmap_size, cluster_index, bat_entry;
+    int n, ret = 0;
+    uint64_t *buf = NULL;
+    bool fixed = false;
+
+    /*
+     * Create a bitmap of used clusters.
+     * If a bit is set, there is a BAT entry pointing to this cluster.
+     * Loop through the BAT entries, check bits relevant to an entry offset.
+     * If bit is set, this entry is duplicated. Otherwise set the bit.
+     *
+     * We shouldn't worry about newly allocated clusters outside the image
+     * because they are created higher then any existing cluster pointed by
+     * a BAT entry.
+     */
+    bitmap_size = host_cluster_index(s, res->image_end_offset);
+    if (bitmap_size == 0) {
+        return 0;
+    }
+    if (res->image_end_offset % s->cluster_size) {
+        /* A not aligned image end leads to a bitmap shorter by 1 */
+        bitmap_size++;
+    }
+
+    bitmap = bitmap_new(bitmap_size);
+
+    buf = qemu_blockalign(bs, s->cluster_size);
+
+    for (i = 0; i < s->bat_size; i++) {
+        host_off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+        if (host_off == 0) {
+            continue;
+        }
+
+        cluster_index = host_cluster_index(s, host_off);
+        assert(cluster_index < bitmap_size);
+        if (!test_bit(cluster_index, bitmap)) {
+            bitmap_set(bitmap, cluster_index, 1);
+            continue;
+        }
+
+        /* this cluster duplicates another one */
+        fprintf(stderr, "%s duplicate offset in BAT entry %u\n",
+                fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
+
+        res->corruptions++;
+
+        if (!(fix & BDRV_FIX_ERRORS)) {
+            continue;
+        }
+
+        /*
+         * Reset the entry and allocate a new cluster
+         * for the relevant guest offset. In this way we let
+         * the lower layer to place the new cluster properly.
+         * Copy the original cluster to the allocated one.
+         * But before save the old offset value for repairing
+         * if we have an error.
+         */
+        bat_entry = s->bat_bitmap[i];
+        parallels_set_bat_entry(s, i, 0);
+
+        ret = bdrv_co_pread(bs->file, host_off, s->cluster_size, buf, 0);
+        if (ret < 0) {
+            res->check_errors++;
+            goto out_repare_bat;
+        }
+
+        guest_sector = (i * (int64_t)s->cluster_size) >> BDRV_SECTOR_BITS;
+        host_sector = allocate_clusters(bs, guest_sector, s->tracks, &n);
+        if (host_sector < 0) {
+            res->check_errors++;
+            goto out_repare_bat;
+        }
+        host_off = host_sector << BDRV_SECTOR_BITS;
+
+        ret = bdrv_co_pwrite(bs->file, host_off, s->cluster_size, buf, 0);
+        if (ret < 0) {
+            res->check_errors++;
+            goto out_repare_bat;
+        }
+
+        if (host_off + s->cluster_size > res->image_end_offset) {
+            res->image_end_offset = host_off + s->cluster_size;
+        }
+
+        /*
+         * In the future allocate_cluster() will reuse holed offsets
+         * inside the image. Keep the used clusters bitmap content
+         * consistent for the new allocated clusters too.
+         *
+         * Note, clusters allocated outside the current image are not
+         * considered, and the bitmap size doesn't change.
+         */
+        cluster_index = host_cluster_index(s, host_off);
+        if (cluster_index < bitmap_size) {
+            bitmap_set(bitmap, cluster_index, 1);
+        }
+
+        fixed = true;
+        res->corruptions_fixed++;
+
+    }
+
+    if (fixed) {
+        /*
+         * When new clusters are allocated, the file size increases by
+         * 128 Mb. We need to truncate the file to the right size. Let
+         * the leak fix code make its job without res changing.
+         */
+        ret = parallels_check_leak(bs, res, fix, false);
+    }
+
+out_free:
+    g_free(buf);
+    g_free(bitmap);
+    return ret;
+/*
+ * We can get here only from places where index and old_offset have
+ * meaningful values.
+ */
+out_repare_bat:
+    s->bat_bitmap[i] = bat_entry;
+    goto out_free;
+}
+
 static void parallels_collect_statistics(BlockDriverState *bs,
                                          BdrvCheckResult *res,
                                          BdrvCheckMode fix)
@@ -580,6 +719,11 @@ parallels_co_check(BlockDriverState *bs, BdrvCheckResult *res,
             return ret;
         }
 
+        ret = parallels_check_duplicate(bs, res, fix);
+        if (ret < 0) {
+            return ret;
+        }
+
         parallels_collect_statistics(bs, res, fix);
     }
 
-- 
2.34.1

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

[Denis V. Lunev]

On 7/1/23 12:07, Alexander Ivanov wrote:
> Cluster offsets must be unique among all the BAT entries. Find duplicate
> offsets in the BAT and fix it by copying the content of the relevant
> cluster to a newly allocated cluster and set the new cluster offset to the
> duplicated entry.
>
> Add host_cluster_index() helper to deduplicate the code.
>
> When new clusters are allocated, the file size increases by 128 Mb. Call
> parallels_check_leak() to fix this leak.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 144 insertions(+)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 374c9d17eb..0f207c4b32 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -136,6 +136,12 @@ static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
>       return MIN(nb_sectors, ret);
>   }
>   
> +static uint32_t host_cluster_index(BDRVParallelsState *s, int64_t off)
> +{
> +    off -= s->data_start << BDRV_SECTOR_BITS;
> +    return off / s->cluster_size;
> +}
> +
>   static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
>                               int nb_sectors, int *pnum)
>   {
> @@ -529,6 +535,139 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
>       return 0;
>   }
>   
> +static int coroutine_fn GRAPH_RDLOCK
> +parallels_check_duplicate(BlockDriverState *bs, BdrvCheckResult *res,
> +                          BdrvCheckMode fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    int64_t host_off, host_sector, guest_sector;
> +    unsigned long *bitmap;
> +    uint32_t i, bitmap_size, cluster_index, bat_entry;
> +    int n, ret = 0;
> +    uint64_t *buf = NULL;
> +    bool fixed = false;
> +
> +    /*
> +     * Create a bitmap of used clusters.
> +     * If a bit is set, there is a BAT entry pointing to this cluster.
> +     * Loop through the BAT entries, check bits relevant to an entry offset.
> +     * If bit is set, this entry is duplicated. Otherwise set the bit.
> +     *
> +     * We shouldn't worry about newly allocated clusters outside the image
> +     * because they are created higher then any existing cluster pointed by
> +     * a BAT entry.
> +     */
> +    bitmap_size = host_cluster_index(s, res->image_end_offset);
> +    if (bitmap_size == 0) {
> +        return 0;
> +    }
> +    if (res->image_end_offset % s->cluster_size) {
> +        /* A not aligned image end leads to a bitmap shorter by 1 */
> +        bitmap_size++;
> +    }
> +
> +    bitmap = bitmap_new(bitmap_size);
> +
> +    buf = qemu_blockalign(bs, s->cluster_size);
> +
> +    for (i = 0; i < s->bat_size; i++) {
> +        host_off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> +        if (host_off == 0) {
> +            continue;
> +        }
> +
> +        cluster_index = host_cluster_index(s, host_off);
> +        assert(cluster_index < bitmap_size);
> +        if (!test_bit(cluster_index, bitmap)) {
> +            bitmap_set(bitmap, cluster_index, 1);
> +            continue;
> +        }
> +
> +        /* this cluster duplicates another one */
> +        fprintf(stderr, "%s duplicate offset in BAT entry %u\n",
> +                fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
> +
> +        res->corruptions++;
> +
> +        if (!(fix & BDRV_FIX_ERRORS)) {
> +            continue;
> +        }
> +
> +        /*
> +         * Reset the entry and allocate a new cluster
> +         * for the relevant guest offset. In this way we let
> +         * the lower layer to place the new cluster properly.
> +         * Copy the original cluster to the allocated one.
> +         * But before save the old offset value for repairing
> +         * if we have an error.
> +         */
> +        bat_entry = s->bat_bitmap[i];
> +        parallels_set_bat_entry(s, i, 0);
> +
> +        ret = bdrv_co_pread(bs->file, host_off, s->cluster_size, buf, 0);
> +        if (ret < 0) {
> +            res->check_errors++;
> +            goto out_repare_bat;
here and below we should use out_repair_bat.
> +        }
> +
> +        guest_sector = (i * (int64_t)s->cluster_size) >> BDRV_SECTOR_BITS;
> +        host_sector = allocate_clusters(bs, guest_sector, s->tracks, &n);
> +        if (host_sector < 0) {
> +            res->check_errors++;
> +            goto out_repare_bat;
> +        }
> +        host_off = host_sector << BDRV_SECTOR_BITS;
> +
> +        ret = bdrv_co_pwrite(bs->file, host_off, s->cluster_size, buf, 0);
> +        if (ret < 0) {
> +            res->check_errors++;
> +            goto out_repare_bat;
> +        }
> +
> +        if (host_off + s->cluster_size > res->image_end_offset) {
> +            res->image_end_offset = host_off + s->cluster_size;
> +        }
> +
> +        /*
> +         * In the future allocate_cluster() will reuse holed offsets
> +         * inside the image. Keep the used clusters bitmap content
> +         * consistent for the new allocated clusters too.
> +         *
> +         * Note, clusters allocated outside the current image are not
> +         * considered, and the bitmap size doesn't change.
> +         */
> +        cluster_index = host_cluster_index(s, host_off);
> +        if (cluster_index < bitmap_size) {
> +            bitmap_set(bitmap, cluster_index, 1);
> +        }
> +
> +        fixed = true;
> +        res->corruptions_fixed++;
> +
> +    }
> +
> +    if (fixed) {
> +        /*
> +         * When new clusters are allocated, the file size increases by
> +         * 128 Mb. We need to truncate the file to the right size. Let
> +         * the leak fix code make its job without res changing.
> +         */
> +        ret = parallels_check_leak(bs, res, fix, false);
> +    }
> +
> +out_free:
> +    g_free(buf);
> +    g_free(bitmap);
> +    return ret;
> +/*
> + * We can get here only from places where index and old_offset have
> + * meaningful values.
> + */
> +out_repare_bat:
> +    s->bat_bitmap[i] = bat_entry;
> +    goto out_free;
> +}
> +
>   static void parallels_collect_statistics(BlockDriverState *bs,
>                                            BdrvCheckResult *res,
>                                            BdrvCheckMode fix)
> @@ -580,6 +719,11 @@ parallels_co_check(BlockDriverState *bs, BdrvCheckResult *res,
>               return ret;
>           }
>   
> +        ret = parallels_check_duplicate(bs, res, fix);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +
>           parallels_collect_statistics(bs, res, fix);
>       }
>   
with old_repare_bat replaced with old_repair_bat:

Reviewed-by: Denis V. Lunev <den@openvz.org>

[PATCH v7 6/8] parallels: Image repairing in parallels_open()

[Alexander Ivanov]

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

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

diff --git a/block/parallels.c b/block/parallels.c
index 0f207c4b32..51fd8ddf5a 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -947,7 +947,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVParallelsState *s = bs->opaque;
     ParallelsHeader ph;
     int ret, size, i;
-    int64_t file_nb_sectors;
+    int64_t file_nb_sectors, sector;
     QemuOpts *opts = NULL;
     Error *local_err = NULL;
     char *buf;
@@ -1018,11 +1018,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
            and actual data. We can't avoid read-modify-write... */
         s->header_size = size;
     }
-    if (s->data_end > file_nb_sectors) {
-        error_setg(errp, "Invalid image: incorrect data_off field");
-        ret = -EINVAL;
-        goto fail;
-    }
 
     ret = bdrv_pread(bs->file, 0, s->header_size, s->header, 0);
     if (ret < 0) {
@@ -1030,33 +1025,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     }
     s->bat_bitmap = (uint32_t *)(s->header + 1);
 
-    for (i = 0; i < s->bat_size; i++) {
-        int64_t off = bat2sect(s, i);
-        if (off >= file_nb_sectors) {
-            if (flags & BDRV_O_CHECK) {
-                continue;
-            }
-            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
-                       "is larger than file size (%" PRIi64 ")",
-                       off << BDRV_SECTOR_BITS, i,
-                       file_nb_sectors << BDRV_SECTOR_BITS);
-            ret = -EINVAL;
-            goto fail;
-        }
-        if (off >= s->data_end) {
-            s->data_end = off + s->tracks;
-        }
-    }
-
     if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
-        /* Image was not closed correctly. The check is mandatory */
         s->header_unclean = true;
-        if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_CHECK)) {
-            error_setg(errp, "parallels: Image was not closed correctly; "
-                       "cannot be opened read/write");
-            ret = -EACCES;
-            goto fail;
-        }
     }
 
     opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
@@ -1117,10 +1087,40 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
                bdrv_get_device_or_node_name(bs));
     ret = migrate_add_blocker(s->migration_blocker, errp);
     if (ret < 0) {
-        error_free(s->migration_blocker);
+        error_setg(errp, "Migration blocker error");
         goto fail;
     }
     qemu_co_mutex_init(&s->lock);
+
+    for (i = 0; i < s->bat_size; i++) {
+        sector = bat2sect(s, i);
+        if (sector + s->tracks > s->data_end) {
+            s->data_end = sector + s->tracks;
+        }
+    }
+
+    /*
+     * We don't repair the image here if it's opened for checks. Also we don't
+     * want to change inactive images and can't change readonly images.
+     */
+    if ((flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) || !(flags & BDRV_O_RDWR)) {
+        return 0;
+    }
+
+    /*
+     * Repair the image if it's dirty or
+     * out-of-image corruption was detected.
+     */
+    if (s->data_end > file_nb_sectors || s->header_unclean) {
+        BdrvCheckResult res;
+        ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Could not repair corrupted image");
+            migrate_del_blocker(s->migration_blocker);
+            goto fail;
+        }
+    }
+
     return 0;
 
 fail_format:
@@ -1128,6 +1128,12 @@ fail_format:
 fail_options:
     ret = -EINVAL;
 fail:
+    /*
+     * "s" object was allocated by g_malloc0 so we can safely
+     * try to free its fields even they were not allocated.
+     */
+    error_free(s->migration_blocker);
+    g_free(s->bat_dirty_bmap);
     qemu_vfree(s->header);
     return ret;
 }
-- 
2.34.1

Re: [PATCH v7 6/8] parallels: Image repairing in parallels_open()

[Denis V. Lunev]

On 7/1/23 12:07, Alexander Ivanov wrote:
> Repair an image at opening if the image is unclean or out-of-image
> corruption was detected.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 70 +++++++++++++++++++++++++----------------------
>   1 file changed, 38 insertions(+), 32 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 0f207c4b32..51fd8ddf5a 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -947,7 +947,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>       BDRVParallelsState *s = bs->opaque;
>       ParallelsHeader ph;
>       int ret, size, i;
> -    int64_t file_nb_sectors;
> +    int64_t file_nb_sectors, sector;
>       QemuOpts *opts = NULL;
>       Error *local_err = NULL;
>       char *buf;
> @@ -1018,11 +1018,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>              and actual data. We can't avoid read-modify-write... */
>           s->header_size = size;
>       }
> -    if (s->data_end > file_nb_sectors) {
> -        error_setg(errp, "Invalid image: incorrect data_off field");
> -        ret = -EINVAL;
> -        goto fail;
> -    }
>   
>       ret = bdrv_pread(bs->file, 0, s->header_size, s->header, 0);
>       if (ret < 0) {
> @@ -1030,33 +1025,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>       }
>       s->bat_bitmap = (uint32_t *)(s->header + 1);
>   
> -    for (i = 0; i < s->bat_size; i++) {
> -        int64_t off = bat2sect(s, i);
> -        if (off >= file_nb_sectors) {
> -            if (flags & BDRV_O_CHECK) {
> -                continue;
> -            }
> -            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
> -                       "is larger than file size (%" PRIi64 ")",
> -                       off << BDRV_SECTOR_BITS, i,
> -                       file_nb_sectors << BDRV_SECTOR_BITS);
> -            ret = -EINVAL;
> -            goto fail;
> -        }
> -        if (off >= s->data_end) {
> -            s->data_end = off + s->tracks;
> -        }
> -    }
> -
>       if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> -        /* Image was not closed correctly. The check is mandatory */
>           s->header_unclean = true;
> -        if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_CHECK)) {
> -            error_setg(errp, "parallels: Image was not closed correctly; "
> -                       "cannot be opened read/write");
> -            ret = -EACCES;
> -            goto fail;
> -        }
>       }
>   
>       opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
> @@ -1117,10 +1087,40 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>                  bdrv_get_device_or_node_name(bs));
>       ret = migrate_add_blocker(s->migration_blocker, errp);
>       if (ret < 0) {
> -        error_free(s->migration_blocker);
> +        error_setg(errp, "Migration blocker error");
>           goto fail;
>       }
>       qemu_co_mutex_init(&s->lock);
> +
> +    for (i = 0; i < s->bat_size; i++) {
> +        sector = bat2sect(s, i);
> +        if (sector + s->tracks > s->data_end) {
> +            s->data_end = sector + s->tracks;
> +        }
> +    }
> +
> +    /*
> +     * We don't repair the image here if it's opened for checks. Also we don't
> +     * want to change inactive images and can't change readonly images.
> +     */
> +    if ((flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) || !(flags & BDRV_O_RDWR)) {
> +        return 0;
> +    }
> +
> +    /*
> +     * Repair the image if it's dirty or
> +     * out-of-image corruption was detected.
> +     */
> +    if (s->data_end > file_nb_sectors || s->header_unclean) {
> +        BdrvCheckResult res;
> +        ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "Could not repair corrupted image");
> +            migrate_del_blocker(s->migration_blocker);
> +            goto fail;
> +        }
> +    }
> +
>       return 0;
>   
>   fail_format:
> @@ -1128,6 +1128,12 @@ fail_format:
>   fail_options:
>       ret = -EINVAL;
>   fail:
> +    /*
> +     * "s" object was allocated by g_malloc0 so we can safely
> +     * try to free its fields even they were not allocated.
> +     */
> +    error_free(s->migration_blocker);
> +    g_free(s->bat_dirty_bmap);
>       qemu_vfree(s->header);
>       return ret;
>   }
Reviewed-by: Denis V. Lunev <den@openvz.org>

[PATCH v7 7/8] parallels: Use bdrv_co_getlength() in parallels_check_outside_image()

[Alexander Ivanov]

bdrv_co_getlength() should be used in coroutine context. Replace
bdrv_getlength() by bdrv_co_getlength() in parallels_check_outside_image().

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 block/parallels.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/parallels.c b/block/parallels.c
index 51fd8ddf5a..456a13bd28 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -496,7 +496,7 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
     int64_t size;
     int ret;
 
-    size = bdrv_getlength(bs->file->bs);
+    size = bdrv_co_getlength(bs->file->bs);
     if (size < 0) {
         res->check_errors++;
         return size;
-- 
2.34.1

Re: [PATCH v7 7/8] parallels: Use bdrv_co_getlength() in parallels_check_outside_image()

[Denis V. Lunev]

On 7/1/23 12:07, Alexander Ivanov wrote:
> bdrv_co_getlength() should be used in coroutine context. Replace
> bdrv_getlength() by bdrv_co_getlength() in parallels_check_outside_image().
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 51fd8ddf5a..456a13bd28 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -496,7 +496,7 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
>       int64_t size;
>       int ret;
>   
> -    size = bdrv_getlength(bs->file->bs);
> +    size = bdrv_co_getlength(bs->file->bs);
>       if (size < 0) {
>           res->check_errors++;
>           return size;
Reviewed-by: Denis V. Lunev <den@openvz.org>

[PATCH v7 8/8] parallels: Add data_off check

[Alexander Ivanov]

data_off field of the parallels image header can be corrupted. Check if
this field greater than the header + BAT size and less than file size.
Change checking code in parallels_open() accordingly.

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

diff --git a/block/parallels.c b/block/parallels.c
index 456a13bd28..51e79056df 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -446,6 +446,65 @@ static void parallels_check_unclean(BlockDriverState *bs,
     }
 }
 
+/*
+ * Returns 0 if data_off is correct or returns correct offset.
+ */
+static uint32_t parallels_test_data_off(BDRVParallelsState *s,
+                                        int64_t file_nb_sectors)
+{
+    uint32_t data_off, min_off;
+    bool old_magic;
+
+    old_magic = !memcmp(s->header->magic, HEADER_MAGIC, 16);
+
+    data_off = le32_to_cpu(s->header->data_off);
+    if (data_off == 0 && old_magic) {
+        return 0;
+    }
+
+    min_off = DIV_ROUND_UP(bat_entry_off(s->bat_size), BDRV_SECTOR_SIZE);
+    if (!old_magic) {
+        min_off = ROUND_UP(min_off, s->cluster_size / BDRV_SECTOR_SIZE);
+    }
+
+    if (data_off >= min_off && data_off <= file_nb_sectors) {
+        return 0;
+    }
+
+    return min_off;
+}
+
+static int coroutine_fn GRAPH_RDLOCK
+parallels_check_data_off(BlockDriverState *bs, BdrvCheckResult *res,
+                         BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int64_t file_size;
+    uint32_t data_off;
+
+    file_size = bdrv_co_nb_sectors(bs->file->bs);
+    if (file_size < 0) {
+        res->check_errors++;
+        return file_size;
+    }
+
+    data_off = parallels_test_data_off(s, file_size);
+    if (data_off == 0) {
+        return 0;
+    }
+
+    res->corruptions++;
+    if (fix & BDRV_FIX_ERRORS) {
+        s->header->data_off = cpu_to_le32(data_off);
+        res->corruptions_fixed++;
+    }
+
+    fprintf(stderr, "%s data_off field has incorrect value\n",
+            fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
+
+    return 0;
+}
+
 static int coroutine_fn GRAPH_RDLOCK
 parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res,
                               BdrvCheckMode fix)
@@ -709,6 +768,11 @@ parallels_co_check(BlockDriverState *bs, BdrvCheckResult *res,
     WITH_QEMU_LOCK_GUARD(&s->lock) {
         parallels_check_unclean(bs, res, fix);
 
+        ret = parallels_check_data_off(bs, res, fix);
+        if (ret < 0) {
+            return ret;
+        }
+
         ret = parallels_check_outside_image(bs, res, fix);
         if (ret < 0) {
             return ret;
@@ -947,7 +1011,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVParallelsState *s = bs->opaque;
     ParallelsHeader ph;
     int ret, size, i;
-    int64_t file_nb_sectors, sector;
+    int64_t file_nb_sectors, sector, new_data_start;
     QemuOpts *opts = NULL;
     Error *local_err = NULL;
     char *buf;
@@ -1008,9 +1072,22 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         ret = -ENOMEM;
         goto fail;
     }
-    s->data_start = le32_to_cpu(ph.data_off);
-    if (s->data_start == 0) {
-        s->data_start = DIV_ROUND_UP(size, BDRV_SECTOR_SIZE);
+
+    ret = bdrv_pread(bs->file, 0, s->header_size, s->header, 0);
+    if (ret < 0) {
+        goto fail;
+    }
+    s->bat_bitmap = (uint32_t *)(s->header + 1);
+
+    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
+        s->header_unclean = true;
+    }
+
+    new_data_start = parallels_test_data_off(s, file_nb_sectors);
+    if (new_data_start == 0) {
+        s->data_start = le32_to_cpu(ph.data_off);
+    } else {
+        s->data_start = new_data_start;
     }
     s->data_end = s->data_start;
     if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
@@ -1019,16 +1096,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         s->header_size = size;
     }
 
-    ret = bdrv_pread(bs->file, 0, s->header_size, s->header, 0);
-    if (ret < 0) {
-        goto fail;
-    }
-    s->bat_bitmap = (uint32_t *)(s->header + 1);
-
-    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
-        s->header_unclean = true;
-    }
-
     opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
     if (!opts) {
         goto fail_options;
@@ -1111,7 +1178,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
      * Repair the image if it's dirty or
      * out-of-image corruption was detected.
      */
-    if (s->data_end > file_nb_sectors || s->header_unclean) {
+    if (s->data_end > file_nb_sectors || s->header_unclean
+        || new_data_start != 0) {
         BdrvCheckResult res;
         ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
         if (ret < 0) {
-- 
2.34.1

Re: [PATCH v7 8/8] parallels: Add data_off check

[Denis V. Lunev]

On 7/1/23 12:07, Alexander Ivanov wrote:
> data_off field of the parallels image header can be corrupted. Check if
> this field greater than the header + BAT size and less than file size.
> Change checking code in parallels_open() accordingly.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
This patch requires a bit more work, unfortunately.

After 'git am' it renders a bit differently as follows:

@@ -1012,18 +1076,6 @@ static int parallels_open(BlockDriverState *bs, 
QDict *options, int flags,
          ret = -ENOMEM;
          goto fail;
      }
-    s->data_start = le32_to_cpu(ph.data_off);
-    if (s->data_start == 0) {
-        s->data_start = DIV_ROUND_UP(size, BDRV_SECTOR_SIZE);
-    }
-    s->data_end = s->data_start;
-    if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
-        /*
-         * There is not enough unused space to fit to block align 
between BAT
-         * and actual data. We can't avoid read-modify-write...
-         */
-        s->header_size = size;
-    }

      ret = bdrv_pread(bs->file, 0, s->header_size, s->header, 0);
      if (ret < 0) {
@@ -1035,6 +1087,21 @@ static int parallels_open(BlockDriverState *bs, 
QDict *options, int flags,
          s->header_unclean = true;
      }

+    new_data_start = parallels_test_data_off(s, file_nb_sectors);
+    if (new_data_start == 0) {
+        s->data_start = le32_to_cpu(ph.data_off);
+    } else {
+        s->data_start = new_data_start;
+    }
+    s->data_end = s->data_start;
+    if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
+        /*
+         * There is not enough unused space to fit to block align 
between BAT
+         * and actual data. We can't avoid read-modify-write...
+         */
+        s->header_size = size;
+    }
+
      opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
      if (!opts) {
          goto fail_options;
@@ -1117,7 +1184,8 @@ static

I would say that we need to
1. move s->data_start/s->data_end calculations below as a separate patch
2. create patch which will change formatting of comments inside 
block/parallels.c
     in order to avoid checkpatch errors.


> ---
>   block/parallels.c | 98 +++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 83 insertions(+), 15 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 456a13bd28..51e79056df 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -446,6 +446,65 @@ static void parallels_check_unclean(BlockDriverState *bs,
>       }
>   }
>   
> +/*
> + * Returns 0 if data_off is correct or returns correct offset.
> + */
> +static uint32_t parallels_test_data_off(BDRVParallelsState *s,
> +                                        int64_t file_nb_sectors)
> +{
> +    uint32_t data_off, min_off;
> +    bool old_magic;
> +
> +    old_magic = !memcmp(s->header->magic, HEADER_MAGIC, 16);
> +
> +    data_off = le32_to_cpu(s->header->data_off);
> +    if (data_off == 0 && old_magic) {
this condition is unclear for the unprepared reader
and we need to provide extensive comment with
the reference to the documentation.

> +        return 0;
> +    }
> +
> +    min_off = DIV_ROUND_UP(bat_entry_off(s->bat_size), BDRV_SECTOR_SIZE);
> +    if (!old_magic) {
> +        min_off = ROUND_UP(min_off, s->cluster_size / BDRV_SECTOR_SIZE);
> +    }
> +
> +    if (data_off >= min_off && data_off <= file_nb_sectors) {
> +        return 0;
> +    }
> +
> +    return min_off;
> +}
> +
> +static int coroutine_fn GRAPH_RDLOCK
> +parallels_check_data_off(BlockDriverState *bs, BdrvCheckResult *res,
> +                         BdrvCheckMode fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    int64_t file_size;
> +    uint32_t data_off;
> +
> +    file_size = bdrv_co_nb_sectors(bs->file->bs);
> +    if (file_size < 0) {
> +        res->check_errors++;
> +        return file_size;
> +    }
> +
> +    data_off = parallels_test_data_off(s, file_size);
> +    if (data_off == 0) {
> +        return 0;
> +    }
> +
> +    res->corruptions++;
> +    if (fix & BDRV_FIX_ERRORS) {
> +        s->header->data_off = cpu_to_le32(data_off);
> +        res->corruptions_fixed++;
> +    }
> +
> +    fprintf(stderr, "%s data_off field has incorrect value\n",
> +            fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
> +
> +    return 0;
> +}
> +
>   static int coroutine_fn GRAPH_RDLOCK
>   parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res,
>                                 BdrvCheckMode fix)
> @@ -709,6 +768,11 @@ parallels_co_check(BlockDriverState *bs, BdrvCheckResult *res,
>       WITH_QEMU_LOCK_GUARD(&s->lock) {
>           parallels_check_unclean(bs, res, fix);
>   
> +        ret = parallels_check_data_off(bs, res, fix);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +
>           ret = parallels_check_outside_image(bs, res, fix);
>           if (ret < 0) {
>               return ret;
> @@ -947,7 +1011,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>       BDRVParallelsState *s = bs->opaque;
>       ParallelsHeader ph;
>       int ret, size, i;
> -    int64_t file_nb_sectors, sector;
> +    int64_t file_nb_sectors, sector, new_data_start;
>       QemuOpts *opts = NULL;
>       Error *local_err = NULL;
>       char *buf;
> @@ -1008,9 +1072,22 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>           ret = -ENOMEM;
>           goto fail;
>       }
> -    s->data_start = le32_to_cpu(ph.data_off);
> -    if (s->data_start == 0) {
> -        s->data_start = DIV_ROUND_UP(size, BDRV_SECTOR_SIZE);
> +
> +    ret = bdrv_pread(bs->file, 0, s->header_size, s->header, 0);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +    s->bat_bitmap = (uint32_t *)(s->header + 1);
> +
> +    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> +        s->header_unclean = true;
> +    }
> +
> +    new_data_start = parallels_test_data_off(s, file_nb_sectors);
> +    if (new_data_start == 0) {
> +        s->data_start = le32_to_cpu(ph.data_off);
> +    } else {
> +        s->data_start = new_data_start;
>       }
>       s->data_end = s->data_start;
>       if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
> @@ -1019,16 +1096,6 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>           s->header_size = size;
>       }
>   
> -    ret = bdrv_pread(bs->file, 0, s->header_size, s->header, 0);
> -    if (ret < 0) {
> -        goto fail;
> -    }
> -    s->bat_bitmap = (uint32_t *)(s->header + 1);
> -
> -    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> -        s->header_unclean = true;
> -    }
> -
>       opts = qemu_opts_create(&parallels_runtime_opts, NULL, 0, errp);
>       if (!opts) {
>           goto fail_options;
> @@ -1111,7 +1178,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>        * Repair the image if it's dirty or
>        * out-of-image corruption was detected.
>        */
> -    if (s->data_end > file_nb_sectors || s->header_unclean) {
> +    if (s->data_end > file_nb_sectors || s->header_unclean
> +        || new_data_start != 0) {
>           BdrvCheckResult res;
>           ret = bdrv_check(bs, &res, BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
>           if (ret < 0) {