Skip to content

Commit

Permalink
perfschema: LOCK_all_status_vars not LOCK_status
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@vuvova
vuvova committed Mar 13, 2024
1 parent f71d7f2 commit 61f6dc5
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 41 deletions.
64 changes: 25 additions & 39 deletions storage/perfschema/pfs_variable.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -766,7 +766,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)
{
Expand Down Expand Up @@ -885,14 +885,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;
}
Expand Down Expand Up @@ -921,13 +919,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)
Expand All @@ -950,8 +947,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");
Expand All @@ -971,13 +967,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)
Expand All @@ -1001,8 +995,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;
}

Expand All @@ -1018,13 +1011,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)
Expand All @@ -1048,8 +1039,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;
}

Expand All @@ -1066,9 +1056,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);
Expand All @@ -1091,8 +1080,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;
}

Expand All @@ -1111,9 +1099,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);
Expand All @@ -1130,8 +1117,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;
Expand Down Expand Up @@ -1224,7 +1210,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)
{
Expand Down Expand Up @@ -1290,7 +1276,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()
{
Expand Down
3 changes: 1 addition & 2 deletions storage/perfschema/pfs_visitor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down

0 comments on commit 61f6dc5

Please sign in to comment.