On Mon, Oct 2, 2023 at 12:01 PM Alexander Ivanov
<alexander.iva...@virtuozzo.com> wrote:
>
> Since we have used bitmap, field data_end in BDRVParallelsState is
> redundant and can be removed.
>
> Add parallels_data_end() helper and remove data_end handling.
>
> Signed-off-by: Alexander Ivanov <alexander.iva...@virtuozzo.com>
> ---
> block/parallels.c | 33 +++++++++++++--------------------
> block/parallels.h | 1 -
> 2 files changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 48ea5b3f03..80a7171b84 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -265,6 +265,13 @@ static void parallels_free_used_bitmap(BlockDriverState
> *bs)
> g_free(s->used_bmap);
> }
>
> +static int64_t parallels_data_end(BDRVParallelsState *s)
> +{
> + int64_t data_end = s->data_start * BDRV_SECTOR_SIZE;
> + data_end += s->used_bmap_size * s->cluster_size;
> + return data_end;
> +}
> +
> int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
> int64_t *clusters)
> {
> @@ -275,7 +282,7 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
> *bs,
>
> first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
> if (first_free == s->used_bmap_size) {
> - host_off = s->data_end * BDRV_SECTOR_SIZE;
> + host_off = parallels_data_end(s);
> prealloc_clusters = *clusters + s->prealloc_size / s->tracks;
> bytes = prealloc_clusters * s->cluster_size;
>
> @@ -297,9 +304,6 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
> *bs,
> s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
> new_usedsize);
> s->used_bmap_size = new_usedsize;
> - if (host_off + bytes > s->data_end * BDRV_SECTOR_SIZE) {
> - s->data_end = (host_off + bytes) / BDRV_SECTOR_SIZE;
> - }
> } else {
> next_used = find_next_bit(s->used_bmap, s->used_bmap_size,
> first_free);
>
> @@ -315,8 +319,7 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
> *bs,
> * branch. In the other case we are likely re-using hole. Preallocate
> * the space if required by the prealloc_mode.
> */
> - if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE &&
> - host_off < s->data_end * BDRV_SECTOR_SIZE) {
> + if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
> ret = bdrv_pwrite_zeroes(bs->file, host_off, bytes, 0);
> if (ret < 0) {
> return ret;
> @@ -757,13 +760,7 @@ parallels_check_outside_image(BlockDriverState *bs,
> BdrvCheckResult *res,
> }
> }
>
> - if (high_off == 0) {
> - res->image_end_offset = s->data_end << BDRV_SECTOR_BITS;
> - } else {
> - res->image_end_offset = high_off + s->cluster_size;
> - s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
> - }
> -
> + res->image_end_offset = parallels_data_end(s);
> return 0;
> }
>
> @@ -806,7 +803,6 @@ parallels_check_leak(BlockDriverState *bs,
> BdrvCheckResult *res,
> 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);
> @@ -1361,8 +1357,7 @@ static int parallels_open(BlockDriverState *bs, QDict
> *options, int flags,
> }
>
> s->data_start = data_start;
> - s->data_end = s->data_start;
> - if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
> + if (s->data_start < (s->header_size >> BDRV_SECTOR_BITS)) {
> /*
> * There is not enough unused space to fit to block align between BAT
> * and actual data. We can't avoid read-modify-write...
> @@ -1403,11 +1398,10 @@ static int parallels_open(BlockDriverState *bs, QDict
> *options, int flags,
>
> for (i = 0; i < s->bat_size; i++) {
> sector = bat2sect(s, i);
> - if (sector + s->tracks > s->data_end) {
> - s->data_end = sector + s->tracks;
> + if (sector + s->tracks > file_nb_sectors) {
> + need_check = true;
> }
> }
> - need_check = need_check || s->data_end > file_nb_sectors;
>
> ret = parallels_fill_used_bitmap(bs);
> if (ret == -ENOMEM) {
> @@ -1461,7 +1455,6 @@ static int
> parallels_truncate_unused_clusters(BlockDriverState *bs)
> 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);
> }
>
> diff --git a/block/parallels.h b/block/parallels.h
> index 18b4f8068e..a6a048d890 100644
> --- a/block/parallels.h
> +++ b/block/parallels.h
> @@ -79,7 +79,6 @@ typedef struct BDRVParallelsState {
> unsigned int bat_size;
>
> int64_t data_start;
> - int64_t data_end;
> uint64_t prealloc_size;
> ParallelsPreallocMode prealloc_mode;
>
> --
> 2.34.1
>
Is it intended behavior?
Run:
1. ./qemu-img create -f parallels $TEST_IMG 1T
2. dd if=/dev/zero of=$TEST_IMG oseek=12 bs=1M count=128 conv=notrunc
3. ./qemu-img check $TEST_IMG
No errors were found on the image.
Image end offset: 150994944
Without this patch `qemu-img check` reports:
ERROR space leaked at the end of the image 145752064
139 leaked clusters were found on the image.
This means waste of disk space, but no harm to data.
Image end offset: 5242880
Note: there is another issue caused by previous commits exists.
g_free asserts from parallels_free_used_bitmap() because of
s->used_bmap is NULL.
To reproduce this crash at revision before or without patch 15/19, run commands:
1. ./qemu-img create -f parallels $TEST_IMG 1T
2. dd if=/dev/zero of=$TEST_IMG oseek=12 bs=1M count=128 conv=notrunc
3. ./qemu-img check -r leaks $TEST_IMG
Regards,
Mike.
