MDEV-33543 Server hang caused by InnoDB change buffer

[mariadb-DebarunBanerjee]

• The Jira issue number for this PR is: MDEV-33543

Description

Issue: When getting a page (buf_page_get_gen) with no latch option (RW_NO_LATCH), the caller is not expected to follow the B-tree latching order. However in buf_page_get_low we try to acquire shared page latch unconditionally to wait for a page that is being loaded by another thread concurrently. In general it could lead to latch order violation and deadlock.

Currently it affects the change buffer insert path btr_latch_prev() which tries to load the previous page out of order with RW_NO_LATCH and two concurrent inserts into IBUF tree cause deadlock. This problem is introduced in 10.6 by MDEV-27058.

Fix: While trying to latch a page with RW_NO_LATCH, always use the "*lock_try" interface and retry operation on failure after unfixing the page.

Release Notes

MDEV-27058 had introduced this regression in 10.6.6. which could hang the server under load If innodb_change_buffering is enabled and user has tables with non unique secondary index.

How can this PR be tested?

It is difficult to maintain an automated mtr test in our regular test suite. The RQG test and the attached code/test patch in MDEV can repeat the issue.

The MDEV has a repeatable test and steps for manual reproduction of the issue.

Basing the PR against the correct MariaDB version

• This is a new feature and the PR is based against the latest MariaDB development branch.
• This is a bug fix and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

• I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
• For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[dr-m]

Thank you. This looks correct to me. In the commit message, please refer to the exact commit that you think introduced this bug, using a wording such as the following:

commit aaef2e1d8c843d1e40b1ce0c5199c3abb3c5da28 (MDEV-27058)

I was thinking if there could be a simpler hang scenario:

1. A page is being read into the buffer pool.
2. The page is being requested by another thread buf_page_get_gen() with RW_NO_LATCH.
3. On read completion, the page turns out to be corrupted, and buf_pool_t::corrupted_evict() will be invoked on it.

This patch does not affect that scenario, because in that case, we would notice that the block state was changed to FREED and release our S-latch and buffer-fix. However, while thinking about this scenario, I noticed that there is a small race condition with buf_pool_t::corrupted_evict(), which would be fixed by releasing the buffer-fix a little later in the error handling code path:

diff --git a/storage/innobase/buf/buf0buf.cc b/storage/innobase/buf/buf0buf.cc
index dc90160a668..12a11aefc12 100644
--- a/storage/innobase/buf/buf0buf.cc
+++ b/storage/innobase/buf/buf0buf.cc
@@ -2587,15 +2587,15 @@ buf_page_get_low(
 		block->page.lock.s_unlock();
 
 		if (UNIV_UNLIKELY(state < buf_page_t::UNFIXED)) {
-			block->page.unfix();
 			if (UNIV_UNLIKELY(id == page_id)) {
 				/* The page read was completed, and
 				another thread marked the page as free
 				while we were waiting. */
-				goto ignore_unfixed;
+				goto ignore;
 			}
 
 			ut_ad(id == page_id_t{~0ULL});
+			block->page.unfix();
 
 			if (++retries < BUF_PAGE_READ_MAX_RETRIES) {
 				goto loop;

If you agree with this, please include this fix as well.