Page MenuHomeFreeBSD

[RFC] limit the max number of iscsi connections to a specific target
Needs ReviewPublic

Authored by nikita_elyzion.net on Aug 31 2017, 3:08 PM.

Details

Reviewers
bapt
manu
avg
trasz
Group Reviewers
cam
Summary

This patch introduce the maxconn property to an iscsi target.
When set, the value is passed to ctl during the port_add ioctl and is saved in the cfiscsi structure along with nb_conn which is increased when we create a new session for a target and decreased when we destroy the session.

The (nbconn < maxconn) check itself is done by ctld when a new connection is accepted.
During the nego phase, ctld get the value of maxconn and nbconn from the target through the CTL_ISCSI IOCTL with the CTL_ISCSI_MAXONN req type.

The check is done in ctld during the nego phase because it is where it seemed the cleaner place to stop the incoming connection.
At first I did it in ctl_frontend_iscsi.c in the handoff ioctl but it is too late since we already have passed Full Feature Phase and the initiator is already logged in. So dropping the connection at this phase is more difficult and less pleasant from the initiator point of view.

I still need to modify some manpages I guess, but the code seemed good enough for a first review.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Updating the diff, I forgot to push a typo fix from a repo.

sys/cam/ctl/ctl_frontend_iscsi.c
101

I don't know if 512 is a good value since we did not limit before, but I don't really have a strong opinion on what to do.

usr.sbin/ctld/login.c
691

Those magic values really hurt my eyes, we should replace all of them with defines.
Also max connection seems to be class 0x2, detail 0x6 according to the rfc (https://www.ietf.org/rfc/rfc3720.txt).

sys/cam/ctl/ctl_frontend_iscsi.c
101

I can put something bigger or even just a MAX_INT ? I have put 512 without thinking too much about the better value.

usr.sbin/ctld/login.c
691

I can replace all those define yes, it won't be that difficult.
Should I replace all of them in this patch ? or should I push another one that will be commited before this one ?

usr.sbin/ctld/login.c
691

Another review would be better.

I like the idea - this would allow us to say "no more than one initiator at a time", greatly simplifying host standby (cluster) setups.

That said, from my understanding there's a race condition - if two initiators try to connect at the same time, they will both make it through the login phase, since each ctld(8) child doesn't know about the other. I believe the check should happen in the kernel, during handoff. The iSCSI spec already contains a mechanism for the target to request logout (or just drop the connection) using the Asynchronous Message PDU; I'd expect initiators to be able to handle it gracefully, as it's mandatory part of the spec - although I've never tried that myself.

I'd also suggest a different name; other ctl.conf parameters don't use underscores, and mostly avoid contractions. How about "connections-max"?

And yeah, the man page :-)

sys/cam/ctl/ctl_frontend_iscsi.c
101

The sysctl seems unneccessary, IMHO. As for the default value - I'd just assume a negative number means "no limit" and default to -1.

usr.sbin/ctld/login.c
691

The problem with those is that they are pairs of values, rather than just values. And each value in a pair doesn't really have a meaning without the other - so how to name them?

usr.sbin/ctld/login.c
691

#define ISCSI_STATUS_SUCCESS 0x00
#define ISCSI_STATUS_REDIRECT 0x01
#define ISCSI_STATUS_INITIATOR_ERROR 0x02
#define ISCSI_STATUS_TARGET_ERROR 0x03

#define ISCSI_REDIRECT_TEMP 0x01

etc ...

This will always be better that magic values.

I like the idea - this would allow us to say "no more than one initiator at a time", greatly simplifying host standby (cluster) setups.

That said, from my understanding there's a race condition - if two initiators try to connect at the same time, they will both make it through the login phase, since each ctld(8) child doesn't know about the other. I believe the check should happen in the kernel, during handoff. The iSCSI spec already contains a mechanism for the target to request logout (or just drop the connection) using the Asynchronous Message PDU; I'd expect initiators to be able to handle it gracefully, as it's mandatory part of the spec - although I've never tried that myself.

I'd also suggest a different name; other ctl.conf parameters don't use underscores, and mostly avoid contractions. How about "connections-max"?

And yeah, the man page :-)

I have first wrote a patch which was dropping those connections in the kernel side with something like in cfiscsi_ioctl_handoff(), just after the ct->ct_online == 0 test :

CFISCSI_DEBUG("%s: cur_nb_conn %d max_conn %d", ct->ct_name, ct->ct_nb_conn, ct->ct_max_conn);
// increase ct->ct_nb_conn here since it will be decreased in cfiscsi_session_delete()
if (ct->ct_max_conn <= ct->ct_nb_conn++) {
        CFISCSI_DEBUG("refused conn for %s due to limits", ct->ct_name);
        mtx_unlock(&softc->lock);
        cfiscsi_session_terminate(cs);
        //XXX releasing the target here seems to lead to a page fault later.
        //cfiscsi_target_release(ct);
        ci->status = CTL_ISCSI_ERROR;
        snprintf(ci->error_str, sizeof(ci->error_str),
                        "%s: reached max number of conn", __func__);
        return;
}

But, closing the session without telling the initiator was, obviously, not clean. I had not tested to send an async PDU because after some thinking (maybe not that successful, I'm a novice here) I favored to switch to the limitation in ctld before the Full Feature Phase, it looked more nice at this moment...

Concerning the race, now that you are pointing it, yes this version of the patch is racy. In my specific use case the race cannot really happen so I was not thinking too much about that, especially since the first version of my patch was (I guess) addressing it.

I will try to see if I can figure how to send those async PDU from cfiscsi and I will come back to tell how the initiator handle it.

And, ok for connection-max, I will rename it.

usr.sbin/ctld/login.c
691

I was more thinking about putting an union in sys/dev/iscsi/iscsi_proto.h :

 struct iscsi_bhs_login_response {
        uint8_t         bhslr_opcode;
        uint8_t         bhslr_flags;
@@ -226,8 +232,13 @@ struct iscsi_bhs_login_response {
        uint32_t        bhslr_statsn;
        uint32_t        bhslr_expcmdsn;
        uint32_t        bhslr_maxcmdsn;
-       uint8_t         bhslr_status_class;
-       uint8_t         bhslr_status_detail;
+       union {
+               struct {
+                       uint8_t         s_class;
+                       uint8_t         s_detail;
+               } full;
+               uint16_t                packed;
+       } bhslr_status;

and add defines for the full status, one per error described in the rfc.
It permit you to only use the first byte if you want only the global status and use the 2bytes version if you want a more precise status.
It leads to a few really trivial modification in usr.sbin/iscsid/login.c and in usr.sbin/ctld/login.c.

What do you think about that ? It is too much modifications and I should just put some defines for each bytes like what manu propose or can I propose the version with the union ?

usr.sbin/ctld/login.c
691

The union solution looks good to me.

usr.sbin/ctld/login.c
691

The union would cause endianess problems, wouldn't it?

There might be a way to do it like this:

struct foo {                                                                                                                                                                                                            int x;                                                                                                                                                                                                         
int y;                                                                                                                                                                                                 
};                                                                                                                                                                                                                                                                                                                                                                                                                              #define MEH     (struct foo){ 11, 42 }                                                                                                                                                                          #define MEH2    (struct foo){ 12, 43 }                                                                                                                                                                                                                                                                                                                                                                                          static void                                                                                                                                                                                                     a(struct foo foo)                                                                                                                                                                                               {                                                                                                                                                                                                                                                                                                                                                                                                                                       printf("%d, %d\n", foo.x, foo.y);                                                                                                                                                                       }                                                                                                                                                                                                                                                                                                                                                                                                                               int                                                                                                                                                                                                             main(void)                                                                                                                                                                                                      {                                                                                                                                                                                                                       a(MEH);                                                                                                                                                                                                         a(MEH2);                                                                                                                                                                                                                                                                                                                                                                                                                        return (0);                                                                                                                                                                                            
}

But to be honest, I'd keep it simple. After all, the whole point of this is to make it as obvious to the reader as possible, and complicating things with unions or structs doesn't really help with that.