From 05d57d9bf093b7c5dea062200b1351e44c4a0970 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Olchawa?= Date: Tue, 21 Jul 2026 09:34:23 +0200 Subject: [PATCH 1/2] Narrow the LRU list mutex scope in buf_page_init_for_read() On IO-bound workloads every physical read serialized on the pool-wide LRU list mutex, because buf_page_init_for_read() held it across the page-hash latch acquisition, the page-hash lookup and insert, and the descriptor initialization. This narrows that scope and adjusts the surrounding paths accordingly. 1. Narrow the LRU list mutex in buf_page_init_for_read() to cover only the LRU list insert. The page-hash lookup and insert, the descriptor initialization and the read io-fix now run under the page-hash cell X-latch alone. Page-hash membership is from now on serialized by the hash cell latch, not by the LRU list mutex. 2. This exposes a short window in which a page is reachable through the page hash but not yet linked into the LRU list. It is made safe by the read io-fix, which is published before the page becomes hash-reachable and cleared only after the LRU-add: threads that find the page through the hash back off on the io-fix, readers wait on the frame X-lock, and eviction cannot see it. In particular buf_page_make_young_if_needed() skips such a page, so promotion never manipulates a not-yet-linked node. 3. Introduce a latching rule: no thread may wait for a block frame rw-lock while holding a buffer pool LRU list mutex. The paths that latch a frame under the LRU list mutex use the rw_lock_*_nowait() variants; the rule is enforced in debug builds by rw_lock_assert_wait_allowed(). 4. Keep the page-hash cell X-latch across the delete + re-insert of the keep-zip path of buf_LRU_free_page() (new keep_hash_lock parameter), so a page id is never observably absent from the page hash while it is still logically in the buffer pool. Without this a concurrent read could insert a duplicate descriptor for the same page id. It also closes, at the root, the window in which a compressed page looked absent from the buffer pool, so that the change buffer could buffer an operation for a page which was in fact still cached in compressed form. 5. Drop the LRU list mutex from buf_pool_watch_set(). It was taken only to avoid the keep-zip gap of point 4; with that gap now closed by the continuous hash cell latch, the hash cell latches purge already holds are sufficient, and purge no longer contends on the LRU list mutex. 6. Add debug assertions guarding the new invariants and a stress test (innodb_zip.lru_mutex_narrow_stress_debug) that widens the windows via debug sync points and drives the attacking compressed-table workload. --- ...buf_page_init_for_read_resize_debug.result | 65 +++++ ...page_init_for_read_resize_debug-master.opt | 4 + .../buf_page_init_for_read_resize_debug.test | 153 ++++++++++ .../r/lru_mutex_narrow_stress_debug.result | 16 + .../lru_mutex_narrow_stress_debug-master.opt | 3 + .../t/lru_mutex_narrow_stress_debug.test | 109 +++++++ storage/innobase/btr/btr0sea.cc | 13 +- storage/innobase/buf/buf0buddy.cc | 25 ++ storage/innobase/buf/buf0buf.cc | 275 ++++++++++++++---- storage/innobase/buf/buf0lru.cc | 116 ++++++-- storage/innobase/buf/buf0rea.cc | 16 +- storage/innobase/include/buf0buf.h | 31 +- storage/innobase/include/buf0buf.ic | 7 +- storage/innobase/sync/sync0debug.cc | 12 +- storage/innobase/sync/sync0rw.cc | 44 +++ 15 files changed, 801 insertions(+), 88 deletions(-) create mode 100644 mysql-test/suite/innodb/r/buf_page_init_for_read_resize_debug.result create mode 100644 mysql-test/suite/innodb/t/buf_page_init_for_read_resize_debug-master.opt create mode 100644 mysql-test/suite/innodb/t/buf_page_init_for_read_resize_debug.test create mode 100644 mysql-test/suite/innodb_zip/r/lru_mutex_narrow_stress_debug.result create mode 100644 mysql-test/suite/innodb_zip/t/lru_mutex_narrow_stress_debug-master.opt create mode 100644 mysql-test/suite/innodb_zip/t/lru_mutex_narrow_stress_debug.test diff --git a/mysql-test/suite/innodb/r/buf_page_init_for_read_resize_debug.result b/mysql-test/suite/innodb/r/buf_page_init_for_read_resize_debug.result new file mode 100644 index 000000000000..b8d966f379b5 --- /dev/null +++ b/mysql-test/suite/innodb/r/buf_page_init_for_read_resize_debug.result @@ -0,0 +1,65 @@ +CREATE TABLE t1 (id INT PRIMARY KEY, c CHAR(200)) ENGINE=InnoDB; +INSERT INTO t1 VALUES (1, 'a'), (2, 'b'), (3, 'c'), (4, 'd'); +INSERT INTO t1 SELECT id + 4, c FROM t1; +INSERT INTO t1 SELECT id + 8, c FROM t1; +INSERT INTO t1 SELECT id + 16, c FROM t1; +INSERT INTO t1 SELECT id + 32, c FROM t1; +INSERT INTO t1 SELECT id + 64, c FROM t1; +INSERT INTO t1 SELECT id + 128, c FROM t1; +INSERT INTO t1 SELECT id + 256, c FROM t1; +INSERT INTO t1 SELECT id + 512, c FROM t1; +INSERT INTO t1 SELECT id + 1024, c FROM t1; +CREATE TABLE t2 (id INT PRIMARY KEY, c CHAR(200)) ENGINE=InnoDB; +INSERT INTO t2 SELECT id, c FROM t1; +INSERT INTO t2 SELECT id + 2048, c FROM t2; +INSERT INTO t2 SELECT id + 4096, c FROM t2; +INSERT INTO t2 SELECT id + 8192, c FROM t2; +INSERT INTO t2 SELECT id + 16384, c FROM t2; +INSERT INTO t2 SELECT id + 32768, c FROM t2; +# +# Scenario 1: the page hash is rebuilt between computing the page hash +# cell latch and acquiring it. +# +# restart +SET @old_buffer_pool_size = @@GLOBAL.innodb_buffer_pool_size; +SELECT c FROM t1 WHERE id = 1; +c +a +SET DEBUG_SYNC = 'buf_page_init_for_read_before_hash_lock SIGNAL paused WAIT_FOR go'; +SELECT c FROM t1 WHERE id = 2000; +SET DEBUG_SYNC = 'now WAIT_FOR paused'; +SET GLOBAL innodb_buffer_pool_size = @old_buffer_pool_size * 8; +SET DEBUG_SYNC = 'now SIGNAL go'; +c +d +SET DEBUG_SYNC = 'RESET'; +# +# Scenario 2: the buffer pool is shrunk while a page is in the page +# hash but not yet on the LRU list. +# +# restart: --innodb-buffer-pool-size=32M +SELECT c FROM t1 WHERE id = 1; +c +a +SELECT SUM(LENGTH(c)) FROM t2; +SET DEBUG_SYNC = 'buf_page_init_for_read_before_lru_add SIGNAL paused WAIT_FOR go'; +SELECT c FROM t1 WHERE id = 2000; +SET DEBUG_SYNC = 'now WAIT_FOR paused'; +SET GLOBAL innodb_buffer_pool_size = 8 * 1024 * 1024; +SELECT SUBSTR(variable_value, 1, 18) = 'Completed resizing' AS completed +FROM performance_schema.global_status +WHERE LOWER(variable_name) = 'innodb_buffer_pool_resize_status'; +completed +0 +SET DEBUG_SYNC = 'now SIGNAL go'; +c +d +SELECT @@GLOBAL.innodb_buffer_pool_size = 8 * 1024 * 1024 AS shrunk; +shrunk +1 +SELECT c FROM t1 WHERE id = 2000; +c +d +SET DEBUG_SYNC = 'RESET'; +DROP TABLE t1, t2; +# restart diff --git a/mysql-test/suite/innodb/t/buf_page_init_for_read_resize_debug-master.opt b/mysql-test/suite/innodb/t/buf_page_init_for_read_resize_debug-master.opt new file mode 100644 index 000000000000..9edc727b4760 --- /dev/null +++ b/mysql-test/suite/innodb/t/buf_page_init_for_read_resize_debug-master.opt @@ -0,0 +1,4 @@ +--innodb-buffer-pool-size=16M +--innodb-buffer-pool-chunk-size=2M +--innodb-page-hash-locks=1024 +--innodb-adaptive-hash-index=OFF diff --git a/mysql-test/suite/innodb/t/buf_page_init_for_read_resize_debug.test b/mysql-test/suite/innodb/t/buf_page_init_for_read_resize_debug.test new file mode 100644 index 000000000000..56ddcee01b63 --- /dev/null +++ b/mysql-test/suite/innodb/t/buf_page_init_for_read_resize_debug.test @@ -0,0 +1,153 @@ +# buf_page_init_for_read() does not hold the LRU list mutex, so it can run +# concurrently with a buffer pool resize. This test covers two windows: +# +# 1) Between computing the page hash cell latch for the page id and acquiring +# it. A resize which rebuilds the page hash (the new size differs from the +# old one by more than a factor of two) changes the number of cells, and +# so the latch the page id maps to. The function must re-confirm the latch +# after acquiring it, otherwise it looks up and inserts into the page hash +# under a latch which does not protect the page id. +# +# 2) Between inserting the page into the page hash and linking it into the +# LRU list. A shrinking resize must not withdraw the block of such a page +# (it is io-fixed for read and not on the LRU list), but retry until the +# read has completed, and it must move the page to its new page hash cell +# when it rebuilds the page hash. + +--source include/have_debug.inc +--source include/have_debug_sync.inc +--source include/have_innodb_16k.inc + +CREATE TABLE t1 (id INT PRIMARY KEY, c CHAR(200)) ENGINE=InnoDB; +INSERT INTO t1 VALUES (1, 'a'), (2, 'b'), (3, 'c'), (4, 'd'); +INSERT INTO t1 SELECT id + 4, c FROM t1; +INSERT INTO t1 SELECT id + 8, c FROM t1; +INSERT INTO t1 SELECT id + 16, c FROM t1; +INSERT INTO t1 SELECT id + 32, c FROM t1; +INSERT INTO t1 SELECT id + 64, c FROM t1; +INSERT INTO t1 SELECT id + 128, c FROM t1; +INSERT INTO t1 SELECT id + 256, c FROM t1; +INSERT INTO t1 SELECT id + 512, c FROM t1; +INSERT INTO t1 SELECT id + 1024, c FROM t1; + +# About 17 MB of data, used to fill the buffer pool in the second scenario. +CREATE TABLE t2 (id INT PRIMARY KEY, c CHAR(200)) ENGINE=InnoDB; +INSERT INTO t2 SELECT id, c FROM t1; +INSERT INTO t2 SELECT id + 2048, c FROM t2; +INSERT INTO t2 SELECT id + 4096, c FROM t2; +INSERT INTO t2 SELECT id + 8192, c FROM t2; +INSERT INTO t2 SELECT id + 16384, c FROM t2; +INSERT INTO t2 SELECT id + 32768, c FROM t2; + +--echo # +--echo # Scenario 1: the page hash is rebuilt between computing the page hash +--echo # cell latch and acquiring it. +--echo # + +# Start with a cold buffer pool, so that the leaf pages of t1 must be read. +# The resize status is empty after the restart, so the wait for +# 'Completed resizing' below cannot be satisfied by an earlier resize. +--source include/restart_mysqld.inc +--source include/count_sessions.inc + +SET @old_buffer_pool_size = @@GLOBAL.innodb_buffer_pool_size; + +# Open the table and read the root and the first leaf page. +SELECT c FROM t1 WHERE id = 1; + +--connect (con1, localhost, root,,) +SET DEBUG_SYNC = 'buf_page_init_for_read_before_hash_lock SIGNAL paused WAIT_FOR go'; +--send SELECT c FROM t1 WHERE id = 2000 + +--connection default +SET DEBUG_SYNC = 'now WAIT_FOR paused'; + +# Grow the buffer pool by more than a factor of two, so that the page hash +# is rebuilt with a different number of cells. +SET GLOBAL innodb_buffer_pool_size = @old_buffer_pool_size * 8; +let $wait_timeout = 180; +let $wait_condition = + SELECT SUBSTR(variable_value, 1, 34) = 'Completed resizing buffer pool at ' + FROM performance_schema.global_status + WHERE LOWER(variable_name) = 'innodb_buffer_pool_resize_status'; +--source include/wait_condition.inc + +SET DEBUG_SYNC = 'now SIGNAL go'; + +--connection con1 +--reap +--disconnect con1 + +--connection default +SET DEBUG_SYNC = 'RESET'; +--source include/wait_until_count_sessions.inc + +--echo # +--echo # Scenario 2: the buffer pool is shrunk while a page is in the page +--echo # hash but not yet on the LRU list. +--echo # + +--let $restart_parameters = restart: --innodb-buffer-pool-size=32M +--source include/restart_mysqld.inc +--source include/count_sessions.inc + +# Open t1 and read its root page, then fill the buffer pool with t2. Free +# blocks are taken from the head of the free list, which is ordered by chunk, +# so the next page read gets a block from one of the last chunks, which the +# shrink below withdraws. +SELECT c FROM t1 WHERE id = 1; +--disable_result_log +SELECT SUM(LENGTH(c)) FROM t2; +--enable_result_log + +--connect (con1, localhost, root,,) +SET DEBUG_SYNC = 'buf_page_init_for_read_before_lru_add SIGNAL paused WAIT_FOR go'; +--send SELECT c FROM t1 WHERE id = 2000 + +--connection default +SET DEBUG_SYNC = 'now WAIT_FOR paused'; + +let $retries_before = `SELECT COUNT(*) FROM performance_schema.error_log + WHERE data LIKE 'Will retry to withdraw%'`; + +# Shrink the buffer pool by more than a factor of two, so that the page hash +# is rebuilt as well. +SET GLOBAL innodb_buffer_pool_size = 8 * 1024 * 1024; + +# The withdrawal cannot complete while the block of the page being read is +# neither free nor on the LRU list; it must keep retrying. Wait for two +# retries: the first one can also be caused by pages of t2 which the +# withdrawal still has to evict. +let $wait_condition = + SELECT COUNT(*) > $retries_before + 1 FROM performance_schema.error_log + WHERE data LIKE 'Will retry to withdraw%'; +--source include/wait_condition.inc + +SELECT SUBSTR(variable_value, 1, 18) = 'Completed resizing' AS completed + FROM performance_schema.global_status + WHERE LOWER(variable_name) = 'innodb_buffer_pool_resize_status'; + +SET DEBUG_SYNC = 'now SIGNAL go'; + +--connection con1 +--reap +--disconnect con1 + +--connection default +let $wait_condition = + SELECT SUBSTR(variable_value, 1, 34) = 'Completed resizing buffer pool at ' + FROM performance_schema.global_status + WHERE LOWER(variable_name) = 'innodb_buffer_pool_resize_status'; +--source include/wait_condition.inc + +SELECT @@GLOBAL.innodb_buffer_pool_size = 8 * 1024 * 1024 AS shrunk; +SELECT c FROM t1 WHERE id = 2000; + +SET DEBUG_SYNC = 'RESET'; +DROP TABLE t1, t2; + +--source include/wait_until_count_sessions.inc + +# Restore the original buffer pool size. +--let $restart_parameters = restart +--source include/restart_mysqld.inc diff --git a/mysql-test/suite/innodb_zip/r/lru_mutex_narrow_stress_debug.result b/mysql-test/suite/innodb_zip/r/lru_mutex_narrow_stress_debug.result new file mode 100644 index 000000000000..81f2172016b8 --- /dev/null +++ b/mysql-test/suite/innodb_zip/r/lru_mutex_narrow_stress_debug.result @@ -0,0 +1,16 @@ +SET GLOBAL DEBUG="+d,buf_lru_free_page_delay_zip_reinsert,buf_page_init_for_read_delay_lru_add"; +CREATE TABLE t1 ( +id INT PRIMARY KEY, +c LONGBLOB +) ENGINE=InnoDB ROW_FORMAT=COMPRESSED KEY_BLOCK_SIZE=8; +CALL scan_t1(6); +CALL scan_t1(6); +CALL update_t1(12); +SELECT COUNT(*), MIN(LENGTH(c)) > 0 FROM t1; +COUNT(*) MIN(LENGTH(c)) > 0 +2000 1 +SET GLOBAL DEBUG="-d,buf_lru_free_page_delay_zip_reinsert,buf_page_init_for_read_delay_lru_add"; +DROP PROCEDURE populate_t1; +DROP PROCEDURE scan_t1; +DROP PROCEDURE update_t1; +DROP TABLE t1; diff --git a/mysql-test/suite/innodb_zip/t/lru_mutex_narrow_stress_debug-master.opt b/mysql-test/suite/innodb_zip/t/lru_mutex_narrow_stress_debug-master.opt new file mode 100644 index 000000000000..451844199700 --- /dev/null +++ b/mysql-test/suite/innodb_zip/t/lru_mutex_narrow_stress_debug-master.opt @@ -0,0 +1,3 @@ +--innodb-buffer-pool-size=24M +--innodb-buffer-pool-chunk-size=2M +--innodb-buffer-pool-instances=1 diff --git a/mysql-test/suite/innodb_zip/t/lru_mutex_narrow_stress_debug.test b/mysql-test/suite/innodb_zip/t/lru_mutex_narrow_stress_debug.test new file mode 100644 index 000000000000..440262324537 --- /dev/null +++ b/mysql-test/suite/innodb_zip/t/lru_mutex_narrow_stress_debug.test @@ -0,0 +1,109 @@ +# buf_page_init_for_read() inserts into the page hash without +# holding the LRU list mutex. This test stresses the two windows that the +# narrowed latching opens, with debug sync points widening them: +# +# 1) buf_lru_free_page_delay_zip_reinsert widens the window between the +# page hash delete and the re-insert of the compressed-only descriptor +# in the keep-zip path of buf_LRU_free_page(). The hash cell X-latch is +# held across that window, so a concurrent read of the page being +# unzip-evicted must block on the cell latch instead of inserting a +# second descriptor for the same page id (which would fail +# ut_a(!buf_page_hash_get_low(buf_pool, b->id)) at the re-insert). +# +# 2) buf_page_init_for_read_delay_lru_add widens the window in which a +# page is hash-visible but not yet linked into the LRU list. Threads +# finding such a page via the page hash (buf_page_get_zip() -> +# buf_block_try_discard_uncompressed() -> buf_LRU_free_page(), +# buf_buddy_relocate(), buf_page_make_young_if_needed()) must back off +# from it because it is io-fixed for read; buf_page_can_relocate() +# accepts it (io-fixed, not in the LRU list) without asserting. +# +# The workload keeps a compressed table with externally stored BLOBs +# (read through buf_page_get_zip()) larger than the buffer pool, so that +# unzip-LRU eviction, keep-zip frees, physical re-reads and buddy +# alloc/free churn all run concurrently. + +--source include/have_debug.inc +--source include/have_innodb_16k.inc +--source include/big_test.inc + +SET GLOBAL DEBUG="+d,buf_lru_free_page_delay_zip_reinsert,buf_page_init_for_read_delay_lru_add"; + +CREATE TABLE t1 ( + id INT PRIMARY KEY, + c LONGBLOB +) ENGINE=InnoDB ROW_FORMAT=COMPRESSED KEY_BLOCK_SIZE=8; + +--disable_query_log +DELIMITER |; +CREATE PROCEDURE populate_t1() +BEGIN + DECLARE i INT DEFAULT 1; + WHILE i <= 2000 DO + INSERT INTO t1 VALUES (i, REPEAT(CONCAT('row', i, '-'), 2048)); + IF i % 200 = 0 THEN + COMMIT; + END IF; + SET i = i + 1; + END WHILE; + COMMIT; +END| +CREATE PROCEDURE scan_t1(IN iterations INT) +BEGIN + DECLARE i INT DEFAULT 1; + DECLARE dummy BIGINT; + WHILE i <= iterations DO + SELECT SUM(LENGTH(c)) INTO dummy FROM t1; + SET i = i + 1; + END WHILE; +END| +CREATE PROCEDURE update_t1(IN iterations INT) +BEGIN + DECLARE i INT DEFAULT 1; + WHILE i <= iterations DO + UPDATE t1 SET c = REPEAT(CONCAT('mod', i, '-'), 2048) + WHERE id % 500 = i % 500; + COMMIT; + SET i = i + 1; + END WHILE; +END| +DELIMITER ;| +SET autocommit = 0; +CALL populate_t1(); +SET autocommit = 1; +--enable_query_log + +# Two connections scanning the BLOB column (physical re-reads + +# buf_page_get_zip() -> buf_block_try_discard_uncompressed()), one +# connection updating (creates dirty compressed pages, so the keep-zip +# eviction path sees both ZIP_PAGE and ZIP_DIRTY re-inserts). +--connect(con_scan1, localhost, root,,) +--send CALL scan_t1(6) + +--connect(con_scan2, localhost, root,,) +--send CALL scan_t1(6) + +--connect(con_upd, localhost, root,,) +--send CALL update_t1(12) + +--connection con_scan1 +--reap +--disconnect con_scan1 + +--connection con_scan2 +--reap +--disconnect con_scan2 + +--connection con_upd +--reap +--disconnect con_upd + +--connection default +SELECT COUNT(*), MIN(LENGTH(c)) > 0 FROM t1; + +SET GLOBAL DEBUG="-d,buf_lru_free_page_delay_zip_reinsert,buf_page_init_for_read_delay_lru_add"; + +DROP PROCEDURE populate_t1; +DROP PROCEDURE scan_t1; +DROP PROCEDURE update_t1; +DROP TABLE t1; diff --git a/storage/innobase/btr/btr0sea.cc b/storage/innobase/btr/btr0sea.cc index 3a8fa00c7091..44a439ff7d39 100644 --- a/storage/innobase/btr/btr0sea.cc +++ b/storage/innobase/btr/btr0sea.cc @@ -1976,12 +1976,13 @@ static bool btr_search_hash_table_validate(ulint part_id) { /* When a block is being freed, buf_LRU_free_page() first removes the block from - buf_pool->page_hash by calling - buf_LRU_block_remove_hashed_page(). - After that, it invokes - buf_LRU_block_remove_hashed() to - remove the block from - btr_search_sys->hash_tables[i]. */ + buf_pool->page_hash and sets it to + BUF_BLOCK_REMOVE_HASH by calling + buf_LRU_block_remove_hashed(). + After that, it removes the block + from btr_search_sys->parts[i] (the + AHI) by calling + btr_search_drop_page_hash_index(). */ ut_a(buf_block_get_state(block) == BUF_BLOCK_REMOVE_HASH); } diff --git a/storage/innobase/buf/buf0buddy.cc b/storage/innobase/buf/buf0buddy.cc index 30be4a231bcc..9e0f54e15d45 100644 --- a/storage/innobase/buf/buf0buddy.cc +++ b/storage/innobase/buf/buf0buddy.cc @@ -40,6 +40,9 @@ this program; if not, write to the Free Software Foundation, Inc., #include "page0zip.h" +#include "sync0debug.h" +#include "sync0types.h" + /** When freeing a buf we attempt to coalesce by looking at its buddy and deciding whether it is free or not. To ascertain if the buddy is free we look for BUF_BUDDY_STAMP_FREE at BUF_BUDDY_STAMP_OFFSET @@ -416,6 +419,25 @@ static void *buf_buddy_alloc_from(buf_pool_t *buf_pool, void *buf, ulint i, return (buf); } +#ifdef UNIV_DEBUG +/** Asserts that the calling thread holds no buffer pool page hash cell +latch (S or X). The buddy allocator must never be entered while one is +held: buf_buddy_free() may recombine and call buf_buddy_relocate(), +which acquires the cell X-latch of whatever page id is stamped in the +buddy frame - possibly the very cell the caller holds, and in any case +an unordered same-level acquisition (page-hash cell -> zip_free_mutex -> +page-hash cell) which can form a deadlock cycle with other threads. +This rule is what makes it safe for the keep-zip path of +buf_LRU_free_page() to keep a cell X-latch across +buf_LRU_block_remove_hashed() (keep_hash_lock): that path never reaches +the buddy allocator. */ +namespace { +void buf_buddy_no_page_hash_latch_validate() { + ut_ad(sync_check_find(SYNC_BUF_PAGE_HASH) == nullptr); +} +} // namespace +#endif /* UNIV_DEBUG */ + /** Allocate a block. @param[in,out] buf_pool buffer pool instance @param[in] i index of buf_pool->zip_free[] @@ -425,6 +447,7 @@ void *buf_buddy_alloc_low(buf_pool_t *buf_pool, ulint i) { buf_block_t *block; ut_ad(!mutex_own(&buf_pool->zip_mutex)); + ut_d(buf_buddy_no_page_hash_latch_validate()); ut_ad(i >= buf_buddy_get_slot(UNIV_ZIP_SIZE_MIN)); if (i < BUF_BUDDY_SIZES) { @@ -476,6 +499,7 @@ static bool buf_buddy_relocate(buf_pool_t *buf_pool, void *src, void *dst, ut_ad(mutex_own(&buf_pool->zip_free_mutex)); ut_ad(!mutex_own(&buf_pool->zip_mutex)); + ut_d(buf_buddy_no_page_hash_latch_validate()); ut_ad(!ut_align_offset(src, size)); ut_ad(!ut_align_offset(dst, size)); ut_ad(i >= buf_buddy_get_slot(UNIV_ZIP_SIZE_MIN)); @@ -611,6 +635,7 @@ void buf_buddy_free_low(buf_pool_t *buf_pool, void *buf, ulint i, buf_buddy_free_t *buddy; ut_ad(!mutex_own(&buf_pool->zip_mutex)); + ut_d(buf_buddy_no_page_hash_latch_validate()); ut_ad(i <= BUF_BUDDY_SIZES); ut_ad(i >= buf_buddy_get_slot(UNIV_ZIP_SIZE_MIN)); diff --git a/storage/innobase/buf/buf0buf.cc b/storage/innobase/buf/buf0buf.cc index 395910c62971..a12ea58e2194 100644 --- a/storage/innobase/buf/buf0buf.cc +++ b/storage/innobase/buf/buf0buf.cc @@ -2993,9 +2993,8 @@ bool buf_pool_watch_is_sentinel(const buf_pool_t *buf_pool, } /** Add watch for the given page to be read in. Caller must have -appropriate hash_lock for the bpage and hold the LRU list mutex to avoid a race -condition with buf_LRU_free_page inserting the same page into the page hash. -This function may release the hash_lock and reacquire it. +appropriate hash_lock for the bpage. This function may release the +hash_lock and reacquire it. @param[in] page_id page id @param[in,out] hash_lock hash_lock currently latched @return NULL if watch set, block if the page is in the buffer pool */ @@ -3027,15 +3026,25 @@ static buf_page_t *buf_pool_watch_set(const page_id_t &page_id, of latching. We acquire all the hash_locks. They are needed because we don't want to read any stale information in buf_pool->watch[]. However, it is not in the critical code path - as this function will be called only by the purge thread. */ + as this function will be called only by the purge thread. + + Holding all the hash cell X-latches also stabilizes page hash + membership: every insert into and delete from the page hash happens + under the affected cell's X-latch, including the delete + re-insert + transition of the keep-zip path of buf_LRU_free_page(), which keeps + the cell's X-latch continuously so that the page id is never + observably absent from the hash (see the page_hash membership-change + protocol at its declaration in buf0buf.h). Therefore the LRU list + mutex does not need to be (and is not) taken here: the recheck below + cannot miss a page which is logically in the buffer pool. */ /* To obey latching order first release the hash_lock. */ rw_lock_x_unlock(*hash_lock); - mutex_enter(&buf_pool->LRU_list_mutex); hash_lock_x_all(buf_pool->page_hash); - /* If not own LRU_list_mutex, page_hash can be changed. */ + /* page_hash could have been resized while we did not hold any + hash cell latch. */ *hash_lock = buf_page_hash_lock_get(buf_pool, page_id); /* We have to recheck that the page @@ -3046,7 +3055,6 @@ static buf_page_t *buf_pool_watch_set(const page_id_t &page_id, bpage = buf_page_hash_get_low(buf_pool, page_id); if (bpage) { - mutex_exit(&buf_pool->LRU_list_mutex); hash_unlock_x_all_but(buf_pool->page_hash, *hash_lock); goto page_found; } @@ -3077,8 +3085,6 @@ static buf_page_t *buf_pool_watch_set(const page_id_t &page_id, HASH_INSERT(buf_page_t, hash, buf_pool->page_hash, page_id.hash(), bpage); - mutex_exit(&buf_pool->LRU_list_mutex); - /* Once the sentinel is in the page_hash we can safely release all locks except just the relevant hash_lock */ @@ -3133,7 +3139,9 @@ void buf_pool_watch_unset(const page_id_t &page_id) { rw_lock_t *hash_lock = buf_page_hash_lock_get(buf_pool, page_id); rw_lock_x_lock(hash_lock, UT_LOCATION_HERE); - /* page_hash can be changed. */ + /* A concurrent buffer pool resize can rehash page_hash and remap this + page id to a different shard latch between computing hash_lock and + latching it; re-confirm and re-latch the correct shard. */ hash_lock = buf_page_hash_lock_x_confirm(hash_lock, buf_pool, page_id); /* The page must exist because buf_pool_watch_set() @@ -3160,7 +3168,9 @@ bool buf_pool_watch_occurred(const page_id_t &page_id) { rw_lock_s_lock(hash_lock, UT_LOCATION_HERE); - /* If not own buf_pool_mutex, page_hash can be changed. */ + /* A concurrent buffer pool resize can rehash page_hash and remap this + page id to a different shard latch between computing hash_lock and + latching it; re-confirm and re-latch the correct shard. */ hash_lock = buf_page_hash_lock_s_confirm(hash_lock, buf_pool, page_id); /* The page must exist because buf_pool_watch_set() @@ -3210,6 +3220,26 @@ static void buf_page_make_young_if_needed(buf_page_t *bpage) { ut_ad(bpage->buf_fix_count > 0); ut_a(buf_page_in_file(bpage)); + /* A page whose read IO is still in progress may not yet be linked into + the LRU list: buf_page_init_for_read() makes the page hash-visible before + it links it into the LRU list. Such a page must not be promoted - + buf_LRU_make_block_young() would unlink a node which is not linked, + corrupting the LRU list. + Reading the io-fix snapshot without the block mutex is correct here: the + LRU-add happens-before the read IO is dispatched, which happens-before + io_fix is reset to BUF_IO_NONE at IO completion. Thus observing + !was_io_fix_read() implies the LRU-add has already happened, and the + buf-fix held by our caller keeps the page in the LRU. Skipping the + promotion on a stale BUF_IO_READ snapshot is benign: the page was just + added at the head of the old sublist and a subsequent access will promote + it. + Both orderings this argument depends on (BUF_IO_READ is published before + the page is hash-reachable; the read io-fix is cleared only after the + LRU-add) are asserted at the transitions in buf_page_t::set_io_fix(). */ + if (bpage->was_io_fix_read()) { + return; + } + if (buf_page_peek_if_too_old(bpage)) { buf_page_make_young(bpage); } @@ -3854,7 +3884,9 @@ buf_block_t *Buf_fetch::lookup() { rw_lock_s_lock(m_hash_lock, UT_LOCATION_HERE); - /* If not own LRU_list_mutex, page_hash can be changed. */ + /* A concurrent buffer pool resize can rehash page_hash and remap this + page id to a different shard latch between computing m_hash_lock and + latching it; re-confirm and re-latch the correct shard. */ m_hash_lock = buf_page_hash_lock_s_confirm(m_hash_lock, m_buf_pool, m_page_id); @@ -3904,7 +3936,10 @@ buf_block_t *Buf_fetch::is_on_watch() { rw_lock_x_lock(m_hash_lock, UT_LOCATION_HERE); - /* If not own LRU_list_mutex, page_hash can be changed. */ + /* A concurrent buffer pool resize can rehash page_hash (buf_pool_resize() + changes the number of cells), remapping this page id to a different shard + latch after we computed m_hash_lock but before we latched it. Re-confirm + the shard the page id currently maps to and re-latch it if it moved. */ m_hash_lock = buf_page_hash_lock_x_confirm(m_hash_lock, m_buf_pool, m_page_id); @@ -3969,7 +4004,10 @@ dberr_t Buf_fetch::zip_page_handler(buf_block_t *&fix_block) { mutex_enter(&m_buf_pool->LRU_list_mutex); - /* If not own LRU_list_mutex, page_hash can be changed. */ + /* We hold the LRU list mutex, which blocks a concurrent buffer pool + resize (buf_pool_resize() takes it), so page_hash cannot be rehashed + here: the shard latch for this page id is stable and no _confirm is + needed. */ m_hash_lock = buf_page_hash_lock_get(m_buf_pool, m_page_id); rw_lock_x_lock(m_hash_lock, UT_LOCATION_HERE); @@ -4256,12 +4294,15 @@ dberr_t Buf_fetch::debug_check(buf_block_t *fix_block) { mutex_enter(fix_mutex); if (buf_LRU_free_page(&fix_block->page, true)) { - /* If not own LRU_list_mutex, page_hash can be changed. */ + /* buf_LRU_free_page() released the page hash latch; re-acquire the + shard latch for this page id before re-checking / re-watching it. */ m_hash_lock = buf_page_hash_lock_get(m_buf_pool, m_page_id); rw_lock_x_lock(m_hash_lock, UT_LOCATION_HERE); - /* If not own LRU_list_mutex, page_hash can be changed. */ + /* We held no shard latch across the lines above, so a concurrent + buffer pool resize may have rehashed page_hash and remapped this page + id to a different shard; re-confirm and re-latch the correct shard. */ m_hash_lock = buf_page_hash_lock_x_confirm(m_hash_lock, m_buf_pool, m_page_id); @@ -4946,12 +4987,18 @@ buf_page_t *buf_page_init_for_read(ulint mode, const page_id_t &page_id, data = buf_buddy_alloc(buf_pool, page_size.physical()); } - mutex_enter(&buf_pool->LRU_list_mutex); - hash_lock = buf_page_hash_lock_get(buf_pool, page_id); + DEBUG_SYNC_C("buf_page_init_for_read_before_hash_lock"); + rw_lock_x_lock(hash_lock, UT_LOCATION_HERE); + /* We do not hold the LRU list mutex, so a concurrent buffer pool resize + may have rehashed page_hash and remapped this page id to a different shard + latch between computing hash_lock and latching it; re-confirm and re-latch + the correct shard. */ + hash_lock = buf_page_hash_lock_x_confirm(hash_lock, buf_pool, page_id); + buf_page_t *watch_page; watch_page = buf_page_hash_get_low(buf_pool, page_id); @@ -4961,8 +5008,6 @@ buf_page_t *buf_page_init_for_read(ulint mode, const page_id_t &page_id, /* The page is already in the buffer pool. */ watch_page = nullptr; - mutex_exit(&buf_pool->LRU_list_mutex); - rw_lock_x_unlock(hash_lock); if (bpage != nullptr) { @@ -4999,38 +5044,82 @@ buf_page_t *buf_page_init_for_read(ulint mode, const page_id_t &page_id, block->mark_for_read_io(); buf_page_set_io_fix(bpage, BUF_IO_READ); - /* The block must be put to the LRU list, to the old blocks */ - buf_LRU_add_block(bpage, true /* to old blocks */); - if (page_size.is_compressed()) { + /* Setting zip.data is still protected by the hash X-latch here: + the page is already in the page hash, but no other thread can look + it up until the latch is released below. */ block->page.zip.data = (page_zip_t *)data; - - /* To maintain the invariant - block->in_unzip_LRU_list - == buf_page_belongs_to_unzip_LRU(&block->page) - we have to add this block to unzip_LRU - after block->page.zip.data is set. */ - ut_ad(buf_page_belongs_to_unzip_LRU(&block->page)); - buf_unzip_LRU_add_block(block, true); } - mutex_exit(&buf_pool->LRU_list_mutex); - - /* We set a pass-type x-lock on the frame because then - the same thread which called for the read operation - (and is running now at this point of code) can wait - for the read to complete by waiting for the x-lock on - the frame; if the x-lock were recursive, the same - thread would illegally get the x-lock before the page - read is completed. The x-lock is cleared by the - io-handler thread. */ - rw_lock_x_lock_gen(&block->lock, BUF_IO_READ, UT_LOCATION_HERE); rw_lock_x_unlock(hash_lock); buf_page_mutex_exit(block); + + /* The page is hash-visible already, but eviction cannot see it + (not on LRU yet) and readers are blocked on the frame X-lock, + so no other thread can race with the add. + + IMPORTANT: we strongly depend here on the fact that there is no + other thread that can try to acquire that frame's S-lock while + holding already the LRU list mutex (it would be deadlock cycle). + For existing use cases, for that thread to exist, the page would + need to be in the LRU list already. This latching rule is documented + at the LRU_list_mutex declaration in buf0buf.h and enforced in debug + builds by rw_lock_assert_wait_allowed() at the rw-lock wait entry + points in sync0rw.cc (it cannot be expressed via latch_level_t + ordering: block->lock is SYNC_LEVEL_VARYING, which LatchDebug + ignores). */ + + /* Widen the hash-visible-not-in-LRU window: the page is already + reachable through the page hash but not yet linked into the LRU list. + Threads finding it via the page hash (buf_page_get_zip() -> + buf_block_try_discard_uncompressed(), buf_buddy_relocate(), + buf_page_make_young_if_needed()) must back off from it, because it is + io-fixed for read. */ + DBUG_EXECUTE_IF( + "buf_page_init_for_read_delay_lru_add", + std::this_thread::sleep_for(std::chrono::microseconds(100));); + + DEBUG_SYNC_C("buf_page_init_for_read_before_lru_add"); + + mutex_enter(&buf_pool->LRU_list_mutex); + + /* For a compressed page zip.data was set above, before the page + became reachable through the page hash, so + buf_page_belongs_to_unzip_LRU() already holds and buf_LRU_add_block() + links the block into the unzip_LRU list as well, within this same + critical section: every observer of the LRU list sees the invariant + block->in_unzip_LRU_list == + buf_page_belongs_to_unzip_LRU(&block->page) hold. (This is unlike the + pre-narrowing code, which set zip.data only after buf_LRU_add_block() + and therefore had to add the block to the unzip_LRU list explicitly + afterwards; an explicit second add here would corrupt the list.) */ + buf_LRU_add_block(bpage, true /* to old blocks */); + + ut_ad(!page_size.is_compressed() || block->in_unzip_LRU_list); + + mutex_exit(&buf_pool->LRU_list_mutex); } else { + /* Compressed-only page: a bare BUF_BLOCK_ZIP_PAGE descriptor with no + uncompressed frame (and thus no frame rw-lock). It is initialized and + made hash-visible while the page hash X-latch and zip_mutex (this + descriptor's "block mutex") are held, and is linked into the LRU list + afterwards under a brief LRU_list_mutex hold - the same narrowed + latching order as for the block-backed pages above. + + Setting io_fix = BUF_IO_READ before the descriptor becomes reachable + through the page hash is what makes the hash-visible-but-not-in-LRU + window safe, exactly as for block-backed pages: every path which + could move or free the page based on finding it in the page hash + backs off from a read-io-fixed page (buf_page_make_young_if_needed() + skips it, buf_page_free_stale() bails out, buf_buddy relocation and + Buf_fetch::zip_page_handler() require io_fix == BUF_IO_NONE), and + readers (e.g. buf_page_get_zip()) wait for the read to complete, + which happens-after the LRU-add below, because the read IO is only + dispatched after this function returns. */ + /* Initialize the buf_pool pointer. */ bpage->buf_pool_index = buf_pool_index(buf_pool); @@ -5061,6 +5150,8 @@ buf_page_t *buf_page_init_for_read(ulint mode, const page_id_t &page_id, ut_d(bpage->in_free_list = false); ut_d(bpage->in_LRU_list = false); + buf_page_set_io_fix(bpage, BUF_IO_READ); + ut_d(bpage->in_page_hash = true); if (watch_page != nullptr) { @@ -5081,16 +5172,31 @@ buf_page_t *buf_page_init_for_read(ulint mode, const page_id_t &page_id, rw_lock_x_unlock(hash_lock); - /* The block must be put to the LRU list, to the old blocks. - The zip size is already set into the page zip */ + mutex_exit(&buf_pool->zip_mutex); + + /* Widen the hash-visible-not-in-LRU window, as in the block-backed + branch above. */ + DBUG_EXECUTE_IF( + "buf_page_init_for_read_delay_lru_add", + std::this_thread::sleep_for(std::chrono::microseconds(100));); + + /* The page is hash-visible already (io-fixed for read, see above), + but eviction cannot see it (not on the LRU list yet), so no other + thread can race with the add. The block must be put to the LRU list, + to the old blocks. The zip size is already set into the page zip. */ + mutex_enter(&buf_pool->LRU_list_mutex); +#if defined UNIV_DEBUG || defined UNIV_BUF_DEBUG + /* buf_LRU_insert_zip_clean() requires the zip_mutex; re-acquired + here under the LRU list mutex, which follows the registered + latch_level_t order (SYNC_BUF_LRU_LIST > SYNC_BUF_BLOCK). */ + mutex_enter(&buf_pool->zip_mutex); +#endif /* UNIV_DEBUG || UNIV_BUF_DEBUG */ buf_LRU_add_block(bpage, true /* to old blocks */); #if defined UNIV_DEBUG || defined UNIV_BUF_DEBUG buf_LRU_insert_zip_clean(bpage); + mutex_exit(&buf_pool->zip_mutex); #endif /* UNIV_DEBUG || UNIV_BUF_DEBUG */ mutex_exit(&buf_pool->LRU_list_mutex); - buf_page_set_io_fix(bpage, BUF_IO_READ); - - mutex_exit(&buf_pool->zip_mutex); } buf_pool->n_pend_reads.fetch_add(1); @@ -5188,16 +5294,25 @@ buf_block_t *buf_page_create(const page_id_t &page_id, /* Latch the page before releasing hash lock so that concurrent request for this page doesn't see half initialized page. ALTER tablespace for encryption and clone page copy can request page for any page id within tablespace - size limit. */ + size limit. + + The nowait variants must be used and cannot fail: the frame comes from + the free list, so its latch is unlocked, and the block is unreachable by + other threads until the page hash X-latch is released below. This keeps + the LRU_list_mutex latching rule (no waiting for a frame latch under the + LRU list mutex, see the LRU_list_mutex declaration) free of blocking + acquisitions - we hold the LRU list mutex here. */ mtr_memo_type_t mtr_latch_type; + bool latched [[maybe_unused]]; if (rw_latch == RW_X_LATCH) { - rw_lock_x_lock(&block->lock, UT_LOCATION_HERE); + latched = rw_lock_x_lock_nowait(&block->lock, UT_LOCATION_HERE); mtr_latch_type = MTR_MEMO_PAGE_X_FIX; } else { - rw_lock_sx_lock(&block->lock, UT_LOCATION_HERE); + latched = rw_lock_sx_lock_nowait(&block->lock, 0, UT_LOCATION_HERE); mtr_latch_type = MTR_MEMO_PAGE_SX_FIX; } + ut_ad(latched); mtr_memo_push(mtr, block, mtr_latch_type); rw_lock_x_unlock(hash_lock); @@ -5746,6 +5861,30 @@ void buf_page_t::set_io_fix(buf_io_fix io_fix) { take_io_responsibility(); } Latching_rules_helpers::on_transition_to(*this, io_fix); + + if (io_fix == BUF_IO_READ) { + /* BUF_IO_READ may only be stored on a page that is not yet reachable + through the page hash: either it is not in the page hash at all, or + the storing thread still holds the hash cell's X-latch. Threads which + find a page through a page hash lookup therefore can never observe a + pre-read BUF_IO_NONE, which is what allows + buf_page_make_young_if_needed() to test was_io_fix_read() without the + block mutex to detect a page whose LRU-add is still pending. + (This cannot be expressed in buf_io_fix_latching_rules: its latch set + does not include the page hash latches.) */ + ut_ad(!in_page_hash || + buf_page_hash_lock_held_x(buf_pool_from_bpage(this), this)); + } + + if (old_io_fix == BUF_IO_READ && io_fix == BUF_IO_NONE) { + /* The read IO is dispatched only after buf_page_init_for_read() has + linked the page into the LRU list, so by the time the read io-fix is + cleared the page must be in the LRU list. + buf_page_make_young_if_needed() relies on this: observing + io_fix != BUF_IO_READ implies the LRU-add has completed and the page + may be promoted. */ + ut_ad(in_LRU_list); + } #endif this->io_fix.store(io_fix, std::memory_order_relaxed); #ifdef UNIV_DEBUG @@ -6113,6 +6252,7 @@ static void buf_pool_validate_instance(buf_pool_t *buf_pool) { ulint n_flush = 0; ulint n_free = 0; ulint n_zip = 0; + ulint n_lru_add_pending = 0; ut_ad(buf_pool); @@ -6161,7 +6301,26 @@ static void buf_pool_validate_instance(buf_pool_t *buf_pool) { } } +#ifdef UNIV_DEBUG + if (!block->page.in_LRU_list) { + /* buf_page_init_for_read() makes the page hash-visible before + linking it into the LRU list. Such a page is still io-fixed for + read. Reading in_LRU_list is stable here: it is only modified + under LRU_list_mutex, which we hold. */ + ut_a(block->page.was_io_fix_read()); + n_lru_add_pending++; + } else { + n_lru++; + } +#else /* UNIV_DEBUG */ + /* Without UNIV_DEBUG there is no in_LRU_list flag; count how many + FILE_PAGE blocks may legitimately be missing from the LRU list so + the length cross-check below can be relaxed by that amount. */ + if (block->page.was_io_fix_read()) { + n_lru_add_pending++; + } n_lru++; +#endif /* UNIV_DEBUG */ break; case BUF_BLOCK_NOT_USED: @@ -6187,10 +6346,10 @@ static void buf_pool_validate_instance(buf_pool_t *buf_pool) { /* All clean blocks should be I/O-unfixed. */ break; case BUF_IO_READ: - /* In buf_LRU_free_page(), we temporarily set - b->io_fix = BUF_IO_READ for a newly allocated - control block in order to prevent - buf_page_get_gen() from decompressing the block. */ + /* A clean compressed-only page can be io-fixed for read only + while its initial read from disk is pending. (buf_LRU_free_page() + pins the re-inserted compressed-only descriptor with + buf_page_set_sticky(), which is BUF_IO_PIN, not BUF_IO_READ.) */ break; default: ut_error; @@ -6262,7 +6421,15 @@ static void buf_pool_validate_instance(buf_pool_t *buf_pool) { << buf_pool->curr_size << " zip " << n_zip << ". Aborting..."; } +#ifdef UNIV_DEBUG + /* Pages whose read IO is in progress and which are not yet linked into + the LRU list were counted into n_lru_add_pending instead of n_lru. */ + (void)n_lru_add_pending; ut_a(UT_LIST_GET_LEN(buf_pool->LRU) == n_lru); +#else /* UNIV_DEBUG */ + ut_a(UT_LIST_GET_LEN(buf_pool->LRU) <= n_lru); + ut_a(n_lru <= UT_LIST_GET_LEN(buf_pool->LRU) + n_lru_add_pending); +#endif /* UNIV_DEBUG */ mutex_exit(&buf_pool->LRU_list_mutex); mutex_exit(&buf_pool->chunks_mutex); diff --git a/storage/innobase/buf/buf0lru.cc b/storage/innobase/buf/buf0lru.cc index ddd37e2d362b..f97cceb16954 100644 --- a/storage/innobase/buf/buf0lru.cc +++ b/storage/innobase/buf/buf0lru.cc @@ -146,13 +146,20 @@ If a compressed page is freed other compressed pages may be relocated. compressed page of an uncompressed page @param[in] ignore_content true if should ignore page content, since it could be not initialized +@param[in] keep_hash_lock true if the hash cell X-latch should be kept + by this function instead of being released; + only allowed when the caller will re-insert + a compressed-only descriptor for this page + id into the page hash (the keep-zip path of + buf_LRU_free_page()), so that the page id is + never observably absent from the page hash @retval true if BUF_BLOCK_FILE_PAGE was removed from page_hash. The caller needs to free the page to the free list @retval false if BUF_BLOCK_ZIP_PAGE was removed from page_hash. In this case the block is already returned to the buddy allocator. */ -[[nodiscard]] static bool buf_LRU_block_remove_hashed(buf_page_t *bpage, - bool zip, - bool ignore_content); +[[nodiscard]] static bool buf_LRU_block_remove_hashed( + buf_page_t *bpage, bool zip, bool ignore_content, + bool keep_hash_lock = false); /** Puts a file page whose has no hash index to the free list. @param[in,out] block Must contain a file page and be in a state @@ -958,16 +965,22 @@ bool buf_LRU_free_page(buf_page_t *bpage, bool zip) { auto block_mutex = buf_page_get_mutex(bpage); auto hash_lock = buf_page_hash_lock_get(buf_pool, bpage->id); - ut_ad(bpage->in_LRU_list); ut_ad(mutex_own(&buf_pool->LRU_list_mutex)); ut_ad(mutex_own(block_mutex)); - ut_ad(buf_page_in_file(bpage)); if (!buf_page_can_relocate(bpage)) { /* Do not free buffer fixed and I/O-fixed blocks. */ return (false); } + /* These assertions can only be checked after the io-fix check + above, because pages become visible in the page hash table before + they are linked into the LRU list (they are io-fixed for read + until after they are linked into the LRU list, so + buf_page_can_relocate() has returned false for them). */ + ut_ad(bpage->in_LRU_list); + ut_ad(buf_page_in_file(bpage)); + #ifdef UNIV_IBUF_COUNT_DEBUG ut_a(ibuf_count_get(bpage->id) == 0); #endif /* UNIV_IBUF_COUNT_DEBUG */ @@ -988,10 +1001,17 @@ bool buf_LRU_free_page(buf_page_t *bpage, bool zip) { return (false); } else if (buf_page_get_state(bpage) == BUF_BLOCK_FILE_PAGE) { + /* This holds because of the "else" keyword above, + but we assert it for clarity. */ + ut_ad(!zip && bpage->zip.data != nullptr); b = buf_page_alloc_descriptor(); ut_a(b); } + /* Protect: buf_buddy_free must not be invoked in buf_LRU_block_remove_hashed, + when passing keep_hash_lock = true (b != nullptr). */ + ut_ad(b == nullptr || (!zip && bpage->zip.data != nullptr)); + ut_ad(buf_page_in_file(bpage)); ut_ad(bpage->in_LRU_list); ut_ad(bpage->in_flush_list == is_dirty); @@ -1037,7 +1057,20 @@ bool buf_LRU_free_page(buf_page_t *bpage, bool zip) { ut_ad(rw_lock_own(hash_lock, RW_LOCK_X)); ut_ad(buf_page_can_relocate(bpage)); - if (!buf_LRU_block_remove_hashed(bpage, zip, false)) { + /* When the compressed page is to be kept (b != nullptr), ask + buf_LRU_block_remove_hashed() to keep the hash cell X-latch, so that it + is held continuously from before the HASH_DELETE until after the + compressed-only descriptor is re-inserted below: the page id is never + observably absent from the page hash. Otherwise a concurrent + buf_page_init_for_read() (which inserts into the page hash without + holding the LRU list mutex) could insert a second descriptor for this + page id in the meantime. + + Protect: buf_buddy_free must not be invoked in buf_LRU_block_remove_hashed, + when passing keep_hash_lock = true (b != nullptr). */ + ut_ad(b == nullptr || (!zip && bpage->zip.data != nullptr)); + + if (!buf_LRU_block_remove_hashed(bpage, zip, false, b != nullptr)) { mutex_exit(&buf_pool->LRU_list_mutex); if (b != nullptr) { @@ -1047,9 +1080,11 @@ bool buf_LRU_free_page(buf_page_t *bpage, bool zip) { } ut_ad(!mutex_own(block_mutex)); - /* buf_LRU_block_remove_hashed() releases the hash_lock */ - ut_ad(!rw_lock_own(hash_lock, RW_LOCK_X) && - !rw_lock_own(hash_lock, RW_LOCK_S)); + /* buf_LRU_block_remove_hashed() releases the hash_lock, unless it was + asked to keep it for the re-insert of the compressed-only descriptor. */ + ut_ad(b != nullptr ? rw_lock_own(hash_lock, RW_LOCK_X) + : (!rw_lock_own(hash_lock, RW_LOCK_X) && + !rw_lock_own(hash_lock, RW_LOCK_S))); /* We have just freed a BUF_BLOCK_FILE_PAGE. If b != nullptr then it was a compressed page with an uncompressed frame and @@ -1058,10 +1093,24 @@ bool buf_LRU_free_page(buf_page_t *bpage, bool zip) { into the LRU and page_hash (and possibly flush_list). if b == nullptr then it was a regular page that has been freed */ + /* Widen the window between the page hash delete and the re-insert of + the compressed-only descriptor. The hash cell X-latch is held here + (keep_hash_lock), so a concurrent buf_page_init_for_read() of this page + must block on the cell latch instead of inserting a second descriptor + for the same page id into the gap. */ + DBUG_EXECUTE_IF( + "buf_lru_free_page_delay_zip_reinsert", if (b != nullptr) { + std::this_thread::sleep_for(std::chrono::microseconds(100)); + }); + if (b != nullptr) { auto prev_b = UT_LIST_GET_PREV(LRU, b); - rw_lock_x_lock(hash_lock, UT_LOCATION_HERE); + /* The hash cell X-latch has been held continuously since before the + HASH_DELETE in buf_LRU_block_remove_hashed() (see keep_hash_lock), + which is what makes the assertion below provable: no other thread can + have inserted a descriptor for this page id in the meantime. */ + ut_ad(rw_lock_own(hash_lock, RW_LOCK_X)); mutex_enter(block_mutex); @@ -1279,7 +1328,8 @@ the object will be freed. The caller must hold buf_pool->LRU_list_mutex, the buf_page_get_mutex() mutex and the appropriate hash_lock. This function will release the -buf_page_get_mutex() and the hash_lock. +buf_page_get_mutex() and the hash_lock (the latter is kept if keep_hash_lock +is passed). If a compressed page is freed other compressed pages may be relocated. @@ -1290,12 +1340,20 @@ If a compressed page is freed other compressed pages may be relocated. compressed page of an uncompressed page @param[in] ignore_content true if should ignore page content, since it could be not initialized +@param[in] keep_hash_lock true if the hash cell X-latch should be kept + by this function instead of being released; + only allowed when the caller will re-insert + a compressed-only descriptor for this page + id into the page hash (the keep-zip path of + buf_LRU_free_page()), so that the page id is + never observably absent from the page hash @retval true if BUF_BLOCK_FILE_PAGE was removed from page_hash. The caller needs to free the page to the free list @retval false if BUF_BLOCK_ZIP_PAGE was removed from page_hash. In this case the block is already returned to the buddy allocator. */ static bool buf_LRU_block_remove_hashed(buf_page_t *bpage, bool zip, - bool ignore_content) { + bool ignore_content, + bool keep_hash_lock) { const buf_page_t *hashed_bpage; buf_pool_t *buf_pool = buf_pool_from_bpage(bpage); rw_lock_t *hash_lock; @@ -1303,6 +1361,16 @@ static bool buf_LRU_block_remove_hashed(buf_page_t *bpage, bool zip, ut_ad(mutex_own(&buf_pool->LRU_list_mutex)); ut_ad(mutex_own(buf_page_get_mutex(bpage))); + /* keep_hash_lock is only supported for the keep-zip path of + buf_LRU_free_page(): an uncompressed frame being freed while its + compressed page is kept and will be re-inserted into the page hash. + In particular the buddy allocator must not be invoked while the hash + cell X-latch is kept (buf_buddy_free() may take page hash latches via + buf_buddy_relocate()), which is guaranteed by zip == false. */ + ut_ad(!keep_hash_lock || + (!zip && buf_page_get_state(bpage) == BUF_BLOCK_FILE_PAGE && + bpage->zip.data != nullptr)); + hash_lock = buf_page_hash_lock_get(buf_pool, bpage->id); ut_ad(rw_lock_own(hash_lock, RW_LOCK_X)); @@ -1482,14 +1550,24 @@ static bool buf_LRU_block_remove_hashed(buf_page_t *bpage, bool zip, avoid relocation during the scan. But that is not possible because we are holding LRU list mutex. - 2) Not possible because in buf_page_init_for_read() - we do a look up of page_hash while holding LRU list - mutex and since we are holding LRU list mutex here - and by the time we'll release it in the caller we'd - have inserted the compressed only descriptor in the - page_hash. */ + 2) When a compressed-only descriptor will be re-inserted + for this page id (the keep-zip path of buf_LRU_free_page(), + keep_hash_lock == true), the hash cell X-latch is kept from + before the HASH_DELETE above until after the re-insert in the + caller, so the page id is never observably absent from the + page hash: a concurrent buf_page_init_for_read() blocks on + the hash cell latch and then finds the compressed-only + descriptor. (It cannot be the LRU list mutex which protects + this transition: buf_page_init_for_read() inserts into the + page hash without holding the LRU list mutex.) + When nothing will be re-inserted (keep_hash_lock == false), + the page is leaving the buffer pool for good and a concurrent + thread reading it from the disk afresh is the normal cache + miss path. */ ut_ad(mutex_own(&buf_pool->LRU_list_mutex)); - rw_lock_x_unlock(hash_lock); + if (!keep_hash_lock) { + rw_lock_x_unlock(hash_lock); + } mutex_exit(&((buf_block_t *)bpage)->mutex); if (zip && bpage->zip.data) { diff --git a/storage/innobase/buf/buf0rea.cc b/storage/innobase/buf/buf0rea.cc index b90fb7135f7e..d6bfb965e42f 100644 --- a/storage/innobase/buf/buf0rea.cc +++ b/storage/innobase/buf/buf0rea.cc @@ -88,10 +88,18 @@ ulint buf_read_page_low(dberr_t *err, bool sync, IORequest::Type type, sync = true; } - /* The following call will also check if the tablespace does not exist - or is being dropped; if we succeed in initializing the page in the buffer - pool for read, then DISCARD cannot proceed until the read has - completed */ + /* buf_page_init_for_read() makes the page hash-visible, io-fixed for + read and linked into the LRU list before we dispatch the read IO below. + This does not stop a concurrent tablespace drop or truncation: + tablespace deletion does not scan the buffer pool; it bumps the space + version and relies on the pages becoming stale. If the tablespace no + longer exists, or is being deleted and the read is asynchronous, + fil_io() refuses the read (DB_TABLESPACE_DELETED, see Fil_shard::do_io()) + and the page is removed by buf_read_page_handle_error() below. Otherwise + the read completes and the page is later detected as stale + (buf_page_t::was_stale()) and freed lazily; buf_page_free_stale() skips + pages which are still io-fixed, so it frees the page only after the read + has completed. */ bpage = buf_page_init_for_read(mode, page_id, page_size, unzip); ut_a(bpage == nullptr || bpage->get_space()->id == page_id.space()); diff --git a/storage/innobase/include/buf0buf.h b/storage/innobase/include/buf0buf.h index 42586078695e..3c9f082e4732 100644 --- a/storage/innobase/include/buf0buf.h +++ b/storage/innobase/include/buf0buf.h @@ -2296,7 +2296,24 @@ struct buf_pool_t { for all buf_pool_t-s */ BufListMutex chunks_mutex; - /** LRU list mutex */ + /** LRU list mutex. + Latching rule: no thread may WAIT for a block's frame rw-lock + (block->lock) while holding this mutex. buf_page_init_for_read() + acquires this mutex while holding the X-latch on the frame of the page + being read in, so waiting for a frame latch under this mutex would + create a deadlock cycle with that path. Consequently, a frame latch may + be taken under this mutex only with the rw_lock_*_nowait() variants: + flushing does so and handles the failure, and buf_page_create() does so + on a frame taken from the free list, asserting success (its latch is + unlocked and the block is unreachable by other threads while the page + hash X-latch is still held, so the attempt cannot fail). Compressed-only + pages (BUF_BLOCK_ZIP_PAGE descriptors) have no frame and no frame + rw-lock, so the paths handling them add no edge to this rule. + This rule cannot be expressed via latch_level_t ordering, because + block->lock is registered with SYNC_LEVEL_VARYING which LatchDebug + ignores; instead it is enforced in debug builds (with + --innodb-sync-debug) by rw_lock_assert_wait_allowed() at the rw-lock + wait entry points in sync0rw.cc. */ BufListMutex LRU_list_mutex; /** free and withdraw list mutex */ @@ -2353,7 +2370,17 @@ struct buf_pool_t { /** Hash table of buf_page_t or buf_block_t file pages, buf_page_in_file() == true, indexed by (space_id, offset). page_hash is protected by an array of - mutexes. */ + mutexes. + Membership-change protocol: a descriptor for a page id may be inserted + only after verifying the id's absence, with the cell's X-latch held + continuously from that verification until the insert. Conversely, a + remover which will re-insert a descriptor for the same page id (the + keep-zip path of buf_LRU_free_page()) must keep the cell's X-latch held + continuously from the delete until the re-insert, so that the page id is + never observably absent from the hash while the page is still logically + in the buffer pool. Note that buf_page_init_for_read() inserts while + holding only the cell's X-latch (not the LRU list mutex), so the LRU + list mutex does NOT stabilize page hash membership. */ hash_table_t *page_hash; /** Hash table of buf_block_t blocks whose frames are allocated to the zip diff --git a/storage/innobase/include/buf0buf.ic b/storage/innobase/include/buf0buf.ic index e1d8f851845e..7180cfab3511 100644 --- a/storage/innobase/include/buf0buf.ic +++ b/storage/innobase/include/buf0buf.ic @@ -505,7 +505,12 @@ static inline bool buf_page_can_relocate( { ut_ad(mutex_own(buf_page_get_mutex(bpage))); ut_ad(buf_page_in_file(bpage)); - ut_ad(bpage->in_LRU_list); + /* A page whose read IO is still in progress may be hash-visible but not + yet linked into the LRU list: buf_page_init_for_read() links it into the + LRU list only after making it hash-visible. Such a page is io-fixed for + read (and not relocatable), which is the only legitimate way to reach + this function for a page which is not in the LRU list. */ + ut_ad(bpage->in_LRU_list || buf_page_get_io_fix(bpage) == BUF_IO_READ); return (buf_page_get_io_fix(bpage) == BUF_IO_NONE && bpage->buf_fix_count == 0); diff --git a/storage/innobase/sync/sync0debug.cc b/storage/innobase/sync/sync0debug.cc index 5ec661fde27c..8d9e6aa52113 100644 --- a/storage/innobase/sync/sync0debug.cc +++ b/storage/innobase/sync/sync0debug.cc @@ -38,6 +38,7 @@ this program; if not, write to the Free Software Foundation, Inc., *******************************************************/ #include "sync0debug.h" +#include "univ.i" #include #include @@ -683,7 +684,12 @@ const latch_t *LatchDebug::find(const Latches *latches, @param[in] level The level to lookup @return latch if found or NULL */ const latch_t *LatchDebug::find(latch_level_t level) UNIV_NOTHROW { - return (find(thread_latches(), level)); + /* A thread which has not acquired any latch yet has no latch list + allocated (thread_latches() without the create flag returns nullptr) + and thus holds nothing at any level. */ + const Latches *latches = thread_latches(); + + return (latches != nullptr ? find(latches, level) : nullptr); } /** @@ -1267,7 +1273,9 @@ static void sync_latch_meta_init() UNIV_NOTHROW { LATCH_ADD_MUTEX(FTS_PLL_TOKENIZE, SYNC_FTS_TOKENIZE, fts_pll_tokenize_mutex_key); - LATCH_ADD_MUTEX(HASH_TABLE_MUTEX, SYNC_BUF_PAGE_HASH, hash_table_mutex_key); + /* Do not use SYNC_BUF_PAGE_HASH for HASH_TABLE_MUTEX. + @see buf_buddy_no_page_hash_latch_validate(). */ + LATCH_ADD_MUTEX(HASH_TABLE_MUTEX, SYNC_ANY_LATCH, hash_table_mutex_key); LATCH_ADD_MUTEX(IBUF_BITMAP, SYNC_IBUF_BITMAP_MUTEX, ibuf_bitmap_mutex_key); diff --git a/storage/innobase/sync/sync0rw.cc b/storage/innobase/sync/sync0rw.cc index 8d28cf845ab8..d5a610c41d63 100644 --- a/storage/innobase/sync/sync0rw.cc +++ b/storage/innobase/sync/sync0rw.cc @@ -270,6 +270,42 @@ rw_lock_t::~rw_lock_t() { ut_d(magic_n = 0); } +#ifdef UNIV_DEBUG +/** Asserts that the calling thread is allowed to start waiting for the +given rw-lock. + +This is the hook for latch-order rules which cannot be expressed through +latch_level_t ordering and which constrain only actual waits, not +non-blocking acquisitions (the rw_lock_*_nowait() variants never reach +this, by design: an acquisition that never waits cannot participate in a +deadlock cycle). It is called at the only spots where an rw-lock +acquisition starts to wait - every unbounded wait, spinning included, +funnels into a sync array reservation. + +Currently there is one such rule, for the buffer block frame locks +(buf_block_t::lock): a thread must not wait for a frame lock while +holding any buffer pool LRU list mutex. buf_page_init_for_read() acquires +buf_pool_t::LRU_list_mutex while holding the X-latch on the frame of the +page being read in, so such a wait would form a deadlock cycle with that +path. The rule cannot use latch levels because buf_block_t::lock is +registered with SYNC_LEVEL_VARYING (B-tree page latching order genuinely +varies), and LatchDebug ignores such latches entirely - both when they +are acquired and when they are held. The code paths which do latch a +frame under the LRU list mutex (flushing, and buf_page_create() on a +block freshly taken from the free list) use the nowait variants, so they +are exempt by construction. + +Note that this check is only effective when LatchDebug is enabled +(--innodb-sync-debug); an actual deadlock occurrence is additionally caught +by the sync array deadlock detector, which does track frame rw-locks. */ +namespace { +void rw_lock_assert_wait_allowed(const rw_lock_t *lock) { + ut_ad(lock->get_id() != LATCH_ID_BUF_BLOCK_LOCK || + sync_check_find(SYNC_BUF_LRU_LIST) == nullptr); +} +} // namespace +#endif /* UNIV_DEBUG */ + void rw_lock_s_lock_spin(rw_lock_t *lock, ulint pass, ut::Location location) { ulint i = 0; /* spin round count */ sync_array_t *sync_arr; @@ -310,6 +346,8 @@ void rw_lock_s_lock_spin(rw_lock_t *lock, ulint pass, ut::Location location) { ++count_os_wait; + ut_d(rw_lock_assert_wait_allowed(lock)); + sync_cell_t *cell; sync_arr = @@ -392,6 +430,8 @@ static inline void rw_lock_x_lock_wait_func(rw_lock_t *lock, } /* If there is still a reader, then go to sleep.*/ + ut_d(rw_lock_assert_wait_allowed(lock)); + sync_cell_t *cell; sync_arr = sync_array_get_and_reserve_cell(lock, RW_LOCK_X_WAIT, @@ -612,6 +652,8 @@ void rw_lock_x_lock_func(rw_lock_t *lock, ulint pass, ut::Location location) { } } + ut_d(rw_lock_assert_wait_allowed(lock)); + sync_cell_t *cell; sync_arr = sync_array_get_and_reserve_cell(lock, RW_LOCK_X, location, &cell); @@ -677,6 +719,8 @@ void rw_lock_sx_lock_func(rw_lock_t *lock, ulint pass, ut::Location location) { } } + ut_d(rw_lock_assert_wait_allowed(lock)); + sync_cell_t *cell; sync_arr = sync_array_get_and_reserve_cell(lock, RW_LOCK_SX, location, &cell); From 142cebb718c534fb1933c48e349cfc70c7143d50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Olchawa?= Date: Tue, 21 Jul 2026 14:56:19 +0200 Subject: [PATCH 2/2] Fix inverted old/young check in LRUItr::start() that always restarted the LRU scan LRUItr::start() is meant to leave the scan hand pointer (m_hp) in place while it is still within the "old" (cold) sublist of the LRU list, and only rewind it to the tail of the LRU list once the pointer has advanced past the old/young boundary into the young (hot) sublist. The boundary check was inverted: `m_hp->old` is true while the pointer is still in the old region, so the scan was rewound on every call instead of only at the old -> young transition. This defeated the intended optimization by repeatedly restarting the scan from the tail rather than letting it continue. Fix the condition to `!m_hp->old`, so the pointer is only reset once it has crossed into the young sublist. --- storage/innobase/buf/buf0buf.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/innobase/buf/buf0buf.cc b/storage/innobase/buf/buf0buf.cc index a12ea58e2194..0c10b5af9556 100644 --- a/storage/innobase/buf/buf0buf.cc +++ b/storage/innobase/buf/buf0buf.cc @@ -2964,7 +2964,7 @@ the LRU list it resets the value to the tail of the LRU list. buf_page_t *LRUItr::start() { ut_ad(mutex_own(m_mutex)); - if (!m_hp || m_hp->old) { + if (!m_hp || !m_hp->old) { m_hp = UT_LIST_GET_LAST(m_buf_pool->LRU); }