Re: [PATCH V1 4/4] cpr: reboot mode

[Peter Xu]

On Thu, Oct 19, 2023 at 01:47:46PM -0700, Steve Sistare wrote:
> Add the cpr-reboot migration mode.  Usage:
> 
> $ qemu-system-$arch -monitor stdio ...
> QEMU 8.1.50 monitor - type 'help' for more information
> (qemu) migrate_set_capability x-ignore-shared on
> (qemu) migrate_set_parameter mode cpr-reboot
> (qemu) migrate -d file:vm.state
> (qemu) info status
> VM status: paused (postmigrate)
> (qemu) quit
> 
> $ qemu-system-$arch -monitor stdio -incoming defer ...
> QEMU 8.1.50 monitor - type 'help' for more information
> (qemu) migrate_set_capability x-ignore-shared on
> (qemu) migrate_set_parameter mode cpr-reboot
> (qemu) migrate_incoming file:vm.state
> (qemu) info status
> VM status: running
> 
> In this mode, the migrate command saves state to a file, allowing one
> to quit qemu, reboot to an updated kernel, and restart an updated version
> of qemu.  The caller must specify a migration URI that writes to and reads
> from a file.  Unlike normal mode, the use of certain local storage options
> does not block the migration, but the caller must not modify guest block
> devices between the quit and restart.  The guest RAM memory-backend must
> be shared, and the @x-ignore-shared migration capability must be set,
> to avoid saving RAM to the file.  Guest RAM must be non-volatile across
> reboot, such as by backing it with a dax device, but this is not enforced.
> The restarted qemu arguments must match those used to initially start qemu,
> plus the -incoming option.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  qapi/migration.json | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 184fb78..2d862fa 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -620,9 +620,23 @@
>  #
>  # @normal: the original form of migration. (since 8.2)
>  #
> +# @cpr-reboot: The migrate command saves state to a file, allowing one to
> +#              quit qemu, reboot to an updated kernel, and restart an updated
> +#              version of qemu.  The caller must specify a migration URI
> +#              that writes to and reads from a file.  Unlike normal mode,
> +#              the use of certain local storage options does not block the
> +#              migration, but the caller must not modify guest block devices
> +#              between the quit and restart.  The guest RAM memory-backend
> +#              must be shared, and the @x-ignore-shared migration capability
> +#              must be set, to avoid saving it to the file.  Guest RAM must
> +#              be non-volatile across reboot, such as by backing it with
> +#              a dax device, but this is not enforced.  The restarted qemu
> +#              arguments must match those used to initially start qemu, plus
> +#              the -incoming option. (since 8.2)

What happens if someone migrates with non-shared memory, or without
ignore-shared?  Is it only because it'll be slow saving and loading?

If that's required, we should fail the mode set if (1) non-shared memory is
used, or (2) x-ignore-shared is not enabled.  But I had a feeling it's the
other way round.

Reading the whole series, if it's so far all about "local storage", why
"cpr-reboot"?  Why not "local" or "local storage" as the name?

I had a feeling that this patchset mixed a lot of higher level use case
into the mode definition.  IMHO we should provide clear definition of each
mode on what it does.  It's so far not so clear to me, even if I kind of
know what you plan to do.

I tried again google what CPR is for and found this:

https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08541.html

I also prefer spell it out, at least make it clear on what that means..  I
didn't even see "Checkpoint/restart" words mentioned anywhere in this
patchset.

Besides: do you have a tree somewhere for the whole set of latest CPR work?

Thanks,

> +#
>  ##
>  { 'enum': 'MigMode',
> -  'data': [ 'normal' ] }
> +  'data': [ 'normal', 'cpr-reboot' ] }
>  
>  ##
>  # @BitmapMigrationBitmapAliasTransform:
> -- 
> 1.8.3.1
> 

-- 
Peter Xu