> - 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.
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
Alexey -
> - typo (double “get”) in the updated buf_LRU_free_page() comments: get_get_ mutex() might be temporarily”
> “function returns false, the buf_page_
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 set_sticky( ). I also found that in_unzip_LRU_list
assert in buf_page_
was converted to a release build flag but no uses were converted.
Reverted that too.
> - spurious blank line changes in buf_buddy_ relocate( ), free_low( ), buf_pool_ watch_set( ),
> buf_buddy_
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( ), check_neighbor( ), buf_flush_ LRU_list_ batch() , drop_page_ hash_for_ tablespace( ) buffer_ pool_evict_ uncompressed( ).
> buf_flush_
> buf_LRU_
> and innodb_
Fixed. Also removed a diagnostic printf from validate_ instance( ).
buf_pool_
> - 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? ZIP_DIRTY: zip_mutex_ for_page( bpage); >zip_mutex; block_mutex) ; buf_fix_ count++ ; FILE_PAGE:
>
> @@ -2114,8 +2098,8 @@ buf_page_get_zip(
> break;
> case BUF_BLOCK_ZIP_PAGE:
> case BUF_BLOCK_
> + buf_enter_
> block_mutex = &buf_pool-
> - mutex_enter(
> bpage->
> goto got_block;
> case BUF_BLOCK_
No, I don't. A debugging leftover, reverted.
> - in the following change: own_zip_ mutex_for_ page(bpage) ); >buf_fix_ count get_io_ fix(bpage) != BUF_IO_NONE) { init_for_ read(). */ block_mutex) ; &buf_pool- >zip_mutex) ; zip_mutex_ for_page( ) call.
>
> @@ -2721,13 +2707,14 @@ buf_page_get_gen(
> }
>
> bpage = &block->page;
> + ut_ad(buf_
>
> if (bpage-
> || buf_page_
> /* This condition often occurs when the buffer
> is not buffer-fixed, but I/O-fixed by
> buf_page_
> - mutex_exit(
> + mutex_exit(
> 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_
Yes. Replaced buf_own_ zip_mutex_ for_page( ) with ut_ad(block_mutex == >zip_mutex) , which also happens to be symmetric with "!=" FILE_PAGE. Reverted the mutex_exit change
&buf_pool-
assert above for BUF_BLOCK_
too.
> - same comments for this change: block_mutex) ; &buf_pool- >zip_mutex) ; get_free_ block(buf_ pool);
>
> @@ -2737,11 +2724,11 @@ buf_page_get_gen(
> }
>
> /* Allocate an uncompressed page. */
> - mutex_exit(
> + mutex_exit(
> block = buf_LRU_
> ut_a(block);
Reverted too.
> - in buf_mark_ space_corrupt( ) I see LRU_list_mutex, hash_lock and get_mutex( ) being acquired, but only LRU_list_mutex and get_mutex( ) being released, i.e. it returns with hash_lock
> buf_page_
> buf_page_
> acquired?
I don't see buf_page_ get_mutex( ) being released directly in space_corrupt( )? buf_LRU_ free_one_ page() will release hash get_mutex( ), thus no bug. Should free_one_ page() locking be documented better in its header
buf_mark_
lock and buf_page_
buf_LRU_
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 validate_ instance( ).
it must happen under flush_state_mutex in order to observe consistent
buffer pool in buf_pool_
> - more spurious changes:
> - UT_LIST_ INSERT_ AFTER(list, buf_pool- >flush_ list, INSERT_ AFTER(list, buf_pool- >flush_ list, prev_b, wait(buf_ pool->no_ flush[type] ); wait(buf_ pool->no_ flush[type] ); >init_flush[ BUF_FLUSH_ LRU]) { >init_flush[ BUF_FLUSH_ LRU]) {
> - prev_b, &block->page);
> + UT_LIST_
> + &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_
> + os_event_
> - || buf_pool-
> + || buf_pool-
> - ibool freed;
> + ibool freed;
All reverted.
> - in the following hunk there’s no really need for the “else” block, try_neighbors(
> and a call to mutex_exit() can be moved out of the “if” statement:
>
> @@ -1316,14 +1317,14 @@ buf_flush_
Yes, fixed.
> - typo in the comments: get_mutex( ) mutex. */ get_mutex( ) mutex therefore
>
> + /* The following call will release the buf_pgae_
> ...
> + As we are not holding LRU list or buf_pgae_
Oh. Fixed.