Re: merge-similar-outgoing-queries.patch and lame servers

On Wed, Mar 04, 2009 at 08:44:37AM +0000, Gerrit Pape wrote:

> > I believe what is happening is that dnscache is hanging in the iopause
> > call due to a bug in the qmerge patch. A patch (on top of qmerge) is
> > below. Let me know if it solves your problem.
> 
> The clients get the answer with this patch, but I don't feel that
> confident.  Two client queries 'dnsqr soa domain.with.lame.server & dnsqr
> soa domain.with.lame.server &' result in about 100 txpb and about 100
> lame log entries within 1.6 seconds before the aswer is sent back to the
> clients.

Hmm. OK, I see what is happening. The queries are being retried to
attempt the next non-lame server (the lame server is byte_zero()'d in
the servers list). But instead of realizing we have no other servers to
query, we notice that we already have a packet and hand that back out.
This only happens with multiple queries because you have one's reference
counter keeping the remembered packet alive until the other one looks at
it.

So with only lame servers, it is arriving at the correct answer, but it
is wasting a little time doing so (since it keeps looking at the same
packet instead of discarding it as generated by a lame server). But what
is much more serious is that a lame response prevents dnscache from
going on to the next server in line.

This is a failing of qmerge's state machine. It assumed we always either
had no packet, a packet, or an error. But that isn't true in the face of
lame packets, where we deliver the packet and still report an error.

The patch below somewhat fixes it; it records that the response is lame,
and doesn't allow new queries to merge into lame ones. So the next try
will then start with the fresh list of servers minus the lame one.

However, it has one failing: the zeroing of the lame server happens
through the dns_transmit object's server list, which is in turn a
pointer to the server list passed in by the first querier. Which means
that if N queries are sharing one dns_transmit object, only the first
will correctly have the lame server erased from its possibilities. The
other ones will see the lame response, and then on retrying use the lame
server again, eventually sending one packet per query.

In theory, you could use this to poison a lame server as if qmerge was
not applied. In practice, I'm not sure how well that will work since
each packet you send requires a round-trip to get the lame response from
the server.

Anyway, here is the patch (on top of the others -- once this is fully
resolved, I will post a cumulative patch).

---
diff --git a/qmerge.c b/qmerge.c
index be2fafd..1d478d1 100644
--- a/qmerge.c
+++ b/qmerge.c
@@ -61,6 +61,7 @@ int qmerge_start(struct qmerge **qm, const char servers[64], int flagrecursive,
   if (!qmerge_key_init(&k, q, qtype, control)) return -1;
   for (i = 0; i < QMERGE_MAX; i++) {
     if (!inprogress[i].active) continue;
+    if (inprogress[i].state == 3) continue; /* ignore lame */
     if (!qmerge_key_equal(&k, &inprogress[i].key)) continue;
     log_tx_piggyback(q, qtype, control);
     inprogress[i].active++;
@@ -107,6 +108,7 @@ int qmerge_get(struct qmerge **x, const iopause_fd *io, const struct taia *when)
   if (qm->state == -1) return -1; /* previous error */
   if (qm->state == 0) return 0; /* no packet */
   if (qm->state == 2) return 1; /* already got packet */
+  if (qm->state == 3) return 1; /* already got lame packet */
 
   r = dns_transmit_get(&qm->dt, io, when);
   if (r == -1) { qm->state = -1; return -1; } /* error */
@@ -114,3 +116,8 @@ int qmerge_get(struct qmerge **x, const iopause_fd *io, const struct taia *when)
   if (r == 1) { qm->state = 2; return 1; } /* got packet */
   return -1; /* bug */
 }
+
+void qmerge_lame(struct qmerge *qm)
+{
+  qm->state = 3;
+}
diff --git a/qmerge.h b/qmerge.h
index 9a58157..ea7ccc5 100644
--- a/qmerge.h
+++ b/qmerge.h
@@ -13,12 +13,14 @@ struct qmerge {
   int active;
   struct qmerge_key key;
   struct dns_transmit dt;
-  int state; /* -1 = error, 0 = need io, 1 = need get, 2 = got packet */
+  int state; /* -1 = error, 0 = need io, 1 = need get, 2 = got packet,
+                 3 = got lame packet */
 };
 
 extern int qmerge_start(struct qmerge **,const char *,int,const char *,const char *,const char *,const char *);
 extern void qmerge_io(struct qmerge *,iopause_fd *,struct taia *);
 extern int qmerge_get(struct qmerge **,const iopause_fd *,const struct taia *);
+extern void qmerge_lame(struct qmerge *);
 extern void qmerge_free(struct qmerge **);
 
 #endif /* QMERGE_H */
diff --git a/query.c b/query.c
index f091fdd..9bb9550 100644
--- a/query.c
+++ b/query.c
@@ -514,6 +514,7 @@ static int doit(struct query *z,int state)
     if (dns_domain_equal(referral,control) || !dns_domain_suffix(referral,control)) {
       log_lame(whichserver,control,referral);
       byte_zero(whichserver,4);
+      qmerge_lame(z->qm);
       goto HAVENS;
     }