| Message ID | 20170511163228.6666-3-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, May 11, 2017 at 06:32:27PM +0200, Juan Quintela wrote: > We have change in the previous patch to use migration capabilities for > it. Notice that we continue using the old command line flags from > migrate command from the time being. Remove the set_params method as > now it is empty. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > include/migration/migration.h | 3 +-- > migration/block.c | 17 ++--------------- > migration/colo.c | 5 +++-- > migration/migration.c | 8 +++++--- > migration/savevm.c | 2 -- > 5 files changed, 11 insertions(+), 24 deletions(-) > > diff --git a/include/migration/migration.h b/include/migration/migration.h > index 30c2913..2d5525c 100644 > --- a/include/migration/migration.h > +++ b/include/migration/migration.h > @@ -39,8 +39,7 @@ > #define QEMU_VM_SECTION_FOOTER 0x7e > > struct MigrationParams { > - bool blk; > - bool shared; > + bool unused; /* C doesn't allow empty structs */ > }; > > /* Messages sent on the return path from destination to source */ > diff --git a/migration/block.c b/migration/block.c > index 060087f..fcfa823 100644 > --- a/migration/block.c > +++ b/migration/block.c > @@ -94,9 +94,6 @@ typedef struct BlkMigBlock { > } BlkMigBlock; > > typedef struct BlkMigState { > - /* Written during setup phase. Can be read without a lock. */ > - int blk_enable; > - int shared_base; > QSIMPLEQ_HEAD(bmds_list, BlkMigDevState) bmds_list; > int64_t total_sector_sum; > bool zero_blocks; > @@ -425,7 +422,7 @@ static int init_blk_migration(QEMUFile *f) > bmds->bulk_completed = 0; > bmds->total_sectors = sectors; > bmds->completed_sectors = 0; > - bmds->shared_base = block_mig_state.shared_base; > + bmds->shared_base = migrate_use_block_shared(); > > assert(i < num_bs); > bmds_bs[i].bmds = bmds; > @@ -994,22 +991,12 @@ static int block_load(QEMUFile *f, void *opaque, int version_id) > return 0; > } > > -static void block_set_params(const MigrationParams *params, void *opaque) > -{ > - block_mig_state.blk_enable = params->blk; > - block_mig_state.shared_base = params->shared; > - > - /* shared base means that blk_enable = 1 */ > - block_mig_state.blk_enable |= params->shared; > -} > - > static bool block_is_active(void *opaque) > { > - return block_mig_state.blk_enable == 1; > + return migrate_use_block_enabled(); > } > > static SaveVMHandlers savevm_block_handlers = { > - .set_params = block_set_params, > .save_live_setup = block_save_setup, > .save_live_iterate = block_save_iterate, > .save_live_complete_precopy = block_save_complete, > diff --git a/migration/colo.c b/migration/colo.c > index 963c802..e772384 100644 > --- a/migration/colo.c > +++ b/migration/colo.c > @@ -14,6 +14,7 @@ > #include "qemu/timer.h" > #include "sysemu/sysemu.h" > #include "migration/colo.h" > +#include "migration/block.h" > #include "io/channel-buffer.h" > #include "trace.h" > #include "qemu/error-report.h" > @@ -345,8 +346,8 @@ static int colo_do_checkpoint_transaction(MigrationState *s, > } > > /* Disable block migration */ > - s->params.blk = 0; > - s->params.shared = 0; > + migrate_set_block_enabled(s, false); > + migrate_set_block_shared(s, false); > qemu_savevm_state_header(fb); > qemu_savevm_state_begin(fb, &s->params); > qemu_mutex_lock_iothread(); > diff --git a/migration/migration.c b/migration/migration.c > index 2f981aa..8a3bf89 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -787,6 +787,10 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params, > s->enabled_capabilities[cap->value->capability] = cap->value->state; > } > > + if (s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED]) { > + s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true; > + } > + [1] > if (migrate_postcopy_ram()) { > if (migrate_use_compression()) { > /* The decompression threads asynchronously write into RAM > @@ -1214,9 +1218,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, > MigrationParams params; > const char *p; > > - params.blk = has_blk && blk; > - params.shared = has_inc && inc; > - > if (migration_is_setup_or_active(s->state) || > s->state == MIGRATION_STATUS_CANCELLING || > s->state == MIGRATION_STATUS_COLO) { > @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, > } > > if (has_inc && inc) { > + migrate_set_block_enabled(s, true); > migrate_set_block_shared(s, true); [2] IIUC for [1] & [2] we are solving the same problem that "shared" depends on "enabled" bit. Would it be good to unitfy this dependency somewhere? E.g., by changing migrate_set_block_shared() into: void migrate_set_block_shared(MigrationState *s, bool value) { s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value; if (value) { migrate_set_block_enabled(s, true); } } ? Another thing to mention: after switching to the capability interface, we'll cache the "enabled" and "shared" bits now while we don't cache it before, right? IIUC it'll affect behavior of such sequence: - 1st migrate with enabled=1, shared=1, then - 2nd migrate with enabled=0, shared=0 Before the series, the 2nd migrate will use enabled=shared=0, but after the series it should be using enabled=shared=1. Not sure whether this would be a problem (or I missed anything?). Thanks, > } > > diff --git a/migration/savevm.c b/migration/savevm.c > index f8d1e8b..221fb4b 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -1233,8 +1233,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp) > { > int ret; > MigrationParams params = { > - .blk = 0, > - .shared = 0 > }; > MigrationState *ms = migrate_init(¶ms); > MigrationStatus status; > -- > 2.9.3 >
Peter Xu <peterx@redhat.com> wrote: > On Thu, May 11, 2017 at 06:32:27PM +0200, Juan Quintela wrote: >> @@ -1214,9 +1218,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, >> MigrationParams params; >> const char *p; >> >> - params.blk = has_blk && blk; >> - params.shared = has_inc && inc; >> - >> if (migration_is_setup_or_active(s->state) || >> s->state == MIGRATION_STATUS_CANCELLING || >> s->state == MIGRATION_STATUS_COLO) { >> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, >> } >> >> if (has_inc && inc) { >> + migrate_set_block_enabled(s, true); >> migrate_set_block_shared(s, true); > > [2] > > IIUC for [1] & [2] we are solving the same problem that "shared" > depends on "enabled" bit. Would it be good to unitfy this dependency > somewhere? E.g., by changing migrate_set_block_shared() into: > > void migrate_set_block_shared(MigrationState *s, bool value) > { > s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value; > if (value) { > migrate_set_block_enabled(s, true); > } > } ok with this. I will add once here that when we disable block enabled, we also disable shared, or just let it that way? > Another thing to mention: after switching to the capability interface, > we'll cache the "enabled" and "shared" bits now while we don't cache > it before, right? IIUC it'll affect behavior of such sequence: > > - 1st migrate with enabled=1, shared=1, then > - 2nd migrate with enabled=0, shared=0 > > Before the series, the 2nd migrate will use enabled=shared=0, but > after the series it should be using enabled=shared=1. Not sure whether > this would be a problem (or I missed anything?). We can't be consistent with both old/new way. Old way: we always setup the capabilities on command line (that should have been deprecated long, long ago) New way: Once set, they stay set. So, alternatives are: - If we are going to deprecate the old way, just let things as they are on this patch (easier for me O:-) - Always disable this features at the end of migration: Compatible with old migration semantics, bet we are inconsistent with all other capabilities. - Add yet more code to only disable them when we are setting them through the command line. More code to maintain. My idea would be to deprecate the migrate command line parameter, that is the reason why I did the 1st option. I hope that in due curse, we would be able to remove the code. But if anyone strongly think that any of the other options is better, please let me know. Later, Juan.
On 05/12/2017 05:55 AM, Juan Quintela wrote: >>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, >>> } >>> >>> if (has_inc && inc) { >>> + migrate_set_block_enabled(s, true); >>> migrate_set_block_shared(s, true); >> >> [2] >> >> IIUC for [1] & [2] we are solving the same problem that "shared" >> depends on "enabled" bit. Would it be good to unitfy this dependency >> somewhere? E.g., by changing migrate_set_block_shared() into: >> >> void migrate_set_block_shared(MigrationState *s, bool value) >> { >> s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value; >> if (value) { >> migrate_set_block_enabled(s, true); >> } >> } > > ok with this. Or, as I commented on 1/3, maybe having a single property that is a tri-state enum value, instead of 2 separate boolean properties, might be nicer (but certainly a bit more complex to code up). > I will add once here that when we disable block enabled, we also disable > shared, or just let it that way? > >> Another thing to mention: after switching to the capability interface, >> we'll cache the "enabled" and "shared" bits now while we don't cache >> it before, right? IIUC it'll affect behavior of such sequence: >> >> - 1st migrate with enabled=1, shared=1, then >> - 2nd migrate with enabled=0, shared=0 >> >> Before the series, the 2nd migrate will use enabled=shared=0, but >> after the series it should be using enabled=shared=1. Not sure whether >> this would be a problem (or I missed anything?). > > We can't be consistent with both old/new way. > > Old way: we always setup the capabilities on command line (that should > have been deprecated long, long ago) Well, the easy way out is to have the HMP migrate command (I assume that's what you mean by "on command line") explicitly clear the parameters if it is called without the -b/-i flag. So the start of each migration is what changes the properties, so long as you are still using HMP to start the migration. Or, on the QMP side, since 'migrate' has optional 'blk' and 'inc' booleans, basically leave the settings alone if the parameters were omitted, and explicitly update the property to the value of those parameters if they were present. Or is the proposal that we are also going to simplify the QMP 'migrate' command to get rid of crufty parameters?
Eric Blake <eblake@redhat.com> wrote: > On 05/12/2017 05:55 AM, Juan Quintela wrote: >>>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, >>>> } >>>> >>>> if (has_inc && inc) { >>>> + migrate_set_block_enabled(s, true); >>>> migrate_set_block_shared(s, true); >>> >>> [2] >>> >>> IIUC for [1] & [2] we are solving the same problem that "shared" >>> depends on "enabled" bit. Would it be good to unitfy this dependency >>> somewhere? E.g., by changing migrate_set_block_shared() into: >>> >>> void migrate_set_block_shared(MigrationState *s, bool value) >>> { >>> s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value; >>> if (value) { >>> migrate_set_block_enabled(s, true); >>> } >>> } >> >> ok with this. > > Or, as I commented on 1/3, maybe having a single property that is a > tri-state enum value, instead of 2 separate boolean properties, might be > nicer (but certainly a bit more complex to code up). If you teach me how to do the qapi/qmp part, I will do the other bits. I don't really care if we do it one way or the other. >> I will add once here that when we disable block enabled, we also disable >> shared, or just let it that way? >> >>> Another thing to mention: after switching to the capability interface, >>> we'll cache the "enabled" and "shared" bits now while we don't cache >>> it before, right? IIUC it'll affect behavior of such sequence: >>> >>> - 1st migrate with enabled=1, shared=1, then >>> - 2nd migrate with enabled=0, shared=0 >>> >>> Before the series, the 2nd migrate will use enabled=shared=0, but >>> after the series it should be using enabled=shared=1. Not sure whether >>> this would be a problem (or I missed anything?). >> >> We can't be consistent with both old/new way. >> >> Old way: we always setup the capabilities on command line (that should >> have been deprecated long, long ago) > > Well, the easy way out is to have the HMP migrate command (I assume > that's what you mean by "on command line") explicitly clear the > parameters if it is called without the -b/-i flag. So the start of each > migration is what changes the properties, so long as you are still using > HMP to start the migration. Or, on the QMP side, since 'migrate' has > optional 'blk' and 'inc' booleans, basically leave the settings alone if > the parameters were omitted, and explicitly update the property to the > value of those parameters if they were present. We are going to have trouble whatever way that we do it, or we start doing lots of strange things. Forget about qmp, we are going to assume that it is consistent with hmp. migrate_set_capabilities block_enabled on migrate -b ..... Should migrate disable the block_enabled capability? Give one warning/error? And notice that this only matter if we do a migration, we cancel/get one error, and then we migrate again. What I tried to do is assume that -b/-i arguments don't exist. And if the user use them, we implement its behaviour with the minimal fuss possibly. Only way that I can think of being consistent and bug compatible will be to store: - old block_shared/enanbeld capability value - if we set -b/-i on the command line In migration cleanup do the right thing depending on this four variables. I think that it is adding lots of complexity for very few improvement. > Or is the proposal that we are also going to simplify the QMP 'migrate' > command to get rid of crufty parameters? I didn't read it that way, but I would not oppose O:-) Later, Juan.
On Fri, May 12, 2017 at 12:55:22PM +0200, Juan Quintela wrote: > Peter Xu <peterx@redhat.com> wrote: > > On Thu, May 11, 2017 at 06:32:27PM +0200, Juan Quintela wrote: > > >> @@ -1214,9 +1218,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, > >> MigrationParams params; > >> const char *p; > >> > >> - params.blk = has_blk && blk; > >> - params.shared = has_inc && inc; > >> - > >> if (migration_is_setup_or_active(s->state) || > >> s->state == MIGRATION_STATUS_CANCELLING || > >> s->state == MIGRATION_STATUS_COLO) { > >> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, > >> } > >> > >> if (has_inc && inc) { > >> + migrate_set_block_enabled(s, true); > >> migrate_set_block_shared(s, true); > > > > [2] > > > > IIUC for [1] & [2] we are solving the same problem that "shared" > > depends on "enabled" bit. Would it be good to unitfy this dependency > > somewhere? E.g., by changing migrate_set_block_shared() into: > > > > void migrate_set_block_shared(MigrationState *s, bool value) > > { > > s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value; > > if (value) { > > migrate_set_block_enabled(s, true); > > } > > } > > ok with this. > I will add once here that when we disable block enabled, we also disable > shared, or just let it that way? I should mark "nitpick" in my above comment. Any way works for me. :) > > > Another thing to mention: after switching to the capability interface, > > we'll cache the "enabled" and "shared" bits now while we don't cache > > it before, right? IIUC it'll affect behavior of such sequence: > > > > - 1st migrate with enabled=1, shared=1, then > > - 2nd migrate with enabled=0, shared=0 > > > > Before the series, the 2nd migrate will use enabled=shared=0, but > > after the series it should be using enabled=shared=1. Not sure whether > > this would be a problem (or I missed anything?). > > We can't be consistent with both old/new way. > > Old way: we always setup the capabilities on command line (that should > have been deprecated long, long ago) > > New way: Once set, they stay set. > > So, alternatives are: > - If we are going to deprecate the old way, just let things as they are > on this patch (easier for me O:-) > > - Always disable this features at the end of migration: Compatible with > old migration semantics, bet we are inconsistent with all other > capabilities. > > - Add yet more code to only disable them when we are setting them > through the command line. More code to maintain. > > My idea would be to deprecate the migrate command line parameter, that > is the reason why I did the 1st option. I hope that in due curse, we > would be able to remove the code. But if anyone strongly think that any > of the other options is better, please let me know. I assume that sending continuous "migrate" is very rare, right (after all, normally source VM is useless after that...)? I was just trying to post this question up, and so... If you/Dave/others won't worry about its compatibility, I won't either. ;) Thanks!
* Juan Quintela (quintela@redhat.com) wrote: > Eric Blake <eblake@redhat.com> wrote: > > On 05/12/2017 05:55 AM, Juan Quintela wrote: > >>>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, > >>>> } > >>>> > >>>> if (has_inc && inc) { > >>>> + migrate_set_block_enabled(s, true); > >>>> migrate_set_block_shared(s, true); > >>> > >>> [2] > >>> > >>> IIUC for [1] & [2] we are solving the same problem that "shared" > >>> depends on "enabled" bit. Would it be good to unitfy this dependency > >>> somewhere? E.g., by changing migrate_set_block_shared() into: > >>> > >>> void migrate_set_block_shared(MigrationState *s, bool value) > >>> { > >>> s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value; > >>> if (value) { > >>> migrate_set_block_enabled(s, true); > >>> } > >>> } > >> > >> ok with this. > > > > Or, as I commented on 1/3, maybe having a single property that is a > > tri-state enum value, instead of 2 separate boolean properties, might be > > nicer (but certainly a bit more complex to code up). > > If you teach me how to do the qapi/qmp part, I will do the other bits. > I don't really care if we do it one way or the other. > > >> I will add once here that when we disable block enabled, we also disable > >> shared, or just let it that way? > >> > >>> Another thing to mention: after switching to the capability interface, > >>> we'll cache the "enabled" and "shared" bits now while we don't cache > >>> it before, right? IIUC it'll affect behavior of such sequence: > >>> > >>> - 1st migrate with enabled=1, shared=1, then > >>> - 2nd migrate with enabled=0, shared=0 > >>> > >>> Before the series, the 2nd migrate will use enabled=shared=0, but > >>> after the series it should be using enabled=shared=1. Not sure whether > >>> this would be a problem (or I missed anything?). > >> > >> We can't be consistent with both old/new way. > >> > >> Old way: we always setup the capabilities on command line (that should > >> have been deprecated long, long ago) > > > > Well, the easy way out is to have the HMP migrate command (I assume > > that's what you mean by "on command line") explicitly clear the > > parameters if it is called without the -b/-i flag. So the start of each > > migration is what changes the properties, so long as you are still using > > HMP to start the migration. Or, on the QMP side, since 'migrate' has > > optional 'blk' and 'inc' booleans, basically leave the settings alone if > > the parameters were omitted, and explicitly update the property to the > > value of those parameters if they were present. > > We are going to have trouble whatever way that we do it, or we start > doing lots of strange things. > > Forget about qmp, we are going to assume that it is consistent with hmp. > > migrate_set_capabilities block_enabled on > migrate -b ..... > > Should migrate disable the block_enabled capability? Give one > warning/error? > > And notice that this only matter if we do a migration, we cancel/get one > error, and then we migrate again. > > What I tried to do is assume that -b/-i arguments don't exist. And if > the user use them, we implement its behaviour with the minimal fuss > possibly. > > Only way that I can think of being consistent and bug compatible will be > to store: > - old block_shared/enanbeld capability value > - if we set -b/-i on the command line > > In migration cleanup do the right thing depending on this four > variables. I think that it is adding lots of complexity for very few > improvement. > > > > Or is the proposal that we are also going to simplify the QMP 'migrate' > > command to get rid of crufty parameters? > > I didn't read it that way, but I would not oppose O:-) > Ewww this is messy; you end up with almost as much code as the old flags you're trying to remove. For HMP you could gently deprecate the flags over time and give a warning telling people to use the capabilities; but doing that in one go is a bit nasty, and you still have to do something cleverer for the QMP which is similar. I think I agree though that migrate, cancel, migrate should work sensibly and it's hard to get it right. Dave > Later, Juan. -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
[adding Markus] On 05/15/2017 04:48 AM, Juan Quintela wrote: > Eric Blake <eblake@redhat.com> wrote: >> On 05/12/2017 05:55 AM, Juan Quintela wrote: >>>>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, >>>>> } >>>>> >>>>> if (has_inc && inc) { >>>>> + migrate_set_block_enabled(s, true); >>>>> migrate_set_block_shared(s, true); >>>> >>>> [2] >>>> >>>> IIUC for [1] & [2] we are solving the same problem that "shared" >>>> depends on "enabled" bit. Would it be good to unitfy this dependency >>>> somewhere? E.g., by changing migrate_set_block_shared() into: >>>> >>>> void migrate_set_block_shared(MigrationState *s, bool value) >>>> { >>>> s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value; >>>> if (value) { >>>> migrate_set_block_enabled(s, true); >>>> } >>>> } >>> >>> ok with this. >> >> Or, as I commented on 1/3, maybe having a single property that is a >> tri-state enum value, instead of 2 separate boolean properties, might be >> nicer (but certainly a bit more complex to code up). > > If you teach me how to do the qapi/qmp part, I will do the other bits. > I don't really care if we do it one way or the other. Adding Markus in, as I value his opinion on matters of UI design.
Juan Quintela <quintela@redhat.com> writes: > Eric Blake <eblake@redhat.com> wrote: >> On 05/12/2017 05:55 AM, Juan Quintela wrote: >>>>> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, >>>>> } >>>>> >>>>> if (has_inc && inc) { >>>>> + migrate_set_block_enabled(s, true); >>>>> migrate_set_block_shared(s, true); >>>> >>>> [2] >>>> >>>> IIUC for [1] & [2] we are solving the same problem that "shared" >>>> depends on "enabled" bit. Would it be good to unitfy this dependency >>>> somewhere? E.g., by changing migrate_set_block_shared() into: >>>> >>>> void migrate_set_block_shared(MigrationState *s, bool value) >>>> { >>>> s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value; >>>> if (value) { >>>> migrate_set_block_enabled(s, true); >>>> } >>>> } >>> >>> ok with this. >> >> Or, as I commented on 1/3, maybe having a single property that is a >> tri-state enum value, instead of 2 separate boolean properties, might be >> nicer (but certainly a bit more complex to code up). > > If you teach me how to do the qapi/qmp part, I will do the other bits. > I don't really care if we do it one way or the other. > >>> I will add once here that when we disable block enabled, we also disable >>> shared, or just let it that way? >>> >>>> Another thing to mention: after switching to the capability interface, >>>> we'll cache the "enabled" and "shared" bits now while we don't cache >>>> it before, right? IIUC it'll affect behavior of such sequence: >>>> >>>> - 1st migrate with enabled=1, shared=1, then >>>> - 2nd migrate with enabled=0, shared=0 >>>> >>>> Before the series, the 2nd migrate will use enabled=shared=0, but >>>> after the series it should be using enabled=shared=1. Not sure whether >>>> this would be a problem (or I missed anything?). >>> >>> We can't be consistent with both old/new way. >>> >>> Old way: we always setup the capabilities on command line (that should >>> have been deprecated long, long ago) >> >> Well, the easy way out is to have the HMP migrate command (I assume >> that's what you mean by "on command line") explicitly clear the >> parameters if it is called without the -b/-i flag. So the start of each >> migration is what changes the properties, so long as you are still using >> HMP to start the migration. Or, on the QMP side, since 'migrate' has >> optional 'blk' and 'inc' booleans, basically leave the settings alone if >> the parameters were omitted, and explicitly update the property to the >> value of those parameters if they were present. > > We are going to have trouble whatever way that we do it, or we start > doing lots of strange things. > > Forget about qmp, we are going to assume that it is consistent with hmp. > > migrate_set_capabilities block_enabled on > migrate -b ..... > > Should migrate disable the block_enabled capability? Give one > warning/error? > > And notice that this only matter if we do a migration, we cancel/get one > error, and then we migrate again. > > What I tried to do is assume that -b/-i arguments don't exist. And if > the user use them, we implement its behaviour with the minimal fuss > possibly. > > Only way that I can think of being consistent and bug compatible will be > to store: > - old block_shared/enanbeld capability value > - if we set -b/-i on the command line > > In migration cleanup do the right thing depending on this four > variables. I think that it is adding lots of complexity for very few > improvement. > > >> Or is the proposal that we are also going to simplify the QMP 'migrate' >> command to get rid of crufty parameters? > > I didn't read it that way, but I would not oppose O:-) > > Later, Juan. I'm not too familiar with this stuff, so please correct my misunderstandings. "Normal" migration configuration is global state, i.e. it applies to all future migrations. Except the "migrate" command's flags apply to just the migration kicked off by that command. QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i). !blk && inc makes no sense and is silently treated like !blk && !inc. There's a third flag "detach" (HMP: -d), but it does nothing in QMP. You'd like to deprecate these flags in favour of "normal" configuration. However, we need to maintain QMP backward compatibility at least for a while. HMP backward compatibility is nice to have, but not required. First step is to design the new interface you want. Second step is to figure out backward compatibility. The new interface adds a block migration tri-state (off, non-incremental, incremental) to global state, default off. Whether it's done as two bools or an enum of three values doesn't matter here. If the new interface isn't used, the old one still needs to work. If it is used, the old one either has to do "the right thing", or fail cleanly. We approximate "new interface isn't used" by "block migration is off in global state". When it is off, the migration command needs to honor its two flags for compatibility. It must leave block migration off in global state. Yes, this will complicate the implementation until we actually remove the deprecated flags. Par for the backward compatility course. When block migration isn't off in global state, we can either * let the flags take precedence over the global state (one interpretation of "do the right thing"), or * reject flags that conflict with global state (another interpretation), or * reject *all* flags (fail cleanly). The last one looks perfectly servicable to me.
Markus Armbruster <armbru@redhat.com> wrote: > Juan Quintela <quintela@redhat.com> writes: > >> Eric Blake <eblake@redhat.com> wrote: >>> Or is the proposal that we are also going to simplify the QMP 'migrate' >>> command to get rid of crufty parameters? >> >> I didn't read it that way, but I would not oppose O:-) >> >> Later, Juan. > > I'm not too familiar with this stuff, so please correct my > misunderstandings. > > "Normal" migration configuration is global state, i.e. it applies to all > future migrations. > > Except the "migrate" command's flags apply to just the migration kicked > off by that command. > > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i). > !blk && inc makes no sense and is silently treated like !blk && !inc. > > There's a third flag "detach" (HMP: -d), but it does nothing in QMP. As qmp command is asynchronous, you can think that -d is *always* on in QMP O:-) > You'd like to deprecate these flags in favour of "normal" configuration. > However, we need to maintain QMP backward compatibility at least for a > while. HMP backward compatibility is nice to have, but not required. > > First step is to design the new interface you want. Second step is to > figure out backward compatibility. > > The new interface adds a block migration tri-state (off, > non-incremental, incremental) to global state, default off. Whether > it's done as two bools or an enum of three values doesn't matter here. Tristates will complicate it. I still think that: - capability: block_migration - parameter: block_shared Makes more sense, no? If block_migration is not enabled, we ignore the shared parameter. We already do that for other parameters. > If the new interface isn't used, the old one still needs to work. If it > is used, the old one either has to do "the right thing", or fail > cleanly. > > We approximate "new interface isn't used" by "block migration is off in > global state". When it is off, the migration command needs to honor its > two flags for compatibility. It must leave block migration off in > global state. Yes, this will complicate the implementation until we > actually remove the deprecated flags. Par for the backward compatility > course. > > When block migration isn't off in global state, we can either > > * let the flags take precedence over the global state (one > interpretation of "do the right thing"), or > > * reject flags that conflict with global state (another interpretation), > or > > * reject *all* flags (fail cleanly). > > The last one looks perfectly servicable to me. Yeap, I think that makes sense. If you use capabilities, parameters, old interface don't work at all. We still have a problem that is what happens if the user does: migrate -b <foo> migrate_cancel (or error) migrate <bar> (without -b) With current patches, it will still use -b. Fixing it requires still anding more code. But I think that this use case is so weird what we should not even care about it. Later, Juan.
* Juan Quintela (quintela@redhat.com) wrote: > Markus Armbruster <armbru@redhat.com> wrote: > > Juan Quintela <quintela@redhat.com> writes: > > > >> Eric Blake <eblake@redhat.com> wrote: > > > >>> Or is the proposal that we are also going to simplify the QMP 'migrate' > >>> command to get rid of crufty parameters? > >> > >> I didn't read it that way, but I would not oppose O:-) > >> > >> Later, Juan. > > > > I'm not too familiar with this stuff, so please correct my > > misunderstandings. > > > > "Normal" migration configuration is global state, i.e. it applies to all > > future migrations. > > > > Except the "migrate" command's flags apply to just the migration kicked > > off by that command. > > > > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i). > > !blk && inc makes no sense and is silently treated like !blk && !inc. > > > > There's a third flag "detach" (HMP: -d), but it does nothing in QMP. > > As qmp command is asynchronous, you can think that -d is *always* on in > QMP O:-) > > > You'd like to deprecate these flags in favour of "normal" configuration. > > However, we need to maintain QMP backward compatibility at least for a > > while. HMP backward compatibility is nice to have, but not required. > > > > First step is to design the new interface you want. Second step is to > > figure out backward compatibility. > > > > The new interface adds a block migration tri-state (off, > > non-incremental, incremental) to global state, default off. Whether > > it's done as two bools or an enum of three values doesn't matter here. > > Tristates will complicate it. I still think that: > > - capability: block_migration > - parameter: block_shared > > Makes more sense, no? I don't understand what making block_shared a parameter gives you as opposed to simply having two capabilities. (And how did we get 'shared'? We started off with block & incremental) Dave > If block_migration is not enabled, we ignore the shared parameter. We > already do that for other parameters. > > > If the new interface isn't used, the old one still needs to work. If it > > is used, the old one either has to do "the right thing", or fail > > cleanly. > > > > We approximate "new interface isn't used" by "block migration is off in > > global state". When it is off, the migration command needs to honor its > > two flags for compatibility. It must leave block migration off in > > global state. Yes, this will complicate the implementation until we > > actually remove the deprecated flags. Par for the backward compatility > > course. > > > > When block migration isn't off in global state, we can either > > > > * let the flags take precedence over the global state (one > > interpretation of "do the right thing"), or > > > > * reject flags that conflict with global state (another interpretation), > > or > > > > * reject *all* flags (fail cleanly). > > > > The last one looks perfectly servicable to me. > > Yeap, I think that makes sense. If you use capabilities, parameters, > old interface don't work at all. > > We still have a problem that is what happens if the user does: > > migrate -b <foo> > migrate_cancel (or error) > migrate <bar> (without -b) > > With current patches, it will still use -b. Fixing it requires still > anding more code. But I think that this use case is so weird what we > should not even care about it. > > Later, Juan. -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> Markus Armbruster <armbru@redhat.com> wrote: >> > Juan Quintela <quintela@redhat.com> writes: >> > >> >> Eric Blake <eblake@redhat.com> wrote: >> >> >> >>> Or is the proposal that we are also going to simplify the QMP 'migrate' >> >>> command to get rid of crufty parameters? >> >> >> >> I didn't read it that way, but I would not oppose O:-) >> >> >> >> Later, Juan. >> > >> > I'm not too familiar with this stuff, so please correct my >> > misunderstandings. >> > >> > "Normal" migration configuration is global state, i.e. it applies to all >> > future migrations. >> > >> > Except the "migrate" command's flags apply to just the migration kicked >> > off by that command. >> > >> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i). >> > !blk && inc makes no sense and is silently treated like !blk && !inc. >> > >> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP. >> >> As qmp command is asynchronous, you can think that -d is *always* on in >> QMP O:-) >> >> > You'd like to deprecate these flags in favour of "normal" configuration. >> > However, we need to maintain QMP backward compatibility at least for a >> > while. HMP backward compatibility is nice to have, but not required. >> > >> > First step is to design the new interface you want. Second step is to >> > figure out backward compatibility. >> > >> > The new interface adds a block migration tri-state (off, >> > non-incremental, incremental) to global state, default off. Whether >> > it's done as two bools or an enum of three values doesn't matter here. >> >> Tristates will complicate it. I still think that: >> >> - capability: block_migration >> - parameter: block_shared >> >> Makes more sense, no? > > I don't understand what making block_shared a parameter gives you as > opposed to simply having two capabilities. > > (And how did we get 'shared'? We started off with block & incremental) The variables on MigrationParams: struct MigrationParams { bool blk; bool shared; }; I can move to incremental. I am not sure which one is clearer. The advantage of having shared as a parameter is that we forget about all this dependency bussiness. Is the same than compression_threads paramter, you setup to whichever value that you want. But you don't get compression_threads until you set the compress capability. So, in this case we will have: block capability: Are we using block migration or not block-incremental parameter: If we are using block migration, are we using incremental copying of the block layer? Later, Juan.
* Juan Quintela (quintela@redhat.com) wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > * Juan Quintela (quintela@redhat.com) wrote: > >> Markus Armbruster <armbru@redhat.com> wrote: > >> > Juan Quintela <quintela@redhat.com> writes: > >> > > >> >> Eric Blake <eblake@redhat.com> wrote: > >> > >> > >> >>> Or is the proposal that we are also going to simplify the QMP 'migrate' > >> >>> command to get rid of crufty parameters? > >> >> > >> >> I didn't read it that way, but I would not oppose O:-) > >> >> > >> >> Later, Juan. > >> > > >> > I'm not too familiar with this stuff, so please correct my > >> > misunderstandings. > >> > > >> > "Normal" migration configuration is global state, i.e. it applies to all > >> > future migrations. > >> > > >> > Except the "migrate" command's flags apply to just the migration kicked > >> > off by that command. > >> > > >> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i). > >> > !blk && inc makes no sense and is silently treated like !blk && !inc. > >> > > >> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP. > >> > >> As qmp command is asynchronous, you can think that -d is *always* on in > >> QMP O:-) > >> > >> > You'd like to deprecate these flags in favour of "normal" configuration. > >> > However, we need to maintain QMP backward compatibility at least for a > >> > while. HMP backward compatibility is nice to have, but not required. > >> > > >> > First step is to design the new interface you want. Second step is to > >> > figure out backward compatibility. > >> > > >> > The new interface adds a block migration tri-state (off, > >> > non-incremental, incremental) to global state, default off. Whether > >> > it's done as two bools or an enum of three values doesn't matter here. > >> > >> Tristates will complicate it. I still think that: > >> > >> - capability: block_migration > >> - parameter: block_shared > >> > >> Makes more sense, no? > > > > I don't understand what making block_shared a parameter gives you as > > opposed to simply having two capabilities. > > > > (And how did we get 'shared'? We started off with block & incremental) > > The variables on MigrationParams: > > struct MigrationParams { > bool blk; > bool shared; > }; > > > I can move to incremental. I am not sure which one is clearer. > > The advantage of having shared as a parameter is that we forget about > all this dependency bussiness. Is the same than compression_threads > paramter, you setup to whichever value that you want. But you don't get > compression_threads until you set the compress capability. > > So, in this case we will have: > > block capability: Are we using block migration or not > block-incremental parameter: If we are using block migration, are we > using incremental copying of the block layer? If it's still a boolean why does having it as a parameter remove the dependency? Dave > > Later, Juan. -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: >> > * Juan Quintela (quintela@redhat.com) wrote: >> >> Markus Armbruster <armbru@redhat.com> wrote: >> >> > Juan Quintela <quintela@redhat.com> writes: >> >> > >> >> >> Eric Blake <eblake@redhat.com> wrote: >> >> >> >> >> >> >>> Or is the proposal that we are also going to simplify the QMP 'migrate' >> >> >>> command to get rid of crufty parameters? >> >> >> >> >> >> I didn't read it that way, but I would not oppose O:-) >> >> >> >> >> >> Later, Juan. >> >> > >> >> > I'm not too familiar with this stuff, so please correct my >> >> > misunderstandings. >> >> > >> >> > "Normal" migration configuration is global state, i.e. it applies to all >> >> > future migrations. >> >> > >> >> > Except the "migrate" command's flags apply to just the migration kicked >> >> > off by that command. >> >> > >> >> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i). >> >> > !blk && inc makes no sense and is silently treated like !blk && !inc. >> >> > >> >> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP. >> >> >> >> As qmp command is asynchronous, you can think that -d is *always* on in >> >> QMP O:-) >> >> >> >> > You'd like to deprecate these flags in favour of "normal" configuration. >> >> > However, we need to maintain QMP backward compatibility at least for a >> >> > while. HMP backward compatibility is nice to have, but not required. >> >> > >> >> > First step is to design the new interface you want. Second step is to >> >> > figure out backward compatibility. >> >> > >> >> > The new interface adds a block migration tri-state (off, >> >> > non-incremental, incremental) to global state, default off. Whether >> >> > it's done as two bools or an enum of three values doesn't matter here. >> >> >> >> Tristates will complicate it. I still think that: >> >> >> >> - capability: block_migration >> >> - parameter: block_shared >> >> >> >> Makes more sense, no? >> > >> > I don't understand what making block_shared a parameter gives you as >> > opposed to simply having two capabilities. >> > >> > (And how did we get 'shared'? We started off with block & incremental) >> >> The variables on MigrationParams: >> >> struct MigrationParams { >> bool blk; >> bool shared; >> }; >> >> >> I can move to incremental. I am not sure which one is clearer. >> >> The advantage of having shared as a parameter is that we forget about >> all this dependency bussiness. Is the same than compression_threads >> paramter, you setup to whichever value that you want. But you don't get >> compression_threads until you set the compress capability. >> >> So, in this case we will have: >> >> block capability: Are we using block migration or not >> block-incremental parameter: If we are using block migration, are we >> using incremental copying of the block layer? > > If it's still a boolean why does having it as a parameter remove the > dependency? Forget -b/-i. migration_set_parameter compression_threads 8 migrate <foo> We don't use compression_threads at all migrate_set_capability compress migrate <foo> Now, we use compression threads. So, compression_threads parameter is a parameter that is only used when compress capability is enabled. Same for block migration. Block_incremental parameter is used only when block migration capability is setup. No dependency check needed at all. Or I am losing something obvious here? Later, Juan.
* Juan Quintela (quintela@redhat.com) wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > * Juan Quintela (quintela@redhat.com) wrote: > >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > >> > * Juan Quintela (quintela@redhat.com) wrote: > >> >> Markus Armbruster <armbru@redhat.com> wrote: > >> >> > Juan Quintela <quintela@redhat.com> writes: > >> >> > > >> >> >> Eric Blake <eblake@redhat.com> wrote: > >> >> > >> >> > >> >> >>> Or is the proposal that we are also going to simplify the QMP 'migrate' > >> >> >>> command to get rid of crufty parameters? > >> >> >> > >> >> >> I didn't read it that way, but I would not oppose O:-) > >> >> >> > >> >> >> Later, Juan. > >> >> > > >> >> > I'm not too familiar with this stuff, so please correct my > >> >> > misunderstandings. > >> >> > > >> >> > "Normal" migration configuration is global state, i.e. it applies to all > >> >> > future migrations. > >> >> > > >> >> > Except the "migrate" command's flags apply to just the migration kicked > >> >> > off by that command. > >> >> > > >> >> > QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i). > >> >> > !blk && inc makes no sense and is silently treated like !blk && !inc. > >> >> > > >> >> > There's a third flag "detach" (HMP: -d), but it does nothing in QMP. > >> >> > >> >> As qmp command is asynchronous, you can think that -d is *always* on in > >> >> QMP O:-) > >> >> > >> >> > You'd like to deprecate these flags in favour of "normal" configuration. > >> >> > However, we need to maintain QMP backward compatibility at least for a > >> >> > while. HMP backward compatibility is nice to have, but not required. > >> >> > > >> >> > First step is to design the new interface you want. Second step is to > >> >> > figure out backward compatibility. > >> >> > > >> >> > The new interface adds a block migration tri-state (off, > >> >> > non-incremental, incremental) to global state, default off. Whether > >> >> > it's done as two bools or an enum of three values doesn't matter here. > >> >> > >> >> Tristates will complicate it. I still think that: > >> >> > >> >> - capability: block_migration > >> >> - parameter: block_shared > >> >> > >> >> Makes more sense, no? > >> > > >> > I don't understand what making block_shared a parameter gives you as > >> > opposed to simply having two capabilities. > >> > > >> > (And how did we get 'shared'? We started off with block & incremental) > >> > >> The variables on MigrationParams: > >> > >> struct MigrationParams { > >> bool blk; > >> bool shared; > >> }; > >> > >> > >> I can move to incremental. I am not sure which one is clearer. > >> > >> The advantage of having shared as a parameter is that we forget about > >> all this dependency bussiness. Is the same than compression_threads > >> paramter, you setup to whichever value that you want. But you don't get > >> compression_threads until you set the compress capability. > >> > >> So, in this case we will have: > >> > >> block capability: Are we using block migration or not > >> block-incremental parameter: If we are using block migration, are we > >> using incremental copying of the block layer? > > > > If it's still a boolean why does having it as a parameter remove the > > dependency? > > Forget -b/-i. > > migration_set_parameter compression_threads 8 > > migrate <foo> > > We don't use compression_threads at all > > migrate_set_capability compress > > migrate <foo> > > Now, we use compression threads. > > So, compression_threads parameter is a parameter that is only used when > compress capability is enabled. > > Same for block migration. Block_incremental parameter is used only when > block migration capability is setup. No dependency check needed at all. > > Or I am losing something obvious here? Ah, you've made up a new rule - I don't think it's a bad rule but is it true? Do we always enable a capability before we use a parameter? I don't think so - I think the tls parameters don't have a capability. My previous rule was just that if it was a bool it was a capability and you can have whatever dependencies you like there - or none. Dave > Later, Juan. -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: >> Forget -b/-i. >> >> migration_set_parameter compression_threads 8 >> >> migrate <foo> >> >> We don't use compression_threads at all >> >> migrate_set_capability compress >> >> migrate <foo> >> >> Now, we use compression threads. >> >> So, compression_threads parameter is a parameter that is only used when >> compress capability is enabled. >> >> Same for block migration. Block_incremental parameter is used only when >> block migration capability is setup. No dependency check needed at all. >> >> Or I am losing something obvious here? > > Ah, you've made up a new rule Oops, I thought that was a rule on how things worked O:-) >- I don't think it's a bad rule > but is it true? Do we always enable a capability before we use a > parameter? I don't think so - I think the tls parameters don't have > a capability. To use tls paramater, we need to use an url with tls, no? > My previous rule was just that if it was a bool it was a capability > and you can have whatever dependencies you like there - or none. Dunno. If you think that we can document it (one way or another), I am for it. It is really weird that we both thought (reading) the interface that the rules are different O:-) Later, Juan.
* Juan Quintela (quintela@redhat.com) wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > >> Forget -b/-i. > >> > >> migration_set_parameter compression_threads 8 > >> > >> migrate <foo> > >> > >> We don't use compression_threads at all > >> > >> migrate_set_capability compress > >> > >> migrate <foo> > >> > >> Now, we use compression threads. > >> > >> So, compression_threads parameter is a parameter that is only used when > >> compress capability is enabled. > >> > >> Same for block migration. Block_incremental parameter is used only when > >> block migration capability is setup. No dependency check needed at all. > >> > >> Or I am losing something obvious here? > > > > Ah, you've made up a new rule > > Oops, I thought that was a rule on how things worked O:-) > > >- I don't think it's a bad rule > > but is it true? Do we always enable a capability before we use a > > parameter? I don't think so - I think the tls parameters don't have > > a capability. > > To use tls paramater, we need to use an url with tls, no? > > > My previous rule was just that if it was a bool it was a capability > > and you can have whatever dependencies you like there - or none. > > Dunno. > > If you think that we can document it (one way or another), I am for it. > > It is really weird that we both thought (reading) the interface that the > rules are different O:-) Well I think that's because no one ever defined any rules! We just created parameters because we needed something that could take non-boolean values, but I don't think there's anything more about the structure of their use that's actually defined. Similarly, I don't think there's anything defined about the structure of capabilities. Dave > Later, Juan. -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Juan Quintela <quintela@redhat.com> writes: > Markus Armbruster <armbru@redhat.com> wrote: >> Juan Quintela <quintela@redhat.com> writes: >> >>> Eric Blake <eblake@redhat.com> wrote: > > >>>> Or is the proposal that we are also going to simplify the QMP 'migrate' >>>> command to get rid of crufty parameters? >>> >>> I didn't read it that way, but I would not oppose O:-) >>> >>> Later, Juan. >> >> I'm not too familiar with this stuff, so please correct my >> misunderstandings. >> >> "Normal" migration configuration is global state, i.e. it applies to all >> future migrations. >> >> Except the "migrate" command's flags apply to just the migration kicked >> off by that command. >> >> QMP command "migrate" has two flags "blk" (HMP: -b) and "inc" (HMP: -i). >> !blk && inc makes no sense and is silently treated like !blk && !inc. >> >> There's a third flag "detach" (HMP: -d), but it does nothing in QMP. > > As qmp command is asynchronous, you can think that -d is *always* on in > QMP O:-) Yes. The existence of "detach" in QMP is owed to limitations of early QMP infrastructure. It's flagged as "invalid" and "should not be used" since 2010. Perhaps we should start a section on QMP in <http://wiki.qemu.org/Features/LegacyRemoval>. But I'd like to first have a way to communicate "you're using a deprecated feature" warnings via QMP. >> You'd like to deprecate these flags in favour of "normal" configuration. >> However, we need to maintain QMP backward compatibility at least for a >> while. HMP backward compatibility is nice to have, but not required. >> >> First step is to design the new interface you want. Second step is to >> figure out backward compatibility. >> >> The new interface adds a block migration tri-state (off, >> non-incremental, incremental) to global state, default off. Whether >> it's done as two bools or an enum of three values doesn't matter here. > > Tristates will complicate it. I still think that: > > - capability: block_migration > - parameter: block_shared > > Makes more sense, no? > > If block_migration is not enabled, we ignore the shared parameter. We > already do that for other parameters. My impression as a superficial reader is that migration configuration is a historically grown mess. Perhaps we shouldn't try to interpret too much intent into it :) If we redo migration as an instance of the "job" abstraction once we have it, then migration configuration & control should become more less messy. Of course, the old messes will stay with us for a while in the form of backward compatibility messes. I'm not too particular on how we do the tri-state now, as long as it reasonably fits what we have, and is documented clearly. >> If the new interface isn't used, the old one still needs to work. If it >> is used, the old one either has to do "the right thing", or fail >> cleanly. >> >> We approximate "new interface isn't used" by "block migration is off in >> global state". When it is off, the migration command needs to honor its >> two flags for compatibility. It must leave block migration off in >> global state. Yes, this will complicate the implementation until we >> actually remove the deprecated flags. Par for the backward compatility >> course. >> >> When block migration isn't off in global state, we can either >> >> * let the flags take precedence over the global state (one >> interpretation of "do the right thing"), or >> >> * reject flags that conflict with global state (another interpretation), >> or >> >> * reject *all* flags (fail cleanly). >> >> The last one looks perfectly servicable to me. > > Yeap, I think that makes sense. If you use capabilities, parameters, > old interface don't work at all. > > We still have a problem that is what happens if the user does: > > migrate -b <foo> > migrate_cancel (or error) > migrate <bar> (without -b) > > With current patches, it will still use -b. Fixing it requires still > anding more code. But I think that this use case is so weird what we > should not even care about it. It's a compatibility break. Whether it's tolerable is a judgement call, and not for me to make. Compatibility breaks need documentation, including release notes. Say you run migrate with -b by accident (say by recalling a prior command from persistent command history, such as qmp-shell's or rlwrap's or socat READLINE's), immediately realize what you've done and cancel the migration. Are you then stuck with -b forever?
Markus Armbruster <armbru@redhat.com> wrote: > Juan Quintela <quintela@redhat.com> writes: ... >> As qmp command is asynchronous, you can think that -d is *always* on in >> QMP O:-) > > Yes. The existence of "detach" in QMP is owed to limitations of early > QMP infrastructure. It's flagged as "invalid" and "should not be > used" since 2010. > > Perhaps we should start a section on QMP in > <http://wiki.qemu.org/Features/LegacyRemoval>. But I'd like to first > have a way to communicate "you're using a deprecated feature" warnings > via QMP. +1 >> Tristates will complicate it. I still think that: >> >> - capability: block_migration >> - parameter: block_shared >> >> Makes more sense, no? >> >> If block_migration is not enabled, we ignore the shared parameter. We >> already do that for other parameters. > > My impression as a superficial reader is that migration configuration is > a historically grown mess. Perhaps we shouldn't try to interpret too > much intent into it :) > > If we redo migration as an instance of the "job" abstraction once we > have it, then migration configuration & control should become more less > messy. Of course, the old messes will stay with us for a while in the > form of backward compatibility messes. > > I'm not too particular on how we do the tri-state now, as long as it > reasonably fits what we have, and is documented clearly. >>> If the new interface isn't used, the old one still needs to work. If it >>> is used, the old one either has to do "the right thing", or fail >>> cleanly. >>> >>> We approximate "new interface isn't used" by "block migration is off in >>> global state". When it is off, the migration command needs to honor its >>> two flags for compatibility. It must leave block migration off in >>> global state. Yes, this will complicate the implementation until we >>> actually remove the deprecated flags. Par for the backward compatility >>> course. >>> >>> When block migration isn't off in global state, we can either >>> >>> * let the flags take precedence over the global state (one >>> interpretation of "do the right thing"), or >>> >>> * reject flags that conflict with global state (another interpretation), >>> or >>> >>> * reject *all* flags (fail cleanly). >>> >>> The last one looks perfectly servicable to me. >> >> Yeap, I think that makes sense. If you use capabilities, parameters, >> old interface don't work at all. >> >> We still have a problem that is what happens if the user does: >> >> migrate -b <foo> >> migrate_cancel (or error) >> migrate <bar> (without -b) >> >> With current patches, it will still use -b. Fixing it requires still >> anding more code. But I think that this use case is so weird what we >> should not even care about it. > > It's a compatibility break. Whether it's tolerable is a judgement call, > and not for me to make. > > Compatibility breaks need documentation, including release notes. > > Say you run migrate with -b by accident (say by recalling a prior > command from persistent command history, such as qmp-shell's or rlwrap's > or socat READLINE's), immediately realize what you've done and cancel > the migration. Are you then stuck with -b forever? migrate_set_capability block off and you are done. But I think that adding documentation would be longer that just adding the code to clean it. Later, Juan.
diff --git a/include/migration/migration.h b/include/migration/migration.h index 30c2913..2d5525c 100644 --- a/include/migration/migration.h +++ b/include/migration/migration.h @@ -39,8 +39,7 @@ #define QEMU_VM_SECTION_FOOTER 0x7e struct MigrationParams { - bool blk; - bool shared; + bool unused; /* C doesn't allow empty structs */ }; /* Messages sent on the return path from destination to source */ diff --git a/migration/block.c b/migration/block.c index 060087f..fcfa823 100644 --- a/migration/block.c +++ b/migration/block.c @@ -94,9 +94,6 @@ typedef struct BlkMigBlock { } BlkMigBlock; typedef struct BlkMigState { - /* Written during setup phase. Can be read without a lock. */ - int blk_enable; - int shared_base; QSIMPLEQ_HEAD(bmds_list, BlkMigDevState) bmds_list; int64_t total_sector_sum; bool zero_blocks; @@ -425,7 +422,7 @@ static int init_blk_migration(QEMUFile *f) bmds->bulk_completed = 0; bmds->total_sectors = sectors; bmds->completed_sectors = 0; - bmds->shared_base = block_mig_state.shared_base; + bmds->shared_base = migrate_use_block_shared(); assert(i < num_bs); bmds_bs[i].bmds = bmds; @@ -994,22 +991,12 @@ static int block_load(QEMUFile *f, void *opaque, int version_id) return 0; } -static void block_set_params(const MigrationParams *params, void *opaque) -{ - block_mig_state.blk_enable = params->blk; - block_mig_state.shared_base = params->shared; - - /* shared base means that blk_enable = 1 */ - block_mig_state.blk_enable |= params->shared; -} - static bool block_is_active(void *opaque) { - return block_mig_state.blk_enable == 1; + return migrate_use_block_enabled(); } static SaveVMHandlers savevm_block_handlers = { - .set_params = block_set_params, .save_live_setup = block_save_setup, .save_live_iterate = block_save_iterate, .save_live_complete_precopy = block_save_complete, diff --git a/migration/colo.c b/migration/colo.c index 963c802..e772384 100644 --- a/migration/colo.c +++ b/migration/colo.c @@ -14,6 +14,7 @@ #include "qemu/timer.h" #include "sysemu/sysemu.h" #include "migration/colo.h" +#include "migration/block.h" #include "io/channel-buffer.h" #include "trace.h" #include "qemu/error-report.h" @@ -345,8 +346,8 @@ static int colo_do_checkpoint_transaction(MigrationState *s, } /* Disable block migration */ - s->params.blk = 0; - s->params.shared = 0; + migrate_set_block_enabled(s, false); + migrate_set_block_shared(s, false); qemu_savevm_state_header(fb); qemu_savevm_state_begin(fb, &s->params); qemu_mutex_lock_iothread(); diff --git a/migration/migration.c b/migration/migration.c index 2f981aa..8a3bf89 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -787,6 +787,10 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params, s->enabled_capabilities[cap->value->capability] = cap->value->state; } + if (s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED]) { + s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_ENABLED] = true; + } + if (migrate_postcopy_ram()) { if (migrate_use_compression()) { /* The decompression threads asynchronously write into RAM @@ -1214,9 +1218,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, MigrationParams params; const char *p; - params.blk = has_blk && blk; - params.shared = has_inc && inc; - if (migration_is_setup_or_active(s->state) || s->state == MIGRATION_STATUS_CANCELLING || s->state == MIGRATION_STATUS_COLO) { @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, } if (has_inc && inc) { + migrate_set_block_enabled(s, true); migrate_set_block_shared(s, true); } diff --git a/migration/savevm.c b/migration/savevm.c index f8d1e8b..221fb4b 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1233,8 +1233,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp) { int ret; MigrationParams params = { - .blk = 0, - .shared = 0 }; MigrationState *ms = migrate_init(¶ms); MigrationStatus status;
We have change in the previous patch to use migration capabilities for it. Notice that we continue using the old command line flags from migrate command from the time being. Remove the set_params method as now it is empty. Signed-off-by: Juan Quintela <quintela@redhat.com> --- include/migration/migration.h | 3 +-- migration/block.c | 17 ++--------------- migration/colo.c | 5 +++-- migration/migration.c | 8 +++++--- migration/savevm.c | 2 -- 5 files changed, 11 insertions(+), 24 deletions(-)