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

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

[Alexander Ivanov]

This patchset is based on
git: https://src.openvz.org/~den/qemu.git parallels

Fix incorrect data end calculation in parallels_open().

Add parallels_handle_leak() and highest_offset() helpers.

Add checking and repairing duplicate offsets in BAT.

Deduplicate highest offset calculation.

Replace fprintf() by qemu_log().

Add check and repair to parallels_open().


Alexander Ivanov (6):
  parallels: Incorrect data end calculation in parallels_open()
  parallels: Create parallels_handle_leak() to truncate excess clusters
  parallels: Add checking and repairing duplicate offsets in BAT
  parallels: Use highest_offset() helper in leak check
  parallels: Replace fprintf by qemu_log
  parallels: Image repairing in parallels_open()

 block/parallels.c | 297 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 241 insertions(+), 56 deletions(-)

-- 
2.34.1

[PATCH 1/6] 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>
---
 block/parallels.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index e6e8b9e369..7b4e997ebd 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -863,9 +863,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

Re: [PATCH 1/6] parallels: Incorrect data end calculation in parallels_open()

[Denis V. Lunev]

On 9/2/22 10:52, Alexander Ivanov wrote:
> 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>
> ---
>   block/parallels.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index e6e8b9e369..7b4e997ebd 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -863,9 +863,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;
Reviewed-by: Denis V. Lunev <den@openvz.org>

Sidenote: if the image is truncated for more than BAT size and there is 
no data inside
the image, we would get memory corruption during BAT access. That should 
be addressed
separately.

[PATCH 2/6] parallels: Create parallels_handle_leak() to truncate excess clusters

[Alexander Ivanov]

This helper will be reused in the next patch for duplications check.

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

diff --git a/block/parallels.c b/block/parallels.c
index 7b4e997ebd..dbcaf5d310 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -475,37 +475,23 @@ static int parallels_check_outside_image(BlockDriverState *bs,
     return 0;
 }
 
-static int parallels_check_leak(BlockDriverState *bs,
-                                BdrvCheckResult *res,
-                                BdrvCheckMode fix)
+static int64_t parallels_handle_leak(BlockDriverState *bs,
+                                     BdrvCheckResult *res,
+                                     int64_t high_off,
+                                     bool fix)
 {
     BDRVParallelsState *s = bs->opaque;
-    int64_t size, off, high_off, count;
-    uint32_t i;
+    int64_t size;
     int ret;
 
     size = bdrv_getlength(bs->file->bs);
     if (size < 0) {
-        res->check_errors++;
         return size;
     }
 
-    high_off = 0;
-    for (i = 0; i < s->bat_size; i++) {
-        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
-        if (off > high_off) {
-            high_off = off;
-        }
-    }
-
     res->image_end_offset = high_off + s->cluster_size;
     if (size > res->image_end_offset) {
-        count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
-        fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
-                fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
-                size - res->image_end_offset);
-        res->leaks += count;
-        if (fix & BDRV_FIX_LEAKS) {
+        if (fix) {
             Error *local_err = NULL;
 
             /*
@@ -516,13 +502,48 @@ static int parallels_check_leak(BlockDriverState *bs,
                                 PREALLOC_MODE_OFF, 0, &local_err);
             if (ret < 0) {
                 error_report_err(local_err);
-                res->check_errors++;
                 return ret;
             }
-            res->leaks_fixed += count;
         }
     }
 
+    return size - res->image_end_offset;
+}
+
+static int parallels_check_leak(BlockDriverState *bs,
+                                BdrvCheckResult *res,
+                                BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int64_t off, high_off, count, cut_out;
+    uint32_t i;
+
+    high_off = 0;
+    for (i = 0; i < s->bat_size; i++) {
+        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+        if (off > high_off) {
+            high_off = off;
+        }
+    }
+
+    cut_out = parallels_handle_leak(bs, res, high_off, fix & BDRV_FIX_LEAKS);
+    if (cut_out < 0) {
+        res->check_errors++;
+        return cut_out;
+    }
+    if (cut_out == 0) {
+        return 0;
+    }
+
+    count = DIV_ROUND_UP(cut_out, s->cluster_size);
+    fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
+            fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", cut_out);
+
+    res->leaks += count;
+    if (fix & BDRV_FIX_LEAKS) {
+        res->leaks_fixed += count;
+    }
+
     return 0;
 }
 
-- 
2.34.1

Re: [PATCH 2/6] parallels: Create parallels_handle_leak() to truncate excess clusters

[Denis V. Lunev]

On 9/2/22 10:52, Alexander Ivanov wrote:
> This helper will be reused in the next patch for duplications check.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 65 +++++++++++++++++++++++++++++++----------------
>   1 file changed, 43 insertions(+), 22 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 7b4e997ebd..dbcaf5d310 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -475,37 +475,23 @@ static int parallels_check_outside_image(BlockDriverState *bs,
>       return 0;
>   }
>   
> -static int parallels_check_leak(BlockDriverState *bs,
> -                                BdrvCheckResult *res,
> -                                BdrvCheckMode fix)
> +static int64_t parallels_handle_leak(BlockDriverState *bs,
> +                                     BdrvCheckResult *res,
> +                                     int64_t high_off,
> +                                     bool fix)
>   {
>       BDRVParallelsState *s = bs->opaque;
> -    int64_t size, off, high_off, count;
> -    uint32_t i;
> +    int64_t size;
>       int ret;
>   
>       size = bdrv_getlength(bs->file->bs);
>       if (size < 0) {
> -        res->check_errors++;
>           return size;
>       }
>   
> -    high_off = 0;
> -    for (i = 0; i < s->bat_size; i++) {
> -        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> -        if (off > high_off) {
> -            high_off = off;
> -        }
> -    }
> -
>       res->image_end_offset = high_off + s->cluster_size;
>       if (size > res->image_end_offset) {
> -        count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
> -        fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
> -                fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
> -                size - res->image_end_offset);
> -        res->leaks += count;
> -        if (fix & BDRV_FIX_LEAKS) {
> +        if (fix) {
>               Error *local_err = NULL;
>   
>               /*
> @@ -516,13 +502,48 @@ static int parallels_check_leak(BlockDriverState *bs,
>                                   PREALLOC_MODE_OFF, 0, &local_err);
>               if (ret < 0) {
>                   error_report_err(local_err);
> -                res->check_errors++;
>                   return ret;
>               }
> -            res->leaks_fixed += count;
>           }
>       }
>   
> +    return size - res->image_end_offset;
> +}
> +
> +static int parallels_check_leak(BlockDriverState *bs,
> +                                BdrvCheckResult *res,
> +                                BdrvCheckMode fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    int64_t off, high_off, count, cut_out;
> +    uint32_t i;
> +
> +    high_off = 0;
> +    for (i = 0; i < s->bat_size; i++) {
> +        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> +        if (off > high_off) {
> +            high_off = off;
> +        }
> +    }
> +
> +    cut_out = parallels_handle_leak(bs, res, high_off, fix & BDRV_FIX_LEAKS);
> +    if (cut_out < 0) {
> +        res->check_errors++;
> +        return cut_out;
> +    }
> +    if (cut_out == 0) {
> +        return 0;
> +    }
> +
> +    count = DIV_ROUND_UP(cut_out, s->cluster_size);
> +    fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
> +            fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", cut_out);
> +
> +    res->leaks += count;
> +    if (fix & BDRV_FIX_LEAKS) {
> +        res->leaks_fixed += count;
> +    }
> +
>       return 0;
>   }
>   
I do not think that we need this helper. See the note to the next patch

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

[Alexander Ivanov]

Cluster offsets must be unique among all BAT entries.
Find duplicate offsets in the BAT.

If a duplicated offset is found fix it by copying the content
of the relevant cluster to a new allocated cluster and
set the new cluster offset to the duplicated entry.

Add host_cluster_index() helper to deduplicate the code.
Add highest_offset() helper. It will be used for code deduplication
in the next patch.

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

diff --git a/block/parallels.c b/block/parallels.c
index dbcaf5d310..339ce45634 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -136,6 +136,26 @@ 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->header->data_off << BDRV_SECTOR_BITS;
+    return off / s->cluster_size;
+}
+
+static int64_t highest_offset(BDRVParallelsState *s)
+{
+    int64_t off, high_off = 0;
+    int i;
+
+    for (i = 0; i < s->bat_size; i++) {
+        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+        if (off > high_off) {
+            high_off = off;
+        }
+    }
+    return high_off;
+}
+
 static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
                             int nb_sectors, int *pnum)
 {
@@ -547,6 +567,114 @@ static int parallels_check_leak(BlockDriverState *bs,
     return 0;
 }
 
+static int parallels_check_duplicate(BlockDriverState *bs,
+                                     BdrvCheckResult *res,
+                                     BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    QEMUIOVector qiov;
+    int64_t off, high_off, sector;
+    unsigned long *bitmap;
+    uint32_t i, bitmap_size, cluster_index;
+    int n, ret = 0;
+    uint64_t *buf = NULL;
+    bool new_allocations = false;
+
+    high_off = highest_offset(s);
+    if (high_off == 0) {
+        return 0;
+    }
+
+    /*
+     * Create a bitmap of used clusters.
+     * If a bit is set, there is a BAT entry pointing to this cluster.
+     * Loop through the BAT entrues, check bits relevant to an entry offset.
+     * If bit is set, this entry is duplicated. Otherwise set the bit.
+     */
+    bitmap_size = host_cluster_index(s, high_off) + 1;
+    bitmap = bitmap_new(bitmap_size);
+
+    buf = g_malloc(s->cluster_size);
+    qemu_iovec_init(&qiov, 0);
+    qemu_iovec_add(&qiov, buf, 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);
+        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.
+                 */
+                parallels_set_bat_entry(s, i, 0);
+
+                ret = bdrv_pread(bs->file, off, s->cluster_size, buf, 0);
+                if (ret < 0) {
+                    res->check_errors++;
+                    goto out;
+                }
+
+                sector = (i * s->cluster_size) >> BDRV_SECTOR_BITS;
+                off = allocate_clusters(bs, sector, s->tracks, &n);
+                if (off < 0) {
+                    res->check_errors++;
+                    ret = off;
+                    goto out;
+                }
+                off <<= BDRV_SECTOR_BITS;
+                if (off > high_off) {
+                    high_off = off;
+                }
+
+                ret = bdrv_co_pwritev(bs->file, off, s->cluster_size, &qiov, 0);
+                if (ret < 0) {
+                    res->check_errors++;
+                    goto out;
+                }
+
+                new_allocations = true;
+                res->corruptions_fixed++;
+            }
+
+        } else {
+            bitmap_set(bitmap, cluster_index, 1);
+        }
+    }
+
+    if (new_allocations) {
+        /*
+         * When new clusters are allocated, file size increases
+         * by 128 Mb blocks. We need to truncate the file to the
+         * right size.
+         */
+        ret = parallels_handle_leak(bs, res, high_off, true);
+        if (ret < 0) {
+            res->check_errors++;
+            goto out;
+        }
+    }
+
+out:
+    qemu_iovec_destroy(&qiov);
+    g_free(buf);
+    g_free(bitmap);
+    return ret;
+}
+
 static void parallels_collect_statistics(BlockDriverState *bs,
                                          BdrvCheckResult *res,
                                          BdrvCheckMode fix)
@@ -595,6 +723,14 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
             return ret;
         }
 
+        /* This checks only for "WithouFreSpacExt" format */
+        if (!memcmp(s->header->magic, HEADER_MAGIC2, 16)) {
+            ret = parallels_check_duplicate(bs, res, fix);
+            if (ret < 0) {
+                return ret;
+            }
+        }
+
         parallels_collect_statistics(bs, res, fix);
     }
 
-- 
2.34.1

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

[Denis V. Lunev]

On 9/2/22 10:52, Alexander Ivanov wrote:
> Cluster offsets must be unique among all BAT entries.
> Find duplicate offsets in the BAT.
>
> If a duplicated offset is found fix it by copying the content
> of the relevant cluster to a new allocated cluster and
> set the new cluster offset to the duplicated entry.
>
> Add host_cluster_index() helper to deduplicate the code.
> Add highest_offset() helper. It will be used for code deduplication
> in the next patch.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 136 insertions(+)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index dbcaf5d310..339ce45634 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -136,6 +136,26 @@ 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->header->data_off << BDRV_SECTOR_BITS;
> +    return off / s->cluster_size;
not properly aligned cluster is a problem by itself
This is possible for older image format and I believe that
we should run "leak check" even for it and note that
in "not aligned" case one should call 'qemu-img convert"
and copy what can be saved. The error is really hard to
recover.

> +}
> +
> +static int64_t highest_offset(BDRVParallelsState *s)
> +{
> +    int64_t off, high_off = 0;
> +    int i;
> +
> +    for (i = 0; i < s->bat_size; i++) {
> +        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> +        if (off > high_off) {
> +            high_off = off;
> +        }
> +    }
> +    return high_off;
> +}
> +
>   static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
>                               int nb_sectors, int *pnum)
>   {
> @@ -547,6 +567,114 @@ static int parallels_check_leak(BlockDriverState *bs,
>       return 0;
>   }
>   
> +static int parallels_check_duplicate(BlockDriverState *bs,
> +                                     BdrvCheckResult *res,
> +                                     BdrvCheckMode fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    QEMUIOVector qiov;
> +    int64_t off, high_off, sector;
> +    unsigned long *bitmap;
> +    uint32_t i, bitmap_size, cluster_index;
> +    int n, ret = 0;
> +    uint64_t *buf = NULL;
> +    bool new_allocations = false;
> +
> +    high_off = highest_offset(s);
> +    if (high_off == 0) {
> +        return 0;
> +    }
> +
> +    /*
> +     * Create a bitmap of used clusters.
> +     * If a bit is set, there is a BAT entry pointing to this cluster.
> +     * Loop through the BAT entrues, check bits relevant to an entry offset.
s/entrues/entries/
> +     * If bit is set, this entry is duplicated. Otherwise set the bit.
> +     */
> +    bitmap_size = host_cluster_index(s, high_off) + 1;
> +    bitmap = bitmap_new(bitmap_size);
this is wrong de-facto. once you deduplicate, you will have clusters
outside the bitmap. We should use MAX(high_off, virtual_size(bds))
OK, may be this is correct, see my note below, but that's needs
a comment.

> +
> +    buf = g_malloc(s->cluster_size);
nope-nope. you should use memalign as you could have
O_DIRECT IO later on with this buffer

> +    qemu_iovec_init(&qiov, 0);
> +    qemu_iovec_add(&qiov, buf, 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);
> +        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.
> +                 */
> +                parallels_set_bat_entry(s, i, 0);
> +
> +                ret = bdrv_pread(bs->file, off, s->cluster_size, buf, 0);
I believe that we are for sure in co-routine context thus
bdrv_co_pread would be better


> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    goto out;
> +                }
> +
> +                sector = (i * s->cluster_size) >> BDRV_SECTOR_BITS;
> +                off = allocate_clusters(bs, sector, s->tracks, &n);
no way. Please do not keep bytes and sectors in the same variable
That makes code unreadable

> +                if (off < 0) {
> +                    res->check_errors++;
> +                    ret = off;
> +                    goto out;
> +                }
> +                off <<= BDRV_SECTOR_BITS;
> +                if (off > high_off) {
> +                    high_off = off;
> +                }
> +
> +                ret = bdrv_co_pwritev(bs->file, off, s->cluster_size, &qiov, 0);
> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    goto out;
> +                }
> +
> +                new_allocations = true;
> +                res->corruptions_fixed++;
We are not setting the bit for the new cluster offset and that requires
a note, that all out of image clusters are already cleared and we
could safely use this position.

Anyway, this is potentially not enough. allocate_cluster() in the
further iterations will reuse holed offsets inside the image and
this potentially puts us into the trouble. This should be noted.

> +            }
> +
> +        } else {
> +            bitmap_set(bitmap, cluster_index, 1);
> +        }
> +    }
> +
> +    if (new_allocations) {
> +        /*
> +         * When new clusters are allocated, file size increases
> +         * by 128 Mb blocks. We need to truncate the file to the
> +         * right size.
> +         */
> +        ret = parallels_handle_leak(bs, res, high_off, true);
> +        if (ret < 0) {
> +            res->check_errors++;
> +            goto out;
> +        }
> +    }
This is redundant I believe. parallels_close should do the job well.
More over, we will have leak detector triggered and thus false
error printed.


> +
> +out:
> +    qemu_iovec_destroy(&qiov);
> +    g_free(buf);
> +    g_free(bitmap);
> +    return ret;
> +}
> +
>   static void parallels_collect_statistics(BlockDriverState *bs,
>                                            BdrvCheckResult *res,
>                                            BdrvCheckMode fix)
> @@ -595,6 +723,14 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>               return ret;
>           }
>   
> +        /* This checks only for "WithouFreSpacExt" format */
> +        if (!memcmp(s->header->magic, HEADER_MAGIC2, 16)) {
That is I am seriously against. Why only newer format supported?

> +            ret = parallels_check_duplicate(bs, res, fix);
> +            if (ret < 0) {
> +                return ret;
> +            }
> +        }
> +
>           parallels_collect_statistics(bs, res, fix);
>       }
>   

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

[Denis V. Lunev]

On 9/2/22 10:52, Alexander Ivanov wrote:
> Cluster offsets must be unique among all BAT entries.
> Find duplicate offsets in the BAT.
>
> If a duplicated offset is found fix it by copying the content
> of the relevant cluster to a new allocated cluster and
> set the new cluster offset to the duplicated entry.
>
> Add host_cluster_index() helper to deduplicate the code.
> Add highest_offset() helper. It will be used for code deduplication
> in the next patch.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 136 insertions(+)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index dbcaf5d310..339ce45634 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -136,6 +136,26 @@ 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->header->data_off << BDRV_SECTOR_BITS;
> +    return off / s->cluster_size;
> +}
> +
> +static int64_t highest_offset(BDRVParallelsState *s)
> +{
> +    int64_t off, high_off = 0;
> +    int i;
> +
> +    for (i = 0; i < s->bat_size; i++) {
> +        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> +        if (off > high_off) {
> +            high_off = off;
> +        }
> +    }
> +    return high_off;
> +}
> +
>   static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
>                               int nb_sectors, int *pnum)
>   {
> @@ -547,6 +567,114 @@ static int parallels_check_leak(BlockDriverState *bs,
>       return 0;
>   }
>   
> +static int parallels_check_duplicate(BlockDriverState *bs,
> +                                     BdrvCheckResult *res,
> +                                     BdrvCheckMode fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    QEMUIOVector qiov;
> +    int64_t off, high_off, sector;
> +    unsigned long *bitmap;
> +    uint32_t i, bitmap_size, cluster_index;
> +    int n, ret = 0;
> +    uint64_t *buf = NULL;
> +    bool new_allocations = false;
> +
> +    high_off = highest_offset(s);
> +    if (high_off == 0) {
> +        return 0;
> +    }
> +
> +    /*
> +     * Create a bitmap of used clusters.
> +     * If a bit is set, there is a BAT entry pointing to this cluster.
> +     * Loop through the BAT entrues, check bits relevant to an entry offset.
> +     * If bit is set, this entry is duplicated. Otherwise set the bit.
> +     */
> +    bitmap_size = host_cluster_index(s, high_off) + 1;
> +    bitmap = bitmap_new(bitmap_size);
> +
> +    buf = g_malloc(s->cluster_size);
> +    qemu_iovec_init(&qiov, 0);
> +    qemu_iovec_add(&qiov, buf, 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);
> +        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.
> +                 */
> +                parallels_set_bat_entry(s, i, 0);
> +
> +                ret = bdrv_pread(bs->file, off, s->cluster_size, buf, 0);
> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    goto out;
> +                }
> +
> +                sector = (i * s->cluster_size) >> BDRV_SECTOR_BITS;
> +                off = allocate_clusters(bs, sector, s->tracks, &n);
> +                if (off < 0) {
> +                    res->check_errors++;
> +                    ret = off;
> +                    goto out;
> +                }
> +                off <<= BDRV_SECTOR_BITS;
> +                if (off > high_off) {
> +                    high_off = off;
> +                }
> +
> +                ret = bdrv_co_pwritev(bs->file, off, s->cluster_size, &qiov, 0);
> +                if (ret < 0) {
> +                    res->check_errors++;
> +                    goto out;
> +                }
> +
> +                new_allocations = true;
> +                res->corruptions_fixed++;
> +            }
> +
> +        } else {
> +            bitmap_set(bitmap, cluster_index, 1);
> +        }
> +    }
> +
> +    if (new_allocations) {
> +        /*
> +         * When new clusters are allocated, file size increases
> +         * by 128 Mb blocks. We need to truncate the file to the
> +         * right size.
> +         */
> +        ret = parallels_handle_leak(bs, res, high_off, true);
> +        if (ret < 0) {
> +            res->check_errors++;
> +            goto out;
> +        }
> +    }
OK. I have re-read the code with test case handy and now
understand the situation completely.

The problem is that img_check() routine calls bdrv_check()
actually TWICE without image reopening and thus we
comes to some trouble on the second check as we have
had preallocated some space inside the image. This
is root of the problem.

Though this kind of the fix seems like overkill, I still do not
like the resulted code. It at least do not scale with the checks
which we will add further.

I think that we could do that in two ways:
* temporary set prealloc_mode to none at start of parallels_co_check
   and return it back at the end
* parallels_leak_check should just set data_end and do nothing
    more + we should have truncate at the end of the
    parallels_co_check() if we have had performed ANY fix

Den

> +
> +out:
> +    qemu_iovec_destroy(&qiov);
> +    g_free(buf);
> +    g_free(bitmap);
> +    return ret;
> +}
> +
>   static void parallels_collect_statistics(BlockDriverState *bs,
>                                            BdrvCheckResult *res,
>                                            BdrvCheckMode fix)
> @@ -595,6 +723,14 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>               return ret;
>           }
>   
> +        /* This checks only for "WithouFreSpacExt" format */
> +        if (!memcmp(s->header->magic, HEADER_MAGIC2, 16)) {
> +            ret = parallels_check_duplicate(bs, res, fix);
> +            if (ret < 0) {
> +                return ret;
> +            }
> +        }
> +
>           parallels_collect_statistics(bs, res, fix);
>       }
>   

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

[Denis V. Lunev]

On 9/8/22 19:15, Denis V. Lunev wrote:
> On 9/2/22 10:52, Alexander Ivanov wrote:
>> Cluster offsets must be unique among all BAT entries.
>> Find duplicate offsets in the BAT.
>>
>> If a duplicated offset is found fix it by copying the content
>> of the relevant cluster to a new allocated cluster and
>> set the new cluster offset to the duplicated entry.
>>
>> Add host_cluster_index() helper to deduplicate the code.
>> Add highest_offset() helper. It will be used for code deduplication
>> in the next patch.
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>>   block/parallels.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 136 insertions(+)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index dbcaf5d310..339ce45634 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -136,6 +136,26 @@ 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->header->data_off << BDRV_SECTOR_BITS;
>> +    return off / s->cluster_size;
>> +}
>> +
>> +static int64_t highest_offset(BDRVParallelsState *s)
>> +{
>> +    int64_t off, high_off = 0;
>> +    int i;
>> +
>> +    for (i = 0; i < s->bat_size; i++) {
>> +        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
>> +        if (off > high_off) {
>> +            high_off = off;
>> +        }
>> +    }
>> +    return high_off;
>> +}
>> +
>>   static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
>>                               int nb_sectors, int *pnum)
>>   {
>> @@ -547,6 +567,114 @@ static int 
>> parallels_check_leak(BlockDriverState *bs,
>>       return 0;
>>   }
>>   +static int parallels_check_duplicate(BlockDriverState *bs,
>> +                                     BdrvCheckResult *res,
>> +                                     BdrvCheckMode fix)
>> +{
>> +    BDRVParallelsState *s = bs->opaque;
>> +    QEMUIOVector qiov;
>> +    int64_t off, high_off, sector;
>> +    unsigned long *bitmap;
>> +    uint32_t i, bitmap_size, cluster_index;
>> +    int n, ret = 0;
>> +    uint64_t *buf = NULL;
>> +    bool new_allocations = false;
>> +
>> +    high_off = highest_offset(s);
>> +    if (high_off == 0) {
>> +        return 0;
>> +    }
>> +
>> +    /*
>> +     * Create a bitmap of used clusters.
>> +     * If a bit is set, there is a BAT entry pointing to this cluster.
>> +     * Loop through the BAT entrues, check bits relevant to an entry 
>> offset.
>> +     * If bit is set, this entry is duplicated. Otherwise set the bit.
>> +     */
>> +    bitmap_size = host_cluster_index(s, high_off) + 1;
>> +    bitmap = bitmap_new(bitmap_size);
>> +
>> +    buf = g_malloc(s->cluster_size);
>> +    qemu_iovec_init(&qiov, 0);
>> +    qemu_iovec_add(&qiov, buf, 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);
>> +        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.
>> +                 */
>> +                parallels_set_bat_entry(s, i, 0);
>> +
>> +                ret = bdrv_pread(bs->file, off, s->cluster_size, 
>> buf, 0);
>> +                if (ret < 0) {
>> +                    res->check_errors++;
>> +                    goto out;
>> +                }
>> +
>> +                sector = (i * s->cluster_size) >> BDRV_SECTOR_BITS;
>> +                off = allocate_clusters(bs, sector, s->tracks, &n);
>> +                if (off < 0) {
>> +                    res->check_errors++;
>> +                    ret = off;
>> +                    goto out;
>> +                }
>> +                off <<= BDRV_SECTOR_BITS;
>> +                if (off > high_off) {
>> +                    high_off = off;
>> +                }
>> +
>> +                ret = bdrv_co_pwritev(bs->file, off, 
>> s->cluster_size, &qiov, 0);
>> +                if (ret < 0) {
>> +                    res->check_errors++;
>> +                    goto out;
>> +                }
>> +
>> +                new_allocations = true;
>> +                res->corruptions_fixed++;
>> +            }
>> +
>> +        } else {
>> +            bitmap_set(bitmap, cluster_index, 1);
>> +        }
>> +    }
>> +
>> +    if (new_allocations) {
>> +        /*
>> +         * When new clusters are allocated, file size increases
>> +         * by 128 Mb blocks. We need to truncate the file to the
>> +         * right size.
>> +         */
>> +        ret = parallels_handle_leak(bs, res, high_off, true);
>> +        if (ret < 0) {
>> +            res->check_errors++;
>> +            goto out;
>> +        }
>> +    }
> OK. I have re-read the code with test case handy and now
> understand the situation completely.
>
> The problem is that img_check() routine calls bdrv_check()
> actually TWICE without image reopening and thus we
> comes to some trouble on the second check as we have
> had preallocated some space inside the image. This
> is root of the problem.
>
> Though this kind of the fix seems like overkill, I still do not
> like the resulted code. It at least do not scale with the checks
> which we will add further.
>
> I think that we could do that in two ways:
> * temporary set prealloc_mode to none at start of parallels_co_check
>   and return it back at the end
> * parallels_leak_check should just set data_end and do nothing
>    more + we should have truncate at the end of the
>    parallels_co_check() if we have had performed ANY fix

better way found. We should check not file length in handle_leak!
We should compare highest_off with the data_end and that is
146% correct.

File length COULD be more than highest possible data cluster
offset, but data_end should point to the correct location.
That is it!

Den

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

[Alexander Ivanov]

On 08.09.2022 19:45, Denis V. Lunev wrote:
> On 9/8/22 19:15, Denis V. Lunev wrote:
>> On 9/2/22 10:52, Alexander Ivanov wrote:
>>> Cluster offsets must be unique among all BAT entries.
>>> Find duplicate offsets in the BAT.
>>>
>>> If a duplicated offset is found fix it by copying the content
>>> of the relevant cluster to a new allocated cluster and
>>> set the new cluster offset to the duplicated entry.
>>>
>>> Add host_cluster_index() helper to deduplicate the code.
>>> Add highest_offset() helper. It will be used for code deduplication
>>> in the next patch.
>>>
>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>> ---
>>>   block/parallels.c | 136 
>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 136 insertions(+)
>>>
>>> diff --git a/block/parallels.c b/block/parallels.c
>>> index dbcaf5d310..339ce45634 100644
>>> --- a/block/parallels.c
>>> +++ b/block/parallels.c
>>> @@ -136,6 +136,26 @@ 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->header->data_off << BDRV_SECTOR_BITS;
>>> +    return off / s->cluster_size;
>>> +}
>>> +
>>> +static int64_t highest_offset(BDRVParallelsState *s)
>>> +{
>>> +    int64_t off, high_off = 0;
>>> +    int i;
>>> +
>>> +    for (i = 0; i < s->bat_size; i++) {
>>> +        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
>>> +        if (off > high_off) {
>>> +            high_off = off;
>>> +        }
>>> +    }
>>> +    return high_off;
>>> +}
>>> +
>>>   static int64_t block_status(BDRVParallelsState *s, int64_t 
>>> sector_num,
>>>                               int nb_sectors, int *pnum)
>>>   {
>>> @@ -547,6 +567,114 @@ static int 
>>> parallels_check_leak(BlockDriverState *bs,
>>>       return 0;
>>>   }
>>>   +static int parallels_check_duplicate(BlockDriverState *bs,
>>> +                                     BdrvCheckResult *res,
>>> +                                     BdrvCheckMode fix)
>>> +{
>>> +    BDRVParallelsState *s = bs->opaque;
>>> +    QEMUIOVector qiov;
>>> +    int64_t off, high_off, sector;
>>> +    unsigned long *bitmap;
>>> +    uint32_t i, bitmap_size, cluster_index;
>>> +    int n, ret = 0;
>>> +    uint64_t *buf = NULL;
>>> +    bool new_allocations = false;
>>> +
>>> +    high_off = highest_offset(s);
>>> +    if (high_off == 0) {
>>> +        return 0;
>>> +    }
>>> +
>>> +    /*
>>> +     * Create a bitmap of used clusters.
>>> +     * If a bit is set, there is a BAT entry pointing to this cluster.
>>> +     * Loop through the BAT entrues, check bits relevant to an 
>>> entry offset.
>>> +     * If bit is set, this entry is duplicated. Otherwise set the bit.
>>> +     */
>>> +    bitmap_size = host_cluster_index(s, high_off) + 1;
>>> +    bitmap = bitmap_new(bitmap_size);
>>> +
>>> +    buf = g_malloc(s->cluster_size);
>>> +    qemu_iovec_init(&qiov, 0);
>>> +    qemu_iovec_add(&qiov, buf, 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);
>>> +        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.
>>> +                 */
>>> +                parallels_set_bat_entry(s, i, 0);
>>> +
>>> +                ret = bdrv_pread(bs->file, off, s->cluster_size, 
>>> buf, 0);
>>> +                if (ret < 0) {
>>> +                    res->check_errors++;
>>> +                    goto out;
>>> +                }
>>> +
>>> +                sector = (i * s->cluster_size) >> BDRV_SECTOR_BITS;
>>> +                off = allocate_clusters(bs, sector, s->tracks, &n);
>>> +                if (off < 0) {
>>> +                    res->check_errors++;
>>> +                    ret = off;
>>> +                    goto out;
>>> +                }
>>> +                off <<= BDRV_SECTOR_BITS;
>>> +                if (off > high_off) {
>>> +                    high_off = off;
>>> +                }
>>> +
>>> +                ret = bdrv_co_pwritev(bs->file, off, 
>>> s->cluster_size, &qiov, 0);
>>> +                if (ret < 0) {
>>> +                    res->check_errors++;
>>> +                    goto out;
>>> +                }
>>> +
>>> +                new_allocations = true;
>>> +                res->corruptions_fixed++;
>>> +            }
>>> +
>>> +        } else {
>>> +            bitmap_set(bitmap, cluster_index, 1);
>>> +        }
>>> +    }
>>> +
>>> +    if (new_allocations) {
>>> +        /*
>>> +         * When new clusters are allocated, file size increases
>>> +         * by 128 Mb blocks. We need to truncate the file to the
>>> +         * right size.
>>> +         */
>>> +        ret = parallels_handle_leak(bs, res, high_off, true);
>>> +        if (ret < 0) {
>>> +            res->check_errors++;
>>> +            goto out;
>>> +        }
>>> +    }
>> OK. I have re-read the code with test case handy and now
>> understand the situation completely.
>>
>> The problem is that img_check() routine calls bdrv_check()
>> actually TWICE without image reopening and thus we
>> comes to some trouble on the second check as we have
>> had preallocated some space inside the image. This
>> is root of the problem.
>>
>> Though this kind of the fix seems like overkill, I still do not
>> like the resulted code. It at least do not scale with the checks
>> which we will add further.
>>
>> I think that we could do that in two ways:
>> * temporary set prealloc_mode to none at start of parallels_co_check
>>   and return it back at the end
>> * parallels_leak_check should just set data_end and do nothing
>>    more + we should have truncate at the end of the
>>    parallels_co_check() if we have had performed ANY fix
>
> better way found. We should check not file length in handle_leak!
> We should compare highest_off with the data_end and that is
> 146% correct.
>
> File length COULD be more than highest possible data cluster
> offset, but data_end should point to the correct location.
> That is it!
>
> Den

Initially data_end points to the end of the cluster with the highest

offset in the BAT, not to the file end. So we can't detect leaks in

such a way.

[PATCH 4/6] parallels: Use highest_offset() helper in leak check

[Alexander Ivanov]

Deduplicate code by using highest_offset() helper.

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

diff --git a/block/parallels.c b/block/parallels.c
index 339ce45634..688aa081e2 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -535,16 +535,9 @@ static int parallels_check_leak(BlockDriverState *bs,
                                 BdrvCheckMode fix)
 {
     BDRVParallelsState *s = bs->opaque;
-    int64_t off, high_off, count, cut_out;
-    uint32_t i;
+    int64_t high_off, count, cut_out;
 
-    high_off = 0;
-    for (i = 0; i < s->bat_size; i++) {
-        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
-        if (off > high_off) {
-            high_off = off;
-        }
-    }
+    high_off = highest_offset(s);
 
     cut_out = parallels_handle_leak(bs, res, high_off, fix & BDRV_FIX_LEAKS);
     if (cut_out < 0) {
-- 
2.34.1

Re: [PATCH 4/6] parallels: Use highest_offset() helper in leak check

[Denis V. Lunev]

On 9/2/22 10:52, Alexander Ivanov wrote:
> Deduplicate code by using highest_offset() helper.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 11 ++---------
>   1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 339ce45634..688aa081e2 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -535,16 +535,9 @@ static int parallels_check_leak(BlockDriverState *bs,
>                                   BdrvCheckMode fix)
>   {
>       BDRVParallelsState *s = bs->opaque;
> -    int64_t off, high_off, count, cut_out;
> -    uint32_t i;
> +    int64_t high_off, count, cut_out;
>   
> -    high_off = 0;
> -    for (i = 0; i < s->bat_size; i++) {
> -        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
> -        if (off > high_off) {
> -            high_off = off;
> -        }
> -    }
> +    high_off = highest_offset(s);
>   
>       cut_out = parallels_handle_leak(bs, res, high_off, fix & BDRV_FIX_LEAKS);
>       if (cut_out < 0) {
Here I kinda disagree. This change and the introduction of the
helper should be done exactly in the same patch as in the other
case it is not possible to say whether this replacement is
correct or not.

Den

[PATCH 5/6] parallels: Replace fprintf by qemu_log

[Alexander Ivanov]

Use a standard QEMU function for logging.

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

diff --git a/block/parallels.c b/block/parallels.c
index 688aa081e2..08526196da 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -42,6 +42,7 @@
 #include "qemu/bswap.h"
 #include "qemu/bitmap.h"
 #include "qemu/memalign.h"
+#include "qemu/log-for-trace.h"
 #include "migration/blocker.h"
 #include "parallels.h"
 
@@ -448,7 +449,7 @@ static void parallels_check_unclean(BlockDriverState *bs,
         return;
     }
 
-    fprintf(stderr, "%s image was not closed correctly\n",
+    qemu_log("%s image was not closed correctly\n",
             fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
     res->corruptions++;
     if (fix & BDRV_FIX_ERRORS) {
@@ -476,7 +477,7 @@ static int parallels_check_outside_image(BlockDriverState *bs,
     for (i = 0; i < s->bat_size; i++) {
         off = bat2sect(s, i) << BDRV_SECTOR_BITS;
         if (off >= size) {
-            fprintf(stderr, "%s cluster %u is outside image\n",
+            qemu_log("%s cluster %u is outside image\n",
                     fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
             res->corruptions++;
             if (fix & BDRV_FIX_ERRORS) {
@@ -549,7 +550,7 @@ static int parallels_check_leak(BlockDriverState *bs,
     }
 
     count = DIV_ROUND_UP(cut_out, s->cluster_size);
-    fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
+    qemu_log("%s space leaked at the end of the image %" PRId64 "\n",
             fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", cut_out);
 
     res->leaks += count;
@@ -600,8 +601,7 @@ static int parallels_check_duplicate(BlockDriverState *bs,
         cluster_index = host_cluster_index(s, off);
         if (test_bit(cluster_index, bitmap)) {
             /* this cluster duplicates another one */
-            fprintf(stderr,
-                    "%s duplicate offset in BAT entry %u\n",
+            qemu_log("%s duplicate offset in BAT entry %u\n",
                     fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
 
             res->corruptions++;
-- 
2.34.1

Re: [PATCH 5/6] parallels: Replace fprintf by qemu_log

[Denis V. Lunev]

On 9/2/22 10:52, Alexander Ivanov wrote:
> Use a standard QEMU function for logging.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 688aa081e2..08526196da 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -42,6 +42,7 @@
>   #include "qemu/bswap.h"
>   #include "qemu/bitmap.h"
>   #include "qemu/memalign.h"
> +#include "qemu/log-for-trace.h"
>   #include "migration/blocker.h"
>   #include "parallels.h"
>   
> @@ -448,7 +449,7 @@ static void parallels_check_unclean(BlockDriverState *bs,
>           return;
>       }
>   
> -    fprintf(stderr, "%s image was not closed correctly\n",
> +    qemu_log("%s image was not closed correctly\n",
>               fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
>       res->corruptions++;
>       if (fix & BDRV_FIX_ERRORS) {
> @@ -476,7 +477,7 @@ static int parallels_check_outside_image(BlockDriverState *bs,
>       for (i = 0; i < s->bat_size; i++) {
>           off = bat2sect(s, i) << BDRV_SECTOR_BITS;
>           if (off >= size) {
> -            fprintf(stderr, "%s cluster %u is outside image\n",
> +            qemu_log("%s cluster %u is outside image\n",
>                       fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
>               res->corruptions++;
>               if (fix & BDRV_FIX_ERRORS) {
> @@ -549,7 +550,7 @@ static int parallels_check_leak(BlockDriverState *bs,
>       }
>   
>       count = DIV_ROUND_UP(cut_out, s->cluster_size);
> -    fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
> +    qemu_log("%s space leaked at the end of the image %" PRId64 "\n",
>               fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", cut_out);
>   
>       res->leaks += count;
> @@ -600,8 +601,7 @@ static int parallels_check_duplicate(BlockDriverState *bs,
>           cluster_index = host_cluster_index(s, off);
>           if (test_bit(cluster_index, bitmap)) {
>               /* this cluster duplicates another one */
> -            fprintf(stderr,
> -                    "%s duplicate offset in BAT entry %u\n",
> +            qemu_log("%s duplicate offset in BAT entry %u\n",
>                       fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
>   
>               res->corruptions++;
formatting is broken. next line after qemu_log should be indented with 
one more space.

I would be happy to get more detailed motivation, which should say that 
once the check
will be called during normal work, tracking of the check MUST be present 
in VM log
to have some clues if something will go wrong with customer's data.

Den

[PATCH 6/6] 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 | 95 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 65 insertions(+), 30 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 08526196da..a7c3af4ef2 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -735,6 +735,18 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
     return ret;
 }
 
+typedef struct ParallelsOpenCheckCo {
+    BlockDriverState *bs;
+    BdrvCheckResult *res;
+    BdrvCheckMode fix;
+    int ret;
+} ParallelsOpenCheckCo;
+
+static void coroutine_fn parallels_co_open_check(void *opaque)
+{
+    ParallelsOpenCheckCo *poc = opaque;
+    poc->ret = parallels_co_check(poc->bs, poc->res, poc->fix);
+}
 
 static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
                                             Error **errp)
@@ -947,8 +959,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
 {
     BDRVParallelsState *s = bs->opaque;
     ParallelsHeader ph;
-    int ret, size, i;
-    int64_t file_size;
+    int ret, size;
+    int64_t file_size, high_off;
     QemuOpts *opts = NULL;
     Error *local_err = NULL;
     char *buf;
@@ -1027,34 +1039,6 @@ 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_size) {
-            if (flags & BDRV_O_CHECK) {
-                continue;
-            }
-            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
-                       "is larger than file size (%" PRIi64 ")",
-                       off, i, file_size);
-            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);
     if (!opts) {
         goto fail_options;
@@ -1116,7 +1100,58 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         error_free(s->migration_blocker);
         goto fail;
     }
+
     qemu_co_mutex_init(&s->lock);
+
+    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
+        s->header_unclean = true;
+    }
+
+    high_off = highest_offset(s) >> BDRV_SECTOR_BITS;
+    if (high_off >= s->data_end) {
+        s->data_end = high_off + s->tracks;
+    }
+
+    /*
+     * We don't repair the image here if it is opened for checks.
+     * Also let to work with images in RO mode.
+     */
+    if ((flags & BDRV_O_CHECK) || !(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_size ||
+        le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
+        BdrvCheckResult res = {0};
+        Coroutine *co;
+        ParallelsOpenCheckCo poc = {
+            .bs = bs,
+            .res = &res,
+            .fix = BDRV_FIX_ERRORS | BDRV_FIX_LEAKS,
+            .ret = -EINPROGRESS
+        };
+
+        if (qemu_in_coroutine()) {
+            /* From bdrv_co_create.  */
+            parallels_co_open_check(&poc);
+        } else {
+            assert(qemu_get_current_aio_context() == qemu_get_aio_context());
+            co = qemu_coroutine_create(parallels_co_open_check, &poc);
+            qemu_coroutine_enter(co);
+            BDRV_POLL_WHILE(bs, poc.ret == -EINPROGRESS);
+        }
+
+        if (poc.ret < 0) {
+            error_setg_errno(errp, -poc.ret,
+                             "Could not repair corrupted image");
+            goto fail;
+        }
+    }
+
     return 0;
 
 fail_format:
-- 
2.34.1

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

[Denis V. Lunev]

On 9/2/22 10:53, 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 | 95 ++++++++++++++++++++++++++++++++---------------
>   1 file changed, 65 insertions(+), 30 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 08526196da..a7c3af4ef2 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -735,6 +735,18 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>       return ret;
>   }
>   
> +typedef struct ParallelsOpenCheckCo {
> +    BlockDriverState *bs;
> +    BdrvCheckResult *res;
> +    BdrvCheckMode fix;
> +    int ret;
> +} ParallelsOpenCheckCo;
> +
> +static void coroutine_fn parallels_co_open_check(void *opaque)
> +{
> +    ParallelsOpenCheckCo *poc = opaque;
> +    poc->ret = parallels_co_check(poc->bs, poc->res, poc->fix);
> +}
>   
>   static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
>                                               Error **errp)
> @@ -947,8 +959,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>   {
>       BDRVParallelsState *s = bs->opaque;
>       ParallelsHeader ph;
> -    int ret, size, i;
> -    int64_t file_size;
> +    int ret, size;
> +    int64_t file_size, high_off;
>       QemuOpts *opts = NULL;
>       Error *local_err = NULL;
>       char *buf;
> @@ -1027,34 +1039,6 @@ 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_size) {
> -            if (flags & BDRV_O_CHECK) {
> -                continue;
> -            }
> -            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
> -                       "is larger than file size (%" PRIi64 ")",
> -                       off, i, file_size);
> -            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);
>       if (!opts) {
>           goto fail_options;
> @@ -1116,7 +1100,58 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>           error_free(s->migration_blocker);
>           goto fail;
>       }
> +
>       qemu_co_mutex_init(&s->lock);
> +
> +    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> +        s->header_unclean = true;
> +    }
> +
> +    high_off = highest_offset(s) >> BDRV_SECTOR_BITS;
> +    if (high_off >= s->data_end) {
> +        s->data_end = high_off + s->tracks;
> +    }
> +
> +    /*
> +     * We don't repair the image here if it is opened for checks.
> +     * Also let to work with images in RO mode.
My silly $0.02.
Also let *us allow* to with in read-only more.

> +     */
> +    if ((flags & BDRV_O_CHECK) || !(flags & BDRV_O_RDWR)) {
> +        return 0;
> +    }
not enough, We are not allowed to make changes with O_INACTIVE.
The check in this case should be postponed till O_INACTIVE clearance.

Very specific note. header_unclean is allowed in O_INACTIVE.
The image could be opened at the moment on the other
host!

This should be clarified.

> +
> +    /*
> +     * Repair the image if it's dirty or
> +     * out-of-image corruption was detected.
> +     */
> +    if (s->data_end > file_size ||
> +        le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
I dislike this. There are detection conditions above
and I think we should respect them adding the flag
'need_check' which should be kept in the BDS to
respect O_INACTIVE.

> +        BdrvCheckResult res = {0};
such assignments are weird (not portable for older compilers).

> +        Coroutine *co;
> +        ParallelsOpenCheckCo poc = {
> +            .bs = bs,
> +            .res = &res,
This is strange, why not to put BdrvCheckResult as
a whole to the ParallelsOpenCheckCo
> +            .fix = BDRV_FIX_ERRORS | BDRV_FIX_LEAKS,
> +            .ret = -EINPROGRESS
pls add comma on the line above, This is common convention
as in the case of addition one more initializing field you will
not change that line.
> +        };
> +
> +        if (qemu_in_coroutine()) {
> +            /* From bdrv_co_create.  */
> +            parallels_co_open_check(&poc);
> +        } else {
> +            assert(qemu_get_current_aio_context() == qemu_get_aio_context());
> +            co = qemu_coroutine_create(parallels_co_open_check, &poc);
> +            qemu_coroutine_enter(co);
> +            BDRV_POLL_WHILE(bs, poc.ret == -EINPROGRESS);
> +        }
> +
> +        if (poc.ret < 0) {
> +            error_setg_errno(errp, -poc.ret,
> +                             "Could not repair corrupted image");
> +            goto fail;
> +        }
> +    }
> +
>       return 0;
>   
>   fail_format:

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

[Denis V. Lunev]

On 9/2/22 10:53, 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 | 95 ++++++++++++++++++++++++++++++++---------------
>   1 file changed, 65 insertions(+), 30 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 08526196da..a7c3af4ef2 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -735,6 +735,18 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>       return ret;
>   }
>   
> +typedef struct ParallelsOpenCheckCo {
> +    BlockDriverState *bs;
> +    BdrvCheckResult *res;
> +    BdrvCheckMode fix;
> +    int ret;
> +} ParallelsOpenCheckCo;
> +
> +static void coroutine_fn parallels_co_open_check(void *opaque)
> +{
> +    ParallelsOpenCheckCo *poc = opaque;
> +    poc->ret = parallels_co_check(poc->bs, poc->res, poc->fix);
> +}
>   
>   static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
>                                               Error **errp)
> @@ -947,8 +959,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>   {
>       BDRVParallelsState *s = bs->opaque;
>       ParallelsHeader ph;
> -    int ret, size, i;
> -    int64_t file_size;
> +    int ret, size;
> +    int64_t file_size, high_off;
>       QemuOpts *opts = NULL;
>       Error *local_err = NULL;
>       char *buf;
> @@ -1027,34 +1039,6 @@ 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_size) {
> -            if (flags & BDRV_O_CHECK) {
> -                continue;
> -            }
> -            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
> -                       "is larger than file size (%" PRIi64 ")",
> -                       off, i, file_size);
> -            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);
>       if (!opts) {
>           goto fail_options;
> @@ -1116,7 +1100,58 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>           error_free(s->migration_blocker);
>           goto fail;
>       }
> +
>       qemu_co_mutex_init(&s->lock);
> +
> +    if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> +        s->header_unclean = true;
> +    }
> +
> +    high_off = highest_offset(s) >> BDRV_SECTOR_BITS;
> +    if (high_off >= s->data_end) {
> +        s->data_end = high_off + s->tracks;
> +    }
> +
> +    /*
> +     * We don't repair the image here if it is opened for checks.
> +     * Also let to work with images in RO mode.
> +     */
> +    if ((flags & BDRV_O_CHECK) || !(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_size ||
> +        le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
> +        BdrvCheckResult res = {0};
> +        Coroutine *co;
> +        ParallelsOpenCheckCo poc = {
> +            .bs = bs,
> +            .res = &res,
> +            .fix = BDRV_FIX_ERRORS | BDRV_FIX_LEAKS,
> +            .ret = -EINPROGRESS
> +        };
> +
> +        if (qemu_in_coroutine()) {
> +            /* From bdrv_co_create.  */
> +            parallels_co_open_check(&poc);
> +        } else {
> +            assert(qemu_get_current_aio_context() == qemu_get_aio_context());
> +            co = qemu_coroutine_create(parallels_co_open_check, &poc);
> +            qemu_coroutine_enter(co);
> +            BDRV_POLL_WHILE(bs, poc.ret == -EINPROGRESS);
> +        }
> +
> +        if (poc.ret < 0) {
> +            error_setg_errno(errp, -poc.ret,
> +                             "Could not repair corrupted image");
> +            goto fail;
> +        }
> +    }
> +
bdrv_check() is your friend. No need to duplicate the code


>       return 0;
>   
>   fail_format:

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

[Denis V. Lunev]

On 9/2/22 10:52, Alexander Ivanov wrote:
> This patchset is based on
> git: https://src.openvz.org/~den/qemu.git parallels
>
> Fix incorrect data end calculation in parallels_open().
>
> Add parallels_handle_leak() and highest_offset() helpers.
>
> Add checking and repairing duplicate offsets in BAT.
>
> Deduplicate highest offset calculation.
>
> Replace fprintf() by qemu_log().
>
> Add check and repair to parallels_open().
>
>
> Alexander Ivanov (6):
>    parallels: Incorrect data end calculation in parallels_open()
>    parallels: Create parallels_handle_leak() to truncate excess clusters
>    parallels: Add checking and repairing duplicate offsets in BAT
>    parallels: Use highest_offset() helper in leak check
>    parallels: Replace fprintf by qemu_log
>    parallels: Image repairing in parallels_open()
>
>   block/parallels.c | 297 +++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 241 insertions(+), 56 deletions(-)
>
common note - please fix formatting in the patch descriptions.
74 characters on a line, empty line between paragraphs.

Den