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

[PATCH v6 0/5] 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().

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 (5):
  parallels: Incorrect data end calculation in parallels_open()
  parallels: Check if data_end greater than the file size
  parallels: Add change_info argument to parallels_check_leak()
  parallels: Add checking and repairing duplicate offsets in BAT
  parallels: Image repairing in parallels_open()

 block/parallels.c | 228 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 195 insertions(+), 33 deletions(-)

-- 
2.34.1

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

[Alexander Ivanov]

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

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

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

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

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
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 7c263d5085..1ec98c722b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -861,9 +861,9 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     }
     s->data_end = le32_to_cpu(ph.data_off);
     if (s->data_end == 0) {
-        s->data_end = ROUND_UP(bat_entry_off(s->bat_size), BDRV_SECTOR_SIZE);
+        s->data_end = DIV_ROUND_UP(size, BDRV_SECTOR_SIZE);
     }
-    if (s->data_end < s->header_size) {
+    if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
         /* there is not enough unused space to fit to block align between BAT
            and actual data. We can't avoid read-modify-write... */
         s->header_size = size;
-- 
2.34.1

[PATCH v6 2/5] 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 1ec98c722b..4b7eafba1e 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 v6 2/5] parallels: Check if data_end greater than the file size

[Denis V. Lunev]

On 6/21/23 10:20, Alexander Ivanov wrote:
> 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 1ec98c722b..4b7eafba1e 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) {
for me this seems incorrect. In this case we would not be able to EVER
open such image, even for a check purpose.

This place should have a clause like

             if (!(flags & BDRV_O_CHECK)) {
                 goto fail
             }

This error is not fatal from the check point of view.

Den

[PATCH v6 3/5] parallels: Add change_info argument to parallels_check_leak()

[Alexander Ivanov]

In the next patch we need to repair leaks without changing leaks and
leaks_fixed info in res. Add change_info argument to skip info changing
if the argument is false.

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

diff --git a/block/parallels.c b/block/parallels.c
index 4b7eafba1e..78a34bd667 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 change_info)
 {
     BDRVParallelsState *s = bs->opaque;
     int64_t size;
@@ -502,7 +502,9 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
         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 (change_info) {
+            res->leaks += count;
+        }
         if (fix & BDRV_FIX_LEAKS) {
             Error *local_err = NULL;
 
@@ -517,7 +519,9 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
                 res->check_errors++;
                 return ret;
             }
-            res->leaks_fixed += count;
+            if (change_info) {
+                res->leaks_fixed += count;
+            }
         }
     }
 
@@ -570,7 +574,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

[PATCH v6 4/5] parallels: Add checking and repairing duplicate offsets in BAT

[Alexander Ivanov]

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

Add host_cluster_index() helper to deduplicate the code.

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 | 142 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 142 insertions(+)

diff --git a/block/parallels.c b/block/parallels.c
index 78a34bd667..d507fe7d90 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -136,6 +136,12 @@ static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
     return MIN(nb_sectors, ret);
 }
 
+static uint32_t host_cluster_index(BDRVParallelsState *s, int64_t off)
+{
+    off -= le32_to_cpu(s->header->data_off) << BDRV_SECTOR_BITS;
+    return off / s->cluster_size;
+}
+
 static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
                             int nb_sectors, int *pnum)
 {
@@ -528,6 +534,137 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
     return 0;
 }
 
+static int parallels_check_duplicate(BlockDriverState *bs,
+                                     BdrvCheckResult *res,
+                                     BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int64_t off, sector;
+    unsigned long *bitmap;
+    uint32_t i, bitmap_size, cluster_index, old_offset;
+    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;
+    }
+
+    bitmap = bitmap_new(bitmap_size);
+
+    buf = qemu_blockalign(bs, s->cluster_size);
+
+    for (i = 0; i < s->bat_size; i++) {
+        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+        if (off == 0) {
+            continue;
+        }
+
+        cluster_index = host_cluster_index(s, off);
+        assert(cluster_index < bitmap_size);
+        if (test_bit(cluster_index, bitmap)) {
+            /* this cluster duplicates another one */
+            fprintf(stderr,
+                    "%s duplicate offset in BAT entry %u\n",
+                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
+
+            res->corruptions++;
+
+            if (fix & BDRV_FIX_ERRORS) {
+                /*
+                 * Reset the entry and allocate a new cluster
+                 * for the relevant guest offset. In this way we let
+                 * the lower layer to place the new cluster properly.
+                 * Copy the original cluster to the allocated one.
+                 * But before save the old offset value for repairing
+                 * if we have an error.
+                 */
+                old_offset = le32_to_cpu(s->bat_bitmap[i]);
+                parallels_set_bat_entry(s, i, 0);
+
+                ret = bdrv_co_pread(bs->file, off, s->cluster_size, buf, 0);
+                if (ret < 0) {
+                    res->check_errors++;
+                    goto out_repare_bat;
+                }
+
+                sector = i * (int64_t)s->tracks;
+                sector = allocate_clusters(bs, sector, s->tracks, &n);
+                if (sector < 0) {
+                    res->check_errors++;
+                    ret = sector;
+                    goto out_repare_bat;
+                }
+                off = sector << BDRV_SECTOR_BITS;
+
+                ret = bdrv_co_pwrite(bs->file, off, s->cluster_size, buf, 0);
+                if (ret < 0) {
+                    res->check_errors++;
+                    goto out_repare_bat;
+                }
+
+                if (off + s->cluster_size > res->image_end_offset) {
+                    res->image_end_offset = off + s->cluster_size;
+                }
+
+                /*
+                 * In the future allocate_cluster() will reuse holed offsets
+                 * inside the image. Keep the used clusters bitmap content
+                 * consistent for the new allocated clusters too.
+                 *
+                 * Note, clusters allocated outside the current image are not
+                 * considered, and the bitmap size doesn't change.
+                 */
+                cluster_index = host_cluster_index(s, off);
+                if (cluster_index < bitmap_size) {
+                    bitmap_set(bitmap, cluster_index, 1);
+                }
+
+                fixed = true;
+                res->corruptions_fixed++;
+            }
+        } else {
+            bitmap_set(bitmap, cluster_index, 1);
+        }
+    }
+
+    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);
+        if (ret < 0) {
+            goto out;
+        }
+    }
+    goto out;
+
+/*
+ * We can get here only from places where index and old_offset have
+ * meaningful values.
+ */
+out_repare_bat:
+    s->bat_bitmap[i] = cpu_to_le32(old_offset);
+
+out:
+    g_free(buf);
+    g_free(bitmap);
+    return ret;
+}
+
 static void parallels_collect_statistics(BlockDriverState *bs,
                                          BdrvCheckResult *res,
                                          BdrvCheckMode fix)
@@ -579,6 +716,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 v6 4/5] parallels: Add checking and repairing duplicate offsets in BAT

[Denis V. Lunev]

On 6/21/23 10:20, 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 | 142 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 142 insertions(+)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 78a34bd667..d507fe7d90 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -136,6 +136,12 @@ static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
>       return MIN(nb_sectors, ret);
>   }
>   
> +static uint32_t host_cluster_index(BDRVParallelsState *s, int64_t off)
> +{
> +    off -= le32_to_cpu(s->header->data_off) << BDRV_SECTOR_BITS;
According to


> +    return off / s->cluster_size;
> +}
> +
>   static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
>                               int nb_sectors, int *pnum)
>   {
> @@ -528,6 +534,137 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
>       return 0;
>   }
>   
> +static int parallels_check_duplicate(BlockDriverState *bs,
> +                                     BdrvCheckResult *res,
> +                                     BdrvCheckMode fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    int64_t off, sector;
> +    unsigned long *bitmap;
> +    uint32_t i, bitmap_size, cluster_index, old_offset;
> +    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;
> +    }
> +
> +    bitmap = bitmap_new(bitmap_size);
> +
> +    buf = qemu_blockalign(bs, s->cluster_size);
> +
> +    for (i = 0; i < s->bat_size; i++) {
> +        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> +        if (off == 0) {
> +            continue;
> +        }
> +
> +        cluster_index = host_cluster_index(s, off);
> +        assert(cluster_index < bitmap_size);
> +        if (test_bit(cluster_index, bitmap)) {
> +            /* this cluster duplicates another one */
> +            fprintf(stderr,
> +                    "%s duplicate offset in BAT entry %u\n",
> +                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
> +
> +            res->corruptions++;
> +
> +            if (fix & BDRV_FIX_ERRORS) {
> +                /*
> +                 * Reset the entry and allocate a new cluster
> +                 * for the relevant guest offset. In this way we let
> +                 * the lower layer to place the new cluster properly.
> +                 * Copy the original cluster to the allocated one.
> +                 * But before save the old offset value for repairing
> +                 * if we have an error.
> +                 */
> +                old_offset = le32_to_cpu(s->bat_bitmap[i]);
> +                parallels_set_bat_entry(s, i, 0);
> +
> +                ret = bdrv_co_pread(bs->file, off, s->cluster_size, buf, 0);
> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    goto out_repare_bat;
> +                }
> +
> +                sector = i * (int64_t)s->tracks;
> +                sector = allocate_clusters(bs, sector, s->tracks, &n);
> +                if (sector < 0) {
> +                    res->check_errors++;
> +                    ret = sector;
> +                    goto out_repare_bat;
> +                }
> +                off = sector << BDRV_SECTOR_BITS;
> +
> +                ret = bdrv_co_pwrite(bs->file, off, s->cluster_size, buf, 0);
> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    goto out_repare_bat;
> +                }
> +
> +                if (off + s->cluster_size > res->image_end_offset) {
> +                    res->image_end_offset = off + s->cluster_size;
> +                }
> +
> +                /*
> +                 * In the future allocate_cluster() will reuse holed offsets
> +                 * inside the image. Keep the used clusters bitmap content
> +                 * consistent for the new allocated clusters too.
> +                 *
> +                 * Note, clusters allocated outside the current image are not
> +                 * considered, and the bitmap size doesn't change.
> +                 */
> +                cluster_index = host_cluster_index(s, off);
> +                if (cluster_index < bitmap_size) {
> +                    bitmap_set(bitmap, cluster_index, 1);
> +                }
> +
> +                fixed = true;
> +                res->corruptions_fixed++;
> +            }
> +        } else {
> +            bitmap_set(bitmap, cluster_index, 1);
> +        }
> +    }
> +
> +    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);
> +        if (ret < 0) {
> +            goto out;
> +        }
> +    }
> +    goto out;
> +
> +/*
> + * We can get here only from places where index and old_offset have
> + * meaningful values.
> + */
> +out_repare_bat:
> +    s->bat_bitmap[i] = cpu_to_le32(old_offset);
> +
> +out:
> +    g_free(buf);
> +    g_free(bitmap);
> +    return ret;
> +}
> +
>   static void parallels_collect_statistics(BlockDriverState *bs,
>                                            BdrvCheckResult *res,
>                                            BdrvCheckMode fix)
> @@ -579,6 +716,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);
>       }
>   

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

[Denis V. Lunev]

On 6/21/23 10:20, 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 | 142 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 142 insertions(+)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 78a34bd667..d507fe7d90 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -136,6 +136,12 @@ static int cluster_remainder(BDRVParallelsState *s, int64_t sector_num,
>       return MIN(nb_sectors, ret);
>   }
>   
> +static uint32_t host_cluster_index(BDRVParallelsState *s, int64_t off)
> +{
> +    off -= le32_to_cpu(s->header->data_off) << BDRV_SECTOR_BITS;
According to parallels_open()
     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);
     }
     if (s->data_end < s->header_size) {
         /* 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;
     }
data_off can be 0. In that case the logic of conversion of
host offset is incorrect.

This does not break THIS code, but by the name of this
helper further code reuse could lead to mistakes in some
cases.

Side note. This function could result in a bitmap which would
be shorter by 1 if file length is not cluster aligned. There are
no such checks.

Side note2. It would be nice to validate data_off in advance
as follows from the specification. In all cases data_off should
be greater than bat_entry_off(s->bat_size) if non-zero.

   48 - 51:    data_off
               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 BAT
                 table plus some padding to ensure sector size alignment.
               - If data_off is non-zero, the offset should be aligned 
to sector
                 size. However it is recommended to align it to cluster 
size for
                 newly created images.

               For "WithouFreSpacExt" images:
               data_off must be non-zero and aligned to cluster size.

> +    return off / s->cluster_size;
> +}
> +
>   static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
>                               int nb_sectors, int *pnum)
>   {
> @@ -528,6 +534,137 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
>       return 0;
>   }
>   
> +static int parallels_check_duplicate(BlockDriverState *bs,
> +                                     BdrvCheckResult *res,
> +                                     BdrvCheckMode fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    int64_t off, sector;
> +    unsigned long *bitmap;
> +    uint32_t i, bitmap_size, cluster_index, old_offset;
> +    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;
> +    }
> +
> +    bitmap = bitmap_new(bitmap_size);
> +
> +    buf = qemu_blockalign(bs, s->cluster_size);
> +
> +    for (i = 0; i < s->bat_size; i++) {
> +        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> +        if (off == 0) {
> +            continue;
> +        }
> +
> +        cluster_index = host_cluster_index(s, off);
> +        assert(cluster_index < bitmap_size);
> +        if (test_bit(cluster_index, bitmap)) {
I would prefer to revert this if and next one and have a code like

if (test_bit(cluster_index, bitmap)) {
    
    bitmap_set(bitmap, cluster_index, 1);
    continue;
}
....
if (!(fix & BDRV_FIX_ERRORS)) {
    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) {
> +                /*
> +                 * 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.
> +                 */
> +                old_offset = le32_to_cpu(s->bat_bitmap[i]);
> +                parallels_set_bat_entry(s, i, 0);
> +
> +                ret = bdrv_co_pread(bs->file, off, s->cluster_size, buf, 0);
> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    goto out_repare_bat;
> +                }
> +
> +                sector = i * (int64_t)s->tracks;
This is somehow counter intuitive. I vole for
     sector  = (i * s->cluster_size) >> BDRV_SECTOR_BITS;
This would be transparent for the rest of the code.

Anyway, we use tracks in other places. The question is
just about readability. I would not insist too much :)

> +                sector = allocate_clusters(bs, sector, s->tracks, &n);
It is better to avoid such code. There are "guest_sector" and "host_sector".
I would say that they should not be mixed. It is too dangerous.

Actually the same applies to the 'off' variable.

> +                if (sector < 0) {
> +                    res->check_errors++;
> +                    ret = sector;
> +                    goto out_repare_bat;
repair

> +                }
> +                off = sector << BDRV_SECTOR_BITS;
> +
> +                ret = bdrv_co_pwrite(bs->file, off, s->cluster_size, buf, 0);
> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    goto out_repare_bat;
> +                }
> +
> +                if (off + s->cluster_size > res->image_end_offset) {
> +                    res->image_end_offset = off + s->cluster_size;
> +                }
> +
> +                /*
> +                 * In the future allocate_cluster() will reuse holed offsets
> +                 * inside the image. Keep the used clusters bitmap content
> +                 * consistent for the new allocated clusters too.
> +                 *
> +                 * Note, clusters allocated outside the current image are not
> +                 * considered, and the bitmap size doesn't change.
> +                 */
> +                cluster_index = host_cluster_index(s, off);
> +                if (cluster_index < bitmap_size) {
> +                    bitmap_set(bitmap, cluster_index, 1);
> +                }
> +
> +                fixed = true;
> +                res->corruptions_fixed++;
> +            }
> +        } else {
> +            bitmap_set(bitmap, cluster_index, 1);
> +        }
> +    }
> +
> +    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);
> +        if (ret < 0) {
> +            goto out;
> +        }
> +    }
> +    goto out;
> +
> +/*
> + * We can get here only from places where index and old_offset have
> + * meaningful values.
> + */
> +out_repare_bat:
> +    s->bat_bitmap[i] = cpu_to_le32(old_offset);
it would be better to move out_repare_bat: below return to get jumps on 
error path only.
> +
> +out:
> +    g_free(buf);
> +    g_free(bitmap);
> +    return ret;
> +}
> +
>   static void parallels_collect_statistics(BlockDriverState *bs,
>                                            BdrvCheckResult *res,
>                                            BdrvCheckMode fix)
> @@ -579,6 +716,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);
>       }
>   

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

[Alexander Ivanov]

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

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

diff --git a/block/parallels.c b/block/parallels.c
index d507fe7d90..dc7eb6375e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -944,7 +944,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;
@@ -1026,33 +1026,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);
@@ -1113,10 +1088,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:
@@ -1124,6 +1129,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 v6 5/5] parallels: Image repairing in parallels_open()

[Denis V. Lunev]

On 6/21/23 10:20, Alexander Ivanov wrote:
> Repair an image at opening if the image is unclean or out-of-image
> corruption was detected.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 65 +++++++++++++++++++++++++++--------------------
>   1 file changed, 38 insertions(+), 27 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index d507fe7d90..dc7eb6375e 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -944,7 +944,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;
> @@ -1026,33 +1026,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);
> @@ -1113,10 +1088,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:
> @@ -1124,6 +1129,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>

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

[Denis V. Lunev]

On 6/21/23 10:20, Alexander Ivanov wrote:
> 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().
>
> 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 (5):
>    parallels: Incorrect data end calculation in parallels_open()
>    parallels: Check if data_end greater than the file size
>    parallels: Add change_info argument to parallels_check_leak()
>    parallels: Add checking and repairing duplicate offsets in BAT
>    parallels: Image repairing in parallels_open()
>
>   block/parallels.c | 228 +++++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 195 insertions(+), 33 deletions(-)
>
could you pls also include tests for this functionality for the next 
iteration?

We have had this series some time ago but it was lost