Improve node reference counting

QP database node data is not reference counted the same way RBT nodes
were: in the RBT, node->references could be zero if the node was in the
tree but was not in use by any caller, whereas in the QP trie, the
database itself uses reference counting of nodes internally.

this caused some subtle errors. in RBTDB, when the newref() function is
called and the node reference count was zero, the node lock reference
counter would also be incremented. in the QP trie, this can never
happen - because as long as the node is in the database its reference
count cannot be zero - and so the node lock reference counter was never
incremented.

reference counting will probably need to be refactored in more detail
later; the node lock reference count may not be needed at all.  but
for now, as a temporary measure, we add a third reference counter,
'erefs' (external references), to the dns_qpdata structure. this is
counted separately from the main reference counter, and should match
the node reference count as it would have been in RBTDB.

this change revealed a number of places where the node reference counter
was being incremented on behalf of a caller without newref() being
called; those were cleaned up as well.

This is an adaptation of commit 3dd686261d2c4bcd15a96ebfea10baffa277732b

diff --git a/lib/dns/qp-cachedb.c b/lib/dns/qp-cachedb.c
index 1d8d03275e0e07ce72d1eb5d894adddfdb0992e4..e5c351453f529c29983b4a9078679af55ff18841 100644 (file)
--- a/lib/dns/qp-cachedb.c
+++ b/lib/dns/qp-cachedb.c
@@ -1562,7 +1562,7 @@ dns__qpcache_expireheader(dns_slabheader_t *header,
        dns__qpdb_mark(header, DNS_SLABHEADERATTR_ANCIENT);
        QPDB_HEADERNODE(header)->dirty = 1;
 
-       if (isc_refcount_current(&QPDB_HEADERNODE(header)->references) == 0) {
+       if (isc_refcount_current(&QPDB_HEADERNODE(header)->erefs) == 0) {
                isc_rwlocktype_t nlocktype = isc_rwlocktype_write;
                dns_qpdb_t *qpdb = (dns_qpdb_t *)header->db;
 

diff --git a/lib/dns/qpdb.c b/lib/dns/qpdb.c
index f41f4f7375083f0c95b7b7df754a1680357dc799..8b87c59c4dd08029668e38fd9d9b326b6ef091f9 100644 (file)
--- a/lib/dns/qpdb.c
+++ b/lib/dns/qpdb.c
@@ -775,14 +775,10 @@ add_changed(dns_slabheader_t *header,
 
        if (changed != NULL) {
                dns_qpdata_t *node = (dns_qpdata_t *)header->node;
-               uint_fast32_t refs = isc_refcount_increment(&node->references);
-#if DNS_DB_NODETRACE
-               fprintf(stderr,
-                       "incr:node:%s:%s:%u:%p->references = %" PRIuFAST32 "\n",
-                       func, file, line, node, refs + 1);
-#else
-               UNUSED(refs);
-#endif
+
+               dns__qpdb_newref(qpdb, node,
+                                isc_rwlocktype_none DNS__DB_FLARG_PASS);
+
                changed->node = node;
                changed->dirty = false;
                ISC_LIST_INITANDAPPEND(version->changed_list, changed, link);
@@ -1105,16 +1101,18 @@ dns__qpdb_newref(dns_qpdb_t *qpdb, dns_qpdata_t *node,
                ISC_LIST_UNLINK(qpdb->deadnodes[node->locknum], node, deadlink);
        }
 
-       refs = isc_refcount_increment0(&node->references);
+       dns_qpdata_ref(node);
+       refs = isc_refcount_increment0(&node->erefs);
+
 #if DNS_DB_NODETRACE
-       fprintf(stderr, "incr:node:%s:%s:%u:%p->references = %" PRIuFAST32 "\n",
+       fprintf(stderr, "incr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n",
                func, file, line, node, refs + 1);
 #else
        UNUSED(refs);
 #endif
 
        if (refs == 0) {
-               /* this is the first reference to the node */
+               /* this is the first external reference to the node */
                refs = isc_refcount_increment0(
                        &qpdb->node_locks[node->locknum].references);
 #if DNS_DB_NODETRACE
@@ -1263,11 +1261,13 @@ reactivate_node(dns_qpdb_t *qpdb, dns_qpdata_t *node,
  * threads are decreasing the reference to zero simultaneously and at least
  * one of them is going to free the node.
  *
- * This function returns true if and only if the node reference decreases
- * to zero.
+ * This decrements both the internal and external node reference counters.
+ * If the external reference count drops to zero, then the node lock
+ * reference count is also decremented.
  *
- * NOTE: Decrementing the reference count of a node to zero does not mean it
- * will be immediately freed.
+ * This function returns true if and only if the node reference decreases
+ * to zero.  (NOTE: Decrementing the reference count of a node to zero does
+ * not mean it will be immediately freed.)
  */
 bool
 dns__qpdb_decref(dns_qpdb_t *qpdb, dns_qpdata_t *node, uint32_t least_serial,
@@ -1291,10 +1291,11 @@ dns__qpdb_decref(dns_qpdb_t *qpdb, dns_qpdata_t *node, uint32_t least_serial,
 
        /* Handle easy and typical case first. */
        if (!node->dirty && KEEP_NODE(node, qpdb, locked)) {
-               refs = isc_refcount_decrement(&node->references);
+               refs = isc_refcount_decrement(&node->erefs);
+
 #if DNS_DB_NODETRACE
                fprintf(stderr,
-                       "decr:node:%s:%s:%u:%p->references = %" PRIuFAST32 "\n",
+                       "decr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n",
                        func, file, line, node, refs - 1);
 #else
                UNUSED(refs);
@@ -1309,10 +1310,13 @@ dns__qpdb_decref(dns_qpdb_t *qpdb, dns_qpdata_t *node, uint32_t least_serial,
 #else
                        UNUSED(refs);
 #endif
-                       return (true);
+                       no_reference = true;
                } else {
-                       return (false);
+                       no_reference = false;
                }
+
+               dns_qpdata_unref(node);
+               return (no_reference);
        }
 
        /* Upgrade the lock? */
@@ -1320,17 +1324,19 @@ dns__qpdb_decref(dns_qpdb_t *qpdb, dns_qpdata_t *node, uint32_t least_serial,
                NODE_FORCEUPGRADE(&nodelock->lock, nlocktypep);
        }
 
-       refs = isc_refcount_decrement(&node->references);
+       refs = isc_refcount_decrement(&node->erefs);
 #if DNS_DB_NODETRACE
-       fprintf(stderr, "decr:node:%s:%s:%u:%p->references = %" PRIuFAST32 "\n",
+       fprintf(stderr, "decr:node:%s:%s:%u:%p->erefs = %" PRIuFAST32 "\n",
                func, file, line, node, refs - 1);
-#else
-       UNUSED(refs);
 #endif
+
        if (refs > 1) {
+               dns_qpdata_unref(node);
                return (false);
        }
 
+       INSIST(refs == 1);
+
        if (node->dirty) {
                if (IS_CACHE(qpdb)) {
                        clean_cache_node(qpdb, node);
@@ -1358,6 +1364,7 @@ dns__qpdb_decref(dns_qpdb_t *qpdb, dns_qpdata_t *node, uint32_t least_serial,
         * we only do a trylock.
         */
        /* We are allowed to upgrade the tree lock */
+
        switch (*tlocktypep) {
        case isc_rwlocktype_write:
                result = ISC_R_SUCCESS;
@@ -1438,6 +1445,7 @@ restore_locks:
                TREE_UNLOCK(&qpdb->tree_lock, tlocktypep);
        }
 
+       dns_qpdata_unref(node);
        return (no_reference);
 }
 
@@ -2160,15 +2168,10 @@ dns__qpdb_attachnode(dns_db_t *db, dns_dbnode_t *source,
        REQUIRE(VALID_QPDB((dns_qpdb_t *)db));
        REQUIRE(targetp != NULL && *targetp == NULL);
 
+       dns_qpdb_t *qpdb = (dns_qpdb_t *)db;
        dns_qpdata_t *node = (dns_qpdata_t *)source;
-       uint_fast32_t refs = isc_refcount_increment(&node->references);
 
-#if DNS_DB_NODETRACE
-       fprintf(stderr, "incr:node:%s:%s:%u:%p->references = %" PRIuFAST32 "\n",
-               func, file, line, node, refs + 1);
-#else
-       UNUSED(refs);
-#endif
+       dns__qpdb_newref(qpdb, node, isc_rwlocktype_none DNS__DB_FLARG_PASS);
 
        *targetp = source;
 }
@@ -2282,7 +2285,6 @@ dns__qpdb_allrdatasets(dns_db_t *db, dns_dbnode_t *node,
        dns_qpdata_t *qpnode = (dns_qpdata_t *)node;
        dns_qpdb_version_t *rbtversion = version;
        qpdb_rdatasetiter_t *iterator = NULL;
-       uint_fast32_t refs;
 
        REQUIRE(VALID_QPDB(qpdb));
 
@@ -2312,17 +2314,10 @@ dns__qpdb_allrdatasets(dns_db_t *db, dns_dbnode_t *node,
        iterator->common.version = (dns_dbversion_t *)rbtversion;
        iterator->common.options = options;
        iterator->common.now = now;
-
-       refs = isc_refcount_increment(&qpnode->references);
-#if DNS_DB_NODETRACE
-       fprintf(stderr, "incr:node:%s:%s:%u:%p->references = %" PRIuFAST32 "\n",
-               func, file, line, node, refs + 1);
-#else
-       UNUSED(refs);
-#endif
-
        iterator->current = NULL;
 
+       dns__qpdb_newref(qpdb, qpnode, isc_rwlocktype_none DNS__DB_FLARG_PASS);
+
        *iteratorp = (dns_rdatasetiter_t *)iterator;
 
        return (ISC_R_SUCCESS);

diff --git a/lib/dns/qpdb_p.h b/lib/dns/qpdb_p.h
index ab0d589f24531373f78d940aa357070bf9b59099..4239419cbe3efa39f50bb9e568f2b4bb7d267118 100644 (file)
--- a/lib/dns/qpdb_p.h
+++ b/lib/dns/qpdb_p.h
@@ -135,6 +135,7 @@ struct dns_qpdata {
        uint8_t       : 0; /* end of bitfields c/o node lock */
        uint16_t locknum;  /* note that this is not in the bitfield */
        isc_refcount_t references;
+       isc_refcount_t erefs;
        /*@}*/
 };