Re: [PATCH v4 21/34] migration/multifd: Add incoming QIOChannelFile support
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
> > ---
> > 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(_recv_state->count);
> > }
>
Re: [PATCH v4 21/34] migration/multifd: Add incoming QIOChannelFile support
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
> ---
> 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?
> +
> +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(_recv_state->count);
> }
>
> p = _recv_state->params[id];
> --
> 2.35.3
>
--
Peter Xu
[PATCH v4 21/34] migration/multifd: Add incoming QIOChannelFile support
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
---
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)));
+
+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(_recv_state->count);
}
p = _recv_state->params[id];
--
2.35.3
