Split iscsi ctl frontend off of ctl(4) as cfiscsi(4)

[ngie]

Summary

Split iscsi ctl frontend off of ctl(4) as cfiscsi(4)

The goal of this work is to remove the (now) explicit dependency for ctl(4)
on iscsi(4), so end-users without iscsi(4) support in the kernel can use
ctl(4) for its other functions.

This allows those without iscsi(4) support built into the kernel (like
my employer) to use ctl(4) as a test mechanism (which was doable around the
10.0-RELEASE period, but broken between 10.0-RELEASE and 11.0-RELEASE).

Add a manpage for the new ctl(4) frontend device, cfiscsi(4).

Automatically load cfiscsi(4) from ctladm(8) and ctld(8) for backwards
compatibility with previously releases.

MFC after: 1 month
Relnotes: yes
Sponsored by: Dell EMC Isilon

Test Plan

I have built and loaded the cfiscsi module and have statically built it
into the kernel, both with and without the iscsi(4) support.

Verified that autoloading cfiscsi via ctladm(8) and ctld(8) works:

$ kldstat | grep cfiscsi
25    1 0xffffffff82fe5000 9731     cfiscsi.ko
$ sudo kldunload cfiscsi
$ kldstat | grep cfiscsi
$ sudo service ctld start
Starting ctld.
$ kldstat | grep iscsi
26    1 0xffffffff82fe5000 9731     cfiscsi.ko
27    1 0xffffffff82fef000 132c8    iscsi.ko
$ sudo ctladm devlist
LUN Backend       Size (Blocks)   BS Serial Number    Device ID
$ sudo kldunload cfiscsi
$ kldstat | grep iscsi
$ sudo ctladm devlist   
LUN Backend       Size (Blocks)   BS Serial Number    Device ID
$ kldstat | grep iscsi
27    1 0xffffffff82fe5000 9731     cfiscsi.ko
28    1 0xffffffff82fef000 132c8    iscsi.ko

[mav]

IMO removing this dependency would be a good thing. I just not very like the proposed module name. We have different naming scheme for each separate frontend now, while some unification would be good. Unification with cfumass in a form of cfiscsi would probably look better for me. Also that name is already used in its code in many places.

Also ctld now can load ctl module on demand. It would be nice to teach it also load cfiscsi, if needed for config.

[ngie]

Update per feedback from mav:

• rename ctl_iscsi(4) to cfiscsi(4) for consistency with cfumass(4) and other areas of the cfiscsi(4) code.

Other updates:

• fix some mdoc issues with cfiscsi(4) improperly specified macro calls.
• expound cfiscsi(4)'s history and lineage from ctl(4).

[ngie]

Include share/man, not share/mk >_>

[mav]

share/man/man4/ctl.4

113 ↗

(On Diff #26560)

While there is indeed confusion between "iscsi" frontend name registered in ctl and "cfiscsi" module name, this wording sounds even more odd to me.

[mav]

I have no objections.

[ngie]

share/man/man4/ctl.4

113 ↗

(On Diff #26560)

I agree -- it's incredibly wordy. I'll reword the sentence.

[ngie]

ctl(4): reword section describing iscsi ctl(4) frontend (per note from mav)
cfiscsi(4):

• Sort device listing in SYNOPSIS per ctl_iscsi -> cfiscsi rename.
• wordsmith DESCRIPTION subject/verb agreement (devices should be device).

[trasz]

The AUTHORS section for cfiscsi.4 seems completely wrong. Can you just copy it from iscsi.4?

[ngie]

Per comments from trasz: copy AUTHORS verbatim from iscsi(4). Note that I wrote the
manpage.

[trasz]

Thanks. Generally speaking I like the change. Two things still bother me. First, as mav@ mentioned, ctld(8) will automatically load ctl(4) kernel module; it would be nice if things kept working for people depending on it. Second - that's a minor one, but the iSCSI frontend was introduced in 10.0, not 12.0.

[ngie]

Fill in gaps in the HISTORY section

[ngie]

In D10099#209213, @trasz wrote:

Thanks. Generally speaking I like the change. Two things still bother me. First, as mav@ mentioned, ctld(8) will automatically load ctl(4) kernel module; it would be nice if things kept working for people depending on it. Second - that's a minor one, but the iSCSI frontend was introduced in 10.0, not 12.0.

I'll see what I can do about making the first concern work.

[ngie]

Add cfiscsi(4) autoload support to ctladm(8) and ctld(8), per mav/trasz's request

[ngie]

mav/trasz: would you like me to make any more changes to the review?

[trasz]

Nice, thanks again! Might be useful to add "Relnotes: yes", so that people who don't want to depend on autoloading can update their configs.

[mav]

In D10099#210450, @trasz wrote:

Nice, thanks again! Might be useful to add "Relnotes: yes", so that people who don't want to depend on autoloading can update their configs.

And respectively mention it in UPDATING.

usr.sbin/ctld/kernel.c

97 ↗

(On Diff #26701)

I suppose "ctlcfiscsi" here is a typo.

I would not say that this code is exactly what I expected to see. I don't see big reason to modularise something and then load it always. I expected loading module when iscsi really present in configuration. It is not so straightforward, but would be nice. Though this was is still probably better then nothing.

[ngie]

usr.sbin/ctld/kernel.c

97 ↗

(On Diff #26701)

It's not a typo, actually. This is the name that is passed to CTL_FRONTEND_DECLARE(..):

From sys/cam/ctl/ctl_frontend_iscsi.c:

187 CTL_FRONTEND_DECLARE(ctlcfiscsi, cfiscsi_frontend);
188 MODULE_DEPEND(ctlcfiscsi, icl, 1, 1, 1);

Seeing as this was never formalized as a module, prior to this proposed change, I'll rename ctlcfiscsi to cfiscsi, since the ctl prefix is redundant (cf standing for ctl frontend).

[ngie]

usr.sbin/ctld/kernel.c

97 ↗

(On Diff #26701)

I removed the redundant ctl prefix here and in ctl_frontend_iscsi.c -- it will be in the final diff.

Thanks for the sanity check ;)..