On 02/03/2017 04:40 AM, Vladimir Sementsov-Ogievskiy wrote:
Realize block bitmap storing interface, to allow qcow2 images store
persistent bitmaps.
Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
---
block/qcow2-bitmap.c | 489 +++++++++++++++++++++++++++++++++++++++++++++++++--
block/qcow2.c | 1 +
block/qcow2.h | 1 +
3 files changed, 476 insertions(+), 15 deletions(-)
+
+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, Qcow2BitmapTable *tb,
uint64_t **bitmap_table)
It would have been nicer to have factored out the size and offset from
the very beginning to avoid churn within this series, but... oh well.
Next time you write a 24 patch series, OK? :)
{
int ret;
@@ -152,20 +218,20 @@ static int bitmap_table_load(BlockDriverState *bs,
Qcow2Bitmap *bm,
uint32_t i;
uint64_t *table;
- assert(bm->table_size != 0);
- table = g_try_new(uint64_t, bm->table_size);
+ assert(tb->size != 0);
+ table = g_try_new(uint64_t, tb->size);
if (table == NULL) {
return -ENOMEM;
}
- assert(bm->table_size <= BME_MAX_TABLE_SIZE);
- ret = bdrv_pread(bs->file, bm->table_offset,
- table, bm->table_size * sizeof(uint64_t));
+ assert(tb->size <= BME_MAX_TABLE_SIZE);
+ ret = bdrv_pread(bs->file, tb->offset,
+ table, tb->size * sizeof(uint64_t));
if (ret < 0) {
goto fail;
}
- for (i = 0; i < bm->table_size; ++i) {
+ for (i = 0; i < tb->size; ++i) {
be64_to_cpus(&table[i]);
ret = check_table_entry(table[i], s->cluster_size);
if (ret < 0) {
@@ -182,6 +248,28 @@ fail:
return ret;
}
+
+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;
+ Qcow2BitmapTableList drop_tables;
+ Qcow2BitmapTable *tb, *tb_next;
+
+ QSIMPLEQ_INIT(&drop_tables);
+
+ if (!can_write(bs)) {
+ error_setg(errp, "No write access");
+ return;
+ }
+
+ 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);
Oh, this isn't cached from the autoload mechanism? We have to re-create
this list every time we save?
I suppose it's safest that way, but it's something we can likely improve on.
+ if (bm_list == NULL) {
+ /* errp is already set */
Usually implicit when checking the return code from a function that
takes an errp parameter.
+ return;
+ }
+ }
+
+ /* check constraints and names */
+ for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
+ bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
+ {
It occurs to me at this point that the framework you have established is
very dirty-bitmap heavy, even though we support other types of bitmaps.
Not really a big problem, as when we go to support OTHER types of
bitmaps, we can just change the names of things as we need to.
Just a comment.
+ 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 (check_constraints_on_bitmap(bs, name, granularity) < 0) {
+ error_setg(errp, "Bitmap '%s' doesn't satisfy the constraints",
+ name);
+ goto fail;
+ }
At this point I really do begin to become concerned that it will be very
easy for people to accidentally back themselves into a case where they
cannot save their bitmaps to disk, but will have no idea why that is
true because the error message is vague.
I am not fully clear on how easy it would be to create a bitmap that
QEMU will accept but refuse to flush to disk for size reasons, but I
think it's fairly easy to create a name that will overcome the string
limit we've imposed.
(Can we begin imposing warnings or errors for people who make bitmaps
with names that are too big? 1023 should be enough for all current uses
of this feature, I'd hope.)
CC eblake, QMP lawyer ...
+
+ bm = find_bitmap_by_name(bm_list, name);
+ if (bm == NULL) {
+ 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;
+ }
+
Suggest "Bitmap directory is too large" instead.
+ 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)) {
+ error_setg(errp, "Bitmap '%s' already exists in the image",
+ name);
+ goto fail;
+ }
+ tb = g_memdup(&bm->table, sizeof(bm->table));
+ bm->table.offset = 0;
+ bm->table.size = 0;
I guess this is so that updating the tables is 'safe'.
+ QSIMPLEQ_INSERT_TAIL(&drop_tables, tb, entry);
+ }
+ 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 directory was successfully updated, so, old data can be dropped.
+ * TODO it is better to reuse these clusters */
+ QSIMPLEQ_FOREACH_SAFE(tb, &drop_tables, entry, tb_next) {
+ free_bitmap_clusters(bs, tb);
+ g_free(tb);
+ }
+
+ 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->table);
+ }
+
+ QSIMPLEQ_FOREACH_SAFE(tb, &drop_tables, entry, tb_next) {
+ g_free(tb);
+ }
+
+ bitmap_list_free(bm_list);
+}
Looks good otherwise, but some clarification on the error messages and
that errant free explained will garner you the R-B.
Thanks,
--js