On 10/6/23 21:43, Mike Maslenkin wrote:
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.1Is 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
The original intention is do nothing at this point if an image is opened as RO. In the next patch parallels_check_leak() was rewritten to detect unused clusters at the image end. But there is a bug: (end_off == s->used_bmap_size) case is handled in an incorrect way. Will fix it, thank you.
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.
Maybe I don't understand your point, but if you meant that g_free() could be called with NULL argument, it's not a problem. GLib Manual says: void g_free (/|gpointer <https://www.manpagez.com/html/glib/glib-2.56.0/glib-Basic-Types.php#gpointer> mem|/); If /|mem|/ is |NULL| <https://www.manpagez.com/html/glib/glib-2.56.0/glib-Standard-Macros.php#NULL:CAPS> it simply returns, so there is no need to check /|mem|/ against |NULL| <https://www.manpagez.com/html/glib/glib-2.56.0/glib-Standard-Macros.php#NULL:CAPS> before calling this function.
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
Sorry, but I couldn't reproduce it. Reset to 14/19, made the three steps and had such output: $ ./qemu-img create -f parallels $TEST_IMG 1T Formatting 'test.img', fmt=parallels size=1099511627776 cluster_size=1048576 $ dd if=/dev/zero of=$TEST_IMG seek=12 bs=1M count=128 conv=notrunc 128+0 records in 128+0 records out 134217728 bytes (134 MB, 128 MiB) copied, 0.0797576 s, 1.7 GB/s $ ./qemu-img check -r leaks $TEST_IMG Repairing space leaked at the end of the image 141557760 The following inconsistencies were found and repaired: 135 leaked clusters 0 corruptions Double checking the fixed image now... No errors were found on the image. Image end offset: 5242880 Checked with GCC and Clang.
Regards, Mike.
-- Best regards, Alexander Ivanov
