[Qemu-devel] [PATCH 0/3] Migration cancel with dead network

[Qemu-devel] [PATCH 0/3] Migration cancel with dead network

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

If the remote host, or networking dies during a migration, the socket can be
waiting for a long timeout, and migration_cancel can't complete the cancel
for a long time (and you can't start a new one to somewhere else).
(Where 'long' is the TCP timeout, that's ~15 mins)

This patch set uses the shutdown(2) syscall to unblock any write/sends that
are in progress to let the migrate_cancel happen quickly.

  1/3: socket shutdown  - An updated patch from my postcopy world to
                          add a shut_down method on QEMUFile - only
                          for 'socket' (where the syscall is supported).

  2/3: Handle bi-directional communication for fd migration
                        - A patch from Cristian Klein to use the socket
                          QEMUFile for FDs that are passed in, if the FDs
                          are sockets; this is needed so that libvirt
                          migrations can take advantage of the other patches.
                          Again this patch (and its naming) come from the
                          postcopy world.

  3/3: migration_cancel: shutdown migration socket
                        - A new patch that uses the shutdown in migrate_fd_cancel


Note this does not fix the timeout if you try to migrate to an already dead host;
the connect timeout is typically a much shorter 2 minutes anyway.

Dave

Cristian Klein (1):
  Handle bi-directional communication for fd migration

Dr. David Alan Gilbert (2):
  socket shutdown
  migration_cancel: shutdown migration socket

 include/migration/qemu-file.h | 10 ++++++++++
 include/qemu/sockets.h        |  7 +++++++
 migration/fd.c                | 24 ++++++++++++++++++++++--
 migration/migration.c         | 12 ++++++++++++
 migration/qemu-file-unix.c    | 23 +++++++++++++++++++----
 migration/qemu-file.c         | 12 ++++++++++++
 6 files changed, 82 insertions(+), 6 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/3] socket shutdown

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Add QEMUFile interface to allow a socket to be 'shut down' - i.e. any
reads/writes will fail (and any blocking read/write will be woken).

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/migration/qemu-file.h | 10 ++++++++++
 include/qemu/sockets.h        |  7 +++++++
 migration/qemu-file-unix.c    | 23 +++++++++++++++++++----
 migration/qemu-file.c         | 12 ++++++++++++
 4 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
index 401676b..d843c00 100644
--- a/include/migration/qemu-file.h
+++ b/include/migration/qemu-file.h
@@ -84,6 +84,14 @@ typedef size_t (QEMURamSaveFunc)(QEMUFile *f, void *opaque,
                                size_t size,
                                int *bytes_sent);
 
+/*
+ * Stop any read or write (depending on flags) on the underlying
+ * transport on the QEMUFile.
+ * Existing blocking reads/writes must be woken
+ * Returns 0 on success, -err on error
+ */
+typedef int (QEMUFileShutdownFunc)(void *opaque, bool rd, bool wr);
+
 typedef struct QEMUFileOps {
     QEMUFilePutBufferFunc *put_buffer;
     QEMUFileGetBufferFunc *get_buffer;
@@ -94,6 +102,7 @@ typedef struct QEMUFileOps {
     QEMURamHookFunc *after_ram_iterate;
     QEMURamHookFunc *hook_ram_load;
     QEMURamSaveFunc *save_page;
+    QEMUFileShutdownFunc *shut_down;
 } QEMUFileOps;
 
 struct QEMUSizedBuffer {
@@ -177,6 +186,7 @@ void qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
 int64_t qemu_file_get_rate_limit(QEMUFile *f);
 int qemu_file_get_error(QEMUFile *f);
 void qemu_file_set_error(QEMUFile *f, int ret);
+int qemu_file_shutdown(QEMUFile *f);
 void qemu_fflush(QEMUFile *f);
 
 static inline void qemu_put_be64s(QEMUFile *f, const uint64_t *pv)
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index f47dae6..7992ece 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -44,6 +44,13 @@ int socket_set_fast_reuse(int fd);
 int send_all(int fd, const void *buf, int len1);
 int recv_all(int fd, void *buf, int len1, bool single_read);
 
+#ifdef WIN32
+/* Windows has different names for the same constants with the same values */
+#define SHUT_RD   0
+#define SHUT_WR   1
+#define SHUT_RDWR 2
+#endif
+
 /* callback function for nonblocking connect
  * valid fd on success, negative error code on failure
  */
diff --git a/migration/qemu-file-unix.c b/migration/qemu-file-unix.c
index 9682396..bfbc086 100644
--- a/migration/qemu-file-unix.c
+++ b/migration/qemu-file-unix.c
@@ -26,6 +26,7 @@
 #include "qemu/sockets.h"
 #include "block/coroutine.h"
 #include "migration/qemu-file.h"
+#include "migration/qemu-file-internal.h"
 
 typedef struct QEMUFileSocket {
     int fd;
@@ -84,6 +85,17 @@ static int socket_close(void *opaque)
     return 0;
 }
 
+static int socket_shutdown(void *opaque, bool rd, bool wr)
+{
+    QEMUFileSocket *s = opaque;
+
+    if (shutdown(s->fd, rd ? (wr ? SHUT_RDWR : SHUT_RD) : SHUT_WR)) {
+        return -errno;
+    } else {
+        return 0;
+    }
+}
+
 static ssize_t unix_writev_buffer(void *opaque, struct iovec *iov, int iovcnt,
                                   int64_t pos)
 {
@@ -192,15 +204,18 @@ QEMUFile *qemu_fdopen(int fd, const char *mode)
 }
 
 static const QEMUFileOps socket_read_ops = {
-    .get_fd =     socket_get_fd,
+    .get_fd     = socket_get_fd,
     .get_buffer = socket_get_buffer,
-    .close =      socket_close
+    .close      = socket_close,
+    .shut_down  = socket_shutdown
+
 };
 
 static const QEMUFileOps socket_write_ops = {
-    .get_fd =     socket_get_fd,
+    .get_fd        = socket_get_fd,
     .writev_buffer = socket_writev_buffer,
-    .close =      socket_close
+    .close         = socket_close,
+    .shut_down     = socket_shutdown
 };
 
 QEMUFile *qemu_fopen_socket(int fd, const char *mode)
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index d2d4007..edb0d10 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -30,6 +30,18 @@
 #include "migration/qemu-file-internal.h"
 #include "trace.h"
 
+/*
+ * Stop a file from being read/written - not all backing files can do this
+ * typically only sockets can.
+ */
+int qemu_file_shutdown(QEMUFile *f)
+{
+    if (!f->ops->shut_down) {
+        return -ENOSYS;
+    }
+    return f->ops->shut_down(f->opaque, true, true);
+}
+
 bool qemu_file_mode_is_not_valid(const char *mode)
 {
     if (mode == NULL ||
-- 
2.1.0

[Qemu-devel] [PATCH 2/3] Handle bi-directional communication for fd migration

[Dr. David Alan Gilbert (git)]

From: Cristian Klein <cristian.klein@cs.umu.se>

libvirt prefers opening the TCP connection itself, for two reasons.
First, connection failed errors can be detected easier, without having
to parse qemu's error output.
Second, libvirt might be asked to secure the transfer by tunnelling the
communication through an TLS layer.
Therefore, libvirt opens the TCP connection itself and passes an FD to qemu
using QMP and a POSIX-specific mechanism.

Hence, in order to make the reverse-path work in such cases, qemu needs to
distinguish if the transmitted FD is a socket (reverse-path available)
or not (reverse-path might not be available) and use the corresponding
abstraction.

Signed-off-by: Cristian Klein <cristian.klein@cs.umu.se>
---
 migration/fd.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/migration/fd.c b/migration/fd.c
index d2e523a..129da99 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -31,13 +31,29 @@
     do { } while (0)
 #endif
 
+static bool fd_is_socket(int fd)
+{
+    struct stat stat;
+    int ret = fstat(fd, &stat);
+    if (ret == -1) {
+        /* When in doubt say no */
+        return false;
+    }
+    return S_ISSOCK(stat.st_mode);
+}
+
 void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp)
 {
     int fd = monitor_get_fd(cur_mon, fdname, errp);
     if (fd == -1) {
         return;
     }
-    s->file = qemu_fdopen(fd, "wb");
+
+    if (fd_is_socket(fd)) {
+        s->file = qemu_fopen_socket(fd, "wb");
+    } else {
+        s->file = qemu_fdopen(fd, "wb");
+    }
 
     migrate_fd_connect(s);
 }
@@ -58,7 +74,11 @@ void fd_start_incoming_migration(const char *infd, Error **errp)
     DPRINTF("Attempting to start an incoming migration via fd\n");
 
     fd = strtol(infd, NULL, 0);
-    f = qemu_fdopen(fd, "rb");
+    if (fd_is_socket(fd)) {
+        f = qemu_fopen_socket(fd, "rb");
+    } else {
+        f = qemu_fdopen(fd, "rb");
+    }
     if(f == NULL) {
         error_setg_errno(errp, errno, "failed to open the source descriptor");
         return;
-- 
2.1.0

[Qemu-devel] [PATCH 3/3] migration_cancel: shutdown migration socket

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Force shutdown on migration socket on cancel to cause the cancel
to complete even if the socket is blocked on a dead network.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/migration.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index c49a05a..b3adbc6 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -330,6 +330,7 @@ void migrate_fd_error(MigrationState *s)
 static void migrate_fd_cancel(MigrationState *s)
 {
     int old_state ;
+    QEMUFile *f = migrate_get_current()->file;
     trace_migrate_fd_cancel();
 
     do {
@@ -339,6 +340,17 @@ static void migrate_fd_cancel(MigrationState *s)
         }
         migrate_set_state(s, old_state, MIG_STATE_CANCELLING);
     } while (s->state != MIG_STATE_CANCELLING);
+
+    /*
+     * If we're unlucky the migration code might be stuck somewhere in a
+     * send/write while the network has failed and is waiting to timeout;
+     * if we've got shutdown(2) available then we can force it to quit.
+     * The outgoing qemu file gets closed in migrate_fd_cleanup that is
+     * called in a bh, so there is no race against this cancel.
+     */
+    if (s->state == MIG_STATE_CANCELLING && f) {
+        qemu_file_shutdown(f);
+    }
 }
 
 void add_migration_state_change_notifier(Notifier *notify)
-- 
2.1.0

Re: [Qemu-devel] [PATCH 0/3] Migration cancel with dead network

[Daniel P. Berrange]

On Thu, Jan 08, 2015 at 11:11:29AM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> If the remote host, or networking dies during a migration, the socket can be
> waiting for a long timeout, and migration_cancel can't complete the cancel
> for a long time (and you can't start a new one to somewhere else).
> (Where 'long' is the TCP timeout, that's ~15 mins)
> 
> This patch set uses the shutdown(2) syscall to unblock any write/sends that
> are in progress to let the migrate_cancel happen quickly.
> 
>   1/3: socket shutdown  - An updated patch from my postcopy world to
>                           add a shut_down method on QEMUFile - only
>                           for 'socket' (where the syscall is supported).
> 
>   2/3: Handle bi-directional communication for fd migration
>                         - A patch from Cristian Klein to use the socket
>                           QEMUFile for FDs that are passed in, if the FDs
>                           are sockets; this is needed so that libvirt
>                           migrations can take advantage of the other patches.
>                           Again this patch (and its naming) come from the
>                           postcopy world.
> 
>   3/3: migration_cancel: shutdown migration socket
>                         - A new patch that uses the shutdown in migrate_fd_cancel
> 
> 
> Note this does not fix the timeout if you try to migrate to an already dead host;
> the connect timeout is typically a much shorter 2 minutes anyway.

In any libvirt managed setup, you'd need to address the connect timeout
issue in libvirt instead, since libvirt always uses fd based migration.
ie libvirt estabishes the connection & then passes the TCP Socket to
QEMU.

It should be possible for libvirt to use a non-blocking connect()
call and catch use of its virDomainMigrateCancel APi to stop the
connection attempt.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 0/3] Migration cancel with dead network

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berrange@redhat.com) wrote:
> On Thu, Jan 08, 2015 at 11:11:29AM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > If the remote host, or networking dies during a migration, the socket can be
> > waiting for a long timeout, and migration_cancel can't complete the cancel
> > for a long time (and you can't start a new one to somewhere else).
> > (Where 'long' is the TCP timeout, that's ~15 mins)
> > 
> > This patch set uses the shutdown(2) syscall to unblock any write/sends that
> > are in progress to let the migrate_cancel happen quickly.
> > 
> >   1/3: socket shutdown  - An updated patch from my postcopy world to
> >                           add a shut_down method on QEMUFile - only
> >                           for 'socket' (where the syscall is supported).
> > 
> >   2/3: Handle bi-directional communication for fd migration
> >                         - A patch from Cristian Klein to use the socket
> >                           QEMUFile for FDs that are passed in, if the FDs
> >                           are sockets; this is needed so that libvirt
> >                           migrations can take advantage of the other patches.
> >                           Again this patch (and its naming) come from the
> >                           postcopy world.
> > 
> >   3/3: migration_cancel: shutdown migration socket
> >                         - A new patch that uses the shutdown in migrate_fd_cancel
> > 
> > 
> > Note this does not fix the timeout if you try to migrate to an already dead host;
> > the connect timeout is typically a much shorter 2 minutes anyway.
> 
> In any libvirt managed setup, you'd need to address the connect timeout
> issue in libvirt instead, since libvirt always uses fd based migration.
> ie libvirt estabishes the connection & then passes the TCP Socket to
> QEMU.
> 
> It should be possible for libvirt to use a non-blocking connect()
> call and catch use of its virDomainMigrateCancel APi to stop the
> connection attempt.

Yes, although as I say I've not fixed the (must shorter) connect side timeout
on the QEMU side either.

How does libvirt behave if you cancel a tunnelled migration with the network
dieing in the middle?

Dave

> 
> Regards,
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/3] Migration cancel with dead network

[Daniel P. Berrange]

On Thu, Jan 08, 2015 at 11:29:59AM +0000, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berrange@redhat.com) wrote:
> > On Thu, Jan 08, 2015 at 11:11:29AM +0000, Dr. David Alan Gilbert (git) wrote:
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > If the remote host, or networking dies during a migration, the socket can be
> > > waiting for a long timeout, and migration_cancel can't complete the cancel
> > > for a long time (and you can't start a new one to somewhere else).
> > > (Where 'long' is the TCP timeout, that's ~15 mins)
> > > 
> > > This patch set uses the shutdown(2) syscall to unblock any write/sends that
> > > are in progress to let the migrate_cancel happen quickly.
> > > 
> > >   1/3: socket shutdown  - An updated patch from my postcopy world to
> > >                           add a shut_down method on QEMUFile - only
> > >                           for 'socket' (where the syscall is supported).
> > > 
> > >   2/3: Handle bi-directional communication for fd migration
> > >                         - A patch from Cristian Klein to use the socket
> > >                           QEMUFile for FDs that are passed in, if the FDs
> > >                           are sockets; this is needed so that libvirt
> > >                           migrations can take advantage of the other patches.
> > >                           Again this patch (and its naming) come from the
> > >                           postcopy world.
> > > 
> > >   3/3: migration_cancel: shutdown migration socket
> > >                         - A new patch that uses the shutdown in migrate_fd_cancel
> > > 
> > > 
> > > Note this does not fix the timeout if you try to migrate to an already dead host;
> > > the connect timeout is typically a much shorter 2 minutes anyway.
> > 
> > In any libvirt managed setup, you'd need to address the connect timeout
> > issue in libvirt instead, since libvirt always uses fd based migration.
> > ie libvirt estabishes the connection & then passes the TCP Socket to
> > QEMU.
> > 
> > It should be possible for libvirt to use a non-blocking connect()
> > call and catch use of its virDomainMigrateCancel APi to stop the
> > connection attempt.
> 
> Yes, although as I say I've not fixed the (must shorter) connect side timeout
> on the QEMU side either.
> 
> How does libvirt behave if you cancel a tunnelled migration with the network
> dieing in the middle?

I think its just reliant on the TCP timeout - we don't have any of the
clever use of 'shutdown' that you're adding here. I guess it could make
sense for libvirt to use shutdown.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 0/3] Migration cancel with dead network

[Paolo Bonzini]

On 08/01/2015 12:11, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> If the remote host, or networking dies during a migration, the socket can be
> waiting for a long timeout, and migration_cancel can't complete the cancel
> for a long time (and you can't start a new one to somewhere else).
> (Where 'long' is the TCP timeout, that's ~15 mins)
> 
> This patch set uses the shutdown(2) syscall to unblock any write/sends that
> are in progress to let the migrate_cancel happen quickly.
> 
>   1/3: socket shutdown  - An updated patch from my postcopy world to
>                           add a shut_down method on QEMUFile - only
>                           for 'socket' (where the syscall is supported).
> 
>   2/3: Handle bi-directional communication for fd migration
>                         - A patch from Cristian Klein to use the socket
>                           QEMUFile for FDs that are passed in, if the FDs
>                           are sockets; this is needed so that libvirt
>                           migrations can take advantage of the other patches.
>                           Again this patch (and its naming) come from the
>                           postcopy world.
> 
>   3/3: migration_cancel: shutdown migration socket
>                         - A new patch that uses the shutdown in migrate_fd_cancel
> 
> 
> Note this does not fix the timeout if you try to migrate to an already dead host;
> the connect timeout is typically a much shorter 2 minutes anyway.

A more precise commit message for 2/3 would be nice, but the code is good.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 1/3] socket shutdown

[Amit Shah]

On (Thu) 08 Jan 2015 [11:11:30], Dr. David Alan Gilbert (git) wrote:

>  static const QEMUFileOps socket_write_ops = {
> -    .get_fd =     socket_get_fd,
> +    .get_fd        = socket_get_fd,
>      .writev_buffer = socket_writev_buffer,
> -    .close =      socket_close
> +    .close         = socket_close,
> +    .shut_down     = socket_shutdown

Just including commas, even for the last item, helps in reducing the
number of changed lines..


		Amit

Re: [Qemu-devel] [PATCH 0/3] Migration cancel with dead network

[Amit Shah]

On (Thu) 08 Jan 2015 [11:11:29], Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> If the remote host, or networking dies during a migration, the socket can be
> waiting for a long timeout, and migration_cancel can't complete the cancel
> for a long time (and you can't start a new one to somewhere else).
> (Where 'long' is the TCP timeout, that's ~15 mins)
> 
> This patch set uses the shutdown(2) syscall to unblock any write/sends that
> are in progress to let the migrate_cancel happen quickly.
> 
>   1/3: socket shutdown  - An updated patch from my postcopy world to
>                           add a shut_down method on QEMUFile - only
>                           for 'socket' (where the syscall is supported).
> 
>   2/3: Handle bi-directional communication for fd migration
>                         - A patch from Cristian Klein to use the socket
>                           QEMUFile for FDs that are passed in, if the FDs
>                           are sockets; this is needed so that libvirt
>                           migrations can take advantage of the other patches.
>                           Again this patch (and its naming) come from the
>                           postcopy world.
> 
>   3/3: migration_cancel: shutdown migration socket
>                         - A new patch that uses the shutdown in migrate_fd_cancel
> 
> 
> Note this does not fix the timeout if you try to migrate to an already dead host;
> the connect timeout is typically a much shorter 2 minutes anyway.

Reviewed-by: Amit Shah <amit.shah@redhat.com>

		Amit