* [PATCH 0/9] parallels: Refactor the code of images checks and fix a bug
@ 2022-08-18 15:14 Alexander Ivanov
2022-08-18 15:14 ` [PATCH 1/9] parallels: Out of image offset in BAT leads to image inflation Alexander Ivanov
` (9 more replies)
0 siblings, 10 replies; 14+ messages in thread
From: Alexander Ivanov @ 2022-08-18 15:14 UTC (permalink / raw
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
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.
v4 changes:
Move s->data_end fixing to parallels_co_check(). Split the check
in parallels_open() and the fix in parallels_co_check() to two patches.
Move offset convertation to parallels_set_bat_entry().
Fix 'ret' rewriting by bdrv_co_flush() results.
Keep 'i' as uint32_t.
Alexander Ivanov (9):
parallels: Out of image offset in BAT leads to image inflation
parallels: Fix data_end field value in parallels_co_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: 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 | 197 +++++++++++++++++++++++++++++++++-------------
1 file changed, 141 insertions(+), 56 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/9] parallels: Out of image offset in BAT leads to image inflation
2022-08-18 15:14 [PATCH 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
@ 2022-08-18 15:14 ` Alexander Ivanov
2022-08-18 15:14 ` [PATCH 2/9] parallels: Fix data_end field value in parallels_co_check() Alexander Ivanov
` (8 subsequent siblings)
9 siblings, 0 replies; 14+ messages in thread
From: Alexander Ivanov @ 2022-08-18 15:14 UTC (permalink / raw
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
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).
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
block/parallels.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index a229c06f25..c245ca35cd 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,19 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
}
}
+ file_size = bdrv_getlength(bs->file->bs);
+ if (file_size < 0) {
+ ret = file_size;
+ goto fail;
+ }
+
+ file_size >>= BDRV_SECTOR_BITS;
+ if (s->data_end > file_size && !(flags & BDRV_O_CHECK)) {
+ 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/9] parallels: Fix data_end field value in parallels_co_check()
2022-08-18 15:14 [PATCH 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
2022-08-18 15:14 ` [PATCH 1/9] parallels: Out of image offset in BAT leads to image inflation Alexander Ivanov
@ 2022-08-18 15:14 ` Alexander Ivanov
2022-08-19 12:58 ` Denis V. Lunev
2022-08-18 15:14 ` [PATCH 3/9] parallels: create parallels_set_bat_entry_helper() to assign BAT value Alexander Ivanov
` (7 subsequent siblings)
9 siblings, 1 reply; 14+ messages in thread
From: Alexander Ivanov @ 2022-08-18 15:14 UTC (permalink / raw
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
When an image is opened for check there is no error if an offset in the BAT
points outside the image. In such a way we can repair the image.
Out-of-image offsets are repaired in the check, but data_end field
still points outside. Fix this field by file size.
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
block/parallels.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/block/parallels.c b/block/parallels.c
index c245ca35cd..24c05b95e8 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -513,7 +513,15 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
res->leaks_fixed += count;
}
}
-
+ /*
+ * If there were an out-of-image cluster it would be repaired,
+ * but s->data_end still would point outside image.
+ * Fix s->data_end by the file size.
+ */
+ size >>= BDRV_SECTOR_BITS;
+ if (s->data_end > size) {
+ s->data_end = size;
+ }
out:
qemu_co_mutex_unlock(&s->lock);
return ret;
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/9] parallels: create parallels_set_bat_entry_helper() to assign BAT value
2022-08-18 15:14 [PATCH 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
2022-08-18 15:14 ` [PATCH 1/9] parallels: Out of image offset in BAT leads to image inflation Alexander Ivanov
2022-08-18 15:14 ` [PATCH 2/9] parallels: Fix data_end field value in parallels_co_check() Alexander Ivanov
@ 2022-08-18 15:14 ` Alexander Ivanov
2022-08-18 15:14 ` [PATCH 4/9] parallels: Use generic infrastructure for BAT writing in parallels_co_check() Alexander Ivanov
` (6 subsequent siblings)
9 siblings, 0 replies; 14+ messages in thread
From: Alexander Ivanov @ 2022-08-18 15:14 UTC (permalink / raw
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
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>
---
block/parallels.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 24c05b95e8..f460b36054 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 int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, int *pnum)
{
@@ -250,10 +257,8 @@ 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, 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/9] parallels: Use generic infrastructure for BAT writing in parallels_co_check()
2022-08-18 15:14 [PATCH 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
` (2 preceding siblings ...)
2022-08-18 15:14 ` [PATCH 3/9] parallels: create parallels_set_bat_entry_helper() to assign BAT value Alexander Ivanov
@ 2022-08-18 15:14 ` Alexander Ivanov
2022-08-18 15:14 ` [PATCH 5/9] parallels: Move check of unclean image to a separate function Alexander Ivanov
` (5 subsequent siblings)
9 siblings, 0 replies; 14+ messages in thread
From: Alexander Ivanov @ 2022-08-18 15:14 UTC (permalink / raw
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
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>
---
block/parallels.c | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index f460b36054..7d76d6ce9d 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) {
@@ -466,9 +465,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;
}
}
@@ -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;
- }
- }
-
res->image_end_offset = high_off + s->cluster_size;
if (size > res->image_end_offset) {
int64_t count;
@@ -529,6 +518,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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 5/9] parallels: Move check of unclean image to a separate function
2022-08-18 15:14 [PATCH 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
` (3 preceding siblings ...)
2022-08-18 15:14 ` [PATCH 4/9] parallels: Use generic infrastructure for BAT writing in parallels_co_check() Alexander Ivanov
@ 2022-08-18 15:14 ` Alexander Ivanov
2022-08-18 15:14 ` [PATCH 6/9] parallels: Move check of cluster outside " Alexander Ivanov
` (4 subsequent siblings)
9 siblings, 0 replies; 14+ messages in thread
From: Alexander Ivanov @ 2022-08-18 15:14 UTC (permalink / raw
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
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 | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 7d76d6ce9d..3900a0f4a9 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 6/9] parallels: Move check of cluster outside image to a separate function
2022-08-18 15:14 [PATCH 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
` (4 preceding siblings ...)
2022-08-18 15:14 ` [PATCH 5/9] parallels: Move check of unclean image to a separate function Alexander Ivanov
@ 2022-08-18 15:14 ` Alexander Ivanov
2022-08-19 12:56 ` Denis V. Lunev
2022-08-18 15:14 ` [PATCH 7/9] parallels: Move check of leaks " Alexander Ivanov
` (3 subsequent siblings)
9 siblings, 1 reply; 14+ messages in thread
From: Alexander Ivanov @ 2022-08-18 15:14 UTC (permalink / raw
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
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.
s->data_end fix relates to out-of-image check so move it
to the helper too.
Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
block/parallels.c | 67 +++++++++++++++++++++++++++++++----------------
1 file changed, 45 insertions(+), 22 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 3900a0f4a9..1c7626c867 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -438,6 +438,46 @@ 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++;
+ }
+ }
+ }
+
+ /*
+ * If there were an out-of-image cluster it would be repaired,
+ * but s->data_end still would point outside image.
+ * Fix s->data_end by the file size.
+ */
+ size >>= BDRV_SECTOR_BITS;
+ if (s->data_end > size) {
+ s->data_end = size;
+ }
+
+ return 0;
+}
+
static int coroutine_fn parallels_co_check(BlockDriverState *bs,
BdrvCheckResult *res,
BdrvCheckMode fix)
@@ -457,6 +497,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 */
@@ -469,19 +514,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;
@@ -518,15 +550,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
res->leaks_fixed += count;
}
}
- /*
- * If there were an out-of-image cluster it would be repaired,
- * but s->data_end still would point outside image.
- * Fix s->data_end by the file size.
- */
- size >>= BDRV_SECTOR_BITS;
- if (s->data_end > size) {
- s->data_end = size;
- }
out:
qemu_co_mutex_unlock(&s->lock);
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 7/9] parallels: Move check of leaks to a separate function
2022-08-18 15:14 [PATCH 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
` (5 preceding siblings ...)
2022-08-18 15:14 ` [PATCH 6/9] parallels: Move check of cluster outside " Alexander Ivanov
@ 2022-08-18 15:14 ` Alexander Ivanov
2022-08-18 15:14 ` [PATCH 8/9] parallels: Move statistic collection " Alexander Ivanov
` (2 subsequent siblings)
9 siblings, 0 replies; 14+ messages in thread
From: Alexander Ivanov @ 2022-08-18 15:14 UTC (permalink / raw
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
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 | 84 +++++++++++++++++++++++++++++------------------
1 file changed, 52 insertions(+), 32 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 1c7626c867..6a5fe8e5b2 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -478,14 +478,14 @@ 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 = 0;
+ int64_t size, off, high_off, count;
uint32_t i;
+ int ret;
size = bdrv_getlength(bs->file->bs);
if (size < 0) {
@@ -493,41 +493,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",
@@ -545,11 +520,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 = 0;
+ 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 8/9] parallels: Move statistic collection to a separate function
2022-08-18 15:14 [PATCH 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
` (6 preceding siblings ...)
2022-08-18 15:14 ` [PATCH 7/9] parallels: Move check of leaks " Alexander Ivanov
@ 2022-08-18 15:14 ` Alexander Ivanov
2022-08-18 15:14 ` [PATCH 9/9] parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD Alexander Ivanov
2022-08-19 9:48 ` [PATCH 0/9] parallels: Refactor the code of images checks and fix a bug Denis V. Lunev
9 siblings, 0 replies; 14+ messages in thread
From: Alexander Ivanov @ 2022-08-18 15:14 UTC (permalink / raw
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
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 | 53 +++++++++++++++++++++++++++--------------------
1 file changed, 31 insertions(+), 22 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 6a5fe8e5b2..f19e86d5d2 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -528,47 +528,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 = 0;
+ 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 = 0;
+
+ 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 9/9] parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD
2022-08-18 15:14 [PATCH 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
` (7 preceding siblings ...)
2022-08-18 15:14 ` [PATCH 8/9] parallels: Move statistic collection " Alexander Ivanov
@ 2022-08-18 15:14 ` Alexander Ivanov
2022-08-19 12:53 ` Denis V. Lunev
2022-08-19 9:48 ` [PATCH 0/9] parallels: Refactor the code of images checks and fix a bug Denis V. Lunev
9 siblings, 1 reply; 14+ messages in thread
From: Alexander Ivanov @ 2022-08-18 15:14 UTC (permalink / raw
To: qemu-devel; +Cc: qemu-block, den, stefanha, vsementsov, kwolf, hreitz
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>
---
block/parallels.c | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index f19e86d5d2..173c5d3721 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -563,24 +563,22 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
BDRVParallelsState *s = bs->opaque;
int ret = 0;
- 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);
+ }
if (ret == 0) {
ret = bdrv_co_flush(bs);
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 0/9] parallels: Refactor the code of images checks and fix a bug
2022-08-18 15:14 [PATCH 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
` (8 preceding siblings ...)
2022-08-18 15:14 ` [PATCH 9/9] parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD Alexander Ivanov
@ 2022-08-19 9:48 ` Denis V. Lunev
9 siblings, 0 replies; 14+ messages in thread
From: Denis V. Lunev @ 2022-08-19 9:48 UTC (permalink / raw
To: Alexander Ivanov, qemu-devel
Cc: qemu-block, stefanha, vsementsov, kwolf, hreitz
On 18.08.2022 17:14, 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.
>
> v4 changes:
>
> Move s->data_end fixing to parallels_co_check(). Split the check
> in parallels_open() and the fix in parallels_co_check() to two patches.
>
> Move offset convertation to parallels_set_bat_entry().
>
> Fix 'ret' rewriting by bdrv_co_flush() results.
>
> Keep 'i' as uint32_t.
>
> Alexander Ivanov (9):
> parallels: Out of image offset in BAT leads to image inflation
> parallels: Fix data_end field value in parallels_co_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: 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 | 197 +++++++++++++++++++++++++++++++++-------------
> 1 file changed, 141 insertions(+), 56 deletions(-)
>
That is VERY bad that:
* you have lost (not applied) Reviewed-by: from previous iteration for
unchanged patches
* you have lost v4 tag in the subject
Den
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 9/9] parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD
2022-08-18 15:14 ` [PATCH 9/9] parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD Alexander Ivanov
@ 2022-08-19 12:53 ` Denis V. Lunev
0 siblings, 0 replies; 14+ messages in thread
From: Denis V. Lunev @ 2022-08-19 12:53 UTC (permalink / raw
To: Alexander Ivanov, qemu-devel
Cc: qemu-block, stefanha, vsementsov, kwolf, hreitz
On 18.08.2022 17:14, 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>
> ---
> block/parallels.c | 26 ++++++++++++--------------
> 1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index f19e86d5d2..173c5d3721 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -563,24 +563,22 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> BDRVParallelsState *s = bs->opaque;
> int ret = 0;
>
> - 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);
> + }
>
> if (ret == 0) {
this check is redundant from now on.
> ret = bdrv_co_flush(bs);
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 6/9] parallels: Move check of cluster outside image to a separate function
2022-08-18 15:14 ` [PATCH 6/9] parallels: Move check of cluster outside " Alexander Ivanov
@ 2022-08-19 12:56 ` Denis V. Lunev
0 siblings, 0 replies; 14+ messages in thread
From: Denis V. Lunev @ 2022-08-19 12:56 UTC (permalink / raw
To: Alexander Ivanov, qemu-devel
Cc: qemu-block, stefanha, vsementsov, kwolf, hreitz
On 18.08.2022 17:14, 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.
> s->data_end fix relates to out-of-image check so move it
> to the helper too.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
> block/parallels.c | 67 +++++++++++++++++++++++++++++++----------------
> 1 file changed, 45 insertions(+), 22 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 3900a0f4a9..1c7626c867 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -438,6 +438,46 @@ 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++;
> + }
> + }
> + }
> +
> + /*
> + * If there were an out-of-image cluster it would be repaired,
> + * but s->data_end still would point outside image.
> + * Fix s->data_end by the file size.
> + */
> + size >>= BDRV_SECTOR_BITS;
> + if (s->data_end > size) {
> + s->data_end = size;
> + }
and this is incorrect IMHO. In the next patch you could truncate the file
inside leak check and thus data_end will point to a wrong too lengthy
value.
> +
> + return 0;
> +}
> +
> static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> BdrvCheckResult *res,
> BdrvCheckMode fix)
> @@ -457,6 +497,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 */
>
> @@ -469,19 +514,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;
> @@ -518,15 +550,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> res->leaks_fixed += count;
> }
> }
> - /*
> - * If there were an out-of-image cluster it would be repaired,
> - * but s->data_end still would point outside image.
> - * Fix s->data_end by the file size.
> - */
> - size >>= BDRV_SECTOR_BITS;
> - if (s->data_end > size) {
> - s->data_end = size;
> - }
> out:
> qemu_co_mutex_unlock(&s->lock);
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/9] parallels: Fix data_end field value in parallels_co_check()
2022-08-18 15:14 ` [PATCH 2/9] parallels: Fix data_end field value in parallels_co_check() Alexander Ivanov
@ 2022-08-19 12:58 ` Denis V. Lunev
0 siblings, 0 replies; 14+ messages in thread
From: Denis V. Lunev @ 2022-08-19 12:58 UTC (permalink / raw
To: Alexander Ivanov, qemu-devel
Cc: qemu-block, stefanha, vsementsov, kwolf, hreitz
On 18.08.2022 17:14, Alexander Ivanov wrote:
> When an image is opened for check there is no error if an offset in the BAT
> points outside the image. In such a way we can repair the image.
> Out-of-image offsets are repaired in the check, but data_end field
> still points outside. Fix this field by file size.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
> block/parallels.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index c245ca35cd..24c05b95e8 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -513,7 +513,15 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
> res->leaks_fixed += count;
> }
> }
> -
> + /*
> + * If there were an out-of-image cluster it would be repaired,
> + * but s->data_end still would point outside image.
> + * Fix s->data_end by the file size.
> + */
> + size >>= BDRV_SECTOR_BITS;
> + if (s->data_end > size) {
> + s->data_end = size;
> + }
> out:
> qemu_co_mutex_unlock(&s->lock);
> return ret;
yes, but the comment is wrong. You could have adjustment to data_end
additionally once clusters outside of image are dropped - inside
leak check. Where data_end could be reduced. And this leads to
error further in the series.
Den
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-08-19 12:59 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-18 15:14 [PATCH 0/9] parallels: Refactor the code of images checks and fix a bug Alexander Ivanov
2022-08-18 15:14 ` [PATCH 1/9] parallels: Out of image offset in BAT leads to image inflation Alexander Ivanov
2022-08-18 15:14 ` [PATCH 2/9] parallels: Fix data_end field value in parallels_co_check() Alexander Ivanov
2022-08-19 12:58 ` Denis V. Lunev
2022-08-18 15:14 ` [PATCH 3/9] parallels: create parallels_set_bat_entry_helper() to assign BAT value Alexander Ivanov
2022-08-18 15:14 ` [PATCH 4/9] parallels: Use generic infrastructure for BAT writing in parallels_co_check() Alexander Ivanov
2022-08-18 15:14 ` [PATCH 5/9] parallels: Move check of unclean image to a separate function Alexander Ivanov
2022-08-18 15:14 ` [PATCH 6/9] parallels: Move check of cluster outside " Alexander Ivanov
2022-08-19 12:56 ` Denis V. Lunev
2022-08-18 15:14 ` [PATCH 7/9] parallels: Move check of leaks " Alexander Ivanov
2022-08-18 15:14 ` [PATCH 8/9] parallels: Move statistic collection " Alexander Ivanov
2022-08-18 15:14 ` [PATCH 9/9] parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD Alexander Ivanov
2022-08-19 12:53 ` Denis V. Lunev
2022-08-19 9:48 ` [PATCH 0/9] parallels: Refactor the code of images checks and fix a bug Denis V. Lunev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).