Re: [PATCH v3 4/7] migration/block-dirty-bitmap: fix bitmaps pre-blockdev migration during mirror job
On 5/19/20 5:51 AM, Vladimir Sementsov-Ogievskiy wrote:
18.05.2020 23:36, Eric Blake wrote:
On 5/15/20 7:40 AM, Vladimir Sementsov-Ogievskiy wrote:
Important thing for bitmap migration is to select destination block
node to obtain the migrated bitmap.
+ /* Skip filters without bitmaos */
+ while (bs && bs->drv && bs->drv->is_filter &&
+ !bdrv_has_named_bitmaps(bs))
+ {
+ bs = bs->backing->bs ?: bs->file->bs;
Is this correct, or should it be:
bs = bs->backing ? bs->backing->bs : bs->file->bs;
Hmm, yes, otherwise it should crash on file-based filter :)
Otherwise looks reasonable, but I'm hesitant to include it in today's
bitmap pull request in order to give it more review/testing time. It
should be ready for a pull request next week, though.
Can you post a v4, to make it easier for me to build?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 4/7] migration/block-dirty-bitmap: fix bitmaps pre-blockdev migration during mirror job
18.05.2020 23:36, Eric Blake wrote:
On 5/15/20 7:40 AM, Vladimir Sementsov-Ogievskiy wrote:
Important thing for bitmap migration is to select destination block
node to obtain the migrated bitmap.
Prepatch, on source we use bdrv_get_device_or_node_name() to identify
the node, and on target we do bdrv_lookup_bs.
bdrv_get_device_or_node_name() returns blk name only for direct
children of blk. So, bitmaps of direct children of blks are migrated by
blk name and others - by node name.
Old libvirt is unprepared to bitmap migration by node-name,
node-names are mostly auto-generated. So actually only migration by blk
name works for it.
Newer libvirt will use new interface (which will be added soon) to
specify node-mapping for bitmaps migration explicitly. Still, let's
improve the current behavior a bit.
Now, consider classic libvirt migrations assisted by mirror block job:
mirror block job inserts filter, so our source is not a direct child of
blk, and bitmaps are migrated by node-names. And this just don't work
either "won't" or "doesn't"
with auto-generated node names
trailing '.'
Let's fix it by allowing use blk-name even if some implicit filters are
inserted.
s/allowing use/using/
Note2: we, of course, can't skip filters and use blk name to migrate
bitmaps in filtered node by blk name for this blk if these filters have
named bitmaps which should be migrated.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652424
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
migration/block-dirty-bitmap.c | 39 +-
1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 7e93718086..5d3a7d2b07 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -319,14 +319,48 @@ static int init_dirty_bitmap_migration(void)
{
BlockDriverState *bs;
DirtyBitmapMigBitmapState *dbms;
+ GHashTable *handled_by_blk = g_hash_table_new(NULL, NULL);
+ BlockBackend *blk;
dirty_bitmap_mig_state.bulk_completed = false;
dirty_bitmap_mig_state.prev_bs = NULL;
dirty_bitmap_mig_state.prev_bitmap = NULL;
dirty_bitmap_mig_state.no_bitmaps = false;
+ /*
+ * Use blockdevice name for direct (or filtered) children of named block
+ * backends.
+ */
+ for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
+ const char *name = blk_name(blk);
+
+ if (!name || strcmp(name, "") == 0) {
+ continue;
+ }
+
+ bs = blk_bs(blk);
+
+ /* Skip filters without bitmaos */
+ while (bs && bs->drv && bs->drv->is_filter &&
+ !bdrv_has_named_bitmaps(bs))
+ {
+ bs = bs->backing->bs ?: bs->file->bs;
Is this correct, or should it be:
bs = bs->backing ? bs->backing->bs : bs->file->bs;
Hmm, yes, otherwise it should crash on file-based filter :)
Otherwise looks reasonable, but I'm hesitant to include it in today's bitmap
pull request in order to give it more review/testing time. It should be ready
for a pull request next week, though.
--
Best regards,
Vladimir
Re: [PATCH v3 4/7] migration/block-dirty-bitmap: fix bitmaps pre-blockdev migration during mirror job
On 5/15/20 7:40 AM, Vladimir Sementsov-Ogievskiy wrote:
Important thing for bitmap migration is to select destination block
node to obtain the migrated bitmap.
Prepatch, on source we use bdrv_get_device_or_node_name() to identify
the node, and on target we do bdrv_lookup_bs.
bdrv_get_device_or_node_name() returns blk name only for direct
children of blk. So, bitmaps of direct children of blks are migrated by
blk name and others - by node name.
Old libvirt is unprepared to bitmap migration by node-name,
node-names are mostly auto-generated. So actually only migration by blk
name works for it.
Newer libvirt will use new interface (which will be added soon) to
specify node-mapping for bitmaps migration explicitly. Still, let's
improve the current behavior a bit.
Now, consider classic libvirt migrations assisted by mirror block job:
mirror block job inserts filter, so our source is not a direct child of
blk, and bitmaps are migrated by node-names. And this just don't work
either "won't" or "doesn't"
with auto-generated node names
trailing '.'
Let's fix it by allowing use blk-name even if some implicit filters are
inserted.
s/allowing use/using/
Note2: we, of course, can't skip filters and use blk name to migrate
bitmaps in filtered node by blk name for this blk if these filters have
named bitmaps which should be migrated.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1652424
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
migration/block-dirty-bitmap.c | 39 +-
1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 7e93718086..5d3a7d2b07 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -319,14 +319,48 @@ static int init_dirty_bitmap_migration(void)
{
BlockDriverState *bs;
DirtyBitmapMigBitmapState *dbms;
+GHashTable *handled_by_blk = g_hash_table_new(NULL, NULL);
+BlockBackend *blk;
dirty_bitmap_mig_state.bulk_completed = false;
dirty_bitmap_mig_state.prev_bs = NULL;
dirty_bitmap_mig_state.prev_bitmap = NULL;
dirty_bitmap_mig_state.no_bitmaps = false;
+/*
+ * Use blockdevice name for direct (or filtered) children of named block
+ * backends.
+ */
+for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
+const char *name = blk_name(blk);
+
+if (!name || strcmp(name, "") == 0) {
+continue;
+}
+
+bs = blk_bs(blk);
+
+/* Skip filters without bitmaos */
+while (bs && bs->drv && bs->drv->is_filter &&
+ !bdrv_has_named_bitmaps(bs))
+{
+bs = bs->backing->bs ?: bs->file->bs;
Is this correct, or should it be:
bs = bs->backing ? bs->backing->bs : bs->file->bs;
Otherwise looks reasonable, but I'm hesitant to include it in today's
bitmap pull request in order to give it more review/testing time. It
should be ready for a pull request next week, though.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
