From 5111258e7a856a8a9f287061b4756a09724cdaf4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 3 Nov 2022 17:42:12 +0100 Subject: [PATCH 1/2] Propagate the shutdown event to the recursing ns_client(s) Send the ns_query_cancel() on the recursing clients when we initiate the named shutdown for faster shutdown. When we are shutting down the resolver, we cancel all the outstanding fetches, and the ISC_R_CANCEL events doesn't propagate to the ns_client callback. In the future, the better solution how to fix this would be to look at the shutdown paths and let them all propagate from bottom (loopmgr) to top (f.e. ns_client). --- lib/ns/client.c | 60 ++++++++++++-------------------------- lib/ns/include/ns/client.h | 8 +++-- lib/ns/interfacemgr.c | 6 +++- 3 files changed, 29 insertions(+), 45 deletions(-) diff --git a/lib/ns/client.c b/lib/ns/client.c index 95b2abed40..39ef9acd9f 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -120,10 +120,6 @@ atomic_uint_fast64_t ns_client_requests = 0; -static void -clientmgr_attach(ns_clientmgr_t *source, ns_clientmgr_t **targetp); -static void -clientmgr_detach(ns_clientmgr_t **mp); static void clientmgr_destroy_cb(void *arg); static void @@ -1680,7 +1676,7 @@ ns__client_put_cb(void *client0) { isc_mem_put(manager->mctx, client, sizeof(*client)); - clientmgr_detach(&manager); + ns_clientmgr_detach(&manager); } /* @@ -2319,7 +2315,7 @@ ns__client_setup(ns_client_t *client, ns_clientmgr_t *mgr, bool new) { *client = (ns_client_t){ .magic = 0 }; - clientmgr_attach(mgr, &client->manager); + ns_clientmgr_attach(mgr, &client->manager); dns_message_create(client->manager->mctx, DNS_MESSAGE_INTENTPARSE, &client->message); @@ -2381,7 +2377,7 @@ cleanup: } if (client->manager != NULL) { - clientmgr_detach(&client->manager); + ns_clientmgr_detach(&client->manager); } return (result); @@ -2391,21 +2387,6 @@ cleanup: *** Client Manager ***/ -static void -clientmgr_attach(ns_clientmgr_t *source, ns_clientmgr_t **targetp) { - int32_t oldrefs; - - REQUIRE(VALID_MANAGER(source)); - REQUIRE(targetp != NULL && *targetp == NULL); - - oldrefs = isc_refcount_increment0(&source->references); - isc_log_write(ns_lctx, NS_LOGCATEGORY_CLIENT, NS_LOGMODULE_CLIENT, - ISC_LOG_DEBUG(3), "clientmgr @%p attach: %d", source, - oldrefs + 1); - - *targetp = source; -} - static void clientmgr_destroy_cb(void *arg) { ns_clientmgr_t *manager = (ns_clientmgr_t *)arg; @@ -2425,23 +2406,13 @@ clientmgr_destroy_cb(void *arg) { } static void -clientmgr_detach(ns_clientmgr_t **mp) { - int32_t oldrefs; - ns_clientmgr_t *mgr = *mp; - *mp = NULL; - - oldrefs = isc_refcount_decrement(&mgr->references); - isc_log_write(ns_lctx, NS_LOGCATEGORY_CLIENT, NS_LOGMODULE_CLIENT, - ISC_LOG_DEBUG(3), "clientmgr @%p detach: %d", mgr, - oldrefs - 1); - if (oldrefs == 1) { - isc_loop_t *loop = isc_loop_get(mgr->loopmgr, mgr->tid); - - /* FIXME: Use isc_loopmgr_teardown() function instead? */ - isc_async_run(loop, clientmgr_destroy_cb, mgr); - } +clientmgr_destroy(ns_clientmgr_t *mgr) { + isc_loop_t *loop = isc_loop_get(mgr->loopmgr, mgr->tid); + isc_async_run(loop, clientmgr_destroy_cb, mgr); } +ISC_REFCOUNT_IMPL(ns_clientmgr, clientmgr_destroy); + isc_result_t ns_clientmgr_create(ns_server_t *sctx, isc_taskmgr_t *taskmgr, isc_loopmgr_t *loopmgr, dns_aclenv_t *aclenv, int tid, @@ -2485,13 +2456,20 @@ ns_clientmgr_create(ns_server_t *sctx, isc_taskmgr_t *taskmgr, } void -ns_clientmgr_destroy(ns_clientmgr_t **managerp) { - REQUIRE(managerp != NULL); - REQUIRE(VALID_MANAGER(*managerp)); +ns_clientmgr_shutdown(ns_clientmgr_t *manager) { + ns_client_t *client; + + REQUIRE(VALID_MANAGER(manager)); MTRACE("destroy"); - clientmgr_detach(managerp); + LOCK(&manager->reclock); + for (client = ISC_LIST_HEAD(manager->recursing); client != NULL; + client = ISC_LIST_NEXT(client, rlink)) + { + ns_query_cancel(client); + } + UNLOCK(&manager->reclock); } isc_sockaddr_t * diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h index 47b7268502..1f92486f61 100644 --- a/lib/ns/include/ns/client.h +++ b/lib/ns/include/ns/client.h @@ -329,10 +329,10 @@ ns_clientmgr_create(ns_server_t *sctx, isc_taskmgr_t *taskmgr, */ void -ns_clientmgr_destroy(ns_clientmgr_t **managerp); +ns_clientmgr_shutdown(ns_clientmgr_t *manager); /*%< - * Destroy a client manager and all ns_client_t objects - * managed by it. + * Shutdown a client manager and all ns_client_t objects + * managed by it */ isc_sockaddr_t * @@ -532,6 +532,8 @@ ns_client_findversion(ns_client_t *client, dns_db_t *db); * allocated by ns_client_newdbversion(). */ +ISC_REFCOUNT_DECL(ns_clientmgr); + isc_result_t ns__client_setup(ns_client_t *client, ns_clientmgr_t *manager, bool new); /*%< diff --git a/lib/ns/interfacemgr.c b/lib/ns/interfacemgr.c index 4f4253e488..dc86e2e975 100644 --- a/lib/ns/interfacemgr.c +++ b/lib/ns/interfacemgr.c @@ -382,7 +382,7 @@ ns_interfacemgr_destroy(ns_interfacemgr_t *mgr) { clearlistenon(mgr); isc_mutex_destroy(&mgr->lock); for (size_t i = 0; i < mgr->ncpus; i++) { - ns_clientmgr_destroy(&mgr->clientmgrs[i]); + ns_clientmgr_detach(&mgr->clientmgrs[i]); } isc_mem_put(mgr->mctx, mgr->clientmgrs, mgr->ncpus * sizeof(mgr->clientmgrs[0])); @@ -451,6 +451,10 @@ ns_interfacemgr_shutdown(ns_interfacemgr_t *mgr) { if (mgr->route != NULL) { isc_nm_cancelread(mgr->route); } + + for (size_t i = 0; i < mgr->ncpus; i++) { + ns_clientmgr_shutdown(mgr->clientmgrs[i]); + } } static void From 48059a1bc2da97968cdb4cea680e4a556a173c29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 3 Nov 2022 18:01:22 +0100 Subject: [PATCH 2/2] Add CHANGES and release notes for [GL #3183] --- CHANGES | 4 ++++ doc/notes/notes-current.rst | 3 +++ 2 files changed, 7 insertions(+) diff --git a/CHANGES b/CHANGES index b3ea4ca14e..4b76081077 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +6040. [bug] Speed up the named shutdown time by explicitly + canceling all recursing ns_client objects for + each ns_clientmgr. [GL #3183] + 6039. [bug] Removing a catalog zone from catalog-zones without also removing the referenced zone could leave a dangling pointer. [GL #3683] diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 49e2931b8d..e122ba5ff1 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -75,6 +75,9 @@ Bug Fixes cases a dangling pointer could cause a :iscman:`named` process crash. This has been fixed. :gl:`#3683` +- The ``named`` would wait for some outstanding recursing queries + to finish before shutting down. This has been fixed. :gl:`#3183` + Known Issues ~~~~~~~~~~~~