Repository navigation
Port the remaining v1.14.4-fly patches to v1.17.0 - #40
Merged
Merged
Conversation
Bring the Threaded engine to its final v1.14.4-fly form, 01c0d7c: the worker thread serves the drive's virtqueue itself (pops, parses, rate-limits, does one preadv/pwritev per request, fills the used ring and raises the interrupt), instead of executing requests handed to it over a channel with completions back through an eventfd. The event loop only sends it control messages. Adapted to v1.17.0: - Discard requests are served by the worker like the others, with the Sync engine's file_discard_range, and only when the guest negotiated VIRTIO_BLK_F_DISCARD; otherwise they complete as unsupported, as with the other engines. The block_io seccomp filter allows the fallocate and BLKDISCARD calls that needs, and the fstat (newfstatat on aarch64) it does first to tell a block device from a regular file. - Device setup now reads the backing file's block attributes through FileEngine::file(), so the engine keeps the backing file and the worker serves requests with its own duplicate. A drive update hands the new file's fd to the worker, which duplicates it, since the VMM thread's seccomp filter forbids that after boot; the block_io filter allows fcntl(F_DUPFD_CLOEXEC) for it, and the fcntl(F_SETFD, FD_CLOEXEC) musl follows it with. The engine then drops the old file. - Tests follow the RateLimiter::new, VirtioBlockConfig and bitcode snapshot API changes, and cover discard through the worker.
…p filter The VMM thread runs under the "vmm" seccomp filter after boot, but unit tests never apply it, so a system call the filter does not allow only shows in production. That is how updating a Threaded drive through PATCH /drives once killed Firecracker with SIGSYS: the update cloned the backing file with fcntl(F_DUPFD_CLOEXEC), which the filter did not allow. Test what the VMM thread does with a Threaded drive after boot, under the "vmm" filter of the default policy, compiled from the JSON at test time: activation and the kick that hands the queue to the worker, a disk update followed by a write past the end of the old disk, a rate limiter update, reading its configuration, a snapshot, the kick on resume and a write after it, and dropping the drive. A system call the filter does not allow kills the test with SIGSYS, and a handler names it. With the file clone put back into the disk update, the test fails on fcntl. The test lives in the firecracker crate, which already depends on seccompiler to compile the default policy, so it adds no dependency. The default policy is written for musl, so the test only runs on musl targets, which is where the unit tests run in CI. In debug builds, which the unit tests are, the standard library checks each file descriptor it closes with fcntl(F_GETFD); the test allows that one call on top of the policy, which is written for release builds. [v1.17.0 port: seccompiler::compile_bpf takes a split_output argument, and RateLimiter::new no longer returns a Result.] (cherry picked from commit e71fe09)
The block device takes one data buffer per request, so a guest has to send one request per physically contiguous piece of memory. Page cache writeback is made of 4 KiB pages that rarely are: a buffered 32 KiB write followed by fdatasync reaches the device as eight requests, each with its own system calls, completion and rate limiter token. With the Threaded engine, offer VIRTIO_BLK_F_SEG_MAX with seg_max = 32: - A read or write may have up to 32 data descriptors between its header and status. Their total length is what has to be a multiple of the sector size and fit the disk, and what the rate limiter accounts for, as one operation. - The worker serves such a request with one preadv or pwritev over all its buffers, and marks every buffer of a read dirty. - The config space grows to include seg_max for these devices only. The Sync and Async engines offer neither the feature nor the longer config space, and parse requests as before. A device restored from a snapshot taken without the feature keeps taking one buffer per request. Measured with the Threaded engine before its worker served the queue itself, with a guest doing buffered 32 KiB sequential writes, each followed by fdatasync, on a backing file that adds 2 ms per request: 56 -> 415 writes/s, with one 32 KiB request per write where there were eight of 4 KiB. [v1.17.0 port: upstream's config space now runs to the discard fields for every engine, seg_max included, so the separate longer config space and its read_config hook are gone: Threaded devices set seg_max in the shared one, at creation and on restore, and the other engines read it as zero.] (cherry picked from commit 65921c0)
…21) Snapshotting saves the vCPUs, the irqchip and the MMIO transport before the block device's prepare_save stops its worker, and stopping waits for the request in flight. A request completed in that window reaches the used ring, which is saved with guest memory, but its interrupt is not saved anywhere. On restore the worker starts serving, finds nothing new and raises no interrupt, and with VIRTIO_RING_F_EVENT_IDX the guest's used_event stays behind, so every later interrupt is suppressed too: the guest never sees a completion again and its writers hang in D state. Raise the queue interrupt whenever the worker starts serving, after it has served what was queued. A spurious interrupt is harmless. (cherry picked from commit cb829eb)
(cherry picked from commit 270f841)
pylint 4 reports four things pylint 3 did not: - A module-level name assigned twice is a variable, so DOC_ONLY_CHANGE in pipeline_pr.py fails the snake_case check. Assign it once. - Three CPU feature checks compare against the same bound twice; write them as chained comparisons (chained-comparison). - A disable comment on its own line above an except clause no longer covers it, so HostEchoWorker.run reports broad-exception-caught. Put the disable on the except line. None of these change behaviour, and pylint 3 accepts them too. [v1.17.0 port: three more in code that is new since v1.14.4, the same chained-comparison twice (test_vulnerabilities, test_topology) and a constant whose name pylint 4 no longer takes as UPPER_CASE (EC2_CMR8i_VIRT_UNAVAILABLE becomes EC2_CMR8I_VIRT_UNAVAILABLE).] (cherry picked from commit 22cc066)
Refresh tools/devctr/poetry.lock so every package with an open advisory reaches its patched release, and raise the pyproject.toml floors the advisories name: - black ^26.10.1 (from ^26.3.1) - pytest ^9.0.3 (from >=8,<10) - pylint ^4.0.9 (from ^3) - filelock >=3.20.3,<5.0.0 (from ^3.20.3), so that it can reach 4.x Locked versions of the remaining advisory packages: pillow 13.0.0, aiohttp 3.14.5, urllib3 2.8.0, certifi 2026.10.23, multidict 7.2.1 and sh 2.4.0, with filelock at 4.2.9. Every package in the lock is clear of OSV advisories as of 2026-11-04. sh could not move while depending on gitlint: that package installs gitlint-core[trusted-deps], whose extra pins sh to 1.14.3 (GHSA-q38v-wp89-2w55) and click to 8.1.3. Depend on gitlint-core directly instead; it provides the same gitlint command and library without the exact pins. The click pin existed only to match gitlint's, so it goes too. The lock was generated with Poetry 2.4.1 on Python 3.12, in the devctr. This folds two v1.14.4-fly commits, d76483d and d96b9fc (the filelock 4 bump), redone against v1.17.0's dependencies.
The test applied the "vmm" filter to the VMM thread only, so the drive's worker, which applies the "block_io" filter in production, ran unfiltered, and a system call that filter does not allow would only show in production. Install the "block_io" filter of the default policy for the worker too, before the drive is created, as Firecracker does, and add a discard to the requests the test makes: with the drive update before it, that covers the calls the block_io filter gained for v1.17.0, the fstat, fallocate and BLKDISCARD of a discard and the duplication of an updated backing file. Dropping fstat from the filter has the test fail on it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brings
v1.17.0-flyup to every Fly change carried onv1.14.4-fly. The branch already had 8 of the 20v1.14.4-flypatches. This PR brings the Threaded engine to its final form and ports the rest. Each port is adapted to v1.17.0 rather than cherry-picked blindly.e0c130032is afixup!ofvmm: add a Threaded block IO engine(8a1f18f), which is already onv1.17.0-fly. Merge without squashing the branch, and the fixup can be folded in when the branch is next rebased.Each v1.14.4-fly patch
e0c130032brings it to the final form, where the worker serves the virtqueue itself. Details below.28f682033):compile_bpftakessplit_output, andRateLimiter::newno longer returns aResult. Extended in8c5ad3948, see below.ea33a4280): upstream's config space now runs pastseg_maxfor every engine, so the separate longer config space and itsread_confighook are gone. Threaded devices setseg_maxin the shared one, at creation and on restore.d639dee30), applied cleanlyad70e2e88), on top of v1.17.0's CODEOWNERS01f53dcce), plus three new pylint 4 findings in code added since v1.14.4: two chained comparisons, andEC2_CMR8i_VIRT_UNAVAILABLErenamed toEC2_CMR8I_VIRT_UNAVAILABLEa34405d80), with the lock regenerated against v1.17.0's dependencies. At v1.17.0's lock, OSV still flags aiohttp, black, certifi, filelock, multidict, pillow, pylint, sh and urllib3. After this PR nothing in the lock is flagged (checked 2026-11-04).a34405d80: filelock is allowed up to <5 and locks at 4.2.9test_serial_active_tx_snapshotis in v1.17.0Not included: the open NBD engine (#37) and vhost-user-blk snapshot (#38) PRs against
v1.14.4-fly.Threaded engine on v1.17.0
file_discard_range, and only when the guest negotiatedVIRTIO_BLK_F_DISCARD. Otherwise the request completes as unsupported, as it does with the other engines.FileEngine::file(): v1.17 device setup reads the backing file's block attributes through it, so the engine now always keeps the backing file, and the worker serves requests from its own duplicate. A drive update passes the new file's fd to the worker, which duplicates it, because the VMM thread's seccomp filter does not allow that after boot. The engine then drops the old file, so the old image isn't held open.block_ioseccomp filter: gainsfallocate(punch hole) andioctl(BLKDISCARD)for discard, plus thefstat(newfstataton aarch64) thatFile::metadatamakes first. Thatfstatwas missing in the earlier rebase, so a discard on the worker would have hit SIGSYS. It also gainsfcntl(F_DUPFD_CLOEXEC), and thefcntl(F_SETFD, FD_CLOEXEC)musl follows it with, for drive updates.8c5ad3948): the test now runs the worker under theblock_iofilter as well as the VMM thread undervmm, and adds a discard after the drive update. It found the missingF_SETFDrule above, and fails onfstatif that rule is removed.Testing
cargo build --all-targets;cargo clippy --all --all-targetshas only the two warnings already onv1.17.0-fly(vsock/unix/muxer.rs:359andjailer/src/env.rs:421), in files this PR doesn't touch;cargo fmt --checkpasses.cargo test -p vmm -p firecracker --target x86_64-unknown-linux-musl, in the v93 devctr, privileged, withRUST_TEST_THREADS=1as CI runs it: everything passes excepttest_setup_sregsandtest_preboot_load_snap_disallowed_after_boot_resources. Both fail the same way on the base branch on this host, which is a nested-KVM guest. This includes the 17 Threaded engine unit tests (cargo test -p vmm --lib threaded), among them a new discard test and newfile()-after-update andseg_max-after-restore assertions.threaded_block_seccomppasses on x86_64 musl. Both seccomp JSONs compile withseccompiler-binfor their arch, but the aarch64 filter has not been exercised at runtime.tests/ tools/ .buildkite/*.py.test_read_writeandtest_end_of_regionfail on shared drive metrics, on the base branch as well. Upstream CI runs single-threaded.