Remove duplicate addresses from the resolver SLIST

The SLIST (essentially `fctx->finds`, forwarders and dual-stack
alternatives aside) can have duplicate server addresses when multiple
in-domain nameservers share the same IP addresses:

  sub.example.          NS      ns1.sub.example.
  sub.example.          NS      ns2.sub.example.
  ns1.sub.example.      A       1.2.3.4
  ns1.sub.example.      A       5.6.7.8
  ns2.sub.example.      A       1.2.3.4
  ns2.sub.example.      A       5.6.7.8

If both 1.2.3.4 and 5.6.7.8 fail to return a valid answer, the resolver
would query each address twice.

The problem is fixed by replacing the two-phase server selection (sort
each find list by SRTT, sort finds by head SRTT) with a single linear
scan in nextaddress() that finds the lowest-SRTT unmarked, non-duplicate
address across all find lists.

The old approach had a correctness bug: after sorting, the resolver
picked the next address from the "current" find list rather than
globally.  For example, with find lists [1, 15, 26] and [3, 4, 5], the
second pick would be SRTT 15 instead of the correct SRTT 3.

The new approach is both simpler and correct: each call to nextaddress()
walks all addresses, skips marked and duplicate entries, and returns the
one with the lowest SRTT.  While this walk is repeated for each server
attempt, it operates on a small bounded list and is negligible compared
to the network I/O of querying the server.

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index ba487961894d8ae36c2026ec7efbb569330f21dc..e66e602888b0c88e7dde0b0f9e31cd22237a2115 100644 (file)
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -368,7 +368,16 @@ struct fetchctx {
        dns_message_t *qmessage;
        ISC_LIST(resquery_t) queries;
        dns_adbfindlist_t finds;
-       dns_adbfind_t *find;
+       /*
+        * This is a state to keep track of the latest upstream server which is
+        * being queried. See `nextaddress()`.
+        *
+        * `addrinfo` is basically a copy of `foundaddrinfo` but came from the
+        * response of the query, so fields like the SRTT/timing might have been
+        * altered. So it might be possible (?) to wrap those two in an union
+        * for clarity (and memory saving).
+        */
+       dns_adbaddrinfo_t *foundaddrinfo;
        /*
         * altfinds are names and/or addresses of dual stack servers that
         * should be used when iterative resolution to a server is not
@@ -1409,7 +1418,7 @@ fctx_cleanup(fetchctx_t *fctx) {
                dns_adb_destroyfind(&find);
                fetchctx_unref(fctx);
        }
-       fctx->find = NULL;
+       fctx->foundaddrinfo = NULL;
 
        ISC_LIST_FOREACH(fctx->altfinds, find, publink) {
                ISC_LIST_UNLINK(fctx->altfinds, find, publink);
@@ -3251,89 +3260,6 @@ add_bad(fetchctx_t *fctx, dns_message_t *rmessage, dns_adbaddrinfo_t *addrinfo,
                      classbuf, addrbuf);
 }
 
-/*
- * Sort addrinfo list by RTT.
- */
-static void
-sort_adbfind(dns_adbfind_t *find, unsigned int bias) {
-       dns_adbaddrinfo_t *best, *curr;
-       dns_adbaddrinfolist_t sorted;
-
-       /* Lame N^2 bubble sort. */
-       ISC_LIST_INIT(sorted);
-       while (!ISC_LIST_EMPTY(find->list)) {
-               unsigned int best_srtt;
-               best = ISC_LIST_HEAD(find->list);
-               best_srtt = best->srtt;
-               if (isc_sockaddr_pf(&best->sockaddr) != AF_INET6) {
-                       best_srtt += bias;
-               }
-               curr = ISC_LIST_NEXT(best, publink);
-               while (curr != NULL) {
-                       unsigned int curr_srtt = curr->srtt;
-                       if (isc_sockaddr_pf(&curr->sockaddr) != AF_INET6) {
-                               curr_srtt += bias;
-                       }
-                       if (curr_srtt < best_srtt) {
-                               best = curr;
-                               best_srtt = curr_srtt;
-                       }
-                       curr = ISC_LIST_NEXT(curr, publink);
-               }
-               ISC_LIST_UNLINK(find->list, best, publink);
-               ISC_LIST_APPEND(sorted, best, publink);
-       }
-       find->list = sorted;
-}
-
-/*
- * Sort a list of finds by server RTT.
- */
-static void
-sort_finds(dns_adbfindlist_t *findlist, unsigned int bias) {
-       dns_adbfind_t *best = NULL;
-       dns_adbfindlist_t sorted;
-       dns_adbaddrinfo_t *addrinfo, *bestaddrinfo;
-
-       /* Sort each find's addrinfo list by SRTT. */
-       ISC_LIST_FOREACH(*findlist, curr, publink) {
-               sort_adbfind(curr, bias);
-       }
-
-       /* Lame N^2 bubble sort. */
-       ISC_LIST_INIT(sorted);
-       while (!ISC_LIST_EMPTY(*findlist)) {
-               dns_adbfind_t *curr = NULL;
-               unsigned int best_srtt;
-
-               best = ISC_LIST_HEAD(*findlist);
-               bestaddrinfo = ISC_LIST_HEAD(best->list);
-               INSIST(bestaddrinfo != NULL);
-               best_srtt = bestaddrinfo->srtt;
-               if (isc_sockaddr_pf(&bestaddrinfo->sockaddr) != AF_INET6) {
-                       best_srtt += bias;
-               }
-               curr = ISC_LIST_NEXT(best, publink);
-               while (curr != NULL) {
-                       unsigned int curr_srtt;
-                       addrinfo = ISC_LIST_HEAD(curr->list);
-                       INSIST(addrinfo != NULL);
-                       curr_srtt = addrinfo->srtt;
-                       if (isc_sockaddr_pf(&addrinfo->sockaddr) != AF_INET6) {
-                               curr_srtt += bias;
-                       }
-                       if (curr_srtt < best_srtt) {
-                               best = curr;
-                               best_srtt = curr_srtt;
-                       }
-                       curr = ISC_LIST_NEXT(curr, publink);
-               }
-               ISC_LIST_UNLINK(*findlist, best, publink);
-               ISC_LIST_APPEND(sorted, best, publink);
-       }
-       *findlist = sorted;
-}
-
 /*
  * Return true iff the ADB find has an already pending fetch for 'type'.  This
  * is used to find out whether we're in a loop, where a fetch is waiting for a
@@ -3459,6 +3385,7 @@ findname(fetchctx_t *fctx, const dns_name_t *name, in_port_t port,
                                }
                        }
                }
+
                if ((flags & FCTX_ADDRINFO_DUALSTACK) != 0) {
                        ISC_LIST_APPEND(fctx->altfinds, find, publink);
                } else {
@@ -4027,8 +3954,6 @@ out:
                 * We've found some addresses.  We might still be
                 * looking for more addresses.
                 */
-               sort_finds(&fctx->finds, res->view->v6bias);
-               sort_finds(&fctx->altfinds, 0);
                return ISC_R_SUCCESS;
        }
 
@@ -4140,6 +4065,76 @@ possibly_mark(fetchctx_t *fctx, dns_adbaddrinfo_t *addr) {
        }
 }
 
+static dns_adbaddrinfo_t *
+nextaddress(fetchctx_t *fctx) {
+       dns_adbaddrinfo_t *prevai = fctx->foundaddrinfo, *lowestsrttai = NULL;
+       unsigned int v6bias = fctx->res->view->v6bias, lowestsrtt = 0;
+
+       /*
+        * Let's walk through the list of dns_adbaddrinfo_t to find the best
+        * next server address to query. This is linear on the number of
+        * dns_adbaddrinfo_t which are grouped in find list (for each ADB find).
+        */
+       ISC_LIST_FOREACH(fctx->finds, find, publink) {
+               ISC_LIST_FOREACH(find->list, ai, publink) {
+                       /*
+                        * This address has been marked already, skip it.
+                        */
+                       if (!UNMARKED(ai)) {
+                               continue;
+                       }
+
+                       /*
+                        * This address is the same as the previously used
+                        * address, it's a duplicate, mark it and skip it!
+                        */
+                       if (prevai != NULL) {
+                               if (prevai->entry == ai->entry) {
+                                       ai->flags |= FCTX_ADDRINFO_MARK;
+                                       continue;
+                               }
+                       }
+
+                       /*
+                        * Mark and skip this address if incompatible (i.e. IPv6
+                        * address on a v4 only server, or for ACL reason, etc.)
+                        */
+                       possibly_mark(fctx, ai);
+                       if (!UNMARKED(ai)) {
+                               continue;
+                       }
+
+                       /*
+                        * This address hasn't been tried yet and is a
+                        * good candidate. Let's keep track of it if it
+                        * has the lowest SRTT so far (or if there is no
+                        * address with lowest SRTT found yet).
+                        */
+                       unsigned int aisrtt = ai->srtt;
+
+                       if (isc_sockaddr_pf(&ai->sockaddr) != AF_INET6) {
+                               aisrtt += v6bias;
+                       }
+
+                       if (lowestsrttai == NULL || aisrtt < lowestsrtt) {
+                               lowestsrttai = ai;
+                               lowestsrtt = aisrtt;
+                               continue;
+                       }
+               }
+       }
+
+       /*
+        * This is the next address to query. If this is NULL, we're done.
+        */
+       if (lowestsrttai != NULL) {
+               lowestsrttai->flags |= FCTX_ADDRINFO_MARK;
+       }
+       fctx->foundaddrinfo = lowestsrttai;
+
+       return lowestsrttai;
+}
+
 static dns_adbaddrinfo_t *
 fctx_nextaddress(fetchctx_t *fctx) {
        dns_adbfind_t *find = NULL, *start = NULL;
@@ -4159,7 +4154,6 @@ fctx_nextaddress(fetchctx_t *fctx) {
                possibly_mark(fctx, ai);
                if (UNMARKED(ai)) {
                        ai->flags |= FCTX_ADDRINFO_MARK;
-                       fctx->find = NULL;
                        fctx->forwarding = true;
 
                        /*
@@ -4180,44 +4174,7 @@ fctx_nextaddress(fetchctx_t *fctx) {
        fctx->forwarding = false;
        FCTX_ATTR_SET(fctx, FCTX_ATTR_TRIEDFIND);
 
-       find = fctx->find;
-       if (find == NULL) {
-               find = ISC_LIST_HEAD(fctx->finds);
-       } else {
-               find = ISC_LIST_NEXT(find, publink);
-               if (find == NULL) {
-                       find = ISC_LIST_HEAD(fctx->finds);
-               }
-       }
-
-       /*
-        * Find the first unmarked addrinfo.
-        */
-       if (find != NULL) {
-               start = find;
-               do {
-                       ISC_LIST_FOREACH(find->list, ai, publink) {
-                               if (!UNMARKED(ai)) {
-                                       continue;
-                               }
-                               possibly_mark(fctx, ai);
-                               if (UNMARKED(ai)) {
-                                       ai->flags |= FCTX_ADDRINFO_MARK;
-                                       faddrinfo = ai;
-                                       break;
-                               }
-                       }
-                       if (faddrinfo != NULL) {
-                               break;
-                       }
-                       find = ISC_LIST_NEXT(find, publink);
-                       if (find == NULL) {
-                               find = ISC_LIST_HEAD(fctx->finds);
-                       }
-               } while (find != start);
-       }
-
-       fctx->find = find;
+       faddrinfo = nextaddress(fctx);
        if (faddrinfo != NULL) {
                return faddrinfo;
        }