Page MenuHomeFreeBSD

Rework CTL frontend & backend options to use nv(3), allow creating multiple ioctl frontend ports

Authored by araujo on Jan 22 2017, 10:47 PM.
Referenced Files
Unknown Object (File)
Apr 23 2017, 2:58 PM
Unknown Object (File)
Apr 11 2017, 10:24 PM
Unknown Object (File)
Apr 11 2017, 10:24 PM
Unknown Object (File)
Mar 7 2017, 12:10 AM
Unknown Object (File)
Jan 31 2017, 2:12 PM
Unknown Object (File)
Jan 30 2017, 11:45 PM



This revision introduces two changes to CTL:

  1. Changes the way options are passed to CTL_LUN_REQ and CTL_PORT_REQ ioctls. Removes ctl_be_arg structure and associated logic and replaces it with nv(3)-based logic for passing in and out arguments.
  2. Allows creating multiple ioctl frontend ports using either ctladm(8) or ctld(8). New frontend ports are represented by /dev/cam/ctl<pp>.<vp> nodes, eg /dev/cam/ctl5.3. Those device nodes respond only to CTL_IO ioctl.

New command-line options for ctladm:

ctladm port -c # creates new ioctl frontend port with using free pp and vp=0
ctladm port -c -o pp=10 # creates new ioctl frontend port with pp=10 and vp=0
ctladm port -c -o pp=11 -o vp=12 # creates new ioctl frontend port with pp=11 and vp=12
ctladm port -r -p 4 # removes port with number 4 (it's a "targ_port" number, not pp number)

Changes in ctladm port command:
ctladm port [-o on|off] changed to ctladm port [-i on|off]

New syntax for ctl.conf:

target ... {
    port ioctl/<pp>

target ... {
    port ioctl/<pp>/<vp>

Attaches a target to given ioctl frontend port - if it doesn't exist, it gets created automatically by ctld and removed during shutdown

Test Plan

Example ctl.conf exercising new ioctl ports:

lun example_0 {
        path /mnt/tank/vol1

lun example_1 {
        path /mnt/tank/vol2

lun example_2 {
        path /mnt/tank/vol3

target {
        port ioctl/5/3
        lun 0 example_0
        lun 1 example_1

target {
        port ioctl/21
        lun 0 example_2

Expected result:

[root@freenas-3] ~# ctladm reportluns 0 -D /dev/cam/ctl5.3
2 LUNs returned
[root@freenas-3] ~# ctladm reportluns 0 -D /dev/cam/ctl21.0
1 LUNs returned

Diff Detail

rS FreeBSD src repository - subversion
Lint Passed
No Test Coverage
Build Status
Buildable 16510
Build 16424: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

This code is incomplete. It will leak different resources.


What happen if somebody try to destroy default ioctl port? It has no dedicated device to destroy, and without that there may be running requests during port destruction. Also attempt to recreate that port will cause behavior different from original.


This won't be correct if we allow default port to be destroyed.


This change is probably not needed. I am usually trying to clean such ones during final patch review.

I decided to not allow to destroy default ioctl port

jceel marked an inline comment as done.
jceel edited edge metadata.

Added -d switch to ctladm port command to specify frontend driver type (default is ioctl). Fixed resource leaks in cfi_shutdown(). Fixed minor gratuitous typecasts. Added seatbelts preventing default ioctl port to be destroyed.

mav edited edge metadata.

I found few more minors, but otherwise let it be probably.


Here should better be NULL instead of 0.


Here should be free(driver) before new assignment in case -d specified twice.


I suppose nvlist_destroy() should be moved up just after nvlist_pack() to not leak memory if one fail. Same time here should probably be freed req.args.


Same as above.

This revision is now accepted and ready to land.Feb 23 2017, 3:21 PM

It looks like it's still missing the update to usage() in ctladm.c, and the ctladm.8 manual page. The rest looks nice, thanks for working on this :-)

jceel edited edge metadata.
jceel marked 4 inline comments as done.

Fixed issued pointed out by mav@, added new switches to usage() in ctladm.c

This revision now requires review to proceed.Mar 26 2017, 10:05 PM

still missing: man page changes. working on them...

Still looks good. Though I added one more minor comment.


Why do you need second strsep() here? tmpstr should already be the value.

This revision is now accepted and ready to land.Mar 27 2017, 12:02 PM
araujo added a reviewer: jceel.

I'm taking over this review as I need it to be able to add virtio-scsi for bhyve.
All the credits are still maintained for the amazing work done by @jceel

Sync this patch with the latest HEAD revision, also bump FreeBSD version due API changes and to make it explicite incompatible with other versions.

This revision now requires review to proceed.Apr 12 2018, 6:02 AM
NOTE: One more commit will be done for the manpage update.
  • Update ctladm(8) manpage to reflect all these changes.
  • Also reorganize the 'example' section where it should have first an explanation about the command line and then the command line example.
  • Add at the example section some examples how to use the new options ioctl frontend.

Note: Other ctl like manpages example section need to be fixed as well. I will do that in another review.

  • Keep the on|off as option "o" or otherwise we will break POLA.
  • Instead to use "o" for pp|vp we will use "O".
  • Update manpage to reflect these changes.
  • Also remove a unnecessary free() that was creating a core dump on ctladm.
  • Fix kernel panic when use 'ctladm portlist' and 'ctladm port -l' because of the misusage of variable cookie on nvlist_next. The variable cookie must be set to NULL for the first call and should not be changed later.
  • As we are inside KERNEL it is safer use dnv(9) and check if we have a return from dnvlist_get_string.
  • Bring back ctladm.8 updated, removed by mistake.
  • Remove an unnecessary free() that was commented already.
  • Initialize a variable to silence clang-analysis warning.
  • Use realloc instead of malloc.
NOTE: I guess I'm done with this review.
  • Restore again the updates on ctladm.8.
wblock added inline comments.

This reads better with s/Creates/Create/.




This is a little weird with the "if". -p targ_port is required for this, right? So maybe:

Remove port.
Port must be specified with
.Pq Fl p Ar targ_port .


Remove specified targ_port.
mav requested changes to this revision.Apr 25 2018, 12:39 AM

Looks mostly fine aside of commented places.


Why to use separate lines?


Description of "-c" and "-O" options is confusing. Somebody may think that "-O" will create something by itself, that is IIRC incorrect. IIRC "-c" just means "create", while "-O" is used to pass arbitrary options, of which we have now pp and vp.


I agree. I would just possibly tell shorter:
Remove port specified with
.Pq Fl p Ar targ_port .




Why not port_str?


Why was that new-line removed?

This revision now requires changes to proceed.Apr 25 2018, 12:39 AM

Fixed a sentence at the example section that was changed by mistake.

Some more man/help tunings and so be it.


"ioctl" frontend here used only as default. There is a -d perameter to allow overriding. At least iscsi ports can also be created now, just not recommended (and pointless) to do that manually without ctld.


Why -d is twice here, while -r is missing?


Here missing -c -d and -r options.


Better, but still seems like a pointless whitespace change.

This revision is now accepted and ready to land.May 4 2018, 6:19 PM
  • Address the latest comments and fixed an issue with copy(9) on sys/cam/ctl/ctl.c.
This revision now requires review to proceed.May 10 2018, 3:16 AM
  • Remove blank line and add copyright for the initial work made by @jceel.

Don't know why the review wasn't updated.
I just committed it:

So, I'm closing this review.

Thanks everybody.