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

[Peter Xu]

On Mon, Oct 23, 2023 at 02:51:50PM -0400, Steven Sistare wrote:
> On 10/23/2023 2:29 PM, Steven Sistare wrote:
> > On 10/23/2023 11:39 AM, Peter Xu wrote:
> >> 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.
> > 
> > Juan also asked me to clarify this.  I plan to resubmit this:
> > 
> > #                                        ...  Private guest RAM is saved in
> > #              the file.  To avoid this cost, the guest RAM memory-backend
> > #              must be shared, and the @x-ignore-shared migration capability
> > #              must be set.  ...

Okay.  We can also avoid mentioning "private guest RAM is saved to ..."
because that's what migration already does.  IMO we can simplify all that
to:

  It is suggested to use share memory with x-ignore-shared when using this
  mode.

> > 
> >>
> >> 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?
> > 
> > The use case is about rebooting and updating the host, so reboot is in 
> > the name.  Local storage just happens to be allowed for it.
> > 
> >> 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 believe I already have, in the cover letter, commit message, and qapi 
> > definition, at the start of each:
> > 
> > # @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.

I think this is why I'm confused: above sentence is describing a very
generic migration to file scenario to me.  IOW, I think I can get the same
result described even with normal migration to file, or am I wrong?

IMHO the description here needs to explain the difference and when an user
should use this mode.  I think the real answer resides in your whole set,
I'll try to read that.

In all cases, can we name it something like "live-upgrade" v.s. "normal"?

> > 
> > The cover letter hints at the cpr-exec use case, and the long V9 patch 
> > series
> > describes it, and I will make sure the use case comes first when I submit 
> > cpr-exec,
> > which is:
>     * restart an updated version of qemu     (I buried the lead - steve)
> >   * much shorter guest downtime than cpr reboot
> >   * support vfio without requiring guest suspension
> >   * keep certain character devices alive
> > 
> >> 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.
> > 
> > Will do.
> > 
> >> Besides: do you have a tree somewhere for the whole set of latest CPR work?
> > 
> > I have the V9 patch series:
> >   
> > https://lore.kernel.org/qemu-devel/1658851843-236870-1-git-send-email-steven.sistare@oracle.com
> > and I can re-send my proposal for breaking it down into patch sets that I 
> > presented in the
> > qemu community meeting, if you did not save it.

No need to resend.  A link is exactly what I need; git tree even better.
I'll comment when I get something when reading that.

Thanks,

-- 
Peter Xu