bzr commit into mysql-trunk-innodb branch (marko.makela:3578)

[marko.makela]

#At file:///home/marko/innobase/dev/mysql2a/5.6-innodb/ based on revid:jimmy.yang@strippedb6imzp2tyl

 3578 Marko Mäkelä	2011-04-13
      Remove race conditions in debug code.
      
      In earlier versions of InnoDB, the debug flag buf_page_t::file_page_was_freed
      is protected by the buffer pool mutex. In mysql-trunk, it was not properly
      protected by any mutex, after the buf_pool->page_hash rw-lock offloaded some
      work from the buffer pool mutex.
      
      Protect buf_page_t::file_page_was_freed with the block mutex.
      
      rb:647 approved by Jimmy Yang

    modified:
      storage/innobase/buf/buf0buf.c
      storage/innobase/include/buf0buf.h
=== modified file 'storage/innobase/buf/buf0buf.c'
--- a/storage/innobase/buf/buf0buf.c	revid:jimmy.yang@stripped
+++ b/storage/innobase/buf/buf0buf.c	revid:[e-mail]-20110413075948-kvytmc37ye1nt7d9
@@ -2366,11 +2366,14 @@ buf_page_set_file_page_was_freed(
 					   &hash_lock);
 
 	if (bpage) {
+		mutex_t*	block_mutex = buf_page_get_mutex(bpage);
 		ut_ad(!buf_pool_watch_is_sentinel(buf_pool, bpage));
+		mutex_enter(block_mutex);
+		rw_lock_s_unlock(hash_lock);
 		/* bpage->file_page_was_freed can already hold
 		when this code is invoked from dict_drop_index_tree() */
 		bpage->file_page_was_freed = TRUE;
-		rw_lock_s_unlock(hash_lock);
+		mutex_exit(block_mutex);
 	}
 
 	return(bpage);
@@ -2396,9 +2399,12 @@ buf_page_reset_file_page_was_freed(
 	bpage = buf_page_hash_get_s_locked(buf_pool, space, offset,
 					   &hash_lock);
 	if (bpage) {
+		mutex_t*	block_mutex = buf_page_get_mutex(bpage);
 		ut_ad(!buf_pool_watch_is_sentinel(buf_pool, bpage));
-		bpage->file_page_was_freed = FALSE;
+		mutex_enter(block_mutex);
 		rw_lock_s_unlock(hash_lock);
+		bpage->file_page_was_freed = FALSE;
+		mutex_exit(block_mutex);
 	}
 
 	return(bpage);
@@ -2534,13 +2540,12 @@ got_block:
 	access_time = buf_page_is_accessed(bpage);
 
 	rw_lock_s_unlock(hash_lock);
-	mutex_exit(block_mutex);
-
-	buf_page_set_accessed_make_young(bpage, access_time);
-
 #if defined UNIV_DEBUG_FILE_ACCESSES || defined UNIV_DEBUG
 	ut_a(!bpage->file_page_was_freed);
 #endif
+	mutex_exit(block_mutex);
+
+	buf_page_set_accessed_make_young(bpage, access_time);
 
 #if defined UNIV_DEBUG || defined UNIV_BUF_DEBUG
 	ut_a(++buf_dbg_counter % 5771 || buf_validate());
@@ -3279,7 +3284,9 @@ wait_until_unfixed:
 #endif /* UNIV_DEBUG || UNIV_IBUF_DEBUG */
 
 	buf_block_buf_fix_inc(block, file, line);
-
+#if defined UNIV_DEBUG_FILE_ACCESSES || defined UNIV_DEBUG
+	ut_a(!block->page.file_page_was_freed);
+#endif
 	mutex_exit(&block->mutex);
 
 	/* Check if this is the first access to the page */
@@ -3290,10 +3297,6 @@ wait_until_unfixed:
 		buf_page_set_accessed_make_young(&block->page, access_time);
 	}
 
-#if defined UNIV_DEBUG_FILE_ACCESSES || defined UNIV_DEBUG
-	ut_a(!block->page.file_page_was_freed);
-#endif
-
 #if defined UNIV_DEBUG || defined UNIV_BUF_DEBUG
 	ut_a(++buf_dbg_counter % 5771 || buf_validate());
 	ut_a(block->page.buf_fix_count > 0);
@@ -3370,8 +3373,7 @@ buf_page_optimistic_get(
 /*====================*/
 	ulint		rw_latch,/*!< in: RW_S_LATCH, RW_X_LATCH */
 	buf_block_t*	block,	/*!< in: guessed buffer block */
-	ib_uint64_t	modify_clock,/*!< in: modify clock value if mode is
-				..._GUESS_ON_CLOCK */
+	ib_uint64_t	modify_clock,/*!< in: modify clock value */
 	const char*	file,	/*!< in: file name */
 	ulint		line,	/*!< in: line where called */
 	mtr_t*		mtr)	/*!< in: mini-transaction */
@@ -3455,8 +3457,11 @@ buf_page_optimistic_get(
 #endif /* UNIV_DEBUG || UNIV_BUF_DEBUG */
 
 #if defined UNIV_DEBUG_FILE_ACCESSES || defined UNIV_DEBUG
-	ut_a(block->page.file_page_was_freed == FALSE);
+	mutex_enter(&block->mutex);
+	ut_a(!block->page.file_page_was_freed);
+	mutex_exit(&block->mutex);
 #endif
+
 	if (UNIV_UNLIKELY(!access_time)) {
 		/* In the case of a first access, try to apply linear
 		read-ahead */
@@ -3568,7 +3573,9 @@ buf_page_get_known_nowait(
 	ut_a(buf_block_get_state(block) == BUF_BLOCK_FILE_PAGE);
 #endif /* UNIV_DEBUG || UNIV_BUF_DEBUG */
 #if defined UNIV_DEBUG_FILE_ACCESSES || defined UNIV_DEBUG
-	ut_a(block->page.file_page_was_freed == FALSE);
+	mutex_enter(&block->mutex);
+	ut_a(!block->page.file_page_was_freed);
+	mutex_exit(&block->mutex);
 #endif
 
 #ifdef UNIV_IBUF_COUNT_DEBUG
@@ -3657,7 +3664,9 @@ buf_page_try_get_func(
 	ut_a(buf_block_get_state(block) == BUF_BLOCK_FILE_PAGE);
 #endif /* UNIV_DEBUG || UNIV_BUF_DEBUG */
 #if defined UNIV_DEBUG_FILE_ACCESSES || defined UNIV_DEBUG
-	ut_a(block->page.file_page_was_freed == FALSE);
+	mutex_enter(&block->mutex);
+	ut_a(!block->page.file_page_was_freed);
+	mutex_exit(&block->mutex);
 #endif /* UNIV_DEBUG_FILE_ACCESSES || UNIV_DEBUG */
 	buf_block_dbg_add_level(block, SYNC_NO_ORDER_CHECK);
 

=== modified file 'storage/innobase/include/buf0buf.h'
--- a/storage/innobase/include/buf0buf.h	revid:jimmy.yang@stripped
+++ b/storage/innobase/include/buf0buf.h	revid:[e-mail]-20110413075948-kvytmc37ye1nt7d9
@@ -331,8 +331,7 @@ buf_page_optimistic_get(
 /*====================*/
 	ulint		rw_latch,/*!< in: RW_S_LATCH, RW_X_LATCH */
 	buf_block_t*	block,	/*!< in: guessed block */
-	ib_uint64_t	modify_clock,/*!< in: modify clock value if mode is
-				..._GUESS_ON_CLOCK */
+	ib_uint64_t	modify_clock,/*!< in: modify clock value */
 	const char*	file,	/*!< in: file name */
 	ulint		line,	/*!< in: line where called */
 	mtr_t*		mtr);	/*!< in: mini-transaction */
@@ -1496,8 +1495,10 @@ struct buf_page_struct{
 	/* @} */
 # if defined UNIV_DEBUG_FILE_ACCESSES || defined UNIV_DEBUG
 	ibool		file_page_was_freed;
-					/*!< this is set to TRUE when fsp
-					frees a page in buffer pool */
+					/*!< this is set to TRUE when
+					fsp frees a page in buffer pool;
+					protected by buf_pool->zip_mutex
+					or buf_block_struct::mutex. */
 # endif /* UNIV_DEBUG_FILE_ACCESSES || UNIV_DEBUG */
 #endif /* !UNIV_HOTBACKUP */
 };

Attachment: [text/bzr-bundle] bzr/[e-mail]