Page MenuHomeFreeBSD

camcontrol: Support NVMe reservations via the persist subcommand
Needs ReviewPublic

Authored by ken on Wed, Sep 9, 3:34 PM.
Tags
None
Referenced Files
F174200842: D59534.diff
Thu, Oct 1, 8:03 AM
Unknown Object (File)
Tue, Sep 29, 9:43 AM
Unknown Object (File)
Tue, Sep 29, 1:01 AM
Unknown Object (File)
Tue, Sep 29, 12:52 AM
Unknown Object (File)
Mon, Sep 28, 2:46 PM
Unknown Object (File)
Mon, Sep 28, 5:38 AM
Unknown Object (File)
Sun, Sep 27, 7:57 PM
Unknown Object (File)
Sun, Sep 27, 9:32 AM
Subscribers

Details

Reviewers
None
Group Reviewers
cam
Summary

The NVMe reservation model is closely patterned after SCSI Persistent
Reservations, so support NVMe devices through the existing persist
subcommand rather than adding a transport-specific subcommand. The
persist subcommand now dispatches on the device protocol, in the same
way the identify subcommand does.

The SCSI syntax is unchanged. For NVMe devices, the -i modes all map
to the Reservation Report command (read_full_status requests the
extended data structure, which contains 128-bit Host Identifiers), and
the -o service actions map to the corresponding NVMe reservation
operations: register, register_ignore, reserve, preempt,
preempt_abort, release and clear, plus two NVMe-only actions,
unregister and replace. The reservation type names for -T are shared
between SCSI and NVMe. A new -c option gives explicit control over
the NVMe Persist Through Power Loss State; -p is equivalent to
-c enable. SCSI-only options return an error for NVMe devices.

Tested against a PASCARI X200 series SSD (reservation capabilities:
WR_EX, PTPL, IEKEY13): register, reserve, release, replace,
unregister, -c enable/disable (Persist Through Power Loss State change
verified in the report output), reservation report, and the error
paths. register_ignore is rejected by drives that implement NVMe 1.3
IEKEY semantics, as expected. SCSI persist against a da(4) device is
unchanged.

sbin/camcontrol/nvres.c:

New file, the NVMe reservation backend for the persist
subcommand.

sbin/camcontrol/camcontrol.c:

Dispatch the persist subcommand to scsipersist() or
nvmepersist() based on the device protocol.

sbin/camcontrol/camcontrol.h:

Add the nvmepersist() prototype.

sbin/camcontrol/Makefile:

Add nvres.c to the build.

sbin/camcontrol/camcontrol.8:

Document NVMe support in the persist subcommand section, and
note the differences from SCSI where they occur.

Co-authored-by: Reid Linnemann <reidl@spectralogic.com>
Sponsored by: Spectra Logic

Depends on D59532
Depends on D59533

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 77425
Build 74308: arc lint + arc unit

Event Timeline

I'd have also broken this up by subcommand because all together the review length is starting to get hard to review.
And it's unclear what the actual syntax here is too. It's a case where the old-school camcontrol commands are getting in the way of having commands that are more similar to linux's nvme cli. Translating between what we do and what they do is a lot of friction for no benefit.

sbin/camcontrol/camcontrol.8
372

why nvres? Why not just reserve? nv is weird to people that are used to other commands.
nvme cli doesn't have any prefixes and has these as separate commands, for example. It prefixes all of them with resv, which is available in camcontrol

In D59534#1366238, @imp wrote:

I'd have also broken this up by subcommand because all together the review length is starting to get hard to review.
And it's unclear what the actual syntax here is too. It's a case where the old-school camcontrol commands are getting in the way of having commands that are more similar to linux's nvme cli. Translating between what we do and what they do is a lot of friction for no benefit.

I think it would make more sense to fold the NVMe reservation command line syntax into the existing SCSI reservation syntax. I had Claude re-work the patch to support both SCSI and NVMe through the "persist" subcommand. SCSI operation is unchanged, and there are notes in the man page where NVMe is a little different.

This has some testing, but I wouldn't consider it fully baked yet. I won't be able to get back to this until at least next week, but I'll go ahead and upload the patch now so you can see the idea and try it out if you like.

sbin/camcontrol/camcontrol.8
372

Well, in this case it would make more sense to do what camcontrol is already doing for reservations. And that would be "persist", for persistent reservations (as opposed to SCSI-2 reserve/release). The NVMe reservations are modeled after the SCSI persistent reservations, with some differences.

ken retitled this revision from camcontrol: Add an nvres subcommand for NVMe reservations to camcontrol: Support NVMe reservations via the persist subcommand.
ken edited the summary of this revision. (Show Details)

Rework per review feedback: fold NVMe reservation support into the existing persist subcommand instead of adding an nvres subcommand. persist dispatches on device protocol like identify. SCSI syntax unchanged; NVMe uses the same -i/-o/-k/-K/-T vocabulary, with NVMe-only unregister/replace actions and a new -c option for Persist Through Power Loss State. Tested on reservation-capable hardware (PASCARI X200); details in the summary.

Thanks for the rework! Looks a lot better. Just found one nit.

sbin/camcontrol/nvres.c
405

num_ents comes from the device. If it described more entries than are read into the ctrlr array, then we will have an out of bounds access.
Suggest that you constrain this to the 8192 buffer you've read the reservation into, or otherwise dynamicall allocate to ensure no overflow.

Constrain the number of reservation report entries printed to what fits in the data buffer, per review feedback.