From e58ce19cf22fd090e09918b842d8ec7f3772c77c Mon Sep 17 00:00:00 2001
From: Evan Hunt <each@isc.org>
Date: Tue, 11 Feb 2025 21:43:09 -0800
Subject: [PATCH] when committing a new qpzone version, delete dead nodes

if all data has been deleted from a node in the qpzone
database, delete the node too.
---
 lib/dns/qpcache.c | 12 +++---
 lib/dns/qpzone.c  | 93 +++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 94 insertions(+), 11 deletions(-)

diff --git a/lib/dns/qpcache.c b/lib/dns/qpcache.c
index 210c2abbbb..c5ea5a1e31 100644
--- a/lib/dns/qpcache.c
+++ b/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;
 
diff --git a/lib/dns/qpzone.c b/lib/dns/qpzone.c
index e772ee11be..d1bb246d2c 100644
--- a/lib/dns/qpzone.c
+++ b/lib/dns/qpzone.c
@@ -874,6 +874,7 @@ clean_zone_node(qpznode_t *node, uint32_t least_serial) {
 		}
 		top_prev = current;
 	}
+
 	if (!still_dirty) {
 		node->dirty = false;
 	}
@@ -1518,10 +1519,8 @@ closeversion(dns_db_t *db, dns_dbversion_t **versionp,
 		NODE_UNLOCK(nlock, &nlocktype);
 	}
 
-	if (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 = HEAD(cleanup_list); changed != NULL;
 	     changed = next_changed)
@@ -1537,14 +1536,100 @@ 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();
+			}
+		}
+
+		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;
 }