| Message ID | 20240313212824.16974-3-farosas@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | migration mapped-ram fixes | expand |
On Wed, Mar 13, 2024 at 06:28:24PM -0300, Fabiano Rosas wrote: > When doing migration using the fd: URI, the incoming migration starts > before the user has passed the file descriptor to QEMU. This means > that the checks at migration_channels_and_transport_compatible() > happen too soon and we need to allow a migration channel of type > SOCKET_ADDRESS_TYPE_FD even though socket migration is not supported > with multifd. Hmm, bare with me if this is a stupid one.. why the incoming migration can start _before_ the user passed in the fd? IOW, why can't we rely on a single fd_is_socket() check for SOCKET_ADDRESS_TYPE_FD in transport_supports_multi_channels()? > > The commit decdc76772 ("migration/multifd: Add mapped-ram support to > fd: URI") was supposed to add a second check prior to starting > migration to make sure a socket fd is not passed instead of a file fd, > but failed to do so. > > Add the missing verification. > > Fixes: decdc76772 ("migration/multifd: Add mapped-ram support to fd: URI") > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > migration/fd.c | 8 ++++++++ > migration/file.c | 7 +++++++ > 2 files changed, 15 insertions(+) > > diff --git a/migration/fd.c b/migration/fd.c > index 39a52e5c90..c07030f715 100644 > --- a/migration/fd.c > +++ b/migration/fd.c > @@ -22,6 +22,7 @@ > #include "migration.h" > #include "monitor/monitor.h" > #include "io/channel-file.h" > +#include "io/channel-socket.h" > #include "io/channel-util.h" > #include "options.h" > #include "trace.h" > @@ -95,6 +96,13 @@ void fd_start_incoming_migration(const char *fdname, Error **errp) > } > > if (migrate_multifd()) { > + if (fd_is_socket(fd)) { > + error_setg(errp, > + "Multifd migration to a socket FD is not supported"); > + object_unref(ioc); > + return; > + } > + > file_create_incoming_channels(ioc, errp); > } else { > qio_channel_set_name(ioc, "migration-fd-incoming"); > diff --git a/migration/file.c b/migration/file.c > index ddde0ca818..b6e8ba13f2 100644 > --- a/migration/file.c > +++ b/migration/file.c > @@ -15,6 +15,7 @@ > #include "file.h" > #include "migration.h" > #include "io/channel-file.h" > +#include "io/channel-socket.h" > #include "io/channel-util.h" > #include "options.h" > #include "trace.h" > @@ -58,6 +59,12 @@ bool file_send_channel_create(gpointer opaque, Error **errp) > int fd = fd_args_get_fd(); > > if (fd && fd != -1) { > + if (fd_is_socket(fd)) { > + error_setg(errp, > + "Multifd migration to a socket FD is not supported"); > + goto out; > + } > + > ioc = qio_channel_file_new_dupfd(fd, errp); > } else { > ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp); > -- > 2.35.3 >
On Thu, Mar 14, 2024 at 11:10:12AM -0400, Peter Xu wrote: > On Wed, Mar 13, 2024 at 06:28:24PM -0300, Fabiano Rosas wrote: > > When doing migration using the fd: URI, the incoming migration starts > > before the user has passed the file descriptor to QEMU. This means > > that the checks at migration_channels_and_transport_compatible() > > happen too soon and we need to allow a migration channel of type > > SOCKET_ADDRESS_TYPE_FD even though socket migration is not supported > > with multifd. > > Hmm, bare with me if this is a stupid one.. why the incoming migration can > start _before_ the user passed in the fd? > > IOW, why can't we rely on a single fd_is_socket() check for > SOCKET_ADDRESS_TYPE_FD in transport_supports_multi_channels()? > > > > > The commit decdc76772 ("migration/multifd: Add mapped-ram support to > > fd: URI") was supposed to add a second check prior to starting > > migration to make sure a socket fd is not passed instead of a file fd, > > but failed to do so. > > > > Add the missing verification. > > > > Fixes: decdc76772 ("migration/multifd: Add mapped-ram support to fd: URI") > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > > --- > > migration/fd.c | 8 ++++++++ > > migration/file.c | 7 +++++++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/migration/fd.c b/migration/fd.c > > index 39a52e5c90..c07030f715 100644 > > --- a/migration/fd.c > > +++ b/migration/fd.c > > @@ -22,6 +22,7 @@ > > #include "migration.h" > > #include "monitor/monitor.h" > > #include "io/channel-file.h" > > +#include "io/channel-socket.h" > > #include "io/channel-util.h" > > #include "options.h" > > #include "trace.h" > > @@ -95,6 +96,13 @@ void fd_start_incoming_migration(const char *fdname, Error **errp) > > } > > > > if (migrate_multifd()) { > > + if (fd_is_socket(fd)) { > > + error_setg(errp, > > + "Multifd migration to a socket FD is not supported"); > > + object_unref(ioc); > > + return; > > + } And... I just noticed this is forbiding multifd+socket+fd in general? But isn't that the majority of multifd usage when with libvirt over sockets? Shouldn't it about fd's seekable-or-not instead when mapped-ram enabled (IOW, migration_needs_seekable_channel() only)? > > + > > file_create_incoming_channels(ioc, errp); > > } else { > > qio_channel_set_name(ioc, "migration-fd-incoming"); > > diff --git a/migration/file.c b/migration/file.c > > index ddde0ca818..b6e8ba13f2 100644 > > --- a/migration/file.c > > +++ b/migration/file.c > > @@ -15,6 +15,7 @@ > > #include "file.h" > > #include "migration.h" > > #include "io/channel-file.h" > > +#include "io/channel-socket.h" > > #include "io/channel-util.h" > > #include "options.h" > > #include "trace.h" > > @@ -58,6 +59,12 @@ bool file_send_channel_create(gpointer opaque, Error **errp) > > int fd = fd_args_get_fd(); > > > > if (fd && fd != -1) { > > + if (fd_is_socket(fd)) { > > + error_setg(errp, > > + "Multifd migration to a socket FD is not supported"); > > + goto out; > > + } > > + > > ioc = qio_channel_file_new_dupfd(fd, errp); > > } else { > > ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp); > > -- > > 2.35.3 > > > > -- > Peter Xu
Peter Xu <peterx@redhat.com> writes: > On Wed, Mar 13, 2024 at 06:28:24PM -0300, Fabiano Rosas wrote: >> When doing migration using the fd: URI, the incoming migration starts >> before the user has passed the file descriptor to QEMU. This means >> that the checks at migration_channels_and_transport_compatible() >> happen too soon and we need to allow a migration channel of type >> SOCKET_ADDRESS_TYPE_FD even though socket migration is not supported >> with multifd. > > Hmm, bare with me if this is a stupid one.. why the incoming migration can > start _before_ the user passed in the fd? It's been a while since I looked at this. Looking into it once more today, I think the issue is actually that we only fetch the fds from the monitor at fd_start_outgoing|incoming_migration(). > > IOW, why can't we rely on a single fd_is_socket() check for > SOCKET_ADDRESS_TYPE_FD in transport_supports_multi_channels()? > There's no fd at that point. Just a string. I think the right fix here would be to move the monitor_fd_get/monitor_fd_param (why two different functions?) earlier into migrate_uri_parse. And possibly also extend FileMigrationArgs to contain an fd. Not sure how easy would that be. >> >> The commit decdc76772 ("migration/multifd: Add mapped-ram support to >> fd: URI") was supposed to add a second check prior to starting >> migration to make sure a socket fd is not passed instead of a file fd, >> but failed to do so. >> >> Add the missing verification. >> >> Fixes: decdc76772 ("migration/multifd: Add mapped-ram support to fd: URI") >> Signed-off-by: Fabiano Rosas <farosas@suse.de> >> --- >> migration/fd.c | 8 ++++++++ >> migration/file.c | 7 +++++++ >> 2 files changed, 15 insertions(+) >> >> diff --git a/migration/fd.c b/migration/fd.c >> index 39a52e5c90..c07030f715 100644 >> --- a/migration/fd.c >> +++ b/migration/fd.c >> @@ -22,6 +22,7 @@ >> #include "migration.h" >> #include "monitor/monitor.h" >> #include "io/channel-file.h" >> +#include "io/channel-socket.h" >> #include "io/channel-util.h" >> #include "options.h" >> #include "trace.h" >> @@ -95,6 +96,13 @@ void fd_start_incoming_migration(const char *fdname, Error **errp) >> } >> >> if (migrate_multifd()) { >> + if (fd_is_socket(fd)) { >> + error_setg(errp, >> + "Multifd migration to a socket FD is not supported"); >> + object_unref(ioc); >> + return; >> + } >> + >> file_create_incoming_channels(ioc, errp); >> } else { >> qio_channel_set_name(ioc, "migration-fd-incoming"); >> diff --git a/migration/file.c b/migration/file.c >> index ddde0ca818..b6e8ba13f2 100644 >> --- a/migration/file.c >> +++ b/migration/file.c >> @@ -15,6 +15,7 @@ >> #include "file.h" >> #include "migration.h" >> #include "io/channel-file.h" >> +#include "io/channel-socket.h" >> #include "io/channel-util.h" >> #include "options.h" >> #include "trace.h" >> @@ -58,6 +59,12 @@ bool file_send_channel_create(gpointer opaque, Error **errp) >> int fd = fd_args_get_fd(); >> >> if (fd && fd != -1) { >> + if (fd_is_socket(fd)) { >> + error_setg(errp, >> + "Multifd migration to a socket FD is not supported"); >> + goto out; >> + } >> + >> ioc = qio_channel_file_new_dupfd(fd, errp); >> } else { >> ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp); >> -- >> 2.35.3 >>
Peter Xu <peterx@redhat.com> writes: > On Thu, Mar 14, 2024 at 11:10:12AM -0400, Peter Xu wrote: >> On Wed, Mar 13, 2024 at 06:28:24PM -0300, Fabiano Rosas wrote: >> > When doing migration using the fd: URI, the incoming migration starts >> > before the user has passed the file descriptor to QEMU. This means >> > that the checks at migration_channels_and_transport_compatible() >> > happen too soon and we need to allow a migration channel of type >> > SOCKET_ADDRESS_TYPE_FD even though socket migration is not supported >> > with multifd. >> >> Hmm, bare with me if this is a stupid one.. why the incoming migration can >> start _before_ the user passed in the fd? >> >> IOW, why can't we rely on a single fd_is_socket() check for >> SOCKET_ADDRESS_TYPE_FD in transport_supports_multi_channels()? >> >> > >> > The commit decdc76772 ("migration/multifd: Add mapped-ram support to >> > fd: URI") was supposed to add a second check prior to starting >> > migration to make sure a socket fd is not passed instead of a file fd, >> > but failed to do so. >> > >> > Add the missing verification. >> > >> > Fixes: decdc76772 ("migration/multifd: Add mapped-ram support to fd: URI") >> > Signed-off-by: Fabiano Rosas <farosas@suse.de> >> > --- >> > migration/fd.c | 8 ++++++++ >> > migration/file.c | 7 +++++++ >> > 2 files changed, 15 insertions(+) >> > >> > diff --git a/migration/fd.c b/migration/fd.c >> > index 39a52e5c90..c07030f715 100644 >> > --- a/migration/fd.c >> > +++ b/migration/fd.c >> > @@ -22,6 +22,7 @@ >> > #include "migration.h" >> > #include "monitor/monitor.h" >> > #include "io/channel-file.h" >> > +#include "io/channel-socket.h" >> > #include "io/channel-util.h" >> > #include "options.h" >> > #include "trace.h" >> > @@ -95,6 +96,13 @@ void fd_start_incoming_migration(const char *fdname, Error **errp) >> > } >> > >> > if (migrate_multifd()) { >> > + if (fd_is_socket(fd)) { >> > + error_setg(errp, >> > + "Multifd migration to a socket FD is not supported"); >> > + object_unref(ioc); >> > + return; >> > + } > > And... I just noticed this is forbiding multifd+socket+fd in general? But > isn't that the majority of multifd usage when with libvirt over sockets? I didn't think multifd supported socket fds, does it? I don't see code to create the multiple channels anywhere. How would that work? Multiple threads writing to a single socket fd? I'm a bit confused. > > Shouldn't it about fd's seekable-or-not instead when mapped-ram enabled > (IOW, migration_needs_seekable_channel() only)? Yes, that could be a validation to be done if we actually get the fd at the right moment. > >> > + >> > file_create_incoming_channels(ioc, errp); >> > } else { >> > qio_channel_set_name(ioc, "migration-fd-incoming"); >> > diff --git a/migration/file.c b/migration/file.c >> > index ddde0ca818..b6e8ba13f2 100644 >> > --- a/migration/file.c >> > +++ b/migration/file.c >> > @@ -15,6 +15,7 @@ >> > #include "file.h" >> > #include "migration.h" >> > #include "io/channel-file.h" >> > +#include "io/channel-socket.h" >> > #include "io/channel-util.h" >> > #include "options.h" >> > #include "trace.h" >> > @@ -58,6 +59,12 @@ bool file_send_channel_create(gpointer opaque, Error **errp) >> > int fd = fd_args_get_fd(); >> > >> > if (fd && fd != -1) { >> > + if (fd_is_socket(fd)) { >> > + error_setg(errp, >> > + "Multifd migration to a socket FD is not supported"); >> > + goto out; >> > + } >> > + >> > ioc = qio_channel_file_new_dupfd(fd, errp); >> > } else { >> > ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp); >> > -- >> > 2.35.3 >> > >> >> -- >> Peter Xu
On Thu, Mar 14, 2024 at 01:44:13PM -0300, Fabiano Rosas wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Wed, Mar 13, 2024 at 06:28:24PM -0300, Fabiano Rosas wrote: > >> When doing migration using the fd: URI, the incoming migration starts > >> before the user has passed the file descriptor to QEMU. This means > >> that the checks at migration_channels_and_transport_compatible() > >> happen too soon and we need to allow a migration channel of type > >> SOCKET_ADDRESS_TYPE_FD even though socket migration is not supported > >> with multifd. > > > > Hmm, bare with me if this is a stupid one.. why the incoming migration can > > start _before_ the user passed in the fd? > > It's been a while since I looked at this. Looking into it once more > today, I think the issue is actually that we only fetch the fds from the > monitor at fd_start_outgoing|incoming_migration(). Yes that looks more reasonable. It means we may want to touch up transport_supports_seeking()'s comment on this if needed. > > > > > IOW, why can't we rely on a single fd_is_socket() check for > > SOCKET_ADDRESS_TYPE_FD in transport_supports_multi_channels()? > > > > There's no fd at that point. Just a string. > > I think the right fix here would be to move the > monitor_fd_get/monitor_fd_param (why two different functions?) Yes this is all confusing. I think it makes sense to use the same guideline for both sides on the fd: protocol, perhaps it means we should always use monitor_fd_param() to be consistent and compatible. > earlier into migrate_uri_parse. And possibly also extend > FileMigrationArgs to contain an fd. Not sure how easy would that be. But if so IIUC the 'filename' parameter will need to be optional if "fd" exists. While that will break QAPI for 8.2. I think it's fine we keep what we do right now (with the above comment fixed on why that check needs to be delayed, though), or if you find it easy to unify the check in some undestructive way.
On Thu, Mar 14, 2024 at 01:50:07PM -0300, Fabiano Rosas wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Thu, Mar 14, 2024 at 11:10:12AM -0400, Peter Xu wrote: > >> On Wed, Mar 13, 2024 at 06:28:24PM -0300, Fabiano Rosas wrote: > >> > When doing migration using the fd: URI, the incoming migration starts > >> > before the user has passed the file descriptor to QEMU. This means > >> > that the checks at migration_channels_and_transport_compatible() > >> > happen too soon and we need to allow a migration channel of type > >> > SOCKET_ADDRESS_TYPE_FD even though socket migration is not supported > >> > with multifd. > >> > >> Hmm, bare with me if this is a stupid one.. why the incoming migration can > >> start _before_ the user passed in the fd? > >> > >> IOW, why can't we rely on a single fd_is_socket() check for > >> SOCKET_ADDRESS_TYPE_FD in transport_supports_multi_channels()? > >> > >> > > >> > The commit decdc76772 ("migration/multifd: Add mapped-ram support to > >> > fd: URI") was supposed to add a second check prior to starting > >> > migration to make sure a socket fd is not passed instead of a file fd, > >> > but failed to do so. > >> > > >> > Add the missing verification. > >> > > >> > Fixes: decdc76772 ("migration/multifd: Add mapped-ram support to fd: URI") > >> > Signed-off-by: Fabiano Rosas <farosas@suse.de> > >> > --- > >> > migration/fd.c | 8 ++++++++ > >> > migration/file.c | 7 +++++++ > >> > 2 files changed, 15 insertions(+) > >> > > >> > diff --git a/migration/fd.c b/migration/fd.c > >> > index 39a52e5c90..c07030f715 100644 > >> > --- a/migration/fd.c > >> > +++ b/migration/fd.c > >> > @@ -22,6 +22,7 @@ > >> > #include "migration.h" > >> > #include "monitor/monitor.h" > >> > #include "io/channel-file.h" > >> > +#include "io/channel-socket.h" > >> > #include "io/channel-util.h" > >> > #include "options.h" > >> > #include "trace.h" > >> > @@ -95,6 +96,13 @@ void fd_start_incoming_migration(const char *fdname, Error **errp) > >> > } > >> > > >> > if (migrate_multifd()) { > >> > + if (fd_is_socket(fd)) { > >> > + error_setg(errp, > >> > + "Multifd migration to a socket FD is not supported"); > >> > + object_unref(ioc); > >> > + return; > >> > + } > > > > And... I just noticed this is forbiding multifd+socket+fd in general? But > > isn't that the majority of multifd usage when with libvirt over sockets? > > I didn't think multifd supported socket fds, does it? I don't see code > to create the multiple channels anywhere. How would that work? Multiple > threads writing to a single socket fd? I'm a bit confused. You're probably right. I somehow had the assumption that Libvirt always used fds to passover to QEMU for migration, but indeed multifd at least shouldn't support it as I read the code again.. It'll be good if Dan would help to clarify when fd will be used in migrations.
diff --git a/migration/fd.c b/migration/fd.c index 39a52e5c90..c07030f715 100644 --- a/migration/fd.c +++ b/migration/fd.c @@ -22,6 +22,7 @@ #include "migration.h" #include "monitor/monitor.h" #include "io/channel-file.h" +#include "io/channel-socket.h" #include "io/channel-util.h" #include "options.h" #include "trace.h" @@ -95,6 +96,13 @@ void fd_start_incoming_migration(const char *fdname, Error **errp) } if (migrate_multifd()) { + if (fd_is_socket(fd)) { + error_setg(errp, + "Multifd migration to a socket FD is not supported"); + object_unref(ioc); + return; + } + file_create_incoming_channels(ioc, errp); } else { qio_channel_set_name(ioc, "migration-fd-incoming"); diff --git a/migration/file.c b/migration/file.c index ddde0ca818..b6e8ba13f2 100644 --- a/migration/file.c +++ b/migration/file.c @@ -15,6 +15,7 @@ #include "file.h" #include "migration.h" #include "io/channel-file.h" +#include "io/channel-socket.h" #include "io/channel-util.h" #include "options.h" #include "trace.h" @@ -58,6 +59,12 @@ bool file_send_channel_create(gpointer opaque, Error **errp) int fd = fd_args_get_fd(); if (fd && fd != -1) { + if (fd_is_socket(fd)) { + error_setg(errp, + "Multifd migration to a socket FD is not supported"); + goto out; + } + ioc = qio_channel_file_new_dupfd(fd, errp); } else { ioc = qio_channel_file_new_path(outgoing_args.fname, flags, 0, errp);
When doing migration using the fd: URI, the incoming migration starts before the user has passed the file descriptor to QEMU. This means that the checks at migration_channels_and_transport_compatible() happen too soon and we need to allow a migration channel of type SOCKET_ADDRESS_TYPE_FD even though socket migration is not supported with multifd. The commit decdc76772 ("migration/multifd: Add mapped-ram support to fd: URI") was supposed to add a second check prior to starting migration to make sure a socket fd is not passed instead of a file fd, but failed to do so. Add the missing verification. Fixes: decdc76772 ("migration/multifd: Add mapped-ram support to fd: URI") Signed-off-by: Fabiano Rosas <farosas@suse.de> --- migration/fd.c | 8 ++++++++ migration/file.c | 7 +++++++ 2 files changed, 15 insertions(+)