MDEV-29930 Lock order inversion in ibuf_remove_free_page()

The function ibuf_remove_free_page() was waiting for ibuf_mutex
while holding ibuf.index->lock. This constitutes a lock order
inversion and may cause InnoDB to hang when innodb_change_buffering
is enabled and ibuf_merge_or_delete_for_page() is being executed
concurrently.

In fact, there is no need for ibuf_remove_free_page() to reacquire
ibuf_mutex if we make ibuf.seg_size and ibuf.free_list_len
protected by the ibuf.index->lock as well as the root page latch rather
than by ibuf_mutex.

ibuf.seg_size, ibuf.free_list_len: Instead of ibuf_mutex, let the
ibuf.index->lock and the root page latch protect these, like ibuf.empty.

ibuf_init_at_db_start(): Acquire the root page latch before updating
ibuf.seg_size. (The ibuf.index would be created later.)

ibuf_data_enough_free_for_insert(), ibuf_data_too_much_free():
Assert also ibuf.index->lock.have_u_or_x().

ibuf_remove_free_page(): Acquire the ibuf.index->lock and the root page
latch before accessing ibuf.free_list_len. Simplify the way how the
root page latch is released and reacquired. Acquire and release
ibuf_mutex only once.

ibuf_free_excess_pages(), ibuf_insert_low(): Acquire also ibuf.index->lock
before reading ibuf.free_list_len.

ibuf_print(): Acquire ibuf.index->lock before reading
ibuf.free_list_len and ibuf.seg_size.

Reviewed by: Vladislav Lesin

Author: dr-m

storage/innobase/ibuf/ibuf0ibuf.cc

@@ -371,6 +371,7 @@ ibuf_size_update(
 	const page_t*	root)	/*!< in: ibuf tree root */
 {
 	mysql_mutex_assert_owner(&ibuf_mutex);
+	ut_ad(!ibuf.index || ibuf.index->lock.have_u_or_x());
 
 	ibuf.free_list_len = flst_get_len(root + PAGE_HEADER
 					   + PAGE_BTR_IBUF_FREE_LIST);
@@ -414,6 +415,15 @@ ibuf_init_at_db_start(void)
 		return err;
 	}
 
+	if (buf_block_t* block =
+	    buf_page_get_gen(page_id_t(IBUF_SPACE_ID,
+				       FSP_IBUF_TREE_ROOT_PAGE_NO),
+			     0, RW_X_LATCH, nullptr, BUF_GET, &mtr, &err)) {
+		root = buf_block_get_frame(block);
+	} else {
+		goto err_exit;
+	}
+
 	fseg_n_reserved_pages(*header_page,
 			      IBUF_HEADER + IBUF_TREE_SEG_HEADER
 			      + header_page->page.frame, &ibuf.seg_size, &mtr);
@@ -424,15 +434,6 @@ ibuf_init_at_db_start(void)
 		ut_ad(ibuf.seg_size >= 2);
 	} while (0);
 
-	if (buf_block_t* block =
-	    buf_page_get_gen(page_id_t(IBUF_SPACE_ID,
-				       FSP_IBUF_TREE_ROOT_PAGE_NO),
-			     0, RW_X_LATCH, nullptr, BUF_GET, &mtr, &err)) {
-		root = buf_block_get_frame(block);
-	} else {
-		goto err_exit;
-	}
-
 	DBUG_EXECUTE_IF("ibuf_init_corrupt",
 			err = DB_CORRUPTION;
 			goto err_exit;);
@@ -1741,6 +1742,7 @@ dare to start a pessimistic insert to the insert buffer.
 static inline bool ibuf_data_enough_free_for_insert()
 {
 	mysql_mutex_assert_owner(&ibuf_mutex);
+	ut_ad(ibuf.index->lock.have_u_or_x());
 
 	/* We want a big margin of free pages, because a B-tree can sometimes
 	grow in size also if records are deleted from it, as the node pointers
@@ -1754,13 +1756,11 @@ static inline bool ibuf_data_enough_free_for_insert()
 /*********************************************************************//**
 Checks if there are enough pages in the free list of the ibuf tree that we
 should remove them and free to the file space management.
-@return TRUE if enough free pages in list */
-UNIV_INLINE
-ibool
-ibuf_data_too_much_free(void)
-/*=========================*/
+@return whether enough free pages in list */
+static inline bool ibuf_data_too_much_free()
 {
 	mysql_mutex_assert_owner(&ibuf_mutex);
+	ut_ad(ibuf.index->lock.have_u_or_x());
 
 	return(ibuf.free_list_len >= 3 + (ibuf.size / 2) + 3 * ibuf.height);
 }
@@ -1839,6 +1839,7 @@ static bool ibuf_add_free_page()
 		goto corrupted;
 	}
 
+	ut_ad(ibuf.index->lock.have_u_or_x());
 	ibuf.seg_size++;
 	ibuf.free_list_len++;
 
@@ -1876,7 +1877,7 @@ static dberr_t ibuf_remove_free_page(bool all = false)
 	mysql_mutex_lock(&ibuf_pessimistic_insert_mutex);
 	mysql_mutex_lock(&ibuf_mutex);
 
-	if (!header_page || (!all && !ibuf_data_too_much_free())) {
+	if (!header_page) {
 early_exit:
 		mysql_mutex_unlock(&ibuf_mutex);
 		mysql_mutex_unlock(&ibuf_pessimistic_insert_mutex);
@@ -1892,7 +1893,10 @@ static dberr_t ibuf_remove_free_page(bool all = false)
 		goto early_exit;
 	}
 
-	const auto root_savepoint = mtr.get_savepoint() - 1;
+	if (!all && !ibuf_data_too_much_free()) {
+		goto early_exit;
+	}
+
 	const uint32_t page_no = flst_get_last(PAGE_HEADER
 					       + PAGE_BTR_IBUF_FREE_LIST
 					       + root->page.frame).page;
@@ -1910,7 +1914,6 @@ static dberr_t ibuf_remove_free_page(bool all = false)
 	because in fseg_free_page we access level 1 pages, and the root
 	is a level 2 page. */
 	root->page.lock.u_unlock();
-	mtr.lock_register(root_savepoint, MTR_MEMO_BUF_FIX);
 	ibuf_exit(&mtr);
 
 	/* Since pessimistic inserts were prevented, we know that the
@@ -1926,15 +1929,14 @@ static dberr_t ibuf_remove_free_page(bool all = false)
 		header_page + IBUF_HEADER + IBUF_TREE_SEG_HEADER,
 		fil_system.sys_space, page_no, &mtr);
 
+	root->page.lock.u_lock();
+
 	if (err != DB_SUCCESS) {
 		goto func_exit;
 	}
 
 	ibuf_enter(&mtr);
 
-	mysql_mutex_lock(&ibuf_mutex);
-	mtr.upgrade_buffer_fix(root_savepoint, RW_X_LATCH);
-
 	/* Remove the page from the free list and update the ibuf size data */
 	if (buf_block_t* block =
 	    buf_page_get_gen(page_id, 0, RW_X_LATCH, nullptr, BUF_GET,
@@ -1946,6 +1948,7 @@ static dberr_t ibuf_remove_free_page(bool all = false)
 	}
 
 	mysql_mutex_unlock(&ibuf_pessimistic_insert_mutex);
+	ut_ad(ibuf.index->lock.have_u_or_x());
 
 	if (err == DB_SUCCESS) {
 		ibuf.seg_size--;
@@ -1954,8 +1957,6 @@ static dberr_t ibuf_remove_free_page(bool all = false)
 	}
 
 func_exit:
-	mysql_mutex_unlock(&ibuf_mutex);
-
 	if (bitmap_page) {
 		/* Set the bit indicating that this page is no more an
 		ibuf tree page (level 2 page) */
@@ -1983,12 +1984,11 @@ ibuf_free_excess_pages(void)
 	requested service too much */
 
 	for (ulint i = 0; i < 4; i++) {
-
-		ibool	too_much_free;
-
 		mysql_mutex_lock(&ibuf_mutex);
-		too_much_free = ibuf_data_too_much_free();
+		ibuf.index->lock.u_lock(SRW_LOCK_CALL);
+		bool too_much_free = ibuf_data_too_much_free();
 		mysql_mutex_unlock(&ibuf_mutex);
+		ibuf.index->lock.u_unlock();
 
 		if (!too_much_free) {
 			return;
@@ -3169,8 +3169,11 @@ ibuf_insert_low(
 		for (;;) {
 			mysql_mutex_lock(&ibuf_pessimistic_insert_mutex);
 			mysql_mutex_lock(&ibuf_mutex);
+			ibuf.index->lock.u_lock(SRW_LOCK_CALL);
+			bool enough = ibuf_data_enough_free_for_insert();
+			ibuf.index->lock.u_unlock();
 
-			if (UNIV_LIKELY(ibuf_data_enough_free_for_insert())) {
+			if (UNIV_LIKELY(enough)) {
 
 				break;
 			}
@@ -4486,8 +4489,11 @@ ibuf_print(
   }
 
   const uint32_t size= ibuf.size;
+  ibuf.index->lock.u_lock(SRW_LOCK_CALL);
   const uint32_t free_list_len= ibuf.free_list_len;
   const uint32_t seg_size= ibuf.seg_size;
+  ibuf.index->lock.u_unlock();
+
   mysql_mutex_unlock(&ibuf_mutex);
 
   fprintf(file,

storage/innobase/include/ibuf0ibuf.h

@@ -68,15 +68,15 @@ struct ibuf_t{
 					ibuf index tree, in pages */
 	uint32_t	seg_size;	/*!< allocated pages of the file
 					segment containing ibuf header and
-					tree */
-	bool		empty;		/*!< Protected by the page
-					latch of the root page of the
-					insert buffer tree
-					(FSP_IBUF_TREE_ROOT_PAGE_NO). true
-					if and only if the insert
-					buffer tree is empty. */
+					tree; protected by ibuf.index->lock
+					and the root page latch */
+	bool		empty;		/*!< whether the change buffer is
+					empty; protected by ibuf.index->lock
+					and the root page latch */
 	uint8_t		height;		/*!< tree height */
-	uint32_t	free_list_len;	/*!< length of the free list */
+	uint32_t	free_list_len;	/*!< length of the free list;
+					protected by ibuf.index->lock and
+					the root page latch */
 	dict_index_t*	index;		/*!< insert buffer index */
 
 	/** number of pages merged */