Re: [PATCH v2 2/5] parallels: Split image leak handling to separate check and fix helpers

[Denis V. Lunev]

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;
>   }
>