qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] Re: [BUG] Regression of exec migration


From: Pierre Riteau
Subject: Re: [Qemu-devel] Re: [BUG] Regression of exec migration
Date: Tue, 1 Sep 2009 13:57:28 +0200

On 1 sept. 09, at 00:55, Charles Duffy wrote:

Anthony Liguori wrote:
I think the fundamental problem is that exec migration shouldn't use popen. It should create a pipe() and do a proper fork/exec.
I don't think the f* function support asynchronous IO properly.

Per this thread and a suggestion from Anthony on IRC, I've taken a shot at modifying the exec: migration code to be closer to the original kvm implementation. The severe performance degradation no longer appears to be present.

This has been successfully smoke tested against the stable-0.11 branch; the rebase to master, attached, has only minor changes.

I'm afraid this solves the problem only because you disabled non blocking operations on the file descriptor, effectively reverting 907500095851230a480b14bc852c4e49d32cb16d.
See comment inline.


Signed-off-by: Charles Duffy <address@hidden>
From c48ba672cd92d1d173fe9cb93f8e268d0d7952bc Mon Sep 17 00:00:00 2001
From: Charles Duffy <address@hidden>
Date: Fri, 28 Aug 2009 22:17:47 -0500
Subject: [PATCH 1/2] stop using popen for outgoing migrations

---
migration-exec.c | 95 +++++++++++++++++++++++++++++++++++ +-----------------
qemu-common.h    |    2 +
2 files changed, 66 insertions(+), 31 deletions(-)

diff --git a/migration-exec.c b/migration-exec.c
index b45c833..c36e756 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -31,25 +31,72 @@
    do { } while (0)
#endif

-static int file_errno(FdMigrationState *s)
+// opaque is pid
+typedef struct ExecMigrationState
+{
+    pid_t pid;
+} ExecMigrationState;
+
+static int exec_errno(FdMigrationState *s)
{
    return errno;
}

-static int file_write(FdMigrationState *s, const void * buf, size_t size) +static int exec_write(FdMigrationState *s, const void * buf, size_t size)
{
    return write(s->fd, buf, size);
}

static int exec_close(FdMigrationState *s)
{
+    int ret, status;
+
    dprintf("exec_close\n");
-    if (s->opaque) {
-        qemu_fclose(s->opaque);
-        s->opaque = NULL;
-        s->fd = -1;
+
+    close(s->fd);
+
+    if (!(s->opaque))
+        return -1;
+
+    do {
+ ret = waitpid(((ExecMigrationState*)(s->opaque))->pid, &status, 0);
+    } while (ret == -1 && errno == EINTR);
+
+    qemu_free(s->opaque);
+    s->opaque = 0;
+
+    return status;
+}
+
+static pid_t exec_start_migration(const char *command, int *fd) {
+    const char *argv[] = { "sh", "-c", command, NULL };
+    int fds[2];
+    pid_t pid;
+
+    if (pipe(fds) == -1) {
+        dprintf("pipe() (%s)\n", strerr(errno));
+        return 0;
    }
-    return 0;
+
+    pid = fork();
+    if(pid == -1) {
+        close(fds[0]);
+        close(fds[1]);
+        dprintf("fork error (%s)\n", strerror(errno));
+        return 0;
+    } else if (pid == 0) {
+        /* child process */
+        close(fds[1]);
+        dup2(fds[0], STDIN_FILENO);
+        execv("/bin/sh", (char **)argv);
+        exit(1);
+    }
+    close(fds[0]);
+
+    fcntl(fds[1], O_NONBLOCK);
+
+    *fd = fds[1];
+    return pid;
}

MigrationState *exec_start_outgoing_migration(const char *command,
@@ -57,29 +104,20 @@ MigrationState *exec_start_outgoing_migration(const char *command,
                                             int detach)
{
    FdMigrationState *s;
-    FILE *f;
+    int fd, pid;

-    s = qemu_mallocz(sizeof(*s));
+    pid = exec_start_migration(command, &fd);
+    if(!pid) return NULL;

-    f = popen(command, "w");
-    if (f == NULL) {
-        dprintf("Unable to popen exec target\n");
-        goto err_after_alloc;
-    }
-
-    s->fd = fileno(f);
-    if (s->fd == -1) {
- dprintf("Unable to retrieve file descriptor for popen'd handle\n");
-        goto err_after_open;
-    }
-
-    socket_set_nonblock(s->fd);
+    s = qemu_mallocz(sizeof(*s));
+    s->opaque = qemu_mallocz(sizeof(ExecMigrationState));

-    s->opaque = qemu_popen(f, "w");
+    ((ExecMigrationState*)(s->opaque))->pid = pid;

+    s->fd = fd;
+    s->get_error = exec_errno;
+    s->write = exec_write;
    s->close = exec_close;
-    s->get_error = file_errno;
-    s->write = file_write;
    s->mig_state.cancel = migrate_fd_cancel;
    s->mig_state.get_status = migrate_fd_get_status;
    s->mig_state.release = migrate_fd_release;
@@ -93,14 +131,9 @@ MigrationState *exec_start_outgoing_migration(const char *command,

    migrate_fd_connect(s);
    return &s->mig_state;
-
-err_after_open:
-    pclose(f);
-err_after_alloc:
-    qemu_free(s);
-    return NULL;
}

+/* helper for incoming case */
static void exec_accept_incoming_migration(void *opaque)
{
    QEMUFile *f = opaque;
diff --git a/qemu-common.h b/qemu-common.h
index c8fb315..aeb1d78 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -24,6 +24,8 @@
#include <fcntl.h>
#include <sys/stat.h>
#include <assert.h>
+#include <sys/types.h>
+#include <sys/wait.h>
#include "config-host.h"

#ifndef O_LARGEFILE
--
1.6.4

From e7066ce85e969e43c8142f4845bc843e9d53f5e4 Mon Sep 17 00:00:00 2001
From: Charles Duffy <address@hidden>
Date: Fri, 28 Aug 2009 23:18:49 -0500
Subject: [PATCH 2/2] stop using popen for incoming migrations

---
migration-exec.c | 51 ++++++++++++++++++++++++++++++ +--------------------
1 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/migration-exec.c b/migration-exec.c
index c36e756..3dfea18 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -31,10 +31,10 @@
    do { } while (0)
#endif

-// opaque is pid
typedef struct ExecMigrationState
{
    pid_t pid;
+    int fd; /* only used for incoming */
} ExecMigrationState;

static int exec_errno(FdMigrationState *s)
@@ -68,9 +68,9 @@ static int exec_close(FdMigrationState *s)
    return status;
}

-static pid_t exec_start_migration(const char *command, int *fd) {
+static pid_t exec_start_migration(const char *command, int *fd, unsigned char incoming) {
    const char *argv[] = { "sh", "-c", command, NULL };
-    int fds[2];
+    int fds[2], parent_fd, child_fd;
    pid_t pid;

    if (pipe(fds) == -1) {
@@ -78,6 +78,16 @@ static pid_t exec_start_migration(const char *command, int *fd) {
        return 0;
    }

+    if(incoming) {
+        /* child writes, parent reads */
+        parent_fd = fds[0];
+        child_fd = fds[1];
+    } else {
+        /* parent writes, child reads */
+        parent_fd = fds[1];
+        child_fd = fds[0];
+    }
+
    pid = fork();
    if(pid == -1) {
        close(fds[0]);
@@ -86,16 +96,14 @@ static pid_t exec_start_migration(const char *command, int *fd) {
        return 0;
    } else if (pid == 0) {
        /* child process */
-        close(fds[1]);
-        dup2(fds[0], STDIN_FILENO);
+        close(parent_fd);
+        dup2(child_fd, incoming ? STDOUT_FILENO : STDIN_FILENO);
        execv("/bin/sh", (char **)argv);
        exit(1);
    }
-    close(fds[0]);
-
-    fcntl(fds[1], O_NONBLOCK);
-
-    *fd = fds[1];
+    close(child_fd);
+    fcntl(parent_fd, O_NONBLOCK);

This doesn't look right, You should be using F_SETFL (like socket_set_nonblock does). If you check the return value of fcntl, you'll get -1 and errno set to "Invalid argument" (at least on my system, with a different ABI it could be a valid call to fcntl but doing something different than setting flags).

+    *fd = parent_fd;
    return pid;
}

@@ -106,7 +114,7 @@ MigrationState *exec_start_outgoing_migration(const char *command,
    FdMigrationState *s;
    int fd, pid;

-    pid = exec_start_migration(command, &fd);
+    pid = exec_start_migration(command, &fd, 0);
    if(!pid) return NULL;

    s = qemu_mallocz(sizeof(*s));
@@ -136,7 +144,8 @@ MigrationState *exec_start_outgoing_migration(const char *command,
/* helper for incoming case */
static void exec_accept_incoming_migration(void *opaque)
{
-    QEMUFile *f = opaque;
+    ExecMigrationState *s = opaque;
+    QEMUFile *f = qemu_popen(fdopen(s->fd, "rb"), "r");
    int ret;

    ret = qemu_loadvm_state(f);
@@ -153,22 +162,24 @@ static void exec_accept_incoming_migration(void *opaque)

err:
    qemu_fclose(f);
+    do {
+        waitpid(s->pid, NULL, 0);
+    } while (ret == -1 && errno == EINTR);
+    qemu_free(s);
}

int exec_start_incoming_migration(const char *command)
{
-    QEMUFile *f;
+    ExecMigrationState *s;
+
+    s = qemu_mallocz(sizeof(*s));

-    dprintf("Attempting to start an incoming migration\n");
-    f = qemu_popen_cmd(command, "r");
-    if(f == NULL) {
-        dprintf("Unable to apply qemu wrapper to popen file\n");
+    s->pid = exec_start_migration(command, &(s->fd), 1);
+    if(!s->pid) {
        return -errno;
    }

-    qemu_set_fd_handler2(qemu_stdio_fd(f), NULL,
-                        exec_accept_incoming_migration, NULL,
-                        (void *)(unsigned long)f);
+ qemu_set_fd_handler2(s->fd, NULL, exec_accept_incoming_migration, NULL, (void*)s);

    return 0;
}
--
1.6.4


--
Pierre Riteau -- http://perso.univ-rennes1.fr/pierre.riteau/





reply via email to

[Prev in Thread] Current Thread [Next in Thread]