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.

(cherry picked from commit ced6b66fafa1badfd044d569a6d3cfbdb600f5a7)

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index 016d8208e8821234815ec1fe248b859c724eee67..1a4dbc6c6003ea745db75987122c73aeea0bb7a1 100644 (file)
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -300,7 +300,7 @@ struct fetchctx {
        dns_message_t *                 qmessage;
        ISC_LIST(resquery_t)            queries;
        dns_adbfindlist_t               finds;
-       dns_adbfind_t *                 find;
+       dns_adbaddrinfo_t               *foundaddrinfo;
        dns_adbfindlist_t               altfinds;
        dns_adbfind_t *                 altfind;
        dns_adbaddrinfolist_t           forwaddrs;
@@ -1192,7 +1192,7 @@ fctx_cleanupfinds(fetchctx_t *fctx) {
                ISC_LIST_UNLINK(fctx->finds, find, publink);
                dns_adb_destroyfind(&find);
        }
-       fctx->find = NULL;
+       fctx->foundaddrinfo = NULL;
 }
 
 static void
@@ -3100,83 +3100,6 @@ add_bad(fetchctx_t *fctx, dns_message_t *rmessage, dns_adbaddrinfo_t *addrinfo,
                      namebuf, typebuf, 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;
-       unsigned int best_srtt, curr_srtt;
-
-       /* Lame N^2 bubble sort. */
-       ISC_LIST_INIT(sorted);
-       while (!ISC_LIST_EMPTY(find->list)) {
-               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) {
-                       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, *curr;
-       dns_adbfindlist_t sorted;
-       dns_adbaddrinfo_t *addrinfo, *bestaddrinfo;
-       unsigned int best_srtt, curr_srtt;
-
-       /* Sort each find's addrinfo list by SRTT. */
-       for (curr = ISC_LIST_HEAD(*findlist);
-            curr != NULL;
-            curr = ISC_LIST_NEXT(curr, publink))
-               sort_adbfind(curr, bias);
-
-       /* Lame N^2 bubble sort. */
-       ISC_LIST_INIT(sorted);
-       while (!ISC_LIST_EMPTY(*findlist)) {
-               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) {
-                       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;
-}
-
 static void
 findname(fetchctx_t *fctx, dns_name_t *name, in_port_t port,
         unsigned int options, unsigned int flags, isc_stdtime_t now,
@@ -3615,8 +3538,6 @@ fctx_getaddresses(fetchctx_t *fctx, bool badcache) {
                 * 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);
                result = ISC_R_SUCCESS;
        }
 
@@ -3688,6 +3609,80 @@ 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).
+        */
+       for (dns_adbfind_t *find = ISC_LIST_HEAD(fctx->finds); find != NULL;
+            find = ISC_LIST_NEXT(find, publink))
+       {
+               for (dns_adbaddrinfo_t *ai = ISC_LIST_HEAD(find->list);
+                    ai != NULL; ai = ISC_LIST_NEXT(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 inline dns_adbaddrinfo_t *
 fctx_nextaddress(fetchctx_t *fctx) {
        dns_adbfind_t *find, *start;
@@ -3709,7 +3704,6 @@ fctx_nextaddress(fetchctx_t *fctx) {
                possibly_mark(fctx, addrinfo);
                if (UNMARKED(addrinfo)) {
                        addrinfo->flags |= FCTX_ADDRINFO_MARK;
-                       fctx->find = NULL;
                        return (addrinfo);
                }
        }
@@ -3720,45 +3714,11 @@ fctx_nextaddress(fetchctx_t *fctx) {
 
        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.
-        */
-       addrinfo = NULL;
-       if (find != NULL) {
-               start = find;
-               do {
-                       for (addrinfo = ISC_LIST_HEAD(find->list);
-                            addrinfo != NULL;
-                            addrinfo = ISC_LIST_NEXT(addrinfo, publink)) {
-                               if (!UNMARKED(addrinfo))
-                                       continue;
-                               possibly_mark(fctx, addrinfo);
-                               if (UNMARKED(addrinfo)) {
-                                       addrinfo->flags |= FCTX_ADDRINFO_MARK;
-                                       break;
-                               }
-                       }
-                       if (addrinfo != NULL)
-                               break;
-                       find = ISC_LIST_NEXT(find, publink);
-                       if (find == NULL)
-                               find = ISC_LIST_HEAD(fctx->finds);
-               } while (find != start);
+       faddrinfo = nextaddress(fctx);
+       if (faddrinfo != NULL) {
+               return faddrinfo;
        }
 
-       fctx->find = find;
-       if (addrinfo != NULL)
-               return (addrinfo);
-
        /*
         * No nameservers left.  Try alternates.
         */
@@ -4440,7 +4400,7 @@ fctx_create(dns_resolver_t *res, dns_name_t *name, dns_rdatatype_t type,
        ISC_LIST_INIT(fctx->bad_edns);
        ISC_LIST_INIT(fctx->validators);
        fctx->validator = NULL;
-       fctx->find = NULL;
+       fctx->foundaddrinfo = NULL;
        fctx->altfind = NULL;
        fctx->pending = 0;
        fctx->restarts = 0;