Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-23158

False negatives from buf_pool.is_uncompressed() cause unnecessary lookups

    XMLWordPrintable

    Details

      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

            Activity

              People

              Assignee:
              marko Marko Mäkelä
              Reporter:
              marko Marko Mäkelä
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Git Integration

                  Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.