| Message ID | 1479835586-74394-14-git-send-email-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote: > Realize block bitmap storing interface, to allow qcow2 images store > persistent bitmaps. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2-bitmap.c | 451 +++++++++++++++++++++++++++++++++++++++++++++++++++ > block/qcow2.c | 1 + > block/qcow2.h | 1 + > 3 files changed, 453 insertions(+) > > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index 81be1ca..a975388 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -28,6 +28,7 @@ > #include "qemu/osdep.h" > #include "qapi/error.h" > #include "exec/log.h" > +#include "qemu/cutils.h" > > #include "block/block_int.h" > #include "block/qcow2.h" > @@ -43,6 +44,10 @@ > #define BME_MIN_GRANULARITY_BITS 9 > #define BME_MAX_NAME_SIZE 1023 > > +#if BME_MAX_TABLE_SIZE * 8ULL > INT_MAX > +#error In the code bitmap table physical size assumed to fit into int > +#endif > + > /* Bitmap directory entry flags */ > #define BME_RESERVED_FLAGS 0xfffffffcU > #define BME_FLAG_IN_USE 1 > @@ -74,6 +79,8 @@ typedef struct Qcow2Bitmap { > uint8_t granularity_bits; > char *name; > > + BdrvDirtyBitmap *dirty_bitmap; (I'm not quite happy with the asymmetry of this field (it isn't set by load_bitmap(), but it is required by store_bitmap()), but making it symmetric either by making load_bitmap() set it or by store_bitmap() not reading it (but getting the value through an explicit parameter) makes the code needlessly more complicated, so I guess I'll have to stay not-quite-happy.) > + > QSIMPLEQ_ENTRY(Qcow2Bitmap) entry; > } Qcow2Bitmap; > typedef QSIMPLEQ_HEAD(Qcow2BitmapList, Qcow2Bitmap) Qcow2BitmapList; > @@ -87,6 +94,27 @@ static inline bool can_write(BlockDriverState *bs) > return !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE); > } > > +static int update_header_sync(BlockDriverState *bs) > +{ > + int ret; > + > + ret = qcow2_update_header(bs); > + if (ret < 0) { > + return ret; > + } > + > + /* We doesn't return bdrv_flush error code. Even if it fails, write was s/doesn't/don't/ (Also, there's a double space after return.) > + * successful and it is more logical to consider that header is in the new > + * state than in the old. > + */ > + ret = bdrv_flush(bs); > + if (ret < 0) { > + fprintf(stderr, "Failed to flush qcow2 header"); > + } > + > + return 0; > +} > + > static inline void bitmap_table_to_cpu(uint64_t *bitmap_table, size_t size) > { > size_t i; > @@ -96,6 +124,15 @@ static inline void bitmap_table_to_cpu(uint64_t *bitmap_table, size_t size) > } > } > > +static inline void bitmap_table_to_be(uint64_t *bitmap_table, size_t size) > +{ > + size_t i; > + > + for (i = 0; i < size; ++i) { > + cpu_to_be64s(&bitmap_table[i]); > + } > +} > + > /* Check table entry specification constraints. If cluster_size is 0, offset > * alignment is not checked. */ > static int check_table_entry(uint64_t entry, int cluster_size) > @@ -121,6 +158,51 @@ static int check_table_entry(uint64_t entry, int cluster_size) > return 0; > } > > +static int check_constraints_on_bitmap(BlockDriverState *bs, > + const char *name, > + uint32_t granularity) > +{ > + BDRVQcow2State *s = bs->opaque; > + int granularity_bits = ctz32(granularity); > + > + int64_t nb_sectors = bdrv_nb_sectors(bs); > + > + if (nb_sectors < 0) { > + return nb_sectors; > + } > + > + uint64_t phys_bitmap_bytes = (nb_sectors << BDRV_SECTOR_BITS) >> Using bdrv_getlength() would be simpler. > + granularity_bits; > + uint64_t bitmap_table_size = phys_bitmap_bytes / s->cluster_size; Should be a DIV_ROUND_UP(). > + size_t name_size = strlen(name); > + > + int fail = I'd personally like a bool more. > + (bitmap_table_size > BME_MAX_TABLE_SIZE) || > + (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) || > + (granularity_bits > BME_MAX_GRANULARITY_BITS) || > + (granularity_bits < BME_MIN_GRANULARITY_BITS) || > + (name_size > BME_MAX_NAME_SIZE); > + > + return fail ? -EINVAL : 0; > +} > + > +static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table, > + uint32_t bitmap_table_size) > +{ > + BDRVQcow2State *s = bs->opaque; > + int i; > + > + for (i = 0; i < bitmap_table_size; ++i) { > + uint64_t addr = bitmap_table[i] & BME_TABLE_ENTRY_OFFSET_MASK; > + if (!addr) { > + continue; > + } > + > + qcow2_free_clusters(bs, addr, s->cluster_size, QCOW2_DISCARD_OTHER); > + bitmap_table[i] = 0; > + } > +} > + > static int bitmap_table_load(BlockDriverState *bs, Qcow2Bitmap *bm, > uint64_t **bitmap_table) > { > @@ -159,6 +241,28 @@ fail: > return ret; > } > > +static int free_bitmap_clusters(BlockDriverState *bs, Qcow2Bitmap *bm) > +{ > + int ret; > + uint64_t *bitmap_table; > + > + ret = bitmap_table_load(bs, bm, &bitmap_table); > + if (ret < 0) { > + assert(bitmap_table == NULL); > + return ret; > + } > + > + clear_bitmap_table(bs, bitmap_table, bm->table_size); > + qcow2_free_clusters(bs, bm->table_offset, bm->table_size * sizeof(uint64_t), > + QCOW2_DISCARD_OTHER); > + g_free(bitmap_table); > + > + bm->table_offset = 0; > + bm->table_size = 0; > + > + return 0; > +} > + > /* This function returns the number of disk sectors covered by a single cluster > * of bitmap data. */ > static uint64_t disk_sectors_in_bitmap_cluster(const BDRVQcow2State *s, > @@ -603,6 +707,72 @@ fail: > * Bitmap List end > */ > > +static int update_ext_header_and_dir(BlockDriverState *bs, > + Qcow2BitmapList *bm_list) > +{ > + BDRVQcow2State *s = bs->opaque; > + int ret; > + Qcow2Bitmap *bm; > + uint64_t new_offset = 0; > + uint64_t new_size = 0; > + uint32_t new_nb_bitmaps = 0; > + uint64_t old_offset = s->bitmap_directory_offset; > + uint64_t old_size = s->bitmap_directory_size; > + uint32_t old_nb_bitmaps = s->nb_bitmaps; > + uint64_t old_autocl = s->autoclear_features; > + > + if (bm_list != NULL && !QSIMPLEQ_EMPTY(bm_list)) { > + QSIMPLEQ_FOREACH(bm, bm_list, entry) { > + new_nb_bitmaps++; > + } > + > + if (new_nb_bitmaps > QCOW2_MAX_BITMAPS) { > + return -EINVAL; > + } > + > + ret = bitmap_list_store(bs, bm_list, &new_offset, &new_size, false); > + if (ret < 0) { > + return ret; > + } > + > + ret = bdrv_flush(bs); > + if (ret < 0) { > + goto fail; > + } I think this only needs to flush bs->file, not bs itself. Alternatively, bitmap_list_store() could just use bdrv_pwrite_sync(). > + > + s->autoclear_features |= QCOW2_AUTOCLEAR_BITMAPS; > + } else { > + s->autoclear_features &= ~(uint64_t)QCOW2_AUTOCLEAR_BITMAPS; > + } > + > + s->bitmap_directory_offset = new_offset; > + s->bitmap_directory_size = new_size; > + s->nb_bitmaps = new_nb_bitmaps; > + > + ret = update_header_sync(bs); > + if (ret < 0) { > + goto fail; > + } > + > + if (old_size > 0) { > + qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_OTHER); > + } > + > + return 0; > + > +fail: > + if (new_offset > 0) { > + qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_OTHER); > + } > + > + s->bitmap_directory_offset = old_offset; > + s->bitmap_directory_size = old_size; > + s->nb_bitmaps = old_nb_bitmaps; > + s->autoclear_features = old_autocl; > + > + return ret; > +} > + > /* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */ > static void release_dirty_bitmap_helper(gpointer bitmap, > gpointer bs) > @@ -664,3 +834,284 @@ fail: > g_slist_free(created_dirty_bitmaps); > bitmap_list_free(bm_list); > } > + > +/* store_bitmap_data() > + * Store bitmap to image, filling bitmap table accordingly. > + */ > +static uint64_t *store_bitmap_data(BlockDriverState *bs, > + BdrvDirtyBitmap *bitmap, > + uint32_t *bitmap_table_size, Error **errp) > +{ > + int ret; > + BDRVQcow2State *s = bs->opaque; > + int64_t sector; > + uint64_t dsc; > + uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap); > + const char *bm_name = bdrv_dirty_bitmap_name(bitmap); > + uint8_t *buf = NULL; > + BdrvDirtyBitmapIter *dbi; > + uint64_t *tb; > + uint64_t tb_size = > + size_to_clusters(s, > + bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size)); > + > + if (tb_size > BME_MAX_TABLE_SIZE || > + tb_size * s->cluster_size > BME_MAX_PHYS_SIZE) { Alignment to the opening parenthesis, please. > + error_setg(errp, "Bitmap '%s' is too big", bm_name); > + return NULL; > + } > + > + tb = g_try_new0(uint64_t, tb_size); > + if (tb == NULL) { > + error_setg(errp, "No memory"); > + return NULL; > + } > + > + dbi = bdrv_dirty_iter_new(bitmap, 0); > + buf = g_malloc(s->cluster_size); > + dsc = disk_sectors_in_bitmap_cluster(s, bitmap); > + > + while ((sector = bdrv_dirty_iter_next(dbi)) != -1) { > + uint64_t cluster = sector / dsc; > + uint64_t end, write_size; > + int64_t off; > + > + sector = cluster * dsc; > + end = MIN(bm_size, sector + dsc); > + write_size = > + bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector); > + > + off = qcow2_alloc_clusters(bs, s->cluster_size); > + if (off < 0) { > + error_setg_errno(errp, -off, > + "Failed to allocate clusters for bitmap '%s'", > + bm_name); > + goto fail; > + } > + tb[cluster] = off; Somehow I would feel better with either an assert(cluster < tb_size); here or an assert(bdrv_nb_sectors(bs) / dsc == tb_size); (plus the error handling for bdrv_nb_sectors()) above the loop. > + > + bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end - sector); > + if (write_size < s->cluster_size) { > + memset(buf + write_size, 0, s->cluster_size - write_size); > + } Should we assert that write_size <= s->cluster_size? > + > + ret = qcow2_pre_write_overlap_check(bs, 0, off, s->cluster_size); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Qcow2 overlap check failed"); > + goto fail; > + } > + > + ret = bdrv_pwrite(bs->file, off, buf, s->cluster_size); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Failed to write bitmap '%s' to file", > + bm_name); > + goto fail; > + } > + > + if (end >= bm_size) { > + break; > + } > + > + bdrv_set_dirty_iter(dbi, end); > + } > + > + *bitmap_table_size = tb_size; > + g_free(buf); > + bdrv_dirty_iter_free(dbi); > + > + return tb; > + > +fail: > + clear_bitmap_table(bs, tb, tb_size); > + g_free(buf); > + bdrv_dirty_iter_free(dbi); > + g_free(tb); > + > + return NULL; > +} > + > +/* store_bitmap() > + * Store bm->dirty_bitmap to qcow2. > + * Set bm->table_offset and bm->table_size accordingly. > + */ > +static int store_bitmap(BlockDriverState *bs, Qcow2Bitmap *bm, Error **errp) > +{ > + int ret; > + uint64_t *tb; > + int64_t tb_offset; > + uint32_t tb_size; > + BdrvDirtyBitmap *bitmap = bm->dirty_bitmap; > + const char *bm_name; > + > + assert(bitmap != NULL); > + > + bm_name = bdrv_dirty_bitmap_name(bitmap); > + > + tb = store_bitmap_data(bs, bitmap, &tb_size, errp); > + if (tb == NULL) { > + g_free(tb); > + return -EINVAL; > + } > + > + assert(tb_size <= BME_MAX_TABLE_SIZE); > + tb_offset = qcow2_alloc_clusters(bs, tb_size * sizeof(tb[0])); > + if (tb_offset < 0) { > + error_setg_errno(errp, -tb_offset, > + "Failed to allocate clusters for bitmap '%s'", > + bm_name); > + goto fail; > + } > + > + ret = qcow2_pre_write_overlap_check(bs, 0, tb_offset, > + tb_size * sizeof(tb[0])); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Qcow2 overlap check failed"); > + goto fail; > + } > + > + bitmap_table_to_be(tb, tb_size); > + ret = bdrv_pwrite(bs->file, tb_offset, tb, tb_size * sizeof(tb[0])); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Failed to write bitmap '%s' to file", > + bm_name); > + goto fail; > + } > + > + g_free(tb); > + > + bm->table_offset = tb_offset; > + bm->table_size = tb_size; > + > + return 0; > + > +fail: > + clear_bitmap_table(bs, tb, tb_size); > + > + if (tb_offset > 0) { > + qcow2_free_clusters(bs, tb_offset, tb_size, QCOW2_DISCARD_OTHER); tb_size should be tb_size * sizeof(uint64_t) (or sizeof(tb[0])). > + } > + > + g_free(tb); > + > + return ret; > +} > + > +static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list, > + const char *name) > +{ > + Qcow2Bitmap *bm; > + > + QSIMPLEQ_FOREACH(bm, bm_list, entry) { > + if (strcmp(name, bm->name) == 0) { > + return bm; > + } > + } > + > + return NULL; > +} > + > +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) > +{ > + BdrvDirtyBitmap *bitmap; > + BDRVQcow2State *s = bs->opaque; > + uint32_t new_nb_bitmaps = s->nb_bitmaps; > + uint64_t new_dir_size = s->bitmap_directory_size; > + int ret; > + Qcow2BitmapList *bm_list; > + Qcow2Bitmap *bm; > + > + if (!bdrv_has_persistent_bitmaps(bs)) { > + /* nothing to do */ > + return; Maybe there is something to do. See [1]. > + } > + > + if (s->nb_bitmaps == 0) { > + bm_list = bitmap_list_new(); > + } else { > + bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, > + s->bitmap_directory_size, errp); > + if (bm_list == NULL) { > + /* errp is already set */ > + return; > + } > + } > + > + /* check constraints and names */ > + for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL; > + bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) { Alignment to the opening parenthesis, please. > + const char *name = bdrv_dirty_bitmap_name(bitmap); > + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); > + Qcow2Bitmap *bm; > + > + if (!bdrv_dirty_bitmap_get_persistance(bitmap)) { > + continue; > + } > + > + if (++new_nb_bitmaps > QCOW2_MAX_BITMAPS) { > + error_setg(errp, "Too many persistent bitmaps"); > + goto fail; > + } > + > + new_dir_size += calc_dir_entry_size(strlen(name), 0); > + if (new_dir_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) { > + error_setg(errp, "Too large bitmap directory"); > + goto fail; > + } You only need to increment new_nb_bitmaps and increase new_dir_size if the bitmap does not already exist in the image (i.e. if find_bitmap_by_name() below returns NULL). > + > + if (check_constraints_on_bitmap(bs, name, granularity) < 0) { > + error_setg(errp, "Bitmap '%s' doesn't satisfy the constraints", > + name); > + goto fail; > + } > + > + bm = find_bitmap_by_name(bm_list, name); > + if (bm == NULL) { > + bm = g_new0(Qcow2Bitmap, 1); > + bm->name = g_strdup(name); > + QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry); > + } else { > + if (!(bm->flags & BME_FLAG_IN_USE) && can_write(bs)) { Shouldn't we error out right at the beginning of this function if can_write(bs) is false? > + error_setg(errp, "Bitmap '%s' is already exists in the image", > + name); > + goto fail; > + } > + free_bitmap_clusters(bs, bm); These clusters should not be freed until the new data has been written successfully. > + /* TODO it is better to reuse these clusters */ > + } > + bm->flags = bdrv_dirty_bitmap_get_autoload(bitmap) ? BME_FLAG_AUTO : 0; > + bm->granularity_bits = ctz32(bdrv_dirty_bitmap_granularity(bitmap)); > + bm->dirty_bitmap = bitmap; > + } [1] What about bitmaps that have BME_FLAG_IN_USE set but do not have a corresponding BDS bitmap? If such a bitmap does not have BME_FLAG_AUTO set, we didn't set the flag, so we should keep it unchanged. That's what this function is currently doing. However, if such a bitmap does have BME_FLAG_AUTO set, it was definitely us who set the IN_USE flag (because otherwise we would have aborted loading the bitmaps, and thus also aborted bdrv_open_common()). Therefore, the only explanation is that the bitmap was deleted in the meantime, and that means we should also delete it in the qcow2 file. Max > + > + /* allocate clusters and store bitmaps */ > + QSIMPLEQ_FOREACH(bm, bm_list, entry) { > + if (bm->dirty_bitmap == NULL) { > + continue; > + } > + > + ret = store_bitmap(bs, bm, errp); > + if (ret < 0) { > + goto fail; > + } > + } > + > + ret = update_ext_header_and_dir(bs, bm_list); > + if (ret < 0) { > + error_setg_errno(errp, -ret, "Failed to update bitmap extension"); > + goto fail; > + } > + > + bitmap_list_free(bm_list); > + return; > + > +fail: > + QSIMPLEQ_FOREACH(bm, bm_list, entry) { > + if (bm->dirty_bitmap == NULL || bm->table_offset == 0) { > + continue; > + } > + > + free_bitmap_clusters(bs, bm); > + } > + > + bitmap_list_free(bm_list); > +} > diff --git a/block/qcow2.c b/block/qcow2.c > index 443be0e..66c7f74 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -3515,6 +3515,7 @@ BlockDriver bdrv_qcow2 = { > .bdrv_attach_aio_context = qcow2_attach_aio_context, > > .bdrv_load_autoloading_dirty_bitmaps = qcow2_load_autoloading_dirty_bitmaps, > + .bdrv_store_persistent_dirty_bitmaps = qcow2_store_persistent_dirty_bitmaps, > }; > > static void bdrv_qcow2_init(void) > diff --git a/block/qcow2.h b/block/qcow2.h > index bcedf5b..d9a7643 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -615,5 +615,6 @@ void qcow2_cache_put(BlockDriverState *bs, Qcow2Cache *c, void **table); > > /* qcow2-bitmap.c functions */ > void qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp); > +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); > > #endif >
09.12.2016 20:05, Max Reitz wrote: > On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote: >> Realize block bitmap storing interface, to allow qcow2 images store >> persistent bitmaps. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/qcow2-bitmap.c | 451 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> block/qcow2.c | 1 + >> block/qcow2.h | 1 + >> 3 files changed, 453 insertions(+) >> >> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >> index 81be1ca..a975388 100644 >> --- a/block/qcow2-bitmap.c [...] >> + return; >> + } >> + } >> + >> + /* check constraints and names */ >> + for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL; >> + bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) { > Alignment to the opening parenthesis, please. Hmm.. without an alignment it is not so simple to distinguish for-loop header from its body. [...] > [1] What about bitmaps that have BME_FLAG_IN_USE set but do not have a > corresponding BDS bitmap? > > If such a bitmap does not have BME_FLAG_AUTO set, we didn't set the > flag, so we should keep it unchanged. That's what this function is > currently doing. > > However, if such a bitmap does have BME_FLAG_AUTO set, it was definitely > us who set the IN_USE flag (because otherwise we would have aborted > loading the bitmaps, and thus also aborted bdrv_open_common()). > Therefore, the only explanation is that the bitmap was deleted in the > meantime, and that means we should also delete it in the qcow2 file. Right. Or, alternatively, these bitmaps may be deleted on corresponding BdrvDirtyBitmap deletion.
On 09.12.2016 18:55, Vladimir Sementsov-Ogievskiy wrote: > 09.12.2016 20:05, Max Reitz wrote: >> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote: >>> Realize block bitmap storing interface, to allow qcow2 images store >>> persistent bitmaps. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> block/qcow2-bitmap.c | 451 >>> +++++++++++++++++++++++++++++++++++++++++++++++++++ >>> block/qcow2.c | 1 + >>> block/qcow2.h | 1 + >>> 3 files changed, 453 insertions(+) >>> >>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >>> index 81be1ca..a975388 100644 >>> --- a/block/qcow2-bitmap.c > > [...] > >>> + return; >>> + } >>> + } >>> + >>> + /* check constraints and names */ >>> + for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL; >>> + bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) { >> Alignment to the opening parenthesis, please. > > Hmm.. without an alignment it is not so simple to distinguish for-loop > header from its body. I know, and it's even worse for "if". That is why I usually put the opening { on a new line if I have to spread an if/while/for header over multiple lines. The usual convention for qemu code is to align at an opening parenthesis if there is one. Admittedly, the reasoning I gave for changing checkpatch.pl to accept opening { on a new line in certain cases was that: (1) We never codified exactly what to allow for multi-line if/while/for conditions. (2) It was existing practice. (1) applies in your case also; we don't have any explicitly written-out convention for alignment of wrapped lines. (2) is more difficult, but there are indeed a handful of cases where lines are wrapped and not aligned to the opening parenthesis but just indented by an additional four spaces... So I guess since I'm insisting on putting the opening { on a new line for multi-line conditions, you are allowed to indent the consecutive lines by an additional level. ;-) (It *is* against existing convention, but I'm not in a position to argue.) > [...] > >> [1] What about bitmaps that have BME_FLAG_IN_USE set but do not have a >> corresponding BDS bitmap? >> >> If such a bitmap does not have BME_FLAG_AUTO set, we didn't set the >> flag, so we should keep it unchanged. That's what this function is >> currently doing. >> >> However, if such a bitmap does have BME_FLAG_AUTO set, it was definitely >> us who set the IN_USE flag (because otherwise we would have aborted >> loading the bitmaps, and thus also aborted bdrv_open_common()). >> Therefore, the only explanation is that the bitmap was deleted in the >> meantime, and that means we should also delete it in the qcow2 file. > > Right. Or, alternatively, these bitmaps may be deleted on corresponding > BdrvDirtyBitmap deletion. Right, that would work, too. Max
10.12.2016 17:53, Max Reitz wrote: > On 09.12.2016 18:55, Vladimir Sementsov-Ogievskiy wrote: >> 09.12.2016 20:05, Max Reitz wrote: >>> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote: >>>> Realize block bitmap storing interface, to allow qcow2 images store >>>> persistent bitmaps. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> block/qcow2-bitmap.c | 451 >>>> +++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> block/qcow2.c | 1 + >>>> block/qcow2.h | 1 + >>>> 3 files changed, 453 insertions(+) >>>> >>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >>>> index 81be1ca..a975388 100644 >>>> --- a/block/qcow2-bitmap.c >> [...] >> >>>> + return; >>>> + } >>>> + } >>>> + >>>> + /* check constraints and names */ >>>> + for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL; >>>> + bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) { >>> Alignment to the opening parenthesis, please. >> Hmm.. without an alignment it is not so simple to distinguish for-loop >> header from its body. > I know, and it's even worse for "if". That is why I usually put the > opening { on a new line if I have to spread an if/while/for header over > multiple lines. > > The usual convention for qemu code is to align at an opening parenthesis > if there is one. > > Admittedly, the reasoning I gave for changing checkpatch.pl to accept > opening { on a new line in certain cases was that: Good news, didn't know) > > (1) We never codified exactly what to allow for multi-line if/while/for > conditions. > (2) It was existing practice. > > (1) applies in your case also; we don't have any explicitly written-out > convention for alignment of wrapped lines. (2) is more difficult, but > there are indeed a handful of cases where lines are wrapped and not > aligned to the opening parenthesis but just indented by an additional > four spaces... > > So I guess since I'm insisting on putting the opening { on a new line > for multi-line conditions, you are allowed to indent the consecutive > lines by an additional level. ;-) > > (It *is* against existing convention, but I'm not in a position to argue.) > >> [...] >> >>> [1] What about bitmaps that have BME_FLAG_IN_USE set but do not have a >>> corresponding BDS bitmap? >>> >>> If such a bitmap does not have BME_FLAG_AUTO set, we didn't set the >>> flag, so we should keep it unchanged. That's what this function is >>> currently doing. >>> >>> However, if such a bitmap does have BME_FLAG_AUTO set, it was definitely >>> us who set the IN_USE flag (because otherwise we would have aborted >>> loading the bitmaps, and thus also aborted bdrv_open_common()). >>> Therefore, the only explanation is that the bitmap was deleted in the >>> meantime, and that means we should also delete it in the qcow2 file. >> Right. Or, alternatively, these bitmaps may be deleted on corresponding >> BdrvDirtyBitmap deletion. > Right, that would work, too. > > Max >
09.12.2016 20:05, Max Reitz wrote: > On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote: >> Realize block bitmap storing interface, to allow qcow2 images store >> persistent bitmaps. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com> >> --- >> block/qcow2-bitmap.c | 451 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> block/qcow2.c | 1 + [...] >> + >> +/* store_bitmap_data() >> + * Store bitmap to image, filling bitmap table accordingly. >> + */ >> +static uint64_t *store_bitmap_data(BlockDriverState *bs, >> + BdrvDirtyBitmap *bitmap, >> + uint32_t *bitmap_table_size, Error **errp) >> +{ >> + int ret; >> + BDRVQcow2State *s = bs->opaque; >> + int64_t sector; >> + uint64_t dsc; >> + uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap); >> + const char *bm_name = bdrv_dirty_bitmap_name(bitmap); >> + uint8_t *buf = NULL; >> + BdrvDirtyBitmapIter *dbi; >> + uint64_t *tb; >> + uint64_t tb_size = >> + size_to_clusters(s, >> + bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size)); >> + >> + if (tb_size > BME_MAX_TABLE_SIZE || >> + tb_size * s->cluster_size > BME_MAX_PHYS_SIZE) { > Alignment to the opening parenthesis, please. > >> + error_setg(errp, "Bitmap '%s' is too big", bm_name); >> + return NULL; >> + } >> + >> + tb = g_try_new0(uint64_t, tb_size); >> + if (tb == NULL) { >> + error_setg(errp, "No memory"); >> + return NULL; >> + } >> + >> + dbi = bdrv_dirty_iter_new(bitmap, 0); >> + buf = g_malloc(s->cluster_size); >> + dsc = disk_sectors_in_bitmap_cluster(s, bitmap); >> + >> + while ((sector = bdrv_dirty_iter_next(dbi)) != -1) { >> + uint64_t cluster = sector / dsc; >> + uint64_t end, write_size; >> + int64_t off; >> + >> + sector = cluster * dsc; >> + end = MIN(bm_size, sector + dsc); >> + write_size = >> + bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector); >> + >> + off = qcow2_alloc_clusters(bs, s->cluster_size); >> + if (off < 0) { >> + error_setg_errno(errp, -off, >> + "Failed to allocate clusters for bitmap '%s'", >> + bm_name); >> + goto fail; >> + } >> + tb[cluster] = off; > Somehow I would feel better with either an assert(cluster < tb_size); > here or an assert(bdrv_nb_sectors(bs) / dsc == tb_size); (plus the error > handling for bdrv_nb_sectors()) above the loop. assert((bm_size - 1) / dsc == tb_size - 1) seems ok. and no additional error handling. Right? >> + >> + bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end - sector); >> + if (write_size < s->cluster_size) { >> + memset(buf + write_size, 0, s->cluster_size - write_size); >> + } > Should we assert that write_size <= s->cluster_size? Ok [...]. > >> + const char *name = bdrv_dirty_bitmap_name(bitmap); >> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); >> + Qcow2Bitmap *bm; >> + >> + if (!bdrv_dirty_bitmap_get_persistance(bitmap)) { >> + continue; >> + } >> + >> + if (++new_nb_bitmaps > QCOW2_MAX_BITMAPS) { >> + error_setg(errp, "Too many persistent bitmaps"); >> + goto fail; >> + } >> + >> + new_dir_size += calc_dir_entry_size(strlen(name), 0); >> + if (new_dir_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) { >> + error_setg(errp, "Too large bitmap directory"); >> + goto fail; >> + } > You only need to increment new_nb_bitmaps and increase new_dir_size if > the bitmap does not already exist in the image (i.e. if > find_bitmap_by_name() below returns NULL). Why? No, I need to check the whole sum and the whole size. > >> + >> + if (check_constraints_on_bitmap(bs, name, granularity) < 0) { >> + error_setg(errp, "Bitmap '%s' doesn't satisfy the constraints", >> + name); >> + goto fail; >> + } >> + >> + bm = find_bitmap_by_name(bm_list, name); >> + if (bm == NULL) { >> + bm = g_new0(Qcow2Bitmap, 1); >> + bm->name = g_strdup(name); >> + QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry); >> + } else { >> + if (!(bm->flags & BME_FLAG_IN_USE) && can_write(bs)) { > Shouldn't we error out right at the beginning of this function if > can_write(bs) is false? Hmm, right. [...]
On 17.12.2016 15:58, Vladimir Sementsov-Ogievskiy wrote: > 09.12.2016 20:05, Max Reitz wrote: >> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote: >>> Realize block bitmap storing interface, to allow qcow2 images store >>> persistent bitmaps. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> block/qcow2-bitmap.c | 451 +++++++++++++++++++++++++++++++++++++++++++++++++++ >>> block/qcow2.c | 1 + > > [...] > >>> + >>> +/* store_bitmap_data() >>> + * Store bitmap to image, filling bitmap table accordingly. >>> + */ >>> +static uint64_t *store_bitmap_data(BlockDriverState *bs, >>> + BdrvDirtyBitmap *bitmap, >>> + uint32_t *bitmap_table_size, Error **errp) >>> +{ >>> + int ret; >>> + BDRVQcow2State *s = bs->opaque; >>> + int64_t sector; >>> + uint64_t dsc; >>> + uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap); >>> + const char *bm_name = bdrv_dirty_bitmap_name(bitmap); >>> + uint8_t *buf = NULL; >>> + BdrvDirtyBitmapIter *dbi; >>> + uint64_t *tb; >>> + uint64_t tb_size = >>> + size_to_clusters(s, >>> + bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size)); >>> + >>> + if (tb_size > BME_MAX_TABLE_SIZE || >>> + tb_size * s->cluster_size > BME_MAX_PHYS_SIZE) { >> Alignment to the opening parenthesis, please. >> >>> + error_setg(errp, "Bitmap '%s' is too big", bm_name); >>> + return NULL; >>> + } >>> + >>> + tb = g_try_new0(uint64_t, tb_size); >>> + if (tb == NULL) { >>> + error_setg(errp, "No memory"); >>> + return NULL; >>> + } >>> + >>> + dbi = bdrv_dirty_iter_new(bitmap, 0); >>> + buf = g_malloc(s->cluster_size); >>> + dsc = disk_sectors_in_bitmap_cluster(s, bitmap); >>> + >>> + while ((sector = bdrv_dirty_iter_next(dbi)) != -1) { >>> + uint64_t cluster = sector / dsc; >>> + uint64_t end, write_size; >>> + int64_t off; >>> + >>> + sector = cluster * dsc; >>> + end = MIN(bm_size, sector + dsc); >>> + write_size = >>> + bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector); >>> + >>> + off = qcow2_alloc_clusters(bs, s->cluster_size); >>> + if (off < 0) { >>> + error_setg_errno(errp, -off, >>> + "Failed to allocate clusters for bitmap '%s'", >>> + bm_name); >>> + goto fail; >>> + } >>> + tb[cluster] = off; >> Somehow I would feel better with either an assert(cluster < tb_size); >> here or an assert(bdrv_nb_sectors(bs) / dsc == tb_size); (plus the error >> handling for bdrv_nb_sectors()) above the loop. > > assert((bm_size - 1) / dsc == tb_size - 1) seems ok. and no additional > error handling. Right? Right, bm_size is already equal to bdrv_nb_sectors(bs), and it's not necessarily a multiple of dsc. So that should be good. Alternatively, I think the following would be slightly easier to read: assert(DIV_ROUND_UP(bm_size, dsc) == tb_size); > >>> + >>> + bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end - sector); >>> + if (write_size < s->cluster_size) { >>> + memset(buf + write_size, 0, s->cluster_size - write_size); >>> + } >> Should we assert that write_size <= s->cluster_size? > > Ok > > [...]. > >> >>> + const char *name = bdrv_dirty_bitmap_name(bitmap); >>> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); >>> + Qcow2Bitmap *bm; >>> + >>> + if (!bdrv_dirty_bitmap_get_persistance(bitmap)) { >>> + continue; >>> + } >>> + >>> + if (++new_nb_bitmaps > QCOW2_MAX_BITMAPS) { >>> + error_setg(errp, "Too many persistent bitmaps"); >>> + goto fail; >>> + } >>> + >>> + new_dir_size += calc_dir_entry_size(strlen(name), 0); >>> + if (new_dir_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) { >>> + error_setg(errp, "Too large bitmap directory"); >>> + goto fail; >>> + } >> You only need to increment new_nb_bitmaps and increase new_dir_size if >> the bitmap does not already exist in the image (i.e. if >> find_bitmap_by_name() below returns NULL). > > Why? No, I need to check the whole sum and the whole size. If the bitmap already exists, you don't create a new directory entry but reuse the existing one. Therefore, the number of bitmaps in the image and the directory size will not grow then. Max >>> + >>> + if (check_constraints_on_bitmap(bs, name, granularity) < 0) { >>> + error_setg(errp, "Bitmap '%s' doesn't satisfy the constraints", >>> + name); >>> + goto fail; >>> + } >>> + >>> + bm = find_bitmap_by_name(bm_list, name); >>> + if (bm == NULL) { >>> + bm = g_new0(Qcow2Bitmap, 1); >>> + bm->name = g_strdup(name); >>> + QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry); >>> + } else { >>> + if (!(bm->flags & BME_FLAG_IN_USE) && can_write(bs)) { >> Shouldn't we error out right at the beginning of this function if >> can_write(bs) is false? > > Hmm, right. > > [...] > > -- > Best regards, > Vladimir >
19.12.2016 18:14, Max Reitz wrote: > On 17.12.2016 15:58, Vladimir Sementsov-Ogievskiy wrote: >> 09.12.2016 20:05, Max Reitz wrote: >>> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote: >>>> Realize block bitmap storing interface, to allow qcow2 images store >>>> persistent bitmaps. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> block/qcow2-bitmap.c | 451 +++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> block/qcow2.c | 1 + >> [...] >> >>>> + >>>> +/* store_bitmap_data() >>>> + * Store bitmap to image, filling bitmap table accordingly. >>>> + */ >>>> +static uint64_t *store_bitmap_data(BlockDriverState *bs, >>>> + BdrvDirtyBitmap *bitmap, >>>> + uint32_t *bitmap_table_size, Error **errp) >>>> +{ >>>> + int ret; >>>> + BDRVQcow2State *s = bs->opaque; >>>> + int64_t sector; >>>> + uint64_t dsc; >>>> + uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap); >>>> + const char *bm_name = bdrv_dirty_bitmap_name(bitmap); >>>> + uint8_t *buf = NULL; >>>> + BdrvDirtyBitmapIter *dbi; >>>> + uint64_t *tb; >>>> + uint64_t tb_size = >>>> + size_to_clusters(s, >>>> + bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size)); >>>> + >>>> + if (tb_size > BME_MAX_TABLE_SIZE || >>>> + tb_size * s->cluster_size > BME_MAX_PHYS_SIZE) { >>> Alignment to the opening parenthesis, please. >>> >>>> + error_setg(errp, "Bitmap '%s' is too big", bm_name); >>>> + return NULL; >>>> + } >>>> + >>>> + tb = g_try_new0(uint64_t, tb_size); >>>> + if (tb == NULL) { >>>> + error_setg(errp, "No memory"); >>>> + return NULL; >>>> + } >>>> + >>>> + dbi = bdrv_dirty_iter_new(bitmap, 0); >>>> + buf = g_malloc(s->cluster_size); >>>> + dsc = disk_sectors_in_bitmap_cluster(s, bitmap); >>>> + >>>> + while ((sector = bdrv_dirty_iter_next(dbi)) != -1) { >>>> + uint64_t cluster = sector / dsc; >>>> + uint64_t end, write_size; >>>> + int64_t off; >>>> + >>>> + sector = cluster * dsc; >>>> + end = MIN(bm_size, sector + dsc); >>>> + write_size = >>>> + bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector); >>>> + >>>> + off = qcow2_alloc_clusters(bs, s->cluster_size); >>>> + if (off < 0) { >>>> + error_setg_errno(errp, -off, >>>> + "Failed to allocate clusters for bitmap '%s'", >>>> + bm_name); >>>> + goto fail; >>>> + } >>>> + tb[cluster] = off; >>> Somehow I would feel better with either an assert(cluster < tb_size); >>> here or an assert(bdrv_nb_sectors(bs) / dsc == tb_size); (plus the error >>> handling for bdrv_nb_sectors()) above the loop. >> assert((bm_size - 1) / dsc == tb_size - 1) seems ok. and no additional >> error handling. Right? > Right, bm_size is already equal to bdrv_nb_sectors(bs), and it's not > necessarily a multiple of dsc. So that should be good. Alternatively, I > think the following would be slightly easier to read: > > assert(DIV_ROUND_UP(bm_size, dsc) == tb_size); > >>>> + >>>> + bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end - sector); >>>> + if (write_size < s->cluster_size) { >>>> + memset(buf + write_size, 0, s->cluster_size - write_size); >>>> + } >>> Should we assert that write_size <= s->cluster_size? >> Ok >> >> [...]. >> >>>> + const char *name = bdrv_dirty_bitmap_name(bitmap); >>>> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); >>>> + Qcow2Bitmap *bm; >>>> + >>>> + if (!bdrv_dirty_bitmap_get_persistance(bitmap)) { >>>> + continue; >>>> + } >>>> + >>>> + if (++new_nb_bitmaps > QCOW2_MAX_BITMAPS) { >>>> + error_setg(errp, "Too many persistent bitmaps"); >>>> + goto fail; >>>> + } >>>> + >>>> + new_dir_size += calc_dir_entry_size(strlen(name), 0); >>>> + if (new_dir_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) { >>>> + error_setg(errp, "Too large bitmap directory"); >>>> + goto fail; >>>> + } >>> You only need to increment new_nb_bitmaps and increase new_dir_size if >>> the bitmap does not already exist in the image (i.e. if >>> find_bitmap_by_name() below returns NULL). >> Why? No, I need to check the whole sum and the whole size. > If the bitmap already exists, you don't create a new directory entry but > reuse the existing one. Therefore, the number of bitmaps in the image > and the directory size will not grow then. new_nb_bitmaps is not number of "newly created bitmaps", but just new value of field nb_bitmaps, so, all bitmaps - old and new are calculated into new_nb_bitmaps. Anyway, this misunderstanding shows that variable name is bad.. > > Max > >>>> + >>>> + if (check_constraints_on_bitmap(bs, name, granularity) < 0) { >>>> + error_setg(errp, "Bitmap '%s' doesn't satisfy the constraints", >>>> + name); >>>> + goto fail; >>>> + } >>>> + >>>> + bm = find_bitmap_by_name(bm_list, name); >>>> + if (bm == NULL) { >>>> + bm = g_new0(Qcow2Bitmap, 1); >>>> + bm->name = g_strdup(name); >>>> + QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry); >>>> + } else { >>>> + if (!(bm->flags & BME_FLAG_IN_USE) && can_write(bs)) { >>> Shouldn't we error out right at the beginning of this function if >>> can_write(bs) is false? >> Hmm, right. >> >> [...] >> >> -- >> Best regards, >> Vladimir >> >
On 19.12.2016 16:26, Vladimir Sementsov-Ogievskiy wrote: > 19.12.2016 18:14, Max Reitz wrote: >> On 17.12.2016 15:58, Vladimir Sementsov-Ogievskiy wrote: >>> 09.12.2016 20:05, Max Reitz wrote: >>>> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote: >>>>> Realize block bitmap storing interface, to allow qcow2 images store >>>>> persistent bitmaps. >>>>> >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>>> --- >>>>> block/qcow2-bitmap.c | 451 >>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>> block/qcow2.c | 1 + >>> [...] >>> >>>>> + >>>>> +/* store_bitmap_data() >>>>> + * Store bitmap to image, filling bitmap table accordingly. >>>>> + */ >>>>> +static uint64_t *store_bitmap_data(BlockDriverState *bs, >>>>> + BdrvDirtyBitmap *bitmap, >>>>> + uint32_t *bitmap_table_size, >>>>> Error **errp) >>>>> +{ >>>>> + int ret; >>>>> + BDRVQcow2State *s = bs->opaque; >>>>> + int64_t sector; >>>>> + uint64_t dsc; >>>>> + uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap); >>>>> + const char *bm_name = bdrv_dirty_bitmap_name(bitmap); >>>>> + uint8_t *buf = NULL; >>>>> + BdrvDirtyBitmapIter *dbi; >>>>> + uint64_t *tb; >>>>> + uint64_t tb_size = >>>>> + size_to_clusters(s, >>>>> + bdrv_dirty_bitmap_serialization_size(bitmap, 0, >>>>> bm_size)); >>>>> + >>>>> + if (tb_size > BME_MAX_TABLE_SIZE || >>>>> + tb_size * s->cluster_size > BME_MAX_PHYS_SIZE) { >>>> Alignment to the opening parenthesis, please. >>>> >>>>> + error_setg(errp, "Bitmap '%s' is too big", bm_name); >>>>> + return NULL; >>>>> + } >>>>> + >>>>> + tb = g_try_new0(uint64_t, tb_size); >>>>> + if (tb == NULL) { >>>>> + error_setg(errp, "No memory"); >>>>> + return NULL; >>>>> + } >>>>> + >>>>> + dbi = bdrv_dirty_iter_new(bitmap, 0); >>>>> + buf = g_malloc(s->cluster_size); >>>>> + dsc = disk_sectors_in_bitmap_cluster(s, bitmap); >>>>> + >>>>> + while ((sector = bdrv_dirty_iter_next(dbi)) != -1) { >>>>> + uint64_t cluster = sector / dsc; >>>>> + uint64_t end, write_size; >>>>> + int64_t off; >>>>> + >>>>> + sector = cluster * dsc; >>>>> + end = MIN(bm_size, sector + dsc); >>>>> + write_size = >>>>> + bdrv_dirty_bitmap_serialization_size(bitmap, sector, >>>>> end - sector); >>>>> + >>>>> + off = qcow2_alloc_clusters(bs, s->cluster_size); >>>>> + if (off < 0) { >>>>> + error_setg_errno(errp, -off, >>>>> + "Failed to allocate clusters for >>>>> bitmap '%s'", >>>>> + bm_name); >>>>> + goto fail; >>>>> + } >>>>> + tb[cluster] = off; >>>> Somehow I would feel better with either an assert(cluster < tb_size); >>>> here or an assert(bdrv_nb_sectors(bs) / dsc == tb_size); (plus the >>>> error >>>> handling for bdrv_nb_sectors()) above the loop. >>> assert((bm_size - 1) / dsc == tb_size - 1) seems ok. and no additional >>> error handling. Right? >> Right, bm_size is already equal to bdrv_nb_sectors(bs), and it's not >> necessarily a multiple of dsc. So that should be good. Alternatively, I >> think the following would be slightly easier to read: >> >> assert(DIV_ROUND_UP(bm_size, dsc) == tb_size); >> >>>>> + >>>>> + bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end >>>>> - sector); >>>>> + if (write_size < s->cluster_size) { >>>>> + memset(buf + write_size, 0, s->cluster_size - >>>>> write_size); >>>>> + } >>>> Should we assert that write_size <= s->cluster_size? >>> Ok >>> >>> [...]. >>> >>>>> + const char *name = bdrv_dirty_bitmap_name(bitmap); >>>>> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); >>>>> + Qcow2Bitmap *bm; >>>>> + >>>>> + if (!bdrv_dirty_bitmap_get_persistance(bitmap)) { >>>>> + continue; >>>>> + } >>>>> + >>>>> + if (++new_nb_bitmaps > QCOW2_MAX_BITMAPS) { >>>>> + error_setg(errp, "Too many persistent bitmaps"); >>>>> + goto fail; >>>>> + } >>>>> + >>>>> + new_dir_size += calc_dir_entry_size(strlen(name), 0); >>>>> + if (new_dir_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) { >>>>> + error_setg(errp, "Too large bitmap directory"); >>>>> + goto fail; >>>>> + } >>>> You only need to increment new_nb_bitmaps and increase new_dir_size if >>>> the bitmap does not already exist in the image (i.e. if >>>> find_bitmap_by_name() below returns NULL). >>> Why? No, I need to check the whole sum and the whole size. >> If the bitmap already exists, you don't create a new directory entry but >> reuse the existing one. Therefore, the number of bitmaps in the image >> and the directory size will not grow then. > > new_nb_bitmaps is not number of "newly created bitmaps", but just new > value of field nb_bitmaps, so, all bitmaps - old and new are calculated > into new_nb_bitmaps. Anyway, this misunderstanding shows that variable > name is bad.. Yes. But when you store a bitmap of the same name as an existing one, you are replacing it. The number of bitmaps does not grow in that case. Max
19.12.2016 18:34, Max Reitz wrote: > On 19.12.2016 16:26, Vladimir Sementsov-Ogievskiy wrote: >> 19.12.2016 18:14, Max Reitz wrote: >>> On 17.12.2016 15:58, Vladimir Sementsov-Ogievskiy wrote: >>>> 09.12.2016 20:05, Max Reitz wrote: >>>>> On 22.11.2016 18:26, Vladimir Sementsov-Ogievskiy wrote: >>>>>> Realize block bitmap storing interface, to allow qcow2 images store >>>>>> persistent bitmaps. >>>>>> >>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>>>> --- >>>>>> block/qcow2-bitmap.c | 451 >>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>> block/qcow2.c | 1 + >>>> [...] >>>> >>>>>> + >>>>>> +/* store_bitmap_data() >>>>>> + * Store bitmap to image, filling bitmap table accordingly. >>>>>> + */ >>>>>> +static uint64_t *store_bitmap_data(BlockDriverState *bs, >>>>>> + BdrvDirtyBitmap *bitmap, >>>>>> + uint32_t *bitmap_table_size, >>>>>> Error **errp) >>>>>> +{ >>>>>> + int ret; >>>>>> + BDRVQcow2State *s = bs->opaque; >>>>>> + int64_t sector; >>>>>> + uint64_t dsc; >>>>>> + uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap); >>>>>> + const char *bm_name = bdrv_dirty_bitmap_name(bitmap); >>>>>> + uint8_t *buf = NULL; >>>>>> + BdrvDirtyBitmapIter *dbi; >>>>>> + uint64_t *tb; >>>>>> + uint64_t tb_size = >>>>>> + size_to_clusters(s, >>>>>> + bdrv_dirty_bitmap_serialization_size(bitmap, 0, >>>>>> bm_size)); >>>>>> + >>>>>> + if (tb_size > BME_MAX_TABLE_SIZE || >>>>>> + tb_size * s->cluster_size > BME_MAX_PHYS_SIZE) { >>>>> Alignment to the opening parenthesis, please. >>>>> >>>>>> + error_setg(errp, "Bitmap '%s' is too big", bm_name); >>>>>> + return NULL; >>>>>> + } >>>>>> + >>>>>> + tb = g_try_new0(uint64_t, tb_size); >>>>>> + if (tb == NULL) { >>>>>> + error_setg(errp, "No memory"); >>>>>> + return NULL; >>>>>> + } >>>>>> + >>>>>> + dbi = bdrv_dirty_iter_new(bitmap, 0); >>>>>> + buf = g_malloc(s->cluster_size); >>>>>> + dsc = disk_sectors_in_bitmap_cluster(s, bitmap); >>>>>> + >>>>>> + while ((sector = bdrv_dirty_iter_next(dbi)) != -1) { >>>>>> + uint64_t cluster = sector / dsc; >>>>>> + uint64_t end, write_size; >>>>>> + int64_t off; >>>>>> + >>>>>> + sector = cluster * dsc; >>>>>> + end = MIN(bm_size, sector + dsc); >>>>>> + write_size = >>>>>> + bdrv_dirty_bitmap_serialization_size(bitmap, sector, >>>>>> end - sector); >>>>>> + >>>>>> + off = qcow2_alloc_clusters(bs, s->cluster_size); >>>>>> + if (off < 0) { >>>>>> + error_setg_errno(errp, -off, >>>>>> + "Failed to allocate clusters for >>>>>> bitmap '%s'", >>>>>> + bm_name); >>>>>> + goto fail; >>>>>> + } >>>>>> + tb[cluster] = off; >>>>> Somehow I would feel better with either an assert(cluster < tb_size); >>>>> here or an assert(bdrv_nb_sectors(bs) / dsc == tb_size); (plus the >>>>> error >>>>> handling for bdrv_nb_sectors()) above the loop. >>>> assert((bm_size - 1) / dsc == tb_size - 1) seems ok. and no additional >>>> error handling. Right? >>> Right, bm_size is already equal to bdrv_nb_sectors(bs), and it's not >>> necessarily a multiple of dsc. So that should be good. Alternatively, I >>> think the following would be slightly easier to read: >>> >>> assert(DIV_ROUND_UP(bm_size, dsc) == tb_size); >>> >>>>>> + >>>>>> + bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end >>>>>> - sector); >>>>>> + if (write_size < s->cluster_size) { >>>>>> + memset(buf + write_size, 0, s->cluster_size - >>>>>> write_size); >>>>>> + } >>>>> Should we assert that write_size <= s->cluster_size? >>>> Ok >>>> >>>> [...]. >>>> >>>>>> + const char *name = bdrv_dirty_bitmap_name(bitmap); >>>>>> + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); >>>>>> + Qcow2Bitmap *bm; >>>>>> + >>>>>> + if (!bdrv_dirty_bitmap_get_persistance(bitmap)) { >>>>>> + continue; >>>>>> + } >>>>>> + >>>>>> + if (++new_nb_bitmaps > QCOW2_MAX_BITMAPS) { >>>>>> + error_setg(errp, "Too many persistent bitmaps"); >>>>>> + goto fail; >>>>>> + } >>>>>> + >>>>>> + new_dir_size += calc_dir_entry_size(strlen(name), 0); >>>>>> + if (new_dir_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) { >>>>>> + error_setg(errp, "Too large bitmap directory"); >>>>>> + goto fail; >>>>>> + } >>>>> You only need to increment new_nb_bitmaps and increase new_dir_size if >>>>> the bitmap does not already exist in the image (i.e. if >>>>> find_bitmap_by_name() below returns NULL). >>>> Why? No, I need to check the whole sum and the whole size. >>> If the bitmap already exists, you don't create a new directory entry but >>> reuse the existing one. Therefore, the number of bitmaps in the image >>> and the directory size will not grow then. >> new_nb_bitmaps is not number of "newly created bitmaps", but just new >> value of field nb_bitmaps, so, all bitmaps - old and new are calculated >> into new_nb_bitmaps. Anyway, this misunderstanding shows that variable >> name is bad.. > Yes. But when you store a bitmap of the same name as an existing one, > you are replacing it. The number of bitmaps does not grow in that case. Oh, I'm stupid)) I see now, you are right. > > Max >
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 81be1ca..a975388 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -28,6 +28,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" #include "exec/log.h" +#include "qemu/cutils.h" #include "block/block_int.h" #include "block/qcow2.h" @@ -43,6 +44,10 @@ #define BME_MIN_GRANULARITY_BITS 9 #define BME_MAX_NAME_SIZE 1023 +#if BME_MAX_TABLE_SIZE * 8ULL > INT_MAX +#error In the code bitmap table physical size assumed to fit into int +#endif + /* Bitmap directory entry flags */ #define BME_RESERVED_FLAGS 0xfffffffcU #define BME_FLAG_IN_USE 1 @@ -74,6 +79,8 @@ typedef struct Qcow2Bitmap { uint8_t granularity_bits; char *name; + BdrvDirtyBitmap *dirty_bitmap; + QSIMPLEQ_ENTRY(Qcow2Bitmap) entry; } Qcow2Bitmap; typedef QSIMPLEQ_HEAD(Qcow2BitmapList, Qcow2Bitmap) Qcow2BitmapList; @@ -87,6 +94,27 @@ static inline bool can_write(BlockDriverState *bs) return !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE); } +static int update_header_sync(BlockDriverState *bs) +{ + int ret; + + ret = qcow2_update_header(bs); + if (ret < 0) { + return ret; + } + + /* We doesn't return bdrv_flush error code. Even if it fails, write was + * successful and it is more logical to consider that header is in the new + * state than in the old. + */ + ret = bdrv_flush(bs); + if (ret < 0) { + fprintf(stderr, "Failed to flush qcow2 header"); + } + + return 0; +} + static inline void bitmap_table_to_cpu(uint64_t *bitmap_table, size_t size) { size_t i; @@ -96,6 +124,15 @@ static inline void bitmap_table_to_cpu(uint64_t *bitmap_table, size_t size) } } +static inline void bitmap_table_to_be(uint64_t *bitmap_table, size_t size) +{ + size_t i; + + for (i = 0; i < size; ++i) { + cpu_to_be64s(&bitmap_table[i]); + } +} + /* Check table entry specification constraints. If cluster_size is 0, offset * alignment is not checked. */ static int check_table_entry(uint64_t entry, int cluster_size) @@ -121,6 +158,51 @@ static int check_table_entry(uint64_t entry, int cluster_size) return 0; } +static int check_constraints_on_bitmap(BlockDriverState *bs, + const char *name, + uint32_t granularity) +{ + BDRVQcow2State *s = bs->opaque; + int granularity_bits = ctz32(granularity); + + int64_t nb_sectors = bdrv_nb_sectors(bs); + + if (nb_sectors < 0) { + return nb_sectors; + } + + uint64_t phys_bitmap_bytes = (nb_sectors << BDRV_SECTOR_BITS) >> + granularity_bits; + uint64_t bitmap_table_size = phys_bitmap_bytes / s->cluster_size; + size_t name_size = strlen(name); + + int fail = + (bitmap_table_size > BME_MAX_TABLE_SIZE) || + (phys_bitmap_bytes > BME_MAX_PHYS_SIZE) || + (granularity_bits > BME_MAX_GRANULARITY_BITS) || + (granularity_bits < BME_MIN_GRANULARITY_BITS) || + (name_size > BME_MAX_NAME_SIZE); + + return fail ? -EINVAL : 0; +} + +static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table, + uint32_t bitmap_table_size) +{ + BDRVQcow2State *s = bs->opaque; + int i; + + for (i = 0; i < bitmap_table_size; ++i) { + uint64_t addr = bitmap_table[i] & BME_TABLE_ENTRY_OFFSET_MASK; + if (!addr) { + continue; + } + + qcow2_free_clusters(bs, addr, s->cluster_size, QCOW2_DISCARD_OTHER); + bitmap_table[i] = 0; + } +} + static int bitmap_table_load(BlockDriverState *bs, Qcow2Bitmap *bm, uint64_t **bitmap_table) { @@ -159,6 +241,28 @@ fail: return ret; } +static int free_bitmap_clusters(BlockDriverState *bs, Qcow2Bitmap *bm) +{ + int ret; + uint64_t *bitmap_table; + + ret = bitmap_table_load(bs, bm, &bitmap_table); + if (ret < 0) { + assert(bitmap_table == NULL); + return ret; + } + + clear_bitmap_table(bs, bitmap_table, bm->table_size); + qcow2_free_clusters(bs, bm->table_offset, bm->table_size * sizeof(uint64_t), + QCOW2_DISCARD_OTHER); + g_free(bitmap_table); + + bm->table_offset = 0; + bm->table_size = 0; + + return 0; +} + /* This function returns the number of disk sectors covered by a single cluster * of bitmap data. */ static uint64_t disk_sectors_in_bitmap_cluster(const BDRVQcow2State *s, @@ -603,6 +707,72 @@ fail: * Bitmap List end */ +static int update_ext_header_and_dir(BlockDriverState *bs, + Qcow2BitmapList *bm_list) +{ + BDRVQcow2State *s = bs->opaque; + int ret; + Qcow2Bitmap *bm; + uint64_t new_offset = 0; + uint64_t new_size = 0; + uint32_t new_nb_bitmaps = 0; + uint64_t old_offset = s->bitmap_directory_offset; + uint64_t old_size = s->bitmap_directory_size; + uint32_t old_nb_bitmaps = s->nb_bitmaps; + uint64_t old_autocl = s->autoclear_features; + + if (bm_list != NULL && !QSIMPLEQ_EMPTY(bm_list)) { + QSIMPLEQ_FOREACH(bm, bm_list, entry) { + new_nb_bitmaps++; + } + + if (new_nb_bitmaps > QCOW2_MAX_BITMAPS) { + return -EINVAL; + } + + ret = bitmap_list_store(bs, bm_list, &new_offset, &new_size, false); + if (ret < 0) { + return ret; + } + + ret = bdrv_flush(bs); + if (ret < 0) { + goto fail; + } + + s->autoclear_features |= QCOW2_AUTOCLEAR_BITMAPS; + } else { + s->autoclear_features &= ~(uint64_t)QCOW2_AUTOCLEAR_BITMAPS; + } + + s->bitmap_directory_offset = new_offset; + s->bitmap_directory_size = new_size; + s->nb_bitmaps = new_nb_bitmaps; + + ret = update_header_sync(bs); + if (ret < 0) { + goto fail; + } + + if (old_size > 0) { + qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_OTHER); + } + + return 0; + +fail: + if (new_offset > 0) { + qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_OTHER); + } + + s->bitmap_directory_offset = old_offset; + s->bitmap_directory_size = old_size; + s->nb_bitmaps = old_nb_bitmaps; + s->autoclear_features = old_autocl; + + return ret; +} + /* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */ static void release_dirty_bitmap_helper(gpointer bitmap, gpointer bs) @@ -664,3 +834,284 @@ fail: g_slist_free(created_dirty_bitmaps); bitmap_list_free(bm_list); } + +/* store_bitmap_data() + * Store bitmap to image, filling bitmap table accordingly. + */ +static uint64_t *store_bitmap_data(BlockDriverState *bs, + BdrvDirtyBitmap *bitmap, + uint32_t *bitmap_table_size, Error **errp) +{ + int ret; + BDRVQcow2State *s = bs->opaque; + int64_t sector; + uint64_t dsc; + uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap); + const char *bm_name = bdrv_dirty_bitmap_name(bitmap); + uint8_t *buf = NULL; + BdrvDirtyBitmapIter *dbi; + uint64_t *tb; + uint64_t tb_size = + size_to_clusters(s, + bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size)); + + if (tb_size > BME_MAX_TABLE_SIZE || + tb_size * s->cluster_size > BME_MAX_PHYS_SIZE) { + error_setg(errp, "Bitmap '%s' is too big", bm_name); + return NULL; + } + + tb = g_try_new0(uint64_t, tb_size); + if (tb == NULL) { + error_setg(errp, "No memory"); + return NULL; + } + + dbi = bdrv_dirty_iter_new(bitmap, 0); + buf = g_malloc(s->cluster_size); + dsc = disk_sectors_in_bitmap_cluster(s, bitmap); + + while ((sector = bdrv_dirty_iter_next(dbi)) != -1) { + uint64_t cluster = sector / dsc; + uint64_t end, write_size; + int64_t off; + + sector = cluster * dsc; + end = MIN(bm_size, sector + dsc); + write_size = + bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector); + + off = qcow2_alloc_clusters(bs, s->cluster_size); + if (off < 0) { + error_setg_errno(errp, -off, + "Failed to allocate clusters for bitmap '%s'", + bm_name); + goto fail; + } + tb[cluster] = off; + + bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end - sector); + if (write_size < s->cluster_size) { + memset(buf + write_size, 0, s->cluster_size - write_size); + } + + ret = qcow2_pre_write_overlap_check(bs, 0, off, s->cluster_size); + if (ret < 0) { + error_setg_errno(errp, -ret, "Qcow2 overlap check failed"); + goto fail; + } + + ret = bdrv_pwrite(bs->file, off, buf, s->cluster_size); + if (ret < 0) { + error_setg_errno(errp, -ret, "Failed to write bitmap '%s' to file", + bm_name); + goto fail; + } + + if (end >= bm_size) { + break; + } + + bdrv_set_dirty_iter(dbi, end); + } + + *bitmap_table_size = tb_size; + g_free(buf); + bdrv_dirty_iter_free(dbi); + + return tb; + +fail: + clear_bitmap_table(bs, tb, tb_size); + g_free(buf); + bdrv_dirty_iter_free(dbi); + g_free(tb); + + return NULL; +} + +/* store_bitmap() + * Store bm->dirty_bitmap to qcow2. + * Set bm->table_offset and bm->table_size accordingly. + */ +static int store_bitmap(BlockDriverState *bs, Qcow2Bitmap *bm, Error **errp) +{ + int ret; + uint64_t *tb; + int64_t tb_offset; + uint32_t tb_size; + BdrvDirtyBitmap *bitmap = bm->dirty_bitmap; + const char *bm_name; + + assert(bitmap != NULL); + + bm_name = bdrv_dirty_bitmap_name(bitmap); + + tb = store_bitmap_data(bs, bitmap, &tb_size, errp); + if (tb == NULL) { + g_free(tb); + return -EINVAL; + } + + assert(tb_size <= BME_MAX_TABLE_SIZE); + tb_offset = qcow2_alloc_clusters(bs, tb_size * sizeof(tb[0])); + if (tb_offset < 0) { + error_setg_errno(errp, -tb_offset, + "Failed to allocate clusters for bitmap '%s'", + bm_name); + goto fail; + } + + ret = qcow2_pre_write_overlap_check(bs, 0, tb_offset, + tb_size * sizeof(tb[0])); + if (ret < 0) { + error_setg_errno(errp, -ret, "Qcow2 overlap check failed"); + goto fail; + } + + bitmap_table_to_be(tb, tb_size); + ret = bdrv_pwrite(bs->file, tb_offset, tb, tb_size * sizeof(tb[0])); + if (ret < 0) { + error_setg_errno(errp, -ret, "Failed to write bitmap '%s' to file", + bm_name); + goto fail; + } + + g_free(tb); + + bm->table_offset = tb_offset; + bm->table_size = tb_size; + + return 0; + +fail: + clear_bitmap_table(bs, tb, tb_size); + + if (tb_offset > 0) { + qcow2_free_clusters(bs, tb_offset, tb_size, QCOW2_DISCARD_OTHER); + } + + g_free(tb); + + return ret; +} + +static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList *bm_list, + const char *name) +{ + Qcow2Bitmap *bm; + + QSIMPLEQ_FOREACH(bm, bm_list, entry) { + if (strcmp(name, bm->name) == 0) { + return bm; + } + } + + return NULL; +} + +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) +{ + BdrvDirtyBitmap *bitmap; + BDRVQcow2State *s = bs->opaque; + uint32_t new_nb_bitmaps = s->nb_bitmaps; + uint64_t new_dir_size = s->bitmap_directory_size; + int ret; + Qcow2BitmapList *bm_list; + Qcow2Bitmap *bm; + + if (!bdrv_has_persistent_bitmaps(bs)) { + /* nothing to do */ + return; + } + + if (s->nb_bitmaps == 0) { + bm_list = bitmap_list_new(); + } else { + bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, + s->bitmap_directory_size, errp); + if (bm_list == NULL) { + /* errp is already set */ + return; + } + } + + /* check constraints and names */ + for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL; + bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) { + const char *name = bdrv_dirty_bitmap_name(bitmap); + uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap); + Qcow2Bitmap *bm; + + if (!bdrv_dirty_bitmap_get_persistance(bitmap)) { + continue; + } + + if (++new_nb_bitmaps > QCOW2_MAX_BITMAPS) { + error_setg(errp, "Too many persistent bitmaps"); + goto fail; + } + + new_dir_size += calc_dir_entry_size(strlen(name), 0); + if (new_dir_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) { + error_setg(errp, "Too large bitmap directory"); + goto fail; + } + + if (check_constraints_on_bitmap(bs, name, granularity) < 0) { + error_setg(errp, "Bitmap '%s' doesn't satisfy the constraints", + name); + goto fail; + } + + bm = find_bitmap_by_name(bm_list, name); + if (bm == NULL) { + bm = g_new0(Qcow2Bitmap, 1); + bm->name = g_strdup(name); + QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry); + } else { + if (!(bm->flags & BME_FLAG_IN_USE) && can_write(bs)) { + error_setg(errp, "Bitmap '%s' is already exists in the image", + name); + goto fail; + } + free_bitmap_clusters(bs, bm); + /* TODO it is better to reuse these clusters */ + } + bm->flags = bdrv_dirty_bitmap_get_autoload(bitmap) ? BME_FLAG_AUTO : 0; + bm->granularity_bits = ctz32(bdrv_dirty_bitmap_granularity(bitmap)); + bm->dirty_bitmap = bitmap; + } + + /* allocate clusters and store bitmaps */ + QSIMPLEQ_FOREACH(bm, bm_list, entry) { + if (bm->dirty_bitmap == NULL) { + continue; + } + + ret = store_bitmap(bs, bm, errp); + if (ret < 0) { + goto fail; + } + } + + ret = update_ext_header_and_dir(bs, bm_list); + if (ret < 0) { + error_setg_errno(errp, -ret, "Failed to update bitmap extension"); + goto fail; + } + + bitmap_list_free(bm_list); + return; + +fail: + QSIMPLEQ_FOREACH(bm, bm_list, entry) { + if (bm->dirty_bitmap == NULL || bm->table_offset == 0) { + continue; + } + + free_bitmap_clusters(bs, bm); + } + + bitmap_list_free(bm_list); +} diff --git a/block/qcow2.c b/block/qcow2.c index 443be0e..66c7f74 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -3515,6 +3515,7 @@ BlockDriver bdrv_qcow2 = { .bdrv_attach_aio_context = qcow2_attach_aio_context, .bdrv_load_autoloading_dirty_bitmaps = qcow2_load_autoloading_dirty_bitmaps, + .bdrv_store_persistent_dirty_bitmaps = qcow2_store_persistent_dirty_bitmaps, }; static void bdrv_qcow2_init(void) diff --git a/block/qcow2.h b/block/qcow2.h index bcedf5b..d9a7643 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -615,5 +615,6 @@ void qcow2_cache_put(BlockDriverState *bs, Qcow2Cache *c, void **table); /* qcow2-bitmap.c functions */ void qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp); +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp); #endif
Realize block bitmap storing interface, to allow qcow2 images store persistent bitmaps. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/qcow2-bitmap.c | 451 +++++++++++++++++++++++++++++++++++++++++++++++++++ block/qcow2.c | 1 + block/qcow2.h | 1 + 3 files changed, 453 insertions(+)