Libguestfs - May 2021 - [PATCH libnbd] copy: Always allow multiple connections

Using multiple connections when the server does not report
can_multi_conn is not safe in general, but for the special access
pattern in nbdcopy it is safe.

Use multiple connections are used even if the destination (e.g.
qemu-nbd) does not report the multi-conn flag.

Here is an example run with this change using qemu-nbd and qcow2
images. Using /dev/shm to get stable results.

When copying to memory, using 8 in-flight requests and request size of
128k best. I'm comparing also 2m requests to match qemu-img default io
size.

Testing with images on /dev/shm shows 25% speedup compared with single
connection. Testing with FC storage on real images shows 10% speedup[1].

$ qemu-img create -f qcow2 /dev/shm/dst.qcow2 6g
$ qemu-nbd -r -t -e 4 -f qcow2 -k /tmp/src.sock /var/tmp/fedora-32.qcow2 &
$ qemu-nbd -t -e 4 -f qcow2 -k /tmp/dst.sock /dev/shm/dst.qcow2 &
$ SRC=nbd+unix:///?socket=/tmp/src.sock
$ DST=nbd+unix:///?socket=/tmp/dst.sock

$ hyperfine -w3 -L s 131072,2097152 \
    ".libs/nbdcopy --requests=2 --request-size={s} --connections=4 --flush
$SRC $DST" \
    ".libs/nbdcopy --requests=8 --request-size={s} --connections=1 --flush
$SRC $DST" \
    "qemu-img convert -n -m 8 -W $SRC $DST"
Benchmark 1: .libs/nbdcopy --requests=2 --request-size=131072 --connections=4
--flush nbd+unix:///?socket=/tmp/src.sock nbd+unix:///?socket=/tmp/dst.sock
  Time (mean ? ?):     593.8 ms ?  16.6 ms    [User: 197.2 ms, System: 506.4 ms]
  Range (min ? max):   567.9 ms ? 617.8 ms    10 runs

Benchmark 2: .libs/nbdcopy --requests=8 --request-size=131072 --connections=1
--flush nbd+unix:///?socket=/tmp/src.sock nbd+unix:///?socket=/tmp/dst.sock
  Time (mean ? ?):     743.4 ms ?  82.4 ms    [User: 180.5 ms, System: 497.9 ms]
  Range (min ? max):   631.2 ms ? 816.5 ms    10 runs

Benchmark 3: qemu-img convert -n -m 8 -W nbd+unix:///?socket=/tmp/src.sock
nbd+unix:///?socket=/tmp/dst.sock
  Time (mean ? ?):      1.029 s ?  0.022 s    [User: 165.7 ms, System: 655.2 ms]
  Range (min ? max):    0.989 s ?  1.067 s    10 runs

Benchmark 4: .libs/nbdcopy --requests=2 --request-size=2097152 --connections=4
--flush nbd+unix:///?socket=/tmp/src.sock nbd+unix:///?socket=/tmp/dst.sock
  Time (mean ? ?):      1.007 s ?  0.012 s    [User: 238.0 ms, System: 961.8 ms]
  Range (min ? max):    0.992 s ?  1.029 s    10 runs

Benchmark 5: .libs/nbdcopy --requests=8 --request-size=2097152 --connections=1
--flush nbd+unix:///?socket=/tmp/src.sock nbd+unix:///?socket=/tmp/dst.sock
  Time (mean ? ?):      1.129 s ?  0.013 s    [User: 184.4 ms, System: 743.6 ms]
  Range (min ? max):    1.106 s ?  1.149 s    10 runs

Summary
  '.libs/nbdcopy --requests=2 --request-size=131072 --connections=4 --flush
nbd+unix:///?socket=/tmp/src.sock nbd+unix:///?socket=/tmp/dst.sock' ran
    1.25 ? 0.14 times faster than '.libs/nbdcopy --requests=8
--request-size=131072 --connections=1 --flush nbd+unix:///?socket=/tmp/src.sock
nbd+unix:///?socket=/tmp/dst.sock'
    1.70 ? 0.05 times faster than '.libs/nbdcopy --requests=2
--request-size=2097152 --connections=4 --flush nbd+unix:///?socket=/tmp/src.sock
nbd+unix:///?socket=/tmp/dst.sock'
    1.73 ? 0.06 times faster than 'qemu-img convert -n -m 8 -W
nbd+unix:///?socket=/tmp/src.sock nbd+unix:///?socket=/tmp/dst.sock'
    1.90 ? 0.06 times faster than '.libs/nbdcopy --requests=8
--request-size=2097152 --connections=1 --flush nbd+unix:///?socket=/tmp/src.sock
nbd+unix:///?socket=/tmp/dst.sock'

[1] https://listman.redhat.com/archives/libguestfs/2021-May/msg00124.html

Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
 copy/main.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/copy/main.c b/copy/main.c
index b9dbe1d..70a4e11 100644
--- a/copy/main.c
+++ b/copy/main.c
@@ -321,10 +321,6 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }
 
-  /* If multi-conn is not supported, force connections to 1. */
-  if (! src->ops->can_multi_conn (src) || !
dst->ops->can_multi_conn (dst))
-    connections = 1;
-
   /* Calculate the number of threads from the number of connections. */
   if (threads == 0) {
     long t;
@@ -379,15 +375,26 @@ main (int argc, char *argv[])
              synchronous ? "true" : "false");
   }
 
-  /* If multi-conn is enabled on either side, then at this point we
+  /* If using multiple connections, then at this point we
    * need to ask the backend to open the extra connections.
+   *
+   * Using multiple connections when the server does not report
+   * can_multi_conn is not safe in general, but is safe for the special
+   * access pattern in nbdcopy.
+   *
+   * - We split the image to 128m segments, and every segment is written
+   *   by single worker.
+   *
+   * - Writes do not overlap, and are never partial block that another
+   *   worker may modify at the same time.
+   *
+   * - We never read from the destination so we don't have caching
+   *   synchronization issue between clients.
    */
   if (connections > 1) {
     assert (threads == connections);
-    if (src->ops->can_multi_conn (src))
-      src->ops->start_multi_conn (src);
-    if (dst->ops->can_multi_conn (dst))
-      dst->ops->start_multi_conn (dst);
+    src->ops->start_multi_conn (src);
+    dst->ops->start_multi_conn (dst);
   }
 
   /* If the source is NBD and we couldn't negotiate meta
-- 
2.26.3

Pushed it, thanks.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Actually no, I had to revert this.

https://gitlab.com/nbdkit/libnbd/-/commit/fb42056929e9177e0de2603e2f430a8111ae7a5f

I added a test:

https://gitlab.com/nbdkit/libnbd/-/commit/23b41ad0a2a6d6582a8e643e42960d2e3c72e53e

However the test deadlocks when we try to open multiple connections to
qemu-nbd (with your patch).  As best I can tell this is because of
qemu's file locking.  qemu-nbd doesn't seem to support the -U option
found in qemu-img, but even if it did I don't think it would be a good
idea to tell users that they have to use it.

I think this needs to be fixed in qemu-nbd itself.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org