Re: [Qemu-devel] [PATCH 3/5] migration: Remove use of old MigrationParams

[Markus Armbruster]

Markus Armbruster  writes:

> Juan Quintela  writes:
>
>> 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.
>>
>> For savevm, one can't do a:
>>
>> savevm -b/-i foo
>
> Yes (savem has no such options).
>
>> but now one can do:
>>
>> migrate_set_capability block on
>> savevm foo
>>
>> And we can't use block migration. We could disable block capability
>> unconditionally, but it would not be much better.
>
> This leaves me confused: what does the example do?  Reading ahead...
> looks like it fails with "Block migration and snapshots are
> incompatible".  What are you trying to say here?

I think I now get what you're trying to say, but only because I've
picked up enough context.  Let me try to rephrase:

  migration: Use new configuration instead of old MigrationParams

  The previous commit introduced a MigrationCapability and a
  MigrationParameter for block migration.  Use them instead of the old
  MigrationParams.

  Take care to reject attempts to combine block migration with
  snapshots, e.g. like this:

  migrate_set_capability block on
  savevm foo

>> Signed-off-by: Juan Quintela 
>> Reviewed-by: Eric Blake 
>
> Patch looks good to me.

Preferably with a commit message I can still understand three weeks from
now:
Reviewed-by: Markus Armbruster 

Re: [Qemu-devel] [PATCH 3/5] migration: Remove use of old MigrationParams

[Markus Armbruster]

I got confused and replied to an old version.  Please ignore.

Markus Armbruster  writes:

> Juan Quintela  writes:
>
>> 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
>
> for the time being
>
>> now it is empty.
>>
>> For savevm, one can't do a:
>>
>> savevm -b/-i foo
>>
>> but now one can do:
>>
>> migrate_set_capability block on
>> savevm foo
>>
>> And we can't use block migration. We could disable block capability
>> unconditionally, but it would not be much better.
>
> I think I get what you're trying to say, but only because I have plenty
> of context right now.  Let me try to rephrase:
>
>   migration: Use new configuration instead of old MigrationParams
>
>   The previous commit introduced a MigrationCapability and a
>   MigrationParameter for block migration.  Use them instead of the old
>   MigrationParams.
>
>   Take care to reject attempts to combine block migration with
>   snapshots, e.g. like this:
>
>   migrate_set_capability block on
>   savevm foo
>
>> Signed-off-by: Juan Quintela 
>
> Preferably with a commit message I can still understand three weeks from
> now:
> Reviewed-by: Markus Armbruster 

Re: [Qemu-devel] [PATCH 3/5] migration: Remove use of old MigrationParams

[Markus Armbruster]

Juan Quintela  writes:

> 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

for the time being

> now it is empty.
>
> For savevm, one can't do a:
>
> savevm -b/-i foo
>
> but now one can do:
>
> migrate_set_capability block on
> savevm foo
>
> And we can't use block migration. We could disable block capability
> unconditionally, but it would not be much better.

I think I get what you're trying to say, but only because I have plenty
of context right now.  Let me try to rephrase:

  migration: Use new configuration instead of old MigrationParams

  The previous commit introduced a MigrationCapability and a
  MigrationParameter for block migration.  Use them instead of the old
  MigrationParams.

  Take care to reject attempts to combine block migration with
  snapshots, e.g. like this:

  migrate_set_capability block on
  savevm foo

> Signed-off-by: Juan Quintela 

Preferably with a commit message I can still understand three weeks from
now:
Reviewed-by: Markus Armbruster 

Re: [Qemu-devel] [PATCH 3/5] migration: Remove use of old MigrationParams

[Markus Armbruster]

Juan Quintela  writes:

> 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.
>
> For savevm, one can't do a:
>
> savevm -b/-i foo

Yes (savem has no such options).

> but now one can do:
>
> migrate_set_capability block on
> savevm foo
>
> And we can't use block migration. We could disable block capability
> unconditionally, but it would not be much better.

This leaves me confused: what does the example do?  Reading ahead...
looks like it fails with "Block migration and snapshots are
incompatible".  What are you trying to say here?

> Signed-off-by: Juan Quintela 
> Reviewed-by: Eric Blake 

Patch looks good to me.

Re: [Qemu-devel] [PATCH 3/5] migration: Remove use of old MigrationParams

[Juan Quintela]

Eric Blake  wrote:
> On 05/17/2017 10:38 AM, 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.
>> 
>> For savevm, one can't do a:
>> 
>> savevm -b/-i foo
>> 
>> but now one can do:
>> 
>> migrate_set_capability block on
>> savevm foo
>> 
>> And we can't use block migration. We could disable block capability
>> unconditionally, but it would not be much better.
>> 
>> Signed-off-by: Juan Quintela 
>> 
>> ---
>> - Maintain shared/enabled dependency (Xu suggestion)
>> - Now we maintain the dependency on the setter functions
>> - improve error messages
>> 
>> Signed-off-by: Juan Quintela 
>
> This second S-o-b gets stripped, but the one that counts is in place
> (I've made the same mistake before, as well - doing 'git commit --amend
> -s' on a commit where I already had a --- separator)

And this time I *really* remove the -s from my git-format-patch alias.
I dropped that last week on the terminal, rebooted the machine and
. magic ... got the old alias O:-)

> Reviewed-by: Eric Blake 

Thanks

Re: [Qemu-devel] [PATCH 3/5] migration: Remove use of old MigrationParams

[Eric Blake]

On 05/17/2017 10:38 AM, 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.
> 
> For savevm, one can't do a:
> 
> savevm -b/-i foo
> 
> but now one can do:
> 
> migrate_set_capability block on
> savevm foo
> 
> And we can't use block migration. We could disable block capability
> unconditionally, but it would not be much better.
> 
> Signed-off-by: Juan Quintela 
> 
> ---
> - Maintain shared/enabled dependency (Xu suggestion)
> - Now we maintain the dependency on the setter functions
> - improve error messages
> 
> Signed-off-by: Juan Quintela 

This second S-o-b gets stripped, but the one that counts is in place
(I've made the same mistake before, as well - doing 'git commit --amend
-s' on a commit where I already had a --- separator)

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 3/5] migration: Remove use of old MigrationParams

[Markus Armbruster]

Juan Quintela  writes:

> Markus Armbruster  wrote:
>> Juan Quintela  writes:
>>
>>> 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 
>
>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>> index b4f736f..5fc10ab 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();
>>>  MigrationStatus status;
>>> @@ -1245,6 +1243,12 @@ static int qemu_savevm_state(QEMUFile *f, Error 
>>> **errp)
>>>  goto done;
>>>  }
>>>  
>>> +if (migrate_use_block()) {
>>> +error_setg(errp, "Block migration and snapshots are incompatible");
>>> +ret = -EINVAL;
>>> +goto done;
>>> +}
>>> +
>>>  qemu_mutex_unlock_iothread();
>>>  qemu_savevm_state_header(f);
>>>  qemu_savevm_state_begin(f, );
>>
>> New error that isn't mentioned in the commit message.  Does it belong
>> here?
>
> It is the equivalent of the previous chunk.
>
> If you do a savevm, you can't do a:
>
> savevm -b/-i foo
>
> but now you can do:
>
> migrate_set_capability block on
> savevm foo
>
> And we can't use block migration.  If you preffer, I can disable the
> feature unconditionally, but I am not sure that it is much better.

I see.

Add your explanation to the commit message, and I'm happy.

Re: [Qemu-devel] [PATCH 3/5] migration: Remove use of old MigrationParams

[Juan Quintela]

Markus Armbruster  wrote:
> Juan Quintela  writes:
>
>> 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 

>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index b4f736f..5fc10ab 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();
>>  MigrationStatus status;
>> @@ -1245,6 +1243,12 @@ static int qemu_savevm_state(QEMUFile *f, Error 
>> **errp)
>>  goto done;
>>  }
>>  
>> +if (migrate_use_block()) {
>> +error_setg(errp, "Block migration and snapshots are incompatible");
>> +ret = -EINVAL;
>> +goto done;
>> +}
>> +
>>  qemu_mutex_unlock_iothread();
>>  qemu_savevm_state_header(f);
>>  qemu_savevm_state_begin(f, );
>
> New error that isn't mentioned in the commit message.  Does it belong
> here?

It is the equivalent of the previous chunk.

If you do a savevm, you can't do a:

savevm -b/-i foo

but now you can do:

migrate_set_capability block on
savevm foo

And we can't use block migration.  If you preffer, I can disable the
feature unconditionally, but I am not sure that it is much better.

Later, Juan.

Re: [Qemu-devel] [PATCH 3/5] migration: Remove use of old MigrationParams

[Markus Armbruster]

Juan Quintela  writes:

> 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 
>
> ---
> - Maintain shared/enabled dependency (Xu suggestion)
> - Now we maintain the dependency on the setter functions
> ---
>  include/migration/migration.h |  3 +--
>  migration/block.c | 17 ++---
>  migration/colo.c  |  4 ++--
>  migration/migration.c |  3 ---
>  migration/savevm.c|  8 ++--
>  5 files changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index adcb8f6..1277226 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -43,8 +43,7 @@
>  extern int only_migratable;
>  
>  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..5d22926 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_incremental();
>  
>  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();
>  }
>  
>  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..8c86892 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,7 @@ static int colo_do_checkpoint_transaction(MigrationState 
> *s,
>  }
>  
>  /* Disable block migration */
> -s->params.blk = 0;
> -s->params.shared = 0;
> +migrate_set_block_enabled(false, _err);
>  qemu_savevm_state_header(fb);
>  qemu_savevm_state_begin(fb, >params);
>  qemu_mutex_lock_iothread();
> diff --git a/migration/migration.c b/migration/migration.c
> index 03f96df..c03f34e 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1249,9 +1249,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool 
> blk,
>  bool block_enabled = false;
>  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) {
> diff --git a/migration/savevm.c b/migration/savevm.c
> index b4f736f..5fc10ab 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();
>  MigrationStatus status;
> @@ -1245,6 +1243,12 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>  goto done;
>  }
>  
> +if (migrate_use_block()) {
> +error_setg(errp, "Block migration and snapshots are incompatible");
> +ret = -EINVAL;
> +goto done;
> +}
> +
>  qemu_mutex_unlock_iothread();
>  qemu_savevm_state_header(f);
>  qemu_savevm_state_begin(f, );

New error that isn't mentioned in the commit message.  Does it belong
here?