Re: [PATCH v4 17/21] parallels: Check unused clusters in parallels_check_leak()

["Denis V. Lunev" <den@virtuozzo.com>]

On 12/28/23 11:12, Alexander Ivanov wrote:
> Since we have used bitmap, leak check is useless. Transform
> parallels_truncate_unused_clusters() to parallels_check_unused_clusters()
> helper and use it in leak check.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>   block/parallels.c | 121 +++++++++++++++++++++++++---------------------
>   1 file changed, 67 insertions(+), 54 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 136865d53e..5ed58826bb 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -768,57 +768,87 @@ parallels_check_outside_image(BlockDriverState *bs, BdrvCheckResult *res,
>       return 0;
>   }
>   
> +static int64_t GRAPH_RDLOCK
> +parallels_check_unused_clusters(BlockDriverState *bs, bool truncate)
> +{
> +    BDRVParallelsState *s = bs->opaque;
> +    int64_t leak, file_size, end_off = 0;
> +    int ret;
> +
> +    file_size = bdrv_getlength(bs->file->bs);
> +    if (file_size < 0) {
> +        return file_size;
> +    }
> +
> +    if (s->used_bmap_size > 0) {
> +        end_off = find_last_bit(s->used_bmap, s->used_bmap_size);
> +        if (end_off == s->used_bmap_size) {
> +            end_off = 0;
> +        } else {
> +            end_off = (end_off + 1) * s->cluster_size;
> +        }
> +    }
> +
> +    end_off += s->data_start * BDRV_SECTOR_SIZE;
> +    leak = file_size - end_off;
> +    if (leak < 0) {
> +        return -EINVAL;
> +    }
> +    if (!truncate || leak == 0) {
> +        return leak;
> +    }
> +
> +    ret = bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    s->data_end = end_off / BDRV_SECTOR_SIZE;
> +
> +    parallels_free_used_bitmap(bs);
> +    ret = parallels_fill_used_bitmap(bs);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    return leak;
> +}
> +
>   static int coroutine_fn GRAPH_RDLOCK
>   parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
>                        BdrvCheckMode fix, bool explicit)
>   {
>       BDRVParallelsState *s = bs->opaque;
> -    int64_t size, count;
> -    int ret;
> +    int64_t leak, count, size;
> +
> +    leak = parallels_check_unused_clusters(bs, fix & BDRV_FIX_LEAKS);
> +    if (leak < 0) {
> +        res->check_errors++;
> +        return leak;
> +    }
> +    if (leak == 0) {
> +        return 0;
> +    }
>   
>       size = bdrv_co_getlength(bs->file->bs);
>       if (size < 0) {
>           res->check_errors++;
>           return size;
>       }
> -    if (size <= res->image_end_offset) {
> +    res->image_end_offset = size;
> +
> +    if (!explicit) {
>           return 0;
>       }
>   
> -    count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
> -    if (explicit) {
> -        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;
> -    }
> +    count = DIV_ROUND_UP(leak, s->cluster_size);
> +    fprintf(stderr,
> +            "%s space leaked at the end of the image %" PRId64 "\n",
> +            fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak);
> +    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;
> -        }
> -        s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
> -
> -        parallels_free_used_bitmap(bs);
> -        ret = parallels_fill_used_bitmap(bs);
> -        if (ret == -ENOMEM) {
> -            res->check_errors++;
> -            return ret;
> -        }
> -
> -        if (explicit) {
> -            res->leaks_fixed += count;
> -        }
> +        res->leaks_fixed += count;
>       }
>   
>       return 0;
> @@ -1454,23 +1484,6 @@ fail:
>       return ret;
>   }
>   
> -static int GRAPH_RDLOCK parallels_truncate_unused_clusters(BlockDriverState *bs)
> -{
> -    BDRVParallelsState *s = bs->opaque;
> -    uint64_t end_off = 0;
> -    if (s->used_bmap_size > 0) {
> -        end_off = find_last_bit(s->used_bmap, s->used_bmap_size);
> -        if (end_off == s->used_bmap_size) {
> -            end_off = 0;
> -        } else {
> -            end_off = (end_off + 1) * s->cluster_size;
> -        }
> -    }
> -    end_off += s->data_start * BDRV_SECTOR_SIZE;
> -    s->data_end = end_off / BDRV_SECTOR_SIZE;
> -    return bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
> -}
> -
>   static int GRAPH_RDLOCK parallels_inactivate(BlockDriverState *bs)
>   {
>       BDRVParallelsState *s = bs->opaque;
> @@ -1488,7 +1501,7 @@ static int GRAPH_RDLOCK parallels_inactivate(BlockDriverState *bs)
>       parallels_update_header(bs);
>   
>       /* errors are ignored, so we might as well pass exact=true */
> -    ret = parallels_truncate_unused_clusters(bs);
> +    ret = parallels_check_unused_clusters(bs, true);
>       if (ret < 0) {
>           error_report("Failed to truncate image: %s", strerror(-ret));
>       }
This particular patch does not look good.

You are deleting the stuff you have just added (in the previous patch)
and you add lengthy operation (recreate used bitmap) on the image close,
which is better to be finished first.

I would say that the concept should be somehow reworked or the
whole patch is to be dropped.