[MDEV-23158] False negatives from buf_pool.is_uncompressed() cause unnecessary lookups Created: 2020-07-13  Updated: 2022-02-21  Resolved: 2022-02-21

Status: Closed
Project: MariaDB Server
Component/s: Storage Engine - InnoDB
Affects Version/s: 5.5, 10.0, 10.1, 10.2, 10.3, 10.4, 10.5
Fix Version/s: 10.6.6

Type: Bug Priority: Major
Reporter: Marko Mäkelä Assignee: Marko Mäkelä
Resolution: Won't Fix Votes: 0
Labels: performance

Issue Links:
Relates
relates to MDEV-27058 Buffer page descriptors are too large Closed
relates to MDEV-27476 heap-use-after-free in buf_pool_t::is... Closed

 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);
	}



 Comments   
Comment by Marko Mäkelä [ 2022-02-21 ]

I think that this bug was essentially fixed by MDEV-27058 and MDEV-27476 in MariaDB Server 10.6.6. We can simply check if buf_page_t::frame is nullptr to find out if an uncompressed page frame is present.

Generated at Thu Feb 08 09:20:17 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.