| Message ID | 3afd4b353cf75c01c9260ca65e073d897e8c42d2.1612356810.git.pkrempa@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: dirty-bitmap: Allow control of bitmap persistence on destination | expand |
03.02.2021 16:00, Peter Krempa wrote: > Bitmap's source persistence is transported over the migration stream and > the destination mirrors it. In some cases the destination might want to > persist bitmaps which are not persistent on the source (e.g. the result > of merge of bitmaps from a number of layers on the source when migrating > into a squashed image) Why not make merge target on source be persistent itself? Then it will be persistent on migration destination. > but currently it would need to create another set > of persistent bitmaps and merge them. > > This adds 'dest-persistent' optional property to > 'BitmapMigrationBitmapAlias' which when present overrides the bitmap > presence state from the source. It's seems simpler to make a separate qmp command block-dirty-bitmap-make-persistent.. Didn't you consider this way?
On Wed, Feb 03, 2021 at 16:23:21 +0300, Vladimir Sementsov-Ogievskiy wrote: > 03.02.2021 16:00, Peter Krempa wrote: > > Bitmap's source persistence is transported over the migration stream and > > the destination mirrors it. In some cases the destination might want to > > persist bitmaps which are not persistent on the source (e.g. the result > > of merge of bitmaps from a number of layers on the source when migrating > > into a squashed image) > > Why not make merge target on source be persistent itself? Then it will be persistent on migration destination. Because they are temporary on the source. I don't want to make it persistent in case of a failure so that it doesn't get written to the disk e.g. in case of VM shutdown. > > > but currently it would need to create another set > > of persistent bitmaps and merge them. > > > > This adds 'dest-persistent' optional property to > > 'BitmapMigrationBitmapAlias' which when present overrides the bitmap > > presence state from the source. > > It's seems simpler to make a separate qmp command block-dirty-bitmap-make-persistent.. Didn't you consider this way? I'm not sure how the internals work entirely. In my case it's way simpler to do this setup when generating the mapping which I need to do anyways rather than calling separate commands.
On Wed, Feb 03, 2021 at 14:27:49 +0100, Peter Krempa wrote: > On Wed, Feb 03, 2021 at 16:23:21 +0300, Vladimir Sementsov-Ogievskiy wrote: > > 03.02.2021 16:00, Peter Krempa wrote: > > > Bitmap's source persistence is transported over the migration stream and > > > the destination mirrors it. In some cases the destination might want to > > > persist bitmaps which are not persistent on the source (e.g. the result > > > of merge of bitmaps from a number of layers on the source when migrating > > > into a squashed image) > > > > Why not make merge target on source be persistent itself? Then it will be persistent on migration destination. > > Because they are temporary on the source. I don't want to make it > persistent in case of a failure so that it doesn't get written to the > disk e.g. in case of VM shutdown. To be a bit more specific, I don't want the bitmaps to stay in the image, that means that I'd have to also delete them on the source after a successfull migration before qemu is terminated, which might not even be possible since source deactivates storage after migration. So making them persistent on source is impossible. > > > > > > but currently it would need to create another set > > > of persistent bitmaps and merge them. > > > > > > This adds 'dest-persistent' optional property to > > > 'BitmapMigrationBitmapAlias' which when present overrides the bitmap > > > presence state from the source. > > > > It's seems simpler to make a separate qmp command block-dirty-bitmap-make-persistent.. Didn't you consider this way? > > I'm not sure how the internals work entirely. In my case it's way > simpler to do this setup when generating the mapping which I need to do > anyways rather than calling separate commands. Similarly here, after a successful migration I'd have to go and make all the bitmaps persistent, which is an extra step, and also a vector for possible failures after migration which also doesn't seem appealing.
03.02.2021 16:39, Peter Krempa wrote: > On Wed, Feb 03, 2021 at 14:27:49 +0100, Peter Krempa wrote: >> On Wed, Feb 03, 2021 at 16:23:21 +0300, Vladimir Sementsov-Ogievskiy wrote: >>> 03.02.2021 16:00, Peter Krempa wrote: >>>> Bitmap's source persistence is transported over the migration stream and >>>> the destination mirrors it. In some cases the destination might want to >>>> persist bitmaps which are not persistent on the source (e.g. the result >>>> of merge of bitmaps from a number of layers on the source when migrating >>>> into a squashed image) >>> >>> Why not make merge target on source be persistent itself? Then it will be persistent on migration destination. >> >> Because they are temporary on the source. I don't want to make it >> persistent in case of a failure so that it doesn't get written to the >> disk e.g. in case of VM shutdown. > > To be a bit more specific, I don't want the bitmaps to stay in the > image, that means that I'd have to also delete them on the source after > a successfull migration before qemu is terminated, which might not even > be possible since source deactivates storage after migration. > > So making them persistent on source is impossible. Actually on success path, persistent bitmaps are not stored on source. Normally persistent bitmaps are stored on image inactivation. But bitmaps involved into migration are an exclusion, they are not stored (otherwise, stoing will influence downtime of migration). And of-course, we can't store bitmaps after disks inactivation. So, on success path, the only way to store bitmaps on source is to do qmp 'cont' command on source after migration.. I'm not so sure about error path. Of course, if something breaks between merge target creation and migration start, bitmaps will be stored. So, I agree that in general it's bad idea to make temporary bitmap 'persistent'. > >> >>> >>>> but currently it would need to create another set >>>> of persistent bitmaps and merge them. >>>> >>>> This adds 'dest-persistent' optional property to >>>> 'BitmapMigrationBitmapAlias' which when present overrides the bitmap >>>> presence state from the source. >>> >>> It's seems simpler to make a separate qmp command block-dirty-bitmap-make-persistent.. Didn't you consider this way? >> >> I'm not sure how the internals work entirely. In my case it's way >> simpler to do this setup when generating the mapping which I need to do >> anyways rather than calling separate commands. > > Similarly here, after a successful migration I'd have to go and make all > the bitmaps persistent, which is an extra step, and also a vector for > possible failures after migration which also doesn't seem appealing. > OK, that's reasonable, thanks for explanation
On 2/3/21 7:00 AM, Peter Krempa wrote: > Bitmap's source persistence is transported over the migration stream and > the destination mirrors it. In some cases the destination might want to > persist bitmaps which are not persistent on the source (e.g. the result > of merge of bitmaps from a number of layers on the source when migrating > into a squashed image) but currently it would need to create another set > of persistent bitmaps and merge them. > > This adds 'dest-persistent' optional property to > 'BitmapMigrationBitmapAlias' which when present overrides the bitmap > presence state from the source. persistance > > Signed-off-by: Peter Krempa <pkrempa@redhat.com> > --- > migration/block-dirty-bitmap.c | 30 +++++++++++++++++++++++++++++- > qapi/migration.json | 7 ++++++- > 2 files changed, 35 insertions(+), 2 deletions(-) > > +++ b/qapi/migration.json > @@ -533,12 +533,17 @@ > # @alias: An alias name for migration (for example the bitmap name on > # the opposite site). > # > +# @dest-persistent: If populated set the bitmap will be turned persistent > +# or transient depending on this parameter. s/populated set/present,/ > +# (since 6.0) > +# > # Since: 5.2 > ## > { 'struct': 'BitmapMigrationBitmapAlias', > 'data': { > 'name': 'str', > - 'alias': 'str' > + 'alias': 'str', > + '*dest-persistent': 'bool' > } } > > ## > The grammar fix is trivial, so Reviewed-by: Eric Blake <eblake@redhat.com> I see there is discussion over whether this is the best approach, but it makes sense to me. Unless there's a good reason why something else would be better, I'm probably going to queue this through my dirty bitmaps tree for a pull request sometime next week.
03.02.2021 16:00, Peter Krempa wrote: > Bitmap's source persistence is transported over the migration stream and > the destination mirrors it. In some cases the destination might want to > persist bitmaps which are not persistent on the source (e.g. the result > of merge of bitmaps from a number of layers on the source when migrating > into a squashed image) but currently it would need to create another set > of persistent bitmaps and merge them. > > This adds 'dest-persistent' optional property to > 'BitmapMigrationBitmapAlias' which when present overrides the bitmap > presence state from the source. > > Signed-off-by: Peter Krempa <pkrempa@redhat.com> > --- > migration/block-dirty-bitmap.c | 30 +++++++++++++++++++++++++++++- > qapi/migration.json | 7 ++++++- > 2 files changed, 35 insertions(+), 2 deletions(-) > > diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c > index b0403dd00c..1876c94c45 100644 > --- a/migration/block-dirty-bitmap.c > +++ b/migration/block-dirty-bitmap.c > @@ -149,6 +149,9 @@ typedef struct DBMLoadState { > > bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start */ > > + bool has_dest_persistent; > + bool dest_persistent; > + > /* > * cancelled > * Incoming migration is cancelled for some reason. That means that we > @@ -171,6 +174,10 @@ static DBMState dbm_state; > > typedef struct AliasMapInnerBitmap { > char *string; > + > + /* for destination's properties setting bitmap state */ > + bool has_dest_persistent; > + bool dest_persistent; > } AliasMapInnerBitmap; > > static void free_alias_map_inner_bitmap(void *amin_ptr) > @@ -289,6 +296,8 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm, > for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) { > const BitmapMigrationBitmapAlias *bmba = bmbal->value; > const char *bmap_map_from, *bmap_map_to; > + bool bmap_has_dest_persistent = false; > + bool bmap_dest_persistent = false; > AliasMapInnerBitmap *bmap_inner; > > if (strlen(bmba->alias) > UINT8_MAX) { > @@ -317,6 +326,9 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm, > bmap_map_from = bmba->alias; > bmap_map_to = bmba->name; > > + bmap_has_dest_persistent = bmba->has_dest_persistent; > + bmap_dest_persistent = bmba->dest_persistent; > + > if (g_hash_table_contains(bitmaps_map, bmba->alias)) { > error_setg(errp, "The bitmap alias '%s'/'%s' is used twice", > bmna->alias, bmba->alias); > @@ -326,6 +338,8 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm, > > bmap_inner = g_new0(AliasMapInnerBitmap, 1); > bmap_inner->string = g_strdup(bmap_map_to); > + bmap_inner->has_dest_persistent = bmap_has_dest_persistent; > + bmap_inner->dest_persistent = bmap_dest_persistent; > > g_hash_table_insert(bitmaps_map, > g_strdup(bmap_map_from), bmap_inner); > @@ -798,6 +812,7 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s) > uint32_t granularity = qemu_get_be32(f); > uint8_t flags = qemu_get_byte(f); > LoadBitmapState *b; > + bool persistent; > > if (s->cancelled) { > return 0; > @@ -822,7 +837,13 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s) > return -EINVAL; > } > > - if (flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT) { > + if (s->has_dest_persistent) { > + persistent = s->dest_persistent; > + } else { > + persistent = flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT; > + } > + > + if (persistent) { > bdrv_dirty_bitmap_set_persistence(s->bitmap, true); > } > > @@ -1087,6 +1108,8 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s, > > if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) { > const char *bitmap_name; > + bool bitmap_has_dest_persistent = false; > + bool bitmap_dest_persistent = false; > > if (!qemu_get_counted_string(f, s->bitmap_alias)) { > error_report("Unable to read bitmap alias string"); > @@ -1107,12 +1130,17 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s, > } > > bitmap_name = bmap_inner->string; > + bitmap_has_dest_persistent = bmap_inner->has_dest_persistent; > + bitmap_dest_persistent = bmap_inner->dest_persistent; > } > > if (!s->cancelled) { > g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name)); > s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name); > > + s->has_dest_persistent = bitmap_has_dest_persistent; > + s->dest_persistent = bitmap_dest_persistent; > + > /* > * bitmap may be NULL here, it wouldn't be an error if it is the > * first occurrence of the bitmap > diff --git a/qapi/migration.json b/qapi/migration.json > index d1d9632c2a..32e64dbce6 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -533,12 +533,17 @@ > # @alias: An alias name for migration (for example the bitmap name on > # the opposite site). > # > +# @dest-persistent: If populated set the bitmap will be turned persistent > +# or transient depending on this parameter. > +# (since 6.0) > +# Thinking of future: will we want modify other bitmap properties "in-flight"? enabled? maybe granularity? Then we'll add additional dest-* properties. Not bad, but probably better to make it nested, like transform: { persistent: bool } So, than we'll can add other properties: transform: { *persistent: bool, *enable: bool, *granularity: bool } Also, in code I see you just ignore dest-persistent if it is set on source. I think we should either error-out, or support it. Why not to allow setup property change on source alias-mapping? (and that's why I suggest "transform" which a bit more generic for using both on source and target) Also, I don't like duplication of AliasMapInnerBitmap in DBMLoadState. Could we instead just add a pointer to bmap_inner, like this: diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index 1876c94c45..93ae76734e 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -138,6 +138,14 @@ typedef struct LoadBitmapState { bool enabled; } LoadBitmapState; +typedef struct AliasMapInnerBitmap { + char *string; + + /* for destination's properties setting bitmap state */ + bool has_dest_persistent; + bool dest_persistent; +} AliasMapInnerBitmap; + /* State of the dirty bitmap migration (DBM) during load process */ typedef struct DBMLoadState { uint32_t flags; @@ -149,8 +157,7 @@ typedef struct DBMLoadState { bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start */ - bool has_dest_persistent; - bool dest_persistent; + AliasMapInnerBitmap *bmap_inner; /* * cancelled @@ -172,14 +179,6 @@ typedef struct DBMState { static DBMState dbm_state; -typedef struct AliasMapInnerBitmap { - char *string; - - /* for destination's properties setting bitmap state */ - bool has_dest_persistent; - bool dest_persistent; -} AliasMapInnerBitmap; - static void free_alias_map_inner_bitmap(void *amin_ptr) { AliasMapInnerBitmap *amin = amin_ptr; @@ -837,8 +836,8 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s) return -EINVAL; } - if (s->has_dest_persistent) { - persistent = s->dest_persistent; + if (s->bmap_inner && s->bmap_inner->has_dest_persistent) { + persistent = s->bmap_inner->dest_persistent; } else { persistent = flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT; } @@ -1108,8 +1107,6 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s, if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) { const char *bitmap_name; - bool bitmap_has_dest_persistent = false; - bool bitmap_dest_persistent = false; if (!qemu_get_counted_string(f, s->bitmap_alias)) { error_report("Unable to read bitmap alias string"); @@ -1130,17 +1127,13 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s, } bitmap_name = bmap_inner->string; - bitmap_has_dest_persistent = bmap_inner->has_dest_persistent; - bitmap_dest_persistent = bmap_inner->dest_persistent; + s->bmap_inner = bmap_inner; } if (!s->cancelled) { g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name)); s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name); - s->has_dest_persistent = bitmap_has_dest_persistent; - s->dest_persistent = bitmap_dest_persistent; - /* * bitmap may be NULL here, it wouldn't be an error if it is the * first occurrence of the bitmap
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index b0403dd00c..1876c94c45 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -149,6 +149,9 @@ typedef struct DBMLoadState { bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start */ + bool has_dest_persistent; + bool dest_persistent; + /* * cancelled * Incoming migration is cancelled for some reason. That means that we @@ -171,6 +174,10 @@ static DBMState dbm_state; typedef struct AliasMapInnerBitmap { char *string; + + /* for destination's properties setting bitmap state */ + bool has_dest_persistent; + bool dest_persistent; } AliasMapInnerBitmap; static void free_alias_map_inner_bitmap(void *amin_ptr) @@ -289,6 +296,8 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm, for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) { const BitmapMigrationBitmapAlias *bmba = bmbal->value; const char *bmap_map_from, *bmap_map_to; + bool bmap_has_dest_persistent = false; + bool bmap_dest_persistent = false; AliasMapInnerBitmap *bmap_inner; if (strlen(bmba->alias) > UINT8_MAX) { @@ -317,6 +326,9 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm, bmap_map_from = bmba->alias; bmap_map_to = bmba->name; + bmap_has_dest_persistent = bmba->has_dest_persistent; + bmap_dest_persistent = bmba->dest_persistent; + if (g_hash_table_contains(bitmaps_map, bmba->alias)) { error_setg(errp, "The bitmap alias '%s'/'%s' is used twice", bmna->alias, bmba->alias); @@ -326,6 +338,8 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm, bmap_inner = g_new0(AliasMapInnerBitmap, 1); bmap_inner->string = g_strdup(bmap_map_to); + bmap_inner->has_dest_persistent = bmap_has_dest_persistent; + bmap_inner->dest_persistent = bmap_dest_persistent; g_hash_table_insert(bitmaps_map, g_strdup(bmap_map_from), bmap_inner); @@ -798,6 +812,7 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s) uint32_t granularity = qemu_get_be32(f); uint8_t flags = qemu_get_byte(f); LoadBitmapState *b; + bool persistent; if (s->cancelled) { return 0; @@ -822,7 +837,13 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s) return -EINVAL; } - if (flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT) { + if (s->has_dest_persistent) { + persistent = s->dest_persistent; + } else { + persistent = flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT; + } + + if (persistent) { bdrv_dirty_bitmap_set_persistence(s->bitmap, true); } @@ -1087,6 +1108,8 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s, if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) { const char *bitmap_name; + bool bitmap_has_dest_persistent = false; + bool bitmap_dest_persistent = false; if (!qemu_get_counted_string(f, s->bitmap_alias)) { error_report("Unable to read bitmap alias string"); @@ -1107,12 +1130,17 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s, } bitmap_name = bmap_inner->string; + bitmap_has_dest_persistent = bmap_inner->has_dest_persistent; + bitmap_dest_persistent = bmap_inner->dest_persistent; } if (!s->cancelled) { g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name)); s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name); + s->has_dest_persistent = bitmap_has_dest_persistent; + s->dest_persistent = bitmap_dest_persistent; + /* * bitmap may be NULL here, it wouldn't be an error if it is the * first occurrence of the bitmap diff --git a/qapi/migration.json b/qapi/migration.json index d1d9632c2a..32e64dbce6 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -533,12 +533,17 @@ # @alias: An alias name for migration (for example the bitmap name on # the opposite site). # +# @dest-persistent: If populated set the bitmap will be turned persistent +# or transient depending on this parameter. +# (since 6.0) +# # Since: 5.2 ## { 'struct': 'BitmapMigrationBitmapAlias', 'data': { 'name': 'str', - 'alias': 'str' + 'alias': 'str', + '*dest-persistent': 'bool' } } ##
Bitmap's source persistence is transported over the migration stream and the destination mirrors it. In some cases the destination might want to persist bitmaps which are not persistent on the source (e.g. the result of merge of bitmaps from a number of layers on the source when migrating into a squashed image) but currently it would need to create another set of persistent bitmaps and merge them. This adds 'dest-persistent' optional property to 'BitmapMigrationBitmapAlias' which when present overrides the bitmap presence state from the source. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- migration/block-dirty-bitmap.c | 30 +++++++++++++++++++++++++++++- qapi/migration.json | 7 ++++++- 2 files changed, 35 insertions(+), 2 deletions(-)