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

More comments on the patch:

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

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

  - spurious blank line changes in buf_buddy_relocate(),
    buf_buddy_free_low(), buf_pool_watch_set(), buf_page_get_gen(),
    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().

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

  bpage = buf_page_hash_get(buf_pool, space, offset);

  if (bpage) {
- buf_LRU_free_page(bpage, false);
+
+ ib_mutex_t* block_mutex = buf_page_get_mutex(bpage);
+
+ mutex_enter(block_mutex);
+
+ if (buf_LRU_free_page(bpage, false)) {
+
+ mutex_exit(block_mutex);
+ return;
+ }
  }

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

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

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

- buf_pool_mutex_enter(buf_pool);
+ mutex_enter(&buf_pool->LRU_list_mutex);

   /* As we have released the page_hash lock and the
   block_mutex to allocate an uncompressed page it is

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

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

  - more spurious changes:

@@ -475,8 +473,8 @@ buf_flush_insert_sorted_into_flush_list(
  if (prev_b == NULL) {
   UT_LIST_ADD_FIRST(list, buf_pool->flush_list, &block->page);
  } else {
- 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);
  }

  incr_flush_list_size_in_bytes(block, buf_pool);

@@ -573,7 +567,7 @@ buf_flush_ready_for_flush(
 }

and

 /********************************************************************//**
-Remove a block from the flush list of modified blocks. */
+Remove a block from the flush list of modified blocks. */
 UNIV_INTERN
 void
 buf_flush_remove(

and

@@ -1693,7 +1729,7 @@ buf_flush_batch(
      (if their number does not exceed
      min_n), otherwise ignored */
 {
- ulint count = 0;
+ ulint count = 0;

  ut_ad(flush_type == BUF_FLUSH_LRU || flush_type == BUF_FLUSH_LIST);
 #ifdef UNIV_SYNC_DEBUG

and

@@ -1836,7 +1870,7 @@ buf_flush_wait_batch_end(
   }
  } else {
   thd_wait_begin(NULL, THD_WAIT_DISKIO);
- os_event_wait(buf_pool->no_flush[type]);
+ os_event_wait(buf_pool->no_flush[type]);
   thd_wait_end(NULL);
  }
 }

and

   if (buf_pool->n_flush[BUF_FLUSH_LRU] > 0
- || buf_pool->init_flush[BUF_FLUSH_LRU]) {
+ || buf_pool->init_flush[BUF_FLUSH_LRU]) {

and

@@ -958,10 +1018,10 @@ buf_LRU_free_from_unzip_LRU_list(
      srv_LRU_scan_depth / 2 blocks. */
 {
  buf_block_t* block;
- ibool freed;
+ ibool freed;
  ulint scanned;

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

     buf_flush_page(buf_pool, bpage, flush_type, false);
     ut_ad(!mutex_own(block_mutex));
- ut_ad(!buf_pool_mutex_own(buf_pool));
     count++;
     continue;
- } else {
- mutex_exit(block_mutex);
    }
+ mutex_exit(block_mutex);
+ } else {
+
+ mutex_exit(block_mutex);
   }
- buf_pool_mutex_exit(buf_pool);
  }

  if (count > 0) {

  - 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