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

[araujo]

Summary

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 iqn.2012-06.com.example:target0 {
        port ioctl/5/3
        lun 0 example_0
        lun 1 example_1
}

target iqn.2012-06.com.example:target1 {
        port ioctl/21
        lun 0 example_2
}

Expected result:

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

[mav]

sys/cam/ctl/ctl_frontend_ioctl.c

155

This code is incomplete. It will leak different resources.

287

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.

588

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

sys/cam/ctl/ctl_frontend_iscsi.c

2097

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

[jceel]

I decided to not allow to destroy default ioctl port

[jceel]

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]

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

sys/cam/ctl/ctl_frontend_iscsi.c

2190

Here should better be NULL instead of 0.

usr.sbin/ctladm/ctladm.c

463

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

2612

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.

2732

Same as above.

[trasz]

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]

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

[jceel]

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

[mav]

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

usr.sbin/ctladm/ctladm.c

450

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

[araujo]

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

[araujo]

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

[araujo]

NOTE: One more commit will be done for the manpage update.

[araujo]

• 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.

[araujo]

• 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.

[araujo]

• 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.

[araujo]

• Bring back ctladm.8 updated, removed by mistake.

[araujo]

• 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.

[araujo]

• Restore again the updates on ctladm.8.

[wblock]

usr.sbin/ctladm/ctladm.8

601

This reads better with s/Creates/Create/.

607

s/Creates/Create/

613

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 .

1028

s/Creates/Create/

1032

Remove specified targ_port.

[mav]

Looks mostly fine aside of commented places.

sys/cam/ctl/ctl.c

3296

Why to use separate lines?

usr.sbin/ctladm/ctladm.8

607

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.

613

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

usr.sbin/ctladm/ctladm.c

599

Why?

3690

Why not port_str?

4001

Why was that new-line removed?

[araujo]

Address @mav and @wblock changes requested.

[araujo]

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

[mav]

Some more man/help tunings and so be it.

usr.sbin/ctladm/ctladm.8

601

"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.

usr.sbin/ctladm/ctladm.c

3863

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

3953

Here missing -c -d and -r options.

4001

Better, but still seems like a pointless whitespace change.

[araujo]

• Address the latest comments and fixed an issue with copy(9) on sys/cam/ctl/ctl.c.

[araujo]

• Remove blank line and add copyright for the initial work made by @jceel.

[araujo]

Don't know why the review wasn't updated.
I just committed it: https://svnweb.freebsd.org/changeset/base/333446

So, I'm closing this review.

Thanks everybody.