Tarantool development patches archive
 help / color / mirror / Atom feed
From: Nikita Pettik <korablev@tarantool.org>
To: tarantool-patches@dev.tarantool.org
Cc: v.shpilevoy@tarantool.org
Subject: [Tarantool-patches] [PATCH v3 3/3] vinyl: clean-up write iterator if vy_task_write_run() fails
Date: Mon, 27 Apr 2020 03:52:16 +0300	[thread overview]
Message-ID: <7965217ceed66d448cee453c690e8d91ba7a841b.1587948306.git.korablev@tarantool.org> (raw)
In-Reply-To: <cover.1587948306.git.korablev@tarantool.org>
In-Reply-To: <cover.1587948306.git.korablev@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_scheduler.c                        |  4 +-
 src/box/vy_write_iterator.c                   |  9 ++
 src/errinj.h                                  |  1 +
 test/box/errinj.result                        |  1 +
 .../gh-4864-stmt-alloc-fail-compact.result    | 85 +++++++++++++++++++
 .../gh-4864-stmt-alloc-fail-compact.test.lua  | 44 ++++++++++
 6 files changed, 143 insertions(+), 1 deletion(-)

diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
index 9dba93d34..387f58723 100644
--- a/src/box/vy_scheduler.c
+++ b/src/box/vy_scheduler.c
@@ -1065,8 +1065,10 @@ vy_task_write_run(struct vy_task *task, bool no_compression)
 				 no_compression) != 0)
 		goto fail;
 
-	if (wi->iface->start(wi) != 0)
+	if (wi->iface->start(wi) != 0) {
+		wi->iface->stop(wi);
 		goto fail_abort_writer;
+	}
 	int rc;
 	int loops = 0;
 	struct tuple *stmt = NULL;
diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index efb88d1ae..0b741b3dc 100644
--- a/src/box/vy_write_iterator.c
+++ b/src/box/vy_write_iterator.c
@@ -400,6 +400,15 @@ vy_write_iterator_start(struct vy_stmt_stream *vstream)
 	rlist_foreach_entry_safe(src, &stream->src_list, in_src_list, tmp) {
 		if (vy_write_iterator_add_src(stream, src) != 0)
 			return -1;
+#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");
+			return -1;
+		}
+#endif
 	}
 	return 0;
 }
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..ea8dce0ba 100644
--- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
@@ -242,6 +242,91 @@ 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
+ | ...
+-- Prevent next attempt to compact in a row.
+--
+errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 1)
+ | ---
+ | - ok
+ | ...
+
+s.index.pk:compact()
+ | ---
+ | ...
+-- Leave a time gap between compaction and index drop just in case
+-- (to make sure that compaction is already finished (re-scheduled)
+--  when at the moment of index drop).
+--
+fiber.sleep(0.5)
+ | ---
+ | ...
+
+-- 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")
+ | ---
+ | - 0
+ | ...
+-- Give GC some time to operate on.
+--
+fiber.sleep(1)
+ | ---
+ | ...
+
+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..3c2b38160 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,48 @@ 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)
+-- Prevent next attempt to compact in a row.
+--
+errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 1)
+
+s.index.pk:compact()
+-- Leave a time gap between compaction and index drop just in case
+-- (to make sure that compaction is already finished (re-scheduled)
+--  when at the moment of index drop).
+--
+fiber.sleep(0.5)
+
+-- 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")
+-- Give GC some time to operate on.
+--
+fiber.sleep(1)
+
+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

  parent reply	other threads:[~2020-04-27  0:52 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-27  0:52 [Tarantool-patches] [PATCH v3 0/3] vinyl: fix uninitialized memory accesses Nikita Pettik
2020-04-27  0:52 ` [Tarantool-patches] [PATCH v3 1/3] vinyl: init all vars before cleanup in vy_lsm_split_range() Nikita Pettik
2020-05-06  9:04   ` Aleksandr Lyapunov
2020-05-06 13:12     ` Nikita Pettik
2020-05-06 17:52       ` Aleksandr Lyapunov
2020-05-07  1:09         ` Nikita Pettik
2020-04-27  0:52 ` [Tarantool-patches] [PATCH v3 2/3] vinyl: clean-up unprocessed read views in *_build_read_views() Nikita Pettik
2020-05-06  9:56   ` Aleksandr Lyapunov
2020-05-07  0:29     ` Nikita Pettik
2020-05-07  8:44       ` Aleksandr Lyapunov
2020-05-07 12:28         ` Nikita Pettik
2020-04-27  0:52 ` Nikita Pettik [this message]
2020-05-01  0:55   ` [Tarantool-patches] [PATCH v3 3/3] vinyl: clean-up write iterator if vy_task_write_run() fails Vladislav Shpilevoy
2020-05-03  9:22     ` Konstantin Osipov
2020-05-07  0:38     ` Nikita Pettik
2020-05-06 10:37   ` Aleksandr Lyapunov
2020-05-07  0:36     ` Nikita Pettik
2020-05-07  7:53       ` Aleksandr Lyapunov
2020-05-07 22:16         ` Nikita Pettik
2020-05-08 16:29           ` Aleksandr Lyapunov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7965217ceed66d448cee453c690e8d91ba7a841b.1587948306.git.korablev@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v3 3/3] vinyl: clean-up write iterator if vy_task_write_run() fails' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox