MDEV-8869: Potential lock_sys->mutex deadlock

In wsrep BF we have already took lock_sys and trx
mutex either on wsrep_abort_transaction() or
before wsrep_kill_victim(). In replication we
could own lock_sys mutex taken in
lock_deadlock_check_and_resolve().

storage/innobase/handler/ha_innodb.cc

@@ -4358,29 +4358,46 @@ innobase_kill_query(
 #endif /* WITH_WSREP */
 	trx = thd_to_trx(thd);
 
-	if (trx) {
+	if (trx && trx->lock.wait_lock) {
 		/* In wsrep BF we have already took lock_sys and trx
 		mutex either on wsrep_abort_transaction() or
-		before wsrep_kill_victim() */
+		before wsrep_kill_victim(). In replication we
+		could own lock_sys mutex taken in
+		lock_deadlock_check_and_resolve(). */
 
-		if (!wsrep_thd_is_BF(trx->mysql_thd, FALSE)) {
+		WSREP_DEBUG("Killing victim trx %p BF %d trx BF %d trx_id " TRX_ID_FMT " ABORT %d thd %p"
+			" current_thd %p BF %d wait_lock_modes: %s\n",
+			trx, wsrep_thd_is_BF(trx->mysql_thd, FALSE),
+			wsrep_thd_is_BF(thd, FALSE),
+			trx->id, trx->abort_type,
+			trx->mysql_thd,
+			current_thd,
+			wsrep_thd_is_BF(current_thd, FALSE),
+			lock_get_info(trx->lock.wait_lock).c_str());
+
+		if (!wsrep_thd_is_BF(trx->mysql_thd, FALSE) &&
+		    trx->abort_type == TRX_SERVER_ABORT) {
 			ut_ad(!lock_mutex_own());
-			ut_ad(!trx_mutex_own(trx));
 			lock_mutex_enter();
+		}
+
+		if (trx->abort_type != TRX_WSREP_ABORT) {
 			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);
 		}
 
-		if (!wsrep_thd_is_BF(trx->mysql_thd, FALSE)) {
+		if (trx->abort_type != TRX_WSREP_ABORT) {
 			trx_mutex_exit(trx);
+		}
+
+		if (!wsrep_thd_is_BF(trx->mysql_thd, FALSE) &&
+		    trx->abort_type == TRX_SERVER_ABORT) {
 			lock_mutex_exit();
 		}
 	}
@@ -17537,12 +17554,12 @@ wsrep_abort_transaction(
 	if (victim_trx) {
 		lock_mutex_enter();
 		trx_mutex_enter(victim_trx);
-		victim_trx->wsrep_abort = TRUE;
+		victim_trx->abort_type = TRX_WSREP_ABORT;
 		int rcode = wsrep_innobase_kill_one_trx(bf_thd, bf_trx,
                                                         victim_trx, signal);
 		trx_mutex_exit(victim_trx);
 		lock_mutex_exit();
-		victim_trx->wsrep_abort = FALSE;
+		victim_trx->abort_type = TRX_SERVER_ABORT;
 		wsrep_srv_conc_cancel_wait(victim_trx);
 		DBUG_RETURN(rcode);
 	} else {

storage/innobase/include/lock0lock.h

@@ -991,6 +991,14 @@ void
 lock_cancel_waiting_and_release(
 /*============================*/
 	lock_t*	lock);	/*!< in/out: waiting lock request */
+
+/*******************************************************************//**
+Get lock mode and table/index name
+@return	string containing lock info */
+std::string
+lock_get_info(
+	const lock_t*);
+
 #endif /* WITH_WSREP */
 #ifndef UNIV_NONINL
 #include "lock0lock.ic"

storage/innobase/include/trx0trx.h

@@ -673,6 +673,12 @@ lock_rec_convert_impl_to_expl()) will access transactions associated
 to other connections. The locks of transactions are protected by
 lock_sys->mutex and sometimes by trx->mutex. */
 
+typedef enum {
+	TRX_SERVER_ABORT = 0,
+	TRX_WSREP_ABORT  = 1,
+	TRX_REPLICATION_ABORT = 2
+} trx_abort_t;
+
 struct trx_t{
 	ulint		magic_n;
 
@@ -848,8 +854,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. */
+	trx_abort_t	abort_type;	/*!< Transaction abort type*/
+
 	const char*	mysql_log_file_name;
 					/*!< if MySQL binlog is used, this field
 					contains a pointer to the latest file

storage/innobase/lock/lock0lock.cc

@@ -57,6 +57,10 @@ extern my_bool wsrep_debug;
 extern my_bool wsrep_log_conflicts;
 #include "ha_prototypes.h"
 #endif
+
+#include <string>
+#include <sstream>
+
 /* Restricts the length of search we will do in the waits-for
 graph of transactions */
 #define LOCK_MAX_N_STEPS_IN_DEADLOCK_CHECK 1000000
@@ -1729,10 +1733,10 @@ wsrep_kill_victim(
 				}
 			}
 
-			lock->trx->wsrep_abort = TRUE;
+			lock->trx->abort_type = TRX_WSREP_ABORT;
 			wsrep_innobase_kill_one_trx(trx->mysql_thd,
 				(const trx_t*) trx, lock->trx, TRUE);
-			lock->trx->wsrep_abort = FALSE;
+			lock->trx->abort_type = TRX_SERVER_ABORT;
 		}
 	}
 }
@@ -4401,7 +4405,9 @@ 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->abort_type = TRX_REPLICATION_ABORT;
 				thd_report_wait_for(mysql_thd, w_trx->mysql_thd);
+				w_trx->abort_type = TRX_SERVER_ABORT;
 			}
 			++i;
 		}
@@ -7741,3 +7747,32 @@ lock_trx_has_rec_x_lock(
 	return(true);
 }
 #endif /* UNIV_DEBUG */
+
+/*******************************************************************//**
+Get lock mode and table/index name
+@return	string containing lock info */
+std::string
+lock_get_info(
+	const lock_t* lock)
+{
+	std::string info;
+	std::string mode("mode ");
+	std::string index("index ");
+	std::string table("table ");
+	std::string n_uniq(" n_uniq");
+	std::string n_user(" n_user");
+	std::string lock_mode((lock_get_mode_str(lock)));
+	std::string iname(lock->index->name);
+	std::string tname(lock->index->table_name);
+
+#define SSTR( x ) reinterpret_cast< std::ostringstream & >(	\
+        ( std::ostringstream() << std::dec << x ) ).str()
+
+	info = mode + lock_mode
+		+ index + iname
+		+ table + tname
+		+ n_uniq + SSTR(lock->index->n_uniq)
+		+ n_user + SSTR(lock->index->n_user_defined_cols);
+
+	return info;
+}

storage/xtradb/handler/ha_innodb.cc

@@ -5039,25 +5039,46 @@ innobase_kill_connection(
 
 	trx = thd_to_trx(thd);
 
-	if (trx) {
+	if (trx && trx->lock.wait_lock) {
 		/* In wsrep BF we have already took lock_sys and trx
 		mutex either on wsrep_abort_transaction() or
-		before wsrep_kill_victim() */
+		before wsrep_kill_victim(). In replication we
+		could own lock_sys mutex taken in
+		lock_deadlock_check_and_resolve().*/
 
-		if (!wsrep_thd_is_BF(trx->mysql_thd, FALSE)) {
+		WSREP_DEBUG("Killing victim trx %p BF %d trx BF %d trx_id " TRX_ID_FMT " ABORT %d thd %p"
+			" current_thd %p BF %d wait_lock_modes: %s\n",
+			trx, wsrep_thd_is_BF(trx->mysql_thd, FALSE),
+			wsrep_thd_is_BF(thd, FALSE),
+			trx->id, trx->abort_type,
+			trx->mysql_thd,
+			current_thd,
+			wsrep_thd_is_BF(current_thd, FALSE),
+			lock_get_info(trx->lock.wait_lock).c_str());
+
+		if (!wsrep_thd_is_BF(trx->mysql_thd, FALSE) &&
+		    trx->abort_type == TRX_SERVER_ABORT) {
 			ut_ad(!lock_mutex_own());
-			ut_ad(!trx_mutex_own(trx));
 			lock_mutex_enter();
+		}
+
+		if (trx->abort_type != TRX_WSREP_ABORT) {
 			trx_mutex_enter(trx);
 		}
 
 		ut_ad(lock_mutex_own());
 		ut_ad(trx_mutex_own(trx));
 
-		lock_cancel_waiting_and_release(trx->lock.wait_lock);
+		if (trx->lock.wait_lock) {
+			lock_cancel_waiting_and_release(trx->lock.wait_lock);
+		}
 
-		if (!wsrep_thd_is_BF(trx->mysql_thd, FALSE)) {
+		if (trx->abort_type != TRX_WSREP_ABORT) {
 			trx_mutex_exit(trx);
+		}
+
+		if (!wsrep_thd_is_BF(trx->mysql_thd, FALSE) &&
+		    trx->abort_type == TRX_SERVER_ABORT) {
 			lock_mutex_exit();
 		}
 	}
@@ -18699,12 +18720,12 @@ wsrep_abort_transaction(handlerton* hton, THD *bf_thd, THD *victim_thd,
 	if (victim_trx) {
 		lock_mutex_enter();
 		trx_mutex_enter(victim_trx);
-		victim_trx->wsrep_abort = TRUE;
+		victim_trx->abort_type = TRX_WSREP_ABORT;
 		int rcode = wsrep_innobase_kill_one_trx(bf_thd, bf_trx,
                                                         victim_trx, signal);
 		trx_mutex_exit(victim_trx);
 		lock_mutex_exit();
-		victim_trx->wsrep_abort = FALSE;
+		victim_trx->abort_type = TRX_SERVER_ABORT;
 		wsrep_srv_conc_cancel_wait(victim_trx);
 		DBUG_RETURN(rcode);
 	} else {

storage/xtradb/include/lock0lock.h

@@ -39,6 +39,8 @@ Created 5/7/1996 Heikki Tuuri
 #include "srv0srv.h"
 #include "ut0vec.h"
 
+#include <string>
+
 #ifdef UNIV_DEBUG
 extern ibool	lock_print_waits;
 #endif /* UNIV_DEBUG */
@@ -995,6 +997,13 @@ extern lock_sys_t*	lock_sys;
 	mutex_exit(&lock_sys->wait_mutex);	\
 } while (0)
 
+/*******************************************************************//**
+Get lock mode and table/index name
+@return	string containing lock info */
+std::string
+lock_get_info(
+	const lock_t*);
+
 #ifndef UNIV_NONINL
 #include "lock0lock.ic"
 #endif

storage/xtradb/include/trx0trx.h

@@ -705,6 +705,12 @@ lock_rec_convert_impl_to_expl()) will access transactions associated
 to other connections. The locks of transactions are protected by
 lock_sys->mutex and sometimes by trx->mutex. */
 
+typedef enum {
+	TRX_SERVER_ABORT = 0,
+	TRX_WSREP_ABORT  = 1,
+	TRX_REPLICATION_ABORT = 2
+} trx_abort_t;
+
 struct trx_t{
 	ulint		magic_n;
 
@@ -881,8 +887,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. */
+	trx_abort_t	abort_type;	/*!< Transaction abort type */
+
 	const char*	mysql_log_file_name;
 					/*!< if MySQL binlog is used, this field
 					contains a pointer to the latest file

storage/xtradb/lock/lock0lock.cc

@@ -57,6 +57,10 @@ extern my_bool wsrep_debug;
 extern my_bool wsrep_log_conflicts;
 #include "ha_prototypes.h"
 #endif
+
+#include <string>
+#include <sstream>
+
 /* Restricts the length of search we will do in the waits-for
 graph of transactions */
 #define LOCK_MAX_N_STEPS_IN_DEADLOCK_CHECK 1000000
@@ -1728,10 +1732,10 @@ wsrep_kill_victim(
 				}
 			}
 
-			lock->trx->wsrep_abort = TRUE;
+			lock->trx->abort_type = TRX_WSREP_ABORT;
 			wsrep_innobase_kill_one_trx(trx->mysql_thd,
 				(const trx_t*) trx, lock->trx, TRUE);
-			lock->trx->wsrep_abort = FALSE;
+			lock->trx->abort_type = TRX_SERVER_ABORT;
 		}
 	}
 }
@@ -4426,7 +4430,9 @@ 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->abort_type = TRX_REPLICATION_ABORT;
 				thd_report_wait_for(mysql_thd, w_trx->mysql_thd);
+				w_trx->abort_type = TRX_SERVER_ABORT;
 			}
 			++i;
 		}
@@ -7852,3 +7858,32 @@ lock_trx_has_rec_x_lock(
 	return(true);
 }
 #endif /* UNIV_DEBUG */
+
+/*******************************************************************//**
+Get lock mode and table/index name
+@return	string containing lock info */
+std::string
+lock_get_info(
+	const lock_t* lock)
+{
+	std::string info;
+	std::string mode("mode ");
+	std::string index("index ");
+	std::string table("table ");
+	std::string n_uniq(" n_uniq");
+	std::string n_user(" n_user");
+	std::string lock_mode((lock_get_mode_str(lock)));
+	std::string iname(lock->index->name);
+	std::string tname(lock->index->table_name);
+
+#define SSTR( x ) reinterpret_cast< std::ostringstream & >(	\
+        ( std::ostringstream() << std::dec << x ) ).str()
+
+	info = mode + lock_mode
+		+ index + iname
+		+ table + tname
+		+ n_uniq + SSTR(lock->index->n_uniq)
+		+ n_user + SSTR(lock->index->n_user_defined_cols);
+
+	return info;
+}