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

Alexey -

> - typo (double “get”) in the updated buf_LRU_free_page() comments:
> “function returns false, the buf_page_get_get_mutex() might be temporarily”

Fixed.

> - 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?

in_LRU_list changes have been reverted with the exception of an extra
assert in buf_page_set_sticky(). I also found that in_unzip_LRU_list
was converted to a release build flag but no uses were converted.
Reverted that too.

> - spurious blank line changes in buf_buddy_relocate(),
> buf_buddy_free_low(), buf_pool_watch_set(),

Fixed.

> buf_page_get_gen(),

I didn't find this one. Will re-check before the final push.

> buf_page_init_for_read(), buf_pool_validate_instance(),
> buf_flush_check_neighbor(), buf_flush_LRU_list_batch(),
> buf_LRU_drop_page_hash_for_tablespace()
> and innodb_buffer_pool_evict_uncompressed().

Fixed. Also removed a diagnostic printf from
buf_pool_validate_instance().

> - the following change in buf_block_try_discard_uncompressed() does
> not release block_mutex if buf_LRU_free_page() returns false.

Fixed.

> - do you really need this change?
>
> @@ -2114,8 +2098,8 @@ buf_page_get_zip(
> break;
> case BUF_BLOCK_ZIP_PAGE:
> case BUF_BLOCK_ZIP_DIRTY:
> + buf_enter_zip_mutex_for_page(bpage);
> block_mutex = &buf_pool->zip_mutex;
> - mutex_enter(block_mutex);
> bpage->buf_fix_count++;
> goto got_block;
> case BUF_BLOCK_FILE_PAGE:

No, I don't. A debugging leftover, reverted.

> - in the following change:
>
> @@ -2721,13 +2707,14 @@ buf_page_get_gen(
> }
>
> bpage = &block->page;
> + ut_ad(buf_own_zip_mutex_for_page(bpage));
>
> if (bpage->buf_fix_count
> || buf_page_get_io_fix(bpage) != BUF_IO_NONE) {
> /* This condition often occurs when the buffer
> is not buffer-fixed, but I/O-fixed by
> buf_page_init_for_read(). */
> - mutex_exit(block_mutex);
> + mutex_exit(&buf_pool->zip_mutex);
> wait_until_unfixed:
> /* The block is buffer-fixed or I/O-fixed.
> Try again later. */
>
> is there a reason for replacing block_mutex with zip_mutex? you
> could just assert that block_mutex is zip_mutex next to, or even
> instead of the buf_own_zip_mutex_for_page() call.

Yes. Replaced buf_own_zip_mutex_for_page() with ut_ad(block_mutex ==
&buf_pool->zip_mutex), which also happens to be symmetric with "!="
assert above for BUF_BLOCK_FILE_PAGE. Reverted the mutex_exit change
too.

> - same comments for this change:
>
> @@ -2737,11 +2724,11 @@ buf_page_get_gen(
> }
>
> /* Allocate an uncompressed page. */
> - mutex_exit(block_mutex);
> + mutex_exit(&buf_pool->zip_mutex);
> block = buf_LRU_get_free_block(buf_pool);
> ut_a(block);

Reverted too.

> - in buf_mark_space_corrupt() I see LRU_list_mutex, hash_lock and
> buf_page_get_mutex() being acquired, but only LRU_list_mutex and
> buf_page_get_mutex() being released, i.e. it returns with hash_lock
> acquired?

I don't see buf_page_get_mutex() being released directly in
buf_mark_space_corrupt()? buf_LRU_free_one_page() will release hash
lock and buf_page_get_mutex(), thus no bug. Should
buf_LRU_free_one_page() locking be documented better in its header
comment?

> - it looks like with the changes in buf_page_io_complete() for io_type
> == BUF_IO_WRITE we don’t set io_fix to BUF_IO_NONE, though we did
> before the changes?

It's pushed down to buf_flush_write_complete(). The reason is that
it must happen under flush_state_mutex in order to observe consistent
buffer pool in buf_pool_validate_instance().

> - more spurious changes:

> - UT_LIST_INSERT_AFTER(list, buf_pool->flush_list,
> - prev_b, &block->page);
> + UT_LIST_INSERT_AFTER(list, buf_pool->flush_list, prev_b,
> + &block->page);
> -Remove a block from the flush list of modified blocks. */
> +Remove a block from the flush list of modified blocks. */
> - ulint count = 0;
> + ulint count = 0;
> - os_event_wait(buf_pool->no_flush[type]);
> + os_event_wait(buf_pool->no_flush[type]);
> - || buf_pool->init_flush[BUF_FLUSH_LRU]) {
> + || buf_pool->init_flush[BUF_FLUSH_LRU]) {
> - ibool freed;
> + ibool freed;

All reverted.

> - in the following hunk there’s no really need for the “else” block,
> and a call to mutex_exit() can be moved out of the “if” statement:
>
> @@ -1316,14 +1317,14 @@ buf_flush_try_neighbors(

Yes, fixed.

> - typo in the comments:
>
> + /* The following call will release the buf_pgae_get_mutex() mutex. */
> ...
> + As we are not holding LRU list or buf_pgae_get_mutex() mutex therefore

Oh. Fixed.