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

[PATCH v3 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 simplier code.


Alexander Ivanov (8):
  parallels: Out of image offset in BAT leads to image inflation
  parallels: create parallels_set_bat_entry_helper() to assign BAT value
  parallels: Use generic infrastructure for BAT writing in
    parallels_co_check()
  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 v3 1/8] parallels: Out of image offset in BAT leads to image inflation

[Alexander Ivanov]

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.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
v2: No change.
v3: Fix commit message.

 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 v3 2/8] parallels: create parallels_set_bat_entry_helper() to assign BAT value

[Alexander Ivanov]

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

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
v2: A new patch - a part of a splitted patch.
v3: Fix commit message.

 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 v3 3/8] parallels: Use generic infrastructure for BAT writing in parallels_co_check()

[Alexander Ivanov]

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.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
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.
v3: Fix commit message.

 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 v3 4/8] parallels: Move check of unclean image to a separate function

[Alexander Ivanov]

We will add more and more checks so we need a better code structure
in parallels_co_check. Let each check performs in a separate loop
in a separate helper.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
v2: Revert the condition with s->header_unclean.
v3: Fix commit message.

 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 v3 5/8] parallels: Move check of cluster outside image to a separate function

[Alexander Ivanov]

We will add more and more checks so we need a better code structure
in parallels_co_check. Let each check performs in a separate loop
in a separate helper.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
v2: Move unrelated helper parallels_set_bat_entry creation to
    a separate patch.
v3: Fix commit message.

 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 v3 6/8] parallels: Move check of leaks to a separate function

[Alexander Ivanov]

We will add more and more checks so we need a better code structure
in parallels_co_check. Let each check performs in a separate loop
in a separate helper.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
v2: No change.
v3: Fix commit message.

 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 v3 7/8] parallels: Move statistic collection to a separate function

[Alexander Ivanov]

We will add more and more checks so we need a better code structure
in parallels_co_check. Let each check performs in a separate loop
in a separate helper.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
v2: Move fragmentation counting code to this function too.
v3: Fix commit message.

 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 v3 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 simplier
and less error prone code.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
v2: Fix an incorrect usage of WITH_QEMU_LOCK_GUARD.
v3: Fix commit message.

 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 v3 0/8] parallels: Refactor the code of images checks and fix a bug

[Denis V. Lunev]

On 15.08.2022 11:02, 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 simplier code.
>
>
> Alexander Ivanov (8):
>    parallels: Out of image offset in BAT leads to image inflation
>    parallels: create parallels_set_bat_entry_helper() to assign BAT value
>    parallels: Use generic infrastructure for BAT writing in
>      parallels_co_check()
>    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(-)
>
Reviewed-by: Denis V. Lunev <den@openvz.org>

I am happy with this code now.

Stefan, Vova,

can we take this to block-next via one of your trees?

Den

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

[Vladimir Sementsov-Ogievskiy]

On 8/15/22 12:02, Alexander Ivanov wrote:
> 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.
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
> v2: No change.
> v3: Fix commit message.
> 
>   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;

Hm. but with this, any further allocation may lead to twice-allocted clusters, as you just modify s->data_end, but havn't yet fixed the BAT entry.. It seems unsafe. Or what I miss?

> +        } 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;


-- 
Best regards,
Vladimir

Re: [PATCH v3 2/8] parallels: create parallels_set_bat_entry_helper() to assign BAT value

[Vladimir Sementsov-Ogievskiy]

On 8/15/22 12:02, Alexander Ivanov wrote:
> This helper will be reused in next patches during parallels_co_check
> rework to simplify its code.
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

> ---
> v2: A new patch - a part of a splitted patch.
> v3: Fix commit message.
> 
>   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)

Rather unobvious that offset should be passed already converted to LE. Worth a comment? Or may be do convertion inside function (depends on further usages of the helper)

> +{
> +    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;


-- 
Best regards,
Vladimir

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

[Denis V. Lunev]

On 17.08.2022 21:13, Vladimir Sementsov-Ogievskiy wrote:
> On 8/15/22 12:02, Alexander Ivanov wrote:
>> 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.
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>> v2: No change.
>> v3: Fix commit message.
>>
>>   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;
>
> Hm. but with this, any further allocation may lead to twice-allocted 
> clusters, as you just modify s->data_end, but havn't yet fixed the BAT 
> entry.. It seems unsafe. Or what I miss?
>
if O_CHECK is specified, we are going to execute parallels_co_check which
will correctly handle this. In the other case (normal open) we will
face the error, which is pretty much correct under this logic.

>> +        } 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 v3 1/8] parallels: Out of image offset in BAT leads to image inflation

[Vladimir Sementsov-Ogievskiy]

On 8/17/22 22:27, Denis V. Lunev wrote:
> On 17.08.2022 21:13, Vladimir Sementsov-Ogievskiy wrote:
>> On 8/15/22 12:02, Alexander Ivanov wrote:
>>> 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.
>>>
>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>> ---
>>> v2: No change.
>>> v3: Fix commit message.
>>>
>>>   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;
>>
>> Hm. but with this, any further allocation may lead to twice-allocted clusters, as you just modify s->data_end, but havn't yet fixed the BAT entry.. It seems unsafe. Or what I miss?
>>
> if O_CHECK is specified, we are going to execute parallels_co_check which
> will correctly handle this. In the other case (normal open) we will
> face the error, which is pretty much correct under this logic.

Sounds like "s->data_end = file_size" is part of this handling and should be in parallels_co_check()..

Looking at it, seems more correct to recalculate s->data_end exactly after for-loop, which fixes out-of-image clusters. Also it would work better in case when we have leaked clusters at the end of file.

Otherwise, ideally, you should have comment at top of parallels_co_check(), that we must first fix out-of-image clusters, before doing any kind of allocation, because data_end is already tweaked.

I agree that patch should work as is.

> 
>>> +        } 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;
>>
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH v3 3/8] parallels: Use generic infrastructure for BAT writing in parallels_co_check()

[Vladimir Sementsov-Ogievskiy]

On 8/15/22 12:02, Alexander Ivanov wrote:
> 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.
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
> 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.
> v3: Fix commit message.
> 
>   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);

Oops that's wrong. Here we probably rewrite previous error of bdrv_truncate stored in ret variable.

> +    if (ret < 0) {
> +        res->check_errors++;
> +    }
> +
>       return ret;
>   }
>   


-- 
Best regards,
Vladimir

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

[Denis V. Lunev]

On 17.08.2022 21:43, Vladimir Sementsov-Ogievskiy wrote:
> On 8/17/22 22:27, Denis V. Lunev wrote:
>> On 17.08.2022 21:13, Vladimir Sementsov-Ogievskiy wrote:
>>> On 8/15/22 12:02, Alexander Ivanov wrote:
>>>> 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.
>>>>
>>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>>> ---
>>>> v2: No change.
>>>> v3: Fix commit message.
>>>>
>>>>   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;
>>>
>>> Hm. but with this, any further allocation may lead to twice-allocted 
>>> clusters, as you just modify s->data_end, but havn't yet fixed the 
>>> BAT entry.. It seems unsafe. Or what I miss?
>>>
>> if O_CHECK is specified, we are going to execute parallels_co_check 
>> which
>> will correctly handle this. In the other case (normal open) we will
>> face the error, which is pretty much correct under this logic.
>
> Sounds like "s->data_end = file_size" is part of this handling and 
> should be in parallels_co_check()..
>
> Looking at it, seems more correct to recalculate s->data_end exactly 
> after for-loop, which fixes out-of-image clusters. Also it would work 
> better in case when we have leaked clusters at the end of file.
>
> Otherwise, ideally, you should have comment at top of 
> parallels_co_check(), that we must first fix out-of-image clusters, 
> before doing any kind of allocation, because data_end is already tweaked.
>
> I agree that patch should work as is.
>

but that will change the game rules.

Ideally we should call parallels_co_check if we face a error
on open and assume that the error is recoverable like
is done in QCOW2, but IMHO this is not the time within
this patchset.

This patch is needed here as we would have tests broken
immediately once we start checking duplicated cluster
management.

That is why I am happy with this code but in general this
would need further rework. This is a question of the
chicken and egg and we can not do all at once :(

>>
>>>> +        } 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 v3 3/8] parallels: Use generic infrastructure for BAT writing in parallels_co_check()

[Denis V. Lunev]

On 17.08.2022 21:48, Vladimir Sementsov-Ogievskiy wrote:
> On 8/15/22 12:02, Alexander Ivanov wrote:
>> 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.
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>> 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.
>> v3: Fix commit message.
>>
>>   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);
>
> Oops that's wrong. Here we probably rewrite previous error of 
> bdrv_truncate stored in ret variable.
>

You absolutely right, we have missed this point.

Good catch!

>> +    if (ret < 0) {
>> +        res->check_errors++;
>> +    }
>> +
>>       return ret;
>>   }
>
>

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

[Vladimir Sementsov-Ogievskiy]

On 8/15/22 12:02, Alexander Ivanov wrote:
> We will add more and more checks so we need a better code structure
> in parallels_co_check. Let each check performs in a separate loop
> in a separate helper.
> 
> Signed-off-by: Alexander Ivanov<alexander.ivanov@virtuozzo.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

Re: [PATCH v3 5/8] parallels: Move check of cluster outside image to a separate function

[Vladimir Sementsov-Ogievskiy]

On 8/15/22 12:02, Alexander Ivanov wrote:
> We will add more and more checks so we need a better code structure
> in parallels_co_check. Let each check performs in a separate loop
> in a separate helper.
> 
> Signed-off-by: Alexander Ivanov<alexander.ivanov@virtuozzo.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

On 8/15/22 12:02, Alexander Ivanov wrote:
> We will add more and more checks so we need a better code structure
> in parallels_co_check. Let each check performs in a separate loop
> in a separate helper.
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
> v2: No change.
> v3: Fix commit message.
> 
>   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;

'i' should be kept to be uint32_t, as pre-patch.

With it fixed:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>



-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

On 8/15/22 12:02, Alexander Ivanov wrote:
> We will add more and more checks so we need a better code structure
> in parallels_co_check. Let each check performs in a separate loop
> in a separate helper.
> 
> Signed-off-by: Alexander Ivanov<alexander.ivanov@virtuozzo.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

On 8/15/22 12:02, Alexander Ivanov wrote:
> Replace the way we use mutex in parallels_co_check() for simplier
> and less error prone code.
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
> v2: Fix an incorrect usage of WITH_QEMU_LOCK_GUARD.
> v3: Fix commit message.
> 
>   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) {

Aha, and here you silently fix the problem I noted in patch 3. Still, all patches should be correct, so this should be rebased onto fixed patch 3.

The final look of parallels_co_check is good.


-- 
Best regards,
Vladimir

Re: [PATCH v3 2/8] parallels: create parallels_set_bat_entry_helper() to assign BAT value

[Alexander Ivanov]

On 17.08.2022 21:21, Vladimir Sementsov-Ogievskiy wrote:
> On 8/15/22 12:02, Alexander Ivanov wrote:
>> This helper will be reused in next patches during parallels_co_check
>> rework to simplify its code.
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
>> ---
>> v2: A new patch - a part of a splitted patch.
>> v3: Fix commit message.
>>
>>   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)
>
> Rather unobvious that offset should be passed already converted to LE. 
> Worth a comment? Or may be do convertion inside function (depends on 
> further usages of the helper)
I agree, it would be better to convert the offset inside the helper.
>
>> +{
>> +    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 v3 1/8] parallels: Out of image offset in BAT leads to image inflation

[Alexander Ivanov]

On 17.08.2022 21:43, Vladimir Sementsov-Ogievskiy wrote:
> On 8/17/22 22:27, Denis V. Lunev wrote:
>> On 17.08.2022 21:13, Vladimir Sementsov-Ogievskiy wrote:
>>> On 8/15/22 12:02, Alexander Ivanov wrote:
>>>> 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.
>>>>
>>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>>> ---
>>>> v2: No change.
>>>> v3: Fix commit message.
>>>>
>>>>   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;
>>>
>>> Hm. but with this, any further allocation may lead to twice-allocted 
>>> clusters, as you just modify s->data_end, but havn't yet fixed the 
>>> BAT entry.. It seems unsafe. Or what I miss?
>>>
>> if O_CHECK is specified, we are going to execute parallels_co_check 
>> which
>> will correctly handle this. In the other case (normal open) we will
>> face the error, which is pretty much correct under this logic.
>
> Sounds like "s->data_end = file_size" is part of this handling and 
> should be in parallels_co_check()..
>
> Looking at it, seems more correct to recalculate s->data_end exactly 
> after for-loop, which fixes out-of-image clusters. Also it would work 
> better in case when we have leaked clusters at the end of file.
>
> Otherwise, ideally, you should have comment at top of 
> parallels_co_check(), that we must first fix out-of-image clusters, 
> before doing any kind of allocation, because data_end is already tweaked.
>
> I agree that patch should work as is.

I would like to leave this check in parallels_open(). I think it's a 
good idea to have an error on a corrupted image. Later we can replace it 
by checking&fixing images in parallels_open().

But I agree, it would be better to move the fix of s->data_end to 
parallels_co_check() and then move to parallels_check_outside_image().

>
>>
>>>> +        } 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 v3 3/8] parallels: Use generic infrastructure for BAT writing in parallels_co_check()

[Alexander Ivanov]

On 17.08.2022 21:48, Vladimir Sementsov-Ogievskiy wrote:
> On 8/15/22 12:02, Alexander Ivanov wrote:
>> 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.
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>> 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.
>> v3: Fix commit message.
>>
>>   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);
>
> Oops that's wrong. Here we probably rewrite previous error of 
> bdrv_truncate stored in ret variable.

Good point!

As you said in 8/8, now it doesn't matter, but I'll fix it for the patch 
correctness.

>
>> +    if (ret < 0) {
>> +        res->check_errors++;
>> +    }
>> +
>>       return ret;
>>   }
>
>

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

[Alexander Ivanov]

On 17.08.2022 23:00, Vladimir Sementsov-Ogievskiy wrote:
> On 8/15/22 12:02, Alexander Ivanov wrote:
>> We will add more and more checks so we need a better code structure
>> in parallels_co_check. Let each check performs in a separate loop
>> in a separate helper.
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> ---
>> v2: No change.
>> v3: Fix commit message.
>>
>>   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;
>
> 'i' should be kept to be uint32_t, as pre-patch.
>
> With it fixed:
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
>
Will fix it, thank you.

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

[Vladimir Sementsov-Ogievskiy]

On 8/18/22 11:49, Alexander Ivanov wrote:
> 
> On 17.08.2022 21:43, Vladimir Sementsov-Ogievskiy wrote:
>> On 8/17/22 22:27, Denis V. Lunev wrote:
>>> On 17.08.2022 21:13, Vladimir Sementsov-Ogievskiy wrote:
>>>> On 8/15/22 12:02, Alexander Ivanov wrote:
>>>>> 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.
>>>>>
>>>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>>>> ---
>>>>> v2: No change.
>>>>> v3: Fix commit message.
>>>>>
>>>>>   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;
>>>>
>>>> Hm. but with this, any further allocation may lead to twice-allocted clusters, as you just modify s->data_end, but havn't yet fixed the BAT entry.. It seems unsafe. Or what I miss?
>>>>
>>> if O_CHECK is specified, we are going to execute parallels_co_check which
>>> will correctly handle this. In the other case (normal open) we will
>>> face the error, which is pretty much correct under this logic.
>>
>> Sounds like "s->data_end = file_size" is part of this handling and should be in parallels_co_check()..
>>
>> Looking at it, seems more correct to recalculate s->data_end exactly after for-loop, which fixes out-of-image clusters. Also it would work better in case when we have leaked clusters at the end of file.
>>
>> Otherwise, ideally, you should have comment at top of parallels_co_check(), that we must first fix out-of-image clusters, before doing any kind of allocation, because data_end is already tweaked.
>>
>> I agree that patch should work as is.
> 
> I would like to leave this check in parallels_open(). I think it's a good idea to have an error on a corrupted image. Later we can replace it by checking&fixing images in parallels_open().
> 
> But I agree, it would be better to move the fix of s->data_end to parallels_co_check() and then move to parallels_check_outside_image().

Yes, right, agree.

> 
>>
>>>
>>>>> +        } 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;
>>>>
>>>>
>>>
>>
>>


-- 
Best regards,
Vladimir