ctld: convert an EEXIST in kernel port creation to an update request

[kevans]

Summary

Kernel ports on startup are technically dummy ports because it's
entirely unknown how these connect in the grand scheme of things without
any kind of portal definition attached to them. Convert an EEXIST here
to an update, assuming that we own the port and should just make it
consistent with our configuration.

The scenario here can be reproduced either by crashing ctld, or by some
other mechanism that prer-creates the port, e.g.:

$ ctladm port -c -d iscsi -O cfiscsi_portal_group_tag=10 \

-O ctld_portal_group_name=pg0 \
-O cfiscsi_target=iqn.2012-06.dev.kevans:target0

$ ctladm port -p 3 -o on

Then firing up ctld with a "target iqn.2012-06.dev.kevans:target0".

[jhb]

So you may be interested in my other fixes where I had incorrectly labeled all kernel ports as dummy ports by mistake. See https://reviews.freebsd.org/D56524 and the related stack (and all the back and forth in the associated PR). I wonder if you even need this change anymore once that series of changes lands.

[kevans]

In D56541#1295966, @jhb wrote:

So you may be interested in my other fixes where I had incorrectly labeled all kernel ports as dummy ports by mistake. See https://reviews.freebsd.org/D56524 and the related stack (and all the back and forth in the associated PR). I wonder if you even need this change anymore once that series of changes lands.

The context for this is the older review that I had tried to get input on: https://reviews.freebsd.org/D51782 -- I don't think your change addresses that, because I seem to be looking at a portal_group_port:

bool                                                                                                          
portal_group_port::is_dummy() const                                                                           
{                                                                                                             
        return (p_portal_group->is_dummy());                                                                  
}

Your out-of-band commentary combined with @asomers notes is what lead me to this path instead, since I couldn't get any traction there...

[jhb]

In D56541#1296015, @kevans wrote:

In D56541#1295966, @jhb wrote:

So you may be interested in my other fixes where I had incorrectly labeled all kernel ports as dummy ports by mistake. See https://reviews.freebsd.org/D56524 and the related stack (and all the back and forth in the associated PR). I wonder if you even need this change anymore once that series of changes lands.

The context for this is the older review that I had tried to get input on: https://reviews.freebsd.org/D51782 -- I don't think your change addresses that, because I seem to be looking at a portal_group_port:

bool                                                                                                          
portal_group_port::is_dummy() const                                                                           
{                                                                                                             
        return (p_portal_group->is_dummy());                                                                  
}

Your out-of-band commentary combined with @asomers notes is what lead me to this path instead, since I couldn't get any traction there...

Oh, oof, there are "kernel" ports and there are "kernel ports". Having been down the long tunnel of the PR I referenced, I am thinking of kernel ports as the things managed by "kports" in ctld now, so physical ports on HBAs, and ioctl ports. Not a pre-existing portal_group port. For a pre-existing portal group port, I guess I'm surprised that we think the port is new. Hmm, looking at your other PR, is the issue that we do indeed add a port and a portal_group, but it has no portals so we think it is a dummy when it isn't? Ah, so it is, and I think I hadn't really understood the issue before now. I think I prefer getting the match right vs ignoring EEXIST errors (which is to say I think I prefer your original patch). If we were going to go the EEXIST route there's not much reason in enumerating non-physical kernel-enumerated ports at all, so I think fixing the match is closer to the code's intent. I might suggest naming the field pg_kernel_enumerated or some such to make it a bit clearer since we have kports so the notion of "kernel port" is a bit overloaded unfortunately.

[kevans]

In D56541#1296047, @jhb wrote:

In D56541#1296015, @kevans wrote:

In D56541#1295966, @jhb wrote:

So you may be interested in my other fixes where I had incorrectly labeled all kernel ports as dummy ports by mistake. See https://reviews.freebsd.org/D56524 and the related stack (and all the back and forth in the associated PR). I wonder if you even need this change anymore once that series of changes lands.

The context for this is the older review that I had tried to get input on: https://reviews.freebsd.org/D51782 -- I don't think your change addresses that, because I seem to be looking at a portal_group_port:

bool                                                                                                          
portal_group_port::is_dummy() const                                                                           
{                                                                                                             
        return (p_portal_group->is_dummy());                                                                  
}

Your out-of-band commentary combined with @asomers notes is what lead me to this path instead, since I couldn't get any traction there...

Oh, oof, there are "kernel" ports and there are "kernel ports". Having been down the long tunnel of the PR I referenced, I am thinking of kernel ports as the things managed by "kports" in ctld now, so physical ports on HBAs, and ioctl ports. Not a pre-existing portal_group port. For a pre-existing portal group port, I guess I'm surprised that we think the port is new. Hmm, looking at your other PR, is the issue that we do indeed add a port and a portal_group, but it has no portals so we think it is a dummy when it isn't? Ah, so it is, and I think I hadn't really understood the issue before now. I think I prefer getting the match right vs ignoring EEXIST errors (which is to say I think I prefer your original patch). If we were going to go the EEXIST route there's not much reason in enumerating non-physical kernel-enumerated ports at all, so I think fixing the match is closer to the code's intent. I might suggest naming the field pg_kernel_enumerated or some such to make it a bit clearer since we have kports so the notion of "kernel port" is a bit overloaded unfortunately.

FWIW, I personally much prefer my original patch as well, since I think it's pretty clear that 9215e501b9e7d3f42567 causing this was a side-effect rather than something intentional. I do think that discrepancy was 100% caused by the noted weirdness in how startup and shutdown works (before your patches) with now-conf::apply, but it'd be nice if we could fix the regression and maybe revisit that after.

[jhb]

In D56541#1296061, @kevans wrote:

In D56541#1296047, @jhb wrote:

In D56541#1296015, @kevans wrote:

In D56541#1295966, @jhb wrote:

So you may be interested in my other fixes where I had incorrectly labeled all kernel ports as dummy ports by mistake. See https://reviews.freebsd.org/D56524 and the related stack (and all the back and forth in the associated PR). I wonder if you even need this change anymore once that series of changes lands.

The context for this is the older review that I had tried to get input on: https://reviews.freebsd.org/D51782 -- I don't think your change addresses that, because I seem to be looking at a portal_group_port:

bool                                                                                                          
portal_group_port::is_dummy() const                                                                           
{                                                                                                             
        return (p_portal_group->is_dummy());                                                                  
}

Your out-of-band commentary combined with @asomers notes is what lead me to this path instead, since I couldn't get any traction there...

Oh, oof, there are "kernel" ports and there are "kernel ports". Having been down the long tunnel of the PR I referenced, I am thinking of kernel ports as the things managed by "kports" in ctld now, so physical ports on HBAs, and ioctl ports. Not a pre-existing portal_group port. For a pre-existing portal group port, I guess I'm surprised that we think the port is new. Hmm, looking at your other PR, is the issue that we do indeed add a port and a portal_group, but it has no portals so we think it is a dummy when it isn't? Ah, so it is, and I think I hadn't really understood the issue before now. I think I prefer getting the match right vs ignoring EEXIST errors (which is to say I think I prefer your original patch). If we were going to go the EEXIST route there's not much reason in enumerating non-physical kernel-enumerated ports at all, so I think fixing the match is closer to the code's intent. I might suggest naming the field pg_kernel_enumerated or some such to make it a bit clearer since we have kports so the notion of "kernel port" is a bit overloaded unfortunately.

FWIW, I personally much prefer my original patch as well, since I think it's pretty clear that 9215e501b9e7d3f42567 causing this was a side-effect rather than something intentional. I do think that discrepancy was 100% caused by the noted weirdness in how startup and shutdown works (before your patches) with now-conf::apply, but it'd be nice if we could fix the regression and maybe revisit that after.

Oh, and also, pg_kernel is fine as the name as it is a property of the portal group, not a port. Probably need the same thing btw when adding transport groups for NVMe controllers as well in the original patch.

I think though one thing we do need is a good comment above is_dummy to explain what a dummy portal group is, something like:

/*
  * Foreign portal groups (which only redirect to other targets), and portal groups
  * without any active portals are considered dummies and ports belonging to such
  * groups are ignored.  However, portal groups that exist in the kernel prior to
  * ctld starting will contain real ports but no portals, so these are never considered
  * dummies.
  */