Skip to content
Permalink
Browse files
MDEV-23693 Failing assertion: my_atomic_load32_explicit(&lock->lock_w…
…ord, MY_MEMORY_ORDER_RELAXED) == X_LOCK_DECR

InnoDB frees the block lock during buffer pool shrinking when other
thread is yet to release the block lock.  While shrinking the
buffer pool, InnoDB allows the page to be freed unless it is buffer
fixed. In some cases, InnoDB releases the latch after unfixing the
block.

Fix:
====
- InnoDB should unfix the block after releases the latch.

- Add more assertion to check buffer fix while accessing the page.

- Introduced block_hint structure to store buf_block_t pointer
and allow accessing the buf_block_t pointer only by passing a
functor. It returns original buf_block_t* pointer if it is valid
or nullptr if the pointer become stale.

- Replace buf_block_is_uncompressed() with
buf_pool_t::is_block_pointer()

This change is motivated by a change in mysql-5.7.32:
mysql/mysql-server@46e60de
Bug #31036301 ASSERTION FAILURE: SYNC0RW.IC:429:LOCK->LOCK_WORD
  • Loading branch information
Thirunarayanan committed Oct 27, 2020
1 parent 6a614d6 commit bc540b8
Show file tree
Hide file tree
Showing 16 changed files with 262 additions and 165 deletions.
@@ -28,6 +28,7 @@ SET(INNOBASE_SOURCES
btr/btr0scrub.cc
btr/btr0sea.cc
btr/btr0defragment.cc
buf/buf0block_hint.cc
buf/buf0buddy.cc
buf/buf0buf.cc
buf/buf0dblwr.cc
@@ -695,6 +695,8 @@ PageBulk::latch()
m_mtr.set_named_space(m_index->space);
}

ut_ad(m_block->page.buf_fix_count);

/* In case the block is S-latched by page_cleaner. */
if (!buf_page_optimistic_get(RW_X_LATCH, m_block, m_modify_clock,
__FILE__, __LINE__, &m_mtr)) {
@@ -713,6 +715,8 @@ PageBulk::latch()

buf_block_buf_fix_dec(m_block);

ut_ad(m_block->page.buf_fix_count);

ut_ad(m_cur_rec > m_page && m_cur_rec < m_heap_top);

return (m_err);
@@ -416,6 +416,8 @@ btr_cur_optimistic_latch_leaves(
ulint mode;
ulint left_page_no;
ulint curr_page_no;
ut_ad(block->page.buf_fix_count);
ut_ad(buf_block_get_state(block) == BUF_BLOCK_FILE_PAGE);

switch (*latch_mode) {
case BTR_SEARCH_LEAF:
@@ -427,20 +429,10 @@ btr_cur_optimistic_latch_leaves(
mode = *latch_mode == BTR_SEARCH_PREV
? RW_S_LATCH : RW_X_LATCH;

buf_page_mutex_enter(block);
if (buf_block_get_state(block) != BUF_BLOCK_FILE_PAGE) {
buf_page_mutex_exit(block);
return(false);
}
/* pin the block not to be relocated */
buf_block_buf_fix_inc(block, file, line);
buf_page_mutex_exit(block);

rw_lock_s_lock(&block->lock);
if (block->modify_clock != modify_clock) {
rw_lock_s_unlock(&block->lock);

goto unpin_failed;
return false;
}

curr_page_no = block->page.id.page_no();
@@ -470,7 +462,7 @@ btr_cur_optimistic_latch_leaves(
/* release the left block */
btr_leaf_page_release(
cursor->left_block, mode, mtr);
goto unpin_failed;
return false;
}
} else {
cursor->left_block = NULL;
@@ -480,23 +472,28 @@ btr_cur_optimistic_latch_leaves(
file, line, mtr)) {
if (btr_page_get_prev(buf_block_get_frame(block))
== left_page_no) {
buf_block_buf_fix_dec(block);
/* block was already buffer-fixed while
entering the function and
buf_page_optimistic_get() buffer-fixes
it again. */
ut_ad(2 <= block->page.buf_fix_count);
*latch_mode = mode;
return(true);
} else {
/* release the block */
/* release the block and decrement of
buf_fix_count which was incremented
in buf_page_optimistic_get() */
btr_leaf_page_release(block, mode, mtr);
}
}

ut_ad(block->page.buf_fix_count);
/* release the left block */
if (cursor->left_block != NULL) {
btr_leaf_page_release(cursor->left_block,
mode, mtr);
}
unpin_failed:
/* unpin the block */
buf_block_buf_fix_dec(block);

return(false);

default:
@@ -1066,12 +1063,7 @@ btr_cur_search_to_nth_level_func(
guess = NULL;
#else
info = btr_search_get_info(index);

if (!buf_pool_is_obsolete(info->withdraw_clock)) {
guess = info->root_guess;
} else {
guess = NULL;
}
guess = info->root_guess;

#ifdef BTR_CUR_HASH_ADAPT
rw_lock_t* const search_latch = btr_get_search_latch(index);
@@ -1509,10 +1501,7 @@ btr_cur_search_to_nth_level_func(
}

#ifdef BTR_CUR_ADAPT
if (block != guess) {
info->root_guess = block;
info->withdraw_clock = buf_withdraw_clock;
}
info->root_guess = block;
#endif
}

@@ -165,11 +165,10 @@ btr_pcur_store_position(
index, rec, &cursor->old_n_fields,
&cursor->old_rec_buf, &cursor->buf_size);

cursor->block_when_stored = block;
cursor->block_when_stored.store(block);

/* Function try to check if block is S/X latch. */
cursor->modify_clock = buf_block_get_modify_clock(block);
cursor->withdraw_clock = buf_withdraw_clock;
}

/**************************************************************//**
@@ -199,6 +198,26 @@ btr_pcur_copy_stored_position(
pcur_receive->old_n_fields = pcur_donate->old_n_fields;
}

/** Structure acts as functor to do the latching of leaf pages.
It returns true if latching of leaf pages succeeded and false
otherwise. */
struct optimistic_latch_leaves
{
btr_pcur_t *const cursor;
ulint *latch_mode;
mtr_t *const mtr;

optimistic_latch_leaves(btr_pcur_t *cursor, ulint *latch_mode, mtr_t *mtr)
:cursor(cursor), latch_mode(latch_mode), mtr(mtr) {}

bool operator() (buf_block_t *hint) const
{
return hint && btr_cur_optimistic_latch_leaves(
hint, cursor->modify_clock, latch_mode,
btr_pcur_get_btr_cur(cursor), __FILE__, __LINE__, mtr);
}
};

/**************************************************************//**
Restores the stored position of a persistent cursor bufferfixing the page and
obtaining the specified latches. If the cursor position was saved when the
@@ -261,7 +280,7 @@ btr_pcur_restore_position_func(
cursor->latch_mode =
BTR_LATCH_MODE_WITHOUT_INTENTION(latch_mode);
cursor->pos_state = BTR_PCUR_IS_POSITIONED;
cursor->block_when_stored = btr_pcur_get_block(cursor);
cursor->block_when_stored.clear();

return(FALSE);
}
@@ -276,12 +295,9 @@ btr_pcur_restore_position_func(
case BTR_MODIFY_PREV:
/* Try optimistic restoration. */

if (!buf_pool_is_obsolete(cursor->withdraw_clock)
&& btr_cur_optimistic_latch_leaves(
cursor->block_when_stored, cursor->modify_clock,
&latch_mode, btr_pcur_get_btr_cur(cursor),
file, line, mtr)) {

if (cursor->block_when_stored.run_with_hint(
optimistic_latch_leaves(cursor, &latch_mode,
mtr))) {
cursor->pos_state = BTR_PCUR_IS_POSITIONED;
cursor->latch_mode = latch_mode;

@@ -378,11 +394,10 @@ btr_pcur_restore_position_func(
since the cursor can now be on a different page!
But we can retain the value of old_rec */

cursor->block_when_stored = btr_pcur_get_block(cursor);
cursor->block_when_stored.store(btr_pcur_get_block(cursor));
cursor->modify_clock = buf_block_get_modify_clock(
cursor->block_when_stored);
cursor->block_when_stored.block());
cursor->old_stored = true;
cursor->withdraw_clock = buf_withdraw_clock;

mem_heap_free(heap);

@@ -0,0 +1,78 @@
/*****************************************************************************
Copyright (c) 2020, Oracle and/or its affiliates. All Rights Reserved.
Copyright (c) 2020, MariaDB Corporation.
This program is free software; you can redistribute it and/or modify it under
the terms of the GNU General Public License, version 2.0, as published by the
Free Software Foundation.
This program is also distributed with certain software (including but not
limited to OpenSSL) that is licensed under separate terms, as designated in a
particular file or component or in included license documentation. The authors
of MySQL hereby grant you an additional permission to link the program and
your derivative works with the separately licensed software that they have
included with MySQL.
This program is distributed in the hope that it will be useful, but WITHOUT
ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
FOR A PARTICULAR PURPOSE. See the GNU General Public License, version 2.0,
for more details.
You should have received a copy of the GNU General Public License along with
this program; if not, write to the Free Software Foundation, Inc.,
51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*****************************************************************************/

#include "buf0block_hint.h"
namespace buf {

void Block_hint::buffer_fix_block_if_still_valid()
{
/* We need to check if m_block points to one of chunks. For this to be
meaningful we need to prevent freeing memory while we check, and until we
buffer-fix the block. For this purpose it is enough to latch any of the many
latches taken by buf_resize().
However, for buffer-fixing to be meaningful, the block has to contain a page
(as opposed to being already empty, which might mean that buf_pool_resize()
can proceed and free it once we free the s-latch), so we confirm that the
block contains a page. However, it is not sufficient to check that this is
just any page, because just after we check it could get freed, unless we
have a latch which prevents this. This is tricky because page_hash latches
are sharded by page_id and we don't know the page_id until we look into the
block. To solve this chicken-and-egg problem somewhat, we latch the shard
for the m_page_id and compare block->page.id to it - so if is equal then we
can be reasonably sure that we have the correct latch.
There is still a theoretical problem here, where other threads might try
to modify the m_block->page.id while we are comparing it, but the chance of
accidentally causing the old space_id == m_page_id.m_space and the new
page_no == m_page_id.m_page_no is minimal as compilers emit a single 8-byte
comparison instruction to compare both at the same time atomically, and f()
will probably double-check the block->page.id again, anyway.
Finally, assuming that we have correct hash bucket latched, we should check if
the state of the block is BUF_BLOCK_FILE_PAGE before buffer-fixing the block,
as otherwise we risk buffer-fixing and operating on a block, which is already
meant to be freed. In particular, buf_LRU_free_page() first calls
buf_LRU_block_remove_hashed() under hash bucket latch protection to change the
state to BUF_BLOCK_REMOVE_HASH and then releases the latch. Later it calls
buf_LRU_block_free_hashed_page() without any latch to change the state to
BUF_BLOCK_MEMORY and reset the page's id, which means buf_resize() can free it
regardless of our buffer-fixing. */
if (m_block)
{
const buf_pool_t *const buf_pool= buf_pool_get(m_page_id);
rw_lock_t *latch= buf_page_hash_lock_get(buf_pool, m_page_id);
rw_lock_s_lock(latch);
/* If not own buf_pool_mutex, page_hash can be changed. */
latch= buf_page_hash_lock_s_confirm(latch, buf_pool, m_page_id);
if (buf_pool->is_block_field(m_block) &&
m_page_id == m_block->page.id &&
buf_block_get_state(m_block) == BUF_BLOCK_FILE_PAGE)
buf_block_buf_fix_inc(m_block, __FILE__, __LINE__);
else
clear();
rw_lock_s_unlock(latch);
}
}
} // namespace buf

0 comments on commit bc540b8

Please sign in to comment.