Re: [PATCH v4 21/34] migration/multifd: Add incoming QIOChannelFile support

[Peter Xu <peterx@redhat.com>]

On Mon, Feb 26, 2024 at 03:34:26PM +0800, Peter Xu wrote:
> On Tue, Feb 20, 2024 at 07:41:25PM -0300, Fabiano Rosas wrote:
> > On the receiving side we don't need to differentiate between main
> > channel and threads, so whichever channel is defined first gets to be
> > the main one. And since there are no packets, use the atomic channel
> > count to index into the params array.
> > 
> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> > ---
> >  migration/file.c      | 34 ++++++++++++++++++++++++++--------
> >  migration/migration.c |  3 ++-
> >  migration/multifd.c   |  3 +--
> >  3 files changed, 29 insertions(+), 11 deletions(-)
> > 
> > diff --git a/migration/file.c b/migration/file.c
> > index ac9f6ae40a..a186dc592a 100644
> > --- a/migration/file.c
> > +++ b/migration/file.c
> > @@ -8,6 +8,7 @@
> >  #include "qemu/osdep.h"
> >  #include "exec/ramblock.h"
> >  #include "qemu/cutils.h"
> > +#include "qemu/error-report.h"
> >  #include "qapi/error.h"
> >  #include "channel.h"
> >  #include "file.h"
> > @@ -15,6 +16,7 @@
> >  #include "multifd.h"
> >  #include "io/channel-file.h"
> >  #include "io/channel-util.h"
> > +#include "options.h"
> >  #include "trace.h"
> >  
> >  #define OFFSET_OPTION ",offset="
> > @@ -111,7 +113,8 @@ void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp)
> >      g_autofree char *filename = g_strdup(file_args->filename);
> >      QIOChannelFile *fioc = NULL;
> >      uint64_t offset = file_args->offset;
> > -    QIOChannel *ioc;
> > +    int channels = 1;
> > +    int i = 0, fd;
> >  
> >      trace_migration_file_incoming(filename);
> >  
> > @@ -120,13 +123,28 @@ void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp)
> >          return;
> >      }
> >  
> > -    ioc = QIO_CHANNEL(fioc);
> > -    if (offset && qio_channel_io_seek(ioc, offset, SEEK_SET, errp) < 0) {
> > +    if (offset &&
> > +        qio_channel_io_seek(QIO_CHANNEL(fioc), offset, SEEK_SET, errp) < 0) {
> >          return;
> >      }
> > -    qio_channel_set_name(QIO_CHANNEL(ioc), "migration-file-incoming");
> > -    qio_channel_add_watch_full(ioc, G_IO_IN,
> > -                               file_accept_incoming_migration,
> > -                               NULL, NULL,
> > -                               g_main_context_get_thread_default());
> > +
> > +    if (migrate_multifd()) {
> > +        channels += migrate_multifd_channels();
> > +    }
> > +
> > +    fd = fioc->fd;
> > +
> > +    do {
> > +        QIOChannel *ioc = QIO_CHANNEL(fioc);
> > +
> > +        qio_channel_set_name(ioc, "migration-file-incoming");
> > +        qio_channel_add_watch_full(ioc, G_IO_IN,
> > +                                   file_accept_incoming_migration,
> > +                                   NULL, NULL,
> > +                                   g_main_context_get_thread_default());
> > +    } while (++i < channels && (fioc = qio_channel_file_new_fd(fd)));
> 
> Note that reusing fd here has similar risk in the future that one iochannel
> can affect the other, as potentially all shares the same fd underneath; I
> think it's the same as "two qemufile v.s. one iochannel" issue that we're
> fighting recently.
> 
> IIUC the clean case is still that we open one fd for each iochannel.  Or
> e.g. as long as one iochannel close() its fd, it immediately invalidates
> all the rest iochannels on something like use-after-free of that fd index;
> any fd operates races with another fd being opened concurrently.
> 
> Maybe we can already use a loop of qio_channel_file_new_path()?  OS should
> already cached the dentry etc. so I assume the following ones should be
> super fast?  Or there's other complexities that I didn't aware?

Or simply use dup()?

> 
> > +
> > +    if (!fioc) {
> > +        error_setg(errp, "Error creating migration incoming channel");
> > +    }
> >  }
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 16da269847..e2218b9de7 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -896,7 +896,8 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
> >      uint32_t channel_magic = 0;
> >      int ret = 0;
> >  
> > -    if (migrate_multifd() && !migrate_postcopy_ram() &&
> > +    if (migrate_multifd() && !migrate_fixed_ram() &&
> > +        !migrate_postcopy_ram() &&
> >          qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
> >          /*
> >           * With multiple channels, it is possible that we receive channels
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index 507b497d52..cb5f4fb3e0 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -1520,8 +1520,7 @@ void multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
> >          }
> >          trace_multifd_recv_new_channel(id);
> >      } else {
> > -        /* next patch gives this a meaningful value */
> > -        id = 0;
> > +        id = qatomic_read(&multifd_recv_state->count);
> >      }
> >  
> >      p = &multifd_recv_state->params[id];
> > -- 
> > 2.35.3
> > 
> 
> -- 
> Peter Xu

-- 
Peter Xu