[PATCH v10 00/12] parallels: Refactor the code of images checks and fix a bug

[PATCH v10 00/12] 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.

Fix incorrect condition in out-of-image check.

v10:
8: Add a comment.
9: Exclude unrelated changes.

v9:
3: Add (high_off == 0) case handling.
7: Move res->image_end_offset setting to parallels_check_outside_image().
8: Add a patch with a statistics calculation fix.
9: Remove redundant high_off calculation.
12: Change the condition to (off + s->cluster_size > size).

v8: Rebase on the top of the current master branch.

v7:
1,2: Fix string lengths in the commit messages.
3: Fix a typo in the commit message.

v6:
1: Move the error check inside the loop. Move file size getting
   to the function beginning. Skip out-of-image offsets.
2: A new patch - don't let high_off be more than the end of the last cluster.
3: Set data_end without any condition.
7: Move data_end setting to parallels_check_outside_image().
8: Remove s->data_end setting from parallels_check_leak().
   Fix 'i' type.

v5:
2: Change the way of data_end fixing.
6,7: Move data_end check to parallels_check_leak().

v4:
1: Move s->data_end fix to parallels_co_check(). Split the check
   in parallels_open() and the fix in parallels_co_check() to two patches.
2: A new patch - a part of the patch 1.
   Add a fix for data_end to parallels_co_check().
3: Move offset convertation to parallels_set_bat_entry().
4: Fix 'ret' rewriting by bdrv_co_flush() results.
7: Keep 'i' as uint32_t.

v3:

1-8: Fix commit message.

v2:

2: A new patch - a part of the splitted patch 2.
3: 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.
4: Revert the condition with s->header_unclean.
5: Move unrelated helper parallels_set_bat_entry creation to a separate patch.
7: Move fragmentation counting code to this function too.
8: Fix an incorrect usage of WITH_QEMU_LOCK_GUARD.

Alexander Ivanov (12):
  parallels: Out of image offset in BAT leads to image inflation
  parallels: Fix high_off calculation in parallels_co_check()
  parallels: Fix image_end_offset and data_end after out-of-image check
  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: Fix statistics calculation
  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
  parallels: Incorrect condition in out-of-image check

 block/parallels.c | 192 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 137 insertions(+), 55 deletions(-)

-- 
2.34.1

[PATCH v10 01/12] 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.

Raise an error in parallels_open() if data_end points outside the image
and it is not a check (let the check to repaire the image). Set data_end
to the end of the cluster with the last correct offset.

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

diff --git a/block/parallels.c b/block/parallels.c
index bbea2f2221..4af68adc61 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;
@@ -741,6 +742,12 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         return ret;
     }
 
+    file_size = bdrv_getlength(bs->file->bs);
+    if (file_size < 0) {
+        return -EINVAL;
+    }
+    file_size >>= BDRV_SECTOR_BITS;
+
     ret = bdrv_pread(bs->file, 0, sizeof(ph), &ph, 0);
     if (ret < 0) {
         goto fail;
@@ -805,6 +812,16 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
 
     for (i = 0; i < s->bat_size; i++) {
         int64_t off = bat2sect(s, i);
+        if (off >= file_size) {
+            if (flags & BDRV_O_CHECK) {
+                continue;
+            }
+            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
+                       "is larger than file size (%" PRIi64 ")",
+                       off, i, file_size);
+            ret = -EINVAL;
+            goto fail;
+        }
         if (off >= s->data_end) {
             s->data_end = off + s->tracks;
         }
-- 
2.34.1

[PATCH v10 02/12] parallels: Fix high_off calculation in parallels_co_check()

[Alexander Ivanov]

Don't let high_off be more than the file size even if we don't fix the
image.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 block/parallels.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 4af68adc61..436b36bbd9 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -460,12 +460,12 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
                     fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
             res->corruptions++;
             if (fix & BDRV_FIX_ERRORS) {
-                prev_off = 0;
                 s->bat_bitmap[i] = 0;
                 res->corruptions_fixed++;
                 flush_bat = true;
-                continue;
             }
+            prev_off = 0;
+            continue;
         }
 
         res->bfi.allocated_clusters++;
-- 
2.34.1

[PATCH v10 03/12] parallels: Fix image_end_offset and data_end after out-of-image check

[Alexander Ivanov]

Set data_end to the end of the last cluster inside the image. In such a
way we can be sure that corrupted offsets in the BAT can't affect on the
image size. If there are no allocated clusters set image_end_offset by
data_end.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 block/parallels.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/parallels.c b/block/parallels.c
index 436b36bbd9..2ed7cca249 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -488,7 +488,13 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
         }
     }
 
-    res->image_end_offset = high_off + s->cluster_size;
+    if (high_off == 0) {
+        res->image_end_offset = s->data_end << BDRV_SECTOR_BITS;
+    } else {
+        res->image_end_offset = high_off + s->cluster_size;
+        s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
+    }
+
     if (size > res->image_end_offset) {
         int64_t count;
         count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
-- 
2.34.1

[PATCH v10 04/12] 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>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/parallels.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 2ed7cca249..b2e3f5f98f 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] = cpu_to_le32(offset);
+    bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block, 1);
+}
+
 static coroutine_fn int64_t allocate_clusters(BlockDriverState *bs,
                                               int64_t sector_num,
                                               int nb_sectors, int *pnum)
@@ -251,10 +258,8 @@ static coroutine_fn int64_t allocate_clusters(BlockDriverState *bs,
     }
 
     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, 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 v10 05/12] 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>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 block/parallels.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index b2e3f5f98f..63bfd7074c 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -425,9 +425,8 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
 {
     BDRVParallelsState *s = bs->opaque;
     int64_t size, prev_off, high_off;
-    int ret;
+    int ret = 0;
     uint32_t i;
-    bool flush_bat = false;
 
     size = bdrv_getlength(bs->file->bs);
     if (size < 0) {
@@ -465,9 +464,8 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
                     fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
             res->corruptions++;
             if (fix & BDRV_FIX_ERRORS) {
-                s->bat_bitmap[i] = 0;
+                parallels_set_bat_entry(s, i, 0);
                 res->corruptions_fixed++;
-                flush_bat = true;
             }
             prev_off = 0;
             continue;
@@ -484,15 +482,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;
-        }
-    }
-
     if (high_off == 0) {
         res->image_end_offset = s->data_end << BDRV_SECTOR_BITS;
     } else {
@@ -527,6 +516,14 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
 
 out:
     qemu_co_mutex_unlock(&s->lock);
+
+    if (ret == 0) {
+        ret = bdrv_co_flush(bs);
+        if (ret < 0) {
+            res->check_errors++;
+        }
+    }
+
     return ret;
 }
 
-- 
2.34.1

[PATCH v10 06/12] 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>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/parallels.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 63bfd7074c..02fbaee1f2 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -418,6 +418,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,
@@ -435,16 +454,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 v10 07/12] 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>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 block/parallels.c | 81 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 52 insertions(+), 29 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 02fbaee1f2..f9acee1fa8 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -438,14 +438,13 @@ static void parallels_check_unclean(BlockDriverState *bs,
     }
 }
 
-static int coroutine_fn parallels_co_check(BlockDriverState *bs,
-                                           BdrvCheckResult *res,
-                                           BdrvCheckMode fix)
+static int parallels_check_outside_image(BlockDriverState *bs,
+                                         BdrvCheckResult *res,
+                                         BdrvCheckMode fix)
 {
     BDRVParallelsState *s = bs->opaque;
-    int64_t size, prev_off, high_off;
-    int ret = 0;
     uint32_t i;
+    int64_t off, high_off, size;
 
     size = bdrv_getlength(bs->file->bs);
     if (size < 0) {
@@ -453,23 +452,9 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
         return size;
     }
 
-    qemu_co_mutex_lock(&s->lock);
-
-    parallels_check_unclean(bs, res, fix);
-
-    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;
-        }
-
-        /* cluster outside the image */
+        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);
@@ -478,19 +463,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
                 parallels_set_bat_entry(s, i, 0);
                 res->corruptions_fixed++;
             }
-            prev_off = 0;
             continue;
         }
-
-        res->bfi.allocated_clusters++;
-        if (off > high_off) {
+        if (high_off < off) {
             high_off = off;
         }
-
-        if (prev_off != 0 && (prev_off + s->cluster_size) != off) {
-            res->bfi.fragmented_clusters++;
-        }
-        prev_off = off;
     }
 
     if (high_off == 0) {
@@ -500,6 +477,52 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
         s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
     }
 
+    return 0;
+}
+
+static int coroutine_fn parallels_co_check(BlockDriverState *bs,
+                                           BdrvCheckResult *res,
+                                           BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int64_t size, prev_off;
+    int ret;
+    uint32_t i;
+
+    size = bdrv_getlength(bs->file->bs);
+    if (size < 0) {
+        res->check_errors++;
+        return size;
+    }
+
+    qemu_co_mutex_lock(&s->lock);
+
+    parallels_check_unclean(bs, res, fix);
+
+    ret = parallels_check_outside_image(bs, res, fix);
+    if (ret < 0) {
+        goto out;
+    }
+
+    res->bfi.total_clusters = s->bat_size;
+    res->bfi.compressed_clusters = 0; /* compression is not supported */
+
+    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;
+    }
+
     if (size > res->image_end_offset) {
         int64_t count;
         count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
-- 
2.34.1

[PATCH v10 08/12] parallels: Fix statistics calculation

[Alexander Ivanov]

Exclude out-of-image clusters from allocated and fragmented clusters
calculation.

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

diff --git a/block/parallels.c b/block/parallels.c
index f9acee1fa8..994528d93c 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -510,7 +510,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
     prev_off = 0;
     for (i = 0; i < s->bat_size; i++) {
         int64_t off = bat2sect(s, i) << BDRV_SECTOR_BITS;
-        if (off == 0) {
+        /*
+         * If BDRV_FIX_ERRORS is not set, out-of-image BAT entries were not
+         * fixed. Skip not allocated and out-of-image BAT entries.
+         */
+        if (off == 0 || off + s->cluster_size > res->image_end_offset) {
             prev_off = 0;
             continue;
         }
-- 
2.34.1

[PATCH v10 09/12] 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>
---
 block/parallels.c | 80 ++++++++++++++++++++++++++++-------------------
 1 file changed, 48 insertions(+), 32 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 994528d93c..c7d37c4580 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -480,21 +480,57 @@ static int parallels_check_outside_image(BlockDriverState *bs,
     return 0;
 }
 
+static int parallels_check_leak(BlockDriverState *bs,
+                                BdrvCheckResult *res,
+                                BdrvCheckMode fix)
+{
+    BDRVParallelsState *s = bs->opaque;
+    int64_t size;
+    int ret;
+
+    size = bdrv_getlength(bs->file->bs);
+    if (size < 0) {
+        res->check_errors++;
+        return 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",
+                size - res->image_end_offset);
+        res->leaks += count;
+        if (fix & BDRV_FIX_LEAKS) {
+            Error *local_err = NULL;
+
+            /*
+             * In order to really repair the image, we must shrink it.
+             * That means we have to pass exact=true.
+             */
+            ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
+                                   PREALLOC_MODE_OFF, 0, &local_err);
+            if (ret < 0) {
+                error_report_err(local_err);
+                res->check_errors++;
+                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 size, prev_off;
+    int64_t prev_off;
     int ret;
     uint32_t i;
 
-    size = bdrv_getlength(bs->file->bs);
-    if (size < 0) {
-        res->check_errors++;
-        return size;
-    }
-
     qemu_co_mutex_lock(&s->lock);
 
     parallels_check_unclean(bs, res, fix);
@@ -504,6 +540,11 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
         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 */
 
@@ -527,31 +568,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
         prev_off = off;
     }
 
-    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",
-                size - res->image_end_offset);
-        res->leaks += count;
-        if (fix & BDRV_FIX_LEAKS) {
-            Error *local_err = NULL;
-
-            /*
-             * In order to really repair the image, we must shrink it.
-             * That means we have to pass exact=true.
-             */
-            ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
-                                   PREALLOC_MODE_OFF, 0, &local_err);
-            if (ret < 0) {
-                error_report_err(local_err);
-                res->check_errors++;
-                goto out;
-            }
-            res->leaks_fixed += count;
-        }
-    }
-
 out:
     qemu_co_mutex_unlock(&s->lock);
 
-- 
2.34.1

[PATCH v10 10/12] 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>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/parallels.c | 52 +++++++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index c7d37c4580..3c38a0fd09 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -522,35 +522,20 @@ 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 BDRV_FIX_ERRORS is not set, out-of-image BAT entries were not
          * fixed. Skip not allocated and out-of-image BAT entries.
@@ -560,13 +545,36 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
             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 v10 11/12] 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>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 block/parallels.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 3c38a0fd09..61e7e4010d 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -560,30 +560,25 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
     BDRVParallelsState *s = bs->opaque;
     int ret;
 
-    qemu_co_mutex_lock(&s->lock);
+    WITH_QEMU_LOCK_GUARD(&s->lock) {
+        parallels_check_unclean(bs, res, fix);
 
-    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) {
+            return ret;
+        }
 
-    ret = parallels_check_leak(bs, res, fix);
-    if (ret < 0) {
-        goto out;
+        parallels_collect_statistics(bs, res, fix);
     }
 
-    parallels_collect_statistics(bs, res, fix);
-
-out:
-    qemu_co_mutex_unlock(&s->lock);
-
-    if (ret == 0) {
-        ret = bdrv_co_flush(bs);
-        if (ret < 0) {
-            res->check_errors++;
-        }
+    ret = bdrv_co_flush(bs);
+    if (ret < 0) {
+        res->check_errors++;
     }
 
     return ret;
-- 
2.34.1

[PATCH v10 12/12] parallels: Incorrect condition in out-of-image check

[Alexander Ivanov]

All the offsets in the BAT must be lower than the file size.
Fix the check condition for correct check.

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

diff --git a/block/parallels.c b/block/parallels.c
index 61e7e4010d..8ff7d55c96 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -455,7 +455,7 @@ static int parallels_check_outside_image(BlockDriverState *bs,
     high_off = 0;
     for (i = 0; i < s->bat_size; i++) {
         off = bat2sect(s, i) << BDRV_SECTOR_BITS;
-        if (off > size) {
+        if (off + s->cluster_size > size) {
             fprintf(stderr, "%s cluster %u is outside image\n",
                     fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
             res->corruptions++;
-- 
2.34.1

Re: [PATCH v10 01/12] parallels: Out of image offset in BAT leads to image inflation

[Vladimir Sementsov-Ogievskiy]

On 03.02.23 12:18, 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.
> 
> Raise an error in parallels_open() if data_end points outside the image
> and it is not a check (let the check to repaire the image). Set data_end
> to the end of the cluster with the last correct offset.
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> Reviewed-by: Denis V. Lunev <den@openvz.org>
> ---
>   block/parallels.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index bbea2f2221..4af68adc61 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;
> @@ -741,6 +742,12 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>           return ret;
>       }
>   
> +    file_size = bdrv_getlength(bs->file->bs);
> +    if (file_size < 0) {
> +        return -EINVAL;
> +    }
> +    file_size >>= BDRV_SECTOR_BITS;

if file size somehow not aligned to BDRV_SECTOR_SIZE, that's not correct, DIV_ROUND_UP would be better

> +
>       ret = bdrv_pread(bs->file, 0, sizeof(ph), &ph, 0);
>       if (ret < 0) {
>           goto fail;
> @@ -805,6 +812,16 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>   
>       for (i = 0; i < s->bat_size; i++) {
>           int64_t off = bat2sect(s, i);
> +        if (off >= file_size) {
> +            if (flags & BDRV_O_CHECK) {
> +                continue;
> +            }
> +            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
> +                       "is larger than file size (%" PRIi64 ")",
> +                       off, i, file_size);

offsets in sectors rather than in bytes may be a bit misleading

> +            ret = -EINVAL;
> +            goto fail;
> +        }
>           if (off >= s->data_end) {
>               s->data_end = off + s->tracks;
>           }

-- 
Best regards,
Vladimir

Re: [PATCH v10 02/12] parallels: Fix high_off calculation in parallels_co_check()

[Vladimir Sementsov-Ogievskiy]

On 03.02.23 12:18, Alexander Ivanov wrote:
> Don't let high_off be more than the file size even if we don't fix the
> image.
> 
> Signed-off-by: Alexander Ivanov<alexander.ivanov@virtuozzo.com>
> Reviewed-by: Denis V. Lunev<den@openvz.org>

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

-- 
Best regards,
Vladimir

Re: [PATCH v10 01/12] parallels: Out of image offset in BAT leads to image inflation

[Denis V. Lunev]

On 2/14/23 18:44, Vladimir Sementsov-Ogievskiy wrote:
> On 03.02.23 12:18, 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.
>>
>> Raise an error in parallels_open() if data_end points outside the image
>> and it is not a check (let the check to repaire the image). Set data_end
>> to the end of the cluster with the last correct offset.
>>
>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> Reviewed-by: Denis V. Lunev <den@openvz.org>
>> ---
>>   block/parallels.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index bbea2f2221..4af68adc61 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;
>> @@ -741,6 +742,12 @@ static int parallels_open(BlockDriverState *bs, 
>> QDict *options, int flags,
>>           return ret;
>>       }
>>   +    file_size = bdrv_getlength(bs->file->bs);
>> +    if (file_size < 0) {
>> +        return -EINVAL;
>> +    }
>> +    file_size >>= BDRV_SECTOR_BITS;
>
> if file size somehow not aligned to BDRV_SECTOR_SIZE, that's not 
> correct, DIV_ROUND_UP would be better
>
I would say that if file length is not aligned to block size - this is a
point to mark such file as broken and call check immediately.


>> +
>>       ret = bdrv_pread(bs->file, 0, sizeof(ph), &ph, 0);
>>       if (ret < 0) {
>>           goto fail;
>> @@ -805,6 +812,16 @@ static int parallels_open(BlockDriverState *bs, 
>> QDict *options, int flags,
>>         for (i = 0; i < s->bat_size; i++) {
>>           int64_t off = bat2sect(s, i);
>> +        if (off >= file_size) {
>> +            if (flags & BDRV_O_CHECK) {
>> +                continue;
>> +            }
>> +            error_setg(errp, "parallels: Offset %" PRIi64 " in 
>> BAT[%d] entry "
>> +                       "is larger than file size (%" PRIi64 ")",
>> +                       off, i, file_size);
>
> offsets in sectors rather than in bytes may be a bit misleading
>

agree. Should be easy

>> +            ret = -EINVAL;
>> +            goto fail;
>> +        }
>>           if (off >= s->data_end) {
>>               s->data_end = off + s->tracks;
>>           }
>

Re: [PATCH v10 01/12] parallels: Out of image offset in BAT leads to image inflation

[Vladimir Sementsov-Ogievskiy]

On 15.02.23 14:29, Denis V. Lunev wrote:
> On 2/14/23 18:44, Vladimir Sementsov-Ogievskiy wrote:
>> On 03.02.23 12:18, 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.
>>>
>>> Raise an error in parallels_open() if data_end points outside the image
>>> and it is not a check (let the check to repaire the image). Set data_end
>>> to the end of the cluster with the last correct offset.
>>>
>>> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>>> Reviewed-by: Denis V. Lunev <den@openvz.org>
>>> ---
>>>   block/parallels.c | 17 +++++++++++++++++
>>>   1 file changed, 17 insertions(+)
>>>
>>> diff --git a/block/parallels.c b/block/parallels.c
>>> index bbea2f2221..4af68adc61 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;
>>> @@ -741,6 +742,12 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>>           return ret;
>>>       }
>>>   +    file_size = bdrv_getlength(bs->file->bs);
>>> +    if (file_size < 0) {
>>> +        return -EINVAL;
>>> +    }
>>> +    file_size >>= BDRV_SECTOR_BITS;
>>
>> if file size somehow not aligned to BDRV_SECTOR_SIZE, that's not correct, DIV_ROUND_UP would be better
>>
> I would say that if file length is not aligned to block size - this is a
> point to mark such file as broken and call check immediately.

I'm not sure about parallels driver, but qcow2 happily produces unaligned to sector files, when used with file opened in cached mode.

But I remembered now, that bdrv_getlength actually can't return unaligned length, as bdrv_co_getlength() is just a wrapper on bdrv_co_nb_sectors(). Then, seems better directly use bdrv_nb_sectors().

-- 
Best regards,
Vladimir

Re: [PATCH v10 07/12] parallels: Move check of cluster outside image to a separate function

[Hanna Czenczek]

On 03.02.23 10:18, 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: Denis V. Lunev <den@openvz.org>
> ---
>   block/parallels.c | 81 ++++++++++++++++++++++++++++++-----------------
>   1 file changed, 52 insertions(+), 29 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 02fbaee1f2..f9acee1fa8 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -438,14 +438,13 @@ static void parallels_check_unclean(BlockDriverState *bs,
>       }
>   }
>   
> -static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> -                                           BdrvCheckResult *res,
> -                                           BdrvCheckMode fix)
> +static int parallels_check_outside_image(BlockDriverState *bs,
> +                                         BdrvCheckResult *res,
> +                                         BdrvCheckMode fix)

I wonder, should we mark this function coroutine_fn?  And with the graph 
lock changes that happened in the meantime, probably also GRAPH_RDLOCK 
(because it’s calling bdrv_getlength()).

Hanna

Re: [PATCH v10 09/12] parallels: Move check of leaks to a separate function

[Hanna Czenczek]

On 03.02.23 10:18, 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>
> ---
>   block/parallels.c | 80 ++++++++++++++++++++++++++++-------------------
>   1 file changed, 48 insertions(+), 32 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 994528d93c..c7d37c4580 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -480,21 +480,57 @@ static int parallels_check_outside_image(BlockDriverState *bs,
>       return 0;
>   }
>   
> +static int parallels_check_leak(BlockDriverState *bs,
> +                                BdrvCheckResult *res,
> +                                BdrvCheckMode fix)

Like parallels_check_outside_image(), I think ideally this should be 
marked as `coroutine_fn GRAPH_RDLOCK` because of the block layer 
functions it calls.

Hanna

Re: [PATCH v10 00/12] parallels: Refactor the code of images checks and fix a bug

[Hanna Czenczek]

On 03.02.23 10:18, 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.
>
> Fix incorrect condition in out-of-image check.

Apart from my comments (and I think regardless of what happens for patch 
1), looks good to me!  Some patches will need to be rebased on the 
GRAPH_RDLOCK changes, but that looks straightforward.

Hanna

Re: [PATCH v10 07/12] parallels: Move check of cluster outside image to a separate function

[Alexander Ivanov]

On 3/7/23 13:17, Hanna Czenczek wrote:
> On 03.02.23 10:18, 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: Denis V. Lunev <den@openvz.org>
>> ---
>>   block/parallels.c | 81 ++++++++++++++++++++++++++++++-----------------
>>   1 file changed, 52 insertions(+), 29 deletions(-)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index 02fbaee1f2..f9acee1fa8 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -438,14 +438,13 @@ static void 
>> parallels_check_unclean(BlockDriverState *bs,
>>       }
>>   }
>>   -static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>> -                                           BdrvCheckResult *res,
>> -                                           BdrvCheckMode fix)
>> +static int parallels_check_outside_image(BlockDriverState *bs,
>> +                                         BdrvCheckResult *res,
>> +                                         BdrvCheckMode fix)
>
> I wonder, should we mark this function coroutine_fn?  And with the 
> graph lock changes that happened in the meantime, probably also 
> GRAPH_RDLOCK (because it’s calling bdrv_getlength()).
>
> Hanna
>
Thank you for your review.

It seems, parallels_collect_statistics(), parallels_check_unclean() and 
parallels_set_bat_entry() also should be marked as coroutine_fn, 
shouldn't they?

Re: [PATCH v10 07/12] parallels: Move check of cluster outside image to a separate function

[Hanna Czenczek]

On 10.03.23 12:08, Alexander Ivanov wrote:
>
>
> On 3/7/23 13:17, Hanna Czenczek wrote:
>> On 03.02.23 10:18, 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: Denis V. Lunev <den@openvz.org>
>>> ---
>>>   block/parallels.c | 81 
>>> ++++++++++++++++++++++++++++++-----------------
>>>   1 file changed, 52 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/block/parallels.c b/block/parallels.c
>>> index 02fbaee1f2..f9acee1fa8 100644
>>> --- a/block/parallels.c
>>> +++ b/block/parallels.c
>>> @@ -438,14 +438,13 @@ static void 
>>> parallels_check_unclean(BlockDriverState *bs,
>>>       }
>>>   }
>>>   -static int coroutine_fn parallels_co_check(BlockDriverState *bs,
>>> -                                           BdrvCheckResult *res,
>>> -                                           BdrvCheckMode fix)
>>> +static int parallels_check_outside_image(BlockDriverState *bs,
>>> +                                         BdrvCheckResult *res,
>>> +                                         BdrvCheckMode fix)
>>
>> I wonder, should we mark this function coroutine_fn?  And with the 
>> graph lock changes that happened in the meantime, probably also 
>> GRAPH_RDLOCK (because it’s calling bdrv_getlength()).
>>
>> Hanna
>>
> Thank you for your review.
>
> It seems, parallels_collect_statistics(), parallels_check_unclean() 
> and parallels_set_bat_entry() also should be marked as coroutine_fn, 
> shouldn't they?

There should be no harm in doing so.  I wasn’t mentioning it just 
because those functions don’t call other coroutine functions themselves, 
so I think it doesn’t really matter.

Hanna

Re: [PATCH v10 00/12] parallels: Refactor the code of images checks and fix a bug

[Denis V. Lunev]

On 3/7/23 13:20, Hanna Czenczek wrote:
> On 03.02.23 10:18, 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.
>>
>> Fix incorrect condition in out-of-image check.
>
> Apart from my comments (and I think regardless of what happens for 
> patch 1), looks good to me!  Some patches will need to be rebased on 
> the GRAPH_RDLOCK changes, but that looks straightforward.
>
> Hanna
>
I think that I should pass the process of learning how to send
pull request and make it as soon as possible in order to
learn.

We are really good enough.

Den