Re: [PATCH v2 14/29] migration/ram: Introduce 'fixed-ram' migration capability

[Markus Armbruster <armbru@redhat.com>]

Fabiano Rosas <farosas@suse.de> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> Add a new migration capability 'fixed-ram'.
>>>
>>> The core of the feature is to ensure that each ram page has a specific
>>> offset in the resulting migration stream. The reason why we'd want
>>> such behavior are two fold:
>>>
>>>  - When doing a 'fixed-ram' migration the resulting file will have a
>>>    bounded size, since pages which are dirtied multiple times will
>>>    always go to a fixed location in the file, rather than constantly
>>>    being added to a sequential stream. This eliminates cases where a vm
>>>    with, say, 1G of ram can result in a migration file that's 10s of
>>>    GBs, provided that the workload constantly redirties memory.
>>>
>>>  - It paves the way to implement DIRECT_IO-enabled save/restore of the
>>>    migration stream as the pages are ensured to be written at aligned
>>>    offsets.
>>>
>>> For now, enabling the capability has no effect. The next couple of
>>> patches implement the core funcionality.
>>>
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>>  docs/devel/migration.rst | 14 ++++++++++++++
>>>  migration/options.c      | 37 +++++++++++++++++++++++++++++++++++++
>>>  migration/options.h      |  1 +
>>>  migration/savevm.c       |  1 +
>>>  qapi/migration.json      |  5 ++++-
>>>  5 files changed, 57 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
>>> index c3e1400c0c..6f898b5dbd 100644
>>> --- a/docs/devel/migration.rst
>>> +++ b/docs/devel/migration.rst
>>> @@ -566,6 +566,20 @@ Others (especially either older devices or system devices which for
>>>  some reason don't have a bus concept) make use of the ``instance id``
>>>  for otherwise identically named devices.
>>>  
>>> +Fixed-ram format
>>> +----------------
>>> +
>>> +When the ``fixed-ram`` capability is enabled, a slightly different
>>> +stream format is used for the RAM section. Instead of having a
>>> +sequential stream of pages that follow the RAMBlock headers, the dirty
>>> +pages for a RAMBlock follow its header. This ensures that each RAM
>>> +page has a fixed offset in the resulting migration stream.
>>
>> This requires the migration stream to be seekable, as documented in the
>> QAPI schema below.  I think it's worth documenting here, as well.
>>
>
> Ok.
>
>>> +
>>> +The ``fixed-ram`` capaility can be enabled in both source and
>>> +destination with:
>>> +
>>> +    ``migrate_set_capability fixed-ram on``
>>
>> Effect of enabling on the destination?
>>
>> What happens when we enable it only on one end?
>>
>
> qemu-system-x86_64: Capability fixed-ram is off, but received capability is on
> qemu-system-x86_64: load of migration failed: Invalid argument
>
> So I guess that *can* be enabled up there should become a *must*.

Makes sense.

>>> +
>>>  Return path
>>>  -----------
>>>  
>>
>> [...]
>>
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index 74f12adc0e..1317dd32ab 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -527,6 +527,9 @@
>>>  #     VM before migration for an optimal migration performance.
>>>  #     Enabled by default. (since 8.1)
>>>  #
>>> +# @fixed-ram: Migrate using fixed offsets for each RAM page. Requires
>>
>> Two spaces between sentences for consistency, please.
>>
>>> +#             a seekable transport such as a file.  (since 8.1)
>>
>> What is a migration transport?  migration.json doesn't define the term.
>>
>
> The medium that transports the migration. We are about to define some
> terms at the QAPI series:
>
> [PATCH v15 00/14] migration: Modify 'migrate' and 'migrate-incoming'
> QAPI commands for migration
> https://lore.kernel.org/r/20231023182053.8711-1-farosas@suse.de

Can't find it there offhand.  No need to explain it further to me now,
just make sure it's defined at this point in the series when you respin.

>> Which transports are seekable?
>>
>
> The ones that implement QIO_CHANNEL_FEATURE_SEEKABLE. Currently only
> QIOChannelFile.

Transport seekability needs to be documented clearly.

>> Out of curiosity: what happens if the transport isn't seekable?
>
> We fail the migration. At migration_channels_and_uri_compatible():
>
>     if (migration_needs_seekable_channel() &&
>         !uri_supports_seeking(uri)) {
>         error_setg(errp, "Migration requires seekable transport (e.g. file)");
>         compatible = false;
>     }

Thanks!

>>> +#
>>>  # Features:
>>>  #
>>>  # @unstable: Members @x-colo and @x-ignore-shared are experimental.
>>> @@ -543,7 +546,7 @@
>>>             { 'name': 'x-ignore-shared', 'features': [ 'unstable' ] },
>>>             'validate-uuid', 'background-snapshot',
>>>             'zero-copy-send', 'postcopy-preempt', 'switchover-ack',
>>> -           'dirty-limit', 'auto-pause'] }
>>> +           'dirty-limit', 'auto-pause', 'fixed-ram'] }
>>>  
>>>  ##
>>>  # @MigrationCapabilityStatus: