oxidecomputer / oxidecomputer/propolis
NVMe devices don't tolerate migration
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 270
- Forks
- 42
- Avg merge
- 4d 5h
- Merged PRs (30d)
- 6
Description
there are at least two bugs here, one much more obvious than the other.. I was trying to tickle a different but I suspect exists, but have been stymied by these for the time being.
I got here via ./propolis-standalone -s ubuntu.22.04.5.toml, ctrl+c, and ./propolis-standalone -r <snapshot_tar> with a file-backed disk and a mem-async disk, but the disk types shouldn't really matter (both are wedged)
issue one is that NVMe devices now have an is_enabled to avoid locking NvmeCtrl to read ctrl.cc on hot paths. that's great, except when we import a device we don't peel out is_enabled again, so the imported controller has cc declaring it's enabled, but is functionally disabled. attempting to submit I/O (when run via propolis-standalone) then gets you nvme reg r/w failure, error: the submission queue specified (4) is invalid, register: IOQueueDoorBells, offset: 4128 in ring_doorbell.
adding a
if ctrl.ctrl.cc.enabled() {
self.is_enabled.store(true, Ordering::Releae);
}
gets you past that and onto the second issue, which I've not debugged at all: I/Os seem to just not get acked (maybe not serviced?) on a migrated controller. it may be relevant that I've also got a local patch for const MAX_NUM_QUEUES: usize = 5; but I've not tested. we'll see how https://github.com/oxidecomputer/propolis/commit/cc43ca00c81e45d35a59155a5e17084bcff878d2 fares!
edit/ps: #966 didn't trip over this because we use state.ctrl.cc.enabled() on the admin SQ/CQ doorbells, but self.is_enabled on the I/O doorbells, and the rudimentary driver there doesn't actually do I/O. so.. probably worth making it do some I/Os..
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Reproduce migration with propolis-standalone using the file-backed and mem-async disks, then inspect lib/propolis/src/hw/nvme/mod.rs around ring_doorbell and the import path near line 1371. Verify that an imported controller accepts I/O and that migrated I/Os are acknowledged, including coverage for the I/O doorbells rather than only the admin queues.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- operating-systems
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100