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

[Laurynas Biveinis]

Fix bug 1122462.
http://jenkins.percona.com/job/percona-server-5.5-param/667/

Fix bug 1122462 (InnoDB: Failing assertion: bpage->in_flush_list in
file buf0lru.c line 605).

The issue is that, even after fixing bug 934377 and friends, there is
not enough protection for the "prev" pointer in the
buf_flush_or_remove_pages() loop to start pointing to a flushed page
or become dangling due to a compressed page flush list relocation.

The fix is two part. First is acquiring the compressed page flush
mutex in buf_flush_dirty_pages() (and releasing it in
buf_flush_yield()). This ensures that the prev pointer always points
to a valid page.

The second part is to fix that, while prev cannot become dangling
anymore, it still may get removed from the flush list when the flush
list mutex is released. This is handled by replacing the assertions
at the top of the buf_flush_or_remove_pages() loop with a block state
check.

Remove some comments that do not apply for XtraDB.

[George Ormond Lorch III (gl-az)]

review: Approve (g2)

[Laurynas Biveinis (laurynas-biveinis)]

I will redo this after the new bug 1086680 fix.

review: Needs Fixing

Unmerged revisions

441. By Laurynas Biveinis on 2013-02-12

Fix bug 1122462 (InnoDB: Failing assertion: bpage->in_flush_list in
file buf0lru.c line 605).

The issue is that, even after fixing bug 934377 and friends, there is
not enough protection for the "prev" pointer in the
buf_flush_or_remove_pages() loop to start pointing to a flushed page
or become dangling due to a compressed page flush list relocation.

The fix is two part. First is acquiring the compressed page flush
mutex in buf_flush_dirty_pages() (and releasing it in
buf_flush_yield()). This ensures that the prev pointer always points
to a valid page.

The second part is to fix that, while prev cannot become dangling
anymore, it still may get removed from the flush list when the flush
list mutex is released. This is handled by replacing the assertions
at the top of the buf_flush_or_remove_pages() loop with a block state
check.

Remove some comments that do not apply for XtraDB.

440. By Laurynas Biveinis on 2013-02-12

Automerge prerequisite branch

=== modified file 'Percona-Server/storage/innobase/buf/buf0lru.c'
--- Percona-Server/storage/innobase/buf/buf0lru.c	2013-02-06 11:01:22 +0000
+++ Percona-Server/storage/innobase/buf/buf0lru.c	2013-02-12 09:23:34 +0000
@@ -397,6 +397,7 @@
 
 	ut_ad(mutex_own(block_mutex));
 	ut_ad(mutex_own(&buf_pool->LRU_list_mutex));
+	ut_ad(mutex_own(&buf_pool->zip_dirty_flush_mutex));
 	ut_ad(buf_page_in_file(bpage));
 
 	/* "Fix" the block so that the position cannot be
@@ -406,11 +407,13 @@
 
 	/* Now it is safe to release the LRU list mutex. */
 	mutex_exit(&buf_pool->LRU_list_mutex);
+	mutex_exit(&buf_pool->zip_dirty_flush_mutex);
 
 	mutex_exit(block_mutex);
 	/* Try and force a context switch. */
 	os_thread_yield();
 
+	mutex_enter(&buf_pool->zip_dirty_flush_mutex);
 	mutex_enter(&buf_pool->LRU_list_mutex);
 
 	mutex_enter(block_mutex);
@@ -435,6 +438,8 @@
 	ibool*		must_restart)	/*!< in/out: if TRUE, we have to
 					restart the flush list scan */
 {
+	ut_ad(mutex_own(&buf_pool->zip_dirty_flush_mutex));
+
 	/* Every BUF_LRU_DROP_SEARCH_SIZE iterations in the
 	loop we release buf_pool->mutex to let other threads
 	do their job but only if the block is not IO fixed. This
@@ -449,11 +454,6 @@
 
 		buf_flush_list_mutex_exit(buf_pool);
 
-		/* We don't have to worry about bpage becoming a dangling
-		pointer by a compressed page flush list relocation because
-		buf_page_get_gen() won't be called for pages from this
-		tablespace.  */
-
 		block_mutex = buf_page_get_mutex_enter(bpage);
 		if (UNIV_UNLIKELY(block_mutex == NULL)) {
 
@@ -517,10 +517,6 @@
 
 	block_mutex = buf_page_get_mutex(bpage);
 
-	/* bpage->space and bpage->io_fix are protected by
-	buf_pool->mutex and block_mutex. It is safe to check
-	them while holding buf_pool->mutex only. */
-
 	if (UNIV_UNLIKELY(buf_page_get_io_fix_unlocked(bpage)
 			  != BUF_IO_NONE)) {
 
@@ -543,9 +539,10 @@
 
 		mutex_enter(block_mutex);
 
-		/* Recheck the page I/O fix and the flush list presence now
-		thatwe hold the right mutex. */
-		if (UNIV_UNLIKELY(buf_page_get_io_fix(bpage) != BUF_IO_NONE
+		/* Recheck the page type, I/O fix and the flush list presence
+		now that we hold the right mutex. */
+		if (UNIV_UNLIKELY(!buf_page_in_file(bpage)
+				  || buf_page_get_io_fix(bpage) != BUF_IO_NONE
 				  || bpage->oldest_modification == 0)) {
 
 			/* The page became I/O-fixed or is not on the flush
@@ -595,13 +592,21 @@
 	ibool		all_freed = TRUE;
 	ibool		must_restart = FALSE;
 
+	ut_ad(mutex_own(&buf_pool->zip_dirty_flush_mutex));
+
 	buf_flush_list_mutex_enter(buf_pool);
 
 	for (bpage = UT_LIST_GET_LAST(buf_pool->flush_list);
 	     !must_restart && bpage != NULL;
 	     bpage = prev) {
 
-		ut_a(buf_page_in_file(bpage));
+		if (!buf_page_in_file(bpage)
+		    || bpage->oldest_modification == 0) {
+
+			all_freed = FALSE;
+			break;
+		}
+
 		ut_ad(bpage->in_flush_list);
 
 		/* Save the previous link because once we free the
@@ -663,11 +668,13 @@
 	ibool	all_freed;
 
 	do {
+		mutex_enter(&buf_pool->zip_dirty_flush_mutex);
 		mutex_enter(&buf_pool->LRU_list_mutex);
 
 		all_freed = buf_flush_or_remove_pages(buf_pool, id);
 
 		mutex_exit(&buf_pool->LRU_list_mutex);
+		mutex_exit(&buf_pool->zip_dirty_flush_mutex);
 
 		ut_ad(buf_flush_validate(buf_pool));