Merge lp:~laurynas-biveinis/percona-server/bug1083058-5.1 into lp:percona-server/5.1

[Laurynas Biveinis]

Fix bug 1083058 for 5.1.
http://jenkins.percona.com/job/percona-server-5.1-param/484/
No BT or ST, but blocking BT 16274 QA work.

[Laurynas Biveinis (laurynas-biveinis)]

Lost part of the fix in the merge :(

review: Needs Fixing

[Laurynas Biveinis (laurynas-biveinis)]

...and made the previous comment on the wrong MP.

[Laurynas Biveinis (laurynas-biveinis)]

review: Abstain

[Stewart Smith (stewart)]

review: Approve

[Sergei Glushchenko (sergei.glushchenko)]

Laurynas,

I'm not clearly understand why the loop (bpage != NULL) is needed in block @@ -3154,28 +3154,24 @@
And it seems to me that your change gives not the same result as the code before the change.
Before we tried to find page with same space and offset as stored in prev_flush_info.
Depending on was page found or not, we had as output:

found:
  blocks_num == length of flush_list before we started scan
  new_blocks_num == the number of flush_list entries we scanned by going all the way from beginning to the end (list length after we finished scan?)

not found:
  blocks_num == new_blocks_num == length of flush_list before we started scan

It seems to me that if we take flush_list_mutex new_blocks_num should always be equal to blocks_num and

flushed_blocks_num = prev_flush_info.count

and loop above is just burning CPU cycles with no reason for it.

You changed the meaning of new_blocks_num to number of items we scanned before found the match. Was it intentional?

Also there are only few lines between releasing flush_list_mutex and grabbing it again in lines 3174 and 3182 and they are should be executed very quickly (just simple math), so dealing with mutex is much more expensive than that lines.

review: Needs Information (g2)

[Sergei Glushchenko (sergei.glushchenko)]

Sorry, didn't notice that the break in the loop was there before this MP :) So the only notice is why not to remove mutex_exit(&flush_list_mutex); before flushed_blocks_num and mutex_enter few lines further.

[Laurynas Biveinis (laurynas-biveinis)]

It gives a chance for other threads to grab the flush list mutex and run.

[Sergei Glushchenko (sergei.glushchenko)]

OK, but code in lines 3182-3190 again is the few assignments which should be done very fast. Does it make sense to release and grab mutex again to execute very this piece of code?

[Laurynas Biveinis (laurynas-biveinis)]

Indeed.

review: Needs Fixing

[Laurynas Biveinis (laurynas-biveinis)]

Too late, the 5.5 branch has been merged. I'll file a bug.

review: Approve

=== modified file 'Percona-Server/storage/innodb_plugin/srv/srv0srv.c'
--- Percona-Server/storage/innodb_plugin/srv/srv0srv.c	2012-12-14 22:29:39 +0000
+++ Percona-Server/storage/innodb_plugin/srv/srv0srv.c	2013-01-08 12:04:26 +0000
@@ -3154,28 +3154,24 @@
 
 				/* prev_flush_info should be the previous loop's */
 				{
-					lint	blocks_num, new_blocks_num, flushed_blocks_num;
-					ibool	found;
+					lint	blocks_num, new_blocks_num = 0;
+					lint	flushed_blocks_num;
 
+					mutex_enter(&flush_list_mutex);
 					blocks_num = UT_LIST_GET_LEN(buf_pool->flush_list);
 					bpage = UT_LIST_GET_FIRST(buf_pool->flush_list);
-					new_blocks_num = 0;
 
-					found = FALSE;
 					while (bpage != NULL) {
 						if (prev_flush_info.space == bpage->space
 						    && prev_flush_info.offset == bpage->offset
 						    && prev_flush_info.oldest_modification
 								== bpage->oldest_modification) {
-							found = TRUE;
 							break;
 						}
 						bpage = UT_LIST_GET_NEXT(flush_list, bpage);
 						new_blocks_num++;
 					}
-					if (!found) {
-						new_blocks_num = blocks_num;
-					}
+					mutex_exit(&flush_list_mutex);
 
 					flushed_blocks_num = new_blocks_num + prev_flush_info.count
 								- blocks_num;
@@ -3183,6 +3179,7 @@
 						flushed_blocks_num = 0;
 					}
 
+					mutex_enter(&flush_list_mutex);
 					bpage = UT_LIST_GET_FIRST(buf_pool->flush_list);
 
 					prev_flush_info.count = UT_LIST_GET_LEN(buf_pool->flush_list);
@@ -3190,7 +3187,9 @@
 						prev_flush_info.space = bpage->space;
 						prev_flush_info.offset = bpage->offset;
 						prev_flush_info.oldest_modification = bpage->oldest_modification;
+						mutex_exit(&flush_list_mutex);
 					} else {
+						mutex_exit(&flush_list_mutex);
 						prev_flush_info.space = 0;
 						prev_flush_info.offset = 0;
 						prev_flush_info.oldest_modification = 0;
@@ -3216,6 +3215,7 @@
 			} else {
 				/* store previous first pages of the flush_list */
 				{
+					mutex_enter(&flush_list_mutex);
 					bpage = UT_LIST_GET_FIRST(buf_pool->flush_list);
 
 					prev_flush_info.count = UT_LIST_GET_LEN(buf_pool->flush_list);
@@ -3223,7 +3223,9 @@
 						prev_flush_info.space = bpage->space;
 						prev_flush_info.offset = bpage->offset;
 						prev_flush_info.oldest_modification = bpage->oldest_modification;
+						mutex_exit(&flush_list_mutex);
 					} else {
+						mutex_exit(&flush_list_mutex);
 						prev_flush_info.space = 0;
 						prev_flush_info.offset = 0;
 						prev_flush_info.oldest_modification = 0;