tree: Improve TCP controller matching algorithm
authorMartin Belanger <martin.belanger@dell.com>
Fri, 18 Aug 2023 01:35:34 +0000 (21:35 -0400)
committerDaniel Wagner <wagi@monom.org>
Wed, 30 Aug 2023 15:21:59 +0000 (17:21 +0200)
The configuration parameters used to connect to a TCP controller are:

1. transport - "tcp"
2. traddr - Destination address (controller's IP address)
3. trsvcid - Service ID (controller's port - typically 8009 or 4420)
4. host-traddr - Source address (host's IP address)
5. host-iface - Physical/Logical interface where the connection will be made

For TCP, transport, traddr, and trsvcid are mandatory, while the
host-traddr and host-iface are optional. The host-traddr and host-iface
can be used as "overrides" to select a different source address and
interface than those that the kernel would choose by default.

When an application using libnvme to connect to a controller does
not specify the host-traddr or host-iface, the kernel will have to
determine the best interface and source address by itself. It does that
by looking up the destination address (traddr) in the routing table to
determine the best interface for the connection. The kernel then
retrieves the primary IP address assigned to that interface and uses that
as the connection's source address. By default, the kernel always uses
the interface's primary address as the connection's source address
unless host-traddr is used to override it.

Prior to version 6.1, the kernel did not reveal the source address or
interface it selected. Therefore, it was impossible for user-space apps
to tell exactly where connections were made. With kernel 6.1 (and later),
the sysfs now exposes the source address as "src_addr=" in the nvme
"address" attribute. The src_addr not only provides us with the
connection's source address, but by scanning the interface map one can
find out which interface owns that source address and precisely determine
on which interface each connection is made.

With TP8010 and the introduction of the Centralized Discovery Controller
(CDC), it is very important for hosts to connect to CDCs with a consistent
source address. That's because of the way the CDC keeps track of all the
hosts that connect to it. In addition to the host NQN, the CDC also checks
the host IP address (the connection's source address) to uniquely identify
a host. This unique identifier is then used for fabric zoning.

With fabric zoning, administrators configure the list of I/O controllers
that a host is allowed to connect to. The CDC sends the list of I/O
controllers to the host in response to a Get Discovery Log Page (DLP)
command from the host. If a host does not connect to the CDC with the
right source address, it will receive invalid DLP entries (wrong zone).
This will cause the host to connect to the wrong I/O controllers.

libnvme tries to avoid making duplicate connections to the same
controller. This avoids consuming precious kernel resources. When an
application requests libnvme to connect to a controller (the candidate
controller), libnvme scans the sysfs to see if an existing connection
matches the candidate. If a matching connection is found, libnvme just
reuses it instead of creating a new one.

Matching the 3 mandatory parameters (transport, traddr, trsvcid) between
existing connections and a candidate connection is easy because they can
never be NULL and can therefore be compared. It is not the case for the
host-traddr and host-iface. These optional parameters can be NULL. A NULL
host-traddr or host-iface means that we have left it to the kernel to
determine the interface and source address to use for the connection.
Therefore, if we want to compare a non-NULL candidate host-traddr to
an existing connection with a NULL host-traddr, we cannot just compare the
two. They will obviously be different. Instead, we need to check the
src_addr of the existing connection to see if it matches the candidate's
host-traddr.

Prior to this patch, libnvme performed a simple string comparison
between the candidate's host-traddr (or host-iface) and the existing
connection's host-traddr (or host-iface). A match would be declared if
both were the same (including both NULL). Also, a match would even be
declared if the existing connection's host-traddr (or host-iface) was
NULL while the candidate's host-traddr (or host-iface) was non-NULL.
This is wrong and can lead to the wrong connections being reused and
the wrong DLP entries returned by the CDC.

With this patch, when a candidate wants a specific source address
(host-traddr != NULL) or interface (host-iface != NULL), libnvme will
now check the src_addr of each existing connection to ensure a 100%
match. If the src_addr is not available (kernel older than 6.1), then we
can still infer the real interface and source address of an existing
connection, if the existing connection has either its host-traddr or
host-iface defined (check code to see how it's done).

It's only when an exsiting connection's src_addr, host-iface, and
host-traddr are all NULL that we cannot clearly match to a candidate.
When that happens, libnvme will take an optimistic approach and
declare a match even though it doesn't have enough info to do so.
This "optimistic match" follows what libnvme was doing prior to this
patch.

Signed-off-by: Martin Belanger <martin.belanger@dell.com>
src/nvme/tree.c

index b641b2bcd434033300e054e9ae6f6b1e1239e2aa..a7cebe96629eb8b5c29d9902459733db07ea7561 100644 (file)
@@ -15,6 +15,7 @@
 #include <fcntl.h>
 #include <libgen.h>
 #include <unistd.h>
+#include <ifaddrs.h>
 
 #include <sys/types.h>
 #include <sys/stat.h>
 #include "log.h"
 #include "private.h"
 
+/**
+ * struct candidate_args - Used to look for a controller matching these parameters
+ * @transport:         Transport type: loop, fc, rdma, tcp
+ * @traddr:            Transport address (destination address)
+ * @trsvcid:           Transport service ID
+ * @subsysnqn:         Subsystem NQN
+ * @host_traddr:       Host transport address (source address)
+ * @host_iface:                Host interface for connection (tcp only)
+ * @iface_list:                Interface list (tcp only)
+ */
+struct candidate_args {
+       const char *transport;
+       const char *traddr;
+       const char *trsvcid;
+       const char *subsysnqn;
+       const char *host_traddr;
+       const char *host_iface;
+       struct ifaddrs *iface_list;
+};
+
 const char *nvme_slots_sysfs_dir = "/sys/bus/pci/slots";
 
 static struct nvme_host *default_host;
@@ -1220,6 +1241,259 @@ struct nvme_ctrl *nvme_create_ctrl(nvme_root_t r,
        return c;
 }
 
+/**
+ * _tcp_ctrl_match_host_traddr_no_src_addr() - Match host_traddr w/o src_addr
+ * @c: An existing controller instance
+ * @candidate: Candidate ctrl we're trying to match with @c.
+ *
+ * On kernels prior to 6.1 (i.e. src_addr is not available), try to match
+ * a candidate controller's host_traddr to that of an existing controller.
+ *
+ * This function takes an optimistic approach. In doubt, it will declare a
+ * match and return true.
+ *
+ * Return: true if @c->host_traddr matches @candidate->host_traddr. false otherwise.
+ */
+static bool _tcp_ctrl_match_host_traddr_no_src_addr(struct nvme_ctrl *c, struct candidate_args *candidate)
+{
+       if (c->cfg.host_traddr) {
+               if (!nvme_ipaddrs_eq(candidate->host_traddr, c->cfg.host_traddr))
+                       return false;
+       } else {
+               /* If c->cfg.host_traddr is NULL, then the controller (c)
+                * uses the interface's primary address as the source
+                * address. If c->cfg.host_iface is defined we can
+                * determine the primary address associated with that
+                * interface and compare that to the candidate->host_traddr.
+                */
+               if (c->cfg.host_iface) {
+                       if (!nvme_iface_primary_addr_matches(candidate->iface_list,
+                                                            c->cfg.host_iface,
+                                                            candidate->host_traddr))
+                               return false;
+               } else {
+                       /* If both c->cfg.host_traddr and c->cfg.host_iface are
+                        * NULL, we don't have enough information to make a
+                        * 100% positive match. Regardless, let's be optimistic
+                        * and assume that we have a match.
+                        */
+                       nvme_msg(root_from_ctrl(c), LOG_DEBUG,
+                                "Not enough data, but assume %s matches candidate's host_traddr: %s\n",
+                                nvme_ctrl_get_name(c),
+                                candidate->host_traddr);
+               }
+       }
+
+       return true;
+}
+
+/**
+ * _tcp_ctrl_match_host_iface_no_src_addr() - Match host_iface w/o src_addr
+ * @c: An existing controller instance
+ * @candidate: Candidate ctrl we're trying to match with @c.
+ *
+ * On kernels prior to 6.1 (i.e. src_addr is not available), try to match
+ * a candidate controller's host_iface to that of an existing controller.
+ *
+ * This function takes an optimistic approach. In doubt, it will declare a
+ * match and return true.
+ *
+ * Return: true if @c->host_iface matches @candidate->host_iface. false otherwise.
+ */
+static bool _tcp_ctrl_match_host_iface_no_src_addr(struct nvme_ctrl *c, struct candidate_args *candidate)
+{
+       if (c->cfg.host_iface) {
+               if (!streq0(candidate->host_iface, c->cfg.host_iface))
+                       return false;
+       } else {
+               if (c->cfg.host_traddr) {
+                       const char *c_host_iface;
+
+                       c_host_iface = nvme_iface_matching_addr(candidate->iface_list,
+                                                               c->cfg.host_traddr);
+                       if (!streq0(candidate->host_iface, c_host_iface))
+                               return false;
+               } else {
+                       /* If both c->cfg.host_traddr and c->cfg.host_iface are
+                        * NULL, we don't have enough information to make a
+                        * 100% positive match. Regardless, let's be optimistic
+                        * and assume that we have a match.
+                        */
+                       nvme_msg(root_from_ctrl(c), LOG_DEBUG,
+                                "Not enough data, but assume %s matches candidate's host_iface: %s\n",
+                                nvme_ctrl_get_name(c),
+                                candidate->host_iface);
+               }
+       }
+
+       return true;
+}
+
+/**
+ * _tcp_opt_params_match_no_src_addr() - Match optional host_traddr/host_iface w/o src_addr
+ * @c: An existing controller instance
+ * @candidate: Candidate ctrl we're trying to match with @c.
+ *
+ * Before kernel 6.1, the src_addr was not reported by the kernel which makes
+ * it hard to match a candidate's host_traddr and host_iface to an existing
+ * controller if that controller was created without specifying the
+ * host_traddr and/or host_iface. This function tries its best in the absense
+ * of a src_addr to match @c to @candidate. This may not be 100% accurate.
+ * Only the src_addr can provide 100% accuracy.
+ *
+ * This function takes an optimistic approach. In doubt, it will declare a
+ * match and return true.
+ *
+ * Return: true if @c matches @candidate. false otherwise.
+ */
+static bool _tcp_opt_params_match_no_src_addr(struct nvme_ctrl *c, struct candidate_args *candidate)
+{
+       /* Check host_traddr only if candidate is interested */
+       if (candidate->host_traddr) {
+               if (!_tcp_ctrl_match_host_traddr_no_src_addr(c, candidate))
+                       return false;
+       }
+
+       /* Check host_iface only if candidate is interested */
+       if (candidate->host_iface) {
+               if (!_tcp_ctrl_match_host_iface_no_src_addr(c, candidate))
+                       return false;
+       }
+
+       return true;
+}
+
+/**
+ * _tcp_opt_params_match() - Match optional host_traddr/host_iface
+ * @c: An existing controller instance
+ * @candidate: Candidate ctrl we're trying to match with @c.
+ *
+ * The host_traddr and host_iface are optional for TCP. When they are not
+ * specified, the kernel looks up the destination IP address (traddr) in the
+ * routing table to determine the best interface for the connection. The
+ * kernel then retrieves the primary IP address assigned to that interface
+ * and uses that as the connection’s source address.
+ *
+ * An interface’s primary address is the default source address used for
+ * all connections made on that interface unless host-traddr is used to
+ * override the default. Kernel-selected interfaces and/or source addresses
+ * are hidden from user-space applications unless the kernel makes that
+ * information available through the "src_addr" attribute in the
+ * sysfs (kernel 6.1 or later).
+ *
+ * Sometimes, an application may force the interface by specifying the
+ * "host-iface" or may force a different source address (instead of the
+ * primary address) by providing the "host-traddr".
+ *
+ * If the candidate specifies the host_traddr and/or host_iface but they
+ * do not match the existing controller's host_traddr and/or host_iface
+ * (they could be NULL), we may still be able to find a match by taking
+ * the existing controller's src_addr into consideration since that
+ * parameter identifies the actual source address of the connection and
+ * therefore can be used to infer the interface of the connection. However,
+ * the src_addr can only be read from the nvme device's sysfs "address"
+ * attribute starting with kernel 6.1 (or kernels that backported the
+ * src_addr patch).
+ *
+ * For legacy kernels that do not provide the src_addr we must use a
+ * different algorithm to match the host_traddr and host_iface, but
+ * it's not 100% accurate.
+ *
+ * Return: true if @c matches @candidate. false otherwise.
+ */
+static bool _tcp_opt_params_match(struct nvme_ctrl *c, struct candidate_args *candidate)
+{
+       char *src_addr, buffer[INET6_ADDRSTRLEN];
+
+       /* Check if src_addr is available (kernel 6.1 or later) */
+       src_addr = nvme_ctrl_get_src_addr(c, buffer, sizeof(buffer));
+       if (!src_addr)
+               return _tcp_opt_params_match_no_src_addr(c, candidate);
+
+       /* Check host_traddr only if candidate is interested */
+       if (candidate->host_traddr &&
+           !nvme_ipaddrs_eq(candidate->host_traddr, src_addr))
+               return false;
+
+       /* Check host_iface only if candidate is interested */
+       if (candidate->host_iface &&
+           !streq0(candidate->host_iface,
+                   nvme_iface_matching_addr(candidate->iface_list, src_addr)))
+               return false;
+
+       return true;
+}
+
+/**
+ * _tcp_lookup_ctrl() - Look for an existing controller that can be reused
+ * @s: Subsystem object under which to do the search
+ * @transport: Transport type ("tcp")
+ * @traddr:    Destination IP address
+ * @host_iface:        Interface for the connection (optional)
+ * @host_traddr:       Source IP address (optional)
+ * @trsvcid:   Destination TCP port
+ * @p: Starting point is the linked-list (NULL to start at the beginning)
+ *
+ * We want to determine if an existing controller can be re-used
+ * for the candidate controller we're trying to instantiate. The
+ * candidate is identified by @transport, @traddr, @trsvcid,
+ * @host_traddr, and @host_iface.
+ *
+ * For TCP, we do not have a match if the candidate's transport, traddr,
+ * trsvcid are not identical to those of the the existing controller.
+ * These 3 parameters are mandatory for a match.
+ *
+ * The host_traddr and host_iface are optional. When the candidate does
+ * not specify them (both NULL), we can ignore them. Otherwise, we must
+ * employ advanced investigation techniques to determine if there's a match.
+ *
+ * Return: Pointer to the matching controller or NULL.
+ */
+static nvme_ctrl_t _tcp_lookup_ctrl(nvme_subsystem_t s, const char *transport,
+                                   const char *traddr, const char *host_traddr,
+                                   const char *host_iface, const char *trsvcid,
+                                   nvme_ctrl_t p)
+{
+       struct candidate_args candidate = {0};
+       struct nvme_ctrl *c, *matching_c = NULL;
+
+       candidate.traddr = traddr;
+       candidate.trsvcid = trsvcid;
+       candidate.transport = transport;
+       candidate.host_iface = host_iface;
+       candidate.host_traddr = host_traddr;
+
+       /* For TCP we may need to access the interface map. Let's retrieve
+        * and cache the map and use it for the duration of the loop below.
+        */
+       if (getifaddrs(&candidate.iface_list) == -1)
+               candidate.iface_list = NULL;
+
+       c = p ? nvme_subsystem_next_ctrl(s, p) : nvme_subsystem_first_ctrl(s);
+       for (; c != NULL; c = nvme_subsystem_next_ctrl(s, c)) {
+               if (!streq0(c->transport, candidate.transport))
+                       continue;
+
+               if (!streq0(c->trsvcid, candidate.trsvcid))
+                       continue;
+
+               if (!nvme_ipaddrs_eq(c->traddr, candidate.traddr))
+                       continue;
+
+               /* Check host_traddr / host_iface only if candidate is interested */
+               if ((candidate.host_iface || candidate.host_traddr) &&
+                   !_tcp_opt_params_match(c, &candidate))
+                       continue;
+
+               matching_c = c;
+               break;
+       }
+
+       freeifaddrs(candidate.iface_list); /* This is NULL-safe */
+
+       return matching_c;
+}
+
 nvme_ctrl_t __nvme_lookup_ctrl(nvme_subsystem_t s, const char *transport,
                               const char *traddr, const char *host_traddr,
                               const char *host_iface, const char *trsvcid,
@@ -1229,10 +1503,15 @@ nvme_ctrl_t __nvme_lookup_ctrl(nvme_subsystem_t s, const char *transport,
        struct nvme_ctrl *c;
        bool (*addreq)(const char *, const char *);
 
-       if (!strcmp(transport, "tcp") || !strcmp(transport, "rdma"))
-               addreq = nvme_ipaddrs_eq; /* IP address compare for TCP/RDMA */
+       /* TCP requires special handling */
+       if (streq0(transport, "tcp"))
+               return _tcp_lookup_ctrl(s, transport, traddr,
+                                       host_traddr, host_iface, trsvcid, p);
+
+       if (streq0(transport, "rdma"))
+               addreq = nvme_ipaddrs_eq; /* IP address compare for RDMA */
        else
-               addreq = streqcase0; /* Case-insensitive for FC (n/a for loop) */
+               addreq = streqcase0; /* Case-insensitive for everything else */
 
        c = p ? nvme_subsystem_next_ctrl(s, p) : nvme_subsystem_first_ctrl(s);
        for (; c != NULL; c = nvme_subsystem_next_ctrl(s, c)) {