From 21faded51e8389d72cc1a729a5e40a5e41aa4540 Mon Sep 17 00:00:00 2001 From: Gopal Shankar Date: Thu, 17 May 2012 18:07:59 +0530 Subject: [PATCH] Bug#12636001 : deadlock from thd_security_context PROBLEM: Threads end-up in deadlock due to locks acquired as described below, con1: Run Query on a table. It is important that this SELECT must back-off while trying to open the t1 and enter into wait_for_condition(). The SELECT then is blocked trying to lock mysys_var->mutex which is held by con3. The very significant fact here is that mysys_var->current_mutex will still point to LOCK_open, even if LOCK_open is no longer held by con1 at this point. con2: Try dropping table used in con1 or query some table. It will hold LOCK_open and be blocked trying to lock kernel_mutex held by con4. con3: Try killing the query run by con1. It will hold THD::LOCK_thd_data belonging to con1 while trying to lock mysys_var->current_mutex belonging to con1. But current_mutex will point to LOCK_open which is held by con2. con4: Get innodb engine status It will hold kernel_mutex, trying to lock THD::LOCK_thd_data belonging to con1 which is held by con3. So while technically only con2, con3 and con4 participate in the deadlock, con1's mysys_var->current_mutex pointing to LOCK_open is a vital component of the deadlock. CYCLE = (THD::LOCK_thd_data -> LOCK_open -> kernel_mutex -> THD::LOCK_thd_data) FIX: LOCK_thd_data has responsibility of protecting, 1) thd->query, thd->query_length 2) VIO 3) thd->mysys_var (used by KILL statement and shutdown) 4) THD during thread delete. Among above responsibilities, 1), 2)and (3,4) seems to be three independent group of responsibility. If there is different LOCK owning responsibility of (3,4), the above mentioned deadlock cycle can be avoid. This fix introduces LOCK_thd_kill to handle responsibility (3,4), which eliminates the deadlock issue. Note: The problem is not found in 5.5. Introduction MDL subsystem caused metadata locking responsibility to be moved from TDC/TC to MDL subsystem. Due to this, responsibility of LOCK_open is reduced. As the use of LOCK_open is removed in open_table() and mysql_rm_table() the above mentioned CYCLE does not form. Revision ID for changes, open_table() = dlenev@mysql.com-20100727133458-m3ua9oslnx8fbbvz mysql_rm_table() = jon.hauglid@oracle.com-20101116100012-kxep9txz2fxy3nmw --- sql/event_scheduler.cc | 4 ++-- sql/slave.cc | 4 ++-- sql/sql_base.cc | 2 ++ sql/sql_class.cc | 13 +++++++++++-- sql/sql_class.h | 16 ++++++++++++++-- sql/sql_parse.cc | 5 +++-- sql/sql_repl.cc | 4 ++-- 7 files changed, 36 insertions(+), 12 deletions(-) diff --git a/sql/event_scheduler.cc b/sql/event_scheduler.cc index a24c70a8494..d217d8d76cd 100755 --- a/sql/event_scheduler.cc +++ b/sql/event_scheduler.cc @@ -632,13 +632,13 @@ Event_scheduler::stop() DBUG_PRINT("info", ("Scheduler thread has id %lu", scheduler_thd->thread_id)); /* Lock from delete */ - pthread_mutex_lock(&scheduler_thd->LOCK_thd_data); + pthread_mutex_lock(&scheduler_thd->LOCK_thd_kill); /* This will wake up the thread if it waits on Queue's conditional */ sql_print_information("Event Scheduler: Killing the scheduler thread, " "thread id %lu", scheduler_thd->thread_id); scheduler_thd->awake(THD::KILL_CONNECTION); - pthread_mutex_unlock(&scheduler_thd->LOCK_thd_data); + pthread_mutex_unlock(&scheduler_thd->LOCK_thd_kill); /* thd could be 0x0, when shutting down */ sql_print_information("Event Scheduler: " diff --git a/sql/slave.cc b/sql/slave.cc index 0910d196583..437762cc318 100644 --- a/sql/slave.cc +++ b/sql/slave.cc @@ -515,7 +515,7 @@ terminate_slave_thread(THD *thd, int error; DBUG_PRINT("loop", ("killing slave thread")); - pthread_mutex_lock(&thd->LOCK_thd_data); + pthread_mutex_lock(&thd->LOCK_thd_kill); #ifndef DONT_USE_THR_ALARM /* Error codes from pthread_kill are: @@ -526,7 +526,7 @@ terminate_slave_thread(THD *thd, DBUG_ASSERT(err != EINVAL); #endif thd->awake(THD::NOT_KILLED); - pthread_mutex_unlock(&thd->LOCK_thd_data); + pthread_mutex_unlock(&thd->LOCK_thd_kill); /* There is a small chance that slave thread might miss the first diff --git a/sql/sql_base.cc b/sql/sql_base.cc index ca24830db63..2ce5ec81917 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -2203,6 +2203,8 @@ void wait_for_condition(THD *thd, pthread_mutex_t *mutex, pthread_cond_t *cond) */ pthread_mutex_unlock(mutex); + DEBUG_SYNC(thd, "waiting_for_table_unlock"); + DBUG_EXECUTE_IF("sleep_after_waiting_for_table", my_sleep(1000000);); pthread_mutex_lock(&thd->mysys_var->mutex); thd->mysys_var->current_mutex= 0; thd->mysys_var->current_cond= 0; diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 2f3e8153cbd..0f6bd4c1954 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -365,6 +365,7 @@ extern "C" char *thd_security_context(THD *thd, char *buffer, unsigned int length, unsigned int max_query_len) { + DEBUG_SYNC(thd, "thd_security_context"); String str(buffer, length, &my_charset_latin1); const Security_context *sctx= &thd->main_security_ctx; char header[64]; @@ -695,6 +696,7 @@ THD::THD() active_vio = 0; #endif pthread_mutex_init(&LOCK_thd_data, MY_MUTEX_INIT_FAST); + pthread_mutex_init(&LOCK_thd_kill, MY_MUTEX_INIT_FAST); /* Variables with default values */ proc_info="login"; @@ -999,6 +1001,8 @@ THD::~THD() /* Ensure that no one is using THD */ pthread_mutex_lock(&LOCK_thd_data); pthread_mutex_unlock(&LOCK_thd_data); + pthread_mutex_lock(&LOCK_thd_kill); + pthread_mutex_unlock(&LOCK_thd_kill); add_to_status(&global_status_var, &status_var); /* Close connection */ @@ -1026,6 +1030,7 @@ THD::~THD() #endif mysys_var=0; // Safety (shouldn't be needed) pthread_mutex_destroy(&LOCK_thd_data); + pthread_mutex_destroy(&LOCK_thd_kill); #ifndef DBUG_OFF dbug_sentry= THD_SENTRY_GONE; #endif @@ -1104,9 +1109,11 @@ void add_diff_to_status(STATUS_VAR *to_var, STATUS_VAR *from_var, void THD::awake(THD::killed_state state_to_set) { DBUG_ENTER("THD::awake"); - DBUG_PRINT("enter", ("this: 0x%lx", (long) this)); + DBUG_PRINT("enter", ("this: 0x%lx thread_id=%lu killed_state=%d", + (long) this, thread_id, state_to_set)); THD_CHECK_SENTRY(this); - safe_mutex_assert_owner(&LOCK_thd_data); + safe_mutex_assert_not_owner(&LOCK_thd_data); + safe_mutex_assert_owner(&LOCK_thd_kill); killed= state_to_set; if (state_to_set != THD::KILL_QUERY) @@ -1127,7 +1134,9 @@ void THD::awake(THD::killed_state state_to_set) hack is not used. */ + pthread_mutex_lock(&LOCK_thd_data); close_active_vio(); + pthread_mutex_unlock(&LOCK_thd_data); } #endif } diff --git a/sql/sql_class.h b/sql/sql_class.h index 31d2c664aae..3ecf032db71 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -1342,11 +1342,23 @@ public: Protects THD data accessed from other threads: - thd->query and thd->query_length (used by SHOW ENGINE INNODB STATUS and SHOW PROCESSLIST - - thd->mysys_var (used by KILL statement and shutdown). - Is locked when THD is deleted. */ pthread_mutex_t LOCK_thd_data; + /** + - Protects thd->mysys_var (used during KILL statement and shutdown). + - Is Locked when THD is deleted. + + Note: This responsibility was earlier handled by LOCK_thd_data. + This lock is introduced to solve a deadlock issue waiting for + LOCK_thd_data. As this lock reduces responsibility of LOCK_thd_data + the deadlock issues is solved. + Caution: LOCK_thd_kill should not be taken while holding LOCK_thd_data. + THD::awake() currently takes LOCK_thd_data after holding + LOCK_thd_kill. + */ + pthread_mutex_t LOCK_thd_kill; + /* all prepared statements and cursors of this connection */ Statement_map stmt_map; /* diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 729a963eef1..6c376d2c0da 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -7187,7 +7187,7 @@ uint kill_one_thread(THD *thd, ulong id, bool only_kill_query) continue; if (tmp->thread_id == id) { - pthread_mutex_lock(&tmp->LOCK_thd_data); // Lock from delete + pthread_mutex_lock(&tmp->LOCK_thd_kill); // Lock from delete break; } } @@ -7215,12 +7215,13 @@ uint kill_one_thread(THD *thd, ulong id, bool only_kill_query) if ((thd->security_ctx->master_access & SUPER_ACL) || thd->security_ctx->user_matches(tmp->security_ctx)) { + DEBUG_SYNC(thd, "kill_one_thread_before_kill"); tmp->awake(only_kill_query ? THD::KILL_QUERY : THD::KILL_CONNECTION); error=0; } else error=ER_KILL_DENIED_ERROR; - pthread_mutex_unlock(&tmp->LOCK_thd_data); + pthread_mutex_unlock(&tmp->LOCK_thd_kill); } DBUG_PRINT("exit", ("%d", error)); DBUG_RETURN(error); diff --git a/sql/sql_repl.cc b/sql/sql_repl.cc index b985805a827..c92d2db2c1e 100644 --- a/sql/sql_repl.cc +++ b/sql/sql_repl.cc @@ -1133,7 +1133,7 @@ void kill_zombie_dump_threads(uint32 slave_server_id) if (tmp->command == COM_BINLOG_DUMP && tmp->server_id == slave_server_id) { - pthread_mutex_lock(&tmp->LOCK_thd_data); // Lock from delete + pthread_mutex_lock(&tmp->LOCK_thd_kill); // Lock from delete break; } } @@ -1146,7 +1146,7 @@ void kill_zombie_dump_threads(uint32 slave_server_id) again. We just to do kill the thread ourselves. */ tmp->awake(THD::KILL_QUERY); - pthread_mutex_unlock(&tmp->LOCK_thd_data); + pthread_mutex_unlock(&tmp->LOCK_thd_kill); } }