[Commits] 4437f51: MDEV-8869: Potential lock_sys->mutex deadlock

Jan Lindström jan.lindstrom at mariadb.com
Mon Dec 14 10:11:04 EET 2015


revision-id: 4437f51682b22caaf2cf0b00de1bf6fa3edf6557 (mariadb-galera-10.0.22-5-g4437f51)
parent(s): b88c67d5f23c6beca5604b31919ab45906dd9b39
committer: Jan Lindström
timestamp: 2015-12-14 10:10:09 +0200
message:

MDEV-8869: Potential lock_sys->mutex deadlock

In wsrep brute force (BF) we have already took lock_sys and trx
mutex either on wsrep_abort_transaction() or
before wsrep_kill_victim().

---
 storage/innobase/handler/ha_innodb.cc | 30 ++++++++++++++----------------
 storage/innobase/include/lock0lock.h  | 10 ++++++++++
 storage/innobase/include/trx0trx.h    |  7 ++-----
 storage/innobase/lock/lock0lock.cc    | 26 ++++++++++++++++----------
 storage/xtradb/handler/ha_innodb.cc   | 27 +++++++++++----------------
 storage/xtradb/include/lock0lock.h    | 10 ++++++++++
 storage/xtradb/include/trx0trx.h      |  7 ++-----
 storage/xtradb/lock/lock0lock.cc      | 26 ++++++++++++++++----------
 8 files changed, 81 insertions(+), 62 deletions(-)

diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc
index 7afc90d..8bc00e4 100644
--- a/storage/innobase/handler/ha_innodb.cc
+++ b/storage/innobase/handler/ha_innodb.cc
@@ -4334,7 +4334,6 @@ innobase_kill_query(
         enum thd_kill_levels level) /*!< in: kill level */
 {
 	trx_t*	trx;
-	bool	took_lock_sys = false;
 
 	DBUG_ENTER("innobase_kill_query");
 	DBUG_ASSERT(hton == innodb_hton_ptr);
@@ -4356,29 +4355,28 @@ innobase_kill_query(
 	trx = thd_to_trx(thd);
 
 	if (trx) {
-		THD *cur = current_thd;
-		THD *owner = trx->current_lock_mutex_owner;
+		/* In wsrep BF we have already took lock_sys and trx
+		mutex either on wsrep_abort_transaction() or
+		before wsrep_kill_victim() */
 
-		/* Cancel a pending lock request. */
-		if (owner != cur) {
+		if (!wsrep_thd_is_BF(trx->mysql_thd, FALSE)) {
 			ut_ad(!lock_mutex_own());
+			ut_ad(!trx_mutex_own(trx));
 			lock_mutex_enter();
-			took_lock_sys = true;
+			trx_mutex_enter(trx);
 		}
 
-		ut_ad(!trx_mutex_own(trx));
-		trx_mutex_enter(trx);
+		ut_ad(lock_mutex_own());
+		ut_ad(trx_mutex_own(trx));
 
+		/* We did dirty read above in case when abort is not
+		from wsrep. */
 		if (trx->lock.wait_lock) {
 			lock_cancel_waiting_and_release(trx->lock.wait_lock);
 		}
 
-	        ut_ad(lock_mutex_own());
-		ut_ad(trx_mutex_own(trx));
-
-		trx_mutex_exit(trx);
-
-		if (took_lock_sys) {
+		if (!wsrep_thd_is_BF(trx->mysql_thd, FALSE)) {
+			trx_mutex_exit(trx);
 			lock_mutex_exit();
 		}
 	}
@@ -17560,13 +17558,13 @@ wsrep_abort_transaction(
 
 	if (victim_trx) {
 		lock_mutex_enter();
-		victim_trx->current_lock_mutex_owner = victim_thd;
 		trx_mutex_enter(victim_trx);
+		victim_trx->wsrep_abort = TRUE;
 		int rcode = wsrep_innobase_kill_one_trx(bf_thd, bf_trx,
                                                         victim_trx, signal);
 		trx_mutex_exit(victim_trx);
-		victim_trx->current_lock_mutex_owner = NULL;
 		lock_mutex_exit();
+		victim_trx->wsrep_abort = FALSE;
 		wsrep_srv_conc_cancel_wait(victim_trx);
 		DBUG_RETURN(rcode);
 	} else {
diff --git a/storage/innobase/include/lock0lock.h b/storage/innobase/include/lock0lock.h
index 88246af..eb7ab5d 100644
--- a/storage/innobase/include/lock0lock.h
+++ b/storage/innobase/include/lock0lock.h
@@ -654,6 +654,16 @@ lock_get_type(
 	const lock_t*	lock);	/*!< in: lock */
 
 /*******************************************************************//**
+Gets the trx of the lock. Non-inline version for using outside of the
+lock module.
+ at return	trx_t* */
+UNIV_INTERN
+trx_t*
+lock_get_trx(
+/*=========*/
+	const lock_t*	lock);	/*!< in: lock */
+
+/*******************************************************************//**
 Gets the id of the transaction owning a lock.
 @return	transaction id */
 UNIV_INTERN
diff --git a/storage/innobase/include/trx0trx.h b/storage/innobase/include/trx0trx.h
index dd26e42..6a58dcd 100644
--- a/storage/innobase/include/trx0trx.h
+++ b/storage/innobase/include/trx0trx.h
@@ -848,6 +848,8 @@ struct trx_t{
 	/*------------------------------*/
 	THD*		mysql_thd;	/*!< MySQL thread handle corresponding
 					to this trx, or NULL */
+	ibool		wsrep_abort;	/*!< Transaction is aborted from
+					wsrep functions. */
 	const char*	mysql_log_file_name;
 					/*!< if MySQL binlog is used, this field
 					contains a pointer to the latest file
@@ -992,11 +994,6 @@ struct trx_t{
 					count of tables being flushed. */
 
 	/*------------------------------*/
-	THD*		current_lock_mutex_owner;
-					/*!< If this is equal to current_thd,
-					then in innobase_kill_query() we know we
-					already hold the lock_sys->mutex. */
-	/*------------------------------*/
 #ifdef UNIV_DEBUG
 	ulint		start_line;	/*!< Track where it was started from */
 	const char*	start_file;	/*!< Filename where it was started */
diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc
index f72f7f0..b072aea 100644
--- a/storage/innobase/lock/lock0lock.cc
+++ b/storage/innobase/lock/lock0lock.cc
@@ -1729,8 +1729,10 @@ wsrep_kill_victim(
 				}
 			}
 
+			lock->trx->wsrep_abort = TRUE;
 			wsrep_innobase_kill_one_trx(trx->mysql_thd,
 				(const trx_t*) trx, lock->trx, TRUE);
+			lock->trx->wsrep_abort = FALSE;
 		}
 	}
 }
@@ -4399,9 +4401,7 @@ lock_report_waiters_to_mysql(
 				innobase_kill_query. We mark this by setting
 				current_lock_mutex_owner, so we can avoid trying
 				to recursively take lock_sys->mutex. */
-				w_trx->current_lock_mutex_owner = mysql_thd;
 				thd_report_wait_for(mysql_thd, w_trx->mysql_thd);
-				w_trx->current_lock_mutex_owner = NULL;
 			}
 			++i;
 		}
@@ -6769,7 +6769,7 @@ lock_clust_rec_modify_check_and_lock(
 
 	lock_mutex_enter();
 	trx_t*		trx = thr_get_trx(thr);
-	trx->current_lock_mutex_owner = trx->mysql_thd;
+
 	ut_ad(lock_table_has(trx, index->table, LOCK_IX));
 
 	err = lock_rec_lock(TRUE, LOCK_X | LOCK_REC_NOT_GAP,
@@ -6777,7 +6777,6 @@ lock_clust_rec_modify_check_and_lock(
 
 	MONITOR_INC(MONITOR_NUM_RECLOCK_REQ);
 
-	trx->current_lock_mutex_owner = NULL;
 	lock_mutex_exit();
 
 	ut_ad(lock_rec_queue_validate(FALSE, block, rec, index, offsets));
@@ -6831,7 +6830,6 @@ lock_sec_rec_modify_check_and_lock(
 
 	trx_t* trx = thr_get_trx(thr);
 	lock_mutex_enter();
-	trx->current_lock_mutex_owner = trx->mysql_thd;
 
 	ut_ad(lock_table_has(trx, index->table, LOCK_IX));
 
@@ -6840,7 +6838,6 @@ lock_sec_rec_modify_check_and_lock(
 
 	MONITOR_INC(MONITOR_NUM_RECLOCK_REQ);
 
-	trx->current_lock_mutex_owner = NULL;
 	lock_mutex_exit();
 
 #ifdef UNIV_DEBUG
@@ -6933,7 +6930,6 @@ lock_sec_rec_read_check_and_lock(
 
 	trx_t* trx = thr_get_trx(thr);
 	lock_mutex_enter();
-	trx->current_lock_mutex_owner = trx->mysql_thd;
 
 	ut_ad(mode != LOCK_X
 	      || lock_table_has(trx, index->table, LOCK_IX));
@@ -6945,7 +6941,6 @@ lock_sec_rec_read_check_and_lock(
 
 	MONITOR_INC(MONITOR_NUM_RECLOCK_REQ);
 
-	trx->current_lock_mutex_owner = NULL;
 	lock_mutex_exit();
 
 	ut_ad(lock_rec_queue_validate(FALSE, block, rec, index, offsets));
@@ -7008,7 +7003,6 @@ lock_clust_rec_read_check_and_lock(
 
 	lock_mutex_enter();
 	trx_t* trx = thr_get_trx(thr);
-	trx->current_lock_mutex_owner = trx->mysql_thd;
 
 	ut_ad(mode != LOCK_X
 	      || lock_table_has(trx, index->table, LOCK_IX));
@@ -7020,7 +7014,6 @@ lock_clust_rec_read_check_and_lock(
 
 	MONITOR_INC(MONITOR_NUM_RECLOCK_REQ);
 
-	trx->current_lock_mutex_owner = NULL;
 	lock_mutex_exit();
 
 	ut_ad(lock_rec_queue_validate(FALSE, block, rec, index, offsets));
@@ -7167,6 +7160,19 @@ lock_get_type(
 }
 
 /*******************************************************************//**
+Gets the trx of the lock. Non-inline version for using outside of the
+lock module.
+ at return	trx_t* */
+UNIV_INTERN
+trx_t*
+lock_get_trx(
+/*=========*/
+	const lock_t*	lock)	/*!< in: lock */
+{
+	return (lock->trx);
+}
+
+/*******************************************************************//**
 Gets the id of the transaction owning a lock.
 @return	transaction id */
 UNIV_INTERN
diff --git a/storage/xtradb/handler/ha_innodb.cc b/storage/xtradb/handler/ha_innodb.cc
index 886a3ef..d4d9e1c 100644
--- a/storage/xtradb/handler/ha_innodb.cc
+++ b/storage/xtradb/handler/ha_innodb.cc
@@ -4982,7 +4982,6 @@ innobase_kill_connection(
         thd_kill_levels)
 {
 	trx_t*	trx;
-	bool	took_lock_sys = false;
 
 	DBUG_ENTER("innobase_kill_connection");
 	DBUG_ASSERT(hton == innodb_hton_ptr);
@@ -5005,28 +5004,24 @@ innobase_kill_connection(
 	trx = thd_to_trx(thd);
 
 	if (trx) {
-		THD *cur = current_thd;
-		THD *owner = trx->current_lock_mutex_owner;
+		/* In wsrep BF we have already took lock_sys and trx
+		mutex either on wsrep_abort_transaction() or
+		before wsrep_kill_victim() */
 
-		if (owner != cur) {
+		if (!wsrep_thd_is_BF(trx->mysql_thd, FALSE)) {
 			ut_ad(!lock_mutex_own());
+			ut_ad(!trx_mutex_own(trx));
 			lock_mutex_enter();
-			took_lock_sys = true;
+			trx_mutex_enter(trx);
 		}
 
-		ut_ad(!trx_mutex_own(trx));
-		trx_mutex_enter(trx);
-
-		/* Cancel a pending lock request. */
-		if (trx->lock.wait_lock)
-			lock_cancel_waiting_and_release(trx->lock.wait_lock);
-
 		ut_ad(lock_mutex_own());
 		ut_ad(trx_mutex_own(trx));
 
-		trx_mutex_exit(trx);
+		lock_cancel_waiting_and_release(trx->lock.wait_lock);
 
-		if (took_lock_sys) {
+		if (!wsrep_thd_is_BF(trx->mysql_thd, FALSE)) {
+			trx_mutex_exit(trx);
 			lock_mutex_exit();
 		}
 	}
@@ -18642,13 +18637,13 @@ wsrep_abort_transaction(handlerton* hton, THD *bf_thd, THD *victim_thd,
 
 	if (victim_trx) {
 		lock_mutex_enter();
-		victim_trx->current_lock_mutex_owner = victim_thd;
 		trx_mutex_enter(victim_trx);
+		victim_trx->wsrep_abort = TRUE;
 		int rcode = wsrep_innobase_kill_one_trx(bf_thd, bf_trx,
                                                         victim_trx, signal);
 		trx_mutex_exit(victim_trx);
-		victim_trx->current_lock_mutex_owner = NULL;
 		lock_mutex_exit();
+		victim_trx->wsrep_abort = FALSE;
 		wsrep_srv_conc_cancel_wait(victim_trx);
 		DBUG_RETURN(rcode);
 	} else {
diff --git a/storage/xtradb/include/lock0lock.h b/storage/xtradb/include/lock0lock.h
index cb95c58..1c07c05 100644
--- a/storage/xtradb/include/lock0lock.h
+++ b/storage/xtradb/include/lock0lock.h
@@ -666,6 +666,16 @@ lock_get_type(
 	const lock_t*	lock);	/*!< in: lock */
 
 /*******************************************************************//**
+Gets the trx of the lock. Non-inline version for using outside of the
+lock module.
+ at return	trx_t* */
+UNIV_INTERN
+trx_t*
+lock_get_trx(
+/*=========*/
+	const lock_t*	lock);	/*!< in: lock */
+
+/*******************************************************************//**
 Gets the id of the transaction owning a lock.
 @return	transaction id */
 UNIV_INTERN
diff --git a/storage/xtradb/include/trx0trx.h b/storage/xtradb/include/trx0trx.h
index ad47eb0..76af1a9 100644
--- a/storage/xtradb/include/trx0trx.h
+++ b/storage/xtradb/include/trx0trx.h
@@ -881,6 +881,8 @@ struct trx_t{
 	/*------------------------------*/
 	THD*		mysql_thd;	/*!< MySQL thread handle corresponding
 					to this trx, or NULL */
+	ibool		wsrep_abort;	/*!< Transaction is aborted from
+					wsrep functions. */
 	const char*	mysql_log_file_name;
 					/*!< if MySQL binlog is used, this field
 					contains a pointer to the latest file
@@ -1031,11 +1033,6 @@ struct trx_t{
 					count of tables being flushed. */
 
 	/*------------------------------*/
-	THD*		current_lock_mutex_owner;
-					/*!< If this is equal to current_thd,
-					then in innobase_kill_query() we know we
-					already hold the lock_sys->mutex. */
-	/*------------------------------*/
 #ifdef UNIV_DEBUG
 	ulint		start_line;	/*!< Track where it was started from */
 	const char*	start_file;	/*!< Filename where it was started */
diff --git a/storage/xtradb/lock/lock0lock.cc b/storage/xtradb/lock/lock0lock.cc
index 2291ba0..a41f60c 100644
--- a/storage/xtradb/lock/lock0lock.cc
+++ b/storage/xtradb/lock/lock0lock.cc
@@ -1728,8 +1728,10 @@ wsrep_kill_victim(
 				}
 			}
 
+			lock->trx->wsrep_abort = TRUE;
 			wsrep_innobase_kill_one_trx(trx->mysql_thd,
 				(const trx_t*) trx, lock->trx, TRUE);
+			lock->trx->wsrep_abort = FALSE;
 		}
 	}
 }
@@ -4424,9 +4426,7 @@ lock_report_waiters_to_mysql(
 				innobase_kill_query. We mark this by setting
 				current_lock_mutex_owner, so we can avoid trying
 				to recursively take lock_sys->mutex. */
-				w_trx->current_lock_mutex_owner = mysql_thd;
 				thd_report_wait_for(mysql_thd, w_trx->mysql_thd);
-				w_trx->current_lock_mutex_owner = NULL;
 			}
 			++i;
 		}
@@ -6836,7 +6836,7 @@ lock_clust_rec_modify_check_and_lock(
 
 	lock_mutex_enter();
 	trx_t*		trx = thr_get_trx(thr);
-	trx->current_lock_mutex_owner = trx->mysql_thd;
+
 	ut_ad(lock_table_has(trx, index->table, LOCK_IX));
 
 	err = lock_rec_lock(TRUE, LOCK_X | LOCK_REC_NOT_GAP,
@@ -6844,7 +6844,6 @@ lock_clust_rec_modify_check_and_lock(
 
 	MONITOR_INC(MONITOR_NUM_RECLOCK_REQ);
 
-	trx->current_lock_mutex_owner = NULL;
 	lock_mutex_exit();
 
 	ut_ad(lock_rec_queue_validate(FALSE, block, rec, index, offsets));
@@ -6902,7 +6901,6 @@ lock_sec_rec_modify_check_and_lock(
 
 	trx_t* trx = thr_get_trx(thr);
 	lock_mutex_enter();
-	trx->current_lock_mutex_owner = trx->mysql_thd;
 
 	ut_ad(lock_table_has(trx, index->table, LOCK_IX));
 
@@ -6911,7 +6909,6 @@ lock_sec_rec_modify_check_and_lock(
 
 	MONITOR_INC(MONITOR_NUM_RECLOCK_REQ);
 
-	trx->current_lock_mutex_owner = NULL;
 	lock_mutex_exit();
 
 #ifdef UNIV_DEBUG
@@ -7013,7 +7010,6 @@ lock_sec_rec_read_check_and_lock(
 
 	trx_t* trx = thr_get_trx(thr);
 	lock_mutex_enter();
-	trx->current_lock_mutex_owner = trx->mysql_thd;
 
 	ut_ad(mode != LOCK_X
 	      || lock_table_has(trx, index->table, LOCK_IX));
@@ -7025,7 +7021,6 @@ lock_sec_rec_read_check_and_lock(
 
 	MONITOR_INC(MONITOR_NUM_RECLOCK_REQ);
 
-	trx->current_lock_mutex_owner = NULL;
 	lock_mutex_exit();
 
 	ut_ad(lock_rec_queue_validate(FALSE, block, rec, index, offsets));
@@ -7097,7 +7092,6 @@ lock_clust_rec_read_check_and_lock(
 
 	lock_mutex_enter();
 	trx_t* trx = thr_get_trx(thr);
-	trx->current_lock_mutex_owner = trx->mysql_thd;
 
 	ut_ad(mode != LOCK_X
 	      || lock_table_has(trx, index->table, LOCK_IX));
@@ -7109,7 +7103,6 @@ lock_clust_rec_read_check_and_lock(
 
 	MONITOR_INC(MONITOR_NUM_RECLOCK_REQ);
 
-	trx->current_lock_mutex_owner = NULL;
 	lock_mutex_exit();
 
 	ut_ad(lock_rec_queue_validate(FALSE, block, rec, index, offsets));
@@ -7256,6 +7249,19 @@ lock_get_type(
 }
 
 /*******************************************************************//**
+Gets the trx of the lock. Non-inline version for using outside of the
+lock module.
+ at return	trx_t* */
+UNIV_INTERN
+trx_t*
+lock_get_trx(
+/*=========*/
+	const lock_t*	lock)	/*!< in: lock */
+{
+	return (lock->trx);
+}
+
+/*******************************************************************//**
 Gets the id of the transaction owning a lock.
 @return	transaction id */
 UNIV_INTERN


More information about the commits mailing list