Repository navigation
add virtual block device servers - #1668
Conversation
including tcmu, ublk, nbd, vhost-user, vduse
blk/test/CMakeLists.txt listed blk/test/tcmu-abi-check.c among the sources of test-blk-tcmu, but that file was never committed, so the branch did not build from a clean checkout. The file existed only to assert tcmu.cpp's hand-defined TCMU literals against the kernel's own <linux/target_core_user.h>. Guarding against a kernel uapi change is not a risk worth carrying, and tcmu.cpp already pins every size and offset it relies on with its own static_asserts -- six of them offsetof, so the claim in the ring-ABI comment that "the static_asserts below pin every offset" stays true. Nothing that verifies our copy is lost; only the cross-check against the kernel header goes. The two tcmu.cpp comment blocks that named the file are rewritten to drop the reference. The rationale they pointed at -- why the header cannot be included from C++, why only the mailbox and entry header are packed, and why the SCSI constants are checked empirically instead -- is already recorded independently in tcmu.cpp and stays.
Reorders the members of 17 hand-written blk structs so each falls to its smallest size under the Itanium ABI -- 8-byte members first, then the 4-, 2- and 1-byte ones. 280 bytes total across seven files. 43 custom structs were measured on aarch64 LP64 with a compile-time sizeof probe rather than hand-computed; 26 already had zero headroom and are left alone. The 46 uapi and wire-format copies are excluded on purpose: their layout is the ABI, not ours to choose. Each reordered struct gains a comment saying the order is padding-driven, so the next reader does not tidy it back. Three of those comments record constraints that are not obvious from the code: a derived Config may place members in its base's tail padding, so its optimum depends on where the base's content ends rather than on the base's sizeof; UblkController::Config's optimum is deliberately ascending by width because that offset is not a multiple of 8; and a polymorphic base contributes a vptr with no tail padding, which is what sets NbdDeviceImpl's floor. No behaviour change, and verified to be a pure move: of the 116 deleted lines only 4 are not re-added verbatim -- two are whitespace normalisation and two are comment rewordings, one of which was required for truth (tcp_endpoint's "enabled by the following flag" became false once enable_tcp moved ahead of it). Builds with zero diagnostics and zero -Wreorder on aarch64 Linux (io_uring on and off) and on macOS. The seven blk suites pass under sudo with no skips and no failures.
blk/test/CMakeLists.txt already describes vhost-user-cli.cc as an existing file, but it was never tracked, so the description pointed at nothing. test-vhost-user.cpp's mock frontend shares this repo's own assumptions about the vhost-user wire format, which structurally cannot catch a format both sides got wrong the same way. These three files make an independent frontend runnable: vhost-user-cli.cc serves one image over a vhost-user socket, qemu-vhost-user-peer.py points QEMU's own vhost-user-blk qtest at it instead of qemu-storage-daemon, and mutate-vhost-user.py reverts one guard at a time so each assertion's value is shown to differ between guarded and unguarded code.
init() falls back through a recommended order and keeps the first engine that initializes, so the mask a caller passed does not say what is running: on Linux INIT_EVENT_DEFAULT names both iouring and epoll and epoll wins. Code that casts the master engine to a concrete type needs the winner, not the request. The answer comes from the engine rather than from a record init() keeps, because the engine is the only thing that cannot be wrong about what it is. That also means no new state to clear: fini() already restores the NullEventEngine, and NONE is what it reports. engine_flag() is pure virtual, so every engine has to answer and a new one cannot silently inherit a guess -- the guess IS the answer here, so a wrong one is a vcpu that gets trusted when it should have been refused. That makes this a source-level break for out-of-tree engines, which have to add the one-liner and rebuild; photon does not promise ABI stability, but the vtable grows a slot either way. The two engines whose answer is NONE say why in a comment: NullEventEngine IS the no-engine state, and the dpdk engine has no INIT_EVENT_* flag and is never installed as a vcpu's master, so nothing can ask it. get_io_engine() reports the request mask, because io engines are independent add-ons rather than a fallback chain, and there is no per-vcpu object to ask -- and libaio's context is thread-local, so a vcpu initialized without it cannot serve a libaio-backed file at all.
get_io_engine() reported "given and initialized", but init() stores the request mask verbatim, so a bit whose engine is compiled out of the build stays set. get_event_engine() called NullEventEngine's answer "the default", which was true of the first draft of the interface and stopped being true when engine_flag() became pure virtual. And NullEventEngine's own comment claimed it covers both sides of a vcpu's lifetime, when outside that lifetime there is no vcpu to ask. Comments only. Rebuilt clean on aarch64 and re-ran test-thread: 3/3 photon.engine_query*, 72/72 overall.
…from The vduse and vhost-user structs are hand-copied from headers too new for many build hosts, and a copied struct proves nothing on its own. vduse had twelve copies and no assertions at all; vhost-user had one. Both are about to read a queue index out of five of them for the first time -- a transposed field or a wrong width compiles clean today and only mis-directs an ioctl once a queue other than 0 is in play. These check that we copied correctly. They are deliberately not a cross-check against the kernel headers: a wrong number fails the build either way, and the cross-check would need a C translation unit plus a CMake entry for no extra signal on the fields we actually read.
It said the in-place scalar reads it justifies are "above", but the only thing above is the discussion of the access rule; the reads themselves are in handle_msg, several hundred lines below. Found in Task 2 review.
migrate_to_pool moves a freshly created serving coroutine into a caller-supplied pool, using an out-of-range index on purpose so WorkPool hands out its own pool-wide cursor -- several devices on one pool then interleave instead of each starting at vcpu 0. The empty-pool test in it is load-bearing: that cursor is a modulo, and size 0 is a SIGFPE. check_pool_engines refuses a pool whose vcpus cannot host the work, deriving the requirement from the caller's own vcpu instead of asking for it in the config. Both halves matter and neither is obvious from the WorkPool API: its constructor defaults to no event engine at all, where every fd wait fails at once and the serving loop hot-spins while logging; and a libaio-backed file needs the io engine on the vcpu it runs on, because that context is thread-local.
The probe coroutines run on different pool vcpus, so they ran concurrently on different OS threads while sharing a countdown and a std::set. A lost update on the countdown left done.wait() waiting forever and hung the whole binary, and the concurrent inserts could corrupt the heap. The semaphore ordered each coroutine against the caller and nothing else. Each coroutine now writes only its own pre-assigned slot and the caller builds the set after every one has signalled, so there are no shared writes to serialize. Also drops a comment in migrate_to_pool that was pasted twice.
BlkConfig::vcpus is gone; BlkConfig::pool is the single source of serving vcpus. Two knobs for one capability meant five transports each maintaining two dispatch paths, and the pool already does the membership management and round-robin that the old per-transport formulas reimplemented. ublk and tcmu are converted here because they were the only two consumers. ublk needed more than a call to migrate: its queue_setup creates the io_uring ring, the pump and every tag submitter, and those must share a vcpu because the submission queue is not thread-safe. Migrating the pump afterwards strands the tags on the caller's vcpu, and migrating them too is impossible -- they are parked, and thread_migrate only takes a READY thread. So the coroutine that runs queue_setup is what gets migrated, and everything it creates inherits its vcpu. It records where it landed, because teardown has to go back there for the same reason, and WorkPool cannot name a vcpu it already handed out. BlkConfig grows a pointer and loses a uint32_t. Placing the pointer first among the 8-byte members keeps sizeof at 88; after stack_size it would leave a 4-byte hole and grow the struct to 96. The four derived Configs' padding comments are re-measured, not recomputed -- three of them grow by 8 because the base's tail padding they were filling shrank from 6 bytes to 2.
Moving the pump into the caller's work pool left serve_stop() behind on the caller's vcpu, and serve_stop touches state the pump's coroutines own: its flush path calls drain_ring, which writes the mailbox tail that in-flight handle_cmd completions also write, and it polls a non-atomic in_flight that their DEFERs decrement. Both were single-vcpu invariants. Record the vcpu pump() actually lands on and hand serve_stop to a coroutine migrated there -- the shape ublk already uses for queue teardown, needed because thread_migrate only accepts a READY thread and a caller cannot move itself. No existing case caught this: the one pool-serving tcmu test detaches with nothing in flight, so the flush drains an empty ring. Add a stop-under-load case that keeps a writer running across detach(wait_pending=true).
run_serve_stop picks its branch on `home`, but home was written by pump on the pool vcpu and read by the teardown on the caller's, with nothing ordering them. A stale null read selects in-place teardown -- the race the migration exists to avoid -- so the branch was decided by a data race rather than by whether a pump exists. serve_start now waits for pump to record home, which also makes the !home branch mean what its comment claims: there is no pump. ublk gets the same guarantee from its owner coroutine's done.wait.
Three comments in nbd.cpp justify leaving conns, Conn::in_flight and stopping unsynchronized by pointing at the single vcpu every serving coroutine runs on. BlkConfig::pool retires that premise: each connection's serve_conn is about to be migrated onto a pool vcpu while accept_loop and the API calls stay on the caller's, so conns would be pushed, erased, iterated and swapped from two OS threads at once, and detach(wait_pending) would poll a plain counter that the other thread increments. conns gets a spinlock rather than a mutex because every critical section is yield-free, which also keeps the struct's padding arithmetic intact. Hardening only: nothing is migrated yet, so the suites are unchanged and no test can observe the lock -- the migration that makes it load-bearing carries the oracle.
conns_lock landed between the 8-aligned doit_thread and conns, stranding a 7-byte hole and taking NbdDeviceImpl from 400 to 408. The struct's own field order is padding-driven and gathers every sub-8 member after doit_thread for exactly this reason, so the lock joins that group; measured back to 400. Also correct two comments that overstated the present: the file header claimed serve_conn already runs on a pool vcpu, which is what the next commit does, not what this one does. The guards exist so that it can; saying they exist because it does would leave a reader unable to tell which commit to blame.
The engine's threading comment justified leaving event_idx, capacity, run and stopping unsynchronized by pointing at the one serving vcpu everything ran on. BlkConfig::pool splits that: the transport's message loop stays on the caller's vcpu while the queue serves on a pool vcpu, so SET_FEATURES writes event_idx and resize() writes capacity on one OS thread while should_notify and serve_chain read them on another, and teardown writes run/stopping across the split. Relaxed ordering is enough -- these are single scalars with no publish relationship to other memory. The fields published as a group (the ring pointers, the kick/call fds) need quiescing rather than atomics and are handled where the migration lands. last_avail stays plain for now: its hardest reader is a teardown drain that polls it while expecting the loop to keep advancing it, which atomics do not fix. That reader moves onto the queue's home vcpu instead.
Three comments stated the pool migration as already done -- that serve_conn's execute coroutines bump in_flight "on a pool vcpu", that serve_conn's DEFER erases "from a pool vcpu", and that the queue "serves on a pool vcpu" while the message loop stays behind. None of it is true at these commits: nothing is migrated yet, and the guards exist so that it can be. Reword to the capability form, which is accurate now and stays accurate after. The engine's threading comment also cited a design document that is not in the repository, so a reader could not follow the reference; the reasoning it was standing in for is now spelled out, including why the group-published fields are quiesced rather than made atomic. nbd's stopping accesses become explicit relaxed load/store to match the rest of the newly atomic fields, and the conns_lock comment is reworded -- it read as though the lock, not conns, were the member declared further up.
thread_enable_join writes a flag in the pump thread's own struct. Done after migrate_to_pool, that write races the pool vcpu's scheduler, which owns the thread by then and is running it. The pump cannot exit that early -- its loop tests a flag that serve_start just cleared -- so the damage is a racing write rather than a use-after-free, but it is unsynchronized all the same and costs nothing to avoid. ublk's start_serving already uses create, enable_join, then migrate; tcmu was the only transport with the two swapped.
VirtQueueServer::notify_valid had the same cross-vcpu shape as event_idx two lines above it: the transports' reset paths clear it from the control plane while should_notify reads and writes it on the serving side. std::atomic<bool> is free here -- same width and alignment, sizeof(VirtQueueServer) still 152. Three comments also claimed protection the code does not have. The VirtQueueServer threading block said the ring pointers are "only mutated with the loop quiesced", which nothing does yet, and credited all five atomics to the message loop when only two of them are written there. Two tcmu comments still said serving runs on the vcpu that called start(), which stopped being true once the pump became migratable.
nbd has no queue count to declare -- its parallelism is however many clients connect -- so the connection coroutine is the unit that moves. It is migrated last, after it has been registered in conns and workers: registration is yield-free, but that only orders it against this vcpu, and a worker already on a pool vcpu does not wait for us to yield before it can fail out and free the Conn. The per-request coroutines inherit the connection's vcpu. The stream an accept produced keeps working after the move because nothing in the socket path caches a vcpu: wait_for_fd_readable resolves the event engine from the current vcpu on every call. The test half adds the observation the whole conversion needs. Placement is the contract BlkConfig::pool makes and nothing in a transport's API reports it, so a pass-through IFile that records get_vcpu() on each backend IO is what turns it into an assertion -- one mechanism, all five transports. The second new case puts eight OS threads through connect/disconnect cycles while the caller polls get_client_connections() and restarts the device, which is what makes the connection list's lock observable now that serving really is off-vcpu.
RecordingFile passed through the 17 pure virtuals and stopped there, on the premise that the transports never call anything else. That premise is false. IFile::trim() and IFile::zero_range() are plain methods that call the VIRTUAL fallocate, so the inherited UNIMPLEMENTED default answered ENOSYS for every discard/write-zeroes request the transports issue: nbd's TRIM and WRITE_ZEROES (nbd.cpp:729/:739 -- ENOSYS is not EOPNOTSUPP, so WRITE_ZEROES never even reaches its zero-fill fallback), tcmu's UNMAP and WRITE ZEROES (tcmu.cpp:1106 /:1131), ublk's discard (ublk.cpp:541) and the shared virtio path (utils.cpp:80), which is how vhost-user and vduse get there too. Likewise the three FUA write sites (nbd.cpp:715, tcmu.cpp:908, ublk.cpp:531) call pwritev2(RWF_DSYNC), whose base default discards the flag. The failure was latent rather than visible because no test issued a discard or a write-zeroes through a RecordingFile: the primitive landed in the same commit as its first user, and the placement assertions only needed preadv/pwritev. connections_spread_over_the_pool now issues TRIM, WRITE_ZEROES and an FUA write through the probe in its own region, which is what makes the omission observable -- reverting only the fallocate override turns both new assertions red with NBD_ENOTSUP (95). The metadata methods stay unrecorded; the reason is that they are not what a serving coroutine does with a request, not that nobody calls them. Also: move <photon/photon.h> to harness.cpp (only TestPool's constructor body uses it), delete TestPool's copy operations (~TestPool deletes the pool), and give the concurrency test a success counter -- `bad == 0` also held when the workers did no verifiable work, because they used to spend their whole iteration budget on connects inside the excused restart window.
Still exactly one queue: GET_QUEUE_NUM answers 1 and num_queues publishes 1, so no frontend sees a difference. What changes is that every piece of per-queue state now lives in a heap-allocated Vq reached by index, and every internal function takes that index. VirtQueueServer holds a std::atomic, so it is neither copyable nor movable and cannot sit in a vector by value -- resize() needs MoveInsertable. A pointer vector also keeps each Vq's address stable for the device's lifetime, which the hook bindings depend on: they carry the Vq* as their context so notify and ready can recover their queue index, and Delegate cannot bind a temporary. Each queue's serving coroutine is handed to the caller's work pool, migration last -- created, null-checked, join-enabled and only then moved. That migration puts the control plane and the serving side on different OS threads, so the ring state they share is no longer protected by sharing a vcpu. The handlers that publish desc/avail/used/num or swap the kick/call fds now quiesce the queue first (join the loop, drain the requests it dispatched) and restore what they found, and teardown -- the backlog wait that reads last_avail while expecting the loop to advance it, the interrupt/join, the drain -- runs on the vcpu the loop recorded when it landed, published synchronously to vq_start. stop_session therefore sets the device-level stopping before joining the accept and message loops, and the engine-level one after the backlog wait: the first is what lets those joins return, the second is what lets the wait make progress. handle_mem_table's was_enabled becomes per-queue, since with several queues a frontend may have enabled only some of them.
The ready hook (vq_may_dispatch) reads VqVhu::enabled on the queue's own vcpu, while handle_mem_table, SET_VRING_ENABLE and stop_session write it on the control plane's vcpu -- and those writes are not all inside a quiesce: handle_mem_table and stop_session clear the flag before vq_stop/join, and SET_VRING_ENABLE sets it before vq_start, which early-returns (leaving the write concurrent with the hook's read) when the loop is already live. Make the field std::atomic<bool> and spell every access as an explicit relaxed load/store: the flag publishes no other memory, it only must not tear or lose an update. Layout unchanged (measured sizeof(VqVhu) == 80 before and after on the aarch64 VM). Also correct blk.h's BlkConfig::queues comment, which claimed the field is honored by vhost-user and vduse: both hard-serve a single queue today and ignore it; only ublk honors it.
…cklog msg_loop's exit path stopped every queue unconditionally. That is what a frontend disconnect needs -- the SERVER listener stays up and a reconnect must start from stopped queues -- but it also fired when stop_session set the device `stopping` flag, and stop_session joins that loop before it reaches its own per-queue teardown. By then the loop coroutine was gone, so the backlog wait returned at its "no loop here" guard without ever running, and detach(true) silently stopped honouring its wait_pending contract. Only stop the queues on the exit path when this loop owns the teardown; when `stopping` ended the session, stop_session and rollback both stop and drain every queue themselves, deliberately after the join so no SET_VRING_* can swap the ring out from under the drain. Also: vq_start logged after thread_create, leaving a yield window before the migration that needs the new coroutine still READY (the default log path takes a spinlock, but a caller-installed sink writing through a photon IFile would yield); the VqVhu::home comment claimed the serving side was the only writer of last_avail/used_idx when the control plane writes both under quiesce; the readiness re-check comment in utils.cpp named SET_VRING_ENABLE as something it catches, but that path clears `run` too and the loop breaks before reaching the re-check -- the two writers it does catch are the ones that clear `enabled` before quiescing.
The teardown order that makes detach(true) honour its wait_pending contract -- drain the avail backlog while the loop is still live, then stop it -- had no test: reverting the guard that keeps the loop alive left the whole vhost-user suite green, because nothing ever held work in the ring while a detach ran. RecordingFile grows a gate so a test can hold every backend IO, which pins the in-flight count at the dispatch cap and leaves the rest of the ring unconsumed indefinitely rather than for one polling interval. The oracle is the used-ring advance: the correct order completes all of it, while stopping the queue first clears `enabled`, so completions stop being published and the backlog is never dispatched at all. The difference is the whole count, not a timing margin. Also state in VqVhu::home that what makes the control plane's writes to last_avail/used_idx exclusive is joining the loop AND draining its requests -- request coroutines outlive the join, so "joined" alone under-describes it.
Every vring message carries a queue index and all seven handlers read it and then dropped it, which is why the device could only ever serve one queue. They now select a slot with it, and reject an index at or past the queue count -- unchecked it is an out-of-bounds vector subscript on a value the peer supplies. The queue count itself now comes from BlkConfig::queues, clamped to the transport maximum and defaulting to one, which is the same reading the ublk transport already gives that field. VIRTIO_BLK_F_MQ is offered exactly when there is more than one queue: offering it at n==1 would have the frontend build one vq while believing the device is multiqueue, and virtio 1.2 §5.2.4 makes num_queues meaningless without the bit. GET_QUEUE_NUM and num_queues now both report that one count, so the two cannot disagree -- a frontend that reads one while the device serves the other addresses queues nobody is listening on.
queue_count_follows_config bracketed the `nqueues >= 2` gate that decides whether to offer VIRTIO_BLK_F_MQ with 1 and 3 but never pinned it: moving the gate to `>= 3` left the whole table green, because n==1 still offers nothing and n==3 still offers the bit. 2 is the one input where the two gates disagree. Adding it turns that mutant red on `F_MQ, queues=2`, while GET_QUEUE_NUM and num_queues keep reporting 2 -- the offered feature bit is the only value this gate controls, so it is the only one that can catch the move. Also drop the "whose test below is unchanged" tail from the vring index comment in SET_VRING_KICK/CALL. It described the diff that introduced it rather than the code, so it lost its referent the moment that diff aged; the wire-encoding half it was attached to -- low 8 bits are the index, bit 8 is NOFD -- is the part that stays true.
Still one queue: CREATE_DEV declares vq_num 1 and VQ_SETUP only runs for index 0, so nothing the kernel sees changes. Every per-queue member now lives in a heap-allocated Vq reached by index, and the hooks carry it as their context. vq_needs_refresh moves into the per-queue state: SET_STATUS is a device-wide message with no index, so it now flags every queue, while the refresh that consumes the flag is per-queue. The iotlb stays device-wide -- an address space does not belong to a queue. The per-queue state is also hardened for the serving vcpus that come from a WorkPool: the iotlb cache takes a photon::mutex split so the blocking IOTLB_GET_FD + mmap slow path never runs under it, the read-and-clear flags use exchange() so a concurrent set cannot be lost, and teardown hops onto the queue's home vcpu before it dereferences the ring. Separately, a pure ordering move: the loop coroutine's create + migrate goes last in start(), after the adoption resync and the ring refresh, instead of before them. Those two write the ring fields, event_idx and the iotlb cache from the caller's vcpu, which is only safe today because the loop they race is on that same vcpu. Nothing else in start() moves. The two shared-header sentences that put vduse's control plane on the loop's vcpu are rewritten to say what protects it now: a quiesce for the messages that clear readiness, a hop for teardown's backlog wait.
msg_loop registers the DEFER that closes a message's fds inside its loop body, before handle_msg, so it runs when the iteration ends -- after the default arm has already written that iteration's ack. The case counted /proc/self/fd right after the eighth ack and treated anything still open as a leak, so what it actually measured was whether the backend coroutine had been scheduled yet: 16 of 50 repeats failed here, and an independent run measured 55 of 200. One more unrecognised message carrying no fd is a deterministic barrier: its ack cannot be written until the previous iteration has ended, and it adds nothing of its own to the count. 200 of 200 repeats now pass, and all seven blk suites are unchanged at 102. The comment claiming the ack came after the DEFER is why this went unnoticed -- it asserted a synchronisation the code never provided.
The previous commit gave vduse one state slot per queue and, at the two control-plane invalidations that clear a queue's readiness, quiesced the queue first: stop its loop, drain the requests that loop dispatched, clear, restart. vduse's control plane cannot afford that. The kernel blocks whoever sent a message until the daemon answers it and gives up after msg_timeout seconds (30 by default), so putting a wait whose only bound is the arrival rate of backend IO in front of a reply can brick the device under a live consumer. The quiesce also had to grow a restart path to survive its own stops, which turned the loop coroutine's lifetime from per-start() into per-negotiation, and it ordered the vq state read drain-before-stop, so the loop kept dispatching into the very count the drain was waiting on. Readiness is published through a per-queue generation counter instead. An invalidation bumps the counter before it clears; vq_refresh snapshots it as its first act and publishes only if it has not moved, which makes the clearer's false the last word. A withheld publish re-arms needs_refresh, because vq_tick consumed that flag before calling the refresh and DRIVER_OK does not come twice: without the re-arm the queue would stay not-ready for good and the device would silently serve nothing. VDUSE_GET_VQ_STATE becomes a pure read. The kernel asks where a queue's last_avail is; it never asks for the queue to be stopped, unlike vhost-user's GET_VRING_BASE, whose protocol meaning is "stop this vq and return it". The handler hops onto the queue's own vcpu, reads the counter and comes back. Three things existed only to serve the quiesce and go with it: vq_start's refusal while the device is stopping, vq_stop_here claiming the coroutine handle before the join instead of after it, and the device-level `stopping` hoisted to the top of stop_serving. That last one sat on the teardown ordering rule -- it made the message loop stop answering during an unbounded backlog wait, and the destructor's fallback is the one caller that reaches stop_serving(true) with a consumer still attached. It is back after the backlog wait and before the interrupt that ends the message loop. Iotlb gets a generation counter of its own. resolve()'s slow path runs outside the cache lock, and the re-scan before the insert cannot tell "still absent" from "invalidated while we were fetching": an invalidated range has left `maps`, and the fetch was never in it. Publishing anyway would cache a mapping of pages the driver has already unmapped, and every later request for that IOVA would be a hit on them. Unreachable while everything serves on one vcpu -- no critical section yields, so the lock is never contended -- and reachable as soon as BlkConfig::pool puts the loops elsewhere. The two shared-header sentences that described vduse as quiescing are rewritten to say what protects it now: a hop for the two reads that need the loop's vcpu, a generation counter for the readiness conflict. Recorded here because it shipped in the previous commit unmentioned: vq_tick's flush_stale guard is device-wide rather than per queue. `stale` is one device-level list while the readiness and the in-flight count that make a munmap safe are per queue, so with more than one queue an idle queue 0 would munmap mappings from under queue 1's live requests. With one queue the two forms are arithmetically identical, so neither commit changes behaviour today.
| // word that the bytes are already durable. A backend opened O_DIRECT makes | ||
| // the extra fdatasync redundant rather than wrong, which is the same trade the | ||
| // virtio engine already accepts. | ||
| if ((fua || write_through) && backend->fdatasync() < 0) |
There was a problem hiding this comment.
Answer to finding 2 of 4 in @liulanzheng's TCMU review — [P1] Make ordinary writes durable when FEATURE_FLUSH is absent. Confirmed, fixed in 279a58f (tcmu) and 7f059f0 (nbd/ublk), taking remedy (i). This was a top-level comment; it belongs here, at the sync the finding says was missing. Your citations were tcmu.cpp:1003 and :1170-1171 at 7b1deb2; at this head the derived flag is :609-610 and the ordinary-write sync is :1167. It turned out not to be tcmu-specific: nbd and ublk had the same defect and are fixed in the same batch, so all five transports now answer the durability question the same way.
Your kernel reading is right and is the part that decides the remedy: sd_set_flush_flag() clears BLK_FEAT_WRITE_CACHE and BLK_FEAT_FUA together when WCE is 0, and blk-flush.c then strips REQ_FUA. DPOFUA is parsed and discarded. So a Linux initiator facing WCE=0 sends neither SYNCHRONIZE CACHE nor FUA, and the comment that claimed FUA was "a per-write durability path distinct from the cache WCE describes" was false — it has been rewritten.
Remedy (i), because remedy (ii) is not available: WCE is already 0, so there is no bit that says "not durable and no command to ask with". write_through, derived as !(features & FEATURE_FLUSH), now makes an ordinary write durable before its GOOD. When FLUSH is offered nothing changes — the client can batch behind one FLUSH, and fdatasync is a second syscall per write waiting on the whole file's dirty data. This is the rule the shared virtio engine already applied for a driver that declined VIRTIO_BLK_F_FLUSH, so tcmu now agrees with it instead of being the exception. features = 0 is covered explicitly, as you asked, and was previously uncovered entirely.
Same defect, same fix, in the other two transports that advertise their own cache policy:
- nbd omitted
NBD_FLAG_SEND_FLUSH/SEND_FUA, so the kernel's nbd client clears both queue cache flags, whileNBD_CMD_WRITEacked afterpwritevalone and synced only onNBD_REQ_FUA. - ublk registered neither
UBLK_ATTR_VOLATILE_CACHEnorUBLK_ATTR_FUA, soublk_drvsets no write-cache feature, while the write completion synced only when the request carried FUA.
vduse and vhost-user were already correct through write_through. For ublk the flag is derived in the constructor from the same word want_attrs() advertises from, so the two halves cannot drift; advertising VOLATILE_CACHE unconditionally instead would not work, because params_features() reads FEATURE_FLUSH back out of those bits and validate_params() would then fail the drift check for every FLUSH-less device, with no repair available since SET_PARAMS is refused once a device has been used.
| // LBA and the count. Reading (12)'s be32 at +7 shifts the count left by eight | ||
| // and folds in cdb[10], the control byte, so a one-block CDB parses as 256 | ||
| // blocks and every legitimate (12) is refused as INVALID FIELD. | ||
| uint64_t cdb_xfer_bytes(uint8_t op, const uint8_t* cdb) { |
There was a problem hiding this comment.
Answer to finding 3 of 4 in @liulanzheng's TCMU review — [P2] Read the READ/WRITE(12) transfer count at CDB byte 6. Confirmed and fixed in 279a58f. This was a top-level comment; it belongs here, at the parser. Your citation was tcmu.cpp:939-940 at 7b1deb2; at this head the helper is cdb_xfer_bytes() at :1084, and its two callers are the READ and WRITE bounds checks at :1111 and :1134.
transport_get_sectors_12() reads get_unaligned_be32(&cdb[6]); we read cdb + 7, which is where the (10) form's be16 lives. Your arithmetic reproduces exactly: with count 1 at bytes 6–9 and a clear byte 10, reading at +7 yields 00 00 01 00 = 256 blocks = 131072 bytes. (6), (10) and (16) all matched the kernel; only (12) was wrong. Fixed, and the comment above cdb_xfer_bytes that documented the wrong offset now states why the four offsets are not a progression. Every legitimate (12) was being refused as INVALID FIELD, so this failed closed rather than silently — but it was total for the family.
You asked for valid READ(12) and WRITE(12) CDBs to be exercised; both are, and rather than assert the new cases have teeth, the production file was reverted to the revision you reviewed and they were re-run against it: 11 of the 13 tcmu cases added in that batch fail against 7b1deb2. The two that pass either way are a control pinning "do not sync when FLUSH was offered" and a destroy_orphan EBUSY refusal — the latter labelled in its own comment as not a witness for the held lock, since the leading probe already refused a held tombstone before that change and the window the hold closes is only observable as an interleaving.
| // reported afterwards. Leaving it unclaimed costs nothing: a directory is not | ||
| // a tombstone any daemon can claim, so there is no successor to race with. | ||
| int claim_fd = -1; | ||
| if (devlock_acquire(lock_dir, lock, &claim_fd) < 0 && errno != EISDIR) { |
There was a problem hiding this comment.
Answer to finding 4 of 4 in @liulanzheng's TCMU review — [P2] Hold exclusion throughout backstore deletion. Confirmed and fixed in 279a58f, and the two sites you named turned out to be two different defects. This was a top-level comment; it belongs here, at the claim this now holds. Your citations were destroy_orphan():2479-2481 and shutdown():1896-1902 at 7b1deb2; at this head destroy_orphan() claims at :3076 on the present branch and :3009 on the absent one.
destroy_orphan() was the one that needed the held lock, and it was the only one of the four transports that probed where ublk, vduse and vhost-user all hold their claim across destructive cleanup. Your inode consequence is real: devlock_acquire() returned the fd with no fstat, no inode comparison and no O_EXCL, and devlock_unlink() removes by path, so after an unlink/recreate two processes can each hold a successful LOCK_EX on the same name. Your point about the backstore-only configuration is the reason no other guard covers it — configfs answers EBUSY on rmdir only while s_dependent_count is nonzero, which a LUN symlink is what raises.
The claim is now taken after the existence check and held across enable/rmdir/unlink, with one deliberate exception: EISDIR falls through unclaimed. devlock_free() reads a directory standing in for the tombstone as free (open(O_RDONLY) and flock both succeed on a directory fd) while devlock_acquire() refuses one (O_CREAT|O_RDWR on a directory), so claiming unconditionally would turn "an operator put a directory in the lock dir" into a refusal to remove the registration — and the documented contract for this call is that the registration goes first and the EISDIR is reported afterwards. That is safe to leave unclaimed because a directory is not a tombstone any daemon can claim, so there is no successor to race. The absent branch has no such fall-through, and the asymmetry is the reason: there, nothing has been destroyed when the directory is met, so there is no order to pin and no success the exception could preserve.
shutdown() had a different problem than the window you named. The release→destroy_backstore() gap is a handful of non-yielding syscalls, so a fresh adoption cannot complete inside it. The real defect is the failure path: an EBUSY from destroy_backstore() returned -1 having already released the claim, leaving an enabled backstore wearing a free tombstone — precisely what list_orphans() and the HBA's initial_scan hand to the next adopter. ublk orders it correctly for exactly this reason, and tcmu's own rollback() already destroyed before releasing. The claim now outlives the teardown, and the retry reuses it — guarded on lock_fd, because flock is per open file description and an unguarded re-acquire answers EWOULDBLOCK against ourselves and reports the device as held by another server. Holding it across the rmdir is safe against the listener: on_removed() reads link.serving, which the line above already cleared, so it takes the "nothing to stop" branch and answers the kernel without touching the lock.
One later finding closed the remaining half of this, and it is worth reading together with the above: the destructor did not release the claim at all when destroy_backstore() had failed, so the fd and its flock outlived the object until process exit. That is answered at tcmu.cpp:2114. The case that reddens the shutdown() fix is shutdown_keeps_the_claim_when_backstore_removal_fails, which takes about 61 seconds and is not a hang: measured at 61166 ms with the fix and 61702 ms against the unfixed code, the gap in the same place both times, the cost being that it attaches a LUN to a backstore that is enabled and being served, which makes the kernel scan a new loopback host through the daemon.
| // queue_setup's very first act never happened, so this queue owns no ring, no pump and | ||
| // no tags, there is nothing of its on the owning vcpu, and queue_teardown on it is the | ||
| // null-safe walk over a half-built Queue that start()'s rollback has always performed. | ||
| void run_queue_teardown(Queue* q) { |
There was a problem hiding this comment.
Answer to @liulanzheng's [P1] Keep queue teardown on its home vCPU when the handoff fails. Confirmed, fixed in 8158bf2 on both error paths and in tcmu too, whose run_serve_stop() had the identical shape — and then superseded in 509344e, which removed the failure rather than handling it. This was a top-level comment; it belongs here, at the function it was about. Your citation was ublk.cpp:1041-1061 at a702958; at this head run_queue_teardown() is :1115-1127.
What the code does now
Neither error path you named exists any more. queue_setup() creates the teardown coroutine as its first act, on the vcpu migrate_to_pool has already landed the owner coroutine on, and parks it on a semaphore. run_queue_teardown() only signals and joins — photon::thread_interrupt and photon::thread_join are both cross-vCPU safe — so there is no allocation to fail and no migration to refuse at teardown time, and nothing left to run in the wrong place. run_queue_teardown(), stop_serving() and with them detach(), shutdown() and rollback() cannot fail for this reason.
The remaining in-place branch is not a fallback for a refused handoff. A null teardown_th means queue_setup's very first act never happened, so that queue owns no ring, no pump and no tags: there is nothing of its on the owning vcpu, and queue_teardown() on it is the null-safe walk over a half-built Queue that start()'s rollback has always performed. The ordering is what makes that sound rather than merely convenient — because the coroutine is created before the mmap, the buffers, the ring, the pump and the tags, a null teardown_th can only describe a queue that owns none of them.
Two corrections to the mechanism, both still true
They are about photon rather than about the old shape, so removing that shape does not retire them.
The race was narrower than described, and the residual was worse. thread_interrupt and thread_join are both cross-vCPU safe: prelocked_thread_interrupt has an explicit branch for a target living on another vCPU and routes it through move_to_standbyq_atomic, and thread_join is th->lock plus th->cond.wait(th->lock). The io_uring generation is not a hazard either — iouring_abandon lands on ring->abandon(), whose entire body is m_generation++, and delete q->ce is sequenced after the pump join and every tag join. The real hazard was q->pump_stop, and it is a hang, not a corruption: a plain bool, and the only condition either of the pump's two exit tests reads (the loop head, and the break it takes when wait_for_events() returns -1 from the interrupt). A write that never becomes visible to the other OS thread leaves the pump re-parked, and queue_teardown's thread_join(pump_th) then waits forever. A missed q->stopping is not fatal by contrast — the tags exit on r != UBLK_IO_RES_OK once the ring is abandoned.
Migrating the caller is not available, and that was measured rather than reasoned. photon does accept th == CURRENT and routes it to defer_migrate_current, but from a vCPU's main coroutine — which is what a detach(), a shutdown() or a destructor usually is — thread_migrate(CURRENT, v) returns 0 and does not move: defer_migrate_current returns 0 unconditionally and do_defer_migrate is void, so do_thread_migrate's verdict is discarded. The caller then dies in std::thread::join with Resource deadlock avoided. I built a probe for this, and both suites aborted until the design changed. Every self-migration in this repo — fs/cache/store.cpp, examples/perf/net-perf.cpp, thread/test/test-sleepq-stale-idx.cpp — is inside a spawned coroutine.
A retraction I owe on this finding
My original reply described the 8158bf2 shape — a refusal that leaves the queue up with its ring mapped and cdev_fd open, a serving_leaked flag, and a pool that then never finishes being destroyed — and called that leak a safe outcome. It was not, and I said so in the ublk thread this also belongs to: retaining the Queue and the backend does not retain the object whose this the surviving member coroutines dereference, and a destructor that returns early still destroys its members and its base. So there was no shape of refuse-and-retain that was safe. blk.h now states the opposite contract — no transport refuses a teardown because of placement, and none leaves serving coroutines behind on one — which is why the refusal is gone rather than hardened.
On the test you suggested
Fault-injecting the allocation failure needs a test-only seam inside blk/ublk.cpp, and this repo has a standing adjudication against exactly that (T10-17, recorded in blk/test/mutate-blk.py). I added two such seams while working on this and removed both.
What covered the migration half was the nohome mutant, which deleted the thread_migrate your finding is about. Its target no longer exists, so it is booked RETIRED rather than killed, with both of its measurements kept in the ledger (killed twice, each by a 124 hang, plus the inverted killing-case booking that went with the second). nomigrate — which deletes the START-time hop — is now the only ublk mutant that can misplace a teardown, and its entry says so. So the invariant you asked for is still mutation-covered, but by a mutant aimed at the hop that now carries it rather than at the two paths you named, and I am not presenting that as the same witness.
There was a problem hiding this comment.
Updated review of 1f46b70f4b3cd7613e03405d368eca68f90049e2 after checking the supported interruption paths.
One P1 remains: socket-path ownership after detach/recovery. A historical sock_owned flag does not establish ownership of the current pathname after a successor has rebound it. The inline socket finding remains valid, including the old-owner shutdown/destructor and refused-restart rollback sequences. The standalone AF_UNIX probe reproduced deletion of the successor's live socket path.
The delegated-work completion P1 is withdrawn. The actual stop paths use default EINTR, which semaphore::wait retries; thread_shutdown's EPERM is also retried. I found no production path in these transports that injects ESHUTDOWN/ETIMEDOUT into the caller waiting for the worker acknowledgement. My earlier example required arbitrary external error injection, so it does not establish an in-scope lifecycle defect. No worker completion/cancellation redesign is requested on that basis.
The NBD diagnosis follow-up and confirmations of the earlier teardown/build-gating fixes remain as stated. This is a source review plus the standalone socket lifecycle probe; no full device-level test, source modification, approval or merge was performed.
| if (!sock_owned) | ||
| return; | ||
| sock_owned = false; | ||
| if (::unlink(sock_path) != 0 && errno != ENOENT) |
There was a problem hiding this comment.
[P1] Verify current socket ownership before unlinking a retained pathname.
sock_owned records that this object once bound the path, not that the current node is still that socket. A normal recovery sequence breaks that assumption: A starts and detaches (the fd closes but sock_owned stays true); destroy_orphan() removes A's dead node; B starts and binds/listens at the same path. A's later shutdown() or destructor reaches this unlink and removes B's live socket. Existing B connections survive, but new frontends cannot connect.
There is also a rollback variant: A tries start() after B has taken over; bind correctly returns EEXIST, but rollback sees A's old sock_owned == true and unlinks B's node anyway. The new comment claiming that a refused bind implies sock_owned == false does not hold across detach/restart.
The concrete resolution is specified in the updated ordinary PR comment: public detach ends the old session's path-deletion authority, so later shutdown/destruction/refused-start rollback cannot unlink the detached path. Successful bind grants a new session ownership. Preserve normal active shutdown's node removal by separating its internal stop/remove path from public detach; simply clearing the flag in today's detach would also affect shutdown(), which calls it. Serialize the active close/unlink removal with the existing identity claim. Update the detach-then-shutdown test and add successor-takeover/failed-restart coverage as listed there.
Confirmed at 1f46b70 by source inspection and a standalone AF_UNIX lifecycle probe using this cleanup function's extracted body: it removed the successor's live path and the next connect failed with ENOENT. The full Photon device test was not run.
There was a problem hiding this comment.
Fixed in 413d6e3, along the three lines you drew.
1. detach() leaves the node and denies itself the deletion. sock_owned was the whole problem: detach() stopped serving but kept it, so the flag still said "mine" about a path the object had already handed over. detach() now goes through end_session(wait_pending, false) and calls abandon_sock_node(), which clears the flag and touches nothing on disk.
2. shutdown() and destruction after a detach no longer unlink. The old shutdown() called release_sock_node() outside its started test — which is what made a detached object's shutdown delete a live successor's node — and the destructor called it unconditionally for the same reason. The removal now lives inside a session that is still serving, reached through one explicit argument rather than a second started test inside detach(), because your point 3 is exactly right: shutdown() is detach(true) plus the removal, so a detach() that always relinquished would silently stop every orderly shutdown from removing its own node.
3. rollback(). start()'s rollback needed no new test, only a corrected invariant: do_listen() is the only setter, and every path that ends a session clears the flag — detach() before it hands the path over, shutdown() after removing its own node, rollback() here. So a refused bind reads false even on an object that owned the path in an earlier session, which is the case that made A's failed restart unlink B's node. The old comment at rollback() asserted that invariant and it was only true for a device that had never bound anything.
Tests. shutdown_after_detach_removes_the_node_so_the_device_can_start_again is replaced by shutdown_after_detach_leaves_the_node_for_destroy_orphan: your A/detach/shutdown sequence, the node still there, destroy_orphan() as the operation that clears it, and then a still-serving start -> shutdown that must remove its own node — that last block is what stops the fix from turning every shutdown into a leak. a_detached_object_must_not_delete_the_successors_node runs your A/B sequence and checks both of A's exits — shutdown() and destruction — each followed by a real connect-and-negotiate, because a stat cannot tell "on disk" from "answering". a_refused_restart_after_handover_keeps_the_successors_node asserts EEXIST and that B is still connectable afterwards.
The witnesses are partitioned, not stacked. Two mutants were added to mutate-vhost-user.py (now 31, all anchors resolve). norelinquish — detach() keeps the flag — reddens the A/B case at its destructor assertion and the refused-restart case, and leaves the first case green, which is the attribution: the first case is about the leave/remove split, not the flag. noleavenode — shutdown() leaves the node — reddens the first case and the A/B case's shutdown() assertion, and leaves the refused-restart case green.
One premise in point 3 I could not confirm, so please treat this as the residue. You asked me to keep "the existing per-identity exclusion across listener close and unlink". There is none: release_sock_node() has never taken the identity lock. do_listen()'s devlock_acquire covers only its own bind -> listen window and is released when do_listen() returns, and destroy_orphan() takes the lock for its own probe-plus-unlink. So the shutdown path still has a window — one syscall wide, between close(listen_fd) and unlink() — in which another process could destroy_orphan the path and bind it, and our unlink would then remove its fresh node. I did not close it, because the natural fix (take the identity lock around the removal) changes a teardown path that I would rather you look at before I do. It goes in on your word.
blk.h's detach() contract now states the ownership handover, including that clearing such a node is destroy_orphan()'s job — the recovery path is the only one that takes the probe and the lock.
Verification: aarch64, kernel 7.0.0-38-generic, MinSizeRel/C++17, test-blk-vhost-user 87/87 with zero build warnings; macOS full build clean, with the vhost-user target absent by its LINUX gate. No other transport changed.
What your PR-level comment asks for that is not about the mechanism.
The trade, stated because it is the case you also filed: start -> detach -> shutdown -> start on the same path now needs destroy_orphan() in between. That is precisely what the old shape bought, and I replaced the case that pinned it rather than keeping both behaviours, because I read your instruction as: removing a path nobody serves belongs to the recovery call, not to the object that gave it up. If "a detached object may still own its backend" is also meant to include "and may restart on its own path without a recovery step in between", then the rule is different from the one I implemented and I would rather you say it than have me infer it.
Noted from the same comment, nothing attached to any of them in code: the NBD follow-up closed on your side, the ARM64 context-preservation item tracked separately in Photon core, and the delegated-work item left untouched as you said.
There was a problem hiding this comment.
Status at 413d6e3, both halves, since my comment above.
CI, per job rather than per run. All three workflows are green on this head — x86_64 40053052171, ARM 40053052165, macOS 40053052127. On x86_64: 100% tests passed, 0 tests failed out of 84, and test-blk-vhost-user ran 87/87 in each of its three engine passes, with all three of the new lifecycle cases showing [ RUN ] / [ OK ] in every pass. So the full lifecycle regression you asked for is exercised by CI, not only on my VM.
A bound on what that green means, because it is the same green you have been reading. On the CI nodes 148 of the 164 tcmu/ublk/vduse cases do nothing — configfs target not mounted, ublk_drv module not available, vduse module not available, which are the suites' own gates — and report_skip() only logs a marker instead of telling gtest, so those suites still print "passed". At this same head on the privileged VM they do run, with nothing skipped: tcmu 62/62, ublk 52/52, vduse 50/50, and the machine is left with no vduse/ublk registrations, no tcmu backstores, no leftover loopback LUN and no D-state. That is the only evidence for those three transports; CI cannot supply it.
Nothing in this message changes the two items from my previous comment, which are still waiting on you: whether a detached object should be able to restart on its own path without destroy_orphan() in between, and whether to close the one-syscall window between close(listen_fd) and unlink().
| SCOPED_LOCK(q->x.handoff); | ||
| q->x.body = body; | ||
| q->x.go.signal(1); | ||
| q->x.done.wait(1); |
There was a problem hiding this comment.
Withdrawn — no supported shutdown/cancellation path establishes the reported trigger.
I rechecked the actual vhost-user and VDUSE stop paths. Their thread_interrupt() calls use the default EINTR, which semaphore::wait() retries internally. thread_shutdown() uses EPERM, which this wait also retries. I found no production path in these transports that interrupts the caller waiting on done with ESHUTDOWN or ETIMEDOUT.
The previous example therefore depended on externally injecting one of those special errors, rather than an established supported lifecycle path. The agreed review scope does not require defending this handoff against arbitrary external shutdown/error injection. Without a concrete active cancellation path reaching this wait, the P1 rating and requested completion/cancellation redesign are not justified and are withdrawn.
No implementation change or regression test is requested for this finding. If a supported active cancellation path is introduced later, its lifetime guarantees can be evaluated with that path. The separate socket-path ownership finding remains open.
|
Follow-up on the NBD diagnosis at The new client substage/error diagnostics and server-side peer/vCPU/reply logs are present; the five-second timeout and The ARM64 attribution is also supported by source inspection. In the current One interpretation in comment 6330986370 still needs narrowing: The current 20/20 CI result is confirmed, while keeping the toolchain/host limitation on the reported reproduction. This follow-up accepts the more specific core-attribution evidence; it does not assert that a green run repaired the baseline context-switch defect. |
| // was established by running against the kernel module (7.0). Where a fact is read | ||
| // out of the kernel source rather than measured, the citation names the function and | ||
| // not a line number, which moves without the fact moving; every such reading in this | ||
| // file is against Linux v7.2.3. |
There was a problem hiding this comment.
Answer to the [P3, non-blocking] keep source citations stable item of review 5568930695, and thanks — the merge-not-rebase reading is the right one. Done in 509344e. This was a top-level comment; it belongs here, at the line where the convention you asked for is now written down. You named thread/thread.cpp:2156 and thread/workerpool.cpp:64 in ublk.cpp, and unversioned kernel locations such as ublk_drv.c:4784 and blk-lib.c:37-46; this preamble is what replaced that shape, and the same pin sits once per file across blk/.
The other three findings of that review are answered in their own inline threads (4321784761 ublk, 4321785019 tcmu, 4344428258 vduse tombstone), and all four were confirmed against the source before any of them was fixed.
P3, citations. Done in 509344e, and swept rather than patched: git ls-files blk | xargs grep -nE '\.(c|cpp|h|cc|hpp):[0-9]+' is now empty, as is the same search for bare (:NNNN) refs. Kernel citations name a function, macro or struct, and the tree is pinned once per file — Linux v7.2.3, in ublk.cpp's "Operating facts" preamble, in utils.h's header, and at each site in the test files that cite one. The two photon-internal citations you named went away with the serving_leaked field they existed to justify — the pool-destruction chain they described is no longer load-bearing anywhere, because nothing is leaked on purpose now. What is still load-bearing is cited by function name instead of by line: thread_migrate refusing a thread that is not READY or not already on the caller's vcpu, defer_migrate_current returning success unconditionally with do_defer_migrate void behind it, and vcpu_t::move_to_standbyq_atomic as the path that makes thread_interrupt cross-vcpu safe. In-repo ones became names too — utils.cpp:148 is now "devlock_free() holds LOCK_EX for as long as the probe runs", and io/fd-events.h:33 is now photon::EOK. mutate-vhost-user.py's ledger also cited gtest failure line numbers in test-vhost-user.cpp; those went, since the case names and the assertion prose already identify them and the numbers were the part that would rot.
Converting them meant re-reading every claim against v7.2.3, so let me be precise about what that found, because it is not the drift I expected: the facts all held, and the line numbers were accurate against that tree. Three were slightly off and I have the specifics — block/blk-lib.c:37-46 stops one line short of the bio_alloc(bdev, 0, REQ_OP_DISCARD, gfp_mask) it was cited for, block/blk-flush.c:400-412 stops one line short of the REQ_FUA strip in blk_insert_flush, and the q_id division in ublk_ch_mmap is one line above where it was pointed. Everything else — __ublk_queue_cmd_buf_size, ublk_max_cmd_buf_size, the sz != ublk_queue_cmd_buf_size(ub) equality, both ublk_ctrl_add_dev checks, vduse_dev_update_iotlb, vduse_vdpa_set_map, VDUSE_BOUNCE_SIZE, vduse_create_dev's nas and bounce_size, vduse_domain_init_bounce_map, and the four virtio_ring.h sites — was exact. So the defect is the one you named rather than the one I would have guessed: nothing told a reader which tree those numbers were true of, and the next kernel bump would have moved them silently while every fact stayed put.
|
Two agreements and one correction that is mine, not yours. The peek: your narrowing is right, and on the second half it is stronger than you put it
Your second sentence — EAGAIN "only establishes an empty receive queue, not why the parser expected more bytes" — is also right, and the log line already answers the why without the peek. On that signature: At that stage So the peek is not an independent observation on either signature: on What put the transmission magic into an option reply's type field, I do not know, and I am not claiming these diagnostics establish it. That is the separately-tracked core question. Scope of the core fixAgreed, and that is how I have described it: the diagnosis comment says the attribution rests on the A/B and explicitly that I am not reading a green CI as a repair. Nothing here claims a repaired NBD transport. On "the context-switch code is unchanged by this PR" you are right — but the sentence I published around it was wider and was false, so: Correction:
|
|
Re: the NBD diagnosis response: I accept the narrowed explanation and the correction about The remaining P1 is vhost-user's socket-path ownership after detach. Here is the concrete lifecycle change requested.
The failing sequence is: Today A retains Please make these behaviors explicit:
Tests to change/add:
This is a request to end path ownership at detach, rather than preserve historical ownership and make every old-object cleanup guess whether it still applies. The earlier standalone AF_UNIX probe reproduced deletion of a live successor's path; the full Photon lifecycle regression still needs to be added. See the original finding. The delegated-work completion P1 remains withdrawn: no supported active cancellation path was established. No changes are requested for that item. |
detach() hands the socket node to whoever binds it next, but it left sock_owned set, and the two calls that read that flag did so outside any test for whether this object was still the one serving. shutdown() called release_sock_node() after its `started` test rather than inside it, and the destructor called it unconditionally, so A.start -> A.detach -> destroy_orphan -> B.start -> A.shutdown (or delete A) unlinked B's live node: B's frontend could no longer connect while B was still answering. The refused-restart half is the same flag through a different site: A's second start() fails on the occupied path with EEXIST, and rollback() then unlinks with whatever sock_owned still said -- true from A's FIRST session, which no longer owns anything. The comment above rollback() asserted that sock_owned is false when do_listen refuses, which only held for an object that had never owned the path. The old shape was deliberate and its reason is still real: keying the unlink on `started` meant start -> detach -> shutdown could not remove a node this device owned, so the same object could not start again. Both readings come from making one call site answer two questions. So the two public end-of-session calls now differ by an explicit argument: end_session(wait_pending, release_node). detach() passes false and calls abandon_sock_node(), which leaves the socket file and clears the right to delete it; shutdown() passes true, and only while `started`, so a session that is still serving still removes its own node and orderly shutdown does not become a leak. rollback() needs no new test: do_listen() is the only setter and every path that ends a session clears the flag, so a refused bind reads false even on an object that owned the path earlier. blk.h now states the contract -- the node stays, the detached object gives it up, and clearing it is destroy_orphan()'s job, because deciding whether a node may go needs the probe and the identity lock that only the recovery path takes. The cost is the one the previous reading was buying: start -> detach -> shutdown -> start on the same path now needs destroy_orphan() in between. That is the intended trade -- the recovery call is where a path nobody serves may be removed -- and the case that pinned the old behaviour was replaced rather than kept. Tests: shutdown_after_detach_leaves_the_node_for_destroy_orphan (the detached half plus a still-serving shutdown that must remove its node), a_detached_object_must_not_delete_the_successors_node (both of A's exits, each followed by a real connect-and-negotiate rather than a stat), and a_refused_restart_after_handover_keeps_the_successors_node. The mutation ledger gains two entries that partition the coverage: norelinquish reddens the destructor assertion and the refused-restart case but NOT the first, whose subject is the leave/remove split; noleavenode reddens the first and the shutdown assertion and leaves the refused-restart case green. Verified on aarch64 (kernel 7.0.0-38-generic, MinSizeRel/C++17): 87/87 in test-blk-vhost-user, zero build warnings; macOS full build clean, the vhost-user target being LINUX-gated. mutate-vhost-user.py check: 31 mutants, all anchors resolve. No other transport changed.
Those three suites gate every case on a kernel-side prerequisite in their own SetUp -- tcmu on a mounted configfs target plus target_core_user and tcm_loop, ublk on /dev/ublk-control after a modprobe of ublk_drv, vduse on /dev/vduse/control after modprobe of vduse -- and skip when it is missing. The skip is a LOG_INFO line: report_skip() never tells gtest, because CI's gtest 1.8 has no GTEST_SKIP, so ctest counts the case as passed. On this head that is 148 of the 164 tcmu/ublk/vduse cases per engine pass, three passes per job, and the suites still print "62 passed" / "50 passed" / "52 passed". Not a sudo problem: both x86_64 job logs have zero "requires root" skips, so geteuid() was already 0 and the gates that fired were the module ones. What was missing is the modules, so this loads them, mounts configfs if it is not, and ensures the vdpa tool that vduse's fourth gate looks for. The step prints what actually came up per module and how many of the three prerequisites are present, and emits a GitHub ::warning:: when none is -- that way a run whose suites were fully skipped cannot be read as one that exercised them. It is not fatal, because an image without these modules still runs the other ~230 cases and should keep doing so. Inert until a runner has the modules; the first run's log will say which. The ARM workflow is untouched: its Test step is gated on matrix.run_test and no matrix entry sets it, so there is no test step to prepare today.
|
Review 看到一个构建兼容性问题，建议修一下： [P2] 默认开启 blk 会破坏 Windows 构建 CMakeLists.txt:237–242 中， 建议先给 以上基于 |
blk/ is POSIX and the switch defaults to ON, so a default Windows configure compiled blk/nbd.cpp and blk/utils.cpp and failed on them: utils.cpp's device lock is flock() behind <sys/file.h> and <unistd.h>, its socket-side helpers use <sys/un.h>, and the Windows compatibility layer shims none of those. nbd.cpp carries the same unconditional <sys/socket.h>/<fcntl.h>/<unistd.h> includes, so neither file could be repaired on its own -- the constraint belongs to the subtree. It is applied to the switch rather than to the two modules every transport needs, for two reasons. PHOTON_MODULE_blk_ENABLED is deliberately equal to PHOTON_ENABLE_BLK, so a per-module clause would make that equality false and the test tree would have to restate the platform rule. And photon_option() is a plain set(... CACHE ...), which does not override a value the caller passed: defaulting the option to OFF on Windows would still let an explicit -DPHOTON_ENABLE_BLK=ON through and fail the same way. FORCE, with a STATUS line saying why, is what "never on Windows" has to mean. Also corrects the comment in blk/utils.cpp that claimed sections 2 and 3 both sit inside #ifdef __linux__. The device lock at the head of section 2 does not, which is exactly why its flock() call compiles unconditionally and why the module is not portable; the comment now says so and names the module's gate. Verified on this machine (macOS, MinSizeRel, C++17): a simulated WIN32 configure with -DPHOTON_ENABLE_BLK=ON ends with the switch OFF and the STATUS line printed; the normal tree still builds all 4 test-blk targets with zero errors; and -DPHOTON_ENABLE_BLK=OFF configures and builds clean with no blk test target at all. No MSVC here, so Windows itself is not compile-verified -- what is verified is that blk no longer enters that build.
|
确认，两条都成立；而且有一处比你写的更硬，还有一处你没点但决定了修法。 更硬的那处： 你没点但决定修法的那处： 修法选在你的方向上，但落在开关上：
落地（ 验证：
没验证的：这台机器没有 MSVC，Windows 本身没有编译过。我能证明的是「blk 不再进入 Windows 构建」，不能证明「Windows 上其余部分本来就没问题」——如果你那边有 MSVC 环境，这一条还要你确认一次。 |
The step that loads ublk_drv / vduse / target_core_user and their companions lived
inline in ci.linux.x86_64.yml, after the build, and it installed iproute when the
vdpa tool was missing. A test step should not install packages: it makes the run
depend on the network and on a package repo, it mutates a machine other jobs
share, and it turns "the tool is absent" into a green-looking "installed vdpa
tool" -- which is the opposite of what this step exists to report. The inline copy
was also invisible to anyone running the suites by hand.
blk/test/load-kernel-modules.sh now holds it: load the six modules, mount configfs
when it is not mounted, print the three prerequisites the suites check plus the
vdpa tool, and emit a warning when none of the three came up. It installs nothing
and is never fatal. The workflow runs it before the build, so a run's log states
what the suites will be able to do before anything is compiled.
Verified on the Linux VM from a genuinely unloaded state. Only tcm_loop and
target_core_mod were left -- tcm_loop cannot be unloaded, its configfs fabric
registration pins it and through it target_core_mod -- and neither ublk's nor
vduse's device node existed. The script loaded ublk_drv, vduse, virtio_vdpa and
target_core_user, recreated /dev/ublk-control and /dev/vduse/control, and
reported prerequisites present: 3/3 with rc=0; a second run is a no-op and the
machine is left with no vdpa devices and no D-state. On macOS it prints "not
Linux, nothing to load" and exits 0.
Known consequence, not addressed here: on the CI nodes these modules do load now,
and that is what arms the tcmu suite's gate. It then fails on open("/dev/uio1")
with ENOENT, because a module's char device node is created in the host's
devtmpfs and the job's /dev is the container's own tmpfs. The fix belongs to the
suite's prerequisite check -- it should probe for a usable uio node rather than
for the modules alone -- and is a separate change.
The tcmu suite gated on root, a mounted configfs target, target_core_user and
tcm_loop, and all four can hold in a container that still cannot run it. The
transport serves through /dev/uioN, which the uio framework creates for an enabled
backstore; the node lands in the host's devtmpfs, so a job whose /dev is its own
tmpfs loads the modules, passes every gate, and then fails all 62 cases at
open("/dev/uio1") with ENOENT. The CI image is that container, and the failure
reads as a product one.
There is no control node to test instead. target_core_user registers a configfs
backend ("TCM-USER") and a genetlink family ("config") and no character device --
/dev/tcmu-control does not exist in mainline, and /proc/misc has no tcmu entry --
so the fixed-path gate ublk and vduse use (/dev/ublk-control, /dev/vduse/control)
has no counterpart here. What those two checks actually test is whether a node the
kernel creates is visible in this process, so this gate asks that directly: is
/dev a devtmpfs mount.
Verified on the VM, where /dev is devtmpfs: the gate passes and the suite is
unchanged at 62/62 with nothing skipped. The parse was exercised in both
directions against stand-in mount tables -- a tmpfs /dev skips, a devtmpfs /dev
does not -- because the real one cannot be remounted to test.
A kernel without TRY_STOP_DEV answers that command with -ENOTSUPP (524 up to Linux 6.12), and the fallback calls STOP_DEV, which on a live device waits for the last opener to close. The contract for that case is EBUSY with nothing torn down, but a caller that holds the node -- an operator shutting down while an fd is open -- then waits for the call that is waiting for it. Measured on the CI nodes: shutdown_busy sat there until ctest's 3600 s timeout, and the case passes on kernels that have TRY_STOP_DEV. Ask the question the kernel will not: an exclusive open of the node fails with EBUSY exactly while any opener exists, and closing the probe changes nothing. One syscall, no TRY_STOP needed, and it restores the EBUSY the contract promises on kernels that lack the command. Verified on the VM: O_EXCL on an already-open block device returns EBUSY (16), and the ublk suite is unchanged at 52/52. That kernel has TRY_STOP_DEV, so it does not take the new branch -- the branch is exercised by the CI nodes, which is where the hang was.
…in apart modprobe is the wrong first question. Some CI nodes have several of these drivers built into the kernel: modprobe answers "Module not found" while the device nodes and configfs entries are all there, so the step printed six MISSING lines next to "prerequisites present: 3/3" -- an answer that is both wrong and self-contradicting, and one that sent a run's failure looking at code that was fine. sysfs answers it without touching anything: /sys/module/<name>/refcnt exists for a loaded module, /sys/module/<name> exists without it for built-in code, and neither exists when the driver is genuinely absent. Only the third case runs modprobe, and only that line can print FAILED, with modprobe's own words. Verified on the VM: with all six loaded the script runs modprobe nowhere; after rmmod ublk_drv it loads exactly that one and /dev/ublk-control reappears; the built-in branch was checked against a driver with a /sys/module entry and no refcnt.
A 6.6 kernel has neither UBLK_CMD_TRY_STOP_DEV nor UBLK_CMD_UPDATE_SIZE, so shutdown() on a held node waited inside STOP_DEV for the holder instead of refusing it (CI sat there for the whole ctest timeout), and resize() failed on a command the driver does not implement. Both answers are already in the ADD_DEV/GET_DEV_INFO flags -- the driver masks off whatever it does not implement -- so start() reads them back instead of claiming the transport's best case on every kernel. shutdown_refusal gains the honest fourth value, WaitsForAttached, and the resize cases assert the EOPNOTSUPP refusal before skipping on such a kernel. The adoption path now validates the registration before START_USER_RECOVERY, which resets kernel-side queue state that a refused adoption has no way to undo; the recovery failure log also names the state and flags it saw.
…EXCL Every timeout in blk/ is now a uint64_t in microseconds with no unit suffix, -1ul meaning "no deadline" (0 is a real zero-length deadline), and the kernel attributes that still count in seconds are converted upward at the boundary so a sub-second value cannot silently become the kernel's 0. The three ublk knobs (quiesce / stop / control) collapse into one BlkConfig::mgmt_timeout: they were all answering "how long may a lifecycle call wait on the kernel", and a caller only had a coherent configuration when it set all three. It bounds each control-command round-trip, each poll for a kernel-side state change, the TRY_STOP retry window and the initial-fetch token. The block node's "is anyone holding it" probe is gone for good, and blk.h says why: an O_EXCL open claims the device in the block layer before its partition scan, and a task that dies in between leaves bd_claiming set with no owner -- every later O_EXCL open of that device then waits uninterruptibly forever while holding the reference that DEL_DEV needs. That cost a VM; ublk's own answer is TRY_STOP_DEV, or shutdown_refusal == WaitsForAttached where the kernel has none.
Two runs in a row burned the whole 24-hour run limit on the x86_64 test steps and were cancelled with their logs purged. Each ctest step now carries a 60-minute wall-clock cap, so a hang costs one failing step and leaves the log that names the case. test-blk-* is excluded from the ctest runs until the ublk hang is settled. The suites are still built -- only their registration is skipped -- so blk keeps its compile check in every job. Remove the -E to re-enable them.
The macOS arm image's X509_get_subject_name() returns a const X509_NAME*, while X509_NAME_add_entry_by_txt() and X509_set_issuer_name() take a mutable one, so client_function_test stopped compiling there -- the intel image's OpenSSL returns the mutable pointer and was fine. The certificates are ours and writable, so the five call sites go through one helper that takes the writable view back.
The getter's return qualifier follows whichever headers are installed, so the fix must not key off a version: removing no qualifier is a no-op, which is what lets the single helper compile unchanged against the mutable and the const signature.
…res" This reverts commit 905c6e5.
…rtificates" This reverts commit f62ccfe.
including tcmu, ublk, nbd, vhost-user, vduse
add virtual block device servers, including tcmu, ublk, nbd, vhost-user, vduse
What this adds
Five ways to export a photon
fs::IFileas a block device, behind oneblk.hinterface:nbdtcmutarget_core_user, via netlink + configfsublkvdusevhost-uservduseandvhost-usershare a virtio-blk core inblk/utils.cpp, so thesplit-ring serving engine, the descriptor walk and the queue lifecycle exist once
rather than twice. Each transport is its own
photon_module()with its ownENABLE,so a platform builds only what it can support.
Operator tools
Two executables are built alongside the transports they drive rather than under
PHOTON_BUILD_TESTING, because they are useful in production and not only in tests:vduse-cli—probecreates and dumps its own device. The vduse uapi isthinly documented and its behaviour had to be measured rather than assumed, so
this is the ABI oracle: re-run it against a new kernel before changing
blk/vduse.cpp's protocol code.rescueadopts somebody else's wedgedregistration and drains every queue it has;
destroyissuesVDUSE_DESTROY_DEVwithout serving. A vduse registration with no daemon serving it wedges anything
that touches its
/dev/vdXin unkillable D state, and the kernel has no timeoutfor it —
rescue+destroyare the drill, and the file's header records themeasured recovery order.
vhost-user-cli— serves one image over a vhost-user socket so that anindependent frontend can be pointed at
blk/vhost-user.cpp.Each tool is gated on its own transport's enable state
(
PHOTON_MODULE_<name>_ENABLED), so it is built exactly when the transport it drivesis, and follows that transport if its gate ever narrows. Both link
photon_static,so either binary can be copied onto a wedged machine with no
libphoton.soand norpath to satisfy. Neither is registered with CTest: both need arguments,
vduse-clineeds root, and neither is a test.
They land in
<build-dir>/output/beside every other executable and are ordinarymembers of the default
alltarget, so a downstream that consumes photon viaadd_subdirectorybuilds them too.vduse-clitakes its uapi from the vendoredblk/vduse-uapi.hrather than<linux/vduse.h>, for the reason that header gives:it arrived in the kernel later than many build hosts' header packages, so a
translation unit that includes it does not compile there at all. That only started to
matter once the file became a build target — measured in
almalinux:8, the image theRocksDB CI job runs in,
<linux/vduse.h>is absent and the unincluded version failsto compile under gcc 8.5.0.
Testing
Eight suites under
blk/test/:harness,pool,nbd,vq,tcmu,ublk,vhost-user,vduse.blk/test/test-vhost-user.cppdrives a mock frontend. A mock necessarily shares thisrepository's reading of the wire format, so it cannot catch a format both halves got
wrong the same way. Two independent checks sit beside it:
blk/test/qemu-vhost-user-peer.pypatches QEMU's owntests/qtest/vhost-user-blk-test.cto accept our backend in place ofqemu-storage-daemon— same frontend, same libqos driver, only the peer swapped,which makes its six cases an A/B baseline.
blk/test/qemu-vhost-user-qtest.pyruns those six cases and reports evidence percase instead of
ok/not ok. The reason is measured: three of the six reportokwhile QEMU never sendsSET_MEM_TABLE,SET_FEATURESorSET_VRING_*at all— the frontend connects, drops, reconnects, drops again and is SIGTERMed, so no
ring goes live and no byte is served. Their green is QEMU's verdict on QEMU's
assertion and says nothing about this repository. So the driver counts our backend's
own log lines, parses qos-test's TAP plan (a path that matches nothing prints
1..0and still exits 0), and designates
basicas a control — if the control produces noevidence the driver declares itself blind rather than reporting six zeros as a
result.
Neither is a CTest case: both need a hand-built QEMU tree. They are manual, and they
exit non-zero on a missing prerequisite rather than skipping quietly, because a suite
that reports success without having run is worse than no suite.
What CI does not execute
The
ublk,vduseandtcmusuites need kernel modules (ublk_drv6.0+,vduse5.15+,
target_core_user) and root. CI's runners are unprivileged pods on a 5.10kernel whose
/lib/modulescontains none of the three, and there is novdpatool,so those cases are not executed by CI. This was probed rather than assumed, and it is
not fixed by
--privileged: a privileged GitHub-hosted runner's image kernel stilllacks the modules. Those suites are verified on a Linux VM that has them.