Re: [PATCH v3 02/14] migration: Add support for 'file:' uri for incoming migration

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Oct 28, 2022 at 01:39:02PM +0300, Nikolay Borisov wrote:
> This is a counterpart to the 'file:' uri support for source migration,
> now a file can also serve as the source of an incoming migration.
> 
> Unlike other migration protocol backends, the 'file' protocol cannot
> honour non-blocking mode. POSIX file/block storage will always report
> ready to read/write, regardless of how slow the underlying storage
> will be at servicing the request.
> 
> For incoming migration this limitation may result in the main event
> loop not being fully responsive while loading the VM state. This
> won't impact the VM since it is not running at this phase, however,
> it may impact management applications.

Looking at the QEMU incoming migration code, we're relying on
coroutines. Specifically we initiate the load of state / RAM
in process_incoming_migration_co, and the coroutine magic
is handled in qemu_fill_buffer in migration/qemu-file.c which
does the coroutine yield.

This should be allowing the QMP monitor to be functional while
incoming migration is loaded. That said, I expect a great many
QMP commands will cause problems if invoked, especially any
that hotplug/unplug stuff. IOW, probably only safe to use the
query-xxx commands in general.

With the unhelpful POSIX semantics for poll() on plain files /
block devices, the qemu_fill_buffer method is unlikely to ever
see  QIO_CHANNEL_ERR_BLOCK, so is unlikely to ever yield in the
coroutine. Thus the QMP monitor will not process data until the
migration load is finished.

For libvirt this is not actually a problem in practice from what
I understand, as we don't try to run any QMP commands, we merely
wait until we see an even indicating migration is finished. 

To solve this problem we would need to make the incoming migration
run inside a thread, instead of coroutine. This does not appear
like it would be too hard, mostly just need to replace the
coroutine creation command with a thread creation command and
switch the QEMUFile to blocking  mode.

The open question is whether there are any locking/concurrency
concerns with doing this. To some extent those concerns would
already exist, even with using coroutines, as there are other
threads present, and QMP commands can change state in the
middle of a migration load if an app is foolish enough to
request it.  So maybe switching to threads isn't actually
that hard ?

Either way, I think libvirt could live with the blocked QMP
monitor during incoming migrate in the short/medium term.
So this doesn't look like a blocker to me currently.

> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  migration/file.c      | 15 +++++++++++++++
>  migration/file.h      |  1 +
>  migration/migration.c |  2 ++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/migration/file.c b/migration/file.c
> index 02896a7cab99..93eb718aa0f4 100644
> --- a/migration/file.c
> +++ b/migration/file.c
> @@ -21,3 +21,18 @@ void file_start_outgoing_migration(MigrationState *s, const char *fname, Error *
>  }
>  
>  
> +void file_start_incoming_migration(const char *fname, Error **errp)
> +{
> +	QIOChannelFile *ioc;
> +
> +	ioc = qio_channel_file_new_path(fname, O_RDONLY, 0, errp);
> +	if (!ioc) {
> +		error_report("Error creating a channel");
> +		return;
> +	}
> +
> +	qio_channel_set_name(QIO_CHANNEL(ioc), "migration-file-incoming");
> +	migration_channel_process_incoming(QIO_CHANNEL(ioc));
> +	object_unref(OBJECT(ioc));
> +}
> +
> diff --git a/migration/file.h b/migration/file.h
> index d476eb1157f9..cdbd291322d4 100644
> --- a/migration/file.h
> +++ b/migration/file.h
> @@ -5,5 +5,6 @@ void file_start_outgoing_migration(MigrationState *s,
>                                     const char *filename,
>                                     Error **errp);
>  
> +void file_start_incoming_migration(const char *fname, Error **errp);
>  #endif
>  
> diff --git a/migration/migration.c b/migration/migration.c
> index b5373db38535..eafd887254dd 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -506,6 +506,8 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
>          exec_start_incoming_migration(p, errp);
>      } else if (strstart(uri, "fd:", &p)) {
>          fd_start_incoming_migration(p, errp);
> +    } else if (strstart(uri, "file:", &p)) {
> +        file_start_incoming_migration(p, errp);
>      } else {
>          error_setg(errp, "unknown migration protocol: %s", uri);
>      }
> -- 
> 2.34.1
> 

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 :|