Make local migration with TAP network device possible

[PATCH 1/2] chardev: don't set O_NONBLOCK on SCM_RIGHTS file descriptors.

Posted by Andrey Ryabinin 3 years, 8 months ago

This reverts commit 9b938c7262e4 ("chardev: clear O_NONBLOCK on SCM_RIGHTS file descriptors").
File descriptor passed to QEMU via 'getfd' QMP command always
changed to blocking mode. Instead of that, change blocking mode by QEMU
file descriptors users when necessary, e.g. like migration.

We need to preserve the state of the file descriptor in case it's still
used by an external process and before the QEMU itself started
using it.

E.g. our local migration scenario with TAP networking looks like this:
 1. Create TAP devices and pass file descriptors to source QEMU
 2. Launch destination QEMU (-incoming defer) and pass same descriptors to it.
 3. Start migration

In such scenario setting blocking state at stage (2) will hang source QEMU
since TAP fd suddenly become blocking.

Signed-off-by: Andrey Ryabinin <arbn@yandex-team.com>
---
 chardev/char-socket.c | 3 ---
 io/channel-socket.c   | 3 ---
 migration/fd.c        | 2 ++
 3 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index dc4e218eeb6..c9592fb5836 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -310,9 +310,6 @@ static ssize_t tcp_chr_recv(Chardev *chr, char *buf, size_t len)
             continue;
         }
 
-        /* O_NONBLOCK is preserved across SCM_RIGHTS so reset it */
-        qemu_socket_set_block(fd);
-
 #ifndef MSG_CMSG_CLOEXEC
         qemu_set_cloexec(fd);
 #endif
diff --git a/io/channel-socket.c b/io/channel-socket.c
index dc9c165de11..8b9679460dc 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -479,9 +479,6 @@ static void qio_channel_socket_copy_fds(struct msghdr *msg,
                 continue;
             }
 
-            /* O_NONBLOCK is preserved across SCM_RIGHTS so reset it */
-            qemu_socket_set_block(fd);
-
 #ifndef MSG_CMSG_CLOEXEC
             qemu_set_cloexec(fd);
 #endif
diff --git a/migration/fd.c b/migration/fd.c
index 6f2f50475f4..793fffeb169 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -60,6 +60,8 @@ void fd_start_incoming_migration(const char *fdname, Error **errp)
         return;
     }
 
+    qemu_socket_set_block(fd);
+
     trace_migration_fd_incoming(fd);
 
     ioc = qio_channel_new_fd(fd, errp);
-- 
2.35.1

Re: [PATCH 1/2] chardev: don't set O_NONBLOCK on SCM_RIGHTS file descriptors.

Posted by Stefan Hajnoczi 3 years, 7 months ago

On Tue, Jun 14, 2022 at 02:18:42PM +0300, Andrey Ryabinin wrote:
> This reverts commit 9b938c7262e4 ("chardev: clear O_NONBLOCK on SCM_RIGHTS file descriptors").
> File descriptor passed to QEMU via 'getfd' QMP command always
> changed to blocking mode. Instead of that, change blocking mode by QEMU
> file descriptors users when necessary, e.g. like migration.
> 
> We need to preserve the state of the file descriptor in case it's still
> used by an external process and before the QEMU itself started
> using it.
> 
> E.g. our local migration scenario with TAP networking looks like this:
>  1. Create TAP devices and pass file descriptors to source QEMU
>  2. Launch destination QEMU (-incoming defer) and pass same descriptors to it.
>  3. Start migration
> 
> In such scenario setting blocking state at stage (2) will hang source QEMU
> since TAP fd suddenly become blocking.

Is it possible to add a special flag or API for preserving the
O_NONBLOCK open flag? That way the rest of QEMU could continue to safely
reset the flag while the tap fd passing code would explicitly ask for
the O_NONBLOCK open flag to be preserved. That seems safer but I haven't
checked whether it's possible to do this.

> 
> Signed-off-by: Andrey Ryabinin <arbn@yandex-team.com>
> ---
>  chardev/char-socket.c | 3 ---
>  io/channel-socket.c   | 3 ---
>  migration/fd.c        | 2 ++
>  3 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index dc4e218eeb6..c9592fb5836 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -310,9 +310,6 @@ static ssize_t tcp_chr_recv(Chardev *chr, char *buf, size_t len)
>              continue;
>          }
>  
> -        /* O_NONBLOCK is preserved across SCM_RIGHTS so reset it */
> -        qemu_socket_set_block(fd);
> -
>  #ifndef MSG_CMSG_CLOEXEC
>          qemu_set_cloexec(fd);
>  #endif
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index dc9c165de11..8b9679460dc 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -479,9 +479,6 @@ static void qio_channel_socket_copy_fds(struct msghdr *msg,
>                  continue;
>              }
>  
> -            /* O_NONBLOCK is preserved across SCM_RIGHTS so reset it */
> -            qemu_socket_set_block(fd);
> -
>  #ifndef MSG_CMSG_CLOEXEC
>              qemu_set_cloexec(fd);
>  #endif
> diff --git a/migration/fd.c b/migration/fd.c
> index 6f2f50475f4..793fffeb169 100644
> --- a/migration/fd.c
> +++ b/migration/fd.c
> @@ -60,6 +60,8 @@ void fd_start_incoming_migration(const char *fdname, Error **errp)
>          return;
>      }
>  
> +    qemu_socket_set_block(fd);
> +
>      trace_migration_fd_incoming(fd);
>  
>      ioc = qio_channel_new_fd(fd, errp);
> -- 
> 2.35.1
>

Re: [PATCH 1/2] chardev: don't set O_NONBLOCK on SCM_RIGHTS file descriptors.

Posted by Andrey Ryabinin 3 years, 7 months ago


On 6/15/22 16:12, Stefan Hajnoczi wrote:
> On Tue, Jun 14, 2022 at 02:18:42PM +0300, Andrey Ryabinin wrote:
>> This reverts commit 9b938c7262e4 ("chardev: clear O_NONBLOCK on SCM_RIGHTS file descriptors").
>> File descriptor passed to QEMU via 'getfd' QMP command always
>> changed to blocking mode. Instead of that, change blocking mode by QEMU
>> file descriptors users when necessary, e.g. like migration.
>>
>> We need to preserve the state of the file descriptor in case it's still
>> used by an external process and before the QEMU itself started
>> using it.
>>
>> E.g. our local migration scenario with TAP networking looks like this:
>>  1. Create TAP devices and pass file descriptors to source QEMU
>>  2. Launch destination QEMU (-incoming defer) and pass same descriptors to it.
>>  3. Start migration
>>
>> In such scenario setting blocking state at stage (2) will hang source QEMU
>> since TAP fd suddenly become blocking.
> 
> Is it possible to add a special flag or API for preserving the
> O_NONBLOCK open flag? That way the rest of QEMU could continue to safely
> reset the flag while the tap fd passing code would explicitly ask for
> the O_NONBLOCK open flag to be preserved. That seems safer but I haven't
> checked whether it's possible to do this.
> 

The only possibility I see here is embedding some kind 'nonblock' in the message
itself along with fds. Not sure if this sensible approach.

Not changing fd state seems like more correct approach to me. E.g. I would expect
that sending fd to qemu and executing qmp commands 'getfd' & 'closefd' shouldn't
induce any changes in fd state. Which is currently no true.

Re: [PATCH 1/2] chardev: don't set O_NONBLOCK on SCM_RIGHTS file descriptors.

Posted by Daniel P. Berrangé 3 years, 7 months ago

On Fri, Jun 24, 2022 at 02:00:15PM +0300, Andrey Ryabinin wrote:
> 
> 
> On 6/15/22 16:12, Stefan Hajnoczi wrote:
> > On Tue, Jun 14, 2022 at 02:18:42PM +0300, Andrey Ryabinin wrote:
> >> This reverts commit 9b938c7262e4 ("chardev: clear O_NONBLOCK on SCM_RIGHTS file descriptors").
> >> File descriptor passed to QEMU via 'getfd' QMP command always
> >> changed to blocking mode. Instead of that, change blocking mode by QEMU
> >> file descriptors users when necessary, e.g. like migration.
> >>
> >> We need to preserve the state of the file descriptor in case it's still
> >> used by an external process and before the QEMU itself started
> >> using it.
> >>
> >> E.g. our local migration scenario with TAP networking looks like this:
> >>  1. Create TAP devices and pass file descriptors to source QEMU
> >>  2. Launch destination QEMU (-incoming defer) and pass same descriptors to it.
> >>  3. Start migration
> >>
> >> In such scenario setting blocking state at stage (2) will hang source QEMU
> >> since TAP fd suddenly become blocking.
> > 
> > Is it possible to add a special flag or API for preserving the
> > O_NONBLOCK open flag? That way the rest of QEMU could continue to safely
> > reset the flag while the tap fd passing code would explicitly ask for
> > the O_NONBLOCK open flag to be preserved. That seems safer but I haven't
> > checked whether it's possible to do this.
> > 
> 
> The only possibility I see here is embedding some kind 'nonblock' in the message
> itself along with fds. Not sure if this sensible approach.
> 
> Not changing fd state seems like more correct approach to me. E.g. I would expect
> that sending fd to qemu and executing qmp commands 'getfd' & 'closefd' shouldn't
> induce any changes in fd state. Which is currently no true.

I think that's a wrong expectation. The contract with 'getfd' is that
you are giving the target QEMU ownership of the file. The caller should
not expect todo anything with the FD after it has passd it to QEMU, and
target QEMU has freedom todo whatever it wants.  Admittedly this usage
model doesn't fit with what you're trying to make it now do, but those
are the historical expectations of getfd.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|