Differential D44713

cam: Add a XPORT_NVMF for NVMe over Fabrics sims
AcceptedPublic
Actions

Authored by jhb on Tue, Apr 9, 11:03 PM.

Tags

None

Referenced Files

	Unknown Object (File)
	Sat, Apr 27, 1:24 AM

	Unknown Object (File)
	Fri, Apr 26, 7:21 PM

	Unknown Object (File)
	Fri, Apr 26, 7:07 AM

	Unknown Object (File)
	Fri, Apr 26, 4:58 AM

	Unknown Object (File)
	Thu, Apr 11, 7:22 PM

Subscribers

Details

Reviewers

imp
ken

Group Reviewers

Summary

Sponsored by: Chelsio Communications

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 57190
Build 54078: arc lint + arc unit

Event Timeline

jhb created this revision.Tue, Apr 9, 11:03 PM

Herald added a reviewer: cam. · View Herald TranscriptTue, Apr 9, 11:03 PM

Herald added a subscriber: dab. · View Herald Transcript

jhb requested review of this revision.Tue, Apr 9, 11:03 PM

Harbormaster completed remote builds in B57004: Diff 136809.Tue, Apr 9, 11:03 PM

jhb added a parent revision: D44712: nvmf_tcp: Add a TCP transport for NVMe over Fabrics.Tue, Apr 9, 11:03 PM

jhb added a child revision: D44714: nvmf: The in-kernel NVMe over Fabrics host.

Looks good. Two minor nits to consider

sbin/camcontrol/camcontrol.c
5270	Can this be a table lookup instead?
5428	switch statements?

This revision is now accepted and ready to land.Wed, Apr 10, 2:50 AM

jhb added inline comments.Wed, Apr 10, 3:21 PM

sbin/camcontrol/camcontrol.c
5270	I wonder actually if I should just move this function to libnvmf. nvmecontrol has a duplicate copy already. I'm sure the compiler compiles it down to a jump table that is stored as a lookup table already in practice.
5428	The surrounding code here is all using if statements so I matched it. Actually, though the rest of the function does transport first and then protocol, so I should maybe match that. I do wonder how much it matters to keep WITH_NVME here vs just always compiling these bits into camcontrol at this point.

ken accepted this revision.Mon, Apr 15, 3:14 PM

ken added a subscriber: ken.

ken added inline comments.

sbin/camcontrol/camcontrol.c
5428	I think we could probably just get rid of WITH_NVME at this point.

imp added inline comments.Mon, Apr 15, 4:56 PM

sbin/camcontrol/camcontrol.c
5428	I think so too. nda hasn't been experimental for at least 5 years. Not sure how that interacts with the rest of the build system, though I'm not entirely sure I care: We don't have WITHOUT_ATA or WITHOUT_SCSI

jhb added inline comments.Tue, Apr 16, 6:08 PM

sbin/camcontrol/camcontrol.c
5428	Today MK_NVME controls this in part because nvmecontrol doesn't build on all platforms, but Warner has a review to fix nvmecontrol, and once that is in I think we should remove MK_NVME and just build these bits unconditionally. I probably will wait for that to happen first before landing this and will fix these up to follow the sensible order instead.

jhb marked an inline comment as done.Tue, Apr 16, 6:52 PM

Move nvmf_transport_type to libnvmf

This revision now requires review to proceed.Tue, Apr 16, 8:45 PM

Harbormaster completed remote builds in B57134: Diff 137179.Tue, Apr 16, 8:45 PM

jhb added a parent revision: D44843: Remove the MK_NVME build option.Thu, Apr 18, 5:44 PM

Rebase for removal of WITH_NVME

Harbormaster completed remote builds in B57190: Diff 137329.Thu, Apr 18, 5:54 PM

jhb marked 2 inline comments as done.Thu, Apr 18, 6:00 PM

imp accepted this revision.Thu, Apr 18, 10:36 PM

This revision is now accepted and ready to land.Thu, Apr 18, 10:36 PM

Revision Contents
Changeset List

Path

Size

rescue/

rescue/

1 line

sbin/

camcontrol/

3 lines

37 lines

sys/

cam/

18 lines

nvme/

1 line

Diff 137329

rescue/rescue/Makefile

Loading...

sbin/camcontrol/Makefile

Loading...

sbin/camcontrol/camcontrol.c

Loading...

sys/cam/cam_ccb.h

Loading...

sys/cam/nvme/nvme_xpt.c

Loading...