properly delete empty qpzone nodes

the closeversion() implementation now correctly detects cases
where the only data left in a node is a single "nonexistent" header,
and deletes the node before committing.

lib/dns/qpcache.c

@@ -202,10 +202,9 @@ struct qpcnode {
 	uint8_t	      : 0;
 
 	/*%
-	 * Used for dead nodes cleaning.  This linked list is used to mark nodes
-	 * which have no data any longer, but we cannot unlink at that exact
-	 * moment because we did not or could not obtain a write lock on the
-	 * tree.
+	 * Used for dead node cleaning. The deadnodes queue is used
+	 * for nodes that have no data any longer, but we can't unlink
+	 * yet because we don't have a tree lock.
 	 */
 	isc_queue_node_t deadlink;
 };
@@ -216,9 +215,8 @@ struct qpcnode {
  * to reduce contention between threads.
  */
 typedef struct qpcache_bucket {
-	/*%
-	 * Temporary storage for stale cache nodes and dynamically
-	 * deleted nodes that await being cleaned up.
+	/*
+	 * Temporary storage for cache nodes that need to be deleted.
 	 */
 	isc_queue_t deadnodes;
 

lib/dns/qpzone.c

@@ -875,6 +875,28 @@ clean_zone_node(qpznode_t *node, uint32_t least_serial) {
 		}
 		top_prev = current;
 	}
+
+	/*
+	 * If all that's left for a type is a single
+	 * NONEXISTENT header, delete it.
+	 */
+	top_prev = NULL;
+	for (current = node->data; current != NULL; current = top_next) {
+		top_next = current->next;
+		if (!NONEXISTENT(current) || current->down != NULL) {
+			top_prev = current;
+			continue;
+		}
+
+		if (top_prev == NULL) {
+			node->data = current->next;
+		} else {
+			top_prev->next = current->next;
+		}
+
+		dns_slabheader_destroy(&current);
+	}
+
 	if (!still_dirty) {
 		node->dirty = false;
 	}
@@ -1526,10 +1548,8 @@ closeversion(dns_db_t *db, dns_dbversion_t **versionp,
 		NODE_UNLOCK(nlock, &nlocktype);
 	}
 
-	if (ISC_LIST_EMPTY(cleanup_list)) {
-		*versionp = NULL;
-		return;
-	}
+	dns_qp_t *tree = NULL, *nsec = NULL, *nsec3 = NULL;
+	bool need_tree = false, need_nsec = false, need_nsec3 = false;
 
 	for (changed = ISC_LIST_HEAD(cleanup_list); changed != NULL;
 	     changed = next_changed)
@@ -1545,14 +1565,104 @@ closeversion(dns_db_t *db, dns_dbversion_t **versionp,
 		if (rollback) {
 			rollback_node(node, serial);
 		}
+
+		qpznode_ref(node);
 		qpznode_release(qpdb, node, least_serial,
 				&nlocktype DNS__DB_FILELINE);
 
+		/* If the node is now empty, we can delete it. */
+		if (commit && node->data == NULL) {
+			switch ((int)node->nsec) {
+			case DNS_DB_NSEC_HAS_NSEC:
+				/*
+				 * Delete the matching node from the NSEC tree
+				 * first, then fall through to the main tree.
+				 */
+				if (nsec == NULL) {
+					need_nsec = true;
+					next_changed = changed;
+				} else {
+					dns_qp_deletename(nsec, &node->name,
+							  NULL, NULL);
+				}
+				FALLTHROUGH;
+			case DNS_DB_NSEC_NORMAL:
+				if (tree == NULL) {
+					need_tree = true;
+					next_changed = changed;
+				} else {
+					dns_qp_deletename(tree, &node->name,
+							  NULL, NULL);
+				}
+				break;
+			case DNS_DB_NSEC_NSEC:
+				if (nsec == NULL) {
+					need_nsec = true;
+					next_changed = changed;
+				} else {
+					dns_qp_deletename(nsec, &node->name,
+							  NULL, NULL);
+				}
+				break;
+			case DNS_DB_NSEC_NSEC3:
+				if (nsec3 == NULL) {
+					need_nsec3 = true;
+					next_changed = changed;
+				} else {
+					dns_qp_deletename(nsec3, &node->name,
+							  NULL, NULL);
+				}
+				break;
+			default:
+				UNREACHABLE();
+			}
+		}
+
+		if (next_changed == changed) {
+			qpznode_acquire(qpdb, node DNS__DB_FLARG_PASS);
+		}
+
+		qpznode_detach(&node);
+
 		NODE_UNLOCK(nlock, &nlocktype);
 
+		if (next_changed == changed) {
+			/*
+			 * We found a node to delete but didn't have a
+			 * QP writer open, so we open one now, then go
+			 * back to delete the node. If there's a next
+			 * time, we'll already have the writer open,
+			 * so we won't need this extra step.
+			 */
+			if (need_tree && tree == NULL) {
+				dns_qpmulti_write(qpdb->tree, &tree);
+			}
+			if (need_nsec && nsec == NULL) {
+				dns_qpmulti_write(qpdb->nsec, &nsec);
+			}
+			if (need_nsec3 && nsec3 == NULL) {
+				dns_qpmulti_write(qpdb->nsec3, &nsec3);
+			}
+
+			continue;
+		}
+
 		isc_mem_put(qpdb->common.mctx, changed, sizeof(*changed));
 	}
 
+	if (tree != NULL) {
+		dns_qp_compact(tree, DNS_QPGC_MAYBE);
+		dns_qpmulti_commit(qpdb->tree, &tree);
+	}
+	if (nsec != NULL) {
+		dns_qp_compact(nsec, DNS_QPGC_MAYBE);
+		dns_qpmulti_commit(qpdb->nsec, &nsec);
+	}
+	if (nsec3 != NULL) {
+		dns_qp_compact(nsec3, DNS_QPGC_MAYBE);
+		dns_qpmulti_commit(qpdb->nsec3, &nsec3);
+	}
+
 	*versionp = NULL;
 }