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
Tested by: Matthias Leich

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;);
@@ -1746,27 +1747,24 @@ 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
 	can change, and we must make sure that we are able to delete the
 	inserts buffered for pages that we read to the buffer pool, without
 	any risk of running out of free space in the insert buffer. */
-
 	return(ibuf.free_list_len >= (ibuf.size / 2) + 3 * ibuf.height);
 }
 
 /*********************************************************************//**
 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);
 }
 
@@ -1844,6 +1842,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++;
 
@@ -1881,7 +1880,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);
@@ -1897,7 +1896,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;
@@ -1915,7 +1917,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
@@ -1931,15 +1932,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,
@@ -1951,6 +1951,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--;
@@ -1959,8 +1960,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) */
@@ -1988,12 +1987,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;
@@ -3175,8 +3173,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;
 			}
@@ -4485,21 +4486,28 @@ ibuf_print(
 	if (UNIV_UNLIKELY(!ibuf.index)) return;
 	mysql_mutex_lock(&ibuf_mutex);
 
+	const ulint size = ibuf.size;
+	ibuf.index->lock.u_lock(SRW_LOCK_CALL);
+	/* Any modification of these fields is enclosed between
+	ibuf_tree_root_get() and mtr_t::commit(), and thus
+	protected by ibuf.index->lock. */
+	const ulint free_list_len = ibuf.free_list_len;
+	const ulint seg_size = ibuf.seg_size;
+	ibuf.index->lock.u_unlock();
+
+	mysql_mutex_unlock(&ibuf_mutex);
+
 	fprintf(file,
 		"Ibuf: size " ULINTPF ", free list len " ULINTPF ","
 		" seg size " ULINTPF ", " ULINTPF " merges\n",
-		ulint{ibuf.size},
-		ibuf.free_list_len,
-		ibuf.seg_size,
+		size, free_list_len, seg_size,
 		ulint{ibuf.n_merges});
 
 	fputs("merged operations:\n ", file);
 	ibuf_print_ops(ibuf.n_merged_ops, file);
 
 	fputs("discarded operations:\n ", file);
 	ibuf_print_ops(ibuf.n_discarded_ops, file);
-
-	mysql_mutex_unlock(&ibuf_mutex);
 }
 
 /** Check the insert buffer bitmaps on IMPORT TABLESPACE.

storage/innobase/include/ibuf0ibuf.h

@@ -68,14 +68,14 @@ struct ibuf_t{
 					ibuf index tree, in pages */
 	ulint		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. */
-	ulint		free_list_len;	/*!< length of the free list */
+					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 */
+	uint32_t	free_list_len;	/*!< length of the free list;
+					protected by ibuf.index->lock and
+					the root page latch */
 	ulint		height;		/*!< tree height */
 	dict_index_t*	index;		/*!< insert buffer index */