Page MenuHomeFreeBSD

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

Authored by araujo on Jan 22 2017, 10:47 PM.

Details

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 16210
Build 16161: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
mav added inline comments.Feb 1 2017, 7:03 PM
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 marked 6 inline comments as done.Feb 22 2017, 1:49 AM

I decided to not allow to destroy default ioctl port

jceel updated this revision to Diff 25560.EditedFeb 22 2017, 7:19 PM
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 accepted this revision.Feb 23 2017, 3:21 PM
mav edited edge metadata.

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.

This revision is now accepted and ready to land.Feb 23 2017, 3:21 PM
trasz edited edge metadata.Feb 25 2017, 11:15 AM

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 updated this revision to Diff 26678.Mar 26 2017, 10:05 PM
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...

mav accepted this revision.Mar 27 2017, 12:02 PM

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.

This revision is now accepted and ready to land.Mar 27 2017, 12:02 PM
araujo added a subscriber: araujo.Apr 11 2018, 10:14 AM
araujo commandeered this revision.Apr 12 2018, 5:58 AM
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

araujo updated this revision to Diff 41390.Apr 12 2018, 6:02 AM

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.
araujo updated this revision to Diff 41393.Apr 12 2018, 7:38 AM
  • 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 updated this revision to Diff 41419.Apr 13 2018, 3:40 AM
  • 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 updated this revision to Diff 41609.Apr 18 2018, 5:08 AM
  • 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 updated this revision to Diff 41610.Apr 18 2018, 5:14 AM
  • Bring back ctladm.8 updated, removed by mistake.
araujo updated this revision to Diff 41615.Apr 18 2018, 9:53 AM
  • 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 updated this revision to Diff 41616.Apr 18 2018, 9:56 AM
  • Restore again the updates on ctladm.8.
wblock added a subscriber: wblock.Apr 24 2018, 1:51 PM
wblock added inline comments.
usr.sbin/ctladm/ctladm.8
601

This reads better with s/Creates/Create/.

607

s/Creates/Create/

612

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 requested changes to this revision.Apr 25 2018, 12:39 AM

Looks mostly fine aside of commented places.

sys/cam/ctl/ctl.c
3315

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.

612

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

usr.sbin/ctladm/ctladm.c
596

Why?

3689

Why not port_str?

3996

Why was that new-line removed?

This revision now requires changes to proceed.Apr 25 2018, 12:39 AM
araujo updated this revision to Diff 41982.Apr 30 2018, 2:55 AM

Address @mav and @wblock changes requested.

araujo updated this revision to Diff 41983.Apr 30 2018, 3:04 AM

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

araujo marked 11 inline comments as done.Apr 30 2018, 8:23 AM
mav accepted this revision.May 4 2018, 6:19 PM

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
3862

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

3949

Here missing -c -d and -r options.

3996

Better, but still seems like a pointless whitespace change.

This revision is now accepted and ready to land.May 4 2018, 6:19 PM
araujo updated this revision to Diff 42351.May 10 2018, 3:16 AM
  • 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
araujo updated this revision to Diff 42352.May 10 2018, 3:30 AM
  • Remove blank line and add copyright for the initial work made by @jceel.
araujo abandoned this revision.May 10 2018, 4:17 AM

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.