On 6/2/23 16:08, Hanna Czenczek wrote:
On 29.05.23 17:15, Alexander Ivanov wrote:
We need to fix leak after deduplication in the next patch. Move leak
fixing to a separate helper parallels_fix_leak() and add
parallels_get_leak_size() helper wich used in parallels_fix_leak() and
parallels_check_leak().
Signed-off-by: Alexander Ivanov <alexander.iva...@virtuozzo.com>
---
block/parallels.c | 86 +++++++++++++++++++++++++++++++++--------------
1 file changed, 61 insertions(+), 25 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 1ec98c722b..64850b9655 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -482,43 +482,79 @@ parallels_check_outside_image(BlockDriverState
*bs, BdrvCheckResult *res,
return 0;
}
+static int64_t parallels_get_leak_size(BlockDriverState *bs,
+ BdrvCheckResult *res)
+{
+ int64_t size;
+
+ size = bdrv_getlength(bs->file->bs);
+ if (size < 0) {
+ return size;
+ }
+
+ /*
+ * Before any usage of this function, image_end_offset has to be
set to the
+ * the highest offset in the BAT, excluding out-of-image offsets.
+ */
+ assert(size >= res->image_end_offset);
If `high_off == 0` in parallels_check_outside_image(), it will use
s->data_end to determine image_end_offset, which is originally read
from the image header. I don’t see any place where we ensure that
`s->data_end <= bdrv_getlength(bs->file->bs)`, so can we be certain
the assertion holds even in that case?
Will add s->data_end > file_nb_sectors check to parallels_open(). Thank you.
+
+ return size - res->image_end_offset;
+}
+
+static int parallels_fix_leak(BlockDriverState *bs,
+ BdrvCheckResult *res)
+{
+ Error *local_err = NULL;
+ int64_t size;
+ int ret;
+
+ size = parallels_get_leak_size(bs, res);
+ if (size <= 0) {
+ return size;
+ }
+
+ /*
+ * 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);
+ return ret;
+ }
+
+ return 0;
+}
+
static int coroutine_fn GRAPH_RDLOCK
parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
BdrvCheckMode fix)
{
BDRVParallelsState *s = bs->opaque;
- int64_t size;
+ int64_t count, leak_size;
int ret;
- size = bdrv_getlength(bs->file->bs);
- if (size < 0) {
+ leak_size = parallels_get_leak_size(bs, res);
+ if (leak_size < 0) {
res->check_errors++;
- return size;
+ return leak_size;
+ }
+ if (leak_size == 0) {
+ return 0;
}
- 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;
+ count = DIV_ROUND_UP(leak_size, s->cluster_size);
+ res->leaks += count;
+ fprintf(stderr, "%s space leaked at the end of the image %"
PRId64 "\n",
+ fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size);
- /*
- * 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;
+ if (fix & BDRV_FIX_LEAKS) {
+ ret = parallels_fix_leak(bs, res);
+ if (ret < 0) {
+ return ret;
We used to increment res->check_errors here – should we still do that?
Hanna
}
+ res->leaks_fixed += count;
}
return 0;
--
Best regards,
Alexander Ivanov