Re: [PATCH v4 11/21] parallels: Add dirty bitmaps saving
On 1/18/24 14:27, Denis V. Lunev wrote: On 12/28/23 11:12, Alexander Ivanov wrote: Now dirty bitmaps can be loaded but there is no their saving. Add code for dirty bitmap storage. Signed-off-by: Alexander Ivanov --- block/parallels-ext.c | 168 ++ block/parallels.c | 16 +++- block/parallels.h | 5 ++ 3 files changed, 187 insertions(+), 2 deletions(-) diff --git a/block/parallels-ext.c b/block/parallels-ext.c index b4e14c88f2..c83d1ea393 100644 --- a/block/parallels-ext.c +++ b/block/parallels-ext.c @@ -24,6 +24,7 @@ */ #include "qemu/osdep.h" +#include "qemu/error-report.h" #include "qapi/error.h" #include "block/block-io.h" #include "block/block_int.h" @@ -300,3 +301,170 @@ out: return ret; } + +static void GRAPH_RDLOCK parallels_save_bitmap(BlockDriverState *bs, + BdrvDirtyBitmap *bitmap, + uint8_t **buf, int *buf_size) Do we need a error? I think no. We save bitmaps in a loop. If some bitmap has a problem it would be better just to print an error message and try to save other bitmaps. +{ + BDRVParallelsState *s = bs->opaque; + ParallelsFeatureHeader *fh; + ParallelsDirtyBitmapFeature *bh; + uint64_t *l1_table, l1_size, granularity, limit; I would say that 'limit' here means 'bits_in_cluster' We are writing the new code and I would prefer if we would have bits, bytes, clusters, sectors etc are clearly seen in variable names. It is quite complex to track various measurables. OK + int64_t bm_size, ser_size, offset, buf_used; + int64_t alloc_size = 1; + const char *name; + uint8_t *bm_buf; + QemuUUID uuid; + int ret = 0; + + if (!bdrv_dirty_bitmap_get_persistence(bitmap) || + bdrv_dirty_bitmap_inconsistent(bitmap)) { + return; + } + + bm_size = bdrv_dirty_bitmap_size(bitmap); + granularity = bdrv_dirty_bitmap_granularity(bitmap); + limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap); + ser_size = bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size); + l1_size = DIV_ROUND_UP(ser_size, s->cluster_size); + + buf_used = l1_size * 8 + sizeof(*fh) + sizeof(*bh); As far as I can see, bdrv_dirty_bitmap_serialization_size() returns bytes. That is correct. Thus multiplying it by 8 seems fatal mistake. l1_size is amount of entries in l1_table. Every entry has 8 bytes size and corresponds to one cluster containing a part of a bitmap. I am also quite unsure that we should roundup to the cluster, that will occupy more clusters than needed. Can you please take a look here https://src.openvz.org/users/ibazhitov/repos/ploop/browse/tools/ploop-cbt.c Why shouldn't we? We need to have l1_table which should be able to contain all the bitmap data. Note: there is not a ROUND_UP(), but DIV_ROUND_UP(). + /* Check if there is enough space for the final section */ + if (*buf_size - buf_used < sizeof(*fh)) { + return; + } + + name = bdrv_dirty_bitmap_name(bitmap); + ret = qemu_uuid_parse(name, ); + if (ret < 0) { + error_report("Can't save dirty bitmap: ID parsing error: '%s'", name); + return; + } + + fh = (ParallelsFeatureHeader *)*buf; + bh = (ParallelsDirtyBitmapFeature *)(*buf + sizeof(*fh)); bh = fh + 1 ? + l1_table = (uint64_t *)((uint8_t *)bh + sizeof(*bh)); l1_table = bh + 1 ? Yes, it's better. + + fh->magic = cpu_to_le64(PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC); + fh->data_size = cpu_to_le32(l1_size * 8 + sizeof(*bh)); I am quite concerned here. Please compare with int save_dirty_bitmap(int devfd, struct delta *delta, off_t offset, void *buf, __u32 *size, void *or_data, writer_fn wr, void *data) { int ret = 0; struct ploop_pvd_header *vh; size_t block_size; __u64 bits, bytes, *p; __u32 byte_granularity; void *block; struct ploop_pvd_dirty_bitmap_raw *raw = (struct ploop_pvd_dirty_bitmap_raw *)buf; char x[50]; vh = (struct ploop_pvd_header *)delta->hdr0; /* granularity and uuid */ if ((ret = cbt_get_dirty_bitmap_metadata(devfd, raw->m_Id, >m_Granularity))) return ret; raw->m_Granularity /= SECTOR_SIZE; block_size = vh->m_Sectors * SECTOR_SIZE; if (p_memalign((void **), 4096, block_size)) return SYSEXIT_MALLOC; raw->m_Size = vh->m_SizeInSectors_v2; byte_granularity = raw->m_Granularity * SECTOR_SIZE; bits = ((raw->m_Size + raw->m_Granularity - 1) / raw->m_Granularity); bytes = (bits + 7) >> 3; raw->m_L1Size = (bytes + block_size - 1) / block_size; which means that the header is rotten. In that case can you pls take a look why this has not been caught by tests? It looks the same if block_size is cluster size in bytes. bytes - bitmap size in bytes raw->m_L1Size - bitmap size in clusters In my code fh->data_size is l1_table size (8 bytes entry for each
Re: [PATCH v4 11/21] parallels: Add dirty bitmaps saving
On 12/28/23 11:12, Alexander Ivanov wrote: Now dirty bitmaps can be loaded but there is no their saving. Add code for dirty bitmap storage. Signed-off-by: Alexander Ivanov --- block/parallels-ext.c | 168 ++ block/parallels.c | 16 +++- block/parallels.h | 5 ++ 3 files changed, 187 insertions(+), 2 deletions(-) diff --git a/block/parallels-ext.c b/block/parallels-ext.c index b4e14c88f2..c83d1ea393 100644 --- a/block/parallels-ext.c +++ b/block/parallels-ext.c @@ -24,6 +24,7 @@ */ #include "qemu/osdep.h" +#include "qemu/error-report.h" #include "qapi/error.h" #include "block/block-io.h" #include "block/block_int.h" @@ -300,3 +301,170 @@ out: return ret; } + +static void GRAPH_RDLOCK parallels_save_bitmap(BlockDriverState *bs, + BdrvDirtyBitmap *bitmap, + uint8_t **buf, int *buf_size) Do we need a error? +{ +BDRVParallelsState *s = bs->opaque; +ParallelsFeatureHeader *fh; +ParallelsDirtyBitmapFeature *bh; +uint64_t *l1_table, l1_size, granularity, limit; I would say that 'limit' here means 'bits_in_cluster' We are writing the new code and I would prefer if we would have bits, bytes, clusters, sectors etc are clearly seen in variable names. It is quite complex to track various measurables. +int64_t bm_size, ser_size, offset, buf_used; +int64_t alloc_size = 1; +const char *name; +uint8_t *bm_buf; +QemuUUID uuid; +int ret = 0; + +if (!bdrv_dirty_bitmap_get_persistence(bitmap) || +bdrv_dirty_bitmap_inconsistent(bitmap)) { +return; +} + +bm_size = bdrv_dirty_bitmap_size(bitmap); +granularity = bdrv_dirty_bitmap_granularity(bitmap); +limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap); +ser_size = bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size); +l1_size = DIV_ROUND_UP(ser_size, s->cluster_size); + +buf_used = l1_size * 8 + sizeof(*fh) + sizeof(*bh); As far as I can see, bdrv_dirty_bitmap_serialization_size() returns bytes. That is correct. Thus multiplying it by 8 seems fatal mistake. I am also quite unsure that we should roundup to the cluster, that will occupy more clusters than needed. Can you please take a look here https://src.openvz.org/users/ibazhitov/repos/ploop/browse/tools/ploop-cbt.c +/* Check if there is enough space for the final section */ +if (*buf_size - buf_used < sizeof(*fh)) { +return; +} + +name = bdrv_dirty_bitmap_name(bitmap); +ret = qemu_uuid_parse(name, ); +if (ret < 0) { +error_report("Can't save dirty bitmap: ID parsing error: '%s'", name); +return; +} + +fh = (ParallelsFeatureHeader *)*buf; +bh = (ParallelsDirtyBitmapFeature *)(*buf + sizeof(*fh)); bh = fh + 1 ? +l1_table = (uint64_t *)((uint8_t *)bh + sizeof(*bh)); l1_table = bh + 1 ? + +fh->magic = cpu_to_le64(PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC); +fh->data_size = cpu_to_le32(l1_size * 8 + sizeof(*bh)); I am quite concerned here. Please compare with int save_dirty_bitmap(int devfd, struct delta *delta, off_t offset, void *buf, __u32 *size, void *or_data, writer_fn wr, void *data) { int ret = 0; struct ploop_pvd_header *vh; size_t block_size; __u64 bits, bytes, *p; __u32 byte_granularity; void *block; struct ploop_pvd_dirty_bitmap_raw *raw = (struct ploop_pvd_dirty_bitmap_raw *)buf; char x[50]; vh = (struct ploop_pvd_header *)delta->hdr0; /* granularity and uuid */ if ((ret = cbt_get_dirty_bitmap_metadata(devfd, raw->m_Id, >m_Granularity))) return ret; raw->m_Granularity /= SECTOR_SIZE; block_size = vh->m_Sectors * SECTOR_SIZE; if (p_memalign((void **), 4096, block_size)) return SYSEXIT_MALLOC; raw->m_Size = vh->m_SizeInSectors_v2; byte_granularity = raw->m_Granularity * SECTOR_SIZE; bits = ((raw->m_Size + raw->m_Granularity - 1) / raw->m_Granularity); bytes = (bits + 7) >> 3; raw->m_L1Size = (bytes + block_size - 1) / block_size; which means that the header is rotten. In that case can you pls take a look why this has not been caught by tests? + +bh->l1_size = cpu_to_le32(l1_size); +bh->size = cpu_to_le64(bm_size >> BDRV_SECTOR_BITS); +bh->granularity = cpu_to_le32(granularity >> BDRV_SECTOR_BITS); +memcpy(bh->id, , sizeof(uuid)); + +bm_buf = qemu_blockalign(bs, s->cluster_size); + +offset = 0; +while ((offset = bdrv_dirty_bitmap_next_dirty(bitmap, offset, bm_size)) >= 0) { +uint64_t idx = offset / limit; +int64_t cluster_off, end, write_size; + +offset = QEMU_ALIGN_DOWN(offset, limit); +end = MIN(bm_size, offset + limit); +write_size = bdrv_dirty_bitmap_serialization_size(bitmap, offset, + end - offset); +
[PATCH v4 11/21] parallels: Add dirty bitmaps saving
Now dirty bitmaps can be loaded but there is no their saving. Add code for dirty bitmap storage. Signed-off-by: Alexander Ivanov --- block/parallels-ext.c | 168 ++ block/parallels.c | 16 +++- block/parallels.h | 5 ++ 3 files changed, 187 insertions(+), 2 deletions(-) diff --git a/block/parallels-ext.c b/block/parallels-ext.c index b4e14c88f2..c83d1ea393 100644 --- a/block/parallels-ext.c +++ b/block/parallels-ext.c @@ -24,6 +24,7 @@ */ #include "qemu/osdep.h" +#include "qemu/error-report.h" #include "qapi/error.h" #include "block/block-io.h" #include "block/block_int.h" @@ -300,3 +301,170 @@ out: return ret; } + +static void GRAPH_RDLOCK parallels_save_bitmap(BlockDriverState *bs, + BdrvDirtyBitmap *bitmap, + uint8_t **buf, int *buf_size) +{ +BDRVParallelsState *s = bs->opaque; +ParallelsFeatureHeader *fh; +ParallelsDirtyBitmapFeature *bh; +uint64_t *l1_table, l1_size, granularity, limit; +int64_t bm_size, ser_size, offset, buf_used; +int64_t alloc_size = 1; +const char *name; +uint8_t *bm_buf; +QemuUUID uuid; +int ret = 0; + +if (!bdrv_dirty_bitmap_get_persistence(bitmap) || +bdrv_dirty_bitmap_inconsistent(bitmap)) { +return; +} + +bm_size = bdrv_dirty_bitmap_size(bitmap); +granularity = bdrv_dirty_bitmap_granularity(bitmap); +limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap); +ser_size = bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size); +l1_size = DIV_ROUND_UP(ser_size, s->cluster_size); + +buf_used = l1_size * 8 + sizeof(*fh) + sizeof(*bh); +/* Check if there is enough space for the final section */ +if (*buf_size - buf_used < sizeof(*fh)) { +return; +} + +name = bdrv_dirty_bitmap_name(bitmap); +ret = qemu_uuid_parse(name, ); +if (ret < 0) { +error_report("Can't save dirty bitmap: ID parsing error: '%s'", name); +return; +} + +fh = (ParallelsFeatureHeader *)*buf; +bh = (ParallelsDirtyBitmapFeature *)(*buf + sizeof(*fh)); +l1_table = (uint64_t *)((uint8_t *)bh + sizeof(*bh)); + +fh->magic = cpu_to_le64(PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC); +fh->data_size = cpu_to_le32(l1_size * 8 + sizeof(*bh)); + +bh->l1_size = cpu_to_le32(l1_size); +bh->size = cpu_to_le64(bm_size >> BDRV_SECTOR_BITS); +bh->granularity = cpu_to_le32(granularity >> BDRV_SECTOR_BITS); +memcpy(bh->id, , sizeof(uuid)); + +bm_buf = qemu_blockalign(bs, s->cluster_size); + +offset = 0; +while ((offset = bdrv_dirty_bitmap_next_dirty(bitmap, offset, bm_size)) >= 0) { +uint64_t idx = offset / limit; +int64_t cluster_off, end, write_size; + +offset = QEMU_ALIGN_DOWN(offset, limit); +end = MIN(bm_size, offset + limit); +write_size = bdrv_dirty_bitmap_serialization_size(bitmap, offset, + end - offset); +assert(write_size <= s->cluster_size); + +bdrv_dirty_bitmap_serialize_part(bitmap, bm_buf, offset, end - offset); +if (write_size < s->cluster_size) { +memset(bm_buf + write_size, 0, s->cluster_size - write_size); +} + +cluster_off = parallels_allocate_host_clusters(bs, _size); +if (cluster_off <= 0) { +goto end; +} + +ret = bdrv_pwrite(bs->file, cluster_off, s->cluster_size, bm_buf, 0); +if (ret < 0) { +memset(>magic, 0, sizeof(fh->magic)); +parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size, + cluster_off, 1); +goto end; +} + +l1_table[idx] = cpu_to_le64(cluster_off >> BDRV_SECTOR_BITS); +offset = end; +} + +*buf_size -= buf_used; +*buf += buf_used; + +end: +qemu_vfree(bm_buf); +} + +void GRAPH_RDLOCK +parallels_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) +{ +BDRVParallelsState *s = bs->opaque; +BdrvDirtyBitmap *bitmap; +ParallelsFormatExtensionHeader *eh; +int remaining = s->cluster_size; +uint8_t *buf, *pos; +int64_t header_off, alloc_size = 1; +g_autofree uint8_t *hash = NULL; +size_t hash_len = 0; +int ret; + +s->header->ext_off = 0; + +if (!bdrv_has_named_bitmaps(bs)) { +return; +} + +buf = qemu_blockalign0(bs, s->cluster_size); + +eh = (ParallelsFormatExtensionHeader *)buf; +pos = buf + sizeof(*eh); + +eh->magic = cpu_to_le64(PARALLELS_FORMAT_EXTENSION_MAGIC); + +FOR_EACH_DIRTY_BITMAP(bs, bitmap) { +parallels_save_bitmap(bs, bitmap, , ); +} + +header_off = parallels_allocate_host_clusters(bs, _size); +if (header_off < 0) { +error_report("Can't save dirty bitmap: cluster allocation error"); +ret = header_off; +goto