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

Proposed by Laurynas Biveinis on 2014-04-24
Status: Merged
Approved by: Laurynas Biveinis on 2014-05-19
Approved revision: 625
Merged at revision: 661
Proposed branch: lp:~laurynas-biveinis/percona-server/bug1305364-5.5
Merge into: lp:percona-server/5.5
Diff against target: 302 lines (+47/-59)
5 files modified
storage/innobase/buf/buf0buf.c (+9/-1)
storage/innobase/buf/buf0flu.c (+5/-1)
storage/innobase/buf/buf0lru.c (+22/-55)
storage/innobase/buf/buf0rea.c (+6/-2)
storage/innobase/include/buf0buf.ic (+5/-0)
To merge this branch: bzr merge lp:~laurynas-biveinis/percona-server/bug1305364-5.5
Reviewer Review Type Date Requested Status
Vlad Lesin (community) g2 2014-04-24 Approve on 2014-04-28
Review via email: mp+217089@code.launchpad.net

Description of the change

Fix bug 1305364 (InnoDB: Assertion failure in file buf0flu.cc line 546
| crashes if RW workload and InnoDB compression are combined) and bug
1219833
(Redundant page state re-checks in
buf_LRU_free_from_unzip_LRU_list() and
buf_LRU_free_from_common_LRU_list()).

The former bug was fixed by reviewing buf_page_in_file calls and
buf_page_get_state == BUF_BLOCK_REMOVE_HASH checks for locking
correctness. The transition for a buffer page state from
BUF_BLOCK_FILE_PAGE to BUF_BLOCK_REMOVE_HASH is protected by a
combination of locked page_hash_latch, LRU_list_mutex, and block
mutex. Thus, any of the checks above must have locked at least one of
those three sync objects.
- buf_page_get_zip: check if buf_page_get_state is still
  BUF_BLOCK_FILE_PAGE between mutex re-acquisition and
  buf_LRU_free_block call.
- buf_LRU_search_and_free_block: lock LRU list mutex unconditionally,
  so that buf_LRU_free_from_unzip_LRU_list and
  buf_LRU_free_from_common_LRU_list do not attempt unlocked LRU list
  scans anymore.
- buf_read_ahead_random: lock page_hash_latch and do not lock buffer
  pool mutex around buf_page_hash_get mutex. This is in line with
  previous fixes to buf_read_ahead_linear.

Add several asserts to document the required locking better in
buf_pool_watch_is_sentinel, buf_flush_page_and_try_neighbors,
buf_LRU_evict_from_unzip_LRU, buf_flush_or_remove_pages,
buf_LRU_free_from_unzip_LRU_list, buf_LRU_free_from_common_LRU_list,
and buf_page_get_block. Some of these are UNIV_SYNC_DEBUG, thus
nothing more than code comments, because currently UNIV_SYNC_DEBUG
builds are broken.

buf_LRU_search_and_free_block locking LRU list unconditionally enables
removing all the code in its callees that attempts to perform lockless
LRU list scans and is broken, as well as fixing bug 1219833. There
- remove have_LRU_mutex args from buf_LRU_evict_from_unzip_LRU,
  buf_LRU_free_from_unzip_LRU_list, buf_LRU_free_from_common_LRU_list,
  as it's always TRUE.
- remove any have_LRU_mutex checks, page state re-checks, and scan
  restarts from the same functions.

http://jenkins.percona.com/job/percona-server-5.5-param/989/

Not sure if G1 or G2.

To post a comment you must log in.
Vlad Lesin (vlad-lesin) wrote :

I don't see any errors here. The code looks good for me and explanations are reasonable.

review: Approve (g2)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'storage/innobase/buf/buf0buf.c'
2--- storage/innobase/buf/buf0buf.c 2014-03-27 15:42:21 +0000
3+++ storage/innobase/buf/buf0buf.c 2014-04-24 16:37:49 +0000
4@@ -1600,6 +1600,12 @@
5 buf_pool_t* buf_pool, /*!< buffer pool instance */
6 const buf_page_t* bpage) /*!< in: block */
7 {
8+#ifdef UNIV_SYNC_DEBUG
9+ ut_ad(rw_lock_own(&buf_pool->page_hash_latch, RW_LOCK_SHARED)
10+ || rw_lock_own(&buf_pool->page_hash_latch, RW_LOCK_EX)
11+ || mutex_own(buf_page_get_mutex(bpage)));
12+#endif
13+
14 ut_ad(buf_page_in_file(bpage));
15
16 if (bpage < &buf_pool->watch[0]
17@@ -2048,7 +2054,9 @@
18 mutex_enter(&buf_pool->LRU_list_mutex);
19 mutex_enter(block_mutex);
20
21- if (UNIV_UNLIKELY(bpage->space != space
22+ if (UNIV_UNLIKELY((buf_page_get_state(bpage)
23+ != BUF_BLOCK_FILE_PAGE)
24+ || bpage->space != space
25 || bpage->offset != offset
26 || !bpage->in_LRU_list
27 || !bpage->zip.data)) {
28
29=== modified file 'storage/innobase/buf/buf0flu.c'
30--- storage/innobase/buf/buf0flu.c 2014-01-15 14:12:15 +0000
31+++ storage/innobase/buf/buf0flu.c 2014-04-24 16:37:49 +0000
32@@ -1606,7 +1606,11 @@
33 }
34
35 ut_a(buf_page_in_file(bpage)
36- || buf_page_get_state(bpage) == BUF_BLOCK_REMOVE_HASH);
37+ || (buf_page_get_state(bpage) == BUF_BLOCK_REMOVE_HASH
38+#ifdef UNIV_DEBUG
39+ && !mutex_own(&buf_pool->LRU_list_mutex)
40+#endif
41+ ));
42
43 if (buf_flush_ready_for_flush(bpage, flush_type)) {
44 ulint space;
45
46=== modified file 'storage/innobase/buf/buf0lru.c'
47--- storage/innobase/buf/buf0lru.c 2013-10-23 08:23:08 +0000
48+++ storage/innobase/buf/buf0lru.c 2014-04-24 16:37:49 +0000
49@@ -177,20 +177,15 @@
50 ibool
51 buf_LRU_evict_from_unzip_LRU(
52 /*=========================*/
53- buf_pool_t* buf_pool,
54- ibool have_LRU_mutex)
55+ buf_pool_t* buf_pool)
56 {
57 ulint io_avg;
58 ulint unzip_avg;
59
60- //ut_ad(buf_pool_mutex_own(buf_pool));
61+ ut_ad(mutex_own(&buf_pool->LRU_list_mutex));
62
63- if (!have_LRU_mutex)
64- mutex_enter(&buf_pool->LRU_list_mutex);
65 /* If the unzip_LRU list is empty, we can only use the LRU. */
66 if (UT_LIST_GET_LEN(buf_pool->unzip_LRU) == 0) {
67- if (!have_LRU_mutex)
68- mutex_exit(&buf_pool->LRU_list_mutex);
69 return(FALSE);
70 }
71
72@@ -199,20 +194,14 @@
73 decompressed pages in the buffer pool. */
74 if (UT_LIST_GET_LEN(buf_pool->unzip_LRU)
75 <= UT_LIST_GET_LEN(buf_pool->LRU) / 10) {
76- if (!have_LRU_mutex)
77- mutex_exit(&buf_pool->LRU_list_mutex);
78 return(FALSE);
79 }
80
81 /* If eviction hasn't started yet, we assume by default
82 that a workload is disk bound. */
83 if (buf_pool->freed_page_clock == 0) {
84- if (!have_LRU_mutex)
85- mutex_exit(&buf_pool->LRU_list_mutex);
86 return(TRUE);
87 }
88- if (!have_LRU_mutex)
89- mutex_exit(&buf_pool->LRU_list_mutex);
90
91 /* Calculate the average over past intervals, and add the values
92 of the current interval. */
93@@ -612,6 +601,8 @@
94 ibool all_freed = TRUE;
95 ibool must_restart = FALSE;
96
97+ ut_ad(mutex_own(&buf_pool->LRU_list_mutex));
98+
99 buf_flush_list_mutex_enter(buf_pool);
100
101 for (bpage = UT_LIST_GET_LAST(buf_pool->flush_list);
102@@ -930,19 +921,18 @@
103 buf_LRU_free_from_unzip_LRU_list(
104 /*=============================*/
105 buf_pool_t* buf_pool, /*!< in: buffer pool instance */
106- ulint n_iterations, /*!< in: how many times this has
107+ ulint n_iterations) /*!< in: how many times this has
108 been called repeatedly without
109 result: a high value means that
110 we should search farther; we will
111 search n_iterations / 5 of the
112 unzip_LRU list, or nothing if
113 n_iterations >= 5 */
114- ibool have_LRU_mutex)
115 {
116 buf_block_t* block;
117 ulint distance;
118
119- //ut_ad(buf_pool_mutex_own(buf_pool));
120+ ut_ad(mutex_own(&buf_pool->LRU_list_mutex));
121
122 /* Theoratically it should be much easier to find a victim
123 from unzip_LRU as we can choose even a dirty block (as we'll
124@@ -952,7 +942,7 @@
125 if we have done five iterations so far. */
126
127 if (UNIV_UNLIKELY(n_iterations >= 5)
128- || !buf_LRU_evict_from_unzip_LRU(buf_pool, have_LRU_mutex)) {
129+ || !buf_LRU_evict_from_unzip_LRU(buf_pool)) {
130
131 return(FALSE);
132 }
133@@ -960,25 +950,18 @@
134 distance = 100 + (n_iterations
135 * UT_LIST_GET_LEN(buf_pool->unzip_LRU)) / 5;
136
137-restart:
138 for (block = UT_LIST_GET_LAST(buf_pool->unzip_LRU);
139 UNIV_LIKELY(block != NULL) && UNIV_LIKELY(distance > 0);
140 block = UT_LIST_GET_PREV(unzip_LRU, block), distance--) {
141
142 ibool freed;
143
144- mutex_enter(&block->mutex);
145- if (!block->in_unzip_LRU_list || !block->page.in_LRU_list
146- || buf_block_get_state(block) != BUF_BLOCK_FILE_PAGE) {
147- mutex_exit(&block->mutex);
148- goto restart;
149- }
150-
151 ut_ad(buf_block_get_state(block) == BUF_BLOCK_FILE_PAGE);
152 ut_ad(block->in_unzip_LRU_list);
153 ut_ad(block->page.in_LRU_list);
154
155- freed = buf_LRU_free_block(&block->page, FALSE, have_LRU_mutex);
156+ mutex_enter(&block->mutex);
157+ freed = buf_LRU_free_block(&block->page, FALSE, TRUE);
158 mutex_exit(&block->mutex);
159
160 if (freed) {
161@@ -997,46 +980,35 @@
162 buf_LRU_free_from_common_LRU_list(
163 /*==============================*/
164 buf_pool_t* buf_pool,
165- ulint n_iterations,
166+ ulint n_iterations)
167 /*!< in: how many times this has been called
168 repeatedly without result: a high value means
169 that we should search farther; if
170 n_iterations < 10, then we search
171 n_iterations / 10 * buf_pool->curr_size
172 pages from the end of the LRU list */
173- ibool have_LRU_mutex)
174 {
175 buf_page_t* bpage;
176 ulint distance;
177
178- //ut_ad(buf_pool_mutex_own(buf_pool));
179+ ut_ad(mutex_own(&buf_pool->LRU_list_mutex));
180
181 distance = 100 + (n_iterations * buf_pool->curr_size) / 10;
182
183-restart:
184 for (bpage = UT_LIST_GET_LAST(buf_pool->LRU);
185 UNIV_LIKELY(bpage != NULL) && UNIV_LIKELY(distance > 0);
186 bpage = UT_LIST_GET_PREV(LRU, bpage), distance--) {
187
188 ibool freed;
189 unsigned accessed;
190- mutex_t* block_mutex = buf_page_get_mutex_enter(bpage);
191-
192- if (!block_mutex) {
193- goto restart;
194- }
195-
196- if (!bpage->in_LRU_list
197- || !buf_page_in_file(bpage)) {
198- mutex_exit(block_mutex);
199- goto restart;
200- }
201+ mutex_t* block_mutex = buf_page_get_mutex(bpage);
202
203 ut_ad(buf_page_in_file(bpage));
204 ut_ad(bpage->in_LRU_list);
205
206+ mutex_enter(block_mutex);
207 accessed = buf_page_is_accessed(bpage);
208- freed = buf_LRU_free_block(bpage, TRUE, have_LRU_mutex);
209+ freed = buf_LRU_free_block(bpage, TRUE, TRUE);
210 mutex_exit(block_mutex);
211
212 if (freed) {
213@@ -1073,23 +1045,18 @@
214 n_iterations / 5 of the unzip_LRU list. */
215 {
216 ibool freed = FALSE;
217- ibool have_LRU_mutex = FALSE;
218-
219- if (UT_LIST_GET_LEN(buf_pool->unzip_LRU))
220- have_LRU_mutex = TRUE;
221-
222- //buf_pool_mutex_enter(buf_pool);
223- if (have_LRU_mutex)
224- mutex_enter(&buf_pool->LRU_list_mutex);
225-
226- freed = buf_LRU_free_from_unzip_LRU_list(buf_pool, n_iterations, have_LRU_mutex);
227+
228+ mutex_enter(&buf_pool->LRU_list_mutex);
229+
230+ freed = buf_LRU_free_from_unzip_LRU_list(buf_pool, n_iterations);
231
232 if (!freed) {
233 freed = buf_LRU_free_from_common_LRU_list(
234- buf_pool, n_iterations, have_LRU_mutex);
235+ buf_pool, n_iterations);
236 }
237
238 buf_pool_mutex_enter(buf_pool);
239+
240 if (!freed) {
241 buf_pool->LRU_flush_ended = 0;
242 } else if (buf_pool->LRU_flush_ended > 0) {
243@@ -1097,8 +1064,8 @@
244 }
245
246 buf_pool_mutex_exit(buf_pool);
247- if (have_LRU_mutex)
248- mutex_exit(&buf_pool->LRU_list_mutex);
249+
250+ mutex_exit(&buf_pool->LRU_list_mutex);
251
252 return(freed);
253 }
254
255=== modified file 'storage/innobase/buf/buf0rea.c'
256--- storage/innobase/buf/buf0rea.c 2013-08-14 03:52:04 +0000
257+++ storage/innobase/buf/buf0rea.c 2014-04-24 16:37:49 +0000
258@@ -340,9 +340,13 @@
259 return(0);
260 }
261
262+ buf_pool_mutex_exit(buf_pool);
263+
264 /* Count how many blocks in the area have been recently accessed,
265 that is, reside near the start of the LRU list. */
266
267+ rw_lock_s_lock(&buf_pool->page_hash_latch);
268+
269 for (i = low; i < high; i++) {
270 const buf_page_t* bpage =
271 buf_page_hash_get(buf_pool, space, i);
272@@ -356,13 +360,13 @@
273 if (recent_blocks
274 >= BUF_READ_AHEAD_RANDOM_THRESHOLD(buf_pool)) {
275
276- buf_pool_mutex_exit(buf_pool);
277+ rw_lock_s_unlock(&buf_pool->page_hash_latch);
278 goto read_ahead;
279 }
280 }
281 }
282
283- buf_pool_mutex_exit(buf_pool);
284+ rw_lock_s_unlock(&buf_pool->page_hash_latch);
285 /* Do nothing */
286 return(0);
287
288
289=== modified file 'storage/innobase/include/buf0buf.ic'
290--- storage/innobase/include/buf0buf.ic 2013-05-23 08:39:28 +0000
291+++ storage/innobase/include/buf0buf.ic 2014-04-24 16:37:49 +0000
292@@ -689,6 +689,11 @@
293 /*===============*/
294 buf_page_t* bpage) /*!< in: control block, or NULL */
295 {
296+#ifdef UNIV_SYNC_DEBUG
297+ buf_pool_t* buf_pool = buf_pool_from_bpage(bpage);
298+ ut_ad(rw_lock_own(&buf_pool->page_hash_latch, RW_LOCK_SHARED)
299+ || rw_lock_own(&buf_pool->page_hash_latch, RW_LOCK_EX));
300+#endif
301 if (UNIV_LIKELY(bpage != NULL)) {
302 ut_ad(buf_page_in_file(bpage));
303

Subscribers

People subscribed via source and target branches