Skip to content

Commit

Permalink
MDEV-22877 Avoid unnecessary buf_pool.page_hash S-latch acquisition
Browse files Browse the repository at this point in the history
MDEV-15053 did not remove all unnecessary buf_pool.page_hash S-latch
acquisition. There are code paths where we are holding buf_pool.mutex
(which will sufficiently protect buf_pool.page_hash against changes)
and unnecessarily acquire the latch. Many invocations of
buf_page_hash_get_locked() can be replaced with the much simpler
buf_pool.page_hash_get_low().

In the worst case the thread that is holding buf_pool.mutex will become
a victim of MDEV-22871, suffering from a spurious reader-reader conflict
with another thread that genuinely needs to acquire a buf_pool.page_hash
S-latch.

In many places, we were also evaluating page_id_t::fold() while holding
buf_pool.mutex. Low-level functions such as buf_pool.page_hash_get_low()
must get the page_id_t::fold() as a parameter.

buf_buddy_relocate(): Defer the hash_lock acquisition to the critical
section that starts by calling buf_page_t::can_relocate().
  • Loading branch information
dr-m committed Jun 12, 2020
1 parent 0b5dc62 commit d2c593c
Show file tree
Hide file tree
Showing 11 changed files with 292 additions and 485 deletions.
4 changes: 3 additions & 1 deletion storage/innobase/btr/btr0cur.cc
Expand Up @@ -7023,9 +7023,11 @@ static void btr_blob_free(buf_block_t *block, bool all, mtr_t *mtr)
ut_ad(mtr->memo_contains_flagged(block, MTR_MEMO_PAGE_X_FIX));
mtr->commit();

const ulint fold= page_id.fold();

mutex_enter(&buf_pool.mutex);

if (buf_page_t *bpage= buf_pool.page_hash_get_low(page_id))
if (buf_page_t *bpage= buf_pool.page_hash_get_low(page_id, fold))
if(!buf_LRU_free_page(bpage, all) && all && bpage->zip.data)
/* Attempt to deallocate the redundant copy of the uncompressed page
if the whole ROW_FORMAT=COMPRESSED block cannot be deallocted. */
Expand Down
36 changes: 14 additions & 22 deletions storage/innobase/btr/btr0sea.cc
Expand Up @@ -2074,7 +2074,6 @@ btr_search_hash_table_validate(ulint hash_table_id)
for (; node != NULL; node = node->next) {
const buf_block_t* block
= buf_pool.block_from_ahi((byte*) node->data);
const buf_block_t* hash_block;
index_id_t page_index_id;

if (UNIV_LIKELY(block->page.state()
Expand All @@ -2085,29 +2084,22 @@ btr_search_hash_table_validate(ulint hash_table_id)
the block is being freed
(BUF_BLOCK_REMOVE_HASH, see the
assertion and the comment below) */
hash_block = buf_block_hash_get(
block->page.id());
} else {
hash_block = NULL;
}

if (hash_block) {
ut_a(hash_block == block);
} else {
/* When a block is being freed,
buf_LRU_search_and_free_block() first
removes the block from
buf_pool.page_hash by calling
buf_LRU_block_remove_hashed_page().
After that, it invokes
btr_search_drop_page_hash_index() to
remove the block from
btr_search_sys->hash_tables[i]. */

ut_a(block->page.state()
== BUF_BLOCK_REMOVE_HASH);
const page_id_t id(block->page.id());
if (const buf_page_t* hash_page
= buf_pool.page_hash_get_low(
id, id.fold())) {
ut_ad(hash_page == &block->page);
goto state_ok;
}
}

/* When a block is being freed,
buf_LRU_search_and_free_block() first removes
the block from buf_pool.page_hash by calling
buf_LRU_block_remove_hashed_page(). Then it
invokes btr_search_drop_page_hash_index(). */
ut_a(block->page.state() == BUF_BLOCK_REMOVE_HASH);
state_ok:
ut_ad(!dict_index_is_ibuf(block->index));
ut_ad(block->page.id().space()
== block->index->table->space_id);
Expand Down
21 changes: 9 additions & 12 deletions storage/innobase/buf/buf0buddy.cc
Expand Up @@ -508,21 +508,16 @@ static bool buf_buddy_relocate(void* src, void* dst, ulint i, bool force)
ut_ad(space != BUF_BUDDY_STAMP_FREE);

const page_id_t page_id(space, offset);
const ulint fold= page_id.fold();

rw_lock_t* hash_lock = buf_pool.hash_lock_get(page_id);

rw_lock_x_lock(hash_lock);

bpage = buf_pool.page_hash_get_low(page_id);
bpage = buf_pool.page_hash_get_low(page_id, fold);

if (!bpage || bpage->zip.data != src) {
/* The block has probably been freshly
allocated by buf_LRU_get_free_block() but not
added to buf_pool.page_hash yet. Obviously,
it cannot be relocated. */

rw_lock_x_unlock(hash_lock);

if (!force || space != 0 || offset != 0) {
return(false);
}
Expand All @@ -534,8 +529,6 @@ static bool buf_buddy_relocate(void* src, void* dst, ulint i, bool force)
while (bpage != NULL) {
if (bpage->zip.data == src) {
ut_ad(bpage->id() == page_id);
hash_lock = buf_pool.hash_lock_get(page_id);
rw_lock_x_lock(hash_lock);
break;
}
bpage = UT_LIST_GET_NEXT(LRU, bpage);
Expand All @@ -551,16 +544,20 @@ static bool buf_buddy_relocate(void* src, void* dst, ulint i, bool force)
have to relocate all blocks covered by src.
For the sake of simplicity, give up. */
ut_ad(page_zip_get_size(&bpage->zip) < size);

rw_lock_x_unlock(hash_lock);

return(false);
}

/* The block must have been allocated, but it may
contain uninitialized data. */
UNIV_MEM_ASSERT_W(src, size);

if (!bpage->can_relocate()) {
return false;
}

rw_lock_t * hash_lock = buf_pool.hash_lock_get_low(fold);
rw_lock_x_lock(hash_lock);

if (bpage->can_relocate()) {
/* Relocate the compressed page. */
const ulonglong ns = my_interval_timer();
Expand Down

0 comments on commit d2c593c

Please sign in to comment.