From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MiFni-00009M-BK for qemu-devel@nongnu.org; Mon, 31 Aug 2009 18:56:38 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MiFnd-00006p-8r for qemu-devel@nongnu.org; Mon, 31 Aug 2009 18:56:37 -0400 Received: from [199.232.76.173] (port=45087 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MiFnd-00006i-2k for qemu-devel@nongnu.org; Mon, 31 Aug 2009 18:56:33 -0400 Received: from lo.gmane.org ([80.91.229.12]:35838) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1MiFnc-0002DI-80 for qemu-devel@nongnu.org; Mon, 31 Aug 2009 18:56:32 -0400 Received: from list by lo.gmane.org with local (Exim 4.50) id 1MiFnY-0001Rq-AZ for qemu-devel@nongnu.org; Tue, 01 Sep 2009 00:56:28 +0200 Received: from 143.166.197.6 ([143.166.197.6]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Tue, 01 Sep 2009 00:56:28 +0200 Received: from charles by 143.166.197.6 with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Tue, 01 Sep 2009 00:56:28 +0200 From: Charles Duffy Date: Mon, 31 Aug 2009 17:55:46 -0500 Message-ID: References: <4A969496.2070305@codemonkey.ws> <076B9FA6-C362-47C1-AA8B-70BF147843A6@irisa.fr> <4A96B353.8070600@codemonkey.ws> <65558604-590A-4FD7-877D-E676CE195C7A@irisa.fr> <4A97FA9B.1030401@codemonkey.ws> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------010205040703060800030301" In-Reply-To: <4A97FA9B.1030401@codemonkey.ws> Sender: news Subject: [Qemu-devel] Re: [BUG] Regression of exec migration List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org This is a multi-part message in MIME format. --------------010205040703060800030301 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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. Signed-off-by: Charles Duffy --------------010205040703060800030301 Content-Type: text/x-patch; name="0001-stop-using-popen-for-outgoing-migrations.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="0001-stop-using-popen-for-outgoing-migrations.patch" >>From c48ba672cd92d1d173fe9cb93f8e268d0d7952bc Mon Sep 17 00:00:00 2001 From: Charles Duffy 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 #include #include +#include +#include #include "config-host.h" #ifndef O_LARGEFILE -- 1.6.4 --------------010205040703060800030301 Content-Type: text/x-patch; name="0002-stop-using-popen-for-incoming-migrations.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="0002-stop-using-popen-for-incoming-migrations.patch" >>From e7066ce85e969e43c8142f4845bc843e9d53f5e4 Mon Sep 17 00:00:00 2001 From: Charles Duffy 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); + *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 --------------010205040703060800030301--