From 07a0173f1c478a8fe237702c7fc161dda82106c6 Mon Sep 17 00:00:00 2001 From: Sergei Golubchik Date: Mon, 18 Dec 2023 17:37:30 +0100 Subject: [PATCH] perfschema: LOCK_all_status_vars not LOCK_status to iterate over all status variables one should use LOCK_all_status_vars not LOCK_status this fixes sporadic mutex lock inversion in plugins.password_reuse_check: * acl_cache->lock is taken over complex operations that might increment status counters (under LOCK_status). * acl_cache->lock is needed to get the values of Acl% status variables when iterating over status variables --- storage/perfschema/pfs_variable.cc | 64 ++++++++++++------------------ storage/perfschema/pfs_visitor.cc | 3 +- 2 files changed, 26 insertions(+), 41 deletions(-) diff --git a/storage/perfschema/pfs_variable.cc b/storage/perfschema/pfs_variable.cc index aa07686139b..3f7fd73683a 100644 --- a/storage/perfschema/pfs_variable.cc +++ b/storage/perfschema/pfs_variable.cc @@ -762,7 +762,7 @@ bool PFS_status_variable_cache::filter_show_var(const SHOW_VAR *show_var, bool s /** Build an array of SHOW_VARs from the global status array. Expand nested subarrays, filter unwanted variables. - NOTE: Must be done inside of LOCK_status to guard against plugin load/unload. + NOTE: Must be done under LOCK_all_status_vars */ bool PFS_status_variable_cache::init_show_var_array(enum_var_type scope, bool strict) { @@ -881,14 +881,12 @@ char * PFS_status_variable_cache::make_show_var_name(const char* prefix, const c */ bool PFS_status_variable_cache::do_initialize_session(void) { - /* Acquire LOCK_status to guard against plugin load/unload. */ - //if (m_current_thd->fill_status_recursion_level++ == 0) - mysql_mutex_lock(&LOCK_status); + /* Acquire LOCK_all_status_vars to guard against plugin load/unload. */ + mysql_rwlock_rdlock(&LOCK_all_status_vars); bool ret= init_show_var_array(OPT_SESSION, true); - //if (m_current_thd->fill_status_recursion_level-- == 1) - mysql_mutex_unlock(&LOCK_status); + mysql_rwlock_unlock(&LOCK_all_status_vars); return ret; } @@ -917,13 +915,12 @@ int PFS_status_variable_cache::do_materialize_global(void) m_materialized= false; DEBUG_SYNC(m_current_thd, "before_materialize_global_status_array"); - /* Acquire LOCK_status to guard against plugin load/unload. */ - //if (m_current_thd->fill_status_recursion_level++ == 0) - mysql_mutex_lock(&LOCK_status); + /* Acquire LOCK_all_status_vars to guard against plugin load/unload. */ + mysql_rwlock_rdlock(&LOCK_all_status_vars); /* - Build array of SHOW_VARs from global status array. Do this within - LOCK_status to ensure that the array remains unchanged during + Build array of SHOW_VARs from global status array. Do this under + LOCK_all_status_vars to ensure that the array remains unchanged during materialization. */ if (!m_external_init) @@ -946,8 +943,7 @@ int PFS_status_variable_cache::do_materialize_global(void) */ manifest(m_current_thd, m_show_var_array.front(), &status_totals, "", false, true); - //if (m_current_thd->fill_status_recursion_level-- == 1) - mysql_mutex_unlock(&LOCK_status); + mysql_rwlock_unlock(&LOCK_all_status_vars); m_materialized= true; DEBUG_SYNC(m_current_thd, "after_materialize_global_status_array"); @@ -967,13 +963,11 @@ int PFS_status_variable_cache::do_materialize_all(THD* unsafe_thd) m_materialized= false; m_cache.clear(); - /* Avoid recursive acquisition of LOCK_status. */ - //if (m_current_thd->fill_status_recursion_level++ == 0) - mysql_mutex_lock(&LOCK_status); + mysql_rwlock_rdlock(&LOCK_all_status_vars); /* - Build array of SHOW_VARs from global status array. Do this within - LOCK_status to ensure that the array remains unchanged while this + Build array of SHOW_VARs from global status array. Do this under + LOCK_all_status_vars to ensure that the array remains unchanged while this thread is materialized. */ if (!m_external_init) @@ -996,8 +990,7 @@ int PFS_status_variable_cache::do_materialize_all(THD* unsafe_thd) ret= 0; } - //if (m_current_thd->fill_status_recursion_level-- == 1) - mysql_mutex_unlock(&LOCK_status); + mysql_rwlock_unlock(&LOCK_all_status_vars); return ret; } @@ -1013,13 +1006,11 @@ int PFS_status_variable_cache::do_materialize_session(THD* unsafe_thd) m_materialized= false; m_cache.clear(); - /* Avoid recursive acquisition of LOCK_status. */ - //if (m_current_thd->fill_status_recursion_level++ == 0) - mysql_mutex_lock(&LOCK_status); + mysql_rwlock_rdlock(&LOCK_all_status_vars); /* - Build array of SHOW_VARs from global status array. Do this within - LOCK_status to ensure that the array remains unchanged while this + Build array of SHOW_VARs from global status array. Do this under + LOCK_all_status_vars to ensure that the array remains unchanged while this thread is materialized. */ if (!m_external_init) @@ -1042,8 +1033,7 @@ int PFS_status_variable_cache::do_materialize_session(THD* unsafe_thd) ret= 0; } - //if (m_current_thd->fill_status_recursion_level-- == 1) - mysql_mutex_unlock(&LOCK_status); + mysql_rwlock_unlock(&LOCK_all_status_vars); return ret; } @@ -1060,9 +1050,8 @@ int PFS_status_variable_cache::do_materialize_session(PFS_thread *pfs_thread) m_materialized= false; m_cache.clear(); - /* Acquire LOCK_status to guard against plugin load/unload. */ - //if (m_current_thd->fill_status_recursion_level++ == 0) - mysql_mutex_lock(&LOCK_status); + /* Acquire LOCK_all_status_vars to guard against plugin load/unload. */ + mysql_rwlock_rdlock(&LOCK_all_status_vars); /* The SHOW_VAR array must be initialized externally. */ assert(m_initialized); @@ -1084,8 +1073,7 @@ int PFS_status_variable_cache::do_materialize_session(PFS_thread *pfs_thread) ret= 0; } - //if (m_current_thd->fill_status_recursion_level-- == 1) - mysql_mutex_unlock(&LOCK_status); + mysql_rwlock_unlock(&LOCK_all_status_vars); return ret; } @@ -1104,9 +1092,8 @@ int PFS_status_variable_cache::do_materialize_client(PFS_client *pfs_client) m_materialized= false; m_cache.clear(); - /* Acquire LOCK_status to guard against plugin load/unload. */ - //if (m_current_thd->fill_status_recursion_level++ == 0) - mysql_mutex_lock(&LOCK_status); + /* Acquire LOCK_all_status_vars to guard against plugin load/unload. */ + mysql_rwlock_rdlock(&LOCK_all_status_vars); /* The SHOW_VAR array must be initialized externally. */ assert(m_initialized); @@ -1123,8 +1110,7 @@ int PFS_status_variable_cache::do_materialize_client(PFS_client *pfs_client) */ manifest(m_current_thd, m_show_var_array.front(), &status_totals, "", false, true); - //if (m_current_thd->fill_status_recursion_level-- == 1) - mysql_mutex_unlock(&LOCK_status); + mysql_rwlock_unlock(&LOCK_all_status_vars); m_materialized= true; return 0; @@ -1217,7 +1203,7 @@ Status_variable::Status_variable(const SHOW_VAR *show_var, STATUS_VAR *status_va /** Resolve status value, convert to string. show_var->value is an offset into status_vars. - NOTE: Assumes LOCK_status is held. + NOTE: Assumes LOCK_all_status_vars is held. */ void Status_variable::init(const SHOW_VAR *show_var, STATUS_VAR *status_vars, enum_var_type query_scope) { @@ -1283,7 +1269,7 @@ void sum_account_status(PFS_client *pfs_account, STATUS_VAR *status_totals) /** Reset aggregated status counter stats for account, user and host. - NOTE: Assumes LOCK_status is held. + NOTE: Assumes LOCK_all_status_vars is held. */ void reset_pfs_status_stats() { diff --git a/storage/perfschema/pfs_visitor.cc b/storage/perfschema/pfs_visitor.cc index 92a5c99e13b..7e3027ac9a6 100644 --- a/storage/perfschema/pfs_visitor.cc +++ b/storage/perfschema/pfs_visitor.cc @@ -1356,8 +1356,7 @@ PFS_connection_status_visitor::~PFS_connection_status_visitor() = default; /** Aggregate from global status. */ void PFS_connection_status_visitor::visit_global() { - /* NOTE: Requires lock on LOCK_status. */ - mysql_mutex_assert_owner(&LOCK_status); + /* NOTE: Requires lock on LOCK_all_status_vars. */ add_to_status(m_status_vars, &global_status_var); }