From 0b96c9234fb157e0a06c9906263fa7c631e20a4d Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Tue, 21 Feb 2023 14:23:38 +0000 Subject: [PATCH 1/4] Offload catalog zone updates Offload catalog zone processing so that the network manager threads are not interrupted by a large catalog zone update. Introduce a new 'updaterunning' state alongside with 'updatepending', like it is done in the RPZ module. Note that the dns__catz_update_cb() function currently holds the catzs->lock during the whole process, which is far from being optimal, but the issue is going to be addressed separately. --- lib/dns/catz.c | 127 ++++++++++++++++++++++++++++++------- lib/dns/include/dns/catz.h | 22 ------- 2 files changed, 103 insertions(+), 46 deletions(-) diff --git a/lib/dns/catz.c b/lib/dns/catz.c index ef1beb7b39..5ee2a83e5a 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -26,6 +26,7 @@ #include #include #include +#include #include #include @@ -73,6 +74,7 @@ struct dns_catz_zone { dns_name_t name; dns_catz_zones_t *catzs; dns_rdata_t soa; + uint32_t version; /* key in entries is 'mhash', not domain name! */ isc_ht_t *entries; /* key in coos is domain name */ @@ -85,11 +87,12 @@ struct dns_catz_zone { dns_catz_options_t defoptions; dns_catz_options_t zoneoptions; isc_time_t lastupdated; - bool updatepending; - uint32_t version; - dns_db_t *db; - dns_dbversion_t *dbversion; + bool updatepending; /* there is an update pending */ + bool updaterunning; /* there is an update running */ + isc_result_t updateresult; /* result from the offloaded work */ + dns_db_t *db; /* zones database */ + dns_dbversion_t *dbversion; /* zones database version */ isc_timer_t *updatetimer; @@ -100,11 +103,18 @@ struct dns_catz_zone { isc_refcount_t references; }; +static void +dns__catz_timer_cb(void *); static void dns__catz_timer_start(dns_catz_zone_t *catz); static void dns__catz_timer_stop(void *arg); +static void +dns__catz_update_cb(void *data); +static void +dns__catz_done_cb(void *data); + static isc_result_t catz_process_zones_entry(dns_catz_zone_t *catz, dns_rdataset_t *value, dns_label_t *mhash); @@ -826,7 +836,7 @@ dns__catz_timer_start(dns_catz_zone_t *catz) { catz->loop = isc_loop_current(catz->catzs->loopmgr); - isc_timer_create(catz->loop, dns_catz_update_action, catz, + isc_timer_create(catz->loop, dns__catz_timer_cb, catz, &catz->updatetimer); isc_timer_start(catz->updatetimer, isc_timertype_once, &interval); } @@ -980,6 +990,8 @@ dns__catz_zone_destroy(dns_catz_zone_t *catz) { dns_db_detach(&catz->db); } + INSIST(!catz->updaterunning); + dns_name_free(&catz->name, mctx); dns_catz_options_free(&catz->defoptions, mctx); dns_catz_options_free(&catz->zoneoptions, mctx); @@ -2029,21 +2041,41 @@ cleanup: return (result); } -void -dns_catz_update_action(void *arg) { +static void +dns__catz_timer_cb(void *arg) { + char domain[DNS_NAME_FORMATSIZE]; isc_result_t result; - dns_catz_zone_t *catz = arg; + dns_catz_zone_t *catz = (dns_catz_zone_t *)arg; REQUIRE(DNS_CATZ_ZONE_VALID(catz)); LOCK(&catz->catzs->lock); + + if (catz->catzs->shuttingdown) { + goto unlock; + } + + INSIST(DNS_DB_VALID(catz->db)); + INSIST(catz->dbversion != NULL); + catz->updatepending = false; - dns_catz_update_from_db(catz->db, catz->catzs); - isc_timer_stop(catz->updatetimer); + catz->updaterunning = true; + catz->updateresult = ISC_R_UNSET; + + dns_name_format(&catz->name, domain, DNS_NAME_FORMATSIZE); + isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_MASTER, + ISC_LOG_INFO, "catz: %s: reload start", domain); + + dns_catz_ref_catzs(catz->catzs); + isc_work_enqueue(catz->loop, dns__catz_update_cb, dns__catz_done_cb, + catz); + isc_timer_destroy(&catz->updatetimer); catz->loop = NULL; + result = isc_time_now(&catz->lastupdated); RUNTIME_CHECK(result == ISC_R_SUCCESS); +unlock: UNLOCK(&catz->catzs->lock); } @@ -2074,7 +2106,7 @@ dns_catz_dbupdate_callback(dns_db_t *db, void *fn_arg) { dns_db_detach(&catz->db); /* * We're not registering db update callback, it will be - * registered at the end of update_from_db + * registered at the end of dns__catz_update_cb() */ catz->db_registered = false; } @@ -2082,7 +2114,7 @@ dns_catz_dbupdate_callback(dns_db_t *db, void *fn_arg) { dns_db_attach(db, &catz->db); } - if (!catz->updatepending) { + if (!catz->updatepending && !catz->updaterunning) { catz->updatepending = true; dns_db_currentversion(db, &catz->dbversion); dns__catz_timer_start(catz); @@ -2113,8 +2145,16 @@ catz_rdatatype_is_processable(const dns_rdatatype_t type) { type != dns_rdatatype_cdnskey && type != dns_rdatatype_zonemd); } -void -dns_catz_update_from_db(dns_db_t *db, dns_catz_zones_t *catzs) { +/* + * Process an updated database for a catalog zone. + * It creates a new catz, iterates over database to fill it with content, and + * then merges new catz into old catz. + */ +static void +dns__catz_update_cb(void *data) { + dns_catz_zone_t *catz = (dns_catz_zone_t *)data; + dns_db_t *db = NULL; + dns_catz_zones_t *catzs = NULL; dns_catz_zone_t *oldcatz = NULL, *newcatz = NULL; isc_result_t result; isc_region_t r; @@ -2131,11 +2171,18 @@ dns_catz_update_from_db(dns_db_t *db, dns_catz_zones_t *catzs) { uint32_t vers; uint32_t catz_vers; - REQUIRE(DNS_DB_VALID(db)); - REQUIRE(DNS_CATZ_ZONES_VALID(catzs)); + REQUIRE(DNS_CATZ_ZONE_VALID(catz)); + REQUIRE(DNS_DB_VALID(catz->db)); + REQUIRE(DNS_CATZ_ZONES_VALID(catz->catzs)); + + db = catz->db; + catzs = catz->catzs; + + LOCK(&catzs->lock); if (atomic_load(&catzs->shuttingdown)) { - return; + result = ISC_R_SHUTTINGDOWN; + goto exit; } dns_name_format(&db->origin, bname, DNS_NAME_FORMATSIZE); @@ -2150,7 +2197,7 @@ dns_catz_update_from_db(dns_db_t *db, dns_catz_zones_t *catzs) { isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_MASTER, ISC_LOG_ERROR, "catz: zone '%s' not in config", bname); - return; + goto exit; } result = dns_db_getsoaserial(db, oldcatz->dbversion, &vers); @@ -2160,7 +2207,7 @@ dns_catz_update_from_db(dns_db_t *db, dns_catz_zones_t *catzs) { DNS_LOGMODULE_MASTER, ISC_LOG_ERROR, "catz: zone '%s' has no SOA record (%s)", bname, isc_result_totext(result)); - return; + goto exit; } isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_MASTER, @@ -2175,7 +2222,7 @@ dns_catz_update_from_db(dns_db_t *db, dns_catz_zones_t *catzs) { DNS_LOGMODULE_MASTER, ISC_LOG_ERROR, "catz: failed to create new zone - %s", isc_result_totext(result)); - return; + goto exit; } result = dns_db_createiterator(db, DNS_DB_NONSEC3, &it); @@ -2186,7 +2233,7 @@ dns_catz_update_from_db(dns_db_t *db, dns_catz_zones_t *catzs) { DNS_LOGMODULE_MASTER, ISC_LOG_ERROR, "catz: failed to create DB iterator - %s", isc_result_totext(result)); - return; + goto exit; } name = dns_fixedname_initname(&fixname); @@ -2205,7 +2252,7 @@ dns_catz_update_from_db(dns_db_t *db, dns_catz_zones_t *catzs) { DNS_LOGMODULE_MASTER, ISC_LOG_ERROR, "catz: failed to create name from string - %s", isc_result_totext(result)); - return; + goto exit; } result = dns_dbiterator_seek(it, name); if (result != ISC_R_SUCCESS) { @@ -2345,7 +2392,8 @@ final: "will not be processed", bname); dns_catz_detach_catz(&newcatz); - return; + result = ISC_R_FAILURE; + goto exit; } /* @@ -2359,7 +2407,7 @@ final: "catz: failed merging zones: %s", isc_result_totext(result)); - return; + goto exit; } isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_MASTER, @@ -2379,6 +2427,37 @@ final: oldcatz->db_registered = true; } } + +exit: + catz->updateresult = result; + + UNLOCK(&catzs->lock); +} + +static void +dns__catz_done_cb(void *data) { + dns_catz_zone_t *catz = (dns_catz_zone_t *)data; + char dname[DNS_NAME_FORMATSIZE]; + + REQUIRE(DNS_CATZ_ZONE_VALID(catz)); + + LOCK(&catz->catzs->lock); + catz->updaterunning = false; + + dns_name_format(&catz->name, dname, DNS_NAME_FORMATSIZE); + + if (catz->updatepending && !catz->catzs->shuttingdown) { + /* Restart the timer */ + dns__catz_timer_start(catz); + } + + UNLOCK(&catz->catzs->lock); + + isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_MASTER, + ISC_LOG_INFO, "catz: %s: reload done: %s", dname, + isc_result_totext(catz->updateresult)); + + dns_catz_unref_catzs(catz->catzs); } void diff --git a/lib/dns/include/dns/catz.h b/lib/dns/include/dns/catz.h index 67f96e2d21..8696dd2c2c 100644 --- a/lib/dns/include/dns/catz.h +++ b/lib/dns/include/dns/catz.h @@ -385,28 +385,6 @@ dns_catz_dbupdate_callback(dns_db_t *db, void *fn_arg); * \li 'fn_arg' is not NULL (casted to dns_catz_zones_t*). */ -void -dns_catz_update_action(void *arg); -/*%< - * Task that launches dns_catz_update_from_db. - * - * Requires: - * \li 'event' is not NULL. - */ - -void -dns_catz_update_from_db(dns_db_t *db, dns_catz_zones_t *catzs); -/*%< - * Process an updated database for a catalog zone. - * It creates a new catz, iterates over database to fill it with content, and - * then merges new catz into old catz. - * - * Requires: - * \li 'db' is a valid DB. - * \li 'catzs' is a valid dns_catz_zones_t. - * - */ - void dns_catz_prereconfig(dns_catz_zones_t *catzs); /*%< From b1cd4a066a63f221a56d9565da4907c2aad7e524 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 24 Feb 2023 09:46:00 +0000 Subject: [PATCH 2/4] Unlock catzs during dns__catz_update_cb() Instead of holding the catzs->lock the whole time we process the catz update, only hold it for hash table lookup and then release it. This should unblock any other threads that might be processing updates to catzs triggered by extra incoming transfer. --- lib/dns/catz.c | 33 ++++++++++++++++++--------------- lib/dns/include/dns/catz.h | 7 +++---- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/lib/dns/catz.c b/lib/dns/catz.c index 5ee2a83e5a..cb11236336 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -1769,8 +1769,8 @@ catz_process_value(dns_catz_zone_t *catz, dns_name_t *name, } isc_result_t -dns_catz_update_process(dns_catz_zones_t *catzs, dns_catz_zone_t *catz, - const dns_name_t *src_name, dns_rdataset_t *rdataset) { +dns_catz_update_process(dns_catz_zone_t *catz, const dns_name_t *src_name, + dns_rdataset_t *rdataset) { isc_result_t result; int order; unsigned int nlabels; @@ -1779,7 +1779,6 @@ dns_catz_update_process(dns_catz_zones_t *catzs, dns_catz_zone_t *catz, dns_rdata_soa_t soa; dns_name_t prefix; - REQUIRE(DNS_CATZ_ZONES_VALID(catzs)); REQUIRE(DNS_CATZ_ZONE_VALID(catz)); REQUIRE(ISC_MAGIC_VALID(src_name, DNS_NAME_MAGIC)); @@ -2049,12 +2048,12 @@ dns__catz_timer_cb(void *arg) { REQUIRE(DNS_CATZ_ZONE_VALID(catz)); - LOCK(&catz->catzs->lock); - - if (catz->catzs->shuttingdown) { - goto unlock; + if (atomic_load(&catz->catzs->shuttingdown)) { + return; } + LOCK(&catz->catzs->lock); + INSIST(DNS_DB_VALID(catz->db)); INSIST(catz->dbversion != NULL); @@ -2075,7 +2074,7 @@ dns__catz_timer_cb(void *arg) { result = isc_time_now(&catz->lastupdated); RUNTIME_CHECK(result == ISC_R_SUCCESS); -unlock: + UNLOCK(&catz->catzs->lock); } @@ -2178,8 +2177,6 @@ dns__catz_update_cb(void *data) { db = catz->db; catzs = catz->catzs; - LOCK(&catzs->lock); - if (atomic_load(&catzs->shuttingdown)) { result = ISC_R_SHUTTINGDOWN; goto exit; @@ -2191,7 +2188,9 @@ dns__catz_update_cb(void *data) { * Create a new catz in the same context as current catz. */ dns_name_toregion(&db->origin, &r); + LOCK(&catzs->lock); result = isc_ht_find(catzs->zones, r.base, r.length, (void **)&oldcatz); + UNLOCK(&catzs->lock); if (result != ISC_R_SUCCESS) { /* This can happen if we remove the zone in the meantime. */ isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, @@ -2272,6 +2271,11 @@ dns__catz_update_cb(void *data) { * Iterate over database to fill the new zone. */ while (result == ISC_R_SUCCESS) { + if (atomic_load(&catzs->shuttingdown)) { + result = ISC_R_SHUTTINGDOWN; + break; + } + result = dns_dbiterator_current(it, &node, name); if (result != ISC_R_SUCCESS) { isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, @@ -2317,7 +2321,7 @@ dns__catz_update_cb(void *data) { goto next; } - result = dns_catz_update_process(catzs, newcatz, name, + result = dns_catz_update_process(newcatz, name, &rdataset); if (result != ISC_R_SUCCESS) { char typebuf[DNS_RDATATYPE_FORMATSIZE]; @@ -2359,7 +2363,8 @@ dns__catz_update_cb(void *data) { dns_db_closeversion(db, &oldcatz->dbversion, false); isc_log_write(dns_lctx, DNS_LOGCATEGORY_GENERAL, DNS_LOGMODULE_MASTER, ISC_LOG_DEBUG(3), - "catz: update_from_db: iteration finished"); + "catz: update_from_db: iteration finished: %s", + isc_result_totext(result)); /* * Check catalog zone version compatibilites. @@ -2430,8 +2435,6 @@ final: exit: catz->updateresult = result; - - UNLOCK(&catzs->lock); } static void @@ -2446,7 +2449,7 @@ dns__catz_done_cb(void *data) { dns_name_format(&catz->name, dname, DNS_NAME_FORMATSIZE); - if (catz->updatepending && !catz->catzs->shuttingdown) { + if (catz->updatepending && !atomic_load(&catz->catzs->shuttingdown)) { /* Restart the timer */ dns__catz_timer_start(catz); } diff --git a/lib/dns/include/dns/catz.h b/lib/dns/include/dns/catz.h index 8696dd2c2c..9c9df5978d 100644 --- a/lib/dns/include/dns/catz.h +++ b/lib/dns/include/dns/catz.h @@ -267,15 +267,14 @@ dns_catz_zones_merge(dns_catz_zone_t *target, dns_catz_zone_t *newzone); */ isc_result_t -dns_catz_update_process(dns_catz_zones_t *catzs, dns_catz_zone_t *zone, - const dns_name_t *src_name, dns_rdataset_t *rdataset); +dns_catz_update_process(dns_catz_zone_t *catz, const dns_name_t *src_name, + dns_rdataset_t *rdataset); /*%< * Process a single rdataset from a catalog zone 'zone' update, src_name is the * record name. * * Requires: - * \li 'catzs' is a valid dns_catz_zones_t. - * \li 'zone' is a valid dns_catz_zone_t. + * \li 'catz' is a valid dns_catz_zone_t. * \li 'src_name' is a valid dns_name_t. * \li 'rdataset' is valid rdataset. */ From 4e7187601f88a15c61282db862f5025703a7a78a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 23 Feb 2023 11:10:39 +0100 Subject: [PATCH 3/4] Pause the catz dbiterator while processing the zone The dbiterator read-locks the whole zone and it stayed locked during whole processing time when catz is being read. Pause the iterator, so the updates to catz zone are not being blocked while processing the catz update. --- lib/dns/catz.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/dns/catz.c b/lib/dns/catz.c index cb11236336..b90fcd277c 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -2285,6 +2285,9 @@ dns__catz_update_cb(void *data) { break; } + result = dns_dbiterator_pause(it); + RUNTIME_CHECK(result == ISC_R_SUCCESS); + if (!is_vers_processed) { /* Keep the version node to skip it later in the loop */ vers_node = node; From cb1cd67bea4cd004de6510e4c65a958fdd1ebe8a Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Tue, 21 Feb 2023 14:39:27 +0000 Subject: [PATCH 4/4] Add CHANGES and release notes for [GL #3881] --- CHANGES | 3 +++ doc/notes/notes-current.rst | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/CHANGES b/CHANGES index 54a2784082..1c33528f4c 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +6114. [func] Run the catalog zone update process on the offload + threads. [GL #3881] + 6113. [func] Add shutdown signaling for catalog zones. [GL !7571] 6112. [func] Add reference count tracing for dns_catz_zone_t and diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index af1d4162d2..c6394d0d21 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -61,6 +61,12 @@ Feature Changes failure when receiving multiple UDP messages in a single system call. :gl:`#3840` +- Run catalog zone updates on the specialized "offload" threads to reduce the + amount of time they block query processing on the main networking + threads. This should increase the responsiveness of :iscman:`named` + when catalog zone updates are being applied after a catalog zone has been + successfully transferred. :gl:`#3881` + Bug Fixes ~~~~~~~~~