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 root page latch rather than ibuf_mutex.

ibuf.seg_size: Instead of ibuf_mutex, let the ibuf.index root page latch
protect this field, just like ibuf.empty.

ibuf.free_list_len: Document that modifications are protected by
the ibuf.index root page latch.

ibuf_init_at_db_start(): Acquire the root page latch before updating
ibuf.seg_size.

ibuf_data_enough_free_for_insert(), ibuf_data_too_much_free():
Note that ibuf.free_list_len is not fully protected.

ibuf_remove_free_page(): Acquire the ibuf.index 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_print(): Acquire the ibuf.index root page latch before reading
ibuf.free_list_len and ibuf.seg_size.

Author: dr-m

storage/innobase/ibuf/ibuf0ibuf.cc

@@ -414,6 +414,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 +433,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;);
@@ -1748,6 +1748,7 @@ static inline bool ibuf_data_enough_free_for_insert()
 	inserts buffered for pages that we read to the buffer pool, without
 	any risk of running out of free space in the insert buffer. */
 
+	/* ibuf.free_list_len is NOT protected by the root page latch here */
 	return(ibuf.free_list_len >= (ibuf.size / 2) + 3 * ibuf.height);
 }
 
@@ -1762,6 +1763,8 @@ ibuf_data_too_much_free(void)
 {
 	mysql_mutex_assert_owner(&ibuf_mutex);
 
+	/* In ibuf_free_excess_pages(), ibuf.free_list_len is
+	NOT protected by the root page latch */
 	return(ibuf.free_list_len >= 3 + (ibuf.size / 2) + 3 * ibuf.height);
 }
 
@@ -1876,7 +1879,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 +1895,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 +1916,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 +1931,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,
@@ -1954,8 +1958,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) */
@@ -4486,8 +4488,14 @@ ibuf_print(
   }
 
   const uint32_t size= ibuf.size;
+  mtr_t mtr;
+  std::ignore=
+    buf_page_get_gen(page_id_t{IBUF_SPACE_ID, FSP_IBUF_TREE_ROOT_PAGE_NO},
+                     0, RW_S_LATCH, nullptr, BUF_GET, &mtr, nullptr);
   const uint32_t free_list_len= ibuf.free_list_len;
   const uint32_t seg_size= ibuf.seg_size;
+  mtr.commit();
+
   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 the ibuf.index
+					root page latch. */
+	bool		empty;		/*!< whether the change buffer is
+					empty; protected by the ibuf.index
+					root page latch. */
 	uint8_t		height;		/*!< tree height */
-	uint32_t	free_list_len;	/*!< length of the free list */
+	Atomic_counter<uint32_t> free_list_len;	/*!< length of the free list;
+					modified when holding
+					the ibuf.index root page latch */
 	dict_index_t*	index;		/*!< insert buffer index */
 
 	/** number of pages merged */