12:04 p.m.
In testing the "vol-download" command in virsh, downloading a
1G file takes a ridiculous amount of time (minutes) with the
new SSH helper.
After the first patch is applied the time gets down to a much
more reasonable 5.5 seconds on my test machine.
By comparison netcat achieved 4 seconds.
After applying the second patch, the time is reduced to 3.5
seconds, so we actually end up beating netcat, as long as
we have glib >= 2.64.0 available.
Daniel P. Berrangé (2):
remote: make ssh-helper massively faster
util: avoid glib event loop workaround where possible
src/remote/remote_ssh_helper.c | 113 ++++++++++++++++++++-------------
src/util/vireventglib.c | 29 ++++++---
2 files changed, 89 insertions(+), 53 deletions(-)
--
2.25.4
12:04 p.m.
New subject: [libvirt PATCH 1/2] remote: make ssh-helper massively faster
It was reported that the performance of tunnelled migration and
volume upload/download regressed in 6.9.0, when the virt-ssh-helper
is used for remote SSH tunnelling instead of netcat.
When seeing data available to read from stdin, or the socket,
the current code will allocate at most 1k of extra space in
the buffer it has.
After writing data to the socket, or stdout, if more than 1k
of extra space is in the buffer, it will reallocate to free
up that space.
This results in a huge number of mallocs when doing I/O, as
well as a huge number of syscalls since at most 1k of data
will be read/written at a time.
Also if writing blocks for some reason, it will continue to
read data with no memory bound which is bad.
This changes the code to use a 1 MB fixed size buffer in each
direction. If that buffer becomes full, it will update the
watches to stop reading more data. It will never reallocate
the buffer at runtime.
This increases the performance by orders of magnitude.
Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
src/remote/remote_ssh_helper.c | 113 ++++++++++++++++++++-------------
1 file changed, 68 insertions(+), 45 deletions(-)
diff --git a/src/remote/remote_ssh_helper.c b/src/remote/remote_ssh_helper.c
index 0da55c1d1f..8ed7e64507 100644
--- a/src/remote/remote_ssh_helper.c
+++ b/src/remote/remote_ssh_helper.c
@@ -32,6 +32,8 @@
#define VIR_FROM_THIS VIR_FROM_REMOTE
+#define SSH_BUF_SIZE (1024 * 1024)
+
VIR_LOG_INIT("remote.remote_ssh_helper");
struct virRemoteSSHHelperBuffer {
@@ -45,8 +47,11 @@ typedef virRemoteSSHHelper *virRemoteSSHHelperPtr;
struct virRemoteSSHHelper {
bool quit;
virNetSocketPtr sock;
+ int sockEvents;
int stdinWatch;
+ int stdinEvents;
int stdoutWatch;
+ int stdoutEvents;
struct virRemoteSSHHelperBuffer sockToTerminal;
struct virRemoteSSHHelperBuffer terminalToSock;
@@ -75,6 +80,40 @@ virRemoteSSHHelperShutdown(virRemoteSSHHelperPtr proxy)
}
+static void
+virRemoteSSHHelperUpdateEvents(virRemoteSSHHelperPtr proxy)
+{
+ int sockEvents = 0;
+ int stdinEvents = 0;
+ int stdoutEvents = 0;
+
+ if (proxy->terminalToSock.offset != 0)
+ sockEvents |= VIR_EVENT_HANDLE_WRITABLE;
+ if (proxy->terminalToSock.offset < proxy->terminalToSock.length)
+ stdinEvents |= VIR_EVENT_HANDLE_READABLE;
+
+ if (proxy->sockToTerminal.offset != 0)
+ stdoutEvents |= VIR_EVENT_HANDLE_WRITABLE;
+ if (proxy->sockToTerminal.offset < proxy->sockToTerminal.length)
+ sockEvents |= VIR_EVENT_HANDLE_READABLE;
+
+ if (sockEvents != proxy->sockEvents) {
+ VIR_DEBUG("Update sock events %d -> %d", proxy->sockEvents,
sockEvents);
+ virNetSocketUpdateIOCallback(proxy->sock, sockEvents);
+ proxy->sockEvents = sockEvents;
+ }
+ if (stdinEvents != proxy->stdinEvents) {
+ VIR_DEBUG("Update stdin events %d -> %d", proxy->stdinEvents,
stdinEvents);
+ virEventUpdateHandle(proxy->stdinWatch, stdinEvents);
+ proxy->stdinEvents = stdinEvents;
+ }
+ if (stdoutEvents != proxy->stdoutEvents) {
+ VIR_DEBUG("Update stdout events %d -> %d", proxy->stdoutEvents,
stdoutEvents);
+ virEventUpdateHandle(proxy->stdoutWatch, stdoutEvents);
+ proxy->stdoutEvents = stdoutEvents;
+ }
+}
+
static void
virRemoteSSHHelperEventOnSocket(virNetSocketPtr sock,
int events,
@@ -91,14 +130,9 @@ virRemoteSSHHelperEventOnSocket(virNetSocketPtr sock,
proxy->sockToTerminal.offset;
int got;
- if (avail < 1024) {
- if (VIR_REALLOC_N(proxy->sockToTerminal.data,
- proxy->sockToTerminal.length + 1024) < 0) {
- virRemoteSSHHelperShutdown(proxy);
- return;
- }
- proxy->sockToTerminal.length += 1024;
- avail += 1024;
+ if (avail == 0) {
+ VIR_DEBUG("Unexpectedly called with no space in buffer");
+ goto cleanup;
}
got = virNetSocketRead(sock,
@@ -117,15 +151,11 @@ virRemoteSSHHelperEventOnSocket(virNetSocketPtr sock,
return;
}
proxy->sockToTerminal.offset += got;
- if (proxy->sockToTerminal.offset)
- virEventUpdateHandle(proxy->stdoutWatch,
- VIR_EVENT_HANDLE_WRITABLE);
}
if (events & VIR_EVENT_HANDLE_WRITABLE &&
proxy->terminalToSock.offset) {
ssize_t done;
- size_t avail;
done = virNetSocketWrite(proxy->sock,
proxy->terminalToSock.data,
proxy->terminalToSock.offset);
@@ -135,26 +165,21 @@ virRemoteSSHHelperEventOnSocket(virNetSocketPtr sock,
virRemoteSSHHelperShutdown(proxy);
return;
}
+
memmove(proxy->terminalToSock.data,
proxy->terminalToSock.data + done,
proxy->terminalToSock.offset - done);
proxy->terminalToSock.offset -= done;
-
- avail = proxy->terminalToSock.length - proxy->terminalToSock.offset;
- if (avail > 1024) {
- ignore_value(VIR_REALLOC_N(proxy->terminalToSock.data,
- proxy->terminalToSock.offset + 1024));
- proxy->terminalToSock.length = proxy->terminalToSock.offset + 1024;
- }
}
- if (!proxy->terminalToSock.offset)
- virNetSocketUpdateIOCallback(proxy->sock,
- VIR_EVENT_HANDLE_READABLE);
if (events & VIR_EVENT_HANDLE_ERROR ||
events & VIR_EVENT_HANDLE_HANGUP) {
virRemoteSSHHelperShutdown(proxy);
+ return;
}
+
+ cleanup:
+ virRemoteSSHHelperUpdateEvents(proxy);
}
@@ -175,14 +200,9 @@ virRemoteSSHHelperEventOnStdin(int watch G_GNUC_UNUSED,
proxy->terminalToSock.offset;
int got;
- if (avail < 1024) {
- if (VIR_REALLOC_N(proxy->terminalToSock.data,
- proxy->terminalToSock.length + 1024) < 0) {
- virRemoteSSHHelperShutdown(proxy);
- return;
- }
- proxy->terminalToSock.length += 1024;
- avail += 1024;
+ if (avail == 0) {
+ VIR_DEBUG("Unexpectedly called with no space in buffer");
+ goto cleanup;
}
got = read(fd,
@@ -203,10 +223,6 @@ virRemoteSSHHelperEventOnStdin(int watch G_GNUC_UNUSED,
}
proxy->terminalToSock.offset += got;
- if (proxy->terminalToSock.offset)
- virNetSocketUpdateIOCallback(proxy->sock,
- VIR_EVENT_HANDLE_READABLE |
- VIR_EVENT_HANDLE_WRITABLE);
}
if (events & VIR_EVENT_HANDLE_ERROR) {
@@ -220,6 +236,9 @@ virRemoteSSHHelperEventOnStdin(int watch G_GNUC_UNUSED,
virRemoteSSHHelperShutdown(proxy);
return;
}
+
+ cleanup:
+ virRemoteSSHHelperUpdateEvents(proxy);
}
@@ -238,7 +257,6 @@ virRemoteSSHHelperEventOnStdout(int watch G_GNUC_UNUSED,
if (events & VIR_EVENT_HANDLE_WRITABLE &&
proxy->sockToTerminal.offset) {
ssize_t done;
- size_t avail;
done = write(fd,
proxy->sockToTerminal.data,
proxy->sockToTerminal.offset);
@@ -253,18 +271,8 @@ virRemoteSSHHelperEventOnStdout(int watch G_GNUC_UNUSED,
proxy->sockToTerminal.data + done,
proxy->sockToTerminal.offset - done);
proxy->sockToTerminal.offset -= done;
-
- avail = proxy->sockToTerminal.length - proxy->sockToTerminal.offset;
- if (avail > 1024) {
- ignore_value(VIR_REALLOC_N(proxy->sockToTerminal.data,
- proxy->sockToTerminal.offset + 1024));
- proxy->sockToTerminal.length = proxy->sockToTerminal.offset + 1024;
- }
}
- if (!proxy->sockToTerminal.offset)
- virEventUpdateHandle(proxy->stdoutWatch, 0);
-
if (events & VIR_EVENT_HANDLE_ERROR) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("IO error
stdout"));
virRemoteSSHHelperShutdown(proxy);
@@ -276,6 +284,8 @@ virRemoteSSHHelperEventOnStdout(int watch G_GNUC_UNUSED,
virRemoteSSHHelperShutdown(proxy);
return;
}
+
+ virRemoteSSHHelperUpdateEvents(proxy);
}
@@ -285,8 +295,21 @@ virRemoteSSHHelperRun(virNetSocketPtr sock)
int ret = -1;
virRemoteSSHHelper proxy = {
.sock = sock,
+ .sockEvents = VIR_EVENT_HANDLE_READABLE,
.stdinWatch = -1,
+ .stdinEvents = VIR_EVENT_HANDLE_READABLE,
.stdoutWatch = -1,
+ .stdoutEvents = 0,
+ .sockToTerminal = {
+ .offset = 0,
+ .length = SSH_BUF_SIZE,
+ .data = g_new0(char, SSH_BUF_SIZE),
+ },
+ .terminalToSock = {
+ .offset = 0,
+ .length = SSH_BUF_SIZE,
+ .data = g_new0(char, SSH_BUF_SIZE),
+ },
};
virEventRegisterDefaultImpl();
--
2.25.4
1:50 p.m.
New subject: [libvirt PATCH 1/2] remote: make ssh-helper massively faster
On 11/25/20 7:04 PM, Daniel P. Berrangé wrote:
It was reported that the performance of tunnelled migration and
volume upload/download regressed in 6.9.0, when the virt-ssh-helper
is used for remote SSH tunnelling instead of netcat.
When seeing data available to read from stdin, or the socket,
the current code will allocate at most 1k of extra space in
the buffer it has.
After writing data to the socket, or stdout, if more than 1k
of extra space is in the buffer, it will reallocate to free
up that space.
This results in a huge number of mallocs when doing I/O, as
well as a huge number of syscalls since at most 1k of data
will be read/written at a time.
Also if writing blocks for some reason, it will continue to
read data with no memory bound which is bad.
This changes the code to use a 1 MB fixed size buffer in each
direction. If that buffer becomes full, it will update the
watches to stop reading more data. It will never reallocate
the buffer at runtime.
This increases the performance by orders of magnitude.
Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
src/remote/remote_ssh_helper.c | 113 ++++++++++++++++++++-------------
1 file changed, 68 insertions(+), 45 deletions(-)
diff --git a/src/remote/remote_ssh_helper.c b/src/remote/remote_ssh_helper.c
index 0da55c1d1f..8ed7e64507 100644
--- a/src/remote/remote_ssh_helper.c
+++ b/src/remote/remote_ssh_helper.c
@@ -32,6 +32,8 @@
#define VIR_FROM_THIS VIR_FROM_REMOTE
+#define SSH_BUF_SIZE (1024 * 1024)
In theory, our RPC messages can be up to 32MB long (since
v3.4.0-rc1~26). Do you see any improvements with bigger buffer? But
since this buffer is allocated at all times it's a trade off I guess.
Michal
4:17 a.m.
New subject: [libvirt PATCH 1/2] remote: make ssh-helper massively faster
On Wed, Nov 25, 2020 at 08:50:07PM +0100, Michal Prívozník wrote:
On 11/25/20 7:04 PM, Daniel P. Berrangé wrote:
> It was reported that the performance of tunnelled migration and
> volume upload/download regressed in 6.9.0, when the virt-ssh-helper
> is used for remote SSH tunnelling instead of netcat.
>
> When seeing data available to read from stdin, or the socket,
> the current code will allocate at most 1k of extra space in
> the buffer it has.
>
> After writing data to the socket, or stdout, if more than 1k
> of extra space is in the buffer, it will reallocate to free
> up that space.
>
> This results in a huge number of mallocs when doing I/O, as
> well as a huge number of syscalls since at most 1k of data
> will be read/written at a time.
>
> Also if writing blocks for some reason, it will continue to
> read data with no memory bound which is bad.
>
> This changes the code to use a 1 MB fixed size buffer in each
> direction. If that buffer becomes full, it will update the
> watches to stop reading more data. It will never reallocate
> the buffer at runtime.
>
> This increases the performance by orders of magnitude.
>
> Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
> ---
> src/remote/remote_ssh_helper.c | 113 ++++++++++++++++++++-------------
> 1 file changed, 68 insertions(+), 45 deletions(-)
>
> diff --git a/src/remote/remote_ssh_helper.c b/src/remote/remote_ssh_helper.c
> index 0da55c1d1f..8ed7e64507 100644
> --- a/src/remote/remote_ssh_helper.c
> +++ b/src/remote/remote_ssh_helper.c
> @@ -32,6 +32,8 @@
> #define VIR_FROM_THIS VIR_FROM_REMOTE
> +#define SSH_BUF_SIZE (1024 * 1024)
In theory, our RPC messages can be up to 32MB long (since v3.4.0-rc1~26). Do
you see any improvements with bigger buffer? But since this buffer is
allocated at all times it's a trade off I guess.
I've not tested such a large buffer. The performance benefits of larger
buffers drop off fairly quickly though - every doubling of buffer has
50% less benefit than the previous doubling. Since tests show that this
1 MB buffer is enough to beat existing netcat perf, I figure we're fine
as is. If performance is absolutely critical, then honestly you should
not use tunnelled migration at all. QEMU's native TLS migration is
better, especially when combined with multi-FD which easily beats libvirtd.
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 :|
12:04 p.m.
New subject: [libvirt PATCH 2/2] util: avoid glib event loop workaround where possible
I previously did a workaround for a glib event loop race
that causes crashes:
commit 0db4743645b7a0611a3c0687f834205c9956f7fc
Author: Daniel P. Berrangé <berrange(a)redhat.com>
Date: Tue Jul 28 16:52:47 2020 +0100
util: avoid crash due to race in glib event loop code
it turns out that the workaround has a significant performance
penalty on I/O intensive workloads. We thus need to avoid the
workaround if we know we have a new enough glib to avoid the
race condition.
Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
src/util/vireventglib.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/src/util/vireventglib.c b/src/util/vireventglib.c
index 6842c6e806..8c5495bfab 100644
--- a/src/util/vireventglib.c
+++ b/src/util/vireventglib.c
@@ -189,9 +189,21 @@ virEventGLibHandleFind(int watch)
* If the last reference to a GSource is released in a non-main
* thread we're exposed to a race condition that causes a
* crash:
- * https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1358
- * Thus we're using an idle func to release our ref
+ *
+ * https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1358
+ *
+ * Thus we're using an idle func to release our ref...
+ *
+ * ...but this imposes a significant performance penalty on
+ * I/O intensive workloads which are sensitive to the iterations
+ * of the event loop, so avoid the workaround if we know we have
+ * new enough glib.
*/
+#if GLIB_CHECK_VERSION(2, 64, 0)
+# define g_vir_source_unref_safe(source) g_source_unref(source)
+#else
+# define g_vir_source_unref_safe(source) g_idle_add(virEventGLibSourceUnrefIdle,
source);
+
static gboolean
virEventGLibSourceUnrefIdle(gpointer data)
{
@@ -201,6 +213,7 @@ virEventGLibSourceUnrefIdle(gpointer data)
return FALSE;
}
+#endif
static void
@@ -231,7 +244,7 @@ virEventGLibHandleUpdate(int watch,
if (data->source != NULL) {
VIR_DEBUG("Removed old handle source=%p", data->source);
g_source_destroy(data->source);
- g_idle_add(virEventGLibSourceUnrefIdle, data->source);
+ g_vir_source_unref_safe(data->source);
}
data->source = virEventGLibAddSocketWatch(
@@ -245,7 +258,7 @@ virEventGLibHandleUpdate(int watch,
VIR_DEBUG("Removed old handle source=%p", data->source);
g_source_destroy(data->source);
- g_idle_add(virEventGLibSourceUnrefIdle, data->source);
+ g_vir_source_unref_safe(data->source);
data->source = NULL;
data->events = 0;
}
@@ -294,7 +307,7 @@ virEventGLibHandleRemove(int watch)
if (data->source != NULL) {
g_source_destroy(data->source);
- g_idle_add(virEventGLibSourceUnrefIdle, data->source);
+ g_vir_source_unref_safe(data->source);
data->source = NULL;
data->events = 0;
}
@@ -427,7 +440,7 @@ virEventGLibTimeoutUpdate(int timer,
if (interval >= 0) {
if (data->source != NULL) {
g_source_destroy(data->source);
- g_idle_add(virEventGLibSourceUnrefIdle, data->source);
+ g_vir_source_unref_safe(data->source);
}
data->interval = interval;
@@ -437,7 +450,7 @@ virEventGLibTimeoutUpdate(int timer,
goto cleanup;
g_source_destroy(data->source);
- g_idle_add(virEventGLibSourceUnrefIdle, data->source);
+ g_vir_source_unref_safe(data->source);
data->source = NULL;
}
@@ -486,7 +499,7 @@ virEventGLibTimeoutRemove(int timer)
if (data->source != NULL) {
g_source_destroy(data->source);
- g_idle_add(virEventGLibSourceUnrefIdle, data->source);
+ g_vir_source_unref_safe(data->source);
data->source = NULL;
}
--
2.25.4
1:37 p.m.
New subject: [libvirt PATCH 2/2] util: avoid glib event loop workaround where possible
On 11/25/20 7:04 PM, Daniel P. Berrangé wrote:
I previously did a workaround for a glib event loop race
that causes crashes:
commit 0db4743645b7a0611a3c0687f834205c9956f7fc
Author: Daniel P. Berrangé <berrange(a)redhat.com>
Date: Tue Jul 28 16:52:47 2020 +0100
util: avoid crash due to race in glib event loop code
it turns out that the workaround has a significant performance
penalty on I/O intensive workloads. We thus need to avoid the
workaround if we know we have a new enough glib to avoid the
race condition.
Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
src/util/vireventglib.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/src/util/vireventglib.c b/src/util/vireventglib.c
index 6842c6e806..8c5495bfab 100644
--- a/src/util/vireventglib.c
+++ b/src/util/vireventglib.c
@@ -189,9 +189,21 @@ virEventGLibHandleFind(int watch)
* If the last reference to a GSource is released in a non-main
* thread we're exposed to a race condition that causes a
* crash:
- * https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1358
- * Thus we're using an idle func to release our ref
+ *
+ * https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1358
+ *
+ * Thus we're using an idle func to release our ref...
+ *
+ * ...but this imposes a significant performance penalty on
+ * I/O intensive workloads which are sensitive to the iterations
+ * of the event loop, so avoid the workaround if we know we have
+ * new enough glib.
*/
+#if GLIB_CHECK_VERSION(2, 64, 0)
+# define g_vir_source_unref_safe(source) g_source_unref(source)
+#else
+# define g_vir_source_unref_safe(source) g_idle_add(virEventGLibSourceUnrefIdle,
source);
s/;//
+
Would something like the following be totally disgusting or only a bit?
#if !GLIB_CHECK_VERSION(2, 64, 0)
# define g_source_unref(source) g_idle_add(virEventGLibSourceUnrefIdle,
source)
#endif
- g_idle_add(...);
+ g_source_unref(...);
This way we could just drop the redefine once we upgrade min glib version.
Michal
4:18 a.m.
New subject: [libvirt PATCH 2/2] util: avoid glib event loop workaround where possible
On Wed, Nov 25, 2020 at 08:37:02PM +0100, Michal Prívozník wrote:
On 11/25/20 7:04 PM, Daniel P. Berrangé wrote:
> I previously did a workaround for a glib event loop race
> that causes crashes:
>
> commit 0db4743645b7a0611a3c0687f834205c9956f7fc
> Author: Daniel P. Berrangé <berrange(a)redhat.com>
> Date: Tue Jul 28 16:52:47 2020 +0100
>
> util: avoid crash due to race in glib event loop code
>
> it turns out that the workaround has a significant performance
> penalty on I/O intensive workloads. We thus need to avoid the
> workaround if we know we have a new enough glib to avoid the
> race condition.
>
> Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
> ---
> src/util/vireventglib.c | 29 +++++++++++++++++++++--------
> 1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/src/util/vireventglib.c b/src/util/vireventglib.c
> index 6842c6e806..8c5495bfab 100644
> --- a/src/util/vireventglib.c
> +++ b/src/util/vireventglib.c
> @@ -189,9 +189,21 @@ virEventGLibHandleFind(int watch)
> * If the last reference to a GSource is released in a non-main
> * thread we're exposed to a race condition that causes a
> * crash:
> - * https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1358
> - * Thus we're using an idle func to release our ref
> + *
> + * https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1358
> + *
> + * Thus we're using an idle func to release our ref...
> + *
> + * ...but this imposes a significant performance penalty on
> + * I/O intensive workloads which are sensitive to the iterations
> + * of the event loop, so avoid the workaround if we know we have
> + * new enough glib.
> */
> +#if GLIB_CHECK_VERSION(2, 64, 0)
> +# define g_vir_source_unref_safe(source) g_source_unref(source)
> +#else
> +# define g_vir_source_unref_safe(source) g_idle_add(virEventGLibSourceUnrefIdle,
source);
s/;//
> +
Would something like the following be totally disgusting or only a bit?
#if !GLIB_CHECK_VERSION(2, 64, 0)
# define g_source_unref(source) g_idle_add(virEventGLibSourceUnrefIdle,
source)
#endif
- g_idle_add(...);
+ g_source_unref(...);
This way we could just drop the redefine once we upgrade min glib version.
it is possible, but the hack with g_idle_add is semantically different
enough from g_source_unref that I want people to know that something
unusal is going on when reading the code.
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 :|
1:18 p.m.
On 11/25/20 3:04 PM, Daniel P. Berrangé wrote:
In testing the "vol-download" command in virsh, downloading
a
1G file takes a ridiculous amount of time (minutes) with the
new SSH helper.
After the first patch is applied the time gets down to a much
more reasonable 5.5 seconds on my test machine.
By comparison netcat achieved 4 seconds.
After applying the second patch, the time is reduced to 3.5
seconds, so we actually end up beating netcat, as long as
we have glib >= 2.64.0 available.
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
Given the annoyance of the regression it fixes I believe it's worth
pushing it through the release freeze.
Thanks,
DHB
Daniel P. Berrangé (2):
remote: make ssh-helper massively faster
util: avoid glib event loop workaround where possible
src/remote/remote_ssh_helper.c | 113 ++++++++++++++++++++-------------
src/util/vireventglib.c | 29 ++++++---
2 files changed, 89 insertions(+), 53 deletions(-)
2:54 a.m.
On Wed, Nov 25, 2020 at 7:04 PM Daniel P. Berrangé <berrange(a)redhat.com> wrote:
In testing the "vol-download" command in virsh, downloading a
1G file takes a ridiculous amount of time (minutes) with the
new SSH helper.
After the first patch is applied the time gets down to a much
more reasonable 5.5 seconds on my test machine.
By comparison netcat achieved 4 seconds.
After applying the second patch, the time is reduced to 3.5
seconds, so we actually end up beating netcat, as long as
we have glib >= 2.64.0 available.
Daniel P. Berrangé (2):
remote: make ssh-helper massively faster
util: avoid glib event loop workaround where possible
I tested both patches and they didn't bring any new regressions as far
as my tests went.
The former issues of slow migration and vol-download are fixed and
speed is now roughly on-par with the old netcat mode.
Tested:
- vol-download local
- vol-download remote
- migration with explicit native transfer ?proxy=native
- migration with auto-mode selection
- virsh console
I also tested migrations with between patched and older versions:
- patched <-> 6.6 - fall back to netcat (expected and ok).
- patched -> 6.9 - slow (as before)
- 6.9 -> patched - fast (which is good as upgrade paths use migration
and it is sufficient to upgrade the target)
Tested-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
Thank you Daniel!
src/remote/remote_ssh_helper.c | 113
++++++++++++++++++++-------------
src/util/vireventglib.c | 29 ++++++---
2 files changed, 89 insertions(+), 53 deletions(-)
--
2.25.4
--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd
1492
days inactive
1493
days old
8 comments
4 participants
participants (4)
-
Christian Ehrhardt -
Daniel Henrique Barboza -
Daniel P. Berrangé -
Michal Prívozník