Here's the 3rd version of file-based migration support [0]. For background
check the cover letter of the initial. The main changes are :

- Updated commit message as per Daniel Berrange's suggestino for Patches 1-2

- Fixed tab in various pages

- Added comments better explaining how json_writer_start_object in
qemu_savevm_state_header is matched and also squashed the analyze-migration.py
parts into patch 3

- Reworked the way pwritv/preadv are introduced. Now there are generic
callbacks in QIOChannel that are implemented for the QIOChannelFile.

- Separated the introduction of QEMUFile-related helpers from the patch
introducing the io interfaces.

- Added qtests for the file-based migration as well as for the fixed-ram
feature.

[0] https://lore.kernel.org/qemu-devel/20221004123733.2745519-1-nborisov@suse.com/

Nikolay Borisov (14):
  migration: support file: uri for source migration
  migration: Add support for 'file:' uri for incoming migration
  migration: Initial support of fixed-ram feature for
    analyze-migration.py
  io: Add generic pwritev/preadv interface
  io: implement io_pwritev for QIOChannelFile
  io: add and implement QIO_CHANNEL_FEATURE_SEEKABLE for channel file
  migration/qemu-file: add utility methods for working with seekable
    channels
  io: Add preadv support to QIOChannelFile
  migration: add qemu_get_buffer_at
  migration/ram: Introduce 'fixed-ram' migration stream capability
  migration: Refactor precopy ram loading code
  migration: Add support for 'fixed-ram' migration restore
  tests: Add migrate_incoming_qmp helper
  tests/qtest: migration-test: Add tests for file-based migration

 include/exec/ramblock.h             |   7 +
 include/io/channel.h                |  50 +++++
 include/migration/qemu-file-types.h |   2 +
 io/channel-file.c                   |  61 ++++++
 io/channel.c                        |  26 +++
 migration/file.c                    |  38 ++++
 migration/file.h                    |  10 +
 migration/meson.build               |   1 +
 migration/migration.c               |  61 +++++-
 migration/migration.h               |   6 +
 migration/qemu-file.c               |  82 +++++++
 migration/qemu-file.h               |   4 +
 migration/ram.c                     | 328 +++++++++++++++++++++-------
 migration/savevm.c                  |  48 ++--
 qapi/migration.json                 |   2 +-
 scripts/analyze-migration.py        |  49 ++++-
 tests/qtest/migration-helpers.c     |  19 ++
 tests/qtest/migration-helpers.h     |   4 +
 tests/qtest/migration-test.c        |  46 ++++
 19 files changed, 743 insertions(+), 101 deletions(-)
 create mode 100644 migration/file.c
 create mode 100644 migration/file.h

--
2.34.1

Hello Daniel and all,

resurrecting this series from end of last year,

do we think that this is the right approach and first step to be able to provide good performance for virsh save and virsh restore?

For reference, our previous attempt to get the performance for our use case (libvirt + qemu virsh save and restore under 5 seconds for a 30GB VM on NVMe disk) is here:

https://listman.redhat.com/archives/libvir-list/2022-June/232252.html

However since the option of a libvirt-only solution is not acceptable for upstream libvirt, Nikolay's attempt to create a file:/// migration target in QEMU
seemed to be the next preparatory step.

Do we still agree on this way forward, any comments? Thanks,

Claudio

On 10/28/22 12:39, Nikolay Borisov wrote:
> Here's the 3rd version of file-based migration support [0]. For background
> check the cover letter of the initial. The main changes are :
> 
> - Updated commit message as per Daniel Berrange's suggestino for Patches 1-2
> 
> - Fixed tab in various pages
> 
> - Added comments better explaining how json_writer_start_object in
> qemu_savevm_state_header is matched and also squashed the analyze-migration.py
> parts into patch 3
> 
> - Reworked the way pwritv/preadv are introduced. Now there are generic
> callbacks in QIOChannel that are implemented for the QIOChannelFile.
> 
> - Separated the introduction of QEMUFile-related helpers from the patch
> introducing the io interfaces.
> 
> - Added qtests for the file-based migration as well as for the fixed-ram
> feature.
> 
> [0] https://lore.kernel.org/qemu-devel/20221004123733.2745519-1-nborisov@suse.com/
> 
> Nikolay Borisov (14):
>   migration: support file: uri for source migration
>   migration: Add support for 'file:' uri for incoming migration
>   migration: Initial support of fixed-ram feature for
>     analyze-migration.py
>   io: Add generic pwritev/preadv interface
>   io: implement io_pwritev for QIOChannelFile
>   io: add and implement QIO_CHANNEL_FEATURE_SEEKABLE for channel file
>   migration/qemu-file: add utility methods for working with seekable
>     channels
>   io: Add preadv support to QIOChannelFile
>   migration: add qemu_get_buffer_at
>   migration/ram: Introduce 'fixed-ram' migration stream capability
>   migration: Refactor precopy ram loading code
>   migration: Add support for 'fixed-ram' migration restore
>   tests: Add migrate_incoming_qmp helper
>   tests/qtest: migration-test: Add tests for file-based migration
> 
>  include/exec/ramblock.h             |   7 +
>  include/io/channel.h                |  50 +++++
>  include/migration/qemu-file-types.h |   2 +
>  io/channel-file.c                   |  61 ++++++
>  io/channel.c                        |  26 +++
>  migration/file.c                    |  38 ++++
>  migration/file.h                    |  10 +
>  migration/meson.build               |   1 +
>  migration/migration.c               |  61 +++++-
>  migration/migration.h               |   6 +
>  migration/qemu-file.c               |  82 +++++++
>  migration/qemu-file.h               |   4 +
>  migration/ram.c                     | 328 +++++++++++++++++++++-------
>  migration/savevm.c                  |  48 ++--
>  qapi/migration.json                 |   2 +-
>  scripts/analyze-migration.py        |  49 ++++-
>  tests/qtest/migration-helpers.c     |  19 ++
>  tests/qtest/migration-helpers.h     |   4 +
>  tests/qtest/migration-test.c        |  46 ++++
>  19 files changed, 743 insertions(+), 101 deletions(-)
>  create mode 100644 migration/file.c
>  create mode 100644 migration/file.h
> 
> --
> 2.34.1
> 
>

On Thu, Feb 09, 2023 at 02:32:01PM +0100, Claudio Fontana wrote:
> Hello Daniel and all,
> 
> resurrecting this series from end of last year,
> 
> do we think that this is the right approach and first step to
> be able to provide good performance for virsh save and virsh
> restore?

I've looked through the series in some more detail now and will
send review comments separately. Overall, I'm pretty pleased with
the series and think it is on the right path. The new format it
provides should be amenable to parallel I/O with multifd and
be able to support O_DIRECT to avoid burning through the host I/O
cache.

There is obviously a bit of extra complexity from having a new
way to map RAM to the output, but it looks fairly well contained
in just a couple of places of the code. The performance wins
should be able to justify the extra maint burden IMHO.

> Do we still agree on this way forward, any comments? Thanks,

I'm not a migration maintainer, but overall I think it is
good.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

(adding Fabiano to the thread)

On 2/10/23 16:35, Daniel P. Berrangé wrote:
> On Thu, Feb 09, 2023 at 02:32:01PM +0100, Claudio Fontana wrote:
>> Hello Daniel and all,
>>
>> resurrecting this series from end of last year,
>>
>> do we think that this is the right approach and first step to
>> be able to provide good performance for virsh save and virsh
>> restore?
> 
> I've looked through the series in some more detail now and will
> send review comments separately. Overall, I'm pretty pleased with
> the series and think it is on the right path. The new format it
> provides should be amenable to parallel I/O with multifd and
> be able to support O_DIRECT to avoid burning through the host I/O
> cache.

Just wanted to add a thought we had with Fabiano a few days ago:

experimentally, it is clear that fixed-ram is an optimization, but the actual scalability seems to come from the successive parallel I/O with multifd.

Since the goal is being able to transfer _to disk_ (fdatasync) the whole 30G memory in 5 seconds, the need to split the cpu-intensive work into smaller tasks remains,
and the main scalability solution seems to come from the multifd part of the work (or another way to split the problem), combined with the O_DIRECT friendliness to avoid the trap of the cache trashing.

Not adding much, just highlighting that fixed-ram _alone_ does not seem to suffice, we probably need all pieces of the puzzle in place.

Thanks!

Claudio

> 
> There is obviously a bit of extra complexity from having a new
> way to map RAM to the output, but it looks fairly well contained
> in just a couple of places of the code. The performance wins
> should be able to justify the extra maint burden IMHO.
> 
>> Do we still agree on this way forward, any comments? Thanks,
> 
> I'm not a migration maintainer, but overall I think it is
> good.
> 
> With regards,
> Daniel

On Mon, Mar 20, 2023 at 12:14:53PM +0100, Claudio Fontana wrote:
> (adding Fabiano to the thread)
> 
> On 2/10/23 16:35, Daniel P. Berrangé wrote:
> > On Thu, Feb 09, 2023 at 02:32:01PM +0100, Claudio Fontana wrote:
> >> Hello Daniel and all,
> >>
> >> resurrecting this series from end of last year,
> >>
> >> do we think that this is the right approach and first step to
> >> be able to provide good performance for virsh save and virsh
> >> restore?
> > 
> > I've looked through the series in some more detail now and will
> > send review comments separately. Overall, I'm pretty pleased with
> > the series and think it is on the right path. The new format it
> > provides should be amenable to parallel I/O with multifd and
> > be able to support O_DIRECT to avoid burning through the host I/O
> > cache.
> 
> Just wanted to add a thought we had with Fabiano a few days ago:
> 
> experimentally, it is clear that fixed-ram is an optimization, but the actual scalability seems to come from the successive parallel I/O with multifd.
> 
> Since the goal is being able to transfer _to disk_ (fdatasync) the whole 30G memory in 5 seconds, the need to split the cpu-intensive work into smaller tasks remains,
> and the main scalability solution seems to come from the multifd part of the work (or another way to split the problem), combined with the O_DIRECT friendliness to avoid the trap of the cache trashing.
> 
> Not adding much, just highlighting that fixed-ram _alone_ does not seem to suffice, we probably need all pieces of the puzzle in place.

Agreed, the only thing that fixed-ram alone really does is to
ensure you have a finite file size if you're saving the state
while the VM is running, because successive dirtying of the
same block will write to the same location in file. Nice to
avoid wasting storage and gives a small speed up on restore.

It isn't going to give the massive speedup you're looking for
though. multifd + O_DIRECT is a critical followup piece to
achieve the major performance improvement. The fixed-ram
feature makes it practical to use multifd, without needing
to split content streams across many files, or have to invent
a way to splice multiple streams into the same file.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|