Code review comment for lp:~laurynas-biveinis/percona-server/bp-split-5.6

Alexey -

Replying to this bit separately as it may need further discussion while I am addressing the rest of comments.

> - what’s the reason for changing buf_page_t::in_LRU_list to be present
> in release builds? Unless I’m missing something in the current code,
> it is only assigned, but never read in release builds?

This is another "automerge" from 5.5 and indeed serves no release
build purpose in the current 5.6 code. But it points out a
non-trivial thing. In 5.5 it is used as follows:
-) for checking whether a given page is still on the LRU list if both
   block and LRU mutexes were temporarily released:
   buf_page_get_zip(), buf_LRU_free_block() (both are different code
   from 5.6).
-) for iterating through the LRU list without holding the LRU list
   mutex at all: buf_LRU_free_from_common_LRU_list(),
   buf_LRU_free_block(),
   buf_flush_LRU_recommendation()/buf_flush_ready_for_replace(). I
   think this is unsafe and a bug in 5.5 due to page relocations
   potentially resulting in wild pointers, even if it does wonders for
   the LRU list contention. 5.6 holds the LRU list mutex in the corresponding code.
-) redundant checks, ie. on LRU list iteration where the mutex is not
   released: buf_LRU_insert_zip_clean(),
   buf_LRU_free_from_unzip_LRU_list().

Thus I think 1) in_LRU_list changes should be reverted now. 2) 5.5
might need fixing. 3) The LRU list mutex is hot in 5.6. If there is
a safe way not to hold it in 5.6 (for example, for
BUF_BLOCK_FILE_PAGE, but hard to tell the page type without
dereferencing page pointer - maybe by comparing the page address
against buffer pool chunk address range?), then it's worth looking
into it.