| Message ID | 20230112150149.1452033-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 1/12/23 16:01, 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 | 88 ++++++++++++++++++++++++++++++++--------------- > 1 file changed, 61 insertions(+), 27 deletions(-) > > diff --git a/block/parallels.c b/block/parallels.c > index ed2cf27abc..da1e75096c 100644 > --- a/block/parallels.c > +++ b/block/parallels.c > @@ -475,21 +475,53 @@ static int parallels_check_outside_image(BlockDriverState *bs, > return 0; > } > > +static int64_t parallels_get_leak_size(BlockDriverState *bs, > + BdrvCheckResult *res) > +{ > + int64_t size; > + size = bdrv_getlength(bs->file->bs); > + /* > + * Before any usage of this function out-of-image corruption has been > + * fixed. If the function returns a negative value, it means an error. > + */ > + return (size < 0) ? size : (size - res->image_end_offset); I'd better use normal 'if' here and will add an assert that 'size' >= 'res->image_end_offset' on success path. > +} > + > +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 parallels_check_leak(BlockDriverState *bs, > BdrvCheckResult *res, > BdrvCheckMode fix) > { > BDRVParallelsState *s = bs->opaque; > - int64_t size, off, high_off, count; > + int64_t off, high_off, count, leak_size; > uint32_t i; > int ret; > > - size = bdrv_getlength(bs->file->bs); > - if (size < 0) { > - res->check_errors++; > - return size; > - } > - > high_off = 0; > for (i = 0; i < s->bat_size; i++) { > off = bat2sect(s, i) << BDRV_SECTOR_BITS; > @@ -499,30 +531,32 @@ static int parallels_check_leak(BlockDriverState *bs, > } > > res->image_end_offset = high_off + s->cluster_size; > - if (size > res->image_end_offset) { > - 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; > + leak_size = parallels_get_leak_size(bs, res); > + if (leak_size < 0) { > + res->check_errors++; > + return leak_size; > + } > + if (leak_size == 0) { > + return 0; > + } > + > + if (fix & BDRV_FIX_LEAKS) { > + ret = parallels_fix_leak(bs, res); you have changed semantics a bit - with a error here the message that we do get leak will be lost. I think the code is to be restructured a bit here: * move printing above this if * fill leaks_fixed inside this if on the success path This will be much straight forward Den > + if (ret < 0) { > + return ret; > } > } > > + count = DIV_ROUND_UP(leak_size, s->cluster_size); > + fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n", > + fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size); > + > + res->leaks += count; > + if (fix & BDRV_FIX_LEAKS) { > + res->leaks_fixed += count; > + } > + > return 0; > } >
diff --git a/block/parallels.c b/block/parallels.c index ed2cf27abc..da1e75096c 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -475,21 +475,53 @@ static int parallels_check_outside_image(BlockDriverState *bs, return 0; } +static int64_t parallels_get_leak_size(BlockDriverState *bs, + BdrvCheckResult *res) +{ + int64_t size; + size = bdrv_getlength(bs->file->bs); + /* + * Before any usage of this function out-of-image corruption has been + * fixed. If the function returns a negative value, it means an error. + */ + return (size < 0) ? size : (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 parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix) { BDRVParallelsState *s = bs->opaque; - int64_t size, off, high_off, count; + int64_t off, high_off, count, leak_size; uint32_t i; int ret; - size = bdrv_getlength(bs->file->bs); - if (size < 0) { - res->check_errors++; - return size; - } - high_off = 0; for (i = 0; i < s->bat_size; i++) { off = bat2sect(s, i) << BDRV_SECTOR_BITS; @@ -499,30 +531,32 @@ static int parallels_check_leak(BlockDriverState *bs, } res->image_end_offset = high_off + s->cluster_size; - if (size > res->image_end_offset) { - 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; + leak_size = parallels_get_leak_size(bs, res); + if (leak_size < 0) { + res->check_errors++; + return leak_size; + } + if (leak_size == 0) { + return 0; + } + + if (fix & BDRV_FIX_LEAKS) { + ret = parallels_fix_leak(bs, res); + if (ret < 0) { + return ret; } } + count = DIV_ROUND_UP(leak_size, s->cluster_size); + fprintf(stderr, "%s space leaked at the end of the image %" PRId64 "\n", + fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak_size); + + res->leaks += count; + if (fix & BDRV_FIX_LEAKS) { + 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 | 88 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 61 insertions(+), 27 deletions(-)