| Message ID | 20240315032040.7974-4-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration mapped-ram fixes | expand |
On Fri, Mar 15, 2024 at 12:20:40AM -0300, Fabiano Rosas wrote: > The fd: URI has supported migration to a file or socket since before > QEMU 8.2. In 8.2 we added the file: URI that supported migration to a > file. So now we have two ways (three if you count exec:>cat) to > migrate to a file. Fine. > > However, > > In 8.2 we also added the new qmp_migrate API that uses a JSON channel > list instead of the URI. It added two migration transports SOCKET and > FILE. It was decided that the new API would classify the fd migration > as a type of socket migration, neglecting the fact that the fd.c code > also supported file migrations. > > In 9.0 we're adding support for fd + multifd + mapped-ram, which is > tied to the file migration. This was implemented in fd.c, which is > only reachable when the SOCKET address type is used. > > The result of this is that we're asking users of the new API to create (1) > something called a "socket" to perform migration to a plain file. And > creating something called a "file" provides no way of passing in a > file descriptor. This is confusing. The 'file:' protocol eventually calls into qemu_open, and this transparently allows for FD passing using /dev/fdset/NNN syntax to pass in FDs. > diff --git a/qapi/migration.json b/qapi/migration.json > index aa1b39bce1..37f4b9c6fb 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -1656,13 +1656,20 @@ > # > # @filename: The file to receive the migration stream > # > +# @fd: A file descriptor name or number. File descriptors must be > +# first added with the 'getfd' command. (since 9.0). > +# > # @offset: The file offset where the migration stream will start > # > +# Since 9.0, all members are optional, but at least one of @filename > +# or @fd are required. > +# > # Since: 8.2 > ## > { 'struct': 'FileMigrationArgs', > - 'data': { 'filename': 'str', > - 'offset': 'uint64' } } > + 'data': { '*filename': 'str', > + '*fd': 'str', > + '*offset': 'uint64' } } Adding 'fd' here is not desirable, because 'filename' is resolved via qemu_open which allows for FD passing without introducing any new syntax in interfaces which take filenames. With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Fri, Mar 15, 2024 at 12:20:40AM -0300, Fabiano Rosas wrote: >> The fd: URI has supported migration to a file or socket since before >> QEMU 8.2. In 8.2 we added the file: URI that supported migration to a >> file. So now we have two ways (three if you count exec:>cat) to >> migrate to a file. Fine. >> >> However, >> >> In 8.2 we also added the new qmp_migrate API that uses a JSON channel >> list instead of the URI. It added two migration transports SOCKET and >> FILE. It was decided that the new API would classify the fd migration >> as a type of socket migration, neglecting the fact that the fd.c code >> also supported file migrations. >> >> In 9.0 we're adding support for fd + multifd + mapped-ram, which is >> tied to the file migration. This was implemented in fd.c, which is >> only reachable when the SOCKET address type is used. >> >> The result of this is that we're asking users of the new API to create (1) >> something called a "socket" to perform migration to a plain file. And >> creating something called a "file" provides no way of passing in a >> file descriptor. This is confusing. > > The 'file:' protocol eventually calls into qemu_open, and this > transparently allows for FD passing using /dev/fdset/NNN syntax > to pass in FDs. > Ok, that's technically correct. But it works differently from fd:fdname. /dev/fdset requires the user to switch from getfd to add-fd and it requires QEMU and libvirt/user to synchronize on the open() flags being used. QEMU cannot just receive any fd, it must be an fd that matches the flags QEMU passed into qio_channel_file_new_path(). Users will have to adapt to the new API anyway, so it might be ok to require these changes around it as well. To keep compatibility, we'd continue to accept the fd that comes from "fd:" or SOCKET_ADDRESS_TYPE_FD. But we'd be making it harder for users of the fd: syntax to move to the new API. I also don't know how we would deal with fdset when (if) we add support for passing in more channels in the new API. It supports multiple fds, but how do we deal with something like: "[ { 'channel-type': 'main'," " 'addr': { 'transport': 'file'," " 'filename': '/dev/fdset/1' } }, " { 'channel-type': 'second'," " 'addr': { 'transport': 'file'," " 'filename': '/dev/fdset/2' } } ]", Maybe that's too far ahead for this discussion. >> diff --git a/qapi/migration.json b/qapi/migration.json >> index aa1b39bce1..37f4b9c6fb 100644 >> --- a/qapi/migration.json >> +++ b/qapi/migration.json >> @@ -1656,13 +1656,20 @@ >> # >> # @filename: The file to receive the migration stream >> # >> +# @fd: A file descriptor name or number. File descriptors must be >> +# first added with the 'getfd' command. (since 9.0). >> +# >> # @offset: The file offset where the migration stream will start >> # >> +# Since 9.0, all members are optional, but at least one of @filename >> +# or @fd are required. >> +# >> # Since: 8.2 >> ## >> { 'struct': 'FileMigrationArgs', >> - 'data': { 'filename': 'str', >> - 'offset': 'uint64' } } >> + 'data': { '*filename': 'str', >> + '*fd': 'str', >> + '*offset': 'uint64' } } > > Adding 'fd' here is not desirable, because 'filename' is > resolved via qemu_open which allows for FD passing without > introducing any new syntax in interfaces which take filenames. > > With regards, > Daniel
[I queued patch 1-2 into -stable, leaving this patch for further discussions] On Fri, Mar 15, 2024 at 08:55:42AM +0000, Daniel P. Berrangé wrote: > The 'file:' protocol eventually calls into qemu_open, and this > transparently allows for FD passing using /dev/fdset/NNN syntax > to pass in FDs. If it always use /dev/fdsets for files, does it mean that the newly added SOCKET_ADDRESS_TYPE_FD support on mapped-ram will never be used (so we can drop them)? What about the old getfd? Is it obsolete because it relies on monitor object? Or maybe it's still in use? It would be greatly helpful if there can be a summary of how libvirt uses fd for migration purpose. Thanks,
Peter Xu <peterx@redhat.com> writes: > [I queued patch 1-2 into -stable, leaving this patch for further > discussions] > > On Fri, Mar 15, 2024 at 08:55:42AM +0000, Daniel P. Berrangé wrote: >> The 'file:' protocol eventually calls into qemu_open, and this >> transparently allows for FD passing using /dev/fdset/NNN syntax >> to pass in FDs. > > If it always use /dev/fdsets for files, does it mean that the newly added > SOCKET_ADDRESS_TYPE_FD support on mapped-ram will never be used (so we can > drop them)? We already have SOCKET_ADDRESS_TYPE_FD + file since 8.2 when the MigrationAddress was added. So this: 'channels': [ { 'channel-type': 'main', 'addr': { 'transport': 'socket', 'type': 'fd', 'str': 'fdname' } } ] works without multifd and without mapped-ram if the fd is a file or socket. So yes, you're correct, but given we already have this^ it would be perhaps more confusing for users to allow it, but not allow the very same JSON when multifd=true, mapped-ram=true. That's the only reason I didn't propose reverting commit decdc76772 ("migration/multifd: Add mapped-ram support to fd: URI"). For mapped-ram in libvirt, we'll definitely not use SOCKET_ADDRESS_TYPE_FD (as in the JSON), because I don't think libvirt supports the new API. As for SOCKET_ADDRESS_TYPE_FD as in "fd:", we could use it when direct-io is disabled. With direct-io, the fdset will be required. > > What about the old getfd? Is it obsolete because it relies on monitor > object? Or maybe it's still in use? > > It would be greatly helpful if there can be a summary of how libvirt uses > fd for migration purpose. > > Thanks,
On Fri, Mar 15, 2024 at 03:01:09PM -0300, Fabiano Rosas wrote: > Peter Xu <peterx@redhat.com> writes: > > > [I queued patch 1-2 into -stable, leaving this patch for further > > discussions] > > > > On Fri, Mar 15, 2024 at 08:55:42AM +0000, Daniel P. Berrangé wrote: > >> The 'file:' protocol eventually calls into qemu_open, and this > >> transparently allows for FD passing using /dev/fdset/NNN syntax > >> to pass in FDs. > > > > If it always use /dev/fdsets for files, does it mean that the newly added > > SOCKET_ADDRESS_TYPE_FD support on mapped-ram will never be used (so we can > > drop them)? > > We already have SOCKET_ADDRESS_TYPE_FD + file since 8.2 when the > MigrationAddress was added. So this: > > 'channels': [ { 'channel-type': 'main', > 'addr': { 'transport': 'socket', > 'type': 'fd', > 'str': 'fdname' } } ] > > works without multifd and without mapped-ram if the fd is a file or > socket. > > So yes, you're correct, but given we already have this^ it would be > perhaps more confusing for users to allow it, but not allow the very > same JSON when multifd=true, mapped-ram=true. I don't think the fd: protocol (no matter the old "fd:", or the new JSON format) is trivial to use. If libvirt didn't use it I won't be surprised to see nobody using it. I want us to avoid working on things that nobody is using, or has a better replacement. So even if Libvirt supports both, I'm wondering whether /dev/fdset/ works for all the cases that libvirt needs. I am aware that the old getfd has the monitor limitation so that if the QMP disconnected and reconnect, the fd can be gone. However I'm not sure whether that's the only reason to have add-fd, and also not sure whether it means add-fd is always preferred, so that maybe we can consider obsolete getfd? > > That's the only reason I didn't propose reverting commit decdc76772 > ("migration/multifd: Add mapped-ram support to fd: URI"). > > For mapped-ram in libvirt, we'll definitely not use > SOCKET_ADDRESS_TYPE_FD (as in the JSON), because I don't think libvirt > supports the new API. > > As for SOCKET_ADDRESS_TYPE_FD as in "fd:", we could use it when > direct-io is disabled. With direct-io, the fdset will be required. > > > > > What about the old getfd? Is it obsolete because it relies on monitor > > object? Or maybe it's still in use? > > > > It would be greatly helpful if there can be a summary of how libvirt uses > > fd for migration purpose. > > > > Thanks, >
On Fri, Mar 15, 2024 at 04:54:27PM -0400, Peter Xu wrote: > On Fri, Mar 15, 2024 at 03:01:09PM -0300, Fabiano Rosas wrote: > > Peter Xu <peterx@redhat.com> writes: > > > > > [I queued patch 1-2 into -stable, leaving this patch for further > > > discussions] > > > > > > On Fri, Mar 15, 2024 at 08:55:42AM +0000, Daniel P. Berrangé wrote: > > >> The 'file:' protocol eventually calls into qemu_open, and this > > >> transparently allows for FD passing using /dev/fdset/NNN syntax > > >> to pass in FDs. > > > > > > If it always use /dev/fdsets for files, does it mean that the newly added > > > SOCKET_ADDRESS_TYPE_FD support on mapped-ram will never be used (so we can > > > drop them)? > > > > We already have SOCKET_ADDRESS_TYPE_FD + file since 8.2 when the > > MigrationAddress was added. So this: > > > > 'channels': [ { 'channel-type': 'main', > > 'addr': { 'transport': 'socket', > > 'type': 'fd', > > 'str': 'fdname' } } ] > > > > works without multifd and without mapped-ram if the fd is a file or > > socket. > > > > So yes, you're correct, but given we already have this^ it would be > > perhaps more confusing for users to allow it, but not allow the very > > same JSON when multifd=true, mapped-ram=true. > > I don't think the fd: protocol (no matter the old "fd:", or the new JSON > format) is trivial to use. If libvirt didn't use it I won't be surprised to > see nobody using it. I want us to avoid working on things that nobody is > using, or has a better replacement. > > So even if Libvirt supports both, I'm wondering whether /dev/fdset/ works > for all the cases that libvirt needs. I am aware that the old getfd has > the monitor limitation so that if the QMP disconnected and reconnect, the > fd can be gone. However I'm not sure whether that's the only reason to > have add-fd, and also not sure whether it means add-fd is always preferred, > so that maybe we can consider obsolete getfd? Historically libvirt primariily uses the 'fd:' protocol, with a socket FD. It never gives QEMU a plain file FD, since it has always added its "iohelper" as a MITM, in order to add O_DIRECT on top. The 'getfd' command is something that is needed when talking to QEMU for any API that involves a "SocketAddress" QAPI type, which is applicable for migration. With the introduction of 'MigrationAddress', the 'socket' protocol is backed by 'SocketAddress' and thus supports FD passing for sockets (or potentally pipes too), in combination with 'getfd'. With the 'file' protocol in 'MigrationAddress', since it gets backed by qemu_open(), then /dev/fdset/NN and 'add-fd' provide passing for plain files. With regards, Daniel
On Fri, Mar 15, 2024 at 09:13:52AM -0300, Fabiano Rosas wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Fri, Mar 15, 2024 at 12:20:40AM -0300, Fabiano Rosas wrote: > >> The fd: URI has supported migration to a file or socket since before > >> QEMU 8.2. In 8.2 we added the file: URI that supported migration to a > >> file. So now we have two ways (three if you count exec:>cat) to > >> migrate to a file. Fine. > >> > >> However, > >> > >> In 8.2 we also added the new qmp_migrate API that uses a JSON channel > >> list instead of the URI. It added two migration transports SOCKET and > >> FILE. It was decided that the new API would classify the fd migration > >> as a type of socket migration, neglecting the fact that the fd.c code > >> also supported file migrations. > >> > >> In 9.0 we're adding support for fd + multifd + mapped-ram, which is > >> tied to the file migration. This was implemented in fd.c, which is > >> only reachable when the SOCKET address type is used. > >> > >> The result of this is that we're asking users of the new API to create (1) > >> something called a "socket" to perform migration to a plain file. And > >> creating something called a "file" provides no way of passing in a > >> file descriptor. This is confusing. > > > > The 'file:' protocol eventually calls into qemu_open, and this > > transparently allows for FD passing using /dev/fdset/NNN syntax > > to pass in FDs. > > > > Ok, that's technically correct. But it works differently from > fd:fdname. > > /dev/fdset requires the user to switch from getfd to add-fd and it > requires QEMU and libvirt/user to synchronize on the open() flags being > used. QEMU cannot just receive any fd, it must be an fd that matches the > flags QEMU passed into qio_channel_file_new_path(). > > Users will have to adapt to the new API anyway, so it might be ok to > require these changes around it as well. Passing plain files to QEMU is a new feature, so apps have to adapt their code already. The expectations around flags are not especially hard - RDONLY|WRONLY|RDWR, optionally with O_DIRECT depending on what migrate feature is enabled. > To keep compatibility, we'd continue to accept the fd that comes from > "fd:" or SOCKET_ADDRESS_TYPE_FD. But we'd be making it harder for users > of the fd: syntax to move to the new API. > > I also don't know how we would deal with fdset when (if) we add support > for passing in more channels in the new API. It supports multiple fds, > but how do we deal with something like: > > "[ { 'channel-type': 'main'," > " 'addr': { 'transport': 'file'," > " 'filename': '/dev/fdset/1' } }, > " { 'channel-type': 'second'," > " 'addr': { 'transport': 'file'," > " 'filename': '/dev/fdset/2' } } ]", Having multiple fdsets should "just work" - qemu_open() handles everything behind the scenes, so the rest of QEMU shouldn't need to know anything about FD passing of plain files. Similarly for socket FD passing, if the QEMU code works with a SocketAddress struct in its public API, FD passing of of sockets should again "just work" without any special logic. The only reason why 'migrate' has this "fd:name" protocol, is because this pre-dates the introduction of "SocketAddress" in QAPI. With the switch to "MigrateAddress" exposing "SocketAddress" directly, 'migrate' has normalized itself with best practice. > > Maybe that's too far ahead for this discussion. > > >> diff --git a/qapi/migration.json b/qapi/migration.json > >> index aa1b39bce1..37f4b9c6fb 100644 > >> --- a/qapi/migration.json > >> +++ b/qapi/migration.json > >> @@ -1656,13 +1656,20 @@ > >> # > >> # @filename: The file to receive the migration stream > >> # > >> +# @fd: A file descriptor name or number. File descriptors must be > >> +# first added with the 'getfd' command. (since 9.0). > >> +# > >> # @offset: The file offset where the migration stream will start > >> # > >> +# Since 9.0, all members are optional, but at least one of @filename > >> +# or @fd are required. > >> +# > >> # Since: 8.2 > >> ## > >> { 'struct': 'FileMigrationArgs', > >> - 'data': { 'filename': 'str', > >> - 'offset': 'uint64' } } > >> + 'data': { '*filename': 'str', > >> + '*fd': 'str', > >> + '*offset': 'uint64' } } > > > > Adding 'fd' here is not desirable, because 'filename' is > > resolved via qemu_open which allows for FD passing without > > introducing any new syntax in interfaces which take filenames. > > > > With regards, > > Daniel > With regards, Daniel
On Tue, Mar 19, 2024 at 04:25:32PM +0000, Daniel P. Berrangé wrote: > On Fri, Mar 15, 2024 at 04:54:27PM -0400, Peter Xu wrote: > > On Fri, Mar 15, 2024 at 03:01:09PM -0300, Fabiano Rosas wrote: > > > Peter Xu <peterx@redhat.com> writes: > > > > > > > [I queued patch 1-2 into -stable, leaving this patch for further > > > > discussions] > > > > > > > > On Fri, Mar 15, 2024 at 08:55:42AM +0000, Daniel P. Berrangé wrote: > > > >> The 'file:' protocol eventually calls into qemu_open, and this > > > >> transparently allows for FD passing using /dev/fdset/NNN syntax > > > >> to pass in FDs. > > > > > > > > If it always use /dev/fdsets for files, does it mean that the newly added > > > > SOCKET_ADDRESS_TYPE_FD support on mapped-ram will never be used (so we can > > > > drop them)? > > > > > > We already have SOCKET_ADDRESS_TYPE_FD + file since 8.2 when the > > > MigrationAddress was added. So this: > > > > > > 'channels': [ { 'channel-type': 'main', > > > 'addr': { 'transport': 'socket', > > > 'type': 'fd', > > > 'str': 'fdname' } } ] > > > > > > works without multifd and without mapped-ram if the fd is a file or > > > socket. > > > > > > So yes, you're correct, but given we already have this^ it would be > > > perhaps more confusing for users to allow it, but not allow the very > > > same JSON when multifd=true, mapped-ram=true. > > > > I don't think the fd: protocol (no matter the old "fd:", or the new JSON > > format) is trivial to use. If libvirt didn't use it I won't be surprised to > > see nobody using it. I want us to avoid working on things that nobody is > > using, or has a better replacement. > > > > So even if Libvirt supports both, I'm wondering whether /dev/fdset/ works > > for all the cases that libvirt needs. I am aware that the old getfd has > > the monitor limitation so that if the QMP disconnected and reconnect, the > > fd can be gone. However I'm not sure whether that's the only reason to > > have add-fd, and also not sure whether it means add-fd is always preferred, > > so that maybe we can consider obsolete getfd? > > Historically libvirt primariily uses the 'fd:' protocol, with a > socket FD. It never gives QEMU a plain file FD, since it has > always added its "iohelper" as a MITM, in order to add O_DIRECT > on top. > > The 'getfd' command is something that is needed when talking to > QEMU for any API that involves a "SocketAddress" QAPI type, > which is applicable for migration. > > With the introduction of 'MigrationAddress', the 'socket' protocol > is backed by 'SocketAddress' and thus supports FD passing for > sockets (or potentally pipes too), in combination with 'getfd'. > > With the 'file' protocol in 'MigrationAddress', since it gets > backed by qemu_open(), then /dev/fdset/NN and 'add-fd' provide > passing for plain files. I see. I assume it means we still have multiple users of getfd so it's still in use where add-fd is not yet avaiable. But then, SOCKET_ADDRESS_TYPE_FD is then not used for libvirt in the whole mapped-ram effort, neither do we need any support on file migrations over "fd", e.g. fd_start_incoming_migration() for files. So we can drop these parts, am I right? Thanks,
On Tue, Mar 19, 2024 at 03:25:18PM -0400, Peter Xu wrote: > On Tue, Mar 19, 2024 at 04:25:32PM +0000, Daniel P. Berrangé wrote: > > On Fri, Mar 15, 2024 at 04:54:27PM -0400, Peter Xu wrote: > > > On Fri, Mar 15, 2024 at 03:01:09PM -0300, Fabiano Rosas wrote: > > > > Peter Xu <peterx@redhat.com> writes: > > > > > > > > > [I queued patch 1-2 into -stable, leaving this patch for further > > > > > discussions] > > > > > > > > > > On Fri, Mar 15, 2024 at 08:55:42AM +0000, Daniel P. Berrangé wrote: > > > > >> The 'file:' protocol eventually calls into qemu_open, and this > > > > >> transparently allows for FD passing using /dev/fdset/NNN syntax > > > > >> to pass in FDs. > > > > > > > > > > If it always use /dev/fdsets for files, does it mean that the newly added > > > > > SOCKET_ADDRESS_TYPE_FD support on mapped-ram will never be used (so we can > > > > > drop them)? > > > > > > > > We already have SOCKET_ADDRESS_TYPE_FD + file since 8.2 when the > > > > MigrationAddress was added. So this: > > > > > > > > 'channels': [ { 'channel-type': 'main', > > > > 'addr': { 'transport': 'socket', > > > > 'type': 'fd', > > > > 'str': 'fdname' } } ] > > > > > > > > works without multifd and without mapped-ram if the fd is a file or > > > > socket. > > > > > > > > So yes, you're correct, but given we already have this^ it would be > > > > perhaps more confusing for users to allow it, but not allow the very > > > > same JSON when multifd=true, mapped-ram=true. > > > > > > I don't think the fd: protocol (no matter the old "fd:", or the new JSON > > > format) is trivial to use. If libvirt didn't use it I won't be surprised to > > > see nobody using it. I want us to avoid working on things that nobody is > > > using, or has a better replacement. > > > > > > So even if Libvirt supports both, I'm wondering whether /dev/fdset/ works > > > for all the cases that libvirt needs. I am aware that the old getfd has > > > the monitor limitation so that if the QMP disconnected and reconnect, the > > > fd can be gone. However I'm not sure whether that's the only reason to > > > have add-fd, and also not sure whether it means add-fd is always preferred, > > > so that maybe we can consider obsolete getfd? > > > > Historically libvirt primariily uses the 'fd:' protocol, with a > > socket FD. It never gives QEMU a plain file FD, since it has > > always added its "iohelper" as a MITM, in order to add O_DIRECT > > on top. > > > > The 'getfd' command is something that is needed when talking to > > QEMU for any API that involves a "SocketAddress" QAPI type, > > which is applicable for migration. > > > > With the introduction of 'MigrationAddress', the 'socket' protocol > > is backed by 'SocketAddress' and thus supports FD passing for > > sockets (or potentally pipes too), in combination with 'getfd'. > > > > With the 'file' protocol in 'MigrationAddress', since it gets > > backed by qemu_open(), then /dev/fdset/NN and 'add-fd' provide > > passing for plain files. > > I see. I assume it means we still have multiple users of getfd so it's > still in use where add-fd is not yet avaiable. > > But then, SOCKET_ADDRESS_TYPE_FD is then not used for libvirt in the whole > mapped-ram effort, neither do we need any support on file migrations over > "fd", e.g. fd_start_incoming_migration() for files. So we can drop these > parts, am I right? Correct, libvirt hasn't got any impl for 'mapped-ram' yet, at least not something merged. Since this is new functionality, libvirt could go straight for the 'file' protocol / address type. At some point I think we can stop using 'fd' for traditional migration too and pass the socket address to QEMU and let QEMU open the socket. With regards, Daniel
On Tue, Mar 19, 2024 at 07:52:47PM +0000, Daniel P. Berrangé wrote: > On Tue, Mar 19, 2024 at 03:25:18PM -0400, Peter Xu wrote: > > On Tue, Mar 19, 2024 at 04:25:32PM +0000, Daniel P. Berrangé wrote: > > > On Fri, Mar 15, 2024 at 04:54:27PM -0400, Peter Xu wrote: > > > > On Fri, Mar 15, 2024 at 03:01:09PM -0300, Fabiano Rosas wrote: > > > > > Peter Xu <peterx@redhat.com> writes: > > > > > > > > > > > [I queued patch 1-2 into -stable, leaving this patch for further > > > > > > discussions] > > > > > > > > > > > > On Fri, Mar 15, 2024 at 08:55:42AM +0000, Daniel P. Berrangé wrote: > > > > > >> The 'file:' protocol eventually calls into qemu_open, and this > > > > > >> transparently allows for FD passing using /dev/fdset/NNN syntax > > > > > >> to pass in FDs. > > > > > > > > > > > > If it always use /dev/fdsets for files, does it mean that the newly added > > > > > > SOCKET_ADDRESS_TYPE_FD support on mapped-ram will never be used (so we can > > > > > > drop them)? > > > > > > > > > > We already have SOCKET_ADDRESS_TYPE_FD + file since 8.2 when the > > > > > MigrationAddress was added. So this: > > > > > > > > > > 'channels': [ { 'channel-type': 'main', > > > > > 'addr': { 'transport': 'socket', > > > > > 'type': 'fd', > > > > > 'str': 'fdname' } } ] > > > > > > > > > > works without multifd and without mapped-ram if the fd is a file or > > > > > socket. > > > > > > > > > > So yes, you're correct, but given we already have this^ it would be > > > > > perhaps more confusing for users to allow it, but not allow the very > > > > > same JSON when multifd=true, mapped-ram=true. > > > > > > > > I don't think the fd: protocol (no matter the old "fd:", or the new JSON > > > > format) is trivial to use. If libvirt didn't use it I won't be surprised to > > > > see nobody using it. I want us to avoid working on things that nobody is > > > > using, or has a better replacement. > > > > > > > > So even if Libvirt supports both, I'm wondering whether /dev/fdset/ works > > > > for all the cases that libvirt needs. I am aware that the old getfd has > > > > the monitor limitation so that if the QMP disconnected and reconnect, the > > > > fd can be gone. However I'm not sure whether that's the only reason to > > > > have add-fd, and also not sure whether it means add-fd is always preferred, > > > > so that maybe we can consider obsolete getfd? > > > > > > Historically libvirt primariily uses the 'fd:' protocol, with a > > > socket FD. It never gives QEMU a plain file FD, since it has > > > always added its "iohelper" as a MITM, in order to add O_DIRECT > > > on top. > > > > > > The 'getfd' command is something that is needed when talking to > > > QEMU for any API that involves a "SocketAddress" QAPI type, > > > which is applicable for migration. > > > > > > With the introduction of 'MigrationAddress', the 'socket' protocol > > > is backed by 'SocketAddress' and thus supports FD passing for > > > sockets (or potentally pipes too), in combination with 'getfd'. > > > > > > With the 'file' protocol in 'MigrationAddress', since it gets > > > backed by qemu_open(), then /dev/fdset/NN and 'add-fd' provide > > > passing for plain files. > > > > I see. I assume it means we still have multiple users of getfd so it's > > still in use where add-fd is not yet avaiable. > > > > But then, SOCKET_ADDRESS_TYPE_FD is then not used for libvirt in the whole > > mapped-ram effort, neither do we need any support on file migrations over > > "fd", e.g. fd_start_incoming_migration() for files. So we can drop these > > parts, am I right? > > Correct, libvirt hasn't got any impl for 'mapped-ram' yet, at least > not something merged. > > Since this is new functionality, libvirt could go straight for the > 'file' protocol / address type. > > At some point I think we can stop using 'fd' for traditional migration > too and pass the socket address to QEMU and let QEMU open the socket. Thanks for confirming this, that sounds good. I quickly discussed this with Fabiano just now, I think there's a plan we start to mark fd migration deprecated for the next release (9.1), then there is a chance we drop it in migration for 9.3. That'll copy libvirt list so we can re-check there. Fabiano will then prepare patches to remove the "fd:" support on file migrations; that will be for 9.0. Thanks,
diff --git a/migration/fd.c b/migration/fd.c index fe0d096abd..2d4414c7ea 100644 --- a/migration/fd.c +++ b/migration/fd.c @@ -20,9 +20,6 @@ #include "fd.h" #include "file.h" #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" @@ -48,8 +45,7 @@ void fd_cleanup_outgoing_migration(void) void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp) { QIOChannel *ioc; - int fd = monitor_get_fd(monitor_cur(), fdname, errp); - int newfd; + int newfd, fd = migrate_fd_str_to_int(fdname, errp); if (fd == -1) { return; @@ -91,7 +87,7 @@ static gboolean fd_accept_incoming_migration(QIOChannel *ioc, void fd_start_incoming_migration(const char *fdname, Error **errp) { QIOChannel *ioc; - int fd = monitor_fd_param(monitor_cur(), fdname, errp); + int fd = migrate_fd_str_to_int(fdname, errp); if (fd == -1) { return; } @@ -105,13 +101,6 @@ 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 b6e8ba13f2..b29521ffa7 100644 --- a/migration/file.c +++ b/migration/file.c @@ -59,12 +59,6 @@ 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); @@ -95,6 +89,10 @@ void file_start_outgoing_migration(MigrationState *s, uint64_t offset = file_args->offset; QIOChannel *ioc; + if (file_args->fd) { + return fd_start_outgoing_migration(s, file_args->fd, errp); + } + trace_migration_file_outgoing(filename); fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY | O_TRUNC, @@ -163,6 +161,10 @@ void file_start_incoming_migration(FileMigrationArgs *file_args, Error **errp) QIOChannelFile *fioc = NULL; uint64_t offset = file_args->offset; + if (file_args->fd) { + return fd_start_incoming_migration(file_args->fd, errp); + } + trace_migration_file_incoming(filename); fioc = qio_channel_file_new_path(filename, O_RDONLY, 0, errp); diff --git a/migration/migration.c b/migration/migration.c index f60bd371e3..88162205a3 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -140,10 +140,6 @@ static bool transport_supports_multi_channels(MigrationAddress *addr) if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) { SocketAddress *saddr = &addr->u.socket; - if (saddr->type == SOCKET_ADDRESS_TYPE_FD) { - return migrate_mapped_ram(); - } - return (saddr->type == SOCKET_ADDRESS_TYPE_INET || saddr->type == SOCKET_ADDRESS_TYPE_UNIX || saddr->type == SOCKET_ADDRESS_TYPE_VSOCK); @@ -165,15 +161,6 @@ static bool transport_supports_seeking(MigrationAddress *addr) return true; } - /* - * At this point QEMU has not yet fetched the fd passed in by the - * user, so we cannot know for sure whether it refers to a plain - * file or a socket. Let it through anyway and check at fd.c. - */ - if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) { - return addr->u.socket.type == SOCKET_ADDRESS_TYPE_FD; - } - return false; } @@ -608,6 +595,85 @@ bool migrate_uri_parse(const char *uri, MigrationChannel **channel, return true; } +/* + * Unlike other types of migration addresses, a file descriptor is not + * just a string that QEMU uses to initiate a connection. An fd has to + * be prepared in advance by the user or management app. We only know + * whether a file descriptor refers to a socket or a file after + * retrieving it from the monitor. + * + * If the method being used to reference the fds for migration is the + * old "fd:" URI, migrate_uri_parse() always parses it into a + * MIGRATION_ADDRESS_TYPE_SOCKET. + * + * If the method is the new channels API, a file descriptor might come + * via either the MIGRATION_ADDRESS_TYPE_SOCKET or the + * MIGRATION_ADDRESS_TYPE_FILE. For backward compatibility with 8.2 + * which allowed a file migration with MIGRATION_ADDRESS_TYPE_SOCKET, + * this is still allowed. + * + * This function converts all of the above into + * MIGRATION_ADDRESS_TYPE_FILE so the rest of code doesn't need to + * care about none of this. + */ +static bool migrate_resolve_fd(MigrationAddress *addr, Error **errp) +{ + int fd; + + if (addr->transport == MIGRATION_ADDRESS_TYPE_FILE) { + FileMigrationArgs *fargs = &addr->u.file; + + if (fargs->fd) { + fd = monitor_fd_param(monitor_cur(), fargs->fd, errp); + if (fd == -1) { + return false; + } + + if (!fd_is_socket(fd)) { + addr->u.file.fd = g_strdup_printf("%d", fd); + } else { + error_setg(errp, + "Migration transport 'file' does not support socket file descriptors"); + return false; + } + } + } else if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) { + SocketAddress *saddr = &addr->u.socket; + + if (saddr->type == SOCKET_ADDRESS_TYPE_FD) { + fd = monitor_fd_param(monitor_cur(), saddr->u.fd.str, errp); + if (fd == -1) { + return false; + } + + g_free(saddr->u.fd.str); + + if (!fd_is_socket(fd)) { + addr->transport = MIGRATION_ADDRESS_TYPE_FILE; + addr->u.file.fd = g_strdup_printf("%d", fd); + addr->u.file.filename = NULL; + addr->u.file.offset = 0; + } else { + saddr->u.fd.str = g_strdup_printf("%d", fd); + } + } + } + + return true; +} + +int migrate_fd_str_to_int(const char *fdname, Error **errp) +{ + int err, fd; + + err = qemu_strtoi(fdname, NULL, 0, &fd); + if (err) { + error_setg_errno(errp, err, "failed to convert fd"); + return -1; + } + return fd; +} + static void qemu_start_incoming_migration(const char *uri, bool has_channels, MigrationChannelList *channels, Error **errp) @@ -641,6 +707,10 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels, addr = channel->addr; } + if (!migrate_resolve_fd(addr, errp)) { + return; + } + /* transport mechanism not suitable for migration? */ if (!migration_channels_and_transport_compatible(addr, errp)) { return; @@ -2086,6 +2156,10 @@ void qmp_migrate(const char *uri, bool has_channels, addr = channel->addr; } + if (!migrate_resolve_fd(addr, errp)) { + return; + } + /* transport mechanism not suitable for migration? */ if (!migration_channels_and_transport_compatible(addr, errp)) { return; diff --git a/migration/migration.h b/migration/migration.h index 8045e39c26..e265d1732c 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -543,5 +543,6 @@ int migration_rp_wait(MigrationState *s); * to remember the target is always the migration thread. */ void migration_rp_kick(MigrationState *s); +int migrate_fd_str_to_int(const char *fdname, Error **errp); #endif diff --git a/qapi/migration.json b/qapi/migration.json index aa1b39bce1..37f4b9c6fb 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -1656,13 +1656,20 @@ # # @filename: The file to receive the migration stream # +# @fd: A file descriptor name or number. File descriptors must be +# first added with the 'getfd' command. (since 9.0). +# # @offset: The file offset where the migration stream will start # +# Since 9.0, all members are optional, but at least one of @filename +# or @fd are required. +# # Since: 8.2 ## { 'struct': 'FileMigrationArgs', - 'data': { 'filename': 'str', - 'offset': 'uint64' } } + 'data': { '*filename': 'str', + '*fd': 'str', + '*offset': 'uint64' } } ## # @MigrationExecCommand:
The fd: URI has supported migration to a file or socket since before QEMU 8.2. In 8.2 we added the file: URI that supported migration to a file. So now we have two ways (three if you count exec:>cat) to migrate to a file. Fine. However, In 8.2 we also added the new qmp_migrate API that uses a JSON channel list instead of the URI. It added two migration transports SOCKET and FILE. It was decided that the new API would classify the fd migration as a type of socket migration, neglecting the fact that the fd.c code also supported file migrations. In 9.0 we're adding support for fd + multifd + mapped-ram, which is tied to the file migration. This was implemented in fd.c, which is only reachable when the SOCKET address type is used. The result of this is that we're asking users of the new API to create (1) something called a "socket" to perform migration to a plain file. And creating something called a "file" provides no way of passing in a file descriptor. This is confusing. The new API also parses the old-style URI into the new-style data structures, so the old and correct fd:<filefd> now also considers fd: to be socket-related only. Unlike the other migration addresses, the fd comes already setup from the user, it's not just a string that QEMU will use to start a connection. We need to actually fetch the fd from the monitor before even being able to poke at it to know if it is a socket. Aside from the issue (1) above, the current approach of parsing the fd into a SOCKET and only later deciding if it is file-backed doesn't work well now that we're adding multifd support for fd: and file: migration via the mapped-ram feature. With a larger number of combinations, we need to be able to know upfront when an fd is backed by a plain file vs. a socket. We're currently using a trick of allowing socket channels to pass some (2) validation that they shouldn't, to only later verify if the fd was indeed a socket. To clean this up I'm proposing we start requiring users of the new API to use the "file" structure when the migration stream will end up in a file and use the "socket" structure when the fd points to an actual socket. This improves (1). To keep backward compatibility, I'm still accepting everything that was accepted before and only changing the structures internally to allow the rest of the code to rely on the MIGRATION_ADDRESS_TYPE. This addresses (2). We can then slowly deprecate the wrong way, i.e. using type socket for fd + file migration. In this patch you'll find: - a new function migrate_resolve_fd() that fetches the fd from the monitor and converts the MigrationAddress into the correct type; - the removal of the hacks for (2); - a helper function to convert the resolved fd that's represented as a string for storage in the MigrationAddress back into an integer for consumption. Signed-off-by: Fabiano Rosas <farosas@suse.de> --- migration/fd.c | 15 +------ migration/file.c | 14 +++--- migration/migration.c | 100 ++++++++++++++++++++++++++++++++++++------ migration/migration.h | 1 + qapi/migration.json | 11 ++++- 5 files changed, 107 insertions(+), 34 deletions(-)