Bug #69146 Needless log flush order mutex acquisition in buf_pool_get_oldest_modification
Submitted: 5 May 2013 7:50 Modified: 3 Jun 2015 10:36
Reporter: Laurynas Biveinis Email Updates:
Status: Verified Impact on me:
None 
Category:MySQL Server: InnoDB storage engine Severity:S5 (Performance)
Version:5.5, 5.6, 5.7 OS:Any
Assigned to: CPU Architecture:Any
Tags: buffer pool, innodb, mutexes

[5 May 2013 7:50] Laurynas Biveinis
Description:
buf_pool_get_oldest_modification() loops over all buffer pool instances, acquiring the flush list mutex for each of them.

Before the loop it enters the log flush order mutex:

	/* When we traverse all the flush lists we don't want another
	thread to add a dirty page to any flush list. */
	log_flush_order_mutex_enter();

In the case of srv_buf_pool_instances == 1 this is unnecessary as the flush list mutex will take care of it

How to repeat:
By code reading.

Suggested fix:
Guard log flush order mutex acquisition by srv_buf_pool_instances == 1 check. Yasufumi added this patch to XtraDB 5.5 starting from day one with no ill effects.
[2 May 2014 5:32] Erlend Dahl
I'll have Yasufumi take a look.
[25 Nov 2014 10:13] Laurynas Biveinis
Actually, taking log flush order mutex in this function seems to be completely redundant for any number of buffer pool instances. All the relevant mutexes are released by the time function quits, making its result potentially outdated. Even without that, if some instance will receive a new dirty page, its first modification LSN will not be older than the result of this function.
[25 Nov 2014 10:14] Laurynas Biveinis
Adjusting title
[25 Nov 2014 11:49] Laurynas Biveinis
Bug 69146 patch for 5.7

(*) I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

Contribution: bug69146.patch (application/octet-stream, text), 1.02 KiB.

[29 May 2015 3:15] Sunny Bains
The single thread case I can see benefiting in some way but the log order mutex is required for correctness. If XtrDB doesn't do the same thing then it has divine blessing :-) is all I can say, or does something which is different from Oracle InnoDB :-)

"r6987 | sbains | 2010-04-14 00:14:13 +0300 (Wed, 14 Apr 2010) | 12 lines
      Changed paths:
         M /branches/innodb+multibp/buf/buf0buf.c
    
      branches/innodb+multibp: When calculating the oldest_lsn we can have a
      situation where we've iterated to say buffer pool 3 and another thread
      adds two new dirty pages, the first to buffer pool 1 and the second to
      buffer pool 4. Up to say buffer pool 3 the oldest_lsn was 0. Now, we will
      end up returning the lsn at buffer pool 4 as the oldest LSN. We prevent this
      by acquiring the flush order mutex.
    
      One other future option is to calculate the min_lsn when flushing pages
      from the list and maintaining a running total using atomics. That way
      we can get rid of this function altogether. The atomics will only really
      be required when we do parallel flushing.
"
[29 May 2015 3:16] Sunny Bains
Should have been "single buffer case" not "single thread case"
[3 Jun 2015 10:36] Laurynas Biveinis
You are right, this function needs the log flush order mutex if running with more than one instance.