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.ivanov@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);
+
+ 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;
}
+ res->leaks_fixed += count;
}
return 0;
--
2.34.1
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.ivanov@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? > + > + 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;
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.ivanov@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
© 2016 - 2024 Red Hat, Inc.