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.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 19, 9:34 PM
Unknown Object (File)
Tue, Mar 19, 5:26 PM
Unknown Object (File)
Feb 18 2024, 5:21 PM
Unknown Object (File)
Feb 9 2024, 10:09 PM
Unknown Object (File)
Feb 4 2024, 3:31 AM
Unknown Object (File)
Jan 29 2024, 12:57 AM
Unknown Object (File)
Jan 21 2024, 1:03 AM
Unknown Object (File)
Jan 16 2024, 5:34 PM
Subscribers

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 - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 16122
Build 16077: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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.

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.

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

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.

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

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

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

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
  • 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: https://svnweb.freebsd.org/changeset/base/333446

So, I'm closing this review.

Thanks everybody.