Merge lp:~laurynas-biveinis/percona-server/bug1305364-5.5 into lp:percona-server/5.5

[Laurynas Biveinis]

Fix bug 1305364 (InnoDB: Assertion failure in file buf0flu.cc line 546
| crashes if RW workload and InnoDB compression are combined) and bug
1219833 (Redundant page state re-checks in
buf_LRU_free_from_unzip_LRU_list() and
buf_LRU_free_from_common_LRU_list()).

The former bug was fixed by reviewing buf_page_in_file calls and
buf_page_get_state == BUF_BLOCK_REMOVE_HASH checks for locking
correctness. The transition for a buffer page state from
BUF_BLOCK_FILE_PAGE to BUF_BLOCK_REMOVE_HASH is protected by a
combination of locked page_hash_latch, LRU_list_mutex, and block
mutex. Thus, any of the checks above must have locked at least one of
those three sync objects.
- buf_page_get_zip: check if buf_page_get_state is still
  BUF_BLOCK_FILE_PAGE between mutex re-acquisition and
  buf_LRU_free_block call.
- buf_LRU_search_and_free_block: lock LRU list mutex unconditionally,
  so that buf_LRU_free_from_unzip_LRU_list and
  buf_LRU_free_from_common_LRU_list do not attempt unlocked LRU list
  scans anymore.
- buf_read_ahead_random: lock page_hash_latch and do not lock buffer
  pool mutex around buf_page_hash_get mutex. This is in line with
  previous fixes to buf_read_ahead_linear.

Add several asserts to document the required locking better in
buf_pool_watch_is_sentinel, buf_flush_page_and_try_neighbors,
buf_LRU_evict_from_unzip_LRU, buf_flush_or_remove_pages,
buf_LRU_free_from_unzip_LRU_list, buf_LRU_free_from_common_LRU_list,
and buf_page_get_block. Some of these are UNIV_SYNC_DEBUG, thus
nothing more than code comments, because currently UNIV_SYNC_DEBUG
builds are broken.

buf_LRU_search_and_free_block locking LRU list unconditionally enables
removing all the code in its callees that attempts to perform lockless
LRU list scans and is broken, as well as fixing bug 1219833. There
- remove have_LRU_mutex args from buf_LRU_evict_from_unzip_LRU,
  buf_LRU_free_from_unzip_LRU_list, buf_LRU_free_from_common_LRU_list,
  as it's always TRUE.
- remove any have_LRU_mutex checks, page state re-checks, and scan
  restarts from the same functions.

http://jenkins.percona.com/job/percona-server-5.5-param/989/

Not sure if G1 or G2.

[Vlad Lesin (vlad-lesin)]

I don't see any errors here. The code looks good for me and explanations are reasonable.

review: Approve (g2)

=== modified file 'storage/innobase/buf/buf0buf.c'
--- storage/innobase/buf/buf0buf.c	2014-03-27 15:42:21 +0000
+++ storage/innobase/buf/buf0buf.c	2014-04-24 16:37:49 +0000
@@ -1600,6 +1600,12 @@
 	buf_pool_t*		buf_pool,	/*!< buffer pool instance */
 	const buf_page_t*	bpage)		/*!< in: block */
 {
+#ifdef UNIV_SYNC_DEBUG
+	ut_ad(rw_lock_own(&buf_pool->page_hash_latch, RW_LOCK_SHARED)
+	      || rw_lock_own(&buf_pool->page_hash_latch, RW_LOCK_EX)
+	      || mutex_own(buf_page_get_mutex(bpage)));
+#endif
+
 	ut_ad(buf_page_in_file(bpage));
 
 	if (bpage < &buf_pool->watch[0]
@@ -2048,7 +2054,9 @@
 		mutex_enter(&buf_pool->LRU_list_mutex);
 		mutex_enter(block_mutex);
 
-		if (UNIV_UNLIKELY(bpage->space != space
+		if (UNIV_UNLIKELY((buf_page_get_state(bpage)
+				   != BUF_BLOCK_FILE_PAGE)
+				  || bpage->space != space
 				  || bpage->offset != offset
 				  || !bpage->in_LRU_list
 				  || !bpage->zip.data)) {

=== modified file 'storage/innobase/buf/buf0flu.c'
--- storage/innobase/buf/buf0flu.c	2014-01-15 14:12:15 +0000
+++ storage/innobase/buf/buf0flu.c	2014-04-24 16:37:49 +0000
@@ -1606,7 +1606,11 @@
 	}
 
 	ut_a(buf_page_in_file(bpage)
-	     || buf_page_get_state(bpage) == BUF_BLOCK_REMOVE_HASH);
+	     || (buf_page_get_state(bpage) == BUF_BLOCK_REMOVE_HASH
+#ifdef UNIV_DEBUG
+		 && !mutex_own(&buf_pool->LRU_list_mutex)
+#endif
+		 ));
 
 	if (buf_flush_ready_for_flush(bpage, flush_type)) {
 		ulint		space;

=== modified file 'storage/innobase/buf/buf0lru.c'
--- storage/innobase/buf/buf0lru.c	2013-10-23 08:23:08 +0000
+++ storage/innobase/buf/buf0lru.c	2014-04-24 16:37:49 +0000
@@ -177,20 +177,15 @@
 ibool
 buf_LRU_evict_from_unzip_LRU(
 /*=========================*/
-	buf_pool_t*	buf_pool,
-	ibool		have_LRU_mutex)
+	buf_pool_t*	buf_pool)
 {
 	ulint	io_avg;
 	ulint	unzip_avg;
 
-	//ut_ad(buf_pool_mutex_own(buf_pool));
+	ut_ad(mutex_own(&buf_pool->LRU_list_mutex));
 
-	if (!have_LRU_mutex)
-		mutex_enter(&buf_pool->LRU_list_mutex);
 	/* If the unzip_LRU list is empty, we can only use the LRU. */
 	if (UT_LIST_GET_LEN(buf_pool->unzip_LRU) == 0) {
-		if (!have_LRU_mutex)
-			mutex_exit(&buf_pool->LRU_list_mutex);
 		return(FALSE);
 	}
 
@@ -199,20 +194,14 @@
 	decompressed pages in the buffer pool. */
 	if (UT_LIST_GET_LEN(buf_pool->unzip_LRU)
 	    <= UT_LIST_GET_LEN(buf_pool->LRU) / 10) {
-		if (!have_LRU_mutex)
-			mutex_exit(&buf_pool->LRU_list_mutex);
 		return(FALSE);
 	}
 
 	/* If eviction hasn't started yet, we assume by default
 	that a workload is disk bound. */
 	if (buf_pool->freed_page_clock == 0) {
-		if (!have_LRU_mutex)
-			mutex_exit(&buf_pool->LRU_list_mutex);
 		return(TRUE);
 	}
-	if (!have_LRU_mutex)
-		mutex_exit(&buf_pool->LRU_list_mutex);
 
 	/* Calculate the average over past intervals, and add the values
 	of the current interval. */
@@ -612,6 +601,8 @@
 	ibool		all_freed = TRUE;
 	ibool		must_restart = FALSE;
 
+	ut_ad(mutex_own(&buf_pool->LRU_list_mutex));
+
 	buf_flush_list_mutex_enter(buf_pool);
 
 	for (bpage = UT_LIST_GET_LAST(buf_pool->flush_list);
@@ -930,19 +921,18 @@
 buf_LRU_free_from_unzip_LRU_list(
 /*=============================*/
 	buf_pool_t*	buf_pool,	/*!< in: buffer pool instance */
-	ulint		n_iterations,	/*!< in: how many times this has
+	ulint		n_iterations)	/*!< in: how many times this has
 					been called repeatedly without
 					result: a high value means that
 					we should search farther; we will
 					search n_iterations / 5 of the
 					unzip_LRU list, or nothing if
 					n_iterations >= 5 */
-	ibool		have_LRU_mutex)
 {
 	buf_block_t*	block;
 	ulint		distance;
 
-	//ut_ad(buf_pool_mutex_own(buf_pool));
+	ut_ad(mutex_own(&buf_pool->LRU_list_mutex));
 
 	/* Theoratically it should be much easier to find a victim
 	from unzip_LRU as we can choose even a dirty block (as we'll
@@ -952,7 +942,7 @@
 	if we have done five iterations so far. */
 
 	if (UNIV_UNLIKELY(n_iterations >= 5)
-	    || !buf_LRU_evict_from_unzip_LRU(buf_pool, have_LRU_mutex)) {
+	    || !buf_LRU_evict_from_unzip_LRU(buf_pool)) {
 
 		return(FALSE);
 	}
@@ -960,25 +950,18 @@
 	distance = 100 + (n_iterations
 			  * UT_LIST_GET_LEN(buf_pool->unzip_LRU)) / 5;
 
-restart:
 	for (block = UT_LIST_GET_LAST(buf_pool->unzip_LRU);
 	     UNIV_LIKELY(block != NULL) && UNIV_LIKELY(distance > 0);
 	     block = UT_LIST_GET_PREV(unzip_LRU, block), distance--) {
 
 		ibool freed;
 
-		mutex_enter(&block->mutex);
-		if (!block->in_unzip_LRU_list || !block->page.in_LRU_list
-		    || buf_block_get_state(block) != BUF_BLOCK_FILE_PAGE) {
-			mutex_exit(&block->mutex);
-			goto restart;
-		}
-
 		ut_ad(buf_block_get_state(block) == BUF_BLOCK_FILE_PAGE);
 		ut_ad(block->in_unzip_LRU_list);
 		ut_ad(block->page.in_LRU_list);
 
-		freed = buf_LRU_free_block(&block->page, FALSE, have_LRU_mutex);
+		mutex_enter(&block->mutex);
+		freed = buf_LRU_free_block(&block->page, FALSE, TRUE);
 		mutex_exit(&block->mutex);
 
 		if (freed) {
@@ -997,46 +980,35 @@
 buf_LRU_free_from_common_LRU_list(
 /*==============================*/
 	buf_pool_t*	buf_pool,
-	ulint		n_iterations,
+	ulint		n_iterations)
 				/*!< in: how many times this has been called
 				repeatedly without result: a high value means
 				that we should search farther; if
 				n_iterations < 10, then we search
 				n_iterations / 10 * buf_pool->curr_size
 				pages from the end of the LRU list */
-	ibool		have_LRU_mutex)
 {
 	buf_page_t*	bpage;
 	ulint		distance;
 
-	//ut_ad(buf_pool_mutex_own(buf_pool));
+	ut_ad(mutex_own(&buf_pool->LRU_list_mutex));
 
 	distance = 100 + (n_iterations * buf_pool->curr_size) / 10;
 
-restart:
 	for (bpage = UT_LIST_GET_LAST(buf_pool->LRU);
 	     UNIV_LIKELY(bpage != NULL) && UNIV_LIKELY(distance > 0);
 	     bpage = UT_LIST_GET_PREV(LRU, bpage), distance--) {
 
 		ibool		freed;
 		unsigned	accessed;
-		mutex_t*	block_mutex = buf_page_get_mutex_enter(bpage);
-
-		if (!block_mutex) {
-			goto restart;
-		}
-
-		if (!bpage->in_LRU_list
-		    || !buf_page_in_file(bpage)) {
-			mutex_exit(block_mutex);
-			goto restart;
-		}
+		mutex_t*	block_mutex = buf_page_get_mutex(bpage);
 
 		ut_ad(buf_page_in_file(bpage));
 		ut_ad(bpage->in_LRU_list);
 
+		mutex_enter(block_mutex);
 		accessed = buf_page_is_accessed(bpage);
-		freed = buf_LRU_free_block(bpage, TRUE, have_LRU_mutex);
+		freed = buf_LRU_free_block(bpage, TRUE, TRUE);
 		mutex_exit(block_mutex);
 
 		if (freed) {
@@ -1073,23 +1045,18 @@
 				n_iterations / 5 of the unzip_LRU list. */
 {
 	ibool	freed = FALSE;
-	ibool	have_LRU_mutex = FALSE;
-
-	if (UT_LIST_GET_LEN(buf_pool->unzip_LRU))
-		have_LRU_mutex = TRUE;
-
-	//buf_pool_mutex_enter(buf_pool);
-	if (have_LRU_mutex)
-		mutex_enter(&buf_pool->LRU_list_mutex);
-
-	freed = buf_LRU_free_from_unzip_LRU_list(buf_pool, n_iterations, have_LRU_mutex);
+
+	mutex_enter(&buf_pool->LRU_list_mutex);
+
+	freed = buf_LRU_free_from_unzip_LRU_list(buf_pool, n_iterations);
 
 	if (!freed) {
 		freed = buf_LRU_free_from_common_LRU_list(
-			buf_pool, n_iterations, have_LRU_mutex);
+			buf_pool, n_iterations);
 	}
 
 	buf_pool_mutex_enter(buf_pool);
+
 	if (!freed) {
 		buf_pool->LRU_flush_ended = 0;
 	} else if (buf_pool->LRU_flush_ended > 0) {
@@ -1097,8 +1064,8 @@
 	}
 
 	buf_pool_mutex_exit(buf_pool);
-	if (have_LRU_mutex)
-		mutex_exit(&buf_pool->LRU_list_mutex);
+
+	mutex_exit(&buf_pool->LRU_list_mutex);
 
 	return(freed);
 }

=== modified file 'storage/innobase/buf/buf0rea.c'
--- storage/innobase/buf/buf0rea.c	2013-08-14 03:52:04 +0000
+++ storage/innobase/buf/buf0rea.c	2014-04-24 16:37:49 +0000
@@ -340,9 +340,13 @@
 		return(0);
 	}
 
+	buf_pool_mutex_exit(buf_pool);
+
 	/* Count how many blocks in the area have been recently accessed,
 	that is, reside near the start of the LRU list. */
 
+	rw_lock_s_lock(&buf_pool->page_hash_latch);
+
 	for (i = low; i < high; i++) {
 		const buf_page_t* bpage =
 			buf_page_hash_get(buf_pool, space, i);
@@ -356,13 +360,13 @@
 			if (recent_blocks
 			    >= BUF_READ_AHEAD_RANDOM_THRESHOLD(buf_pool)) {
 
-				buf_pool_mutex_exit(buf_pool);
+				rw_lock_s_unlock(&buf_pool->page_hash_latch);
 				goto read_ahead;
 			}
 		}
 	}
 
-	buf_pool_mutex_exit(buf_pool);
+	rw_lock_s_unlock(&buf_pool->page_hash_latch);
 	/* Do nothing */
 	return(0);
 

=== modified file 'storage/innobase/include/buf0buf.ic'
--- storage/innobase/include/buf0buf.ic	2013-05-23 08:39:28 +0000
+++ storage/innobase/include/buf0buf.ic	2014-04-24 16:37:49 +0000
@@ -689,6 +689,11 @@
 /*===============*/
 	buf_page_t*	bpage)	/*!< in: control block, or NULL */
 {
+#ifdef UNIV_SYNC_DEBUG
+	buf_pool_t*	buf_pool = buf_pool_from_bpage(bpage);
+	ut_ad(rw_lock_own(&buf_pool->page_hash_latch, RW_LOCK_SHARED)
+	      || rw_lock_own(&buf_pool->page_hash_latch, RW_LOCK_EX));
+#endif
 	if (UNIV_LIKELY(bpage != NULL)) {
 		ut_ad(buf_page_in_file(bpage));