Fix page cache descriptor race condition (#6202)
Markos Fountoulakis committed
Jun 4, 2019 at 11:00 UTC
2f7b222ff1487f32ee9621bc56968ede0995ad3b
2 files changed
+64
-46
database/engine/pagecache.c
+23
-29
@@ -368,43 +368,37 @@ void pg_cache_punch_hole(struct rrdengine_instance *ctx, struct rrdeng_page_desc
368
--pg_cache->page_descriptors;
369
uv_rwlock_wrunlock(&pg_cache->pg_cache_rwlock);
370
371
- if (0 != descr->pg_cache_descr_state) {
372
- /* there is page cache descriptor state under possible contention */
373
-
374
- rrdeng_page_descr_mutex_lock(ctx, descr);
375
- pg_cache_descr = descr->pg_cache_descr;
376
- while (!pg_cache_try_get_unsafe(descr, 1)) {
377
- debug(D_RRDENGINE, "%s: Waiting for locked page:", __func__);
371
+ rrdeng_page_descr_mutex_lock(ctx, descr);
372
+ pg_cache_descr = descr->pg_cache_descr;
373
+ while (!pg_cache_try_get_unsafe(descr, 1)) {
374
+ debug(D_RRDENGINE, "%s: Waiting for locked page:", __func__);
375
+ if (unlikely(debug_flags & D_RRDENGINE))
376
+ print_page_cache_descr(descr);
377
+ pg_cache_wait_event_unsafe(descr);
378
+ }
379
+ if (!remove_dirty) {
380
+ /* even a locked page could be dirty */
381
+ while (unlikely(pg_cache_descr->flags & RRD_PAGE_DIRTY)) {
382
+ debug(D_RRDENGINE, "%s: Found dirty page, waiting for it to be flushed:", __func__);
383
if (unlikely(debug_flags & D_RRDENGINE))
384
print_page_cache_descr(descr);
385
pg_cache_wait_event_unsafe(descr);
386
}
382
- if (!remove_dirty) {
383
- /* even a locked page could be dirty */
384
- while (unlikely(pg_cache_descr->flags & RRD_PAGE_DIRTY)) {
385
- debug(D_RRDENGINE, "%s: Found dirty page, waiting for it to be flushed:", __func__);
386
- if (unlikely(debug_flags & D_RRDENGINE))
387
- print_page_cache_descr(descr);
388
- pg_cache_wait_event_unsafe(descr);
389
- }
390
- }
391
- if (pg_cache_descr->flags & RRD_PAGE_POPULATED) {
392
- /* only after locking can it be safely deleted from LRU */
393
- pg_cache_replaceQ_delete(ctx, descr);
387
+ }
388
+ rrdeng_page_descr_mutex_unlock(ctx, descr);
389
395
- uv_rwlock_wrlock(&pg_cache->pg_cache_rwlock);
396
- pg_cache_evict_unsafe(ctx, descr);
397
- uv_rwlock_wrunlock(&pg_cache->pg_cache_rwlock);
398
- }
399
- pg_cache_put_unsafe(descr);
400
- rrdeng_page_descr_mutex_unlock(ctx, descr);
390
+ if (pg_cache_descr->flags & RRD_PAGE_POPULATED) {
391
+ /* only after locking can it be safely deleted from LRU */
392
+ pg_cache_replaceQ_delete(ctx, descr);
393
402
- rrdeng_destroy_pg_cache_descr(ctx, pg_cache_descr);
394
+ uv_rwlock_wrlock(&pg_cache->pg_cache_rwlock);
395
+ pg_cache_evict_unsafe(ctx, descr);
396
+ uv_rwlock_wrunlock(&pg_cache->pg_cache_rwlock);
397
}
398
+ pg_cache_put(ctx, descr);
399
+
400
+ rrdeng_destroy_pg_cache_descr(ctx, pg_cache_descr);
401
destroy:
405
- if (!remove_dirty) {
406
- assert(0 == descr->pg_cache_descr_state);
407
- }
402
freez(descr);
403
pg_cache_update_metric_times(page_index);
404
}
database/engine/rrdenglocking.c
+41
-17
@@ -40,9 +40,7 @@ void rrdeng_page_descr_mutex_lock(struct rrdengine_instance *ctx, struct rrdeng_
40
41
if (unlikely(we_locked)) {
42
assert(old_state & PG_CACHE_DESCR_LOCKED);
43
- new_state = (1 << PG_CACHE_DESCR_SHIFT) | (old_state & PG_CACHE_DESCR_FLAGS_MASK);
44
- new_state &= ~PG_CACHE_DESCR_LOCKED;
45
- new_state |= PG_CACHE_DESCR_ALLOCATED;
43
+ new_state = (1 << PG_CACHE_DESCR_SHIFT) | PG_CACHE_DESCR_ALLOCATED;
44
ret_state = ulong_compare_and_swap(&descr->pg_cache_descr_state, old_state, new_state);
45
if (old_state == ret_state) {
46
/* success */
@@ -87,7 +85,7 @@ void rrdeng_page_descr_mutex_lock(struct rrdengine_instance *ctx, struct rrdeng_
85
}
86
87
if (pg_cache_descr) {
90
- free(pg_cache_descr);
88
+ rrdeng_destroy_pg_cache_descr(ctx, pg_cache_descr);
89
}
90
pg_cache_descr = descr->pg_cache_descr;
91
uv_mutex_lock(&pg_cache_descr->mutex);
@@ -158,24 +156,47 @@ void rrdeng_try_deallocate_pg_cache_descr(struct rrdengine_instance *ctx, struct
156
{
157
unsigned long old_state, new_state, ret_state, old_users;
158
struct page_cache_descr *pg_cache_descr;
161
- uint8_t we_locked;
159
+ uint8_t just_locked, we_freed, must_unlock;
160
163
- we_locked = 0;
161
+ just_locked = 0;
162
+ we_freed = 0;
163
+ must_unlock = 0;
164
while (1) { /* spin */
165
old_state = descr->pg_cache_descr_state;
166
old_users = old_state >> PG_CACHE_DESCR_SHIFT;
167
168
- if (unlikely(we_locked)) {
168
+ if (unlikely(just_locked)) {
169
assert(0 == old_users);
170
171
- ret_state = ulong_compare_and_swap(&descr->pg_cache_descr_state, old_state, 0);
172
- if (old_state == ret_state) {
171
+ must_unlock = 1;
172
+ just_locked = 0;
173
+ /* Try deallocate if there are no pending references on the page */
174
+ if (!pg_cache_descr->flags && !pg_cache_descr->refcnt) {
175
+ rrdeng_destroy_pg_cache_descr(ctx, pg_cache_descr);
176
+ we_freed = 1;
177
/* success */
174
- break;
178
+ continue;
179
+ }
180
+ continue; /* spin */
181
+ }
182
+ if (unlikely(must_unlock)) {
183
+ assert(0 == old_users);
184
+
185
+ if (we_freed) {
186
+ /* success */
187
+ new_state = 0;
188
+ } else {
189
+ new_state = old_state | PG_CACHE_DESCR_DESTROY;
190
+ new_state &= ~PG_CACHE_DESCR_LOCKED;
191
+ }
192
+ ret_state = ulong_compare_and_swap(&descr->pg_cache_descr_state, old_state, new_state);
193
+ if (old_state == ret_state) {
194
+ /* unlocked */
195
+ return;
196
}
197
continue; /* spin */
198
}
178
- if (!(old_state & PG_CACHE_DESCR_ALLOCATED) || (old_state & PG_CACHE_DESCR_DESTROY)) {
199
+ if (!(old_state & PG_CACHE_DESCR_ALLOCATED)) {
200
/* don't do anything */
201
return;
202
}
@@ -184,25 +205,28 @@ void rrdeng_try_deallocate_pg_cache_descr(struct rrdengine_instance *ctx, struct
205
continue; /* spin */
206
}
207
pg_cache_descr = descr->pg_cache_descr;
187
- /* caller is the only page cache descriptor user and there are no pending references on the page */
188
- if ((0 == old_users) && !pg_cache_descr->flags && !pg_cache_descr->refcnt) {
189
- new_state = PG_CACHE_DESCR_LOCKED;
208
+ /* caller is the only page cache descriptor user */
209
+ if (0 == old_users) {
210
+ new_state = old_state | PG_CACHE_DESCR_LOCKED;
211
ret_state = ulong_compare_and_swap(&descr->pg_cache_descr_state, old_state, new_state);
212
if (old_state == ret_state) {
192
- we_locked = 1;
193
- rrdeng_destroy_pg_cache_descr(ctx, pg_cache_descr);
213
+ just_locked = 1;
214
/* retry */
215
continue;
216
}
217
continue; /* spin */
218
}
219
+ if (old_state & PG_CACHE_DESCR_DESTROY) {
220
+ /* don't do anything */
221
+ return;
222
+ }
223
/* plant PG_CACHE_DESCR_DESTROY so that other contexts eventually free the page cache descriptor */
224
new_state = old_state | PG_CACHE_DESCR_DESTROY;
225
226
ret_state = ulong_compare_and_swap(&descr->pg_cache_descr_state, old_state, new_state);
227
if (old_state == ret_state) {
228
/* success */
205
- break;
229
+ return;
230
}
231
/* spin */
232
}