[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 15/19] parallels: Remove unnecessary data_end field
From: |
Mike Maslenkin |
Subject: |
Re: [PATCH 15/19] parallels: Remove unnecessary data_end field |
Date: |
Fri, 6 Oct 2023 22:43:34 +0300 |
On Mon, Oct 2, 2023 at 12:01 PM Alexander Ivanov
<alexander.ivanov@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.ivanov@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.
- [PATCH 01/19] parallels: Move inactivation code to a separate function, (continued)
- [PATCH 01/19] parallels: Move inactivation code to a separate function, Alexander Ivanov, 2023/10/02
- [PATCH 11/19] parallels: Handle L1 entries equal to one, Alexander Ivanov, 2023/10/02
- [PATCH 07/19] parallels: Create used bitmap even if checks needed, Alexander Ivanov, 2023/10/02
- [PATCH 17/19] tests: Add parallels images support to test 165, Alexander Ivanov, 2023/10/02
- [PATCH 09/19] parallels: Add dirty bitmaps saving, Alexander Ivanov, 2023/10/02
- [PATCH 08/19] parallels: Make mark_used() and mark_unused() global functions, Alexander Ivanov, 2023/10/02
- [PATCH 14/19] parallels: Truncate images on the last used cluster, Alexander Ivanov, 2023/10/02
- [PATCH 10/19] parallels: Let image extensions work in RW mode, Alexander Ivanov, 2023/10/02
- [PATCH 12/19] parallels: Make a loaded dirty bitmap persistent, Alexander Ivanov, 2023/10/02
- [PATCH 15/19] parallels: Remove unnecessary data_end field, Alexander Ivanov, 2023/10/02
- Re: [PATCH 15/19] parallels: Remove unnecessary data_end field,
Mike Maslenkin <=
- Re: [PATCH 15/19] parallels: Remove unnecessary data_end field, Alexander Ivanov, 2023/10/07
- Re: [PATCH 15/19] parallels: Remove unnecessary data_end field, Mike Maslenkin, 2023/10/07
- Re: [PATCH 15/19] parallels: Remove unnecessary data_end field, Alexander Ivanov, 2023/10/07
- Re: [PATCH 15/19] parallels: Remove unnecessary data_end field, Mike Maslenkin, 2023/10/07
- Re: [PATCH 15/19] parallels: Remove unnecessary data_end field, Alexander Ivanov, 2023/10/08
[PATCH 13/19] parallels: Reverse a conditional in parallels_check_leak() to reduce indents, Alexander Ivanov, 2023/10/02
[PATCH 19/19] tests: Add parallels format support to image-fleecing, Alexander Ivanov, 2023/10/02
[PATCH 16/19] parallels: Check unused clusters in parallels_check_leak(), Alexander Ivanov, 2023/10/02