Re: [RFC PATCH v3 13/30] migration/multifd: Add outgoing QIOChannelFile support

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Jan 16, 2024 at 10:37:48AM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Mon, Nov 27, 2023 at 05:25:55PM -0300, Fabiano Rosas wrote:
>> >> Allow multifd to open file-backed channels. This will be used when
>> >> enabling the fixed-ram migration stream format which expects a
>> >> seekable transport.
>> >> 
>> >> The QIOChannel read and write methods will use the preadv/pwritev
>> >> versions which don't update the file offset at each call so we can
>> >> reuse the fd without re-opening for every channel.
>> >> 
>> >> Note that this is just setup code and multifd cannot yet make use of
>> >> the file channels.
>> >> 
>> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> >> ---
>> >> - open multifd channels with O_WRONLY and no mode
>> >> - stop cancelling migration and propagate error via qio_task
>> >> ---
>> >>  migration/file.c      | 47 +++++++++++++++++++++++++++++++++++++++++--
>> >>  migration/file.h      |  5 +++++
>> >>  migration/multifd.c   | 14 +++++++++++--
>> >>  migration/options.c   |  7 +++++++
>> >>  migration/options.h   |  1 +
>> >>  migration/qemu-file.h |  1 -
>> >>  6 files changed, 70 insertions(+), 5 deletions(-)
>> >> 
>> >> diff --git a/migration/file.c b/migration/file.c
>> >> index 5d4975f43e..67d6f42da7 100644
>> >> --- a/migration/file.c
>> >> +++ b/migration/file.c
>> >> @@ -17,6 +17,10 @@
>> >>  
>> >>  #define OFFSET_OPTION ",offset="
>> >>  
>> >> +static struct FileOutgoingArgs {
>> >> +    char *fname;
>> >> +} outgoing_args;
>> >> +
>> >>  /* Remove the offset option from @filespec and return it in @offsetp. */
>> >>  
>> >>  int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp)
>> >> @@ -36,6 +40,42 @@ int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp)
>> >>      return 0;
>> >>  }
>> >>  
>> >> +static void qio_channel_file_connect_worker(QIOTask *task, gpointer opaque)
>> >> +{
>> >> +    /* noop */
>> >> +}
>> >> +
>> >> +int file_send_channel_destroy(QIOChannel *ioc)
>> >> +{
>> >> +    if (ioc) {
>> >> +        qio_channel_close(ioc, NULL);
>> >> +        object_unref(OBJECT(ioc));
>> >> +    }
>> >> +    g_free(outgoing_args.fname);
>> >> +    outgoing_args.fname = NULL;
>> >> +
>> >> +    return 0;
>> >> +}
>> >> +
>> >> +void file_send_channel_create(QIOTaskFunc f, void *data)
>> >> +{
>> >> +    QIOChannelFile *ioc;
>> >> +    QIOTask *task;
>> >> +    Error *err = NULL;
>> >> +    int flags = O_WRONLY;
>> >> +
>> >> +    ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, &err);
>> >> +
>> >> +    task = qio_task_new(OBJECT(ioc), f, (gpointer)data, NULL);
>> >> +    if (!ioc) {
>> >> +        qio_task_set_error(task, err);
>> >> +        return;
>> >> +    }
>> >> +
>> >> +    qio_task_run_in_thread(task, qio_channel_file_connect_worker,
>> >> +                           (gpointer)data, NULL, NULL);
>> >
>> > This is pretty weird.  This invokes a thread, but it'll run a noop.  It
>> > seems meaningless to me.
>> >
>> 
>> That's QIOTask weirdness isn't it? It will run the worker in the thread,
>> but it also schedules the completion function as a glib event. So that's
>> when multifd_new_send_channel_async() will run. The crucial aspect here
>> is that it gets dispatched by glib on the main loop. I'm just keeping
>> the model, except that I don't have work to do during the "connection"
>> phase.
>
> The question is why do we need that if "file:" can be done synchronously.

I guess I tend to avoid changing existing patterns when adding a new
feature. But you're right, we don't really need this.

> Please see below.
>
>> 
>> > I assume you wanted to keep using the same async model as the socket typed
>> > multifd, but I don't think that works anyway, because file open blocks at
>> > qio_channel_file_new_path() so it's sync anyway.
>> 
>> It's async regarding multifd_channel_connect(). The connections will be
>> happening while multifd_save_setup() continues execution, IIUC.
>
> Yes.  But I'm wondering whether we can start to simplify at least the
> "file:" for this process.  We all know that we _may_ have created too many
> threads each doing very light work, which might not be needed.  We haven't
> yet resolved the "how to kill a thread along this process if migration
> cancels during when one thread got blocked in a syscall" issue.  We'll need
> to start recording tids for every thread, and that'll be a mess for sure
> when there're tons of threads.
>
>> 
>> >
>> > AFAICT we still share the code, as long as the file path properly invokes
>> > multifd_channel_connect() after the iochannel is setup.
>> >
>> 
>> I don't see the point in moving any of that logic into the URI
>> implementation. We already have the TLS handshake code which can also
>> call multifd_channel_connect() and that is a mess. IMO we should be
>> keeping the interface between multifd and the frontends as boilerplate
>> as possible.
>
> Hmm, I don't think it's a mess?  At least multifd_channel_connect(). AFAICT
> multifd_channel_connect() can be called in any context.

Well this sequence:

multifd_new_send_channel_async() -> multifd_channel_connect() ->
multifd_tls_channel_connect() -> new thread ->
multifd_tls_handshake_thread() -> new task ->
multifd_tls_outgoing_handshake() -> multifd_channel_connect()

...is not what I would call intuitive. Specifically with
multifd_channel_connect() being called more than the number of multifd
channels.

This would be "not a mess" IMO:

for (i = 0; i < migrate_multifd_channels(); i++) {
    multifd_tls_channel_connect();
    multifd_channel_connect() -> 
        qemu_thread_create(..., multifd_send_thread);
}

> multifd_channel_connect() always creates yet another thread, no matter it's
> for tls handshake, or it's one of the multifd send thread.
>
> Here this series already treat file/socket differently:
>
> static void multifd_new_send_channel_create(gpointer opaque)
> {
>     if (migrate_to_file()) {
>         file_send_channel_create(multifd_new_send_channel_async, opaque);
>     } else {
>         socket_send_channel_create(multifd_new_send_channel_async, opaque);
>     }
> }
>
> What I am thinking is it could be much simpler if
> multifd_new_send_channel_create() can create the multifd channels
> synchronously here, then directly call multifd_channel_connect(), further
> that'll create threads for whatever purposes.
>
> When TLS is not enabled, I'd expect if with that change and with a "file:"
> URI, after multifd_save_setup() completes, all send threads will be created
> already.
>
> I think multifd_new_send_channel_create() can already take
> "MultiFDSendParams *p" as parameter, then:
>
> static void multifd_new_send_channel_create(MultiFDSendParams *p)
> {
>     if (migrate_to_file()) {
>         file_send_channel_create(p);
>     } else {
>         socket_send_channel_create(multifd_new_send_channel_async, p);
>     }
> }
>
> Where file_send_channel_create() can call multifd_channel_connect()
> directly upon the ioc created.
>
> Would that work for us, and much cleaner?

Looks cleaner indeed, let me give it a try.