From aa3408235abf02654ccf703ad8d0031adc211bd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Wed, 13 Aug 2025 08:45:45 +0200 Subject: [PATCH 1/2] Always return DNS_R_UNCHANGED when new slabheader was not added Change the add() function in the dns_qpcache to properly return DNS_R_UNCHANGED if the newheader was not actually consumed, and move the dns_slabheader_destroy() call outside of the add() function. --- lib/dns/qpcache.c | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/lib/dns/qpcache.c b/lib/dns/qpcache.c index c5de17203a..c61b57a50a 100644 --- a/lib/dns/qpcache.c +++ b/lib/dns/qpcache.c @@ -55,6 +55,7 @@ #include #include #include +#include #include #include "db_p.h" @@ -841,9 +842,9 @@ static void mark(dns_slabheader_t *header, uint_least16_t flag) { uint_least16_t attributes = atomic_load_acquire(&header->attributes); uint_least16_t newattributes = 0; + qpcache_t *qpdb = HEADERNODE(header)->qpdb; dns_stats_t *stats = NULL; - qpcache_t *qpdb = HEADERNODE(header)->qpdb; /* * If we are already ancient there is nothing to do. */ @@ -860,10 +861,8 @@ mark(dns_slabheader_t *header, uint_least16_t flag) { * RRtype. */ stats = dns_db_getrrsetstats(&qpdb->common); - if (stats != NULL) { - update_rrsetstats(stats, header->typepair, attributes, false); - update_rrsetstats(stats, header->typepair, newattributes, true); - } + update_rrsetstats(stats, header->typepair, attributes, false); + update_rrsetstats(stats, header->typepair, newattributes, true); } static void @@ -2654,7 +2653,6 @@ add(qpcache_t *qpdb, qpcnode_t *qpnode, * The NXDOMAIN/NODATA(QTYPE=ANY) * is more trusted. */ - dns_slabheader_destroy(&newheader); if (addedrdataset != NULL) { bindrdataset( qpdb, qpnode, topheader, @@ -2703,7 +2701,6 @@ find_header: * Deleting an already non-existent rdataset has no effect. */ if (!EXISTS(header) && !EXISTS(newheader)) { - dns_slabheader_destroy(&newheader); return DNS_R_UNCHANGED; } @@ -2717,7 +2714,6 @@ find_header: if (trust < header->trust && (ACTIVE(header, now) || !EXISTS(header))) { - dns_slabheader_destroy(&newheader); if (addedrdataset != NULL) { bindrdataset(qpdb, qpnode, header, now, nlocktype, tlocktype, @@ -2764,13 +2760,12 @@ find_header: header->closest = newheader->closest; newheader->closest = NULL; } - dns_slabheader_destroy(&newheader); if (addedrdataset != NULL) { bindrdataset(qpdb, qpnode, header, now, nlocktype, tlocktype, addedrdataset DNS__DB_FLARG_PASS); } - return ISC_R_SUCCESS; + return DNS_R_UNCHANGED; } /* @@ -2819,13 +2814,12 @@ find_header: header->closest = newheader->closest; newheader->closest = NULL; } - dns_slabheader_destroy(&newheader); if (addedrdataset != NULL) { bindrdataset(qpdb, qpnode, header, now, nlocktype, tlocktype, addedrdataset DNS__DB_FLARG_PASS); } - return ISC_R_SUCCESS; + return DNS_R_UNCHANGED; } qpcache_miss(qpdb, newheader, &nlocktype, @@ -2848,7 +2842,6 @@ find_header: * The type already doesn't exist; no point trying * to delete it. */ - dns_slabheader_destroy(&newheader); return DNS_R_UNCHANGED; } else { /* No rdatasets of the given type exist at the node. */ @@ -3051,7 +3044,6 @@ qpcache_addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, result = addnoqname(qpnode->mctx, newheader, qpdb->maxrrperset, rdataset); if (result != ISC_R_SUCCESS) { - dns_slabheader_destroy(&newheader); return result; } } @@ -3059,7 +3051,6 @@ qpcache_addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, result = addclosest(qpnode->mctx, newheader, qpdb->maxrrperset, rdataset); if (result != ISC_R_SUCCESS) { - dns_slabheader_destroy(&newheader); return result; } } @@ -3125,8 +3116,12 @@ qpcache_addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, result = add(qpdb, qpnode, name, newheader, options, addedrdataset, now, nlocktype, tlocktype DNS__DB_FLARG_PASS); - if (result == ISC_R_SUCCESS && delegating) { - qpnode->delegating = 1; + if (result == ISC_R_SUCCESS) { + if (delegating) { + qpnode->delegating = 1; + } + } else { + dns_slabheader_destroy(&newheader); } NODE_UNLOCK(nlock, &nlocktype); @@ -3181,6 +3176,9 @@ qpcache_deleterdataset(dns_db_t *db, dns_dbnode_t *node, NODE_WRLOCK(nlock, &nlocktype); result = add(qpdb, qpnode, NULL, newheader, DNS_DBADD_FORCE, NULL, 0, nlocktype, isc_rwlocktype_none DNS__DB_FLARG_PASS); + if (result != ISC_R_SUCCESS) { + dns_slabheader_destroy(&newheader); + } NODE_UNLOCK(nlock, &nlocktype); return result; From 7b882474648166b6ddc7a9f59ce130153d68c747 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Wed, 13 Aug 2025 08:45:45 +0200 Subject: [PATCH 2/2] Don't count failed additions into the cache Previously, when the new header was NOT added into the cache, we would increment and then decrement stat counters immediately. Delay incrementing the stat counters until after the newheader has been actually added into the database. A little cleanup to accomodate the fact that qpdb->rrsetstats is always available was also done here. --- lib/dns/qpcache.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/lib/dns/qpcache.c b/lib/dns/qpcache.c index c61b57a50a..8a2cd03879 100644 --- a/lib/dns/qpcache.c +++ b/lib/dns/qpcache.c @@ -843,7 +843,6 @@ mark(dns_slabheader_t *header, uint_least16_t flag) { uint_least16_t attributes = atomic_load_acquire(&header->attributes); uint_least16_t newattributes = 0; qpcache_t *qpdb = HEADERNODE(header)->qpdb; - dns_stats_t *stats = NULL; /* * If we are already ancient there is nothing to do. @@ -860,9 +859,10 @@ mark(dns_slabheader_t *header, uint_least16_t flag) { * Decrement and increment the stats counter for the appropriate * RRtype. */ - stats = dns_db_getrrsetstats(&qpdb->common); - update_rrsetstats(stats, header->typepair, attributes, false); - update_rrsetstats(stats, header->typepair, newattributes, true); + update_rrsetstats(qpdb->rrsetstats, header->typepair, attributes, + false); + update_rrsetstats(qpdb->rrsetstats, header->typepair, newattributes, + true); } static void @@ -2279,9 +2279,8 @@ qpcache__destroy(qpcache_t *qpdb) { isc_heap_destroy(&qpdb->buckets[i].heap); } - if (qpdb->rrsetstats != NULL) { - dns_stats_detach(&qpdb->rrsetstats); - } + dns_stats_detach(&qpdb->rrsetstats); + if (qpdb->cachestats != NULL) { isc_stats_detach(&qpdb->cachestats); } @@ -3087,13 +3086,6 @@ qpcache_addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, NODE_WRLOCK(nlock, &nlocktype); - if (qpdb->rrsetstats != NULL) { - DNS_SLABHEADER_SETATTR(newheader, DNS_SLABHEADERATTR_STATCOUNT); - update_rrsetstats(qpdb->rrsetstats, newheader->typepair, - atomic_load_acquire(&newheader->attributes), - true); - } - expire_ttl_headers(qpdb, qpnode->locknum, &nlocktype, &tlocktype, now DNS__DB_FLARG_PASS); @@ -3117,6 +3109,10 @@ qpcache_addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, nlocktype, tlocktype DNS__DB_FLARG_PASS); if (result == ISC_R_SUCCESS) { + DNS_SLABHEADER_SETATTR(newheader, DNS_SLABHEADERATTR_STATCOUNT); + update_rrsetstats(qpdb->rrsetstats, newheader->typepair, + newheader->attributes, true); + if (delegating) { qpnode->delegating = 1; }