[PATCH 0/9] parallels: Refactor the code of images checks and fix a bug

[PATCH 0/9] parallels: Refactor the code of images checks and fix a bug

[Alexander Ivanov]

We will add more and more checks of images so we need to reorganize the code.
Put each check to a separate helper function with a separate loop.

Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD for more clean code.

Fix a bug when out of image offset in BAT leads to image inflation.

Replace bdrv_co_pwrite_sync by bdrv_co_flush for writing to the disk
only dirty blocks.

Merge parallels_check_fragmentation to parallels_collect_statistics.


Alexander Ivanov (9):
  parallels: Move check of unclean image to a separate function
  parallels: Move check of cluster outside image to a separate function
  parallels: Move check of leaks to a separate function
  parallels: Move check of fragmentation to a separate function
  parallels: Move statistic collection to a separate function
  parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD
  parallels: Out of image offset in BAT leads to image inflation
  parallels: Replace bdrv_co_pwrite_sync by bdrv_co_flush for BAT
    flushing
  parallels: Merge parallels_check_fragmentation to
    parallels_collect_statistics

 block/parallels.c | 173 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 127 insertions(+), 46 deletions(-)

-- 
2.34.1

[PATCH 1/9] parallels: Move check of unclean image to a separate function

[Alexander Ivanov]

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

diff --git a/block/parallels.c b/block/parallels.c
index a229c06f25..108aa907b8 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -413,6 +413,23 @@ static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
     return ret;
 }
 
+static void parallels_check_unclean(BlockDriverState *bs,
+                                    BdrvCheckResult *res,
+                                    BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+
+    if (s->header_unclean) {
+        fprintf(stderr, "%s image was not closed correctly\n",
+                fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
+        res->corruptions++;
+        if (fix & BDRV_FIX_ERRORS) {
+            /* parallels_close will do the job right */
+            res->corruptions_fixed++;
+            s->header_unclean = false;
+        }
+    }
+}
 
 static int coroutine_fn parallels_co_check(BlockDriverState *bs,
                                            BdrvCheckResult *res,
@@ -431,16 +448,8 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
     }
 
     qemu_co_mutex_lock(&s->lock);
-    if (s->header_unclean) {
-        fprintf(stderr, "%s image was not closed correctly\n",
-                fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
-        res->corruptions++;
-        if (fix & BDRV_FIX_ERRORS) {
-            /* parallels_close will do the job right */
-            res->corruptions_fixed++;
-            s->header_unclean = false;
-        }
-    }
+
+    parallels_check_unclean(bs, res, fix);
 
     res->bfi.total_clusters = s->bat_size;
     res->bfi.compressed_clusters = 0; /* compression is not supported */
-- 
2.34.1

[PATCH 2/9] parallels: Move check of cluster outside image to a separate function

[Alexander Ivanov]

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

diff --git a/block/parallels.c b/block/parallels.c
index 108aa907b8..7b400ecdcc 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -413,6 +413,13 @@ static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
     return ret;
 }
 
+static void parallels_set_bat_entry(BDRVParallelsState *s,
+                                    uint32_t index, uint32_t offset)
+{
+    s->bat_bitmap[index] = offset;
+    bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block, 1);
+}
+
 static void parallels_check_unclean(BlockDriverState *bs,
                                     BdrvCheckResult *res,
                                     BdrvCheckMode fix)
@@ -431,6 +438,47 @@ static void parallels_check_unclean(BlockDriverState *bs,
     }
 }
 
+static int parallels_check_outside_image(BlockDriverState *bs,
+                                         BdrvCheckResult *res,
+                                         BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    uint32_t i;
+    int64_t off, size;
+    int ret;
+    bool flush_bat = false;
+
+    size = bdrv_getlength(bs->file->bs);
+    if (size < 0) {
+        res->check_errors++;
+        return size;
+    }
+
+    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",
+                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
+            res->corruptions++;
+            if (fix & BDRV_FIX_ERRORS) {
+                parallels_set_bat_entry(s, i, 0);
+                res->corruptions_fixed++;
+                flush_bat = true;
+            }
+        }
+    }
+
+    if (flush_bat) {
+        ret = bdrv_co_pwrite_sync(bs->file, 0, s->header_size, s->header, 0);
+        if (ret < 0) {
+            res->check_errors++;
+            return ret;
+        }
+    }
+
+    return 0;
+}
+
 static int coroutine_fn parallels_co_check(BlockDriverState *bs,
                                            BdrvCheckResult *res,
                                            BdrvCheckMode fix)
@@ -439,7 +487,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
     int64_t size, prev_off, high_off;
     int ret;
     uint32_t i;
-    bool flush_bat = false;
 
     size = bdrv_getlength(bs->file->bs);
     if (size < 0) {
@@ -451,6 +498,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
 
     parallels_check_unclean(bs, res, fix);
 
+    ret = parallels_check_outside_image(bs, res, fix);
+    if (ret < 0) {
+        goto out;
+    }
+
     res->bfi.total_clusters = s->bat_size;
     res->bfi.compressed_clusters = 0; /* compression is not supported */
 
@@ -463,20 +515,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
             continue;
         }
 
-        /* cluster outside the image */
-        if (off > size) {
-            fprintf(stderr, "%s cluster %u is outside image\n",
-                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
-            res->corruptions++;
-            if (fix & BDRV_FIX_ERRORS) {
-                prev_off = 0;
-                s->bat_bitmap[i] = 0;
-                res->corruptions_fixed++;
-                flush_bat = true;
-                continue;
-            }
-        }
-
         res->bfi.allocated_clusters++;
         if (off > high_off) {
             high_off = off;
@@ -489,14 +527,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
     }
 
     ret = 0;
-    if (flush_bat) {
-        ret = bdrv_co_pwrite_sync(bs->file, 0, s->header_size, s->header, 0);
-        if (ret < 0) {
-            res->check_errors++;
-            goto out;
-        }
-    }
-
     res->image_end_offset = high_off + s->cluster_size;
     if (size > res->image_end_offset) {
         int64_t count;
-- 
2.34.1

[PATCH 3/9] parallels: Move check of leaks to a separate function

[Alexander Ivanov]

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

diff --git a/block/parallels.c b/block/parallels.c
index 7b400ecdcc..6d4cfb738b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -479,57 +479,31 @@ static int parallels_check_outside_image(BlockDriverState *bs,
     return 0;
 }
 
-static int coroutine_fn parallels_co_check(BlockDriverState *bs,
-                                           BdrvCheckResult *res,
-                                           BdrvCheckMode fix)
+static int parallels_check_leak(BlockDriverState *bs,
+                                BdrvCheckResult *res,
+                                BdrvCheckMode fix)
 {
     BDRVParallelsState *s = bs->opaque;
-    int64_t size, prev_off, high_off;
-    int ret;
     uint32_t i;
-
-    size = bdrv_getlength(bs->file->bs);
-    if (size < 0) {
-        res->check_errors++;
-        return size;
-    }
-
-    qemu_co_mutex_lock(&s->lock);
-
-    parallels_check_unclean(bs, res, fix);
-
-    ret = parallels_check_outside_image(bs, res, fix);
-    if (ret < 0) {
-        goto out;
-    }
-
-    res->bfi.total_clusters = s->bat_size;
-    res->bfi.compressed_clusters = 0; /* compression is not supported */
+    int64_t off, high_off, size, count;
+    int ret;
 
     high_off = 0;
-    prev_off = 0;
     for (i = 0; i < s->bat_size; i++) {
-        int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS;
-        if (off == 0) {
-            prev_off = 0;
-            continue;
-        }
-
-        res->bfi.allocated_clusters++;
+        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
         if (off > high_off) {
             high_off = off;
         }
+    }
 
-        if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
-            res->bfi.fragmented_clusters++;
-        }
-        prev_off = off;
+    size = bdrv_getlength(bs->file->bs);
+    if (size < 0) {
+        res->check_errors++;
+        return size;
     }
 
-    ret = 0;
     res->image_end_offset = high_off + s->cluster_size;
     if (size > res->image_end_offset) {
-        int64_t count;
         count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
         fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n",
                 fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
@@ -547,12 +521,58 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
             if (ret < 0) {
                 error_report_err(local_err);
                 res->check_errors++;
-                goto out;
+                return ret;
             }
             res->leaks_fixed += count;
         }
     }
 
+    return 0;
+}
+
+static int coroutine_fn parallels_co_check(BlockDriverState *bs,
+                                           BdrvCheckResult *res,
+                                           BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int64_t prev_off;
+    int ret;
+    uint32_t i;
+
+    qemu_co_mutex_lock(&s->lock);
+
+    parallels_check_unclean(bs, res, fix);
+
+    ret = parallels_check_outside_image(bs, res, fix);
+    if (ret < 0) {
+        goto out;
+    }
+
+    ret = parallels_check_leak(bs, res, fix);
+    if (ret < 0) {
+        goto out;
+    }
+
+    res->bfi.total_clusters = s->bat_size;
+    res->bfi.compressed_clusters = 0; /* compression is not supported */
+
+    prev_off = 0;
+    for (i = 0; i < s->bat_size; i++) {
+        int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+        if (off == 0) {
+            prev_off = 0;
+            continue;
+        }
+
+        res->bfi.allocated_clusters++;
+
+        if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
+            res->bfi.fragmented_clusters++;
+        }
+        prev_off = off;
+    }
+
+    ret = 0;
 out:
     qemu_co_mutex_unlock(&s->lock);
     return ret;
-- 
2.34.1

[PATCH 4/9] parallels: Move check of fragmentation to a separate function

[Alexander Ivanov]

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

diff --git a/block/parallels.c b/block/parallels.c
index 6d4cfb738b..0edbb812dd 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -530,12 +530,34 @@ static int parallels_check_leak(BlockDriverState *bs,
     return 0;
 }
 
+static void parallels_check_fragmentation(BlockDriverState *bs,
+                                          BdrvCheckResult *res,
+                                          BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    uint32_t i;
+    int64_t off, prev_off;
+
+    prev_off = 0;
+    for (i = 0; i < s->bat_size; i++) {
+        off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+        if (off == 0) {
+            prev_off = 0;
+            continue;
+        }
+        if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
+            res->bfi.fragmented_clusters++;
+        }
+        prev_off = off;
+    }
+
+}
+
 static int coroutine_fn parallels_co_check(BlockDriverState *bs,
                                            BdrvCheckResult *res,
                                            BdrvCheckMode fix)
 {
     BDRVParallelsState *s = bs->opaque;
-    int64_t prev_off;
     int ret;
     uint32_t i;
 
@@ -553,23 +575,13 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
         goto out;
     }
 
+    parallels_check_fragmentation(bs, res, fix);
+
     res->bfi.total_clusters = s->bat_size;
     res->bfi.compressed_clusters = 0; /* compression is not supported */
 
-    prev_off = 0;
     for (i = 0; i < s->bat_size; i++) {
-        int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS;
-        if (off == 0) {
-            prev_off = 0;
-            continue;
-        }
-
         res->bfi.allocated_clusters++;
-
-        if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
-            res->bfi.fragmented_clusters++;
-        }
-        prev_off = off;
     }
 
     ret = 0;
-- 
2.34.1

[PATCH 5/9] parallels: Move statistic collection to a separate function

[Alexander Ivanov]

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

diff --git a/block/parallels.c b/block/parallels.c
index 0edbb812dd..b0982d60d0 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -553,13 +553,29 @@ static void parallels_check_fragmentation(BlockDriverState *bs,
 
 }
 
+static void parallels_collect_statistics(BlockDriverState *bs,
+                                         BdrvCheckResult *res,
+                                         BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    uint32_t i;
+
+    res->bfi.total_clusters = s->bat_size;
+    res->bfi.compressed_clusters = 0; /* compression is not supported */
+
+    for (i = 0; i < s->bat_size; i++) {
+        if (bat2sect(s, i) != 0) {
+            res->bfi.allocated_clusters++;
+        }
+    }
+}
+
 static int coroutine_fn parallels_co_check(BlockDriverState *bs,
                                            BdrvCheckResult *res,
                                            BdrvCheckMode fix)
 {
     BDRVParallelsState *s = bs->opaque;
     int ret;
-    uint32_t i;
 
     qemu_co_mutex_lock(&s->lock);
 
@@ -577,12 +593,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
 
     parallels_check_fragmentation(bs, res, fix);
 
-    res->bfi.total_clusters = s->bat_size;
-    res->bfi.compressed_clusters = 0; /* compression is not supported */
-
-    for (i = 0; i < s->bat_size; i++) {
-        res->bfi.allocated_clusters++;
-    }
+    parallels_collect_statistics(bs, res, fix);
 
     ret = 0;
 out:
-- 
2.34.1

[PATCH 6/9] parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD

[Alexander Ivanov]

Replace the way that we use mutex in parallels_co_check() for more clean code.

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

diff --git a/block/parallels.c b/block/parallels.c
index b0982d60d0..3cb5452613 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -577,28 +577,25 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
     BDRVParallelsState *s = bs->opaque;
     int ret;
 
-    qemu_co_mutex_lock(&s->lock);
+    WITH_QEMU_LOCK_GUARD(&s->lock);
 
     parallels_check_unclean(bs, res, fix);
 
     ret = parallels_check_outside_image(bs, res, fix);
     if (ret < 0) {
-        goto out;
+        return ret;
     }
 
     ret = parallels_check_leak(bs, res, fix);
     if (ret < 0) {
-        goto out;
+        return ret;
     }
 
     parallels_check_fragmentation(bs, res, fix);
 
     parallels_collect_statistics(bs, res, fix);
 
-    ret = 0;
-out:
-    qemu_co_mutex_unlock(&s->lock);
-    return ret;
+    return 0;
 }
 
 
-- 
2.34.1

[PATCH 7/9] parallels: Out of image offset in BAT leads to image inflation

[Alexander Ivanov]

When an image is opened, data_end field in BDRVParallelsState
is setted as the biggest offset in the BAT plus cluster size.
If there is a corrupted offset pointing outside the image,
the image size increase accordingly. It potentially leads
to attempts to create a file size of petabytes.

Set the data_end field with the original file size if the image
was opened for checking and repairing purposes or raise an error.

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

diff --git a/block/parallels.c b/block/parallels.c
index 3cb5452613..72cf7499c1 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -811,6 +811,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVParallelsState *s = bs->opaque;
     ParallelsHeader ph;
     int ret, size, i;
+    int64_t file_size;
     QemuOpts *opts = NULL;
     Error *local_err = NULL;
     char *buf;
@@ -890,6 +891,22 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
+    file_size = bdrv_getlength(bs->file->bs);
+    if (file_size < 0) {
+        goto fail;
+    }
+
+    file_size >>= BDRV_SECTOR_BITS;
+    if (s->data_end > file_size) {
+        if (flags & BDRV_O_CHECK) {
+            s->data_end = file_size;
+        } else {
+            error_setg(errp, "parallels: Offset in BAT is out of image");
+            ret = -EINVAL;
+            goto fail;
+        }
+    }
+
     if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
         /* Image was not closed correctly. The check is mandatory */
         s->header_unclean = true;
-- 
2.34.1

[PATCH 8/9] parallels: Replace bdrv_co_pwrite_sync by bdrv_co_flush for BAT flushing

[Alexander Ivanov]

It's too costly to write all the BAT to the disk. Let the flush function
write only dirty blocks.

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

diff --git a/block/parallels.c b/block/parallels.c
index 72cf7499c1..38b1482e81 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -469,7 +469,7 @@ static int parallels_check_outside_image(BlockDriverState *bs,
     }
 
     if (flush_bat) {
-        ret = bdrv_co_pwrite_sync(bs->file, 0, s->header_size, s->header, 0);
+        ret = bdrv_co_flush(bs);
         if (ret < 0) {
             res->check_errors++;
             return ret;
-- 
2.34.1

[PATCH 9/9] parallels: Merge parallels_check_fragmentation to parallels_collect_statistics

[Alexander Ivanov]

Frgamentation is a part of statistics so it is better to count the statistics
in one function.

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

diff --git a/block/parallels.c b/block/parallels.c
index 38b1482e81..11597c5dc4 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -530,13 +530,16 @@ static int parallels_check_leak(BlockDriverState *bs,
     return 0;
 }
 
-static void parallels_check_fragmentation(BlockDriverState *bs,
-                                          BdrvCheckResult *res,
-                                          BdrvCheckMode fix)
+static void parallels_collect_statistics(BlockDriverState *bs,
+                                         BdrvCheckResult *res,
+                                         BdrvCheckMode fix)
 {
     BDRVParallelsState *s = bs->opaque;
-    uint32_t i;
     int64_t off, prev_off;
+    uint32_t i;
+
+    res->bfi.total_clusters = s->bat_size;
+    res->bfi.compressed_clusters = 0; /* compression is not supported */
 
     prev_off = 0;
     for (i = 0; i < s->bat_size; i++) {
@@ -549,24 +552,8 @@ static void parallels_check_fragmentation(BlockDriverState *bs,
             res->bfi.fragmented_clusters++;
         }
         prev_off = off;
-    }
 
-}
-
-static void parallels_collect_statistics(BlockDriverState *bs,
-                                         BdrvCheckResult *res,
-                                         BdrvCheckMode fix)
-{
-    BDRVParallelsState *s = bs->opaque;
-    uint32_t i;
-
-    res->bfi.total_clusters = s->bat_size;
-    res->bfi.compressed_clusters = 0; /* compression is not supported */
-
-    for (i = 0; i < s->bat_size; i++) {
-        if (bat2sect(s, i) != 0) {
-            res->bfi.allocated_clusters++;
-        }
+        res->bfi.allocated_clusters++;
     }
 }
 
@@ -591,8 +578,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
         return ret;
     }
 
-    parallels_check_fragmentation(bs, res, fix);
-
     parallels_collect_statistics(bs, res, fix);
 
     return 0;
-- 
2.34.1

Re: [PATCH 5/9] parallels: Move statistic collection to a separate function

[Denis V. Lunev]

On 08.08.2022 14:07, Alexander Ivanov wrote:
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 25 ++++++++++++++++++-------
>   1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 0edbb812dd..b0982d60d0 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -553,13 +553,29 @@ static void parallels_check_fragmentation(BlockDriverState *bs,
>   
>   }
>   
> +static void parallels_collect_statistics(BlockDriverState *bs,
> +                                         BdrvCheckResult *res,
> +                                         BdrvCheckMode fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    uint32_t i;
> +
> +    res->bfi.total_clusters = s->bat_size;
> +    res->bfi.compressed_clusters = 0; /* compression is not supported */
> +
> +    for (i = 0; i < s->bat_size; i++) {
> +        if (bat2sect(s, i) != 0) {
> +            res->bfi.allocated_clusters++;
> +        }
> +    }
> +}
> +
>   static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>                                              BdrvCheckResult *res,
>                                              BdrvCheckMode fix)
>   {
>       BDRVParallelsState *s = bs->opaque;
>       int ret;
> -    uint32_t i;
>   
>       qemu_co_mutex_lock(&s->lock);
>   
> @@ -577,12 +593,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>   
>       parallels_check_fragmentation(bs, res, fix);
>   
> -    res->bfi.total_clusters = s->bat_size;
> -    res->bfi.compressed_clusters = 0; /* compression is not supported */
> -
> -    for (i = 0; i < s->bat_size; i++) {
> -        res->bfi.allocated_clusters++;
> -    }
> +    parallels_collect_statistics(bs, res, fix);
>   
>       ret = 0;
>   out:
for me fragmentation dances are pure statistics thing, this should be
done in once function/patch

Re: [PATCH 2/9] parallels: Move check of cluster outside image to a separate function

[Denis V. Lunev]

On 08.08.2022 14:07, Alexander Ivanov wrote:
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 76 +++++++++++++++++++++++++++++++++--------------
>   1 file changed, 53 insertions(+), 23 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 108aa907b8..7b400ecdcc 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -413,6 +413,13 @@ static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
>       return ret;
>   }
>   
> +static void parallels_set_bat_entry(BDRVParallelsState *s,
> +                                    uint32_t index, uint32_t offset)
> +{
> +    s->bat_bitmap[index] = offset;
> +    bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block, 1);
> +}
> +
>   static void parallels_check_unclean(BlockDriverState *bs,
>                                       BdrvCheckResult *res,
>                                       BdrvCheckMode fix)

This helper seems unrelated to the patch subject and should be done 
separately.

> @@ -431,6 +438,47 @@ static void parallels_check_unclean(BlockDriverState *bs,
>       }
>   }
>   
> +static int parallels_check_outside_image(BlockDriverState *bs,
> +                                         BdrvCheckResult *res,
> +                                         BdrvCheckMode fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    uint32_t i;
> +    int64_t off, size;
> +    int ret;
> +    bool flush_bat = false;
> +
> +    size = bdrv_getlength(bs->file->bs);
> +    if (size < 0) {
> +        res->check_errors++;
> +        return size;
> +    }
> +
> +    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",
> +                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
> +            res->corruptions++;
> +            if (fix & BDRV_FIX_ERRORS) {
> +                parallels_set_bat_entry(s, i, 0);
> +                res->corruptions_fixed++;
> +                flush_bat = true;
> +            }
> +        }
> +    }
> +
> +    if (flush_bat) {
> +        ret = bdrv_co_pwrite_sync(bs->file, 0, s->header_size, s->header, 0);
> +        if (ret < 0) {
> +            res->check_errors++;
> +            return ret;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>   static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>                                              BdrvCheckResult *res,
>                                              BdrvCheckMode fix)
> @@ -439,7 +487,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>       int64_t size, prev_off, high_off;
>       int ret;
>       uint32_t i;
> -    bool flush_bat = false;
>   
>       size = bdrv_getlength(bs->file->bs);
>       if (size < 0) {
> @@ -451,6 +498,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>   
>       parallels_check_unclean(bs, res, fix);
>   
> +    ret = parallels_check_outside_image(bs, res, fix);
> +    if (ret < 0) {
> +        goto out;
> +    }
> +
>       res->bfi.total_clusters = s->bat_size;
>       res->bfi.compressed_clusters = 0; /* compression is not supported */
>   
> @@ -463,20 +515,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>               continue;
>           }
>   
> -        /* cluster outside the image */
> -        if (off > size) {
> -            fprintf(stderr, "%s cluster %u is outside image\n",
> -                    fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
> -            res->corruptions++;
> -            if (fix & BDRV_FIX_ERRORS) {
> -                prev_off = 0;
> -                s->bat_bitmap[i] = 0;
> -                res->corruptions_fixed++;
> -                flush_bat = true;
> -                continue;
> -            }
> -        }
> -
>           res->bfi.allocated_clusters++;
>           if (off > high_off) {
>               high_off = off;
> @@ -489,14 +527,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>       }
>   
>       ret = 0;
> -    if (flush_bat) {
> -        ret = bdrv_co_pwrite_sync(bs->file, 0, s->header_size, s->header, 0);
> -        if (ret < 0) {
> -            res->check_errors++;
> -            goto out;
> -        }
> -    }
> -
>       res->image_end_offset = high_off + s->cluster_size;
>       if (size > res->image_end_offset) {
>           int64_t count;

Re: [PATCH 8/9] parallels: Replace bdrv_co_pwrite_sync by bdrv_co_flush for BAT flushing

[Denis V. Lunev]

On 08.08.2022 14:07, Alexander Ivanov wrote:
> It's too costly to write all the BAT to the disk. Let the flush function
> write only dirty blocks.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 72cf7499c1..38b1482e81 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -469,7 +469,7 @@ static int parallels_check_outside_image(BlockDriverState *bs,
>       }
>   
>       if (flush_bat) {
> -        ret = bdrv_co_pwrite_sync(bs->file, 0, s->header_size, s->header, 0);
> +        ret = bdrv_co_flush(bs);
>           if (ret < 0) {
>               res->check_errors++;
>               return ret;
no-no-no, absolutely no.

Please drop ALL flush_bat dances. We do not need them once we have switched
to parallels_set_bat_entry. We do NOT need to track this anymore, this is
handled inside and thus flush should be made OUTSIDE of helpers in the
generic code of the parallels_co_check

Re: [PATCH 9/9] parallels: Merge parallels_check_fragmentation to parallels_collect_statistics

[Denis V. Lunev]

On 08.08.2022 14:07, Alexander Ivanov wrote:
> Frgamentation is a part of statistics so it is better to count the statistics
> in one function.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 31 ++++++++-----------------------
>   1 file changed, 8 insertions(+), 23 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 38b1482e81..11597c5dc4 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -530,13 +530,16 @@ static int parallels_check_leak(BlockDriverState *bs,
>       return 0;
>   }
>   
> -static void parallels_check_fragmentation(BlockDriverState *bs,
> -                                          BdrvCheckResult *res,
> -                                          BdrvCheckMode fix)
> +static void parallels_collect_statistics(BlockDriverState *bs,
> +                                         BdrvCheckResult *res,
> +                                         BdrvCheckMode fix)
>   {
>       BDRVParallelsState *s = bs->opaque;
> -    uint32_t i;
>       int64_t off, prev_off;
> +    uint32_t i;
> +
> +    res->bfi.total_clusters = s->bat_size;
> +    res->bfi.compressed_clusters = 0; /* compression is not supported */
>   
>       prev_off = 0;
>       for (i = 0; i < s->bat_size; i++) {
> @@ -549,24 +552,8 @@ static void parallels_check_fragmentation(BlockDriverState *bs,
>               res->bfi.fragmented_clusters++;
>           }
>           prev_off = off;
> -    }
>   
> -}
> -
> -static void parallels_collect_statistics(BlockDriverState *bs,
> -                                         BdrvCheckResult *res,
> -                                         BdrvCheckMode fix)
> -{
> -    BDRVParallelsState *s = bs->opaque;
> -    uint32_t i;
> -
> -    res->bfi.total_clusters = s->bat_size;
> -    res->bfi.compressed_clusters = 0; /* compression is not supported */
> -
> -    for (i = 0; i < s->bat_size; i++) {
> -        if (bat2sect(s, i) != 0) {
> -            res->bfi.allocated_clusters++;
> -        }
> +        res->bfi.allocated_clusters++;
>       }
>   }
>   
> @@ -591,8 +578,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>           return ret;
>       }
>   
> -    parallels_check_fragmentation(bs, res, fix);
> -
>       parallels_collect_statistics(bs, res, fix);
>   
>       return 0;
please squash this and helper creation patches. There is no sense to have
this as a separate things.

Re: [PATCH 2/9] parallels: Move check of cluster outside image to a separate function

[Denis V. Lunev]

On 08.08.2022 14:19, Denis V. Lunev wrote:
> On 08.08.2022 14:07, Alexander Ivanov wrote:
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>>   block/parallels.c | 76 +++++++++++++++++++++++++++++++++--------------
>>   1 file changed, 53 insertions(+), 23 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 108aa907b8..7b400ecdcc 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -413,6 +413,13 @@ static coroutine_fn int 
>> parallels_co_readv(BlockDriverState *bs,
>>       return ret;
>>   }
>>   +static void parallels_set_bat_entry(BDRVParallelsState *s,
>> +                                    uint32_t index, uint32_t offset)
>> +{
>> +    s->bat_bitmap[index] = offset;
>> +    bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / 
>> s->bat_dirty_block, 1);
>> +}
>> +
>>   static void parallels_check_unclean(BlockDriverState *bs,
>>                                       BdrvCheckResult *res,
>>                                       BdrvCheckMode fix)
>
> This helper seems unrelated to the patch subject and should be done 
> separately.

more specifically - you have created the helper BUT you have not used
it at ALL places where we update BAT. That is seriously wrong from the
code completness POW. If you have getter - you MUST use it everywhere,
even within allocate_clusters().

Normally you create helper, f.e. refactoring code of allocate cluster 
promising
that the code will be reused later (non-functional changes) and after that
in the separate patch you spread its usage refactoring other code, f.e.
to avoid flush_bat tracking.

Re: [PATCH 1/9] parallels: Move check of unclean image to a separate function

[Denis V. Lunev]

On 08.08.2022 14:07, Alexander Ivanov wrote:
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 29 +++++++++++++++++++----------
>   1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index a229c06f25..108aa907b8 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -413,6 +413,23 @@ static coroutine_fn int parallels_co_readv(BlockDriverState *bs,
>       return ret;
>   }
>   
> +static void parallels_check_unclean(BlockDriverState *bs,
> +                                    BdrvCheckResult *res,
> +                                    BdrvCheckMode fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +
> +    if (s->header_unclean) {
I'd better revert this condition if we have moved code to helper.

> +        fprintf(stderr, "%s image was not closed correctly\n",
> +                fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
> +        res->corruptions++;
> +        if (fix & BDRV_FIX_ERRORS) {
> +            /* parallels_close will do the job right */
> +            res->corruptions_fixed++;
> +            s->header_unclean = false;
> +        }
> +    }
> +}
>   
>   static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>                                              BdrvCheckResult *res,
> @@ -431,16 +448,8 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>       }
>   
>       qemu_co_mutex_lock(&s->lock);
> -    if (s->header_unclean) {
> -        fprintf(stderr, "%s image was not closed correctly\n",
> -                fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR");
> -        res->corruptions++;
> -        if (fix & BDRV_FIX_ERRORS) {
> -            /* parallels_close will do the job right */
> -            res->corruptions_fixed++;
> -            s->header_unclean = false;
> -        }
> -    }
> +
> +    parallels_check_unclean(bs, res, fix);
>   
>       res->bfi.total_clusters = s->bat_size;
>       res->bfi.compressed_clusters = 0; /* compression is not supported */

Re: [PATCH 7/9] parallels: Out of image offset in BAT leads to image inflation

[Denis V. Lunev]

On 08.08.2022 14:07, Alexander Ivanov wrote:
> When an image is opened, data_end field in BDRVParallelsState
> is setted as the biggest offset in the BAT plus cluster size.
> If there is a corrupted offset pointing outside the image,
> the image size increase accordingly. It potentially leads
> to attempts to create a file size of petabytes.
>
> Set the data_end field with the original file size if the image
> was opened for checking and repairing purposes or raise an error.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 3cb5452613..72cf7499c1 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -811,6 +811,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>       BDRVParallelsState *s = bs->opaque;
>       ParallelsHeader ph;
>       int ret, size, i;
> +    int64_t file_size;
>       QemuOpts *opts = NULL;
>       Error *local_err = NULL;
>       char *buf;
> @@ -890,6 +891,22 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>           }
>       }
>   
> +    file_size = bdrv_getlength(bs->file->bs);
> +    if (file_size < 0) {
> +        goto fail;
> +    }
> +
> +    file_size >>= BDRV_SECTOR_BITS;
> +    if (s->data_end > file_size) {
> +        if (flags & BDRV_O_CHECK) {
> +            s->data_end = file_size;
> +        } else {
> +            error_setg(errp, "parallels: Offset in BAT is out of image");
> +            ret = -EINVAL;
> +            goto fail;
> +        }
> +    }
> +
>       if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
>           /* Image was not closed correctly. The check is mandatory */
>           s->header_unclean = true;

for me it would be more logical to have this check inside the loop,
calculating data_offset. Though this is personal.