riscv/qemu - qemu - Gitea: Git with a cup of tea

Commit Graph

Author	SHA1	Message	Date
Markus Armbruster	19552884e4	error: Consistently name Error * objects err, and not errp This touches code in xen_enable_tpm() that is obviously wrong. Since I don't know how to fix it properly, I'm adding a FIXME there. Signed-off-by: Markus Armbruster <armbru@redhat.com> Message-ID: <20251119130855.105479-6-armbru@redhat.com> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>	4 months ago
Markus Armbruster	cfd5d4eafb	nbd/client-connection: Replace error_propagate() by assignment connect_thread_func() sets a variable to null, then error_propagate()s an Error * to it. This is a roundabout way to assign the Error * to it, so replace it by just that. Signed-off-by: Markus Armbruster <armbru@redhat.com> Message-ID: <20251119130855.105479-4-armbru@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Peter Xu <peterx@redhat.com> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>	4 months ago
Vladimir Sementsov-Ogievskiy	1ed8903916	treewide: handle result of qio_channel_set_blocking() Currently, we just always pass NULL as errp argument. That doesn't look good. Some realizations of interface may actually report errors. Channel-socket realization actually either ignore or crash on errors, but we are going to straighten it out to always reporting an errp in further commits. So, convert all callers to either handle the error (where environment allows) or explicitly use &error_abort. Take also a chance to change the return value to more convenient bool (keeping also in mind, that underlying realizations may return -1 on failure, not -errno). Suggested-by: Daniel P. Berrangé <berrange@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> [DB: fix return type mismatch in TLS/websocket channel impls for qio_channel_set_blocking] Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>	7 months ago
Nir Soffer	479ec81061	nbd: Set unix socket send buffer on Linux Like macOS we have similar issue on Linux. For TCP socket the send buffer size is 2626560 bytes (~2.5 MiB) and we get good performance. However for unix socket the default and maximum buffer size is 212992 bytes (208 KiB) and we see poor performance when using one NBD connection, up to 4 times slower than macOS on the same machine. Tracing shows that for every 2 MiB payload (qemu uses 2 MiB io size), we do 1 recvmsg call with TCP socket, and 10 recvmsg calls with unix socket. Fixing this issue requires changing the maximum send buffer size (the receive buffer size is ignored). This can be done using: $ cat /etc/sysctl.d/net-mem-max.conf net.core.wmem_max = 2097152 $ sudo sysctl -p /etc/sysctl.d/net-mem-max.conf With this we can set the socket buffer size to 2 MiB. With the defaults the value requested by qemu is clipped to the maximum size and has no effect. I tested on 2 machines: - Fedora 42 VM on MacBook Pro M2 Max - Dell PowerEdge R640 (Intel(R) Xeon(R) Gold 6230 CPU @ 2.10GHz) On the older Dell machine we see very little improvement, up to 1.03 higher throughput. On the M2 machine we see up to 2.67 times higher throughput. The following results are from the M2 machine. Reading from qemu-nbd with qemu-img convert. In this test buffer size of 4m is optimal (2.28 times faster). \| buffer size \| time \| user \| system \| \|-------------\|---------\|---------\|---------\| \| default \| 4.292 \| 0.243 \| 1.604 \| \| 524288 \| 2.167 \| 0.058 \| 1.288 \| \| 1048576 \| 2.041 \| 0.060 \| 1.238 \| \| 2097152 \| 1.884 \| 0.060 \| 1.191 \| \| 4194304 \| 1.881 \| 0.054 \| 1.196 \| Writing to qemu-nbd with qemu-img convert. In this test buffer size of 1m is optimal (2.67 times faster). \| buffer size \| time \| user \| system \| \|-------------\|---------\|---------\|---------\| \| default \| 3.113 \| 0.334 \| 1.094 \| \| 524288 \| 1.173 \| 0.179 \| 0.654 \| \| 1048576 \| 1.164 \| 0.164 \| 0.670 \| \| 2097152 \| 1.227 \| 0.197 \| 0.663 \| \| 4194304 \| 1.227 \| 0.198 \| 0.666 \| Computing a blkhash with nbdcopy. In this test buffer size of 512k is optimal (1.19 times faster). \| buffer size \| time \| user \| system \| \|-------------\|---------\|---------\|---------\| \| default \| 2.140 \| 4.483 \| 2.681 \| \| 524288 \| 1.794 \| 4.467 \| 2.572 \| \| 1048576 \| 1.807 \| 4.447 \| 2.644 \| \| 2097152 \| 1.822 \| 4.461 \| 2.698 \| \| 4194304 \| 1.827 \| 4.465 \| 2.700 \| Computing a blkhash with blksum. In this test buffer size of 4m is optimal (2.65 times faster). \| buffer size \| time \| user \| system \| \|-------------\|---------\|---------\|---------\| \| default \| 3.582 \| 4.595 \| 2.392 \| \| 524288 \| 1.499 \| 4.384 \| 1.482 \| \| 1048576 \| 1.377 \| 4.381 \| 1.345 \| \| 2097152 \| 1.388 \| 4.389 \| 1.354 \| \| 4194304 \| 1.352 \| 4.395 \| 1.302 \| Signed-off-by: Nir Soffer <nirsof@gmail.com> Message-ID: <20250517201154.88456-4-nirsof@gmail.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com>	11 months ago
Nir Soffer	e9f4550b74	nbd: Set unix socket send buffer on macOS On macOS we need to increase unix socket buffers size on the client and server to get good performance. We set socket buffers on macOS after connecting or accepting a client connection. Testing shows that setting socket receive buffer size (SO_RCVBUF) has no effect on performance, so we set only the send buffer size (SO_SNDBUF). It seems to work like Linux but not documented. Testing shows that optimal buffer size is 512k to 4 MiB, depending on the test case. The difference is very small, so I chose 2 MiB. I tested reading from qemu-nbd and writing to qemu-nbd with qemu-img and computing a blkhash with nbdcopy and blksum. To focus on NBD communication and get less noisy results, I tested reading and writing to null-co driver. I added a read-pattern option to the null-co driver to return data full of 0xff: NULL="json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '10g', 'read-pattern': 255}}" For testing buffer size I added an environment variable for setting the socket buffer size. Read from qemu-nbd via qemu-img convert. In this test buffer size of 2m is optimal (12.6 times faster). qemu-nbd -r -t -e 0 -f raw -k /tmp/nbd.sock "$NULL" & qemu-img convert -f raw -O raw -W -n "nbd+unix:///?socket=/tmp/nbd.sock" "$NULL" \| buffer size \| time \| user \| system \| \|-------------\|---------\|---------\|---------\| \| default \| 13.361 \| 2.653 \| 5.702 \| \| 65536 \| 2.283 \| 0.204 \| 1.318 \| \| 131072 \| 1.673 \| 0.062 \| 1.008 \| \| 262144 \| 1.592 \| 0.053 \| 0.952 \| \| 524288 \| 1.496 \| 0.049 \| 0.887 \| \| 1048576 \| 1.234 \| 0.047 \| 0.738 \| \| 2097152 \| 1.060 \| 0.080 \| 0.602 \| \| 4194304 \| 1.061 \| 0.076 \| 0.604 \| Write to qemu-nbd with qemu-img convert. In this test buffer size of 2m is optimal (9.2 times faster). qemu-nbd -t -e 0 -f raw -k /tmp/nbd.sock "$NULL" & qemu-img convert -f raw -O raw -W -n "$NULL" "nbd+unix:///?socket=/tmp/nbd.sock" \| buffer size \| time \| user \| system \| \|-------------\|---------\|---------\|---------\| \| default \| 8.063 \| 2.522 \| 4.184 \| \| 65536 \| 1.472 \| 0.430 \| 0.867 \| \| 131072 \| 1.071 \| 0.297 \| 0.654 \| \| 262144 \| 1.012 \| 0.239 \| 0.587 \| \| 524288 \| 0.970 \| 0.201 \| 0.514 \| \| 1048576 \| 0.895 \| 0.184 \| 0.454 \| \| 2097152 \| 0.877 \| 0.174 \| 0.440 \| \| 4194304 \| 0.944 \| 0.231 \| 0.535 \| Compute a blkhash with nbdcopy, using 4 NBD connections and 256k request size. In this test buffer size of 4m is optimal (5.1 times faster). qemu-nbd -r -t -e 0 -f raw -k /tmp/nbd.sock "$NULL" & nbdcopy --blkhash "nbd+unix:///?socket=/tmp/nbd.sock" null: \| buffer size \| time \| user \| system \| \|-------------\|---------\|---------\|---------\| \| default \| 8.624 \| 5.727 \| 6.507 \| \| 65536 \| 2.563 \| 4.760 \| 2.498 \| \| 131072 \| 1.903 \| 4.559 \| 2.093 \| \| 262144 \| 1.759 \| 4.513 \| 1.935 \| \| 524288 \| 1.729 \| 4.489 \| 1.924 \| \| 1048576 \| 1.696 \| 4.479 \| 1.884 \| \| 2097152 \| 1.710 \| 4.480 \| 1.763 \| \| 4194304 \| 1.687 \| 4.479 \| 1.712 \| Compute a blkhash with blksum, using 1 NBD connection and 256k read size. In this test buffer size of 512k is optimal (10.3 times faster). qemu-nbd -r -t -e 0 -f raw -k /tmp/nbd.sock "$NULL" & blksum "nbd+unix:///?socket=/tmp/nbd.sock" \| buffer size \| time \| user \| system \| \|-------------\|---------\|---------\|---------\| \| default \| 13.085 \| 5.664 \| 6.461 \| \| 65536 \| 3.299 \| 5.106 \| 2.515 \| \| 131072 \| 2.396 \| 4.989 \| 2.069 \| \| 262144 \| 1.607 \| 4.724 \| 1.555 \| \| 524288 \| 1.271 \| 4.528 \| 1.224 \| \| 1048576 \| 1.294 \| 4.565 \| 1.333 \| \| 2097152 \| 1.299 \| 4.569 \| 1.344 \| \| 4194304 \| 1.291 \| 4.559 \| 1.327 \| Signed-off-by: Nir Soffer <nirsof@gmail.com> Message-ID: <20250517201154.88456-3-nirsof@gmail.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com>	11 months ago
Kevin Wolf	2e73a17c68	nbd/server: Support inactive nodes In order to support running an NBD export on inactive nodes, we must make sure to return errors for any operations that aren't allowed on inactive nodes. Reads are the only operation we know we need for inactive images, so to err on the side of caution, return errors for everything else, even if some operations could possibly be okay. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Acked-by: Fabiano Rosas <farosas@suse.de> Message-ID: <20250204211407.381505-14-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>	1 year ago
Philippe Mathieu-Daudé	32cad1ffb8	include: Rename sysemu/ -> system/ Headers in include/sysemu/ are not only related to system emulation, they are also used by virtualization. Rename as system/ which is clearer. Files renamed manually then mechanical change using sed tool. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Tested-by: Lei Yang <leiyang@redhat.com> Message-Id: <20241203172445.28576-1-philmd@linaro.org>	1 year ago
Eric Blake	efd3dda312	nbd-server: Silence server warnings on port probes While testing the use of qemu-nbd in a Pod of a Kubernetes cluster, I got LOTS of log messages of the forms: qemu-nbd: option negotiation failed: Failed to read flags: Unexpected end-of-file before all data were read qemu-nbd: option negotiation failed: Failed to read flags: Unable to read from socket: Connection reset by peer While it is nice to warn about clients that aren't following protocol (in case it helps diagnosing bugs in those clients), a mere port probe (where the client never write()s any bytes, and where we might even hit EPIPE in trying to send our greeting to the client) is NOT abnormal, but merely serves to pollute the log. And Kubernetes _really_ likes to do port probes to determine whether a given Pod is up and running. Easy ways to demonstrate the above port probes: $ qemu-nbd -r -f raw path/to/file & $ nc localhost 10809 </dev/null $ bash -c 'exec </dev/tcp/localhost/10809' $ kill $! Silence the noise by not capturing errors until after our first successful read() from a client. Signed-off-by: Eric Blake <eblake@redhat.com> Message-ID: <20241115195638.1132007-2-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>	1 year ago
Marc-André Lureau	73ce9bbf8a	nbd: fix -Werror=maybe-uninitialized false-positive ../nbd/client-connection.c:419:8: error: ‘wait_co’ may be used uninitialized [-Werror=maybe-uninitialized] Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com>	2 years ago
Eric Blake	b9b72cb3ce	nbd/server: CVE-2024-7409: Drop non-negotiating clients A client that opens a socket but does not negotiate is merely hogging qemu's resources (an open fd and a small amount of memory); and a malicious client that can access the port where NBD is listening can attempt a denial of service attack by intentionally opening and abandoning lots of unfinished connections. The previous patch put a default bound on the number of such ongoing connections, but once that limit is hit, no more clients can connect (including legitimate ones). The solution is to insist that clients complete handshake within a reasonable time limit, defaulting to 10 seconds. A client that has not successfully completed NBD_OPT_GO by then (including the case of where the client didn't know TLS credentials to even reach the point of NBD_OPT_GO) is wasting our time and does not deserve to stay connected. Later patches will allow fine-tuning the limit away from the default value (including disabling it for doing integration testing of the handshake process itself). Note that this patch in isolation actually makes it more likely to see qemu SEGV after nbd-server-stop, as any client socket still connected when the server shuts down will now be closed after 10 seconds rather than at the client's whims. That will be addressed in the next patch. For a demo of this patch in action: $ qemu-nbd -f raw -r -t -e 10 file & $ nbdsh --opt-mode -c ' H = list() for i in range(20): print(i) H.insert(i, nbd.NBD()) H[i].set_opt_mode(True) H[i].connect_uri("nbd://localhost") ' $ kill $! where later connections get to start progressing once earlier ones are forcefully dropped for taking too long, rather than hanging. Suggested-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> Message-ID: <20240807174943.771624-13-eblake@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> [eblake: rebase to changes earlier in series, reduce scope of timer] Signed-off-by: Eric Blake <eblake@redhat.com>	2 years ago
Eric Blake	fb1c2aaa98	nbd/server: Plumb in new args to nbd_client_add() Upcoming patches to fix a CVE need to track an opaque pointer passed in by the owner of a client object, as well as request for a time limit on how fast negotiation must complete. Prepare for that by changing the signature of nbd_client_new() and adding an accessor to get at the opaque pointer, although for now the two servers (qemu-nbd.c and blockdev-nbd.c) do not change behavior even though they pass in a new default timeout value. Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Signed-off-by: Eric Blake <eblake@redhat.com> Message-ID: <20240807174943.771624-11-eblake@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> [eblake: s/LIMIT/MAX_SECS/ as suggested by Dan] Signed-off-by: Eric Blake <eblake@redhat.com>	2 years ago
Eric Blake	c719573d71	nbd: Minor style and typo fixes Touch up a comment with the wrong type name, and an over-long line, both noticed while working on upcoming patches. Signed-off-by: Eric Blake <eblake@redhat.com> Message-ID: <20240807174943.771624-10-eblake@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>	2 years ago
Eric Blake	4fa333e08d	nbd/server: Mark negotiation functions as coroutine_fn nbd_negotiate() is already marked coroutine_fn. And given the fix in the previous patch to have nbd_negotiate_handle_starttls not create and wait on a g_main_loop (as that would violate coroutine constraints), it is worth marking the rest of the related static functions reachable only during option negotiation as also being coroutine_fn. Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Signed-off-by: Eric Blake <eblake@redhat.com> Message-ID: <20240408160214.1200629-6-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> [eblake: drop one spurious coroutine_fn marking] Signed-off-by: Eric Blake <eblake@redhat.com>	2 years ago
Zhu Yangyang	ae6d91a7e9	nbd/server: do not poll within a coroutine context Coroutines are not supposed to block. Instead, they should yield. The client performs TLS upgrade outside of an AIOContext, during synchronous handshake; this still requires g_main_loop. But the server responds to TLS upgrade inside a coroutine, so a nested g_main_loop is wrong. Since the two callbacks no longer share more than the setting of data.complete and data.error, it's just as easy to use static helpers instead of trying to share a common code path. It is also possible to add assertions that no other code is interfering with the eventual path to qio reaching the callback, whether or not it required a yield or main loop. Fixes: `f95910f` ("nbd: implement TLS support in the protocol negotiation") Signed-off-by: Zhu Yangyang <zhuyangyang14@huawei.com> [eblake: move callbacks to their use point, add assertions] Signed-off-by: Eric Blake <eblake@redhat.com> Message-ID: <20240408160214.1200629-5-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>	2 years ago
Kevin Wolf	9c707525cb	nbd/server: Fix race in draining the export When draining an NBD export, nbd_drained_begin() first sets client->quiescing so that nbd_client_receive_next_request() won't start any new request coroutines. Then nbd_drained_poll() tries to makes sure that we wait for any existing request coroutines by checking that client->nb_requests has become 0. However, there is a small window between creating a new request coroutine and increasing client->nb_requests. If a coroutine is in this state, it won't be waited for and drain returns too early. In the context of switching to a different AioContext, this means that blk_aio_attached() will see client->recv_coroutine != NULL and fail its assertion. Fix this by increasing client->nb_requests immediately when starting the coroutine. Doing this after the checks if we should create a new coroutine is okay because client->lock is held. Cc: qemu-stable@nongnu.org Fixes: `fd6afc501a` ("nbd/server: Use drained block ops to quiesce the server") Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20240314165825.40261-2-kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>	2 years ago
Stefan Hajnoczi	7075d23511	nbd/server: introduce NBDClient->lock to protect fields NBDClient has a number of fields that are accessed by both the export AioContext and the main loop thread. When the AioContext lock is removed these fields will need another form of protection. Add NBDClient->lock and protect fields that are accessed by both threads. Also add assertions where possible and otherwise add doc comments stating assumptions about which thread and lock holding. Note this patch moves the client->recv_coroutine assertion from nbd_co_receive_request() to nbd_trip() where client->lock is held. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-ID: <20231221192452.1785567-7-stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>	2 years ago
Stefan Hajnoczi	f816310d0c	nbd/server: only traverse NBDExport->clients from main loop thread The NBD clients list is currently accessed from both the export AioContext and the main loop thread. When the AioContext lock is removed there will be nothing protecting the clients list. Adding a lock around the clients list is tricky because NBDClient structs are refcounted and may be freed from the export AioContext or the main loop thread. nbd_export_request_shutdown() -> client_close() -> nbd_client_put() is also tricky because the list lock would be held while indirectly dropping references to NDBClients. A simpler approach is to only allow nbd_client_put() and client_close() calls from the main loop thread. Then the NBD clients list is only accessed from the main loop thread and no fancy locking is needed. nbd_trip() just needs to reschedule itself in the main loop AioContext before calling nbd_client_put() and client_close(). This costs more CPU cycles per NBD request so add nbd_client_put_nonzero() to optimize the common case where more references to NBDClient remain. Note that nbd_client_get() can still be called from either thread, so make NBDClient->refcount atomic. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Message-ID: <20231221192452.1785567-6-stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>	2 years ago
Stefan Hajnoczi	efade66d58	nbd/server: avoid per-NBDRequest nbd_client_get/put() nbd_trip() processes a single NBD request from start to finish and holds an NBDClient reference throughout. NBDRequest does not outlive the scope of nbd_trip(). Therefore it is unnecessary to ref/unref NBDClient for each NBDRequest. Removing these nbd_client_get()/nbd_client_put() calls will make thread-safety easier in the commits that follow. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Message-ID: <20231221192452.1785567-5-stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>	2 years ago
Kevin Wolf	372b69f503	block: Mark bdrv_filter_or_cow_bs() and callers GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_filter_or_cow_bs() need to hold a reader lock for the graph because it calls bdrv_filter_or_cow_child(), which accesses bs->file/backing. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-ID: <20231027155333.420094-7-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>	2 years ago
Eric Blake	2dcbb11b39	nbd/server: Add FLAG_PAYLOAD support to CMD_BLOCK_STATUS Allow a client to request a subset of negotiated meta contexts. For example, a client may ask to use a single connection to learn about both block status and dirty bitmaps, but where the dirty bitmap queries only need to be performed on a subset of the disk; forcing the server to compute that information on block status queries in the rest of the disk is wasted effort (both at the server, and on the amount of traffic sent over the wire to be parsed and ignored by the client). Qemu as an NBD client never requests to use more than one meta context, so it has no need to use block status payloads. Testing this instead requires support from libnbd, which CAN access multiple meta contexts in parallel from a single NBD connection; an interop test submitted to the libnbd project at the same time as this patch demonstrates the feature working, as well as testing some corner cases (for example, when the payload length is longer than the export length), although other corner cases (like passing the same id duplicated) requires a protocol fuzzer because libnbd is not wired up to break the protocol that badly. This also includes tweaks to 'qemu-nbd --list' to show when a server is advertising the capability, and to the testsuite to reflect the addition to that output. Of note: qemu will always advertise the new feature bit during NBD_OPT_INFO if extended headers have alreay been negotiated (regardless of whether any NBD_OPT_SET_META_CONTEXT negotiation has occurred); but for NBD_OPT_GO, qemu only advertises the feature if block status is also enabled (that is, if the client does not negotiate any contexts, then NBD_CMD_BLOCK_STATUS cannot be used, so the feature is not advertised). Signed-off-by: Eric Blake <eblake@redhat.com> Message-ID: <20230925192229.3186470-26-eblake@redhat.com> [eblake: fix logic to reject unnegotiated contexts] Signed-off-by: Eric Blake <eblake@redhat.com>	3 years ago
Eric Blake	1dec4643d1	nbd/server: Prepare for per-request filtering of BLOCK_STATUS The next commit will add support for the optional extension NBD_CMD_FLAG_PAYLOAD during NBD_CMD_BLOCK_STATUS, where the client can request that the server only return a subset of negotiated contexts, rather than all contexts. To make that task easier, this patch populates the list of contexts to return on a per-command basis (for now, identical to the full set of negotiated contexts). Signed-off-by: Eric Blake <eblake@redhat.com> Message-ID: <20230925192229.3186470-25-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>	3 years ago
Eric Blake	fd358d8390	nbd/server: Refactor list of negotiated meta contexts Peform several minor refactorings of how the list of negotiated meta contexts is managed, to make upcoming patches easier: Promote the internal type NBDExportMetaContexts to the public opaque type NBDMetaContexts, and mark exp const. Use a shorter member name in NBDClient. Hoist calls to nbd_check_meta_context() earlier in their callers, as the number of negotiated contexts may impact the flags exposed in regards to an export, which in turn requires a new parameter. Drop a redundant parameter to nbd_negotiate_meta_queries. No semantic change intended on the success path; on the failure path, dropping context in nbd_check_meta_export even when reporting an error is safer. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-ID: <20230925192229.3186470-24-eblake@redhat.com>	3 years ago
Eric Blake	56cf9d0471	nbd/client: Request extended headers during negotiation All the pieces are in place for a client to finally request extended headers. Note that we must not request extended headers when qemu-nbd is used to connect to the kernel module (as nbd.ko does not expect them, but expects us to do the negotiation in userspace before handing the socket over to the kernel), but there is no harm in all other clients requesting them. Extended headers are not essential to the information collected during 'qemu-nbd --list', but probing for it gives us one more piece of information in that output. Update the iotests affected by the new line of output. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-ID: <20230925192229.3186470-23-eblake@redhat.com>	3 years ago
Eric Blake	4fc55bf3b0	nbd/client: Initial support for extended headers Update the client code to be able to send an extended request, and parse an extended header from the server. Note that since we reject any structured reply with a too-large payload, we can always normalize a valid header back into the compact form, so that the caller need not deal with two branches of a union. Still, until a later patch lets the client negotiate extended headers, the code added here should not be reached. Note that because of the different magic numbers, it is just as easy to trace and then tolerate a non-compliant server sending the wrong header reply as it would be to insist that the server is compliant. Signed-off-by: Eric Blake <eblake@redhat.com> Message-ID: <20230925192229.3186470-21-eblake@redhat.com> [eblake: fix trace format] Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>	3 years ago
Eric Blake	9c1d261437	nbd/server: Enable initial support for extended headers Time to start supporting clients that request extended headers. Now we can finally reach the code added across several previous patches. Even though the NBD spec has been altered to allow us to accept NBD_CMD_READ larger than the max payload size (provided our response is a hole or broken up over more than one data chunk), we are not planning to take advantage of that, and continue to cap NBD_CMD_READ to 32M regardless of header size. For NBD_CMD_WRITE_ZEROES and NBD_CMD_TRIM, the block layer already supports 64-bit operations without any effort on our part. For NBD_CMD_BLOCK_STATUS, the client's length is a hint, and the previous patch took care of implementing the required NBD_REPLY_TYPE_BLOCK_STATUS_EXT. We do not yet support clients that want to do request payload filtering of NBD_CMD_BLOCK_STATUS; that will be added in later patches, but is not essential for qemu as a client since qemu only requests the single context base:allocation. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-ID: <20230925192229.3186470-19-eblake@redhat.com>	3 years ago
Eric Blake	bcc16cc19e	nbd/server: Support 64-bit block status The NBD spec states that if the client negotiates extended headers, the server must avoid NBD_REPLY_TYPE_BLOCK_STATUS and instead use NBD_REPLY_TYPE_BLOCK_STATUS_EXT which supports 64-bit lengths, even if the reply does not need more than 32 bits. As of this patch, client->mode is still never NBD_MODE_EXTENDED, so the code added here does not take effect until the next patch enables negotiation. For now, all metacontexts that we know how to export never populate more than 32 bits of information, so we don't have to worry about NBD_REP_ERR_EXT_HEADER_REQD or filtering during handshake, and we always send all zeroes for the upper 32 bits of status during NBD_CMD_BLOCK_STATUS. Note that we previously had some interesting size-juggling on call chains, such as: nbd_co_send_block_status(uint32_t length) -> blockstatus_to_extents(uint32_t bytes) -> bdrv_block_status_above(bytes, &uint64_t num) -> nbd_extent_array_add(uint64_t num) -> store num in 32-bit length But we were lucky that it never overflowed: bdrv_block_status_above never sets num larger than bytes, and we had previously been capping 'bytes' at 32 bits (since the protocol does not allow sending a larger request without extended headers). This patch adds some assertions that ensure we continue to avoid overflowing 32 bits for a narrow client, while fully utilizing 64-bits all the way through when the client understands that. Even in 64-bit math, overflow is not an issue, because all lengths are coming from the block layer, and we know that the block layer does not support images larger than off_t (if lengths were coming from the network, the story would be different). Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-ID: <20230925192229.3186470-18-eblake@redhat.com>	3 years ago
Eric Blake	11d3355fc8	nbd/server: Prepare to send extended header replies Although extended mode is not yet enabled, once we do turn it on, we need to reply with extended headers to all messages. Update the low level entry points necessary so that all other callers automatically get the right header based on the current mode. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-ID: <20230925192229.3186470-17-eblake@redhat.com>	3 years ago
Eric Blake	c8720ca03e	nbd/server: Prepare to receive extended header requests Although extended mode is not yet enabled, once we do turn it on, we need to accept extended requests for all messages. Previous patches have already taken care of supporting 64-bit lengths, now we just need to read it off the wire. Note that this implementation will block indefinitely on a buggy client that sends a non-extended payload (that is, we try to read a full packet before we ever check the magic number, but a client that mistakenly sends a simple request after negotiating extended headers doesn't send us enough bytes), but it's no different from any other client that stops talking to us partway through a packet and thus not worth coding around. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-ID: <20230925192229.3186470-16-eblake@redhat.com>	3 years ago
Eric Blake	009cd86650	nbd/server: Support a request payload Upcoming additions to support NBD 64-bit effect lengths allow for the possibility to distinguish between payload length (capped at 32M) and effect length (64 bits, although we generally assume 63 bits because of off_t limitations). Without that extension, only the NBD_CMD_WRITE request has a payload; but with the extension, it makes sense to allow at least NBD_CMD_BLOCK_STATUS to have both a payload and effect length in a future patch (where the payload is a limited-size struct that in turn gives the real effect length as well as a subset of known ids for which status is requested). Other future NBD commands may also have a request payload, so the 64-bit extension introduces a new NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header length is a payload length or an effect length, rather than hard-coding the decision based on the command. According to the spec, a client should never send a command with a payload without the negotiation phase proving such extension is available. So in the unlikely event the bit is set or cleared incorrectly, the client is already at fault; if the client then provides the payload, we can gracefully consume it off the wire and fail the command with NBD_EINVAL (subsequent checks for magic numbers ensure we are still in sync), while if the client fails to send payload we block waiting for it (basically deadlocking our connection to the bad client, but not negatively impacting our ability to service other clients, so not a security risk). Note that we do not support the payload version of BLOCK_STATUS yet. This patch also fixes a latent bug introduced in b2578459: once request->len can be 64 bits, assigning it to a 32-bit payload_len can cause wraparound to 0 which then sets req->complete prematurely; thankfully, the bug was not possible back then (it takes this and later patches to even allow request->len larger than 32 bits; and since previously the only 'payload_len = request->len' assignment was in NBD_CMD_WRITE which also sets check_length, which in turn rejects lengths larger than 32M before relying on any possibly-truncated value stored in payload_len). Signed-off-by: Eric Blake <eblake@redhat.com> Message-ID: <20230925192229.3186470-15-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> [eblake: enhance comment on handling client error, fix type bug] Signed-off-by: Eric Blake <eblake@redhat.com>	3 years ago
Eric Blake	8db7e2d657	nbd/server: Refactor handling of command sanity checks Upcoming additions to support NBD 64-bit effect lengths will add a new command flag NBD_CMD_FLAG_PAYLOAD_LEN that needs to be considered in our sanity checks of the client's messages (that is, more than just CMD_WRITE have the potential to carry a client payload when extended headers are in effect). But before we can start to support that, it is easier to first refactor the existing set of various if statements over open-coded combinations of request->type to instead be a single switch statement over all command types that sets witnesses, then straight-line processing based on the witnesses. No semantic change is intended. Signed-off-by: Eric Blake <eblake@redhat.com> Message-ID: <20230829175826.377251-24-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>	3 years ago
Eric Blake	b257845932	nbd: Prepare for 64-bit request effect lengths Widen the length field of NBDRequest to 64-bits, although we can assert that all current uses are still under 32 bits: either because of NBD_MAX_BUFFER_SIZE which is even smaller (and where size_t can still be appropriate, even on 32-bit platforms), or because nothing ever puts us into NBD_MODE_EXTENDED yet (and while future patches will allow larger transactions, the lengths in play here are still capped at 32-bit). There are no semantic changes, other than a typo fix in a couple of error messages. Signed-off-by: Eric Blake <eblake@redhat.com> Message-ID: <20230829175826.377251-23-eblake@redhat.com> [eblake: fix assertion bug in nbd_co_send_simple_reply] Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>	3 years ago
Eric Blake	d95ffb6fe6	nbd: Add types for extended headers Add the constants and structs necessary for later patches to start implementing the NBD_OPT_EXTENDED_HEADERS extension in both the client and server, matching recent upstream nbd.git (through commit e6f3b94a934). This patch does not change any existing behavior, but merely sets the stage for upcoming patches. This patch does not change the status quo that neither the client nor server use a packed-struct representation for the request header. While most of the patch adds new types, there is also some churn for renaming the existing NBDExtent to NBDExtent32 to contrast it with NBDExtent64, which I thought was a nicer name than NBDExtentExt. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-ID: <20230829175826.377251-22-eblake@redhat.com>	3 years ago
Eric Blake	297365b40f	nbd/client: Pass mode through to nbd_send_request Once the 64-bit headers extension is enabled, the data layout we send over the wire for a client request depends on the mode negotiated with the server. Rather than adding a parameter to nbd_send_request, we can add a member to struct NBDRequest, since it already does not reflect on-wire format. Some callers initialize it directly; many others rely on a common initialization point during nbd_co_send_request(). At this point, there is no semantic change. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-ID: <20230829175826.377251-21-eblake@redhat.com>	3 years ago
Eric Blake	ac132d0520	nbd: Replace bool structured_reply with mode enum The upcoming patches for 64-bit extensions requires various points in the protocol to make decisions based on what was negotiated. While we could easily add a 'bool extended_headers' alongside the existing 'bool structured_reply', this does not scale well if more modes are added in the future. Better is to expose the mode enum added in the recent commit `bfe04d0a7d` out to a wider use in the code base. Where the code previously checked for structured_reply being set or clear, it now prefers checking for an inequality; this works because the nodes are in a continuum of increasing abilities, and allows us to touch fewer places if we ever insert other modes in the middle of the enum. There should be no semantic change in this patch. Signed-off-by: Eric Blake <eblake@redhat.com> Message-ID: <20230829175826.377251-20-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>	3 years ago
Michael Tokarev	0a19d87995	misc/other: spelling fixes Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> Reviewed-by: Eric Blake <eblake@redhat.com>	3 years ago
Stefan Hajnoczi	06e0f098d6	io: follow coroutine AioContext in qio_channel_yield() The ongoing QEMU multi-queue block layer effort makes it possible for multiple threads to process I/O in parallel. The nbd block driver is not compatible with the multi-queue block layer yet because QIOChannel cannot be used easily from coroutines running in multiple threads. This series changes the QIOChannel API to make that possible. In the current API, calling qio_channel_attach_aio_context() sets the AioContext where qio_channel_yield() installs an fd handler prior to yielding: qio_channel_attach_aio_context(ioc, my_ctx); ... qio_channel_yield(ioc); // my_ctx is used here ... qio_channel_detach_aio_context(ioc); This API design has limitations: reading and writing must be done in the same AioContext and moving between AioContexts involves a cumbersome sequence of API calls that is not suitable for doing on a per-request basis. There is no fundamental reason why a QIOChannel needs to run within the same AioContext every time qio_channel_yield() is called. QIOChannel only uses the AioContext while inside qio_channel_yield(). The rest of the time, QIOChannel is independent of any AioContext. In the new API, qio_channel_yield() queries the AioContext from the current coroutine using qemu_coroutine_get_aio_context(). There is no need to explicitly attach/detach AioContexts anymore and qio_channel_attach_aio_context() and qio_channel_detach_aio_context() are gone. One coroutine can read from the QIOChannel while another coroutine writes from a different AioContext. This API change allows the nbd block driver to use QIOChannel from any thread. It's important to keep in mind that the block driver already synchronizes QIOChannel access and ensures that two coroutines never read simultaneously or write simultaneously. This patch updates all users of qio_channel_attach_aio_context() to the new API. Most conversions are simple, but vhost-user-server requires a new qemu_coroutine_yield() call to quiesce the vu_client_trip() coroutine when not attached to any AioContext. While the API is has become simpler, there is one wart: QIOChannel has a special case for the iohandler AioContext (used for handlers that must not run in nested event loops). I didn't find an elegant way preserve that behavior, so I added a new API called qio_channel_set_follow_coroutine_ctx(ioc, true\|false) for opting in to the new AioContext model. By default QIOChannel uses the iohandler AioHandler. Code that formerly called qio_channel_attach_aio_context() now calls qio_channel_set_follow_coroutine_ctx(ioc, true) once after the QIOChannel is created. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Acked-by: Daniel P. Berrangé <berrange@redhat.com> Message-ID: <20230830224802.493686-5-stefanha@redhat.com> [eblake: also fix migration/rdma.c] Signed-off-by: Eric Blake <eblake@redhat.com>	3 years ago
Stefan Hajnoczi	078c8adaa6	nbd: drop unused nbd_start_negotiate() aio_context argument aio_context is always NULL, so drop it. Suggested-by: Fabiano Rosas <farosas@suse.de> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Message-ID: <20230830224802.493686-3-stefanha@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com>	3 years ago
Stefan Hajnoczi	b84ca91ca2	nbd: drop unused nbd_receive_negotiate() aio_context argument aio_context is always NULL, so drop it. Suggested-by: Fabiano Rosas <farosas@suse.de> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Message-ID: <20230830224802.493686-2-stefanha@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com>	3 years ago
Eric Blake	bfe04d0a7d	nbd: Use enum for various negotiation modes Deciphering the hard-coded list of integer return values from nbd_start_negotiate() will only get more confusing when adding support for 64-bit extended headers. Better is to name things in an enum. Although the function in question is private to client.c, putting the enum in a public header and including an enum-to-string conversion will allow its use in more places in upcoming patches. The enum is intentionally laid out so that operators like <= can be used to group multiple modes with similar characteristics, and where the least powerful mode has value 0, even though this patch does not exploit that. No semantic change intended. Signed-off-by: Eric Blake <eblake@redhat.com> Message-ID: <20230608135653.2918540-9-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>	3 years ago
Eric Blake	70fa99f445	nbd/client: Add safety check on chunk payload length Our existing use of structured replies either reads into a qiov capped at 32M (NBD_CMD_READ) or caps allocation to 1000 bytes (see NBD_MAX_MALLOC_PAYLOAD in block/nbd.c). But the existing length checks are rather late; if we encounter a buggy (or malicious) server that sends a super-large payload length, we should drop the connection right then rather than assuming the layer on top will be careful. This becomes more important when we permit 64-bit lengths which are even more likely to have the potential for attempted denial of service abuse. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-ID: <20230608135653.2918540-8-eblake@redhat.com>	3 years ago
Eric Blake	22efd81104	nbd: s/handle/cookie/ to match NBD spec Externally, libnbd exposed the 64-bit opaque marker for each client NBD packet as the "cookie", because it was less confusing when contrasted with 'struct nbd_handle *' holding all libnbd state. It also avoids confusion between the noun 'handle' as a way to identify a packet and the verb 'handle' for reacting to things like signals. Upstream NBD changed their spec to favor the name "cookie" based on libnbd's recommendations[1], so we can do likewise. [1] https://github.com/NetworkBlockDevice/nbd/commit/ca4392eb2b Signed-off-by: Eric Blake <eblake@redhat.com> Message-ID: <20230608135653.2918540-6-eblake@redhat.com> [eblake: typo fix] Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>	3 years ago
Eric Blake	66d4f4fe2f	nbd/server: Refactor to pass full request around Part of NBD's 64-bit headers extension involves passing the client's requested offset back as part of the reply header (one reason it stated for this change: converting absolute offsets stored in NBD_REPLY_TYPE_OFFSET_DATA to relative offsets within the buffer is easier if the absolute offset of the buffer is also available). This is a refactoring patch to pass the full request around the reply stack, rather than just the handle, so that later patches can then access request->from when extended headers are active. Meanwhile, this patch enables us to now assert that simple replies are only attempted when appropriate, and otherwise has no semantic change. Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-ID: <20230608135653.2918540-5-eblake@redhat.com>	3 years ago
Eric Blake	a7c8ed36bf	nbd/server: Prepare for alternate-size headers Upstream NBD now documents[1] an extension that supports 64-bit effect lengths in requests. As part of that extension, the size of the reply headers will change in order to permit a 64-bit length in the reply for symmetry[2]. Additionally, where the reply header is currently 16 bytes for simple reply, and 20 bytes for structured reply; with the extension enabled, there will only be one extended reply header, of 32 bytes, with both structured and extended modes sending identical payloads for chunked replies. Since we are already wired up to use iovecs, it is easiest to allow for this change in header size by splitting each structured reply across multiple iovecs, one for the header (which will become wider in a future patch according to client negotiation), and the other(s) for the chunk payload, and removing the header from the payload struct definitions. Rename the affected functions with s/structured/chunk/ to make it obvious that the code will be reused in extended mode. Interestingly, the client side code never utilized the packed types, so only the server code needs to be updated. [1] https://github.com/NetworkBlockDevice/nbd/blob/extension-ext-header/doc/proto.md as of NBD commit e6f3b94a934 [2] Note that on the surface, this is because some future server might permit a 4G+ NBD_CMD_READ and need to reply with that much data in one transaction. But even though the extended reply length is widened to 64 bits, for now the NBD spec is clear that servers will not reply with more than a maximum payload bounded by the 32-bit NBD_INFO_BLOCK_SIZE field; allowing a client and server to mutually agree to transactions larger than 4G would require yet another extension. Signed-off-by: Eric Blake <eblake@redhat.com> Message-ID: <20230608135653.2918540-4-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>	3 years ago
Eric Blake	f47b6eab83	nbd/client: Use smarter assert Assigning strlen() to a uint32_t and then asserting that it isn't too large doesn't catch the case of an input string 4G in length. Thankfully, the incoming strings can never be that large: if the export name or query is reflecting a string the client got from the server, we already guarantee that we dropped the NBD connection if the server sent more than 32M in a single reply to our NBD_OPT_* request; if the export name is coming from qemu, nbd_receive_negotiate() asserted that strlen(info->name) <= NBD_MAX_STRING_SIZE; and similarly, a query string via x->dirty_bitmap coming from the user was bounds-checked in either qemu-nbd or by the limitations of QMP. Still, it doesn't hurt to be more explicit in how we write our assertions to not have to analyze whether inadvertent wraparound is possible. Fixes: `93676c88` ("nbd: Don't send oversize strings", v4.2.0) Reported-by: Dr. David Alan Gilbert <dave@treblig.org> Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Message-ID: <20230608135653.2918540-2-eblake@redhat.com>	3 years ago
Philippe Mathieu-Daudé	7d5b0d6864	bulk: Remove pointless QOM casts Mechanical change running Coccinelle spatch with content generated from the qom-cast-macro-clean-cocci-gen.py added in the previous commit. Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Message-Id: <20230601093452.38972-3-philmd@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Thomas Huth <thuth@redhat.com>	3 years ago
Kevin Wolf	7c1f51bf38	nbd/server: Fix drained_poll to wake coroutine in right AioContext nbd_drained_poll() generally runs in the main thread, not whatever iothread the NBD server coroutine is meant to run in, so it can't directly reenter the coroutines to wake them up. The code seems to have the right intention, it specifies the correct AioContext when it calls qemu_aio_coroutine_enter(). However, this functions doesn't schedule the coroutine to run in that AioContext, but it assumes it is already called in the home thread of the AioContext. To fix this, add a new thread-safe qio_channel_wake_read() that can be called in the main thread to wake up the coroutine in its AioContext, and use this in nbd_drained_poll(). Cc: qemu-stable@nongnu.org Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20230517152834.277483-3-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>	3 years ago
Paolo Bonzini	d2223cddce	nbd: mark more coroutine_fns, do not use co_wrappers Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>	3 years ago
Paolo Bonzini	dd5b6780f7	nbd: a BlockExport always has a BlockBackend exp->common.blk cannot be NULL, nbd_export_delete() is only called (through a bottom half) from blk_exp_unref() and in turn that can only happen after blk_exp_add() has asserted exp->blk != NULL. Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>	3 years ago
Eric Blake	f1426881a8	nbd/server: Request TCP_NODELAY Nagle's algorithm adds latency in order to reduce network packet overhead on small packets. But when we are already using corking to merge smaller packets into transactional requests, the extra delay from TCP defaults just gets in the way (see recent commit `bd2cd4a4`). For reference, qemu as an NBD client already requests TCP_NODELAY (see nbd_connect() in nbd/client-connection.c); as does libnbd as a client [1], and nbdkit as a server [2]. Furthermore, the NBD spec recommends the use of TCP_NODELAY [3]. [1] https://gitlab.com/nbdkit/libnbd/-/blob/a48a1142/generator/states-connect.c#L39 [2] https://gitlab.com/nbdkit/nbdkit/-/blob/45b72f5b/server/sockets.c#L430 [3] https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md#protocol-phases CC: Florian Westphal <fw@strlen.de> Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20230404004047.142086-1-eblake@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>	3 years ago
Florian Westphal	bd2cd4a441	nbd/server: push pending frames after sending reply qemu-nbd doesn't set TCP_NODELAY on the tcp socket. Kernel waits for more data and avoids transmission of small packets. Without TLS this is barely noticeable, but with TLS this really shows. Booting a VM via qemu-nbd on localhost (with tls) takes more than 2 minutes on my system. tcpdump shows frequent wait periods, where no packets get sent for a 40ms period. Add explicit (un)corking when processing (and responding to) requests. "TCP_CORK, &zero" after earlier "CORK, &one" will flush pending data. VM Boot time: main: no tls: 23s, with tls: 2m45s patched: no tls: 14s, with tls: 15s VM Boot time, qemu-nbd via network (same lan): main: no tls: 18s, with tls: 1m50s patched: no tls: 17s, with tls: 18s Future optimization: if we could detect if there is another pending request we could defer the uncork operation because more data would be appended. Signed-off-by: Florian Westphal <fw@strlen.de> Message-Id: <20230324104720.2498-1-fw@strlen.de> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>	3 years ago

1 2 3 4 5 ...

395 Commits (7ddd3aca5f2efcf6aae10f836011e445ee77a6e1)