[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@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/fb42056929e9177e0de2603e2f430a8111... I added a test: https://gitlab.com/nbdkit/libnbd/-/commit/23b41ad0a2a6d6582a8e643e42960d2e3c... 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
On Thu, May 27, 2021 at 1:49 PM Richard W.M. Jones <rjones@redhat.com> wrote:
Actually no, I had to revert this.
https://gitlab.com/nbdkit/libnbd/-/commit/fb42056929e9177e0de2603e2f430a8111...
I added a test:
https://gitlab.com/nbdkit/libnbd/-/commit/23b41ad0a2a6d6582a8e643e42960d2e3c...
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.
When using qemu-nbd you need to specify --shared=4 on both source and destination. Current nbdcopy depends on the server supporting multiple connections, so we try to open the additional connections in the main thread. If the server supports only one connection (--shared=4 not specified), this will deadlock. One way to fix this is to open the extra connection in the worker threads. If the server does not support more then one connection, only one worker can make progress, and the other will just block on nbd_connect(). When the first worker finishes all the work, the rest of the workers will connect and find that they have no work to do and exit.
I think this needs to be fixed in qemu-nbd itself.
I don't think we can change qemu-nbd behavior at this point. Nir
On Thu, May 27, 2021 at 02:13:24PM +0300, Nir Soffer wrote:
On Thu, May 27, 2021 at 1:49 PM Richard W.M. Jones <rjones@redhat.com> wrote:
Actually no, I had to revert this.
https://gitlab.com/nbdkit/libnbd/-/commit/fb42056929e9177e0de2603e2f430a8111...
I added a test:
https://gitlab.com/nbdkit/libnbd/-/commit/23b41ad0a2a6d6582a8e643e42960d2e3c...
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.
When using qemu-nbd you need to specify --shared=4 on both source and destination.
Ah, I didn't realise that. So my explanation for why it failed was wrong. It has to be said qemu-nbd's defaults are a pain, especially having to always specify the -t option.
Current nbdcopy depends on the server supporting multiple connections, so we try to open the additional connections in the main thread. If the server supports only one connection (--shared=4 not specified), this will deadlock.
One way to fix this is to open the extra connection in the worker threads. If the server does not support more then one connection, only one worker can make progress, and the other will just block on nbd_connect(). When the first worker finishes all the work, the rest of the workers will connect and find that they have no work to do and exit.
Agreed.
I think this needs to be fixed in qemu-nbd itself.
I don't think we can change qemu-nbd behavior at this point.
IIUC the reason why qemu-nbd doesn't support multi-conn is because of the generality of the qemu block layer. If qemu-nbd is backed by -- for example -- a Ceph device then we'd need to check if Ceph was safe for multi-conn. However I wonder if it's possible to advertise multi-conn in some limited scenarios. Could qemu-nbd check if the backing block device is a plain raw file (or other limited cases) and offer multi-conn then? 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
participants (3)
-
Nir Soffer -
Nir Soffer -
Richard W.M. Jones