nvme_sim: Attach as a child of nvme
ClosedPublic
Actions

Authored by imp on Jul 17 2025, 7:51 PM.

Details

Reviewers

jhb
mav
chs
chuck
ken

Commits

rG1e39b5d4833e: nvme_sim: Attach as a child of nvme

Summary

Rather than registering as a consumer of the nvme controller, hook into
the child device and use that.

This is a small regression at the moment: we don't fail the device when
that happens at runtime, and we don't handle new namespaces when they
arrive (though that feature is currently fragile).

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 65530
Build 62413: arc lint + arc unit

Event Timeline

imp created this revision.Jul 17 2025, 7:51 PM

Herald added a subscriber: dab. · View Herald TranscriptJul 17 2025, 7:51 PM

imp requested review of this revision.Jul 17 2025, 7:51 PM

imp added a parent revision: D51383: nvme: Add child device for each controller.

imp added a child revision: D51385: nvd: Attach as a child of nvme.

Harbormaster completed remote builds in B65530: Diff 158691.Jul 17 2025, 7:52 PM

jhb added inline comments.Jul 23 2025, 6:50 PM

sys/dev/nvme/nvme_sim.c
374	Have you thought about defining a nvme_if.m and having a `nvme_ns_changed` DEVMETHOD? You could then handle enumerating namespaces in the controller driver and just invoke the method in the child device each time a namespace changes. (FWIW, nvmf(4) does handle namespaces coming and going dynamically using an AER to notice when a namespace changes. It works in that you can use ctladm to add new LUNs on the fly on the target side, and the FreeBSD nvmf client will notice the new namespace.)

I also don't understand the regression described in the commit message. (What do you mean by "that" in "failing devices when that happens at runtime"? Is "that" an AER, is it an I/O error, completion timeout?)

BTW, should this mean that you can kldunload nvme_sim and then kldload nvd to switch from nda to nvd? Or adjust the tunable in kenv and then do a 'devctl detach/attach' of the device? (Granted, I don't think devctl can name this device for attach to work)

In D51384#1175594, @jhb wrote:

I also don't understand the regression described in the commit message. (What do you mean by "that" in "failing devices when that happens at runtime"? Is "that" an AER, is it an I/O error, completion timeout?)

BTW, should this mean that you can kldunload nvme_sim and then kldload nvd to switch from nda to nvd? Or adjust the tunable in kenv and then do a 'devctl detach/attach' of the device? (Granted, I don't think devctl can name this device for attach to work)

This commit regresses calling the failure callback that the 'consumer' interface has when the controller fails (and calls the right failure routine). One could, in theory, attach different types of nda / nvd devices after this change, but I've not tested to see if it breaks things... nvme_sim isn't a loadable module, it's co-located in the nvme module when cam is enabloed. These things should be possible, but are not likely to be seamless just yet..

sys/dev/nvme/nvme_sim.c
374	There's a future review that does exactly this sort of thing. nvd doesn't handle dynamic namespaces at all... That future change will allow them to arrive. nda allows them to come, but not depart or change

jhb added inline comments.Jul 23 2025, 7:48 PM

sys/dev/nvme/nvme_sim.c
374	Hmm, nda handles it fine for me with nvmf when I've tested it. At least removing devices does work.

imp added inline comments.Jul 23 2025, 7:50 PM

sys/dev/nvme/nvme_sim.c
374	Hmmm, reading the code suggests it shouldn't... But maybe I fixed this case better than I'd thought when I was working on dynamic namespace support.... Adding works I know, and removing and adding back kinda works... I didn't think I had all the kinks out of simply removing though.