MDEV-28369 ibuf_bitmap_mutex is an unnecessary contention point

The only purpose of ibuf_bitmap_mutex is to prevent a deadlock between two concurrent invocations of ibuf_update_free_bits_for_two_pages_low() on the same pair of bitmap pages, but in opposite order. The mutex is unnecessarily serializing the execution of the function even when it is being invoked on totally different tablespaces. To avoid deadlocks, it suffices to ensure that the two page latches are being acquired in a deterministic (sorted) order.
MariaDB · Apr 21, 2022 · 4730314 · 4730314
1 parent 372b0e6
commit 4730314
Show file tree

Hide file tree

Showing 6 changed files with 14 additions and 50 deletions.
diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc
@@ -629,7 +629,6 @@ static PSI_mutex_info all_innodb_mutexes[] = {
 	PSI_KEY(fts_doc_id_mutex),
 	PSI_KEY(log_flush_order_mutex),
 	PSI_KEY(hash_table_mutex),
-	PSI_KEY(ibuf_bitmap_mutex),
 	PSI_KEY(ibuf_mutex),
 	PSI_KEY(ibuf_pessimistic_insert_mutex),
 	PSI_KEY(index_online_log),

diff --git a/storage/innobase/ibuf/ibuf0ibuf.cc b/storage/innobase/ibuf/ibuf0ibuf.cc
@@ -1,7 +1,7 @@
 /*****************************************************************************
 
 Copyright (c) 1997, 2016, Oracle and/or its affiliates. All Rights Reserved.
-Copyright (c) 2016, 2021, MariaDB Corporation.
+Copyright (c) 2016, 2022, MariaDB Corporation.
 
 This program is free software; you can redistribute it and/or modify it under
 the terms of the GNU General Public License as published by the Free Software
@@ -253,9 +253,6 @@ static ib_mutex_t	ibuf_pessimistic_insert_mutex;
 /** The mutex protecting the insert buffer structs */
 static ib_mutex_t	ibuf_mutex;
 
-/** The mutex protecting the insert buffer bitmaps */
-static ib_mutex_t	ibuf_bitmap_mutex;
-
 /** The area in pages from which contract looks for page numbers for merge */
 const ulint		IBUF_MERGE_AREA = 8;
 
@@ -402,8 +399,6 @@ ibuf_close(void)
 
 	mutex_free(&ibuf_mutex);
 
-	mutex_free(&ibuf_bitmap_mutex);
-
 	dict_table_t*	ibuf_table = ibuf->index->table;
 	rw_lock_free(&ibuf->index->lock);
 	dict_mem_index_free(ibuf->index);
@@ -459,8 +454,6 @@ ibuf_init_at_db_start(void)
 
 	mutex_create(LATCH_ID_IBUF, &ibuf_mutex);
 
-	mutex_create(LATCH_ID_IBUF_BITMAP, &ibuf_bitmap_mutex);
-
 	mutex_create(LATCH_ID_IBUF_PESSIMISTIC_INSERT,
 		     &ibuf_pessimistic_insert_mutex);
 
@@ -1035,26 +1028,16 @@ ibuf_update_free_bits_for_two_pages_low(
 	buf_block_t*	block2,	/*!< in: index page */
 	mtr_t*		mtr)	/*!< in: mtr */
 {
-	ulint	state;
-
-	ut_ad(mtr->is_named_space(block1->page.id.space()));
-	ut_ad(block1->page.id.space() == block2->page.id.space());
-
-	/* As we have to x-latch two random bitmap pages, we have to acquire
-	the bitmap mutex to prevent a deadlock with a similar operation
-	performed by another OS thread. */
-
-	mutex_enter(&ibuf_bitmap_mutex);
-
-	state = ibuf_index_page_calc_free(block1);
-
-	ibuf_set_free_bits_low(block1, state, mtr);
-
-	state = ibuf_index_page_calc_free(block2);
+  ut_ad(mtr->is_named_space(block1->page.id.space()));
+  ut_ad(block1->page.id.space() == block2->page.id.space());
 
-	ibuf_set_free_bits_low(block2, state, mtr);
+  /* Avoid deadlocks by acquiring multiple bitmap page latches in
+  a consistent order (smaller pointer first). */
+  if (block1 > block2)
+    std::swap(block1, block2);
 
-	mutex_exit(&ibuf_bitmap_mutex);
+  ibuf_set_free_bits_low(block1, ibuf_index_page_calc_free(block1), mtr);
+  ibuf_set_free_bits_low(block2, ibuf_index_page_calc_free(block2), mtr);
 }
 
 /** Returns TRUE if the page is one of the fixed address ibuf pages.

diff --git a/storage/innobase/include/sync0sync.h b/storage/innobase/include/sync0sync.h
@@ -3,7 +3,7 @@
 Copyright (c) 1995, 2016, Oracle and/or its affiliates. All Rights Reserved.
 Copyright (c) 2008, Google Inc.
 Copyright (c) 2012, Facebook Inc.
-Copyright (c) 2020, MariaDB Corporation.
+Copyright (c) 2020, 2022, MariaDB Corporation.
 
 Portions of this file contain modifications contributed and copyrighted by
 Google, Inc. Those modifications are gratefully acknowledged and are described
@@ -63,7 +63,6 @@ extern mysql_pfs_key_t	fts_delete_mutex_key;
 extern mysql_pfs_key_t	fts_doc_id_mutex_key;
 extern mysql_pfs_key_t	fts_pll_tokenize_mutex_key;
 extern mysql_pfs_key_t	hash_table_mutex_key;
-extern mysql_pfs_key_t	ibuf_bitmap_mutex_key;
 extern mysql_pfs_key_t	ibuf_mutex_key;
 extern mysql_pfs_key_t	ibuf_pessimistic_insert_mutex_key;
 extern mysql_pfs_key_t	log_sys_mutex_key;

diff --git a/storage/innobase/include/sync0types.h b/storage/innobase/include/sync0types.h
@@ -1,7 +1,7 @@
 /*****************************************************************************
 
 Copyright (c) 1995, 2016, Oracle and/or its affiliates. All Rights Reserved.
-Copyright (c) 2017, 2020, MariaDB Corporation.
+Copyright (c) 2017, 2022, MariaDB Corporation.
 
 This program is free software; you can redistribute it and/or modify it under
 the terms of the GNU General Public License as published by the Free Software
@@ -227,7 +227,6 @@ enum latch_level_t {
 	SYNC_INDEX_ONLINE_LOG,
 
 	SYNC_IBUF_BITMAP,
-	SYNC_IBUF_BITMAP_MUTEX,
 	SYNC_IBUF_TREE_NODE,
 	SYNC_IBUF_TREE_NODE_NEW,
 	SYNC_IBUF_INDEX_TREE,
@@ -295,7 +294,6 @@ enum latch_id_t {
 	LATCH_ID_FTS_DOC_ID,
 	LATCH_ID_FTS_PLL_TOKENIZE,
 	LATCH_ID_HASH_TABLE_MUTEX,
-	LATCH_ID_IBUF_BITMAP,
 	LATCH_ID_IBUF,
 	LATCH_ID_IBUF_PESSIMISTIC_INSERT,
 	LATCH_ID_LOG_SYS,

diff --git a/storage/innobase/sync/sync0debug.cc b/storage/innobase/sync/sync0debug.cc
@@ -473,7 +473,6 @@ LatchDebug::LatchDebug()
 	LEVEL_MAP_INSERT(SYNC_LOCK_WAIT_SYS);
 	LEVEL_MAP_INSERT(SYNC_INDEX_ONLINE_LOG);
 	LEVEL_MAP_INSERT(SYNC_IBUF_BITMAP);
-	LEVEL_MAP_INSERT(SYNC_IBUF_BITMAP_MUTEX);
 	LEVEL_MAP_INSERT(SYNC_IBUF_TREE_NODE);
 	LEVEL_MAP_INSERT(SYNC_IBUF_TREE_NODE_NEW);
 	LEVEL_MAP_INSERT(SYNC_IBUF_INDEX_TREE);
@@ -749,7 +748,6 @@ LatchDebug::check_order(
 	case SYNC_LOCK_WAIT_SYS:
 	case SYNC_RW_TRX_HASH_ELEMENT:
 	case SYNC_TRX_SYS:
-	case SYNC_IBUF_BITMAP_MUTEX:
 	case SYNC_REDO_RSEG:
 	case SYNC_NOREDO_RSEG:
 	case SYNC_PURGE_LATCH:
@@ -834,22 +832,13 @@ LatchDebug::check_order(
 		break;
 
 	case SYNC_IBUF_BITMAP:
-
-		/* Either the thread must own the master mutex to all
-		the bitmap pages, or it is allowed to latch only ONE
-		bitmap page. */
-
-		if (find(latches, SYNC_IBUF_BITMAP_MUTEX) != 0) {
-
-			basic_check(latches, level, SYNC_IBUF_BITMAP - 1);
-
-		} else if (!srv_is_being_started) {
+		if (!srv_is_being_started) {
 
 			/* This is violated during trx_sys_create_rsegs()
 			when creating additional rollback segments during
 			upgrade. */
 
-			basic_check(latches, level, SYNC_IBUF_BITMAP);
+			basic_check(latches, level, SYNC_IBUF_BITMAP - 1);
 		}
 		break;
 
@@ -1291,9 +1280,6 @@ sync_latch_meta_init()
 	LATCH_ADD_MUTEX(HASH_TABLE_MUTEX, SYNC_BUF_PAGE_HASH,
 			hash_table_mutex_key);
 
-	LATCH_ADD_MUTEX(IBUF_BITMAP, SYNC_IBUF_BITMAP_MUTEX,
-			ibuf_bitmap_mutex_key);
-
 	LATCH_ADD_MUTEX(IBUF, SYNC_IBUF_MUTEX, ibuf_mutex_key);
 
 	LATCH_ADD_MUTEX(IBUF_PESSIMISTIC_INSERT, SYNC_IBUF_PESS_INSERT_MUTEX,

diff --git a/storage/innobase/sync/sync0sync.cc b/storage/innobase/sync/sync0sync.cc
@@ -2,7 +2,7 @@
 
 Copyright (c) 1995, 2016, Oracle and/or its affiliates. All Rights Reserved.
 Copyright (c) 2008, Google Inc.
-Copyright (c) 2020, MariaDB Corporation.
+Copyright (c) 2020, 2022, MariaDB Corporation.
 
 Portions of this file contain modifications contributed and copyrighted by
 Google, Inc. Those modifications are gratefully acknowledged and are described
@@ -49,7 +49,6 @@ mysql_pfs_key_t	fts_delete_mutex_key;
 mysql_pfs_key_t	fts_doc_id_mutex_key;
 mysql_pfs_key_t	fts_pll_tokenize_mutex_key;
 mysql_pfs_key_t	hash_table_mutex_key;
-mysql_pfs_key_t	ibuf_bitmap_mutex_key;
 mysql_pfs_key_t	ibuf_mutex_key;
 mysql_pfs_key_t	ibuf_pessimistic_insert_mutex_key;
 mysql_pfs_key_t	log_sys_mutex_key;