Merge lp:~akopytov/percona-server/bug1163262-5.5 into lp:percona-server/5.5

Proposed by Alexey Kopytov
Status: Merged
Approved by: Laurynas Biveinis
Approved revision: 461
Merged at revision: 492
Proposed branch: lp:~akopytov/percona-server/bug1163262-5.5
Merge into: lp:percona-server/5.5
Diff against target: 137 lines (+56/-4)
4 files modified
Percona-Server/storage/innobase/include/buf0flu.ic (+1/-1)
Percona-Server/storage/innobase/include/mtr0mtr.h (+2/-0)
Percona-Server/storage/innobase/include/mtr0mtr.ic (+21/-0)
Percona-Server/storage/innobase/mtr/mtr0mtr.c (+32/-3)
To merge this branch: bzr merge lp:~akopytov/percona-server/bug1163262-5.5
Reviewer Review Type Date Requested Status
Laurynas Biveinis (community) Approve
Registry Administrators Pending
Review via email: mp+156630@code.launchpad.net

Description of the change

    Bug #1163262: Unnecessary log_flush_order_mutex acquisition

    log_sys->log_flush_order_mutex was introduced as an optimization to
    reduce contention on log_sys->mutex. On a mini-transaction commit
    InnoDB adds modified pages to the flush list and uses
    log_sys->log_flush_order_mutex to make sure pages are added to the
    flush list in the correct LSN order.

    One thing that was handled inefficiently wrt. that mutex is that it was
    acquired even if no modifications from a mini-transaction had to be
    added to flush list, i.e. when an mtr has only modified dirty pages
    (if any). This unnecessarily increased contention on
    log_sys->log_flush_order_mutex and consequently on log_sys->mutex in
    write-intensive workloads, because the former is acquired with the
    latter locked.

    The problem is fixed in MySQL 5.6 with the following revision:
    http://lists.mysql.com/commits/134048

    This is a backport of that change to Percona Server 5.5.

http://jenkins.percona.com/view/PS%205.5/job/percona-server-5.5-param/712/

To post a comment you must log in.
Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

The patch introduces a new function mtr_block_dirtied(). Its definition is provided in mtr0log.c, it's a small function, and has only one caller in in mtr_memo_push(), which is defined in mtr0mtr.ic. Surely this means that we want mtr_block_dirtied() defined as a static inline function in mtr0mtr.ic as well?

review: Needs Information
Revision history for this message
Alexey Kopytov (akopytov) wrote :

I noticed that too, but it's a backport of the upstream 5.6 code and it's still defined like that in upstream LP. So I decided to keep things simple and not diverge from the upstream code.

Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

I wonder if the corresponding upstream bug to point this out to them would be "a misaligned source comment" kind of bug or if it's worthwhile to report it.

In any case, looks good, and we can wait for mtr_block_dirtied() call overhead to show up in profiles before diverging.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'Percona-Server/storage/innobase/include/buf0flu.ic'
2--- Percona-Server/storage/innobase/include/buf0flu.ic 2012-05-10 07:49:14 +0000
3+++ Percona-Server/storage/innobase/include/buf0flu.ic 2013-04-02 16:21:25 +0000
4@@ -70,7 +70,7 @@
5
6 ut_ad(!buf_pool_mutex_own(buf_pool));
7 ut_ad(!buf_flush_list_mutex_own(buf_pool));
8- ut_ad(log_flush_order_mutex_own());
9+ ut_ad(!mtr->made_dirty || log_flush_order_mutex_own());
10
11 ut_ad(mtr->start_lsn != 0);
12 ut_ad(mtr->modifications);
13
14=== modified file 'Percona-Server/storage/innobase/include/mtr0mtr.h'
15--- Percona-Server/storage/innobase/include/mtr0mtr.h 2012-02-17 09:52:51 +0000
16+++ Percona-Server/storage/innobase/include/mtr0mtr.h 2013-04-02 16:21:25 +0000
17@@ -375,6 +375,8 @@
18 ibool modifications;
19 /* TRUE if the mtr made modifications to
20 buffer pool pages */
21+ ibool made_dirty;/*!< TRUE if mtr has made at least
22+ one buffer pool page dirty */
23 ulint n_log_recs;
24 /* count of how many page initial log records
25 have been written to the mtr log */
26
27=== modified file 'Percona-Server/storage/innobase/include/mtr0mtr.ic'
28--- Percona-Server/storage/innobase/include/mtr0mtr.ic 2012-03-20 09:44:51 +0000
29+++ Percona-Server/storage/innobase/include/mtr0mtr.ic 2013-04-02 16:21:25 +0000
30@@ -29,6 +29,17 @@
31 #endif /* !UNIV_HOTBACKUP */
32 #include "mach0data.h"
33
34+/***************************************************//**
35+Checks if a mini-transaction is dirtying a clean page.
36+@return TRUE if the mtr is dirtying a clean page. */
37+UNIV_INTERN
38+ibool
39+mtr_block_dirtied(
40+/*==============*/
41+ const buf_block_t* block) /*!< in: block being x-fixed */
42+ __attribute__((nonnull,warn_unused_result));
43+
44+
45 /***************************************************************//**
46 Starts a mini-transaction. */
47 UNIV_INLINE
48@@ -47,6 +58,7 @@
49 mtr->inside_ibuf = FALSE;
50 mtr->n_log_recs = 0;
51 mtr->n_freed_pages = 0;
52+ mtr->made_dirty = FALSE;
53
54 ut_d(mtr->state = MTR_ACTIVE);
55 ut_d(mtr->magic_n = MTR_MAGIC_N);
56@@ -65,6 +77,15 @@
57 dyn_array_t* memo;
58 mtr_memo_slot_t* slot;
59
60+ /* If this mtr has x-fixed a clean page then we set
61+ the made_dirty flag. This tells us if we need to
62+ grab log_flush_order_mutex at mtr_commit so that we
63+ can insert the dirtied page to the flush list. */
64+ if (type == MTR_MEMO_PAGE_X_FIX && !mtr->made_dirty) {
65+ mtr->made_dirty =
66+ mtr_block_dirtied((const buf_block_t *)object);
67+ }
68+
69 ut_ad(object);
70 ut_ad(type >= MTR_MEMO_PAGE_S_FIX);
71 ut_ad(type <= MTR_MEMO_X_LOCK);
72
73=== modified file 'Percona-Server/storage/innobase/mtr/mtr0mtr.c'
74--- Percona-Server/storage/innobase/mtr/mtr0mtr.c 2012-05-10 07:49:14 +0000
75+++ Percona-Server/storage/innobase/mtr/mtr0mtr.c 2013-04-02 16:21:25 +0000
76@@ -37,6 +37,25 @@
77
78 #ifndef UNIV_HOTBACKUP
79 # include "log0recv.h"
80+
81+/***************************************************//**
82+Checks if a mini-transaction is dirtying a clean page.
83+@return TRUE if the mtr is dirtying a clean page. */
84+UNIV_INTERN
85+ibool
86+mtr_block_dirtied(
87+/*==============*/
88+ const buf_block_t* block) /*!< in: block being x-fixed */
89+{
90+ ut_ad(buf_block_get_state(block) == BUF_BLOCK_FILE_PAGE);
91+ ut_ad(block->page.buf_fix_count > 0);
92+
93+ /* It is OK to read oldest_modification because no
94+ other thread can be performing a write of it and it
95+ is only during write that the value is reset to 0. */
96+ return(block->page.oldest_modification == 0);
97+}
98+
99 /*****************************************************************//**
100 Releases the item in the slot given. */
101 static
102@@ -125,7 +144,7 @@
103 buf_block_t* block = (buf_block_t*) slot->object;
104
105 #ifdef UNIV_DEBUG
106- ut_ad(log_flush_order_mutex_own());
107+ ut_ad(!mtr->made_dirty || log_flush_order_mutex_own());
108 #endif /* UNIV_DEBUG */
109 buf_flush_note_modification(block, mtr);
110 }
111@@ -225,7 +244,15 @@
112 mtr->end_lsn = log_close();
113
114 func_exit:
115- log_flush_order_mutex_enter();
116+
117+ /* No need to acquire log_flush_order_mutex if this mtr has
118+ not dirtied a clean page. log_flush_order_mutex is used to
119+ ensure ordered insertions in the flush_list. We need to
120+ insert in the flush_list iff the page in question was clean
121+ before modifications. */
122+ if (mtr->made_dirty) {
123+ log_flush_order_mutex_enter();
124+ }
125
126 /* It is now safe to release the log mutex because the
127 flush_order mutex will ensure that we are the first one
128@@ -236,7 +263,9 @@
129 mtr_memo_note_modifications(mtr);
130 }
131
132- log_flush_order_mutex_exit();
133+ if (mtr->made_dirty) {
134+ log_flush_order_mutex_exit();
135+ }
136 }
137 #endif /* !UNIV_HOTBACKUP */
138

Subscribers

People subscribed via source and target branches