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

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

[Alexander Ivanov]

Fix image inflation when offset in BAT is out of image.

Replace whole BAT syncing by flushing only dirty blocks.

Move all the checks outside the main check function in
separate functions

Use WITH_QEMU_LOCK_GUARD for more clean code.

Alexander Ivanov (8):
  parallels: Out of image offset in BAT leads to image inflation
  parallels: Move BAT entry setting to a separate function
  parallels: Replace bdrv_co_pwrite_sync by bdrv_co_flush for BAT
    flushing
  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 statistic collection to a separate function
  parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD

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

-- 
2.34.1

[PATCH v2 1/8] 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.

v2: No changes.

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 a229c06f25..a76cf9d993 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -732,6 +732,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;
@@ -811,6 +812,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 v2 2/8] parallels: Move BAT entry setting to a separate function

[Alexander Ivanov]

Will need to set BAT entry in multiple places.
Move the code of settings entries and marking relevant blocks dirty
to a separate helper parallels_set_bat_entry.

v2: A new patch - a part of a splitted patch.

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

diff --git a/block/parallels.c b/block/parallels.c
index a76cf9d993..7f68f3cbc9 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -165,6 +165,13 @@ static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
     return start_off;
 }
 
+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 int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
                                  int nb_sectors, int *pnum)
 {
@@ -250,10 +257,9 @@ static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
     }
 
     for (i = 0; i < to_allocate; i++) {
-        s->bat_bitmap[idx + i] = cpu_to_le32(s->data_end / s->off_multiplier);
+        parallels_set_bat_entry(s, idx + i,
+                                cpu_to_le32(s->data_end / s->off_multiplier));
         s->data_end += s->tracks;
-        bitmap_set(s->bat_dirty_bmap,
-                   bat_entry_off(idx + i) / s->bat_dirty_block, 1);
     }
 
     return bat2sect(s, idx) + sector_num % s->tracks;
-- 
2.34.1

[PATCH v2 3/8] 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.
Use parallels_set_bat_entry for setting a BAT entry and marking a relevant
block as dirty.
Move bdrv_co_flush call outside the locked area.

v2: Patch order was changed so the replacement is done in parallels_co_check.
    Now we use a helper to set BAT entry and mark the block dirty.

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

diff --git a/block/parallels.c b/block/parallels.c
index 7f68f3cbc9..6879ea4597 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -428,7 +428,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) {
@@ -467,9 +466,8 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
             res->corruptions++;
             if (fix & BDRV_FIX_ERRORS) {
                 prev_off = 0;
-                s->bat_bitmap[i] = 0;
+                parallels_set_bat_entry(s, i, 0);
                 res->corruptions_fixed++;
-                flush_bat = true;
                 continue;
             }
         }
@@ -485,15 +483,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
         prev_off = off;
     }
 
-    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;
@@ -522,6 +511,12 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
 
 out:
     qemu_co_mutex_unlock(&s->lock);
+
+    ret = bdrv_co_flush(bs);
+    if (ret < 0) {
+        res->check_errors++;
+    }
+
     return ret;
 }
 
-- 
2.34.1

[PATCH v2 4/8] parallels: Move check of unclean image to a separate function

[Alexander Ivanov]

v2: Revert the condition with s->header_unclean.

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

diff --git a/block/parallels.c b/block/parallels.c
index 6879ea4597..c53b2810cf 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -419,6 +419,25 @@ 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) {
+        return;
+    }
+
+    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,
@@ -436,16 +455,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 v2 5/8] parallels: Move check of cluster outside image to a separate function

[Alexander Ivanov]

v2: Move unrelated helper parallels_set_bat_entry creation to
    a separate patch.

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

diff --git a/block/parallels.c b/block/parallels.c
index c53b2810cf..12104ba5ad 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -439,6 +439,36 @@ 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;
+
+    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++;
+            }
+        }
+    }
+
+    return 0;
+}
+
 static int coroutine_fn parallels_co_check(BlockDriverState *bs,
                                            BdrvCheckResult *res,
                                            BdrvCheckMode fix)
@@ -458,6 +488,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 */
 
@@ -470,19 +505,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;
-                parallels_set_bat_entry(s, i, 0);
-                res->corruptions_fixed++;
-                continue;
-            }
-        }
-
         res->bfi.allocated_clusters++;
         if (off > high_off) {
             high_off = off;
-- 
2.34.1

[PATCH v2 6/8] parallels: Move check of leaks to a separate function

[Alexander Ivanov]

v2: No changes.

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

diff --git a/block/parallels.c b/block/parallels.c
index 12104ba5ad..8737eadfb4 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -469,14 +469,13 @@ 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;
+    int64_t size, off, high_off, count;
+    int i, ret;
 
     size = bdrv_getlength(bs->file->bs);
     if (size < 0) {
@@ -484,41 +483,16 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
         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 */
-
     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;
     }
 
     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",
@@ -536,11 +510,56 @@ 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;
+    }
 
 out:
     qemu_co_mutex_unlock(&s->lock);
-- 
2.34.1

[PATCH v2 7/8] parallels: Move statistic collection to a separate function

[Alexander Ivanov]

v2: Move fragmentation counting code to this function too.

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

diff --git a/block/parallels.c b/block/parallels.c
index 8737eadfb4..d0364182bb 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -518,48 +518,56 @@ static int parallels_check_leak(BlockDriverState *bs,
     return 0;
 }
 
-static int coroutine_fn parallels_co_check(BlockDriverState *bs,
-                                           BdrvCheckResult *res,
-                                           BdrvCheckMode fix)
+static void parallels_collect_statistics(BlockDriverState *bs,
+                                         BdrvCheckResult *res,
+                                         BdrvCheckMode fix)
 {
     BDRVParallelsState *s = bs->opaque;
-    int64_t prev_off;
-    int ret;
+    int64_t off, prev_off;
     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;
+        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;
+        res->bfi.allocated_clusters++;
     }
+}
+
+static int coroutine_fn parallels_co_check(BlockDriverState *bs,
+                                           BdrvCheckResult *res,
+                                           BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int ret;
+
+    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;
+    }
+
+    parallels_collect_statistics(bs, res, fix);
 
 out:
     qemu_co_mutex_unlock(&s->lock);
-- 
2.34.1

[PATCH v2 8/8] parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD

[Alexander Ivanov]

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

v2: Fix an incorrect usage of WITH_QEMU_LOCK_GUARD.

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

diff --git a/block/parallels.c b/block/parallels.c
index d0364182bb..e124a8bb7d 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -553,24 +553,22 @@ 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);
 
-    parallels_check_unclean(bs, res, fix);
+        ret = parallels_check_outside_image(bs, res, fix);
+        if (ret < 0) {
+            return ret;
+        }
 
-    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;
-    }
+        ret = parallels_check_leak(bs, res, fix);
+        if (ret < 0) {
+            return ret;
+        }
 
-    parallels_collect_statistics(bs, res, fix);
+        parallels_collect_statistics(bs, res, fix);
 
-out:
-    qemu_co_mutex_unlock(&s->lock);
+    }
 
     ret = bdrv_co_flush(bs);
     if (ret < 0) {
-- 
2.34.1

Re: [PATCH v2 1/8] parallels: Out of image offset in BAT leads to image inflation

[Denis V. Lunev]

On 11.08.2022 17:00, 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.
>
> v2: No changes.
Changelog should be below ---
In that case it will not be merged.

There are a lot of typos/mistakes inside, I'd better use the comment
below.

"data_end field in BDRVParallelsState is set to the biggest offset present
in BAT. If this offset is outside of the image, any further write will 
create
the cluster at this offset and/or the image will be truncated to this
offset on close. This is definitely not correct and should be fixed."

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

> 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 a229c06f25..a76cf9d993 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -732,6 +732,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;
> @@ -811,6 +812,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;

Re: [PATCH v2 2/8] parallels: Move BAT entry setting to a separate function

[Denis V. Lunev]

On 11.08.2022 17:00, Alexander Ivanov wrote:
> Will need to set BAT entry in multiple places.
> Move the code of settings entries and marking relevant blocks dirty
> to a separate helper parallels_set_bat_entry.
The comment and the patch text is ambiguous.
You say that we need to set BAT in multiple places
but patch changes one place only. I think that it would
be better to say like the following:

"parallels: create parallels_set_bat_entry_helper() to assign BAT value

This helper will be reused in next patches during parallels_co_check
rework to simplify its code"

> v2: A new patch - a part of a splitted patch.
Same note about version tracking

With above taken into account:
Reviewed-by: Denis V. Lunev <den@openvz.org>

> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index a76cf9d993..7f68f3cbc9 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -165,6 +165,13 @@ static int64_t block_status(BDRVParallelsState *s, int64_t sector_num,
>       return start_off;
>   }
>   
> +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 int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
>                                    int nb_sectors, int *pnum)
>   {
> @@ -250,10 +257,9 @@ static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
>       }
>   
>       for (i = 0; i < to_allocate; i++) {
> -        s->bat_bitmap[idx + i] = cpu_to_le32(s->data_end / s->off_multiplier);
> +        parallels_set_bat_entry(s, idx + i,
> +                                cpu_to_le32(s->data_end / s->off_multiplier));
>           s->data_end += s->tracks;
> -        bitmap_set(s->bat_dirty_bmap,
> -                   bat_entry_off(idx + i) / s->bat_dirty_block, 1);
>       }
>   
>       return bat2sect(s, idx) + sector_num % s->tracks;

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

[Denis V. Lunev]

Use generic infrastructure for BAT writing in parallels_co_check()

On 11.08.2022 17:00, Alexander Ivanov wrote:
> It's too costly to write all the BAT to the disk. Let the flush function
> write only dirty blocks.
> Use parallels_set_bat_entry for setting a BAT entry and marking a relevant
> block as dirty.
> Move bdrv_co_flush call outside the locked area.
This is not correct too:
- with a lot of cases inside parallels_co_check, which will be split to
   smaller functions, we would like to avoid writing BAT once each
   stage is complete

Thus the comment should look like the following:
"BAT is written in the context of conventional operations over
the image inside bdrv_co_flush() when it calls
parallels_co_flush_to_os() callback. Thus we should not
modify BAT array directly, but call parallels_set_bat_entry()
helper and bdrv_co_flush() further on. After that there is no
need to manually write BAT and track its modification.

This makes code more generic and allows to split
parallels_set_bat_entry() for independent pieces."

> v2: Patch order was changed so the replacement is done in parallels_co_check.
>      Now we use a helper to set BAT entry and mark the block dirty.
Same note about changelog as n other patches.

Side note. I like Linux kernel a lot and there is a good
guide in. Please look how commit message
https://www.kernel.org/doc/html/latest/process/submitting-patches.html

May be you could spend some more time on commit
message.

With a better message:
Reviewed-by: Denis V. Lunev <den@openvz.org>

> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 19 +++++++------------
>   1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 7f68f3cbc9..6879ea4597 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -428,7 +428,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) {
> @@ -467,9 +466,8 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>               res->corruptions++;
>               if (fix & BDRV_FIX_ERRORS) {
>                   prev_off = 0;
> -                s->bat_bitmap[i] = 0;
> +                parallels_set_bat_entry(s, i, 0);
>                   res->corruptions_fixed++;
> -                flush_bat = true;
>                   continue;
>               }
>           }
> @@ -485,15 +483,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>           prev_off = off;
>       }
>   
> -    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;
> @@ -522,6 +511,12 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>   
>   out:
>       qemu_co_mutex_unlock(&s->lock);
> +
> +    ret = bdrv_co_flush(bs);
> +    if (ret < 0) {
> +        res->check_errors++;
> +    }
> +
>       return ret;
>   }
>   

Re: [PATCH v2 4/8] parallels: Move check of unclean image to a separate function

[Denis V. Lunev]

On 11.08.2022 17:00, Alexander Ivanov wrote:
> v2: Revert the condition with s->header_unclean.
same comment about change log as previously

And commit message misses motivation part, why we are
doing this rework. What is the goal of this change?

The code part is clean.

> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 31 +++++++++++++++++++++----------
>   1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 6879ea4597..c53b2810cf 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -419,6 +419,25 @@ 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) {
> +        return;
> +    }
> +
> +    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,
> @@ -436,16 +455,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 v2 5/8] parallels: Move check of cluster outside image to a separate function

[Denis V. Lunev]

On 11.08.2022 17:00, Alexander Ivanov wrote:
> v2: Move unrelated helper parallels_set_bat_entry creation to
>      a separate patch.
same notes as for previous patch


> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 48 ++++++++++++++++++++++++++++++++++-------------
>   1 file changed, 35 insertions(+), 13 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index c53b2810cf..12104ba5ad 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -439,6 +439,36 @@ 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;
> +
> +    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++;
> +            }
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>   static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>                                              BdrvCheckResult *res,
>                                              BdrvCheckMode fix)
> @@ -458,6 +488,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 */
>   
> @@ -470,19 +505,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;
> -                parallels_set_bat_entry(s, i, 0);
> -                res->corruptions_fixed++;
> -                continue;
> -            }
> -        }
> -
>           res->bfi.allocated_clusters++;
>           if (off > high_off) {
>               high_off = off;

Re: [PATCH v2 6/8] parallels: Move check of leaks to a separate function

[Denis V. Lunev]

On 11.08.2022 17:00, Alexander Ivanov wrote:
> v2: No changes.
same notes about motivation, changelog as before

> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 85 +++++++++++++++++++++++++++++------------------
>   1 file changed, 52 insertions(+), 33 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 12104ba5ad..8737eadfb4 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -469,14 +469,13 @@ 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;
> +    int64_t size, off, high_off, count;
> +    int i, ret;
>   
>       size = bdrv_getlength(bs->file->bs);
>       if (size < 0) {
> @@ -484,41 +483,16 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>           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 */
> -
>       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;
>       }
>   
>       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",
> @@ -536,11 +510,56 @@ 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;
> +
> +

please remove extra empty line. This looks like an accident
> +    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;
> +    }
>   
>   out:
>       qemu_co_mutex_unlock(&s->lock);

Re: [PATCH v2 7/8] parallels: Move statistic collection to a separate function

[Denis V. Lunev]

On 11.08.2022 17:00, Alexander Ivanov wrote:
> v2: Move fragmentation counting code to this function too.

same note here about ChnageLog and motivation

> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 54 +++++++++++++++++++++++++++--------------------
>   1 file changed, 31 insertions(+), 23 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 8737eadfb4..d0364182bb 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -518,48 +518,56 @@ static int parallels_check_leak(BlockDriverState *bs,
>       return 0;
>   }
>   
> -static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> -                                           BdrvCheckResult *res,
> -                                           BdrvCheckMode fix)
> +static void parallels_collect_statistics(BlockDriverState *bs,
> +                                         BdrvCheckResult *res,
> +                                         BdrvCheckMode fix)
>   {
>       BDRVParallelsState *s = bs->opaque;
> -    int64_t prev_off;
> -    int ret;
> +    int64_t off, prev_off;
>       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;
> +        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;
> +        res->bfi.allocated_clusters++;
>       }
> +}
> +
> +static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> +                                           BdrvCheckResult *res,
> +                                           BdrvCheckMode fix)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    int ret;
> +
> +    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;
> +    }
> +
> +    parallels_collect_statistics(bs, res, fix);
>   
>   out:
>       qemu_co_mutex_unlock(&s->lock);

Re: [PATCH v2 8/8] parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD

[Denis V. Lunev]

On 11.08.2022 17:00, Alexander Ivanov wrote:
> Replace the way we use mutex in parallels_co_check() for more clean code.
I think that "cleaness" is the same, but new code would be just shorter ;)
or less error prone.

> v2: Fix an incorrect usage of WITH_QEMU_LOCK_GUARD.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 26 ++++++++++++--------------
>   1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index d0364182bb..e124a8bb7d 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -553,24 +553,22 @@ 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);
>   
> -    parallels_check_unclean(bs, res, fix);
> +        ret = parallels_check_outside_image(bs, res, fix);
> +        if (ret < 0) {
> +            return ret;
> +        }
>   
> -    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;
> -    }
> +        ret = parallels_check_leak(bs, res, fix);
> +        if (ret < 0) {
> +            return ret;
> +        }
>   
> -    parallels_collect_statistics(bs, res, fix);
> +        parallels_collect_statistics(bs, res, fix);
>   
> -out:
> -    qemu_co_mutex_unlock(&s->lock);
> +    }
>   
>       ret = bdrv_co_flush(bs);
>       if (ret < 0) {

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

[Denis V. Lunev]

On 11.08.2022 17:00, Alexander Ivanov wrote:
> Fix image inflation when offset in BAT is out of image.
>
> Replace whole BAT syncing by flushing only dirty blocks.
>
> Move all the checks outside the main check function in
> separate functions
>
> Use WITH_QEMU_LOCK_GUARD for more clean code.
>
> Alexander Ivanov (8):
>    parallels: Out of image offset in BAT leads to image inflation
>    parallels: Move BAT entry setting to a separate function
>    parallels: Replace bdrv_co_pwrite_sync by bdrv_co_flush for BAT
>      flushing
>    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 statistic collection to a separate function
>    parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD
>
>   block/parallels.c | 188 ++++++++++++++++++++++++++++++++--------------
>   1 file changed, 132 insertions(+), 56 deletions(-)
>
I believe that this series is ready to go once commit
messages will be improved.

Den