Details
-
Bug
-
Status: Closed (View Workflow)
-
Major
-
Resolution: Won't Fix
-
5.5, 10.0, 10.1, 10.2, 10.3, 10.4, 10.5
Description
The function buf_pool.is_uncompressed() is expected to hold on any buf_block_t* that has been allocated by buf_LRU_get_free_block(). But it turns out that it does not hold:
diff --git a/storage/innobase/buf/buf0lru.cc b/storage/innobase/buf/buf0lru.cc
|
index 577f1dae91c..decdf8a27f7 100644
|
--- a/storage/innobase/buf/buf0lru.cc
|
+++ b/storage/innobase/buf/buf0lru.cc
|
@@ -766,6 +766,7 @@ buf_block_t* buf_LRU_get_free_block(bool have_mutex)
|
}
|
memset(&block->page.zip, 0, sizeof block->page.zip);
|
block->skip_flush_check = false;
|
+ ut_ad(buf_pool.is_uncompressed(block));
|
return(block);
|
}
|
|
|
With the above patch, 10.5 would crash on bootstrap. Also 10.1 will crash with an equivalent patch:
diff --git a/storage/xtradb/buf/buf0lru.cc b/storage/xtradb/buf/buf0lru.cc
|
index 13bf5e79f2d..340f9e498c3 100644
|
--- a/storage/xtradb/buf/buf0lru.cc
|
+++ b/storage/xtradb/buf/buf0lru.cc
|
@@ -1124,6 +1124,55 @@ buf_LRU_handle_lack_of_free_blocks(
|
}
|
}
|
|
+/********************************************************************//**
|
+Find out if a pointer belongs to a buf_block_t. It can be a pointer to
|
+the buf_block_t itself or a member of it. This functions checks one of
|
+the buffer pool instances.
|
+@return TRUE if ptr belongs to a buf_block_t struct */
|
+static
|
+ibool
|
+buf_pointer_is_block_field_instance(
|
+/*================================*/
|
+ buf_pool_t* buf_pool, /*!< in: buffer pool instance */
|
+ const void* ptr) /*!< in: pointer not dereferenced */
|
+{
|
+ const buf_chunk_t* chunk = buf_pool->chunks;
|
+ const buf_chunk_t* const echunk = chunk + buf_pool->n_chunks;
|
+
|
+ /* TODO: protect buf_pool->chunks with a mutex (it will
|
+ currently remain constant after buf_pool_init()) */
|
+ while (chunk < echunk) {
|
+ if (ptr >= (void*) chunk->blocks
|
+ && ptr < (void*) (chunk->blocks + chunk->size)) {
|
+
|
+ return(TRUE);
|
+ }
|
+
|
+ chunk++;
|
+ }
|
+
|
+ return(FALSE);
|
+}
|
+
|
+/********************************************************************//**
|
+Find out if a buffer block was created by buf_chunk_init().
|
+@return TRUE if "block" has been added to buf_pool->free by buf_chunk_init() */
|
+static
|
+ibool
|
+buf_block_is_uncompressed(
|
+/*======================*/
|
+ buf_pool_t* buf_pool, /*!< in: buffer pool instance */
|
+ const buf_block_t* block) /*!< in: pointer to block,
|
+ not dereferenced */
|
+{
|
+ if ((((ulint) block) % sizeof *block) != 0) {
|
+ /* The pointer should be aligned. */
|
+ return(FALSE);
|
+ }
|
+
|
+ return(buf_pointer_is_block_field_instance(buf_pool, (void*) block));
|
+}
|
+
|
/** The maximum allowed backoff sleep time duration, microseconds */
|
#define MAX_FREE_LIST_BACKOFF_SLEEP 10000
|
|
@@ -1187,6 +1236,7 @@ buf_LRU_get_free_block(
|
|
ut_ad(buf_pool_from_block(block) == buf_pool);
|
memset(&block->page.zip, 0, sizeof block->page.zip);
|
+ ut_ad(buf_block_is_uncompressed(buf_pool, block));
|
return(block);
|
}
|
|
|
Apparently, the assumption about alignment does not hold. Can we replace it with something that is correct? The goal is to protect against ‘fake’ buf_block_t such as the one that is allocated in fil_tablespace_iterate() as well as against buffer pool resizing.
The false negatives are reducing the performance of buf_page_get_gen() by triggering unnecessary hash table lookups. The original motivation of the check was simple, and the code became dead code when buf_page_alloc_descriptor() was rewritten to not use buf_buddy_alloc() any more. But, maybe starting with 10.2, more considerations could be necessary due to buffer pool resizing?
if (block != NULL) { |
|
|
/* If the guess is a compressed page descriptor that |
has been allocated by buf_page_alloc_descriptor(),
|
it may have been freed by buf_relocate(). */
|
|
|
if (!buf_block_is_uncompressed(buf_pool, block) |
|| offset != block->page.offset
|
|| space != block->page.space
|
|| buf_block_get_state(block) != BUF_BLOCK_FILE_PAGE) {
|
|
|
/* Our guess was bogus or things have changed |
since. */
|
block = guess = NULL;
|
} else { |
ut_ad(!block->page.in_zip_hash);
|
}
|
}
|
|
|
if (block == NULL) { |
block = (buf_block_t*) buf_page_hash_get_low(
|
buf_pool, space, offset, fold);
|
}
|
Attachments
Issue Links
- relates to
-
-
- Closed
-
-
-
- Closed
-