* [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries
@ 2018-05-12 1:25 John Snow
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 01/12] qcow2-bitmap: cache bm_list John Snow
` (13 more replies)
0 siblings, 14 replies; 37+ messages in thread
From: John Snow @ 2018-05-12 1:25 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake,
Vladimir Sementsov-Ogievskiy, John Snow
Allow qemu-img to show information about bitmaps stored in qcow2 images.
Add a 'bitmap' meta-command with 'dump' sub-command to retrieve a list of
dirty regions in bitmaps stored in a qcow2 image.
RFC:
- I am not 1000% convinced the bm_list caching is perfectly safe,
especially with respect to migration and inactivation. There are
more efficiencies and tweaks I can make, but I think this is the
minimal set.
- I decided not to gate bitmap info in the "info" command behind
extra flags.
- Bitmap data-gathering in "bitmap dump" could be made more
space-efficient by just reporting one segment at a time,
probably.
- `bitmap rm` and `bitmap mk` need extra work, so I am not submitting
them just yet: rm needs more work around the remove persistence API,
and make needs more work around the "can add" API.
- None of these commands will work with "in use" bitmaps; we need
qemu-img check -r bitmaps support for this. I'm not sure what the
right behavior to "fix" in-use bitmaps should be:
- Cleared: This might be dangerous.
- Fully Set: This is safer, but stupid.
- Deleted: This might be the best option.
- Un-set in-use: VERY dangerous; would rather not.
John Snow (12):
qcow2-bitmap: cache bm_list
qcow2/dirty-bitmap: cache loaded bitmaps
block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps
qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO
qcow2-bitmap: track bitmap type
qapi: add bitmap info
qcow2-bitmap: add basic bitmaps info
qjson: allow caller to ask for arbitrary indent
qapi/block-core: add BitmapMapping and BitmapEntry structs
qemu-img: split off common chunk of map command
qemu-img: add bitmap dump
qemu-img: add bitmap clear
block/qcow2-bitmap.c | 220 +++++++++++++++++--------
block/qcow2.c | 9 +
block/qcow2.h | 3 +
include/qapi/qmp/qjson.h | 1 +
qapi/block-core.json | 92 ++++++++++-
qemu-img-cmds.hx | 6 +
qemu-img.c | 419 +++++++++++++++++++++++++++++++++++++----------
qobject/qjson.c | 21 +--
8 files changed, 612 insertions(+), 159 deletions(-)
--
2.14.3
^ permalink raw reply [flat|nested] 37+ messages in thread
* [Qemu-devel] [RFC PATCH 01/12] qcow2-bitmap: cache bm_list
2018-05-12 1:25 [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries John Snow
@ 2018-05-12 1:25 ` John Snow
2018-05-14 11:55 ` Vladimir Sementsov-Ogievskiy
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 02/12] qcow2/dirty-bitmap: cache loaded bitmaps John Snow
` (12 subsequent siblings)
13 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2018-05-12 1:25 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake,
Vladimir Sementsov-Ogievskiy, John Snow
We don't need to re-read this list every time, exactly. We can keep it cached
and delete our copy when we flush to disk.
Because we don't try to flush bitmaps on close if there's nothing to flush,
add a new conditional to delete the state anyway for a clean exit.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/qcow2-bitmap.c | 74 ++++++++++++++++++++++++++++++++--------------------
block/qcow2.c | 2 ++
block/qcow2.h | 2 ++
3 files changed, 50 insertions(+), 28 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 6e93ec43e1..fb0a4f3ec4 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -536,8 +536,7 @@ static uint32_t bitmap_list_count(Qcow2BitmapList *bm_list)
* Get bitmap list from qcow2 image. Actually reads bitmap directory,
* checks it and convert to bitmap list.
*/
-static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
- uint64_t size, Error **errp)
+static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error **errp)
{
int ret;
BDRVQcow2State *s = bs->opaque;
@@ -545,6 +544,8 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
Qcow2BitmapDirEntry *e;
uint32_t nb_dir_entries = 0;
Qcow2BitmapList *bm_list = NULL;
+ uint64_t offset = s->bitmap_directory_offset;
+ uint64_t size = s->bitmap_directory_size;
if (size == 0) {
error_setg(errp, "Requested bitmap directory size is zero");
@@ -636,6 +637,30 @@ fail:
return NULL;
}
+static Qcow2BitmapList *get_bitmap_list(BlockDriverState *bs, Error **errp)
+{
+ BDRVQcow2State *s = bs->opaque;
+ Qcow2BitmapList *bm_list;
+
+ if (s->bitmap_list) {
+ return (Qcow2BitmapList *)s->bitmap_list;
+ }
+
+ bm_list = bitmap_list_load(bs, errp);
+ s->bitmap_list = bm_list;
+ return bm_list;
+}
+
+static void del_bitmap_list(BlockDriverState *bs)
+{
+ BDRVQcow2State *s = bs->opaque;
+
+ if (s->bitmap_list) {
+ bitmap_list_free(s->bitmap_list);
+ s->bitmap_list = NULL;
+ }
+}
+
int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
void **refcount_table,
int64_t *refcount_table_size)
@@ -656,8 +681,7 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
return ret;
}
- bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
- s->bitmap_directory_size, NULL);
+ bm_list = get_bitmap_list(bs, NULL);
if (bm_list == NULL) {
res->corruptions++;
return -EINVAL;
@@ -707,8 +731,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
}
out:
- bitmap_list_free(bm_list);
-
return ret;
}
@@ -953,8 +975,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
return false;
}
- bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
- s->bitmap_directory_size, errp);
+ bm_list = get_bitmap_list(bs, errp);
if (bm_list == NULL) {
return false;
}
@@ -992,14 +1013,12 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
}
g_slist_free(created_dirty_bitmaps);
- bitmap_list_free(bm_list);
-
return header_updated;
fail:
g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs);
g_slist_free(created_dirty_bitmaps);
- bitmap_list_free(bm_list);
+ del_bitmap_list(bs);
return false;
}
@@ -1027,8 +1046,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
return -EINVAL;
}
- bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
- s->bitmap_directory_size, errp);
+ bm_list = get_bitmap_list(bs, errp);
if (bm_list == NULL) {
return -EINVAL;
}
@@ -1068,7 +1086,6 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
out:
g_slist_free(ro_dirty_bitmaps);
- bitmap_list_free(bm_list);
return ret;
}
@@ -1277,8 +1294,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
return;
}
- bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
- s->bitmap_directory_size, errp);
+ bm_list = get_bitmap_list(bs, errp);
if (bm_list == NULL) {
return;
}
@@ -1300,7 +1316,11 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
fail:
bitmap_free(bm);
- bitmap_list_free(bm_list);
+}
+
+void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs)
+{
+ del_bitmap_list(bs);
}
void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
@@ -1317,12 +1337,12 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
if (!bdrv_has_changed_persistent_bitmaps(bs)) {
/* nothing to do */
- return;
+ goto out;
}
if (!can_write(bs)) {
error_setg(errp, "No write access");
- return;
+ goto out;
}
QSIMPLEQ_INIT(&drop_tables);
@@ -1330,10 +1350,9 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
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);
+ bm_list = get_bitmap_list(bs, errp);
if (bm_list == NULL) {
- return;
+ goto out;
}
}
@@ -1414,8 +1433,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
g_free(tb);
}
- bitmap_list_free(bm_list);
- return;
+ goto out;
fail:
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
@@ -1430,7 +1448,9 @@ fail:
g_free(tb);
}
- bitmap_list_free(bm_list);
+ out:
+ del_bitmap_list(bs);
+ return;
}
int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
@@ -1495,14 +1515,12 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
goto fail;
}
- bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
- s->bitmap_directory_size, errp);
+ bm_list = get_bitmap_list(bs, errp);
if (bm_list == NULL) {
goto fail;
}
found = find_bitmap_by_name(bm_list, name);
- bitmap_list_free(bm_list);
if (found) {
error_setg(errp, "Bitmap with the same name is already stored");
goto fail;
diff --git a/block/qcow2.c b/block/qcow2.c
index 2f36e632f9..7ae9000656 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2135,6 +2135,8 @@ static void qcow2_close(BlockDriverState *bs)
if (!(s->flags & BDRV_O_INACTIVE)) {
qcow2_inactivate(bs);
+ } else {
+ qcow2_persistent_dirty_bitmaps_cache_destroy(bs);
}
cache_clean_timer_del(bs);
diff --git a/block/qcow2.h b/block/qcow2.h
index adf5c3950f..796a8c914b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -299,6 +299,7 @@ typedef struct BDRVQcow2State {
uint64_t bitmap_directory_size;
uint64_t bitmap_directory_offset;
bool dirty_bitmaps_loaded;
+ void *bitmap_list;
int flags;
int qcow_version;
@@ -675,6 +676,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
Error **errp);
int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
+void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs);
void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
--
2.14.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [RFC PATCH 02/12] qcow2/dirty-bitmap: cache loaded bitmaps
2018-05-12 1:25 [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries John Snow
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 01/12] qcow2-bitmap: cache bm_list John Snow
@ 2018-05-12 1:25 ` John Snow
2018-05-14 12:33 ` Vladimir Sementsov-Ogievskiy
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 03/12] block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps John Snow
` (11 subsequent siblings)
13 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2018-05-12 1:25 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake,
Vladimir Sementsov-Ogievskiy, John Snow
For bitmaps that we succeeded in loading, we can cache a reference
to that object. This will let us iterate over the more convenient
form of in-memory bitmaps for qemu-img bitmap manipulation tools.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/qcow2-bitmap.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index fb0a4f3ec4..d89758f235 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -986,6 +986,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
if (bitmap == NULL) {
goto fail;
}
+ bm->dirty_bitmap = bitmap;
if (!(bm->flags & BME_FLAG_AUTO)) {
bdrv_disable_dirty_bitmap(bitmap);
--
2.14.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [RFC PATCH 03/12] block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps
2018-05-12 1:25 [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries John Snow
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 01/12] qcow2-bitmap: cache bm_list John Snow
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 02/12] qcow2/dirty-bitmap: cache loaded bitmaps John Snow
@ 2018-05-12 1:25 ` John Snow
2018-05-14 12:44 ` Vladimir Sementsov-Ogievskiy
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 04/12] qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO John Snow
` (10 subsequent siblings)
13 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2018-05-12 1:25 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake,
Vladimir Sementsov-Ogievskiy, John Snow
Instead of always setting IN_USE, handle whether or not the bitmap
is read-only instead of a two-loop pass. This will allow us to show
the flags exactly as they appear for bitmaps in `qemu-img info`.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/qcow2-bitmap.c | 48 ++++++++++++++++++++----------------------------
1 file changed, 20 insertions(+), 28 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index d89758f235..fc8d52fc3e 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -942,13 +942,6 @@ fail:
return ret;
}
-/* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */
-static void release_dirty_bitmap_helper(gpointer bitmap,
- gpointer bs)
-{
- bdrv_release_dirty_bitmap(bs, bitmap);
-}
-
/* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */
static void set_readonly_helper(gpointer bitmap, gpointer value)
{
@@ -967,8 +960,8 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
BDRVQcow2State *s = bs->opaque;
Qcow2BitmapList *bm_list;
Qcow2Bitmap *bm;
- GSList *created_dirty_bitmaps = NULL;
bool header_updated = false;
+ bool needs_update = false;
if (s->nb_bitmaps == 0) {
/* No bitmaps - nothing to do */
@@ -992,33 +985,32 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
bdrv_disable_dirty_bitmap(bitmap);
}
bdrv_dirty_bitmap_set_persistance(bitmap, true);
- bm->flags |= BME_FLAG_IN_USE;
- created_dirty_bitmaps =
- g_slist_append(created_dirty_bitmaps, bitmap);
- }
- }
-
- if (created_dirty_bitmaps != NULL) {
- if (can_write(bs)) {
- /* in_use flags must be updated */
- int ret = update_ext_header_and_dir_in_place(bs, bm_list);
- if (ret < 0) {
- error_setg_errno(errp, -ret, "Can't update bitmap directory");
- goto fail;
+ if (can_write(bs)) {
+ bm->flags |= BME_FLAG_IN_USE;
+ needs_update = true;
+ } else {
+ bdrv_dirty_bitmap_set_readonly(bitmap, true);
}
- header_updated = true;
- } else {
- g_slist_foreach(created_dirty_bitmaps, set_readonly_helper,
- (gpointer)true);
}
}
- g_slist_free(created_dirty_bitmaps);
+ /* in_use flags must be updated */
+ if (needs_update) {
+ int ret = update_ext_header_and_dir_in_place(bs, bm_list);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Can't update bitmap directory");
+ goto fail;
+ }
+ header_updated = true;
+ }
return header_updated;
fail:
- g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs);
- g_slist_free(created_dirty_bitmaps);
+ QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+ if (bm->dirty_bitmap) {
+ bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
+ }
+ }
del_bitmap_list(bs);
return false;
--
2.14.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [RFC PATCH 04/12] qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO
2018-05-12 1:25 [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries John Snow
` (2 preceding siblings ...)
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 03/12] block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps John Snow
@ 2018-05-12 1:25 ` John Snow
2018-05-14 12:55 ` Vladimir Sementsov-Ogievskiy
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 05/12] qcow2-bitmap: track bitmap type John Snow
` (9 subsequent siblings)
13 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2018-05-12 1:25 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake,
Vladimir Sementsov-Ogievskiy, John Snow
For the purposes of qemu-img manipulation and querying of bitmaps, load
bitmaps that are "in use" -- if the image is read only. This will allow
us to diagnose problems with this flag using the qemu-img tool.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/qcow2-bitmap.c | 32 +++++++++++++++-----------------
1 file changed, 15 insertions(+), 17 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index fc8d52fc3e..b556dbdccd 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -346,7 +346,7 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
uint32_t granularity;
BdrvDirtyBitmap *bitmap = NULL;
- if (bm->flags & BME_FLAG_IN_USE) {
+ if (can_write(bs) && (bm->flags & BME_FLAG_IN_USE)) {
error_setg(errp, "Bitmap '%s' is in use", bm->name);
goto fail;
}
@@ -974,23 +974,21 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
}
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
- if (!(bm->flags & BME_FLAG_IN_USE)) {
- BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
- if (bitmap == NULL) {
- goto fail;
- }
- bm->dirty_bitmap = bitmap;
+ BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
+ if (bitmap == NULL) {
+ goto fail;
+ }
+ bm->dirty_bitmap = bitmap;
- if (!(bm->flags & BME_FLAG_AUTO)) {
- bdrv_disable_dirty_bitmap(bitmap);
- }
- bdrv_dirty_bitmap_set_persistance(bitmap, true);
- if (can_write(bs)) {
- bm->flags |= BME_FLAG_IN_USE;
- needs_update = true;
- } else {
- bdrv_dirty_bitmap_set_readonly(bitmap, true);
- }
+ if (!(bm->flags & BME_FLAG_AUTO)) {
+ bdrv_disable_dirty_bitmap(bitmap);
+ }
+ bdrv_dirty_bitmap_set_persistance(bitmap, true);
+ if (can_write(bs)) {
+ bm->flags |= BME_FLAG_IN_USE;
+ needs_update = true;
+ } else {
+ bdrv_dirty_bitmap_set_readonly(bitmap, true);
}
}
--
2.14.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [RFC PATCH 05/12] qcow2-bitmap: track bitmap type
2018-05-12 1:25 [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries John Snow
` (3 preceding siblings ...)
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 04/12] qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO John Snow
@ 2018-05-12 1:25 ` John Snow
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 06/12] qapi: add bitmap info John Snow
` (8 subsequent siblings)
13 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2018-05-12 1:25 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake,
Vladimir Sementsov-Ogievskiy, John Snow
We only have one type of persistent bitmap right now, but I'd like the
qemu-img tool to be able to give good diagnostic information if it sees
an unknown/unsupported type.
We do enforce it to be the dirty tracking type in check_dir_entry, but
I wanted positive affirmation of the type in the forthcoming info script.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/qcow2-bitmap.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b556dbdccd..60e01abfd7 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -83,6 +83,7 @@ typedef QSIMPLEQ_HEAD(Qcow2BitmapTableList, Qcow2BitmapTable)
typedef struct Qcow2Bitmap {
Qcow2BitmapTable table;
uint32_t flags;
+ uint8_t type;
uint8_t granularity_bits;
char *name;
@@ -608,6 +609,7 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error **errp)
bm->table.offset = e->bitmap_table_offset;
bm->table.size = e->bitmap_table_size;
bm->flags = e->flags;
+ bm->type = e->type;
bm->granularity_bits = e->granularity_bits;
bm->name = dir_entry_copy_name(e);
QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry);
--
2.14.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [RFC PATCH 06/12] qapi: add bitmap info
2018-05-12 1:25 [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries John Snow
` (4 preceding siblings ...)
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 05/12] qcow2-bitmap: track bitmap type John Snow
@ 2018-05-12 1:25 ` John Snow
2018-05-14 14:30 ` Vladimir Sementsov-Ogievskiy
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 07/12] qcow2-bitmap: add basic bitmaps info John Snow
` (7 subsequent siblings)
13 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2018-05-12 1:25 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake,
Vladimir Sementsov-Ogievskiy, John Snow
Add some of the necessary scaffolding for reporting bitmap information.
Signed-off-by: John Snow <jsnow@redhat.com>
---
qapi/block-core.json | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 59 insertions(+), 1 deletion(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index c50517bff3..8f33f41ce7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -33,6 +33,61 @@
'date-sec': 'int', 'date-nsec': 'int',
'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
+##
+# @BitmapTypeEnum:
+#
+# An enumeration of possible serialized bitmap types.
+#
+# @dirty-tracking: This bitmap records information on dirty
+# segments within the file.
+#
+# @unknown: This bitmap is of an unknown or reserved type.
+#
+# Since: 2.13
+##
+{ 'enum': 'BitmapTypeEnum', 'data': [ 'dirty-tracking', 'unknown' ] }
+
+##
+# @BitmapFlagEnum:
+#
+# An enumeration of possible flags for serialized bitmaps.
+#
+# @in-use: This bitmap is considered to be in-use, and may now be inconsistent.
+#
+# @auto: This bitmap must reflect any and all changes to the file it describes.
+# The type of this bitmap must be @DirtyTrackingBitmap.
+#
+# @extra-data-compatible: This bitmap has extra information associated with it.
+#
+# @unknown: This bitmap has unknown or reserved properties.
+#
+# Since: 2.13
+##
+{ 'enum': 'BitmapFlagEnum', 'data': [ 'in-use', 'auto',
+ 'extra-data-compatible', 'unknown' ] }
+
+##
+# @BitmapInfo:
+#
+# @name: The name of the bitmap.
+#
+# @type: The type of bitmap.
+#
+# @granularity: Bitmap granularity, in bytes.
+#
+# @count: Overall bitmap dirtiness, in bytes.
+#
+# @flags: Bitmap flags, if any.
+#
+# Since: 2.13
+#
+##
+{ 'struct': 'BitmapInfo',
+ 'data': { 'name': 'str', 'type': 'BitmapTypeEnum', 'granularity': 'int',
+ 'count': 'int', '*flags': ['BitmapFlagEnum']
+ }
+}
+
##
# @ImageInfoSpecificQCow2EncryptionBase:
#
@@ -69,6 +124,8 @@
# @encrypt: details about encryption parameters; only set if image
# is encrypted (since 2.10)
#
+# @bitmaps: list of image bitmaps (since 2.13)
+#
# Since: 1.7
##
{ 'struct': 'ImageInfoSpecificQCow2',
@@ -77,7 +134,8 @@
'*lazy-refcounts': 'bool',
'*corrupt': 'bool',
'refcount-bits': 'int',
- '*encrypt': 'ImageInfoSpecificQCow2Encryption'
+ '*encrypt': 'ImageInfoSpecificQCow2Encryption',
+ '*bitmaps': ['BitmapInfo']
} }
##
--
2.14.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [RFC PATCH 07/12] qcow2-bitmap: add basic bitmaps info
2018-05-12 1:25 [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries John Snow
` (5 preceding siblings ...)
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 06/12] qapi: add bitmap info John Snow
@ 2018-05-12 1:25 ` John Snow
2018-05-14 15:12 ` Vladimir Sementsov-Ogievskiy
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 08/12] qjson: allow caller to ask for arbitrary indent John Snow
` (6 subsequent siblings)
13 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2018-05-12 1:25 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake,
Vladimir Sementsov-Ogievskiy, John Snow
Add functions for querying the basic information inside of bitmaps.
Restructure the bitmaps flags masks to facilitate providing a list of
flags belonging to the bitmap(s) being queried.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/qcow2-bitmap.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++--
block/qcow2.c | 7 +++++
block/qcow2.h | 1 +
3 files changed, 87 insertions(+), 2 deletions(-)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 60e01abfd7..811b82743a 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -49,8 +49,28 @@
/* Bitmap directory entry flags */
#define BME_RESERVED_FLAGS 0xfffffffcU
-#define BME_FLAG_IN_USE (1U << 0)
-#define BME_FLAG_AUTO (1U << 1)
+
+enum BME_FLAG_BITS {
+ BME_FLAG_BIT__BEGIN = 0,
+ BME_FLAG_BIT_IN_USE = BME_FLAG_BIT__BEGIN,
+ BME_FLAG_BIT_AUTO = 1,
+ BME_FLAG_BIT_EXTRA = 2,
+ BME_FLAG_BIT__MAX,
+};
+
+#define BME_FLAG_IN_USE (1U << BME_FLAG_BIT_IN_USE)
+#define BME_FLAG_AUTO (1U << BME_FLAG_BIT_AUTO)
+#define BME_FLAG_EXTRA (1U << BME_FLAG_BIT_EXTRA)
+
+/* Correlate canonical spec values to autogenerated QAPI values */
+struct {
+ uint32_t mask;
+ int qapi_value;
+} BMEFlagMap[BME_FLAG_BIT__MAX] = {
+ [BME_FLAG_BIT_IN_USE] = { BME_FLAG_IN_USE, BITMAP_FLAG_ENUM_IN_USE },
+ [BME_FLAG_BIT_AUTO] = { BME_FLAG_AUTO, BITMAP_FLAG_ENUM_AUTO },
+ [BME_FLAG_BIT_EXTRA] = { BME_FLAG_EXTRA, BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE },
+};
/* bits [1, 8] U [56, 63] are reserved */
#define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001feULL
@@ -663,6 +683,63 @@ static void del_bitmap_list(BlockDriverState *bs)
}
}
+static BitmapFlagEnumList *get_bitmap_flags(uint32_t flags)
+{
+ int i;
+ BitmapFlagEnumList *flist = NULL;
+ BitmapFlagEnumList *ftmp;
+
+ while (flags) {
+ i = ctz32(flags);
+ ftmp = g_new0(BitmapFlagEnumList, 1);
+ if (i >= BME_FLAG_BIT__BEGIN && i < BME_FLAG_BIT__MAX) {
+ ftmp->value = BMEFlagMap[i].qapi_value;
+ } else {
+ ftmp->value = BITMAP_FLAG_ENUM_UNKNOWN;
+ }
+ ftmp->next = flist;
+ flist = ftmp;
+ flags &= ~(1 << i);
+ }
+
+ return flist;
+}
+
+BitmapInfoList *qcow2_get_bitmap_info(BlockDriverState *bs)
+{
+ Qcow2BitmapList *bm_list;
+ Qcow2Bitmap *bm;
+ BitmapInfoList *info_list = NULL;
+ BitmapInfoList *tmp;
+ BitmapInfo *bitmap_info;
+
+ bm_list = get_bitmap_list(bs, NULL);
+ if (!bm_list) {
+ return info_list;
+ }
+
+ QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+ bitmap_info = g_new0(BitmapInfo, 1);
+ bitmap_info->name = g_strdup(bm->name);
+ if (bm->type == BT_DIRTY_TRACKING_BITMAP) {
+ bitmap_info->type = BITMAP_TYPE_ENUM_DIRTY_TRACKING;
+ } else {
+ bitmap_info->type = BITMAP_TYPE_ENUM_UNKNOWN;
+ }
+ bitmap_info->granularity = 1 << bm->granularity_bits;
+ bitmap_info->count = bdrv_get_dirty_count(bm->dirty_bitmap);
+ bitmap_info->flags = get_bitmap_flags(bm->flags);
+ bitmap_info->has_flags = !!bitmap_info->flags;
+
+ tmp = g_new0(BitmapInfoList, 1);
+ tmp->value = bitmap_info;
+ tmp->next = info_list;
+ info_list = tmp;
+ }
+
+ return info_list;
+}
+
int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
void **refcount_table,
int64_t *refcount_table_size)
diff --git a/block/qcow2.c b/block/qcow2.c
index 7ae9000656..0b24ecb6b2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3960,6 +3960,7 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
BDRVQcow2State *s = bs->opaque;
ImageInfoSpecific *spec_info;
QCryptoBlockInfo *encrypt_info = NULL;
+ BitmapInfoList *bitmap_list = NULL;
if (s->crypto != NULL) {
encrypt_info = qcrypto_block_get_info(s->crypto, &error_abort);
@@ -4016,6 +4017,12 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
spec_info->u.qcow2.data->encrypt = qencrypt;
}
+ bitmap_list = qcow2_get_bitmap_info(bs);
+ if (bitmap_list) {
+ spec_info->u.qcow2.data->has_bitmaps = true;
+ spec_info->u.qcow2.data->bitmaps = bitmap_list;
+ }
+
return spec_info;
}
diff --git a/block/qcow2.h b/block/qcow2.h
index 796a8c914b..76bf5fa55d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -686,5 +686,6 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
const char *name,
Error **errp);
+BitmapInfoList *qcow2_get_bitmap_info(BlockDriverState *bs);
#endif
--
2.14.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [RFC PATCH 08/12] qjson: allow caller to ask for arbitrary indent
2018-05-12 1:25 [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries John Snow
` (6 preceding siblings ...)
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 07/12] qcow2-bitmap: add basic bitmaps info John Snow
@ 2018-05-12 1:25 ` John Snow
2018-05-16 21:34 ` Eric Blake
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 09/12] qapi/block-core: add BitmapMapping and BitmapEntry structs John Snow
` (5 subsequent siblings)
13 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2018-05-12 1:25 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake,
Vladimir Sementsov-Ogievskiy, John Snow
The function already exists, just expose it.
Signed-off-by: John Snow <jsnow@redhat.com>
---
include/qapi/qmp/qjson.h | 1 +
qobject/qjson.c | 21 +++++++++++----------
2 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
index b274ac3a86..0e8624c1fa 100644
--- a/include/qapi/qmp/qjson.h
+++ b/include/qapi/qmp/qjson.h
@@ -19,6 +19,7 @@ QObject *qobject_from_jsonf(const char *string, ...) GCC_FMT_ATTR(1, 2);
QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
GCC_FMT_ATTR(1, 0);
+QString *qobject_to_json_opt(const QObject *obj, int pretty, int indent);
QString *qobject_to_json(const QObject *obj);
QString *qobject_to_json_pretty(const QObject *obj);
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 9816a65c7d..d603e1cce1 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -252,20 +252,21 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
}
}
+QString *qobject_to_json_opt(const QObject *obj, int pretty, int indent)
+{
+ QString *str = qstring_new();
+
+ to_json(obj, str, pretty, indent);
+
+ return str;
+}
+
QString *qobject_to_json(const QObject *obj)
{
- QString *str = qstring_new();
-
- to_json(obj, str, 0, 0);
-
- return str;
+ return qobject_to_json_opt(obj, 0, 0);
}
QString *qobject_to_json_pretty(const QObject *obj)
{
- QString *str = qstring_new();
-
- to_json(obj, str, 1, 0);
-
- return str;
+ return qobject_to_json_opt(obj, 1, 0);
}
--
2.14.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [RFC PATCH 09/12] qapi/block-core: add BitmapMapping and BitmapEntry structs
2018-05-12 1:25 [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries John Snow
` (7 preceding siblings ...)
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 08/12] qjson: allow caller to ask for arbitrary indent John Snow
@ 2018-05-12 1:25 ` John Snow
2018-05-16 21:37 ` Eric Blake
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 10/12] qemu-img: split off common chunk of map command John Snow
` (4 subsequent siblings)
13 siblings, 1 reply; 37+ messages in thread
From: John Snow @ 2018-05-12 1:25 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake,
Vladimir Sementsov-Ogievskiy, John Snow
Add two new structures for detailing the marked regions of bitmaps as
saved in e.g. qcow2 files.
Signed-off-by: John Snow <jsnow@redhat.com>
---
qapi/block-core.json | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8f33f41ce7..de8ad73a78 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -298,6 +298,38 @@
'zero': 'bool', 'depth': 'int', '*offset': 'int',
'*filename': 'str' } }
+##
+# @BitmapEntry:
+#
+# Dirty Bitmap region information for a virtual block range
+#
+# @offset: the start byte of the dirty virtual range
+#
+# @length: the number of bytes of the dirty virtual range
+#
+# Since: 2.13
+#
+##
+{ 'struct': 'BitmapEntry',
+ 'data': { 'offset': 'int', 'length': 'int' } }
+
+##
+# @BitmapMapping:
+#
+# List of described regions correlated to a named bitmap.
+#
+# @name: The name of the bitmap whose range is described here
+#
+# @entries: A list of zero or more @BitmapEntry elements representing
+# the range(s) described by the bitmap.
+#
+# Since: 2.13
+#
+##
+{ 'struct': 'BitmapMapping',
+ 'data': { 'name': 'str',
+ 'entries': [ 'BitmapEntry' ] } }
+
##
# @BlockdevCacheInfo:
#
--
2.14.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [RFC PATCH 10/12] qemu-img: split off common chunk of map command
2018-05-12 1:25 [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries John Snow
` (8 preceding siblings ...)
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 09/12] qapi/block-core: add BitmapMapping and BitmapEntry structs John Snow
@ 2018-05-12 1:25 ` John Snow
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 11/12] qemu-img: add bitmap dump John Snow
` (3 subsequent siblings)
13 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2018-05-12 1:25 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake,
Vladimir Sementsov-Ogievskiy, John Snow
It will be re-used for a bitmap listing command.
Signed-off-by: John Snow <jsnow@redhat.com>
---
qemu-img.c | 192 +++++++++++++++++++++++++++++++++++--------------------------
1 file changed, 110 insertions(+), 82 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index ea62d2d61e..e31e38f674 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -69,8 +69,8 @@ enum {
};
typedef enum OutputFormat {
- OFORMAT_JSON,
OFORMAT_HUMAN,
+ OFORMAT_JSON,
} OutputFormat;
/* Default to cache=writeback as data integrity is not important for qemu-img */
@@ -2728,96 +2728,122 @@ static inline bool entry_mergeable(const MapEntry *curr, const MapEntry *next)
return true;
}
+typedef struct CommonOpts {
+ OutputFormat output_format;
+ const char *filename;
+ const char *fmt;
+ bool force_share;
+ bool image_opts;
+} CommonOpts;
+
+static int parse_opts_common(CommonOpts *opts, int argc, char **argv)
+{
+ int c;
+
+ for (;;) {
+ int option_index = 0;
+ static const struct option long_options[] = {
+ {"help", no_argument, 0, 'h'},
+ {"format", required_argument, 0, 'f'},
+ {"output", required_argument, 0, OPTION_OUTPUT},
+ {"object", required_argument, 0, OPTION_OBJECT},
+ {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+ {"force-share", no_argument, 0, 'U'},
+ {0, 0, 0, 0}
+ };
+ c = getopt_long(argc, argv, ":f:hU",
+ long_options, &option_index);
+ if (c == -1) {
+ break;
+ }
+ switch (c) {
+ case ':':
+ missing_argument(argv[optind - 1]);
+ break;
+ case '?':
+ unrecognized_option(argv[optind - 1]);
+ break;
+ case 'h':
+ help();
+ break;
+ case 'f':
+ opts->fmt = optarg;
+ break;
+ case 'U':
+ opts->force_share = true;
+ break;
+ case OPTION_OUTPUT:
+ if (optarg && !strcmp(optarg, "json")) {
+ opts->output_format = OFORMAT_JSON;
+ } else if (optarg && !strcmp(optarg, "human")) {
+ opts->output_format = OFORMAT_HUMAN;
+ } else {
+ error_report("--output must be used with human or json as argument.");
+ return -EINVAL;
+ }
+ break;
+ case OPTION_OBJECT: {
+ QemuOpts *opts;
+ opts = qemu_opts_parse_noisily(&qemu_object_opts,
+ optarg, true);
+ if (!opts) {
+ return -EINVAL;
+ }
+ } break;
+ case OPTION_IMAGE_OPTS:
+ opts->image_opts = true;
+ break;
+ }
+ }
+ if (qemu_opts_foreach(&qemu_object_opts,
+ user_creatable_add_opts_foreach,
+ NULL, NULL)) {
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static void parse_positional(int argc, char *argv[],
+ bool opt, const char **str, const char *name) {
+ if (!opt && optind >= argc) {
+ error_exit("Expecting '%s' positional parameter", name);
+ } else if (optind < argc) {
+ *str = argv[optind++];
+ }
+}
+
+static void parse_unexpected(int argc, char *argv[]) {
+ if (optind != argc) {
+ error_exit("Unexpected argument '%s'\n", argv[optind]);
+ }
+}
+
static int img_map(int argc, char **argv)
{
- int c;
- OutputFormat output_format = OFORMAT_HUMAN;
BlockBackend *blk;
BlockDriverState *bs;
- const char *filename, *fmt, *output;
int64_t length;
+ int ret = 0;
MapEntry curr = { .length = 0 }, next;
- int ret = 0;
- bool image_opts = false;
- bool force_share = false;
+ CommonOpts *opts;
- fmt = NULL;
- output = NULL;
- for (;;) {
- int option_index = 0;
- static const struct option long_options[] = {
- {"help", no_argument, 0, 'h'},
- {"format", required_argument, 0, 'f'},
- {"output", required_argument, 0, OPTION_OUTPUT},
- {"object", required_argument, 0, OPTION_OBJECT},
- {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
- {"force-share", no_argument, 0, 'U'},
- {0, 0, 0, 0}
- };
- c = getopt_long(argc, argv, ":f:hU",
- long_options, &option_index);
- if (c == -1) {
- break;
- }
- switch (c) {
- case ':':
- missing_argument(argv[optind - 1]);
- break;
- case '?':
- unrecognized_option(argv[optind - 1]);
- break;
- case 'h':
- help();
- break;
- case 'f':
- fmt = optarg;
- break;
- case 'U':
- force_share = true;
- break;
- case OPTION_OUTPUT:
- output = optarg;
- break;
- case OPTION_OBJECT: {
- QemuOpts *opts;
- opts = qemu_opts_parse_noisily(&qemu_object_opts,
- optarg, true);
- if (!opts) {
- return 1;
- }
- } break;
- case OPTION_IMAGE_OPTS:
- image_opts = true;
- break;
- }
- }
- if (optind != argc - 1) {
- error_exit("Expecting one image file name");
- }
- filename = argv[optind];
-
- if (output && !strcmp(output, "json")) {
- output_format = OFORMAT_JSON;
- } else if (output && !strcmp(output, "human")) {
- output_format = OFORMAT_HUMAN;
- } else if (output) {
- error_report("--output must be used with human or json as argument.");
- return 1;
- }
-
- if (qemu_opts_foreach(&qemu_object_opts,
- user_creatable_add_opts_foreach,
- NULL, NULL)) {
- return 1;
+ opts = g_new0(CommonOpts, 1);
+ if (parse_opts_common(opts, argc, argv)) {
+ return EXIT_FAILURE;
}
+ parse_positional(argc, argv, 0, &opts->filename, "filename");
+ parse_unexpected(argc, argv);
- blk = img_open(image_opts, filename, fmt, 0, false, false, force_share);
+ blk = img_open(opts->image_opts, opts->filename, opts->fmt, 0,
+ false, false, opts->force_share);
if (!blk) {
- return 1;
+ ret = -1;
+ goto out1;
}
bs = blk_bs(blk);
- if (output_format == OFORMAT_HUMAN) {
+ if (opts->output_format == OFORMAT_HUMAN) {
printf("%-16s%-16s%-16s%s\n", "Offset", "Length", "Mapped to", "File");
}
@@ -2832,7 +2858,7 @@ static int img_map(int argc, char **argv)
if (ret < 0) {
error_report("Could not read file metadata: %s", strerror(-ret));
- goto out;
+ goto out2;
}
if (entry_mergeable(&curr, &next)) {
@@ -2841,15 +2867,17 @@ static int img_map(int argc, char **argv)
}
if (curr.length > 0) {
- dump_map_entry(output_format, &curr, &next);
+ dump_map_entry(opts->output_format, &curr, &next);
}
curr = next;
}
- dump_map_entry(output_format, &curr, NULL);
+ dump_map_entry(opts->output_format, &curr, NULL);
-out:
+out2:
blk_unref(blk);
+out1:
+ g_free(opts);
return ret < 0;
}
--
2.14.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [RFC PATCH 11/12] qemu-img: add bitmap dump
2018-05-12 1:25 [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries John Snow
` (9 preceding siblings ...)
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 10/12] qemu-img: split off common chunk of map command John Snow
@ 2018-05-12 1:25 ` John Snow
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 12/12] qemu-img: add bitmap clear John Snow
` (2 subsequent siblings)
13 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2018-05-12 1:25 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake,
Vladimir Sementsov-Ogievskiy, John Snow
Signed-off-by: John Snow <jsnow@redhat.com>
---
qemu-img-cmds.hx | 6 ++
qemu-img.c | 185 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 191 insertions(+)
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 2fe31893cf..d25f359f5a 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -22,6 +22,12 @@ STEXI
@item bench [-c @var{count}] [-d @var{depth}] [-f @var{fmt}] [--flush-interval=@var{flush_interval}] [-n] [--no-drain] [-o @var{offset}] [--pattern=@var{pattern}] [-q] [-s @var{buffer_size}] [-S @var{step_size}] [-t @var{cache}] [-w] [-U] @var{filename}
ETEXI
+DEF("bitmap", img_bitmap,
+ "bitmap dump [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-U] filename [bitmap]")
+STEXI
+@item bitmap dump [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [--output=@var{ofmt}] [-U] @var{filename} [@var{bitmap}]
+ETEXI
+
DEF("check", img_check,
"check [-q] [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] [-U] filename")
STEXI
diff --git a/qemu-img.c b/qemu-img.c
index e31e38f674..1141216617 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2881,6 +2881,191 @@ out1:
return ret < 0;
}
+static BitmapMapping *get_bitmap_mapping(BdrvDirtyBitmap *bitmap)
+{
+ BdrvDirtyBitmapIter *it = bdrv_dirty_iter_new(bitmap);
+ BitmapMapping *bm = g_new0(BitmapMapping, 1);
+ BitmapEntry *entry;
+ BitmapEntryList *tmp, **last;
+ int64_t offset;
+ int64_t zero_offset;
+
+ bm->name = g_strdup(bdrv_dirty_bitmap_name(bitmap));
+ bm->entries = NULL;
+ last = &bm->entries;
+
+ while (true) {
+ offset = bdrv_dirty_iter_next(it);
+ if (offset == -1) {
+ break;
+ }
+
+ zero_offset = bdrv_dirty_bitmap_next_zero(bitmap, offset);
+ if (zero_offset == -1) {
+ zero_offset = bdrv_dirty_bitmap_size(bitmap);
+ }
+
+ entry = g_new0(BitmapEntry, 1);
+ entry->offset = offset;
+ entry->length = zero_offset - offset;
+
+ tmp = g_new0(BitmapEntryList, 1);
+ tmp->value = entry;
+ *last = tmp;
+ last = &tmp->next;
+
+ if (zero_offset >= bdrv_dirty_bitmap_size(bitmap)) {
+ break;
+ }
+ bdrv_set_dirty_iter(it, zero_offset);
+ }
+
+ bdrv_dirty_iter_free(it);
+ return bm;
+}
+
+static void dump_bitmap_mapping_json(BitmapMapping *bm, int indent,
+ FILE *stream)
+{
+ QString *str;
+ QObject *obj;
+ Visitor *v = qobject_output_visitor_new(&obj);
+ const int pretty = 1;
+
+ visit_type_BitmapMapping(v, NULL, &bm, &error_abort);
+ visit_complete(v, &obj);
+ str = qobject_to_json_opt(obj, pretty, indent);
+ assert(str != NULL);
+ fprintf(stream, "%*s%s", 4 * indent, "", qstring_get_str(str));
+ qobject_unref(obj);
+ visit_free(v);
+ qobject_unref(str);
+}
+
+static void dump_bitmap_mapping_human(BitmapMapping *bm, FILE *stream)
+{
+ BitmapEntryList *bl = bm->entries;
+
+ fprintf(stream, "# %s\n", bm->name);
+ fprintf(stream, "%-16s%-16s\n", "Offset", "Length");
+ for ( ; bl; bl = bl->next) {
+ BitmapEntry *be = bl->value;
+ fprintf(stream, "%-16"PRId64"%-16"PRId64"\n", be->offset, be->length);
+ }
+}
+
+static void dump_bitmap_mapping(BitmapMapping *bm, OutputFormat output_format,
+ int indent, FILE *stream)
+{
+ switch (output_format) {
+ case OFORMAT_JSON:
+ dump_bitmap_mapping_json(bm, indent, stream);
+ return;
+ case OFORMAT_HUMAN:
+ dump_bitmap_mapping_human(bm, stream);
+ return;
+ }
+}
+
+/* img_bitmap subcommands: dump */
+
+typedef struct BitmapOpts {
+ CommonOpts base;
+ const char *name;
+ uint32_t granularity;
+} BitmapOpts;
+
+static int bitmap_cmd_dump(BlockDriverState *bs, BitmapOpts *opts)
+{
+ BdrvDirtyBitmap *bitmap;
+ BitmapMapping *bm;
+ bool printed;
+ OutputFormat ofmt = opts->base.output_format;
+
+ if (opts->name) {
+ bitmap = bdrv_find_dirty_bitmap(bs, opts->name);
+ if (bitmap) {
+ bm = get_bitmap_mapping(bitmap);
+ dump_bitmap_mapping(bm, ofmt, 0, stdout);
+ qapi_free_BitmapMapping(bm);
+ } else {
+ if (ofmt == OFORMAT_HUMAN) {
+ fprintf(stdout, "Bitmap '%s' not found\n", opts->name);
+ }
+ }
+ } else {
+ if (ofmt == OFORMAT_JSON) {
+ fprintf(stdout, "[\n");
+ }
+ for (bitmap = NULL, printed = false;
+ (bitmap = bdrv_dirty_bitmap_next(bs, bitmap)); printed = true) {
+ if (printed) {
+ fprintf(stdout, ofmt == OFORMAT_JSON ? ",\n" : "\n");
+ }
+ bm = get_bitmap_mapping(bitmap);
+ dump_bitmap_mapping(bm, ofmt, 1, stdout);
+ qapi_free_BitmapMapping(bm);
+ }
+ if (ofmt == OFORMAT_JSON) {
+ fprintf(stdout, "%s%s", printed ? "\n" : "", "]\n");
+ }
+ if (ofmt == OFORMAT_HUMAN && printed == false) {
+ fprintf(stdout, "No bitmaps are present in this image.\n");
+ }
+ }
+
+ return 0;
+}
+
+static int img_bitmap(int argc, char **argv)
+{
+ const char *cmd;
+ BitmapOpts *opts;
+ BlockBackend *blk;
+ BlockDriverState *bs;
+ const char *bname = NULL;
+ int flags = 0;
+ int ret = EXIT_SUCCESS;
+ int (* handler)(BlockDriverState *b, BitmapOpts *opts);
+
+ if (argc < 2) {
+ error_report("Expected a bitmap subcommand: <dump>");
+ return EXIT_FAILURE;
+ }
+ cmd = argv[1];
+ if (strcmp(cmd, "dump") == 0) {
+ handler = bitmap_cmd_dump;
+ } else {
+ error_report("Unrecognized bitmap subcommand '%s'", cmd);
+ return EXIT_FAILURE;
+ }
+
+ opts = g_new0(BitmapOpts, 1);
+ if (parse_opts_common(&opts->base, --argc, ++argv)) {
+ return EXIT_FAILURE;
+ }
+ parse_positional(argc, argv, 0, &opts->base.filename, "filename");
+ parse_positional(argc, argv, 1, &bname, "bitmap");
+ parse_unexpected(argc, argv);
+
+ blk = img_open(opts->base.image_opts, opts->base.filename, opts->base.fmt,
+ flags, false, false, opts->base.force_share);
+ if (!blk) {
+ ret = EXIT_FAILURE;
+ goto out;
+ }
+ bs = blk_bs(blk);
+
+ if (handler(bs, opts)) {
+ ret = EXIT_FAILURE;
+ }
+
+ blk_unref(blk);
+ out:
+ g_free(opts);
+ return ret;
+}
+
#define SNAPSHOT_LIST 1
#define SNAPSHOT_CREATE 2
#define SNAPSHOT_APPLY 3
--
2.14.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [Qemu-devel] [RFC PATCH 12/12] qemu-img: add bitmap clear
2018-05-12 1:25 [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries John Snow
` (10 preceding siblings ...)
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 11/12] qemu-img: add bitmap dump John Snow
@ 2018-05-12 1:25 ` John Snow
2018-05-12 1:38 ` [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries no-reply
2018-05-14 11:32 ` Vladimir Sementsov-Ogievskiy
13 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2018-05-12 1:25 UTC (permalink / raw)
To: qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake,
Vladimir Sementsov-Ogievskiy, John Snow
Signed-off-by: John Snow <jsnow@redhat.com>
---
qemu-img-cmds.hx | 4 ++--
qemu-img.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 46 insertions(+), 4 deletions(-)
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index d25f359f5a..7b6ec73488 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -23,9 +23,9 @@ STEXI
ETEXI
DEF("bitmap", img_bitmap,
- "bitmap dump [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-U] filename [bitmap]")
+ "bitmap <dump|clear> [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-U] filename [bitmap]")
STEXI
-@item bitmap dump [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [--output=@var{ofmt}] [-U] @var{filename} [@var{bitmap}]
+@item bitmap <dump|clear> [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [--output=@var{ofmt}] [-U] @var{filename} [@var{bitmap}]
ETEXI
DEF("check", img_check,
diff --git a/qemu-img.c b/qemu-img.c
index 1141216617..b5ab8a3837 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2967,7 +2967,22 @@ static void dump_bitmap_mapping(BitmapMapping *bm, OutputFormat output_format,
}
}
-/* img_bitmap subcommands: dump */
+static int do_bitmap_clear(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+ bool enabled = bdrv_dirty_bitmap_enabled(bitmap);
+
+ if (!enabled) {
+ bdrv_enable_dirty_bitmap(bitmap);
+ }
+ bdrv_clear_dirty_bitmap(bitmap, NULL);
+ if (!enabled) {
+ bdrv_disable_dirty_bitmap(bitmap);
+ }
+
+ return 0;
+}
+
+/* img_bitmap subcommands: dump, clear */
typedef struct BitmapOpts {
CommonOpts base;
@@ -3017,6 +3032,30 @@ static int bitmap_cmd_dump(BlockDriverState *bs, BitmapOpts *opts)
return 0;
}
+static int bitmap_cmd_clear(BlockDriverState *bs, BitmapOpts *opts)
+{
+ BdrvDirtyBitmap *bitmap = NULL;
+
+ if (opts->name) {
+ bitmap = bdrv_find_dirty_bitmap(bs, opts->name);
+ if (!bitmap) {
+ error_report("No bitmap named '%s' found", opts->name);
+ return -1;
+ }
+ if (do_bitmap_clear(bs, bitmap)) {
+ return -1;
+ }
+ } else {
+ while ((bitmap = bdrv_dirty_bitmap_next(bs, bitmap))) {
+ if (do_bitmap_clear(bs, bitmap)) {
+ return -1;
+ }
+ }
+ }
+
+ return 0;
+}
+
static int img_bitmap(int argc, char **argv)
{
const char *cmd;
@@ -3029,12 +3068,15 @@ static int img_bitmap(int argc, char **argv)
int (* handler)(BlockDriverState *b, BitmapOpts *opts);
if (argc < 2) {
- error_report("Expected a bitmap subcommand: <dump>");
+ error_report("Expected a bitmap subcommand: <dump | clear>");
return EXIT_FAILURE;
}
cmd = argv[1];
if (strcmp(cmd, "dump") == 0) {
handler = bitmap_cmd_dump;
+ } else if (strcmp(cmd, "clear") == 0) {
+ flags |= BDRV_O_RDWR;
+ handler = bitmap_cmd_clear;
} else {
error_report("Unrecognized bitmap subcommand '%s'", cmd);
return EXIT_FAILURE;
--
2.14.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries
2018-05-12 1:25 [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries John Snow
` (11 preceding siblings ...)
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 12/12] qemu-img: add bitmap clear John Snow
@ 2018-05-12 1:38 ` no-reply
2018-05-14 11:32 ` Vladimir Sementsov-Ogievskiy
13 siblings, 0 replies; 37+ messages in thread
From: no-reply @ 2018-05-12 1:38 UTC (permalink / raw)
To: jsnow; +Cc: famz, qemu-block, qemu-devel, kwolf, vsementsov, armbru, mreitz
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180512012537.22478-1-jsnow@redhat.com
Subject: [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/20180512012537.22478-1-jsnow@redhat.com -> patchew/20180512012537.22478-1-jsnow@redhat.com
Switched to a new branch 'test'
98c78cde32 qemu-img: add bitmap clear
64b46d376f qemu-img: add bitmap dump
e820e6d20d qemu-img: split off common chunk of map command
15894bec9b qapi/block-core: add BitmapMapping and BitmapEntry structs
a09d1c3a65 qjson: allow caller to ask for arbitrary indent
384e244766 qcow2-bitmap: add basic bitmaps info
35cf9ace22 qapi: add bitmap info
36be89af25 qcow2-bitmap: track bitmap type
d347564ea3 qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO
c554bff42b block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps
9d00c36fba qcow2/dirty-bitmap: cache loaded bitmaps
7f34c78783 qcow2-bitmap: cache bm_list
=== OUTPUT BEGIN ===
Checking PATCH 1/12: qcow2-bitmap: cache bm_list...
Checking PATCH 2/12: qcow2/dirty-bitmap: cache loaded bitmaps...
Checking PATCH 3/12: block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps...
Checking PATCH 4/12: qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO...
Checking PATCH 5/12: qcow2-bitmap: track bitmap type...
Checking PATCH 6/12: qapi: add bitmap info...
Checking PATCH 7/12: qcow2-bitmap: add basic bitmaps info...
WARNING: line over 80 characters
#43: FILE: block/qcow2-bitmap.c:72:
+ [BME_FLAG_BIT_EXTRA] = { BME_FLAG_EXTRA, BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE },
total: 0 errors, 1 warnings, 118 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 8/12: qjson: allow caller to ask for arbitrary indent...
Checking PATCH 9/12: qapi/block-core: add BitmapMapping and BitmapEntry structs...
Checking PATCH 10/12: qemu-img: split off common chunk of map command...
WARNING: line over 80 characters
#74: FILE: qemu-img.c:2781:
+ error_report("--output must be used with human or json as argument.");
ERROR: open brace '{' following function declarations go on the next line
#127: FILE: qemu-img.c:2816:
+static void parse_unexpected(int argc, char *argv[]) {
total: 1 errors, 1 warnings, 173 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 11/12: qemu-img: add bitmap dump...
ERROR: space prohibited after that '*' (ctx:BxW)
#179: FILE: qemu-img.c:3029:
+ int (* handler)(BlockDriverState *b, BitmapOpts *opts);
^
total: 1 errors, 0 warnings, 203 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 12/12: qemu-img: add bitmap clear...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries
2018-05-12 1:25 [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries John Snow
` (12 preceding siblings ...)
2018-05-12 1:38 ` [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries no-reply
@ 2018-05-14 11:32 ` Vladimir Sementsov-Ogievskiy
13 siblings, 0 replies; 37+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-05-14 11:32 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake
12.05.2018 04:25, John Snow wrote:
> Allow qemu-img to show information about bitmaps stored in qcow2 images.
> Add a 'bitmap' meta-command with 'dump' sub-command to retrieve a list of
> dirty regions in bitmaps stored in a qcow2 image.
>
> RFC:
> - I am not 1000% convinced the bm_list caching is perfectly safe,
> especially with respect to migration and inactivation. There are
> more efficiencies and tweaks I can make, but I think this is the
> minimal set.
>
> - I decided not to gate bitmap info in the "info" command behind
> extra flags.
>
> - Bitmap data-gathering in "bitmap dump" could be made more
> space-efficient by just reporting one segment at a time,
> probably.
>
> - `bitmap rm` and `bitmap mk` need extra work, so I am not submitting
> them just yet: rm needs more work around the remove persistence API,
> and make needs more work around the "can add" API.
>
> - None of these commands will work with "in use" bitmaps; we need
> qemu-img check -r bitmaps support for this. I'm not sure what the
> right behavior to "fix" in-use bitmaps should be:
> - Cleared: This might be dangerous.
> - Fully Set: This is safer, but stupid.
> - Deleted: This might be the best option.
I think it's the only valid option.
> - Un-set in-use: VERY dangerous; would rather not.
>
> John Snow (12):
> qcow2-bitmap: cache bm_list
> qcow2/dirty-bitmap: cache loaded bitmaps
> block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps
> qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO
> qcow2-bitmap: track bitmap type
> qapi: add bitmap info
> qcow2-bitmap: add basic bitmaps info
> qjson: allow caller to ask for arbitrary indent
> qapi/block-core: add BitmapMapping and BitmapEntry structs
> qemu-img: split off common chunk of map command
> qemu-img: add bitmap dump
> qemu-img: add bitmap clear
>
> block/qcow2-bitmap.c | 220 +++++++++++++++++--------
> block/qcow2.c | 9 +
> block/qcow2.h | 3 +
> include/qapi/qmp/qjson.h | 1 +
> qapi/block-core.json | 92 ++++++++++-
> qemu-img-cmds.hx | 6 +
> qemu-img.c | 419 +++++++++++++++++++++++++++++++++++++----------
> qobject/qjson.c | 21 +--
> 8 files changed, 612 insertions(+), 159 deletions(-)
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 01/12] qcow2-bitmap: cache bm_list
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 01/12] qcow2-bitmap: cache bm_list John Snow
@ 2018-05-14 11:55 ` Vladimir Sementsov-Ogievskiy
2018-05-14 12:15 ` Vladimir Sementsov-Ogievskiy
2018-05-15 20:27 ` John Snow
0 siblings, 2 replies; 37+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-05-14 11:55 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake
12.05.2018 04:25, John Snow wrote:
> We don't need to re-read this list every time, exactly. We can keep it cached
> and delete our copy when we flush to disk.
Why not simply delete cache only on close (unconditionally)? Why do we
need to remove it after flush?
Actually, I think we need to remove it only in qcow2_inactive, after
storing persistent bitmaps.
>
> Because we don't try to flush bitmaps on close if there's nothing to flush,
> add a new conditional to delete the state anyway for a clean exit.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/qcow2-bitmap.c | 74 ++++++++++++++++++++++++++++++++--------------------
> block/qcow2.c | 2 ++
> block/qcow2.h | 2 ++
> 3 files changed, 50 insertions(+), 28 deletions(-)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 6e93ec43e1..fb0a4f3ec4 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -536,8 +536,7 @@ static uint32_t bitmap_list_count(Qcow2BitmapList *bm_list)
> * Get bitmap list from qcow2 image. Actually reads bitmap directory,
> * checks it and convert to bitmap list.
> */
> -static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
> - uint64_t size, Error **errp)
> +static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error **errp)
> {
> int ret;
> BDRVQcow2State *s = bs->opaque;
> @@ -545,6 +544,8 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
> Qcow2BitmapDirEntry *e;
> uint32_t nb_dir_entries = 0;
> Qcow2BitmapList *bm_list = NULL;
> + uint64_t offset = s->bitmap_directory_offset;
> + uint64_t size = s->bitmap_directory_size;
Worth split this change as a refactoring patch (just remove extra
parameters)?
>
> if (size == 0) {
> error_setg(errp, "Requested bitmap directory size is zero");
> @@ -636,6 +637,30 @@ fail:
> return NULL;
> }
>
> +static Qcow2BitmapList *get_bitmap_list(BlockDriverState *bs, Error **errp)
> +{
> + BDRVQcow2State *s = bs->opaque;
> + Qcow2BitmapList *bm_list;
> +
> + if (s->bitmap_list) {
> + return (Qcow2BitmapList *)s->bitmap_list;
> + }
> +
> + bm_list = bitmap_list_load(bs, errp);
> + s->bitmap_list = bm_list;
> + return bm_list;
> +}
> +
> +static void del_bitmap_list(BlockDriverState *bs)
> +{
> + BDRVQcow2State *s = bs->opaque;
> +
> + if (s->bitmap_list) {
> + bitmap_list_free(s->bitmap_list);
> + s->bitmap_list = NULL;
> + }
> +}
> +
> int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
> void **refcount_table,
> int64_t *refcount_table_size)
> @@ -656,8 +681,7 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
> return ret;
> }
>
> - bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> - s->bitmap_directory_size, NULL);
> + bm_list = get_bitmap_list(bs, NULL);
> if (bm_list == NULL) {
> res->corruptions++;
> return -EINVAL;
> @@ -707,8 +731,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
> }
>
> out:
> - bitmap_list_free(bm_list);
> -
> return ret;
> }
>
> @@ -953,8 +975,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> return false;
> }
>
> - bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> - s->bitmap_directory_size, errp);
> + bm_list = get_bitmap_list(bs, errp);
> if (bm_list == NULL) {
> return false;
> }
> @@ -992,14 +1013,12 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> }
>
> g_slist_free(created_dirty_bitmaps);
> - bitmap_list_free(bm_list);
> -
> return header_updated;
>
> fail:
> g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs);
> g_slist_free(created_dirty_bitmaps);
> - bitmap_list_free(bm_list);
> + del_bitmap_list(bs);
>
> return false;
> }
> @@ -1027,8 +1046,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
> return -EINVAL;
> }
>
> - bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> - s->bitmap_directory_size, errp);
> + bm_list = get_bitmap_list(bs, errp);
> if (bm_list == NULL) {
> return -EINVAL;
> }
> @@ -1068,7 +1086,6 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>
> out:
> g_slist_free(ro_dirty_bitmaps);
> - bitmap_list_free(bm_list);
>
> return ret;
> }
> @@ -1277,8 +1294,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> return;
> }
>
> - bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> - s->bitmap_directory_size, errp);
> + bm_list = get_bitmap_list(bs, errp);
> if (bm_list == NULL) {
> return;
> }
> @@ -1300,7 +1316,11 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>
> fail:
> bitmap_free(bm);
> - bitmap_list_free(bm_list);
> +}
> +
> +void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs)
> +{
> + del_bitmap_list(bs);
> }
>
> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> @@ -1317,12 +1337,12 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>
> if (!bdrv_has_changed_persistent_bitmaps(bs)) {
> /* nothing to do */
> - return;
> + goto out;
> }
>
> if (!can_write(bs)) {
> error_setg(errp, "No write access");
> - return;
> + goto out;
> }
>
> QSIMPLEQ_INIT(&drop_tables);
> @@ -1330,10 +1350,9 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> 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);
> + bm_list = get_bitmap_list(bs, errp);
> if (bm_list == NULL) {
> - return;
> + goto out;
> }
> }
>
> @@ -1414,8 +1433,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> g_free(tb);
> }
>
> - bitmap_list_free(bm_list);
> - return;
> + goto out;
>
> fail:
> QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> @@ -1430,7 +1448,9 @@ fail:
> g_free(tb);
> }
>
> - bitmap_list_free(bm_list);
> + out:
> + del_bitmap_list(bs);
> + return;
> }
>
> int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
> @@ -1495,14 +1515,12 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
> goto fail;
> }
>
> - bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> - s->bitmap_directory_size, errp);
> + bm_list = get_bitmap_list(bs, errp);
> if (bm_list == NULL) {
> goto fail;
> }
>
> found = find_bitmap_by_name(bm_list, name);
> - bitmap_list_free(bm_list);
> if (found) {
> error_setg(errp, "Bitmap with the same name is already stored");
> goto fail;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2f36e632f9..7ae9000656 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2135,6 +2135,8 @@ static void qcow2_close(BlockDriverState *bs)
>
> if (!(s->flags & BDRV_O_INACTIVE)) {
> qcow2_inactivate(bs);
> + } else {
> + qcow2_persistent_dirty_bitmaps_cache_destroy(bs);
> }
>
> cache_clean_timer_del(bs);
> diff --git a/block/qcow2.h b/block/qcow2.h
> index adf5c3950f..796a8c914b 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -299,6 +299,7 @@ typedef struct BDRVQcow2State {
> uint64_t bitmap_directory_size;
> uint64_t bitmap_directory_offset;
> bool dirty_bitmaps_loaded;
> + void *bitmap_list;
>
> int flags;
> int qcow_version;
> @@ -675,6 +676,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
> int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
> Error **errp);
> int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
> +void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs);
> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
> int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
> bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 01/12] qcow2-bitmap: cache bm_list
2018-05-14 11:55 ` Vladimir Sementsov-Ogievskiy
@ 2018-05-14 12:15 ` Vladimir Sementsov-Ogievskiy
2018-05-15 20:38 ` John Snow
2018-05-15 20:27 ` John Snow
1 sibling, 1 reply; 37+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-05-14 12:15 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake
14.05.2018 14:55, Vladimir Sementsov-Ogievskiy wrote:
> 12.05.2018 04:25, John Snow wrote:
>> We don't need to re-read this list every time, exactly. We can keep
>> it cached
>> and delete our copy when we flush to disk.
>
> Why not simply delete cache only on close (unconditionally)? Why do we
> need to remove it after flush?
>
> Actually, I think we need to remove it only in qcow2_inactive, after
> storing persistent bitmaps.
>
>
>>
>> Because we don't try to flush bitmaps on close if there's nothing to
>> flush,
>> add a new conditional to delete the state anyway for a clean exit.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block/qcow2-bitmap.c | 74
>> ++++++++++++++++++++++++++++++++--------------------
>> block/qcow2.c | 2 ++
>> block/qcow2.h | 2 ++
>> 3 files changed, 50 insertions(+), 28 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 6e93ec43e1..fb0a4f3ec4 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -536,8 +536,7 @@ static uint32_t bitmap_list_count(Qcow2BitmapList
>> *bm_list)
>> * Get bitmap list from qcow2 image. Actually reads bitmap directory,
>> * checks it and convert to bitmap list.
>> */
>> -static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs,
>> uint64_t offset,
>> - uint64_t size, Error **errp)
>> +static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error
>> **errp)
>> {
>> int ret;
>> BDRVQcow2State *s = bs->opaque;
>> @@ -545,6 +544,8 @@ static Qcow2BitmapList
>> *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
>> Qcow2BitmapDirEntry *e;
>> uint32_t nb_dir_entries = 0;
>> Qcow2BitmapList *bm_list = NULL;
>> + uint64_t offset = s->bitmap_directory_offset;
>> + uint64_t size = s->bitmap_directory_size;
>
> Worth split this change as a refactoring patch (just remove extra
> parameters)?
>
>> if (size == 0) {
>> error_setg(errp, "Requested bitmap directory size is zero");
>> @@ -636,6 +637,30 @@ fail:
>> return NULL;
>> }
>> +static Qcow2BitmapList *get_bitmap_list(BlockDriverState *bs,
>> Error **errp)
>> +{
>> + BDRVQcow2State *s = bs->opaque;
>> + Qcow2BitmapList *bm_list;
>> +
>> + if (s->bitmap_list) {
>> + return (Qcow2BitmapList *)s->bitmap_list;
>> + }
>> +
>> + bm_list = bitmap_list_load(bs, errp);
>> + s->bitmap_list = bm_list;
>> + return bm_list;
>> +}
>> +
>> +static void del_bitmap_list(BlockDriverState *bs)
>> +{
>> + BDRVQcow2State *s = bs->opaque;
>> +
>> + if (s->bitmap_list) {
>> + bitmap_list_free(s->bitmap_list);
>> + s->bitmap_list = NULL;
>> + }
>> +}
so, with this functions, we see, that list is always safely loaded
through the cache. But we need also guarantee, that list is always saved
through the cache. There are a lot of functions, which stores an
abstract bitmap list, given as a parameter, but we want always store our
cache..
>> +
>> int qcow2_check_bitmaps_refcounts(BlockDriverState *bs,
>> BdrvCheckResult *res,
>> void **refcount_table,
>> int64_t *refcount_table_size)
>> @@ -656,8 +681,7 @@ int
>> qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult
>> *res,
>> return ret;
>> }
>> - bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> - s->bitmap_directory_size, NULL);
>> + bm_list = get_bitmap_list(bs, NULL);
>> if (bm_list == NULL) {
>> res->corruptions++;
>> return -EINVAL;
>> @@ -707,8 +731,6 @@ int
>> qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult
>> *res,
>> }
>> out:
>> - bitmap_list_free(bm_list);
>> -
>> return ret;
>> }
>> @@ -953,8 +975,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState
>> *bs, Error **errp)
>> return false;
>> }
>> - bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> - s->bitmap_directory_size, errp);
>> + bm_list = get_bitmap_list(bs, errp);
>> if (bm_list == NULL) {
>> return false;
>> }
>> @@ -992,14 +1013,12 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState
>> *bs, Error **errp)
>> }
>> g_slist_free(created_dirty_bitmaps);
>> - bitmap_list_free(bm_list);
>> -
>> return header_updated;
>> fail:
>> g_slist_foreach(created_dirty_bitmaps,
>> release_dirty_bitmap_helper, bs);
>> g_slist_free(created_dirty_bitmaps);
>> - bitmap_list_free(bm_list);
>> + del_bitmap_list(bs);
>> return false;
>> }
>> @@ -1027,8 +1046,7 @@ int
>> qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>> return -EINVAL;
>> }
>> - bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> - s->bitmap_directory_size, errp);
>> + bm_list = get_bitmap_list(bs, errp);
>> if (bm_list == NULL) {
>> return -EINVAL;
>> }
>> @@ -1068,7 +1086,6 @@ int
>> qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>> out:
>> g_slist_free(ro_dirty_bitmaps);
>> - bitmap_list_free(bm_list);
>> return ret;
>> }
>> @@ -1277,8 +1294,7 @@ void
>> qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> return;
>> }
>> - bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> - s->bitmap_directory_size, errp);
>> + bm_list = get_bitmap_list(bs, errp);
>> if (bm_list == NULL) {
>> return;
>> }
>> @@ -1300,7 +1316,11 @@ void
>> qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> fail:
>> bitmap_free(bm);
>> - bitmap_list_free(bm_list);
>> +}
>> +
>> +void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs)
>> +{
>> + del_bitmap_list(bs);
>> }
>> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>> Error **errp)
>> @@ -1317,12 +1337,12 @@ void
>> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>> if (!bdrv_has_changed_persistent_bitmaps(bs)) {
>> /* nothing to do */
>> - return;
>> + goto out;
>> }
>> if (!can_write(bs)) {
>> error_setg(errp, "No write access");
>> - return;
>> + goto out;
>> }
>> QSIMPLEQ_INIT(&drop_tables);
>> @@ -1330,10 +1350,9 @@ void
>> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>> 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);
>> + bm_list = get_bitmap_list(bs, errp);
>> if (bm_list == NULL) {
>> - return;
>> + goto out;
>> }
>> }
>> @@ -1414,8 +1433,7 @@ void
>> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>> g_free(tb);
>> }
>> - bitmap_list_free(bm_list);
>> - return;
>> + goto out;
>> fail:
>> QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>> @@ -1430,7 +1448,9 @@ fail:
>> g_free(tb);
>> }
>> - bitmap_list_free(bm_list);
>> + out:
>> + del_bitmap_list(bs);
>> + return;
>> }
>> int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
>> @@ -1495,14 +1515,12 @@ bool
>> qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>> goto fail;
>> }
>> - bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>> - s->bitmap_directory_size, errp);
>> + bm_list = get_bitmap_list(bs, errp);
>> if (bm_list == NULL) {
>> goto fail;
>> }
>> found = find_bitmap_by_name(bm_list, name);
>> - bitmap_list_free(bm_list);
>> if (found) {
>> error_setg(errp, "Bitmap with the same name is already
>> stored");
>> goto fail;
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 2f36e632f9..7ae9000656 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2135,6 +2135,8 @@ static void qcow2_close(BlockDriverState *bs)
>> if (!(s->flags & BDRV_O_INACTIVE)) {
>> qcow2_inactivate(bs);
>> + } else {
>> + qcow2_persistent_dirty_bitmaps_cache_destroy(bs);
>> }
>> cache_clean_timer_del(bs);
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index adf5c3950f..796a8c914b 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -299,6 +299,7 @@ typedef struct BDRVQcow2State {
>> uint64_t bitmap_directory_size;
>> uint64_t bitmap_directory_offset;
>> bool dirty_bitmaps_loaded;
>> + void *bitmap_list;
>> int flags;
>> int qcow_version;
>> @@ -675,6 +676,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState
>> *bs, Error **errp);
>> int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool
>> *header_updated,
>> Error **errp);
>> int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>> +void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState
>> *bs);
>> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>> Error **errp);
>> int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>> bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 02/12] qcow2/dirty-bitmap: cache loaded bitmaps
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 02/12] qcow2/dirty-bitmap: cache loaded bitmaps John Snow
@ 2018-05-14 12:33 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 37+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-05-14 12:33 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake
12.05.2018 04:25, John Snow wrote:
> For bitmaps that we succeeded in loading, we can cache a reference
> to that object. This will let us iterate over the more convenient
> form of in-memory bitmaps for qemu-img bitmap manipulation tools.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/qcow2-bitmap.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index fb0a4f3ec4..d89758f235 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -986,6 +986,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> if (bitmap == NULL) {
> goto fail;
> }
> + bm->dirty_bitmap = bitmap;
>
> if (!(bm->flags & BME_FLAG_AUTO)) {
> bdrv_disable_dirty_bitmap(bitmap);
Looks correct, I just suppose to reuse this information somehow in
qcow2_store_persistent_dirty_bitmaps, at least for an assert.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 03/12] block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 03/12] block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps John Snow
@ 2018-05-14 12:44 ` Vladimir Sementsov-Ogievskiy
2018-05-15 20:59 ` John Snow
0 siblings, 1 reply; 37+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-05-14 12:44 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake
12.05.2018 04:25, John Snow wrote:
> Instead of always setting IN_USE, handle whether or not the bitmap
> is read-only instead of a two-loop pass. This will allow us to show
> the flags exactly as they appear for bitmaps in `qemu-img info`.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/qcow2-bitmap.c | 48 ++++++++++++++++++++----------------------------
> 1 file changed, 20 insertions(+), 28 deletions(-)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index d89758f235..fc8d52fc3e 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -942,13 +942,6 @@ fail:
> return ret;
> }
>
> -/* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */
> -static void release_dirty_bitmap_helper(gpointer bitmap,
> - gpointer bs)
> -{
> - bdrv_release_dirty_bitmap(bs, bitmap);
> -}
> -
> /* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */
> static void set_readonly_helper(gpointer bitmap, gpointer value)
> {
> @@ -967,8 +960,8 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> BDRVQcow2State *s = bs->opaque;
> Qcow2BitmapList *bm_list;
> Qcow2Bitmap *bm;
> - GSList *created_dirty_bitmaps = NULL;
> bool header_updated = false;
> + bool needs_update = false;
>
> if (s->nb_bitmaps == 0) {
> /* No bitmaps - nothing to do */
> @@ -992,33 +985,32 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> bdrv_disable_dirty_bitmap(bitmap);
> }
> bdrv_dirty_bitmap_set_persistance(bitmap, true);
> - bm->flags |= BME_FLAG_IN_USE;
> - created_dirty_bitmaps =
> - g_slist_append(created_dirty_bitmaps, bitmap);
> - }
> - }
> -
> - if (created_dirty_bitmaps != NULL) {
> - if (can_write(bs)) {
> - /* in_use flags must be updated */
> - int ret = update_ext_header_and_dir_in_place(bs, bm_list);
> - if (ret < 0) {
> - error_setg_errno(errp, -ret, "Can't update bitmap directory");
> - goto fail;
> + if (can_write(bs)) {
> + bm->flags |= BME_FLAG_IN_USE;
> + needs_update = true;
> + } else {
> + bdrv_dirty_bitmap_set_readonly(bitmap, true);
oops. This shows, that patch 01 was incorrect: before this (03) patch,
in this case we'll have IN_USE set in cache but not in the image.
> }
> - header_updated = true;
> - } else {
> - g_slist_foreach(created_dirty_bitmaps, set_readonly_helper,
> - (gpointer)true);
> }
> }
>
> - g_slist_free(created_dirty_bitmaps);
> + /* in_use flags must be updated */
> + if (needs_update) {
> + int ret = update_ext_header_and_dir_in_place(bs, bm_list);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "Can't update bitmap directory");
> + goto fail;
> + }
> + header_updated = true;
> + }
> return header_updated;
may be safely changed to return needs_update, as if we failed to update,
we'll go to fail.
>
> fail:
> - g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs);
> - g_slist_free(created_dirty_bitmaps);
> + QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> + if (bm->dirty_bitmap) {
> + bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
> + }
> + }
> del_bitmap_list(bs);
>
> return false;
overall looks good, may be worth move it before 01.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 04/12] qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 04/12] qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO John Snow
@ 2018-05-14 12:55 ` Vladimir Sementsov-Ogievskiy
2018-05-15 20:52 ` John Snow
0 siblings, 1 reply; 37+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-05-14 12:55 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake
12.05.2018 04:25, John Snow wrote:
> For the purposes of qemu-img manipulation and querying of bitmaps, load
> bitmaps that are "in use" -- if the image is read only. This will allow
> us to diagnose problems with this flag using the qemu-img tool.
It looks unsafe.. We can load them, then reopen rw and then store them
on vm close, the result would be unpredictable. And they become
available for qmp commands..
If we want to load them, we may need some additional state for such
dirty bitmaps, something like "invalid", to be sure, that they will not
be used like normal bitmap. And we may
need some additional flag to load or not load them, and this behavior
will be unrelated to can_write().
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/qcow2-bitmap.c | 32 +++++++++++++++-----------------
> 1 file changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index fc8d52fc3e..b556dbdccd 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -346,7 +346,7 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
> uint32_t granularity;
> BdrvDirtyBitmap *bitmap = NULL;
>
> - if (bm->flags & BME_FLAG_IN_USE) {
> + if (can_write(bs) && (bm->flags & BME_FLAG_IN_USE)) {
> error_setg(errp, "Bitmap '%s' is in use", bm->name);
> goto fail;
> }
> @@ -974,23 +974,21 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> }
>
> QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> - if (!(bm->flags & BME_FLAG_IN_USE)) {
> - BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
> - if (bitmap == NULL) {
> - goto fail;
> - }
> - bm->dirty_bitmap = bitmap;
> + BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
> + if (bitmap == NULL) {
> + goto fail;
> + }
> + bm->dirty_bitmap = bitmap;
>
> - if (!(bm->flags & BME_FLAG_AUTO)) {
> - bdrv_disable_dirty_bitmap(bitmap);
> - }
> - bdrv_dirty_bitmap_set_persistance(bitmap, true);
> - if (can_write(bs)) {
> - bm->flags |= BME_FLAG_IN_USE;
> - needs_update = true;
> - } else {
> - bdrv_dirty_bitmap_set_readonly(bitmap, true);
> - }
> + if (!(bm->flags & BME_FLAG_AUTO)) {
> + bdrv_disable_dirty_bitmap(bitmap);
> + }
> + bdrv_dirty_bitmap_set_persistance(bitmap, true);
> + if (can_write(bs)) {
> + bm->flags |= BME_FLAG_IN_USE;
> + needs_update = true;
> + } else {
> + bdrv_dirty_bitmap_set_readonly(bitmap, true);
> }
> }
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 06/12] qapi: add bitmap info
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 06/12] qapi: add bitmap info John Snow
@ 2018-05-14 14:30 ` Vladimir Sementsov-Ogievskiy
2018-05-15 20:56 ` John Snow
2018-05-16 21:15 ` John Snow
0 siblings, 2 replies; 37+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-05-14 14:30 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake
12.05.2018 04:25, John Snow wrote:
> Add some of the necessary scaffolding for reporting bitmap information.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> qapi/block-core.json | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index c50517bff3..8f33f41ce7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -33,6 +33,61 @@
> 'date-sec': 'int', 'date-nsec': 'int',
> 'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
>
> +##
> +# @BitmapTypeEnum:
> +#
> +# An enumeration of possible serialized bitmap types.
> +#
> +# @dirty-tracking: This bitmap records information on dirty
> +# segments within the file.
> +#
> +# @unknown: This bitmap is of an unknown or reserved type.
> +#
> +# Since: 2.13
> +##
> +{ 'enum': 'BitmapTypeEnum', 'data': [ 'dirty-tracking', 'unknown' ] }
> +
> +##
> +# @BitmapFlagEnum:
> +#
> +# An enumeration of possible flags for serialized bitmaps.
> +#
> +# @in-use: This bitmap is considered to be in-use, and may now be inconsistent.
> +#
> +# @auto: This bitmap must reflect any and all changes to the file it describes.
> +# The type of this bitmap must be @DirtyTrackingBitmap.
logical, but I don't see this restriction in the spec. May be we need to
update the spec
> +#
> +# @extra-data-compatible: This bitmap has extra information associated with it.
no, this flag means, that extra data is compatible. So, if you don't
know what is this extra data, you can read and modify the bitmap,
leaving this data as is. If this flag is unset, and there are some extra
data, bitmap must not be used.
Finally, this spec should be consistent (or, may be better, duplicate)
spec from docs/interop/qcow2.txt..
> +#
> +# @unknown: This bitmap has unknown or reserved properties.
Better is only "reserved flags" (not unknown and not properties), they
are reserved by spec.
> +#
> +# Since: 2.13
> +##
> +{ 'enum': 'BitmapFlagEnum', 'data': [ 'in-use', 'auto',
> + 'extra-data-compatible', 'unknown' ] }
> +
> +##
> +# @BitmapInfo:
> +#
> +# @name: The name of the bitmap.
> +#
> +# @type: The type of bitmap.
> +#
> +# @granularity: Bitmap granularity, in bytes.
> +#
> +# @count: Overall bitmap dirtiness, in bytes.
> +#
> +# @flags: Bitmap flags, if any.
> +#
> +# Since: 2.13
> +#
> +##
> +{ 'struct': 'BitmapInfo',
> + 'data': { 'name': 'str', 'type': 'BitmapTypeEnum', 'granularity': 'int',
> + 'count': 'int', '*flags': ['BitmapFlagEnum']
may be worth add 'has-extra-data'
> + }
> +}
> +
> ##
> # @ImageInfoSpecificQCow2EncryptionBase:
> #
> @@ -69,6 +124,8 @@
> # @encrypt: details about encryption parameters; only set if image
> # is encrypted (since 2.10)
> #
> +# @bitmaps: list of image bitmaps (since 2.13)
> +#
> # Since: 1.7
> ##
> { 'struct': 'ImageInfoSpecificQCow2',
> @@ -77,7 +134,8 @@
> '*lazy-refcounts': 'bool',
> '*corrupt': 'bool',
> 'refcount-bits': 'int',
> - '*encrypt': 'ImageInfoSpecificQCow2Encryption'
> + '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> + '*bitmaps': ['BitmapInfo']
> } }
>
> ##
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 07/12] qcow2-bitmap: add basic bitmaps info
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 07/12] qcow2-bitmap: add basic bitmaps info John Snow
@ 2018-05-14 15:12 ` Vladimir Sementsov-Ogievskiy
2018-05-15 21:03 ` John Snow
2018-05-16 21:17 ` John Snow
0 siblings, 2 replies; 37+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-05-14 15:12 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Eric Blake
12.05.2018 04:25, John Snow wrote:
> Add functions for querying the basic information inside of bitmaps.
> Restructure the bitmaps flags masks to facilitate providing a list of
> flags belonging to the bitmap(s) being queried.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/qcow2-bitmap.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> block/qcow2.c | 7 +++++
> block/qcow2.h | 1 +
> 3 files changed, 87 insertions(+), 2 deletions(-)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 60e01abfd7..811b82743a 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -49,8 +49,28 @@
>
> /* Bitmap directory entry flags */
> #define BME_RESERVED_FLAGS 0xfffffffcU
> -#define BME_FLAG_IN_USE (1U << 0)
> -#define BME_FLAG_AUTO (1U << 1)
> +
> +enum BME_FLAG_BITS {
> + BME_FLAG_BIT__BEGIN = 0,
> + BME_FLAG_BIT_IN_USE = BME_FLAG_BIT__BEGIN,
> + BME_FLAG_BIT_AUTO = 1,
> + BME_FLAG_BIT_EXTRA = 2,
> + BME_FLAG_BIT__MAX,
> +};
> +
> +#define BME_FLAG_IN_USE (1U << BME_FLAG_BIT_IN_USE)
> +#define BME_FLAG_AUTO (1U << BME_FLAG_BIT_AUTO)
> +#define BME_FLAG_EXTRA (1U << BME_FLAG_BIT_EXTRA)
> +
> +/* Correlate canonical spec values to autogenerated QAPI values */
> +struct {
> + uint32_t mask;
mask is unused in this patch
> + int qapi_value;
> +} BMEFlagMap[BME_FLAG_BIT__MAX] = {
> + [BME_FLAG_BIT_IN_USE] = { BME_FLAG_IN_USE, BITMAP_FLAG_ENUM_IN_USE },
> + [BME_FLAG_BIT_AUTO] = { BME_FLAG_AUTO, BITMAP_FLAG_ENUM_AUTO },
> + [BME_FLAG_BIT_EXTRA] = { BME_FLAG_EXTRA, BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE },
> +};
>
> /* bits [1, 8] U [56, 63] are reserved */
> #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001feULL
> @@ -663,6 +683,63 @@ static void del_bitmap_list(BlockDriverState *bs)
> }
> }
>
> +static BitmapFlagEnumList *get_bitmap_flags(uint32_t flags)
> +{
> + int i;
> + BitmapFlagEnumList *flist = NULL;
> + BitmapFlagEnumList *ftmp;
> +
> + while (flags) {
> + i = ctz32(flags);
> + ftmp = g_new0(BitmapFlagEnumList, 1);
> + if (i >= BME_FLAG_BIT__BEGIN && i < BME_FLAG_BIT__MAX) {
> + ftmp->value = BMEFlagMap[i].qapi_value;
> + } else {
> + ftmp->value = BITMAP_FLAG_ENUM_UNKNOWN;
so, there may be several "unknown" entries. It's inconsistent with
"@unknown: This bitmap has unknown or reserved properties.".
Finally, can we export values for unknown flags? It should be more
informative.
> + }
> + ftmp->next = flist;
> + flist = ftmp;
> + flags &= ~(1 << i);
> + }
> +
> + return flist;
> +}
> +
> +BitmapInfoList *qcow2_get_bitmap_info(BlockDriverState *bs)
> +{
> + Qcow2BitmapList *bm_list;
> + Qcow2Bitmap *bm;
> + BitmapInfoList *info_list = NULL;
> + BitmapInfoList *tmp;
> + BitmapInfo *bitmap_info;
> +
> + bm_list = get_bitmap_list(bs, NULL);
> + if (!bm_list) {
> + return info_list;
> + }
> +
> + QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> + bitmap_info = g_new0(BitmapInfo, 1);
> + bitmap_info->name = g_strdup(bm->name);
> + if (bm->type == BT_DIRTY_TRACKING_BITMAP) {
> + bitmap_info->type = BITMAP_TYPE_ENUM_DIRTY_TRACKING;
> + } else {
> + bitmap_info->type = BITMAP_TYPE_ENUM_UNKNOWN;
> + }
> + bitmap_info->granularity = 1 << bm->granularity_bits;
> + bitmap_info->count = bdrv_get_dirty_count(bm->dirty_bitmap);
> + bitmap_info->flags = get_bitmap_flags(bm->flags);
> + bitmap_info->has_flags = !!bitmap_info->flags;
> +
> + tmp = g_new0(BitmapInfoList, 1);
> + tmp->value = bitmap_info;
> + tmp->next = info_list;
> + info_list = tmp;
> + }
> +
> + return info_list;
> +}
> +
> int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
> void **refcount_table,
> int64_t *refcount_table_size)
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7ae9000656..0b24ecb6b2 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3960,6 +3960,7 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
> BDRVQcow2State *s = bs->opaque;
> ImageInfoSpecific *spec_info;
> QCryptoBlockInfo *encrypt_info = NULL;
> + BitmapInfoList *bitmap_list = NULL;
>
> if (s->crypto != NULL) {
> encrypt_info = qcrypto_block_get_info(s->crypto, &error_abort);
> @@ -4016,6 +4017,12 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
> spec_info->u.qcow2.data->encrypt = qencrypt;
> }
>
> + bitmap_list = qcow2_get_bitmap_info(bs);
> + if (bitmap_list) {
> + spec_info->u.qcow2.data->has_bitmaps = true;
> + spec_info->u.qcow2.data->bitmaps = bitmap_list;
> + }
> +
> return spec_info;
> }
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 796a8c914b..76bf5fa55d 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -686,5 +686,6 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
> void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> const char *name,
> Error **errp);
> +BitmapInfoList *qcow2_get_bitmap_info(BlockDriverState *bs);
>
> #endif
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 01/12] qcow2-bitmap: cache bm_list
2018-05-14 11:55 ` Vladimir Sementsov-Ogievskiy
2018-05-14 12:15 ` Vladimir Sementsov-Ogievskiy
@ 2018-05-15 20:27 ` John Snow
1 sibling, 0 replies; 37+ messages in thread
From: John Snow @ 2018-05-15 20:27 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: Kevin Wolf, Markus Armbruster, Max Reitz
On 05/14/2018 07:55 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.05.2018 04:25, John Snow wrote:
>> We don't need to re-read this list every time, exactly. We can keep it
>> cached
>> and delete our copy when we flush to disk.
>
> Why not simply delete cache only on close (unconditionally)? Why do we
> need to remove it after flush?
>
I was being imprecise with my language again -- I'm doing it on
qcow2_inactivate, through qcow2_store_persistent_dirty_bitmaps.
I technically don't even need to destroy the cache *then*, I could just
do it unconditionally in qcow2_close.
I just felt it was "safer" to drop the cache after a store as a natural
point of synchronization, but it *should* be fine without.
> Actually, I think we need to remove it only in qcow2_inactive, after
> storing persistent bitmaps.
>
So would you prefer the unconditional drop on close, or a drop for every
inactivate? You still need a drop on close in that case, because we may
not call inactivate on close, and I am still trying to build the cache
for read only images, so to prevent leaks I need to clean that up.
>
>>
>> Because we don't try to flush bitmaps on close if there's nothing to
>> flush,
>> add a new conditional to delete the state anyway for a clean exit.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block/qcow2-bitmap.c | 74
>> ++++++++++++++++++++++++++++++++--------------------
>> block/qcow2.c | 2 ++
>> block/qcow2.h | 2 ++
>> 3 files changed, 50 insertions(+), 28 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 6e93ec43e1..fb0a4f3ec4 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -536,8 +536,7 @@ static uint32_t bitmap_list_count(Qcow2BitmapList
>> *bm_list)
>> * Get bitmap list from qcow2 image. Actually reads bitmap directory,
>> * checks it and convert to bitmap list.
>> */
>> -static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs,
>> uint64_t offset,
>> - uint64_t size, Error **errp)
>> +static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error
>> **errp)
>> {
>> int ret;
>> BDRVQcow2State *s = bs->opaque;
>> @@ -545,6 +544,8 @@ static Qcow2BitmapList
>> *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
>> Qcow2BitmapDirEntry *e;
>> uint32_t nb_dir_entries = 0;
>> Qcow2BitmapList *bm_list = NULL;
>> + uint64_t offset = s->bitmap_directory_offset;
>> + uint64_t size = s->bitmap_directory_size;
>
> Worth split this change as a refactoring patch (just remove extra
> parameters)?
>
Sure, this patch looks a little long.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 01/12] qcow2-bitmap: cache bm_list
2018-05-14 12:15 ` Vladimir Sementsov-Ogievskiy
@ 2018-05-15 20:38 ` John Snow
0 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2018-05-15 20:38 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: Kevin Wolf, Markus Armbruster, Max Reitz
On 05/14/2018 08:15 AM, Vladimir Sementsov-Ogievskiy wrote:
> 14.05.2018 14:55, Vladimir Sementsov-Ogievskiy wrote:
>> 12.05.2018 04:25, John Snow wrote:
>>> We don't need to re-read this list every time, exactly. We can keep
>>> it cached
>>> and delete our copy when we flush to disk.
>>
>> Why not simply delete cache only on close (unconditionally)? Why do we
>> need to remove it after flush?
>>
>> Actually, I think we need to remove it only in qcow2_inactive, after
>> storing persistent bitmaps.
>>
>>
>>>
>>> Because we don't try to flush bitmaps on close if there's nothing to
>>> flush,
>>> add a new conditional to delete the state anyway for a clean exit.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> block/qcow2-bitmap.c | 74
>>> ++++++++++++++++++++++++++++++++--------------------
>>> block/qcow2.c | 2 ++
>>> block/qcow2.h | 2 ++
>>> 3 files changed, 50 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>> index 6e93ec43e1..fb0a4f3ec4 100644
>>> --- a/block/qcow2-bitmap.c
>>> +++ b/block/qcow2-bitmap.c
>>> @@ -536,8 +536,7 @@ static uint32_t bitmap_list_count(Qcow2BitmapList
>>> *bm_list)
>>> * Get bitmap list from qcow2 image. Actually reads bitmap directory,
>>> * checks it and convert to bitmap list.
>>> */
>>> -static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs,
>>> uint64_t offset,
>>> - uint64_t size, Error **errp)
>>> +static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error
>>> **errp)
>>> {
>>> int ret;
>>> BDRVQcow2State *s = bs->opaque;
>>> @@ -545,6 +544,8 @@ static Qcow2BitmapList
>>> *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
>>> Qcow2BitmapDirEntry *e;
>>> uint32_t nb_dir_entries = 0;
>>> Qcow2BitmapList *bm_list = NULL;
>>> + uint64_t offset = s->bitmap_directory_offset;
>>> + uint64_t size = s->bitmap_directory_size;
>>
>> Worth split this change as a refactoring patch (just remove extra
>> parameters)?
>>
>>> if (size == 0) {
>>> error_setg(errp, "Requested bitmap directory size is zero");
>>> @@ -636,6 +637,30 @@ fail:
>>> return NULL;
>>> }
>>> +static Qcow2BitmapList *get_bitmap_list(BlockDriverState *bs,
>>> Error **errp)
>>> +{
>>> + BDRVQcow2State *s = bs->opaque;
>>> + Qcow2BitmapList *bm_list;
>>> +
>>> + if (s->bitmap_list) {
>>> + return (Qcow2BitmapList *)s->bitmap_list;
>>> + }
>>> +
>>> + bm_list = bitmap_list_load(bs, errp);
>>> + s->bitmap_list = bm_list;
>>> + return bm_list;
>>> +}
>>> +
>>> +static void del_bitmap_list(BlockDriverState *bs)
>>> +{
>>> + BDRVQcow2State *s = bs->opaque;
>>> +
>>> + if (s->bitmap_list) {
>>> + bitmap_list_free(s->bitmap_list);
>>> + s->bitmap_list = NULL;
>>> + }
>>> +}
>
> so, with this functions, we see, that list is always safely loaded
> through the cache. But we need also guarantee, that list is always saved
> through the cache. There are a lot of functions, which stores an
> abstract bitmap list, given as a parameter, but we want always store our
> cache..
>
Do you have an example?
These functions return a bitmap list:
bitmap_list_new()
bitmap_list_load(...);
get_bitmap_list(...);
bitmap_list_new creates a new bitmap_list, it's called by
bitmap_list_load and qcow2_store_persistent_dirty_bitmaps.
qcow2_store_persistent_dirty_bitmaps doesn't matter right now because we
drop the cache by the end of that function.
bitmap_list_load is only called by get_bitmap_list now, and
get_bitmap_list caches that bitmap list as s->bitmap_list.
I think any other function that takes a bitmap list must be getting it
from a location where it's cached -- except in store, which trashes it
anyway.
What I can do here is to make get_bitmap_list create a new bitmap list
if s->nb_bitmaps is zero, and remove the new call in store -- then we
can be really very confident that any bitmap list getting passed around
is definitely the cached one.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 04/12] qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO
2018-05-14 12:55 ` Vladimir Sementsov-Ogievskiy
@ 2018-05-15 20:52 ` John Snow
0 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2018-05-15 20:52 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: Kevin Wolf, Markus Armbruster, Max Reitz
On 05/14/2018 08:55 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.05.2018 04:25, John Snow wrote:
>> For the purposes of qemu-img manipulation and querying of bitmaps, load
>> bitmaps that are "in use" -- if the image is read only. This will allow
>> us to diagnose problems with this flag using the qemu-img tool.
>
> It looks unsafe.. We can load them, then reopen rw and then store them
> on vm close, the result would be unpredictable. And they become
> available for qmp commands..
>
> If we want to load them, we may need some additional state for such
> dirty bitmaps, something like "invalid", to be sure, that they will not
> be used like normal bitmap. And we may
> need some additional flag to load or not load them, and this behavior
> will be unrelated to can_write().
>
Yeah, you're right -- reopening is scary. I'll see what I can do.
The goal is just to get them in memory so that qemu-img can use the
standard BdrvDirtyBitmap API to print various info about them for the user.
Since 'info' and 'dump' don't need RW access, it'd be nicest for these
commands to work in RO mode, hence the patch.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 06/12] qapi: add bitmap info
2018-05-14 14:30 ` Vladimir Sementsov-Ogievskiy
@ 2018-05-15 20:56 ` John Snow
2018-05-16 21:15 ` John Snow
1 sibling, 0 replies; 37+ messages in thread
From: John Snow @ 2018-05-15 20:56 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: Kevin Wolf, Markus Armbruster, Max Reitz
On 05/14/2018 10:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.05.2018 04:25, John Snow wrote:
>> Add some of the necessary scaffolding for reporting bitmap information.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> qapi/block-core.json | 60
>> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index c50517bff3..8f33f41ce7 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -33,6 +33,61 @@
>> 'date-sec': 'int', 'date-nsec': 'int',
>> 'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
>> +##
>> +# @BitmapTypeEnum:
>> +#
>> +# An enumeration of possible serialized bitmap types.
>> +#
>> +# @dirty-tracking: This bitmap records information on dirty
>> +# segments within the file.
>> +#
>> +# @unknown: This bitmap is of an unknown or reserved type.
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'enum': 'BitmapTypeEnum', 'data': [ 'dirty-tracking', 'unknown' ] }
>> +
>> +##
>> +# @BitmapFlagEnum:
>> +#
>> +# An enumeration of possible flags for serialized bitmaps.
>> +#
>> +# @in-use: This bitmap is considered to be in-use, and may now be
>> inconsistent.
>> +#
>> +# @auto: This bitmap must reflect any and all changes to the file it
>> describes.
>> +# The type of this bitmap must be @DirtyTrackingBitmap.
>
> logical, but I don't see this restriction in the spec. May be we need to
> update the spec
>
Where did I even get this idea?
>> +#
>> +# @extra-data-compatible: This bitmap has extra information
>> associated with it.
>
> no, this flag means, that extra data is compatible. So, if you don't
> know what is this extra data, you can read and modify the bitmap,
> leaving this data as is. If this flag is unset, and there are some extra
> data, bitmap must not be used.
>
Oh, understood! Thank you for clearing that up.
> Finally, this spec should be consistent (or, may be better, duplicate)
> spec from docs/interop/qcow2.txt..
*cough* I guess if I get enough things wrong it's fair to say "RTFM!"
>> +#
>> +# @unknown: This bitmap has unknown or reserved properties.
>
> Better is only "reserved flags" (not unknown and not properties), they
> are reserved by spec.
>
Yeah, I'll take that stance.
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'enum': 'BitmapFlagEnum', 'data': [ 'in-use', 'auto',
>> + 'extra-data-compatible',
>> 'unknown' ] }
>> +
>> +##
>> +# @BitmapInfo:
>> +#
>> +# @name: The name of the bitmap.
>> +#
>> +# @type: The type of bitmap.
>> +#
>> +# @granularity: Bitmap granularity, in bytes.
>> +#
>> +# @count: Overall bitmap dirtiness, in bytes.
>> +#
>> +# @flags: Bitmap flags, if any.
>> +#
>> +# Since: 2.13
>> +#
>> +##
>> +{ 'struct': 'BitmapInfo',
>> + 'data': { 'name': 'str', 'type': 'BitmapTypeEnum', 'granularity':
>> 'int',
>> + 'count': 'int', '*flags': ['BitmapFlagEnum']
>
> may be worth add 'has-extra-data'
>
Since you pointed out the difference, I agree.
>> + }
>> +}
>> +
>> ##
>> # @ImageInfoSpecificQCow2EncryptionBase:
>> #
>> @@ -69,6 +124,8 @@
>> # @encrypt: details about encryption parameters; only set if image
>> # is encrypted (since 2.10)
>> #
>> +# @bitmaps: list of image bitmaps (since 2.13)
>> +#
>> # Since: 1.7
>> ##
>> { 'struct': 'ImageInfoSpecificQCow2',
>> @@ -77,7 +134,8 @@
>> '*lazy-refcounts': 'bool',
>> '*corrupt': 'bool',
>> 'refcount-bits': 'int',
>> - '*encrypt': 'ImageInfoSpecificQCow2Encryption'
>> + '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>> + '*bitmaps': ['BitmapInfo']
>> } }
>> ##
>
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 03/12] block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps
2018-05-14 12:44 ` Vladimir Sementsov-Ogievskiy
@ 2018-05-15 20:59 ` John Snow
0 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2018-05-15 20:59 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: Kevin Wolf, Markus Armbruster, Max Reitz
On 05/14/2018 08:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.05.2018 04:25, John Snow wrote:
>> Instead of always setting IN_USE, handle whether or not the bitmap
>> is read-only instead of a two-loop pass. This will allow us to show
>> the flags exactly as they appear for bitmaps in `qemu-img info`.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block/qcow2-bitmap.c | 48
>> ++++++++++++++++++++----------------------------
>> 1 file changed, 20 insertions(+), 28 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index d89758f235..fc8d52fc3e 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -942,13 +942,6 @@ fail:
>> return ret;
>> }
>> -/* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */
>> -static void release_dirty_bitmap_helper(gpointer bitmap,
>> - gpointer bs)
>> -{
>> - bdrv_release_dirty_bitmap(bs, bitmap);
>> -}
>> -
>> /* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */
>> static void set_readonly_helper(gpointer bitmap, gpointer value)
>> {
>> @@ -967,8 +960,8 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState
>> *bs, Error **errp)
>> BDRVQcow2State *s = bs->opaque;
>> Qcow2BitmapList *bm_list;
>> Qcow2Bitmap *bm;
>> - GSList *created_dirty_bitmaps = NULL;
>> bool header_updated = false;
>> + bool needs_update = false;
>> if (s->nb_bitmaps == 0) {
>> /* No bitmaps - nothing to do */
>> @@ -992,33 +985,32 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState
>> *bs, Error **errp)
>> bdrv_disable_dirty_bitmap(bitmap);
>> }
>> bdrv_dirty_bitmap_set_persistance(bitmap, true);
>> - bm->flags |= BME_FLAG_IN_USE;
>> - created_dirty_bitmaps =
>> - g_slist_append(created_dirty_bitmaps, bitmap);
>> - }
>> - }
>> -
>> - if (created_dirty_bitmaps != NULL) {
>> - if (can_write(bs)) {
>> - /* in_use flags must be updated */
>> - int ret = update_ext_header_and_dir_in_place(bs, bm_list);
>> - if (ret < 0) {
>> - error_setg_errno(errp, -ret, "Can't update bitmap
>> directory");
>> - goto fail;
>> + if (can_write(bs)) {
>> + bm->flags |= BME_FLAG_IN_USE;
>> + needs_update = true;
>> + } else {
>> + bdrv_dirty_bitmap_set_readonly(bitmap, true);
>
> oops. This shows, that patch 01 was incorrect: before this (03) patch,
> in this case we'll have IN_USE set in cache but not in the image.
>
Ah, I can try to order this patch first so that the cache stays
semantically correct.
>> }
>> - header_updated = true;
>> - } else {
>> - g_slist_foreach(created_dirty_bitmaps, set_readonly_helper,
>> - (gpointer)true);
>> }
>> }
>> - g_slist_free(created_dirty_bitmaps);
>> + /* in_use flags must be updated */
>> + if (needs_update) {
>> + int ret = update_ext_header_and_dir_in_place(bs, bm_list);
>> + if (ret < 0) {
>> + error_setg_errno(errp, -ret, "Can't update bitmap
>> directory");
>> + goto fail;
>> + }
>> + header_updated = true;
>> + }
>> return header_updated;
>
> may be safely changed to return needs_update, as if we failed to update,
> we'll go to fail.
>
Ugh, yes.
>> fail:
>> - g_slist_foreach(created_dirty_bitmaps,
>> release_dirty_bitmap_helper, bs);
>> - g_slist_free(created_dirty_bitmaps);
>> + QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>> + if (bm->dirty_bitmap) {
>> + bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
>> + }
>> + }
>> del_bitmap_list(bs);
>> return false;
>
> overall looks good, may be worth move it before 01.
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 07/12] qcow2-bitmap: add basic bitmaps info
2018-05-14 15:12 ` Vladimir Sementsov-Ogievskiy
@ 2018-05-15 21:03 ` John Snow
2018-05-16 21:17 ` John Snow
1 sibling, 0 replies; 37+ messages in thread
From: John Snow @ 2018-05-15 21:03 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: Kevin Wolf, Markus Armbruster, Max Reitz
On 05/14/2018 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.05.2018 04:25, John Snow wrote:
>> Add functions for querying the basic information inside of bitmaps.
>> Restructure the bitmaps flags masks to facilitate providing a list of
>> flags belonging to the bitmap(s) being queried.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block/qcow2-bitmap.c | 81
>> ++++++++++++++++++++++++++++++++++++++++++++++++++--
>> block/qcow2.c | 7 +++++
>> block/qcow2.h | 1 +
>> 3 files changed, 87 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 60e01abfd7..811b82743a 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -49,8 +49,28 @@
>> /* Bitmap directory entry flags */
>> #define BME_RESERVED_FLAGS 0xfffffffcU
>> -#define BME_FLAG_IN_USE (1U << 0)
>> -#define BME_FLAG_AUTO (1U << 1)
>> +
>> +enum BME_FLAG_BITS {
>> + BME_FLAG_BIT__BEGIN = 0,
>> + BME_FLAG_BIT_IN_USE = BME_FLAG_BIT__BEGIN,
>> + BME_FLAG_BIT_AUTO = 1,
>> + BME_FLAG_BIT_EXTRA = 2,
>> + BME_FLAG_BIT__MAX,
>> +};
>> +
>> +#define BME_FLAG_IN_USE (1U << BME_FLAG_BIT_IN_USE)
>> +#define BME_FLAG_AUTO (1U << BME_FLAG_BIT_AUTO)
>> +#define BME_FLAG_EXTRA (1U << BME_FLAG_BIT_EXTRA)
>> +
>> +/* Correlate canonical spec values to autogenerated QAPI values */
>> +struct {
>> + uint32_t mask;
>
> mask is unused in this patch
>
It sure is! I'll remove it. It's an artifact from an earlier revision.
>> + int qapi_value;
>> +} BMEFlagMap[BME_FLAG_BIT__MAX] = {
>> + [BME_FLAG_BIT_IN_USE] = { BME_FLAG_IN_USE,
>> BITMAP_FLAG_ENUM_IN_USE },
>> + [BME_FLAG_BIT_AUTO] = { BME_FLAG_AUTO, BITMAP_FLAG_ENUM_AUTO },
>> + [BME_FLAG_BIT_EXTRA] = { BME_FLAG_EXTRA,
>> BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE },
>> +};
>> /* bits [1, 8] U [56, 63] are reserved */
>> #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001feULL
>> @@ -663,6 +683,63 @@ static void del_bitmap_list(BlockDriverState *bs)
>> }
>> }
>> +static BitmapFlagEnumList *get_bitmap_flags(uint32_t flags)
>> +{
>> + int i;
>> + BitmapFlagEnumList *flist = NULL;
>> + BitmapFlagEnumList *ftmp;
>> +
>> + while (flags) {
>> + i = ctz32(flags);
>> + ftmp = g_new0(BitmapFlagEnumList, 1);
>> + if (i >= BME_FLAG_BIT__BEGIN && i < BME_FLAG_BIT__MAX) {
>> + ftmp->value = BMEFlagMap[i].qapi_value;
>> + } else {
>> + ftmp->value = BITMAP_FLAG_ENUM_UNKNOWN;
>
> so, there may be several "unknown" entries. It's inconsistent with
> "@unknown: This bitmap has unknown or reserved properties.".
>
Based on your feedback from the previous patch I'll probably just call
this @reserved for any unrecognized flag.
> Finally, can we export values for unknown flags? It should be more
> informative.
>
I'll see what I can do -- I hate losing this information too.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 06/12] qapi: add bitmap info
2018-05-14 14:30 ` Vladimir Sementsov-Ogievskiy
2018-05-15 20:56 ` John Snow
@ 2018-05-16 21:15 ` John Snow
2018-05-17 10:01 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 37+ messages in thread
From: John Snow @ 2018-05-16 21:15 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: Kevin Wolf, Markus Armbruster, Max Reitz
On 05/14/2018 10:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.05.2018 04:25, John Snow wrote:
>> Add some of the necessary scaffolding for reporting bitmap information.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> qapi/block-core.json | 60
>> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index c50517bff3..8f33f41ce7 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -33,6 +33,61 @@
>> 'date-sec': 'int', 'date-nsec': 'int',
>> 'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
>> +##
>> +# @BitmapTypeEnum:
>> +#
>> +# An enumeration of possible serialized bitmap types.
>> +#
>> +# @dirty-tracking: This bitmap records information on dirty
>> +# segments within the file.
>> +#
>> +# @unknown: This bitmap is of an unknown or reserved type.
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'enum': 'BitmapTypeEnum', 'data': [ 'dirty-tracking', 'unknown' ] }
>> +
>> +##
>> +# @BitmapFlagEnum:
>> +#
>> +# An enumeration of possible flags for serialized bitmaps.
>> +#
>> +# @in-use: This bitmap is considered to be in-use, and may now be
>> inconsistent.
>> +#
>> +# @auto: This bitmap must reflect any and all changes to the file it
>> describes.
>> +# The type of this bitmap must be @DirtyTrackingBitmap.
>
> logical, but I don't see this restriction in the spec. May be we need to
> update the spec
>
1: auto
"The bitmap must reflect all changes of the virtual disk by any
application that would write to this qcow2 file (including writes,
snapshot switching, etc.). The type of this bitmap must be 'dirty
tracking bitmap'."
Actually, this looks correct now that I'm looking at the spec again.
I've used a terser phrasing but I think it's correct.
>> +#
>> +# @extra-data-compatible: This bitmap has extra information
>> associated with it.
>
> no, this flag means, that extra data is compatible. So, if you don't
> know what is this extra data, you can read and modify the bitmap,
> leaving this data as is. If this flag is unset, and there are some extra
> data, bitmap must not be used.
>
> Finally, this spec should be consistent (or, may be better, duplicate)
> spec from docs/interop/qcow2.txt..
I might suggest a rewrite of this portion of the spec as it's a little
unclear to me.
I've given this portion a rewrite.
>> +#
>> +# @unknown: This bitmap has unknown or reserved properties.
>
> Better is only "reserved flags" (not unknown and not properties), they
> are reserved by spec.
>
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'enum': 'BitmapFlagEnum', 'data': [ 'in-use', 'auto',
>> + 'extra-data-compatible',
>> 'unknown' ] }
>> +
>> +##
>> +# @BitmapInfo:
>> +#
>> +# @name: The name of the bitmap.
>> +#
>> +# @type: The type of bitmap.
>> +#
>> +# @granularity: Bitmap granularity, in bytes.
>> +#
>> +# @count: Overall bitmap dirtiness, in bytes.
>> +#
>> +# @flags: Bitmap flags, if any.
>> +#
>> +# Since: 2.13
>> +#
>> +##
>> +{ 'struct': 'BitmapInfo',
>> + 'data': { 'name': 'str', 'type': 'BitmapTypeEnum', 'granularity':
>> 'int',
>> + 'count': 'int', '*flags': ['BitmapFlagEnum']
>
> may be worth add 'has-extra-data'
>
>> + }
>> +}
>> +
>> ##
>> # @ImageInfoSpecificQCow2EncryptionBase:
>> #
>> @@ -69,6 +124,8 @@
>> # @encrypt: details about encryption parameters; only set if image
>> # is encrypted (since 2.10)
>> #
>> +# @bitmaps: list of image bitmaps (since 2.13)
>> +#
>> # Since: 1.7
>> ##
>> { 'struct': 'ImageInfoSpecificQCow2',
>> @@ -77,7 +134,8 @@
>> '*lazy-refcounts': 'bool',
>> '*corrupt': 'bool',
>> 'refcount-bits': 'int',
>> - '*encrypt': 'ImageInfoSpecificQCow2Encryption'
>> + '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>> + '*bitmaps': ['BitmapInfo']
>> } }
>> ##
>
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 07/12] qcow2-bitmap: add basic bitmaps info
2018-05-14 15:12 ` Vladimir Sementsov-Ogievskiy
2018-05-15 21:03 ` John Snow
@ 2018-05-16 21:17 ` John Snow
2018-05-17 10:03 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 37+ messages in thread
From: John Snow @ 2018-05-16 21:17 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: Kevin Wolf, Markus Armbruster, Max Reitz
On 05/14/2018 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> 12.05.2018 04:25, John Snow wrote:
>> Add functions for querying the basic information inside of bitmaps.
>> Restructure the bitmaps flags masks to facilitate providing a list of
>> flags belonging to the bitmap(s) being queried.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> block/qcow2-bitmap.c | 81
>> ++++++++++++++++++++++++++++++++++++++++++++++++++--
>> block/qcow2.c | 7 +++++
>> block/qcow2.h | 1 +
>> 3 files changed, 87 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 60e01abfd7..811b82743a 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -49,8 +49,28 @@
>> /* Bitmap directory entry flags */
>> #define BME_RESERVED_FLAGS 0xfffffffcU
>> -#define BME_FLAG_IN_USE (1U << 0)
>> -#define BME_FLAG_AUTO (1U << 1)
>> +
>> +enum BME_FLAG_BITS {
>> + BME_FLAG_BIT__BEGIN = 0,
>> + BME_FLAG_BIT_IN_USE = BME_FLAG_BIT__BEGIN,
>> + BME_FLAG_BIT_AUTO = 1,
>> + BME_FLAG_BIT_EXTRA = 2,
>> + BME_FLAG_BIT__MAX,
>> +};
>> +
>> +#define BME_FLAG_IN_USE (1U << BME_FLAG_BIT_IN_USE)
>> +#define BME_FLAG_AUTO (1U << BME_FLAG_BIT_AUTO)
>> +#define BME_FLAG_EXTRA (1U << BME_FLAG_BIT_EXTRA)
>> +
>> +/* Correlate canonical spec values to autogenerated QAPI values */
>> +struct {
>> + uint32_t mask;
>
> mask is unused in this patch
>
>> + int qapi_value;
>> +} BMEFlagMap[BME_FLAG_BIT__MAX] = {
>> + [BME_FLAG_BIT_IN_USE] = { BME_FLAG_IN_USE,
>> BITMAP_FLAG_ENUM_IN_USE },
>> + [BME_FLAG_BIT_AUTO] = { BME_FLAG_AUTO, BITMAP_FLAG_ENUM_AUTO },
>> + [BME_FLAG_BIT_EXTRA] = { BME_FLAG_EXTRA,
>> BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE },
>> +};
>> /* bits [1, 8] U [56, 63] are reserved */
>> #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001feULL
>> @@ -663,6 +683,63 @@ static void del_bitmap_list(BlockDriverState *bs)
>> }
>> }
>> +static BitmapFlagEnumList *get_bitmap_flags(uint32_t flags)
>> +{
>> + int i;
>> + BitmapFlagEnumList *flist = NULL;
>> + BitmapFlagEnumList *ftmp;
>> +
>> + while (flags) {
>> + i = ctz32(flags);
>> + ftmp = g_new0(BitmapFlagEnumList, 1);
>> + if (i >= BME_FLAG_BIT__BEGIN && i < BME_FLAG_BIT__MAX) {
>> + ftmp->value = BMEFlagMap[i].qapi_value;
>> + } else {
>> + ftmp->value = BITMAP_FLAG_ENUM_UNKNOWN;
>
> so, there may be several "unknown" entries. It's inconsistent with
> "@unknown: This bitmap has unknown or reserved properties.".
>
> Finally, can we export values for unknown flags? It should be more
> informative.
>
I changed my mind -- qemu-img is not a debugging tool and shouldn't get
lost in the weeds trying to enumerate the different kinds of reserved
values that are set.
It's an error to have them set, or not -- which ones specifically is not
information that an end-user need know or care about, and I don't want
to create an extra structure to contain the value.
I'm going to rewrite this loop to just only ever have one unknown value
at a maximum.
--js
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 08/12] qjson: allow caller to ask for arbitrary indent
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 08/12] qjson: allow caller to ask for arbitrary indent John Snow
@ 2018-05-16 21:34 ` Eric Blake
2018-05-16 21:49 ` John Snow
0 siblings, 1 reply; 37+ messages in thread
From: Eric Blake @ 2018-05-16 21:34 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Vladimir Sementsov-Ogievskiy
On 05/11/2018 08:25 PM, John Snow wrote:
> The function already exists, just expose it.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> include/qapi/qmp/qjson.h | 1 +
> qobject/qjson.c | 21 +++++++++++----------
> 2 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
> index b274ac3a86..0e8624c1fa 100644
> --- a/include/qapi/qmp/qjson.h
> +++ b/include/qapi/qmp/qjson.h
> @@ -19,6 +19,7 @@ QObject *qobject_from_jsonf(const char *string, ...) GCC_FMT_ATTR(1, 2);
> QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
> GCC_FMT_ATTR(1, 0);
>
> +QString *qobject_to_json_opt(const QObject *obj, int pretty, int indent);
pretty seems like it is better as a bool. Even though to_json() isn't
typed correctly, that's no excuse for the public function to copy a bad
interface (and maybe it's worth a separate cleanup patch to fix
to_json() first).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 09/12] qapi/block-core: add BitmapMapping and BitmapEntry structs
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 09/12] qapi/block-core: add BitmapMapping and BitmapEntry structs John Snow
@ 2018-05-16 21:37 ` Eric Blake
2018-05-16 21:55 ` John Snow
0 siblings, 1 reply; 37+ messages in thread
From: Eric Blake @ 2018-05-16 21:37 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Max Reitz, Markus Armbruster, Kevin Wolf, Vladimir Sementsov-Ogievskiy
On 05/11/2018 08:25 PM, John Snow wrote:
> Add two new structures for detailing the marked regions of bitmaps as
> saved in e.g. qcow2 files.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> qapi/block-core.json | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 8f33f41ce7..de8ad73a78 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -298,6 +298,38 @@
> 'zero': 'bool', 'depth': 'int', '*offset': 'int',
> '*filename': 'str' } }
>
> +##
> +# @BitmapEntry:
> +#
> +# Dirty Bitmap region information for a virtual block range
> +#
> +# @offset: the start byte of the dirty virtual range
> +#
> +# @length: the number of bytes of the dirty virtual range
> +#
> +# Since: 2.13
> +#
> +##
> +{ 'struct': 'BitmapEntry',
> + 'data': { 'offset': 'int', 'length': 'int' } }
> +
> +##
> +# @BitmapMapping:
> +#
> +# List of described regions correlated to a named bitmap.
> +#
> +# @name: The name of the bitmap whose range is described here
> +#
> +# @entries: A list of zero or more @BitmapEntry elements representing
> +# the range(s) described by the bitmap.
Is it also worth documenting that the list will be in ascending order,
with no overlaps (no two entries covering the same offset); and in fact
with a gap between all entries (as otherwise those two consecutive
entries could have been consolidated to one)?
> +#
> +# Since: 2.13
> +#
> +##
> +{ 'struct': 'BitmapMapping',
> + 'data': { 'name': 'str',
> + 'entries': [ 'BitmapEntry' ] } }
> +
> ##
> # @BlockdevCacheInfo:
> #
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 08/12] qjson: allow caller to ask for arbitrary indent
2018-05-16 21:34 ` Eric Blake
@ 2018-05-16 21:49 ` John Snow
0 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2018-05-16 21:49 UTC (permalink / raw)
To: Eric Blake, qemu-block, qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Markus Armbruster, Max Reitz
On 05/16/2018 05:34 PM, Eric Blake wrote:
> On 05/11/2018 08:25 PM, John Snow wrote:
>> The function already exists, just expose it.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> include/qapi/qmp/qjson.h | 1 +
>> qobject/qjson.c | 21 +++++++++++----------
>> 2 files changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
>> index b274ac3a86..0e8624c1fa 100644
>> --- a/include/qapi/qmp/qjson.h
>> +++ b/include/qapi/qmp/qjson.h
>> @@ -19,6 +19,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
>> GCC_FMT_ATTR(1, 2);
>> QObject *qobject_from_jsonv(const char *string, va_list *ap, Error
>> **errp)
>> GCC_FMT_ATTR(1, 0);
>> +QString *qobject_to_json_opt(const QObject *obj, int pretty, int
>> indent);
>
> pretty seems like it is better as a bool. Even though to_json() isn't
> typed correctly, that's no excuse for the public function to copy a bad
> interface (and maybe it's worth a separate cleanup patch to fix
> to_json() first).
>
Eh, sure, why not. Done.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 09/12] qapi/block-core: add BitmapMapping and BitmapEntry structs
2018-05-16 21:37 ` Eric Blake
@ 2018-05-16 21:55 ` John Snow
0 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2018-05-16 21:55 UTC (permalink / raw)
To: Eric Blake, qemu-block, qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Markus Armbruster, Max Reitz
On 05/16/2018 05:37 PM, Eric Blake wrote:
>> +##
>> +# @BitmapMapping:
>> +#
>> +# List of described regions correlated to a named bitmap.
>> +#
>> +# @name: The name of the bitmap whose range is described here
>> +#
>> +# @entries: A list of zero or more @BitmapEntry elements representing
>> +# the range(s) described by the bitmap.
>
> Is it also worth documenting that the list will be in ascending order,
> with no overlaps (no two entries covering the same offset); and in fact
> with a gap between all entries (as otherwise those two consecutive
> entries could have been consolidated to one)?
>
Hm. I didn't necessarily want to guarantee the order, but being more
specific about the output will assist legibility of the spec.
I'll amend the documentation and make some stronger guarantees.
>> +#
>> +# Since: 2.13
>> +#
>> +##
>> +{ 'struct': 'BitmapMapping',
>> + 'data': { 'name': 'str',
>> + 'entries': [ 'BitmapEntry' ] } }
>> +
>> ##
>> # @BlockdevCacheInfo:
>> #
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 06/12] qapi: add bitmap info
2018-05-16 21:15 ` John Snow
@ 2018-05-17 10:01 ` Vladimir Sementsov-Ogievskiy
2018-05-17 16:43 ` John Snow
0 siblings, 1 reply; 37+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-05-17 10:01 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Kevin Wolf, Markus Armbruster, Max Reitz
17.05.2018 00:15, John Snow wrote:
>
> On 05/14/2018 10:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 12.05.2018 04:25, John Snow wrote:
>>> Add some of the necessary scaffolding for reporting bitmap information.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> qapi/block-core.json | 60
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 59 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index c50517bff3..8f33f41ce7 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -33,6 +33,61 @@
>>> 'date-sec': 'int', 'date-nsec': 'int',
>>> 'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
>>> +##
>>> +# @BitmapTypeEnum:
>>> +#
>>> +# An enumeration of possible serialized bitmap types.
>>> +#
>>> +# @dirty-tracking: This bitmap records information on dirty
>>> +# segments within the file.
>>> +#
>>> +# @unknown: This bitmap is of an unknown or reserved type.
>>> +#
>>> +# Since: 2.13
>>> +##
>>> +{ 'enum': 'BitmapTypeEnum', 'data': [ 'dirty-tracking', 'unknown' ] }
>>> +
>>> +##
>>> +# @BitmapFlagEnum:
>>> +#
>>> +# An enumeration of possible flags for serialized bitmaps.
>>> +#
>>> +# @in-use: This bitmap is considered to be in-use, and may now be
>>> inconsistent.
>>> +#
>>> +# @auto: This bitmap must reflect any and all changes to the file it
>>> describes.
>>> +# The type of this bitmap must be @DirtyTrackingBitmap.
>> logical, but I don't see this restriction in the spec. May be we need to
>> update the spec
>>
> 1: auto
>
> "The bitmap must reflect all changes of the virtual disk by any
> application that would write to this qcow2 file (including writes,
> snapshot switching, etc.). The type of this bitmap must be 'dirty
> tracking bitmap'."
>
> Actually, this looks correct now that I'm looking at the spec again.
> I've used a terser phrasing but I think it's correct.
another thought: why must? We know nothing about other types.. May be
for other type this flag will have similar or other meaning.. For me,
this flag looks like a property of dirty-tracking bitmap, not the thing
which dictates only that type.
>
>>> +#
>>> +# @extra-data-compatible: This bitmap has extra information
>>> associated with it.
>> no, this flag means, that extra data is compatible. So, if you don't
>> know what is this extra data, you can read and modify the bitmap,
>> leaving this data as is. If this flag is unset, and there are some extra
>> data, bitmap must not be used.
>>
>> Finally, this spec should be consistent (or, may be better, duplicate)
>> spec from docs/interop/qcow2.txt..
> I might suggest a rewrite of this portion of the spec as it's a little
> unclear to me.
>
> I've given this portion a rewrite.
>
>>> +#
>>> +# @unknown: This bitmap has unknown or reserved properties.
>> Better is only "reserved flags" (not unknown and not properties), they
>> are reserved by spec.
>>
>>> +#
>>> +# Since: 2.13
>>> +##
>>> +{ 'enum': 'BitmapFlagEnum', 'data': [ 'in-use', 'auto',
>>> + 'extra-data-compatible',
>>> 'unknown' ] }
>>> +
>>> +##
>>> +# @BitmapInfo:
>>> +#
>>> +# @name: The name of the bitmap.
>>> +#
>>> +# @type: The type of bitmap.
>>> +#
>>> +# @granularity: Bitmap granularity, in bytes.
>>> +#
>>> +# @count: Overall bitmap dirtiness, in bytes.
>>> +#
>>> +# @flags: Bitmap flags, if any.
>>> +#
>>> +# Since: 2.13
>>> +#
>>> +##
>>> +{ 'struct': 'BitmapInfo',
>>> + 'data': { 'name': 'str', 'type': 'BitmapTypeEnum', 'granularity':
>>> 'int',
>>> + 'count': 'int', '*flags': ['BitmapFlagEnum']
>> may be worth add 'has-extra-data'
>>
>>> + }
>>> +}
>>> +
>>> ##
>>> # @ImageInfoSpecificQCow2EncryptionBase:
>>> #
>>> @@ -69,6 +124,8 @@
>>> # @encrypt: details about encryption parameters; only set if image
>>> # is encrypted (since 2.10)
>>> #
>>> +# @bitmaps: list of image bitmaps (since 2.13)
>>> +#
>>> # Since: 1.7
>>> ##
>>> { 'struct': 'ImageInfoSpecificQCow2',
>>> @@ -77,7 +134,8 @@
>>> '*lazy-refcounts': 'bool',
>>> '*corrupt': 'bool',
>>> 'refcount-bits': 'int',
>>> - '*encrypt': 'ImageInfoSpecificQCow2Encryption'
>>> + '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>>> + '*bitmaps': ['BitmapInfo']
>>> } }
>>> ##
>>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 07/12] qcow2-bitmap: add basic bitmaps info
2018-05-16 21:17 ` John Snow
@ 2018-05-17 10:03 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 37+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-05-17 10:03 UTC (permalink / raw)
To: John Snow, qemu-block, qemu-devel
Cc: Kevin Wolf, Markus Armbruster, Max Reitz
17.05.2018 00:17, John Snow wrote:
>
> On 05/14/2018 11:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 12.05.2018 04:25, John Snow wrote:
>>> Add functions for querying the basic information inside of bitmaps.
>>> Restructure the bitmaps flags masks to facilitate providing a list of
>>> flags belonging to the bitmap(s) being queried.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> block/qcow2-bitmap.c | 81
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>> block/qcow2.c | 7 +++++
>>> block/qcow2.h | 1 +
>>> 3 files changed, 87 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>> index 60e01abfd7..811b82743a 100644
>>> --- a/block/qcow2-bitmap.c
>>> +++ b/block/qcow2-bitmap.c
>>> @@ -49,8 +49,28 @@
>>> /* Bitmap directory entry flags */
>>> #define BME_RESERVED_FLAGS 0xfffffffcU
>>> -#define BME_FLAG_IN_USE (1U << 0)
>>> -#define BME_FLAG_AUTO (1U << 1)
>>> +
>>> +enum BME_FLAG_BITS {
>>> + BME_FLAG_BIT__BEGIN = 0,
>>> + BME_FLAG_BIT_IN_USE = BME_FLAG_BIT__BEGIN,
>>> + BME_FLAG_BIT_AUTO = 1,
>>> + BME_FLAG_BIT_EXTRA = 2,
>>> + BME_FLAG_BIT__MAX,
>>> +};
>>> +
>>> +#define BME_FLAG_IN_USE (1U << BME_FLAG_BIT_IN_USE)
>>> +#define BME_FLAG_AUTO (1U << BME_FLAG_BIT_AUTO)
>>> +#define BME_FLAG_EXTRA (1U << BME_FLAG_BIT_EXTRA)
>>> +
>>> +/* Correlate canonical spec values to autogenerated QAPI values */
>>> +struct {
>>> + uint32_t mask;
>> mask is unused in this patch
>>
>>> + int qapi_value;
>>> +} BMEFlagMap[BME_FLAG_BIT__MAX] = {
>>> + [BME_FLAG_BIT_IN_USE] = { BME_FLAG_IN_USE,
>>> BITMAP_FLAG_ENUM_IN_USE },
>>> + [BME_FLAG_BIT_AUTO] = { BME_FLAG_AUTO, BITMAP_FLAG_ENUM_AUTO },
>>> + [BME_FLAG_BIT_EXTRA] = { BME_FLAG_EXTRA,
>>> BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE },
>>> +};
>>> /* bits [1, 8] U [56, 63] are reserved */
>>> #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001feULL
>>> @@ -663,6 +683,63 @@ static void del_bitmap_list(BlockDriverState *bs)
>>> }
>>> }
>>> +static BitmapFlagEnumList *get_bitmap_flags(uint32_t flags)
>>> +{
>>> + int i;
>>> + BitmapFlagEnumList *flist = NULL;
>>> + BitmapFlagEnumList *ftmp;
>>> +
>>> + while (flags) {
>>> + i = ctz32(flags);
>>> + ftmp = g_new0(BitmapFlagEnumList, 1);
>>> + if (i >= BME_FLAG_BIT__BEGIN && i < BME_FLAG_BIT__MAX) {
>>> + ftmp->value = BMEFlagMap[i].qapi_value;
>>> + } else {
>>> + ftmp->value = BITMAP_FLAG_ENUM_UNKNOWN;
>> so, there may be several "unknown" entries. It's inconsistent with
>> "@unknown: This bitmap has unknown or reserved properties.".
>>
>> Finally, can we export values for unknown flags? It should be more
>> informative.
>>
> I changed my mind -- qemu-img is not a debugging tool and shouldn't get
> lost in the weeds trying to enumerate the different kinds of reserved
> values that are set.
>
> It's an error to have them set, or not -- which ones specifically is not
> information that an end-user need know or care about, and I don't want
> to create an extra structure to contain the value.
>
> I'm going to rewrite this loop to just only ever have one unknown value
> at a maximum.
>
> --js
>
Ok, I agree.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 06/12] qapi: add bitmap info
2018-05-17 10:01 ` Vladimir Sementsov-Ogievskiy
@ 2018-05-17 16:43 ` John Snow
0 siblings, 0 replies; 37+ messages in thread
From: John Snow @ 2018-05-17 16:43 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: Kevin Wolf, Markus Armbruster, Max Reitz
On 05/17/2018 06:01 AM, Vladimir Sementsov-Ogievskiy wrote:
> 17.05.2018 00:15, John Snow wrote:
>>
>> On 05/14/2018 10:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 12.05.2018 04:25, John Snow wrote:
>>>> Add some of the necessary scaffolding for reporting bitmap information.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>> qapi/block-core.json | 60
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 59 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index c50517bff3..8f33f41ce7 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -33,6 +33,61 @@
>>>> 'date-sec': 'int', 'date-nsec': 'int',
>>>> 'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
>>>> +##
>>>> +# @BitmapTypeEnum:
>>>> +#
>>>> +# An enumeration of possible serialized bitmap types.
>>>> +#
>>>> +# @dirty-tracking: This bitmap records information on dirty
>>>> +# segments within the file.
>>>> +#
>>>> +# @unknown: This bitmap is of an unknown or reserved type.
>>>> +#
>>>> +# Since: 2.13
>>>> +##
>>>> +{ 'enum': 'BitmapTypeEnum', 'data': [ 'dirty-tracking', 'unknown' ] }
>>>> +
>>>> +##
>>>> +# @BitmapFlagEnum:
>>>> +#
>>>> +# An enumeration of possible flags for serialized bitmaps.
>>>> +#
>>>> +# @in-use: This bitmap is considered to be in-use, and may now be
>>>> inconsistent.
>>>> +#
>>>> +# @auto: This bitmap must reflect any and all changes to the file it
>>>> describes.
>>>> +# The type of this bitmap must be @DirtyTrackingBitmap.
>>> logical, but I don't see this restriction in the spec. May be we need to
>>> update the spec
>>>
>> 1: auto
>>
>> "The bitmap must reflect all changes of the virtual disk by any
>> application that would write to this qcow2 file (including writes,
>> snapshot switching, etc.). The type of this bitmap must be 'dirty
>> tracking bitmap'."
>>
>> Actually, this looks correct now that I'm looking at the spec again.
>> I've used a terser phrasing but I think it's correct.
>
> another thought: why must? We know nothing about other types.. May be
> for other type this flag will have similar or other meaning.. For me,
> this flag looks like a property of dirty-tracking bitmap, not the thing
> which dictates only that type.
>
Yeah, there may be other kinds that want this behavior. The restriction
can probably be eliminated from the spec, I think, but that's to be
handled outside of this series.
I'll remove the restriction from the QAPI flag, because what "should" or
"must" happen is not relevant to what _is_ happening. :)
--js
^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2018-05-17 16:43 UTC | newest]
Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-12 1:25 [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries John Snow
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 01/12] qcow2-bitmap: cache bm_list John Snow
2018-05-14 11:55 ` Vladimir Sementsov-Ogievskiy
2018-05-14 12:15 ` Vladimir Sementsov-Ogievskiy
2018-05-15 20:38 ` John Snow
2018-05-15 20:27 ` John Snow
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 02/12] qcow2/dirty-bitmap: cache loaded bitmaps John Snow
2018-05-14 12:33 ` Vladimir Sementsov-Ogievskiy
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 03/12] block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps John Snow
2018-05-14 12:44 ` Vladimir Sementsov-Ogievskiy
2018-05-15 20:59 ` John Snow
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 04/12] qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO John Snow
2018-05-14 12:55 ` Vladimir Sementsov-Ogievskiy
2018-05-15 20:52 ` John Snow
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 05/12] qcow2-bitmap: track bitmap type John Snow
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 06/12] qapi: add bitmap info John Snow
2018-05-14 14:30 ` Vladimir Sementsov-Ogievskiy
2018-05-15 20:56 ` John Snow
2018-05-16 21:15 ` John Snow
2018-05-17 10:01 ` Vladimir Sementsov-Ogievskiy
2018-05-17 16:43 ` John Snow
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 07/12] qcow2-bitmap: add basic bitmaps info John Snow
2018-05-14 15:12 ` Vladimir Sementsov-Ogievskiy
2018-05-15 21:03 ` John Snow
2018-05-16 21:17 ` John Snow
2018-05-17 10:03 ` Vladimir Sementsov-Ogievskiy
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 08/12] qjson: allow caller to ask for arbitrary indent John Snow
2018-05-16 21:34 ` Eric Blake
2018-05-16 21:49 ` John Snow
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 09/12] qapi/block-core: add BitmapMapping and BitmapEntry structs John Snow
2018-05-16 21:37 ` Eric Blake
2018-05-16 21:55 ` John Snow
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 10/12] qemu-img: split off common chunk of map command John Snow
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 11/12] qemu-img: add bitmap dump John Snow
2018-05-12 1:25 ` [Qemu-devel] [RFC PATCH 12/12] qemu-img: add bitmap clear John Snow
2018-05-12 1:38 ` [Qemu-devel] [RFC PATCH 00/12] qemu-img: add bitmap queries no-reply
2018-05-14 11:32 ` Vladimir Sementsov-Ogievskiy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.