@TeddyAstie you were right, and it's the bigger win. I prototyped the pipelining you described and measured it. Same rig, same 107 GiB disk, same direction as the earlier runs.
Arm
Wall clock
vs control
Peak rate
Stalls
A : stock behaviour
2080.8 s
1.00x
63 MiB/s
62.2%
B : TCP_NODELAY only
743.8 s
2.80x
210 MiB/s
0.6%
C : + pipelined writes, depth 8
377.4 s
5.51x
432 MiB/s
1.6%
Pipelining is worth a further 1.97x on top of the Nagle fix. Your diagnosis was correct: the per-request reply gating, not TCP, is the dominant limit.
Verified byte for byte, source and destination md5 of the 107 GiB disk both ce647d9436b48401cd4b489c955ef0f7. That mattered more than the stopwatch here, for reasons below.
It's cheaper than you thought: the multiplexer already exists
No NBD redesign is needed. nbd/lib/client.ml:78 is already module Rpc = Mux.Make (NbdRpc), and that multiplexer:
assigns every request a unique handle (get_handle)
registers a waiter in id_to_wakeup keyed by that handle
serialises only the send under outgoing_mutex, then returns a promise
runs a background dispatcher thread that reads replies and wakes the matching waiter
So concurrent Client.write calls already interleave correctly. The whole request/reply machinery is there and unused. The serialisation is one fold_left in stream_nbd that awaits each write before pulling the next element.
The rest of the chain was already fine too:
Layer
Verdict
xapi nbdproxy
Unixext.proxy, raw bidirectional byte copy, never parses NBD, cannot serialise
tapdisk NBD server
NBD_SERVER_NUM_REQS 8, per-client request pool
destination storage
fio at 2 MiB blocks: 399 MiB/s at qd=1, ~1700 MiB/s at qd=2..8, collapses at qd=16
Depth 8 matches tapdisk's pool. The fio sweep says deeper is not better.
The actual trap: buffer ownership
This is the part worth writing down, because it is invisible from the protocol level and it bites silently.
Vhd_format.F.expand_copy allocates one 2 MiB buffer and hands out slices of it:
let buffer = Memory.alloc twomib_bytes in
...
let data = Cstruct.sub buffer 0 (this * 512) in
really_read h (sector_start ** 512L) data >>= fun () ->
return (Cons (`Sectors data, next))
It refills that same buffer on every step. The current sequential code is safe only as a side effect of awaiting each write before pulling the next element.
Pipeline it naively and you get: launch write N, pull element N+1, really_read overwrites the buffer, write N puts block N+1's bytes at block N's offset. The migration completes, reports success, and the disk is corrupt. Nothing in the stack flags it.
So any implementation of this needs buffer ownership solved alongside the concurrency. My prototype takes the cheap local route: a pool of depth buffers in stream_nbd, one memcpy per 2 MiB block, buffer returned only once its write completes. The proper fix is a buffer pool inside expand_copy itself, but f.ml is a shared library with other consumers, so that is a wider change than I wanted for a measurement.
Prototype patch
Against xapi-project/xen-api, ocaml/vhd-tool/src/impl.ml, on top of the TCP_NODELAY patch from the previous post.
This is a measurement prototype, not mergeable as-is. Known gaps:
progress reporting counts issued rather than completed work
a failed write leaves its siblings unawaited rather than cancelled
the per-block memcpy is a workaround for the shared buffer, not the right fix
--- a/ocaml/vhd-tool/src/impl.ml
+++ b/ocaml/vhd-tool/src/impl.ml
@@ stream_nbd
(if not prezeroed then expand_empty s else return s) >>= fun s ->
expand_copy s >>= fun s ->
+ (* Pipelined writes. The NBD client already multiplexes: every request gets a
+ unique handle and a background dispatcher matches replies back to waiters,
+ so several writes may be outstanding at once. Issuing them one at a time
+ makes every request pay a full round trip.
+
+ Depth 8 matches NBD_SERVER_NUM_REQS in tapdisk's NBD server. Deeper just
+ queues.
+
+ Buffer ownership matters here. [expand_copy] hands out slices of a single
+ shared 2MiB buffer that it refills on every step, so an in-flight write
+ cannot keep pointing at it: pulling the next element would overwrite the
+ bytes before they reach the wire. Each outstanding write therefore gets a
+ private buffer from a pool sized to the pipeline depth, returned only once
+ the write has completed. *)
+ let depth = 8 in
+ let twomib = 2 * 1024 * 1024 in
+ let free = ref (List.init depth (fun _ -> IO.alloc twomib)) in
+ let inflight = ref [] in
+ let reap () =
+ match !inflight with
+ | [] ->
+ return ()
+ | l ->
+ Lwt.nchoose_split (List.map fst l) >>= fun (_, pending) ->
+ let still, done_ =
+ List.partition (fun (t, _) -> List.memq t pending) l
+ in
+ inflight := still ;
+ free := List.map snd done_ @ !free ;
+ return ()
+ in
+ let rec drain () =
+ if !inflight = [] then return () else reap () >>= fun () -> drain ()
+ in
fold_left
(fun (sector, work_done) x ->
( match x with
- | `Sectors data -> (
- Client.write server (Int64.mul sector 512L) [data] >>= function
- | Ok () ->
- return Int64.(of_int (Cstruct.length data))
- | Error _e ->
- fail (Failure "Got error from NBD library")
- )
+ | `Sectors data ->
+ (* Block only when the pipeline is full. *)
+ (if !free = [] then reap () else return ()) >>= fun () ->
+ let buf = List.hd !free in
+ free := List.tl !free ;
+ let len = Cstruct.length data in
+ let mine = Cstruct.sub buf 0 len in
+ Cstruct.blit data 0 mine 0 len ;
+ let t =
+ Client.write server (Int64.mul sector 512L) [mine] >>= function
+ | Ok () ->
+ return ()
+ | Error _e ->
+ fail (Failure "Got error from NBD library")
+ in
+ inflight := (t, buf) :: !inflight ;
+ return Int64.(of_int len)
| `Empty _n ->
(* must be prezeroed *)
assert prezeroed ;
return 0L
@@
(0L, 0L) s.elements
>>= fun _ ->
+ (* Every write must land before the stream is declared complete. *)
+ drain () >>= fun () ->
p total_work ;
return (Some total_work)
Caveats on the numbers
The destination SSD degrades partway through a large transfer (DRAM-less Lexar NM790, host with 3.9 GB RAM), which is why arm C starts at ~432 MiB/s and settles around 275. Arm C runs closest to that ceiling so it feels it most. On better destination storage the gap should widen, not narrow.
Also worth saying: after pipelining, TCP_NODELAY matters much less, since with 8 requests in flight there is almost always at least an MSS queued. It is still correct to set it, and it is what every other NBD client in the stack does, but the 2.8x from the previous post should be read as "what you get today with a 12 line change", not as something that stacks cleanly onto the 5.5x.
