From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp21.mail.ru (smtp21.mail.ru [94.100.179.250]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 6FF4D469711 for ; Thu, 7 May 2020 04:10:12 +0300 (MSK) From: Nikita Pettik Date: Thu, 7 May 2020 04:10:09 +0300 Message-Id: <798a7296d58b65ded6d04e9087f5b3c01c717f5a.1588812793.git.korablev@tarantool.org> In-Reply-To: References: In-Reply-To: References: Subject: [Tarantool-patches] [PATCH v4 2/2] vinyl: clean-up write iterator if vy_task_write_run() fails List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org Cc: v.shpilevoy@tarantool.org In vy_task_write_run if write_iterator->start() fails, there's no call of corresponding stop() method. vy_task_write_run() is executed in auxiliary thread (dump or compaction). Meanwhile, creating/destroying tuples in these threads does not change reference counter of corresponding tuple formats (see vy_tuple_delete() and vy_stmt_alloc()). Without cleaning up write iterator right in vy_task_write_run() after fail, this procedure takes place in vy_task_compaction_abort() and vy_task_dump_abort(). These *_abort() functions in turn are executed in the main thread. Taking this into consideration, tuple may be allocated in aux. thread and deleted in the main thread. As a result, format reference counter decreases, whereas it shouldn't change (otherwise tuple format will be destroyed before all tuples of this format are gone). Real example of the bug described above can be achieved in the following way: 1. run compaction process; 2. add one or more slice sources in vy_write_iterator_start(): corresponding slice_stream structures obtain newly created tuples in vy_slice_stream_next(); 3. the next call of vy_write_iterator_add_src() fails due to OOM, invalid run file or whatever; 4. since there's no clean-up of tuples in slice streams, they are destroyed in vy_task_compaction_abort() in the main thread exit; 5. now format reference counter is less than it was before compaction. Closes #4864 --- src/box/vy_write_iterator.c | 16 +++- src/errinj.h | 1 + test/box/errinj.result | 1 + .../gh-4864-stmt-alloc-fail-compact.result | 80 +++++++++++++++++++ .../gh-4864-stmt-alloc-fail-compact.test.lua | 40 ++++++++++ 5 files changed, 137 insertions(+), 1 deletion(-) diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c index 7784dd13a..33ad5ed51 100644 --- a/src/box/vy_write_iterator.c +++ b/src/box/vy_write_iterator.c @@ -401,9 +401,23 @@ vy_write_iterator_start(struct vy_stmt_stream *vstream) struct vy_write_src *src, *tmp; rlist_foreach_entry_safe(src, &stream->src_list, in_src_list, tmp) { if (vy_write_iterator_add_src(stream, src) != 0) - return -1; + goto fail; +#ifndef NDEBUG + struct errinj *inj = + errinj(ERRINJ_VY_WRITE_ITERATOR_START_FAIL, ERRINJ_BOOL); + if (inj != NULL && inj->bparam) { + inj->bparam = false; + diag_set(OutOfMemory, 666, "malloc", "struct vy_stmt"); + goto fail; + } +#endif } return 0; +fail: + /* Clean-up all previously added sources. */ + rlist_foreach_entry_safe(src, &stream->src_list, in_src_list, tmp) + vy_write_iterator_delete_src(stream, src); + return -1; } /** diff --git a/src/errinj.h b/src/errinj.h index b7550bb5e..8562aab1c 100644 --- a/src/errinj.h +++ b/src/errinj.h @@ -129,6 +129,7 @@ struct errinj { _(ERRINJ_INDEX_RESERVE, ERRINJ_BOOL, {.bparam = false})\ _(ERRINJ_VY_STMT_ALLOC, ERRINJ_INT, {.iparam = -1})\ _(ERRINJ_VY_READ_VIEW_MERGE_FAIL, ERRINJ_BOOL, {.bparam = false})\ + _(ERRINJ_VY_WRITE_ITERATOR_START_FAIL, ERRINJ_BOOL, {.bparam = false})\ ENUM0(errinj_id, ERRINJ_LIST); extern struct errinj errinjs[]; diff --git a/test/box/errinj.result b/test/box/errinj.result index e1b9fbe2a..2a87b5f33 100644 --- a/test/box/errinj.result +++ b/test/box/errinj.result @@ -78,6 +78,7 @@ evals - ERRINJ_VY_SQUASH_TIMEOUT: 0 - ERRINJ_VY_STMT_ALLOC: -1 - ERRINJ_VY_TASK_COMPLETE: false + - ERRINJ_VY_WRITE_ITERATOR_START_FAIL: false - ERRINJ_WAL_BREAK_LSN: -1 - ERRINJ_WAL_DELAY: false - ERRINJ_WAL_FALLOCATE: 0 diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result index af116a4b4..133afab1c 100644 --- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result @@ -242,6 +242,86 @@ s:drop() | --- | ... +-- Make sure that there's no extra format unref due to tuple +-- clean-up in the main thread. To achieve this let's sabotage +-- compaction process and delete all tuples: in case ref/unref +-- is the same, format will be deleted alongside with the last +-- tuple. +-- +s = box.schema.space.create('test', {engine = 'vinyl'}) + | --- + | ... +_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024}) + | --- + | ... + +dump(true) + | --- + | ... +dump() + | --- + | ... + +compact() + | --- + | ... + +dump() + | --- + | ... +assert(s.index.pk:stat().range_count == 1) + | --- + | - true + | ... +assert(s.index.pk:stat().run_count == 2) + | --- + | - true + | ... + +errinj.set('ERRINJ_VY_WRITE_ITERATOR_START_FAIL', true) + | --- + | - ok + | ... +errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.1) + | --- + | - ok + | ... +tasks_completed = box.stat.vinyl().scheduler.tasks_completed + | --- + | ... +s.index.pk:compact() + | --- + | ... +-- Tuple clean-up takes place after compaction is completed. +-- Meanwhile range count is updated during compaction process. +-- So instead of relying on range/run match, let's check explicitly +-- number of completed tasks. +-- +repeat fiber.sleep(0.001) until box.stat.vinyl().scheduler.tasks_completed >= tasks_completed + 1 + | --- + | ... + +-- Drop is required to unref all tuples. +-- +s:drop() + | --- + | ... +-- After index is dropped, not all tuples are deallocated at once: +-- they may be still referenced (while being pushed) in Lua. So +-- invoke GC explicitly. +-- +_ = collectgarbage("collect") + | --- + | ... + +assert(errinj.get('ERRINJ_VY_WRITE_ITERATOR_START_FAIL') == false) + | --- + | - true + | ... +errinj.set('ERRINJ_VY_WRITE_ITERATOR_START_FAIL', false) + | --- + | - ok + | ... errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0) | --- | - ok diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua index a68c73d32..547ab628e 100644 --- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua @@ -105,4 +105,44 @@ assert(errinj.get('ERRINJ_VY_READ_VIEW_MERGE_FAIL') == false) errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', false) s:drop() +-- Make sure that there's no extra format unref due to tuple +-- clean-up in the main thread. To achieve this let's sabotage +-- compaction process and delete all tuples: in case ref/unref +-- is the same, format will be deleted alongside with the last +-- tuple. +-- +s = box.schema.space.create('test', {engine = 'vinyl'}) +_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024}) + +dump(true) +dump() + +compact() + +dump() +assert(s.index.pk:stat().range_count == 1) +assert(s.index.pk:stat().run_count == 2) + +errinj.set('ERRINJ_VY_WRITE_ITERATOR_START_FAIL', true) +errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.1) +tasks_completed = box.stat.vinyl().scheduler.tasks_completed +s.index.pk:compact() +-- Tuple clean-up takes place after compaction is completed. +-- Meanwhile range count is updated during compaction process. +-- So instead of relying on range/run match, let's check explicitly +-- number of completed tasks. +-- +repeat fiber.sleep(0.001) until box.stat.vinyl().scheduler.tasks_completed >= tasks_completed + 1 + +-- Drop is required to unref all tuples. +-- +s:drop() +-- After index is dropped, not all tuples are deallocated at once: +-- they may be still referenced (while being pushed) in Lua. So +-- invoke GC explicitly. +-- +_ = collectgarbage("collect") + +assert(errinj.get('ERRINJ_VY_WRITE_ITERATOR_START_FAIL') == false) +errinj.set('ERRINJ_VY_WRITE_ITERATOR_START_FAIL', false) errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0) -- 2.17.1