[Draft] buf_page_get_mutex_enter() is used either redundantly, either dangerously

[Laurynas Biveinis]

This is a draft description for further analysis.

A port of buf_page_get_mutex_enter() from 5.5 to 5.6 showed that it is used either redundantly, either dangerously, and the port removed it. It needs to be seen whether that applies to it in 5.5 as well.

In 5.5 it is defined as

/*********************************************************************//**
Gets the mutex of a block.
@return pointer to mutex protecting bpage */
UNIV_INLINE
mutex_t*
buf_page_get_mutex(
/*===============*/
 const buf_page_t* bpage) /*!< in: pointer to control block */
{
 buf_pool_t* buf_pool = buf_pool_from_bpage(bpage);

 if (/*equivalent to buf_pool_watch_is_sentinel(buf_pool, bpage)*/
     bpage >= &buf_pool->watch[0]
     && bpage < &buf_pool->watch[BUF_POOL_WATCH_SIZE]) {
  /* TODO: this code is the interim. should be confirmed later. */
  return(&buf_pool->zip_mutex);
 }

 switch (buf_page_get_state(bpage)) {
 case BUF_BLOCK_ZIP_FREE:
  /* ut_error; */ /* optimistic */
  return(NULL);
 case BUF_BLOCK_ZIP_PAGE:
 case BUF_BLOCK_ZIP_DIRTY:
  return(&buf_pool->zip_mutex);
 default:
  return(&((buf_block_t*) bpage)->mutex);
 }
}

/*************************************************************************
Gets the mutex of a block and enter the mutex with consistency. */
UNIV_INLINE
mutex_t*
buf_page_get_mutex_enter(
/*=========================*/
 const buf_page_t* bpage) /*!< in: pointer to control block */
{
 mutex_t* block_mutex;

 while(1) {
  block_mutex = buf_page_get_mutex(bpage);
  if (!block_mutex)
   return block_mutex;

  mutex_enter(block_mutex);
  if (block_mutex == buf_page_get_mutex(bpage))
   return block_mutex;
  mutex_exit(block_mutex);
 }
}

It hard to say what "consistency" in "enter the mutex with consistency" means.

It appears to protect from the buffer page state transition race conditions, namely, from the page moving between BUF_POOL_BLOCK_WATCH|BUF_BLOCK_ZIP_PAGE|BUF_BLOCK_ZIP_DIRTY and BUF_BLOCK_ZIP_FREE and all the remaining states. The sets of these states is defined by the patched-in-XtraDB buf_page_get_mutex(). Were it not patched, then the sets of the states would be BUF_BLOCK_ZIP_PAGE|BUF_BLOCK_ZIP_DIRTY and everything else, with BUF_BLOCK_ZIP_FREE forbidden.

The buffer page state transitions are protected by buffer pool mutexes, thus this appears to handle the situation where none are held. But then, depending on where the bpage pointer was taken from, the pointer itself may become invalid if it points not to a buffer pool frame but to a compressed page descriptor that might get freed. Then the protection offered by buf_page_get_mutex_enter() is useless too. If it can be proven that bpage may not become invalid, then most probably its state may not change neither and simple buf_page_get_mutex/mutex_enter call pair is enough. In the worst case (this is why this bug is here), the bpage may indeed become invalid and the correct locking must be ensured.

[Laurynas Biveinis (laurynas-biveinis)]

buf_page_get_mutex() patch to handle BUF_BLOCK_ZIP_FREE and BUF_BLOCK_POOL_WATCH is suspicious on its own too.

[Roel Van de Paar (roel11)]

Confirmed not a 5.6 issue as per email Laurynas

[Laurynas Biveinis (laurynas-biveinis)]

Bug 1250762 might be related.

[Laurynas Biveinis (laurynas-biveinis)]

Bug 1250762 not related.

[Shahriyar Rzayev (rzayev-sehriyar)]

Percona now uses JIRA for bug reports so this bug report is migrated to: https://jira.percona.com/browse/PS-717