| Message ID | 20191219085106.22309-5-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | fix migration with bitmaps and mirror | expand |
On 12/19/19 2:51 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. > > Libvirt currently is unprepared to bitmap migration by node-name, > node-names are mostly auto-generated. So actually only migration by blk > name works. It depends on whether -blockdev is in use. With -blockdev, libvirt should be supplying a node name everywhere, without, it is only device names. > > 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. Does Max' work to improve seeing through filters fix this? > > Let's fix it by allowing use blk-name even if some implicit filters are > inserted. > > Note, that we possibly want to allow explicit filters skipping too, but > this is another story. > > 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 That bug has been marked CLOSED in the meantime, but it appears to be only because libvirt is now using -blockdev rather than the older drive, while the problem affects drive usage. > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > migration/block-dirty-bitmap.c | 39 +++++++++++++++++++++++++++++++++- > 1 file changed, 38 insertions(+), 1 deletion(-) > > Okay, after reading some more history on this project (the curse of coming up to speed after volunteering to become a co-maintainer), it looks like Max's idea replaces this patch altogether. How much of the rest of the series is still important?
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; + } + + if (bs && bs->drv && !bs->drv->is_filter) { + if (add_bitmaps_to_list(bs, name)) { + goto fail; + } + g_hash_table_add(handled_by_blk, bs); + } + } + for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) { - if (add_bitmaps_to_list(bs, bdrv_get_device_or_node_name(bs))) { + if (g_hash_table_contains(handled_by_blk, bs)) { + continue; + } + + if (add_bitmaps_to_list(bs, bdrv_get_node_name(bs))) { goto fail; } } @@ -340,9 +374,12 @@ static int init_dirty_bitmap_migration(void) dirty_bitmap_mig_state.no_bitmaps = true; } + g_hash_table_destroy(handled_by_blk); + return 0; fail: + g_hash_table_destroy(handled_by_blk); dirty_bitmap_mig_cleanup(); return -1;
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. Libvirt currently is unprepared to bitmap migration by node-name, node-names are mostly auto-generated. So actually only migration by blk name works. 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. Let's fix it by allowing use blk-name even if some implicit filters are inserted. Note, that we possibly want to allow explicit filters skipping too, but this is another story. 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 <vsementsov@virtuozzo.com> --- migration/block-dirty-bitmap.c | 39 +++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)