| Message ID | 20230529151503.34006-3-alexander.ivanov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | parallels: Add duplication check, repair at open, fix bugs | expand |
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; >
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;
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(-)