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 18BDA469710 for ; Thu, 7 May 2020 04:10:12 +0300 (MSK) From: Nikita Pettik Date: Thu, 7 May 2020 04:10:08 +0300 Message-Id: <06536de4a53c1aedc98fa84c6dbaff7477be0d1f.1588812793.git.korablev@tarantool.org> In-Reply-To: References: In-Reply-To: References: Subject: [Tarantool-patches] [PATCH v4 1/2] vinyl: clean-up unprocessed read views in *_build_read_views() 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 vy_write_iterator->read_views[i].history objects are allocated on region (see vy_write_iterator_push_rv()) during building history of the given key. However, in case of fail of vy_write_iterator_build_history() region is truncated but pointers to vy_write_history objects are not nullified. As a result, they may be accessed (for instance while finalizing write_iterator object in vy_write_iterator_stop) which in turn may lead to crash, segfaul or disk formatting. The same may happen if vy_read_view_merge() fails during processing of read view array. Let's clean-up those objects in case of error takes place. Part of #4864 --- src/box/vy_write_iterator.c | 61 +++++++-- src/errinj.h | 1 + test/box/errinj.result | 1 + .../gh-4864-stmt-alloc-fail-compact.result | 125 ++++++++++++++++++ .../gh-4864-stmt-alloc-fail-compact.test.lua | 53 ++++++++ 5 files changed, 227 insertions(+), 14 deletions(-) diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c index 7a6a20627..7784dd13a 100644 --- a/src/box/vy_write_iterator.c +++ b/src/box/vy_write_iterator.c @@ -151,9 +151,11 @@ vy_read_view_stmt_destroy(struct vy_read_view_stmt *rv) if (rv->tuple != NULL) vy_stmt_unref_if_possible(rv->tuple); rv->tuple = NULL; - if (rv->history != NULL) - vy_write_history_destroy(rv->history); - rv->history = NULL; + /* + * History must be already cleaned up in + * vy_write_iterator_build_read_views(). + */ + assert(rv->history == NULL); } /* @sa vy_write_iterator.h */ @@ -790,8 +792,7 @@ next_lsn: * statement around if this is major compaction, because * there's no tuple it could overwrite. */ - if (rc == 0 && stream->is_last_level && - stream->deferred_delete_stmt != NULL) { + if (stream->is_last_level && stream->deferred_delete_stmt != NULL) { vy_stmt_unref_if_possible(stream->deferred_delete_stmt); stream->deferred_delete_stmt = NULL; } @@ -834,6 +835,15 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint, rv->history = NULL; return 0; } +#ifndef NDEBUG + struct errinj *inj = + errinj(ERRINJ_VY_READ_VIEW_MERGE_FAIL, ERRINJ_BOOL); + if (inj != NULL && inj->bparam) { + inj->bparam = false; + diag_set(OutOfMemory, 666, "malloc", "struct vy_stmt"); + return -1; + } +#endif /* * Two possible hints to remove the current UPSERT. * 1. If the stream is working on the last level, we @@ -940,6 +950,25 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint, return 0; } +/** + * Clean up all histories related to given write iterator. + * Particular history is allocated using region, so single + * region truncation is enough to release all memory at once. + * Before that we should also unref tuples stored in those + * histories (which is done in vy_write_history_destroy()). + */ +static void +vy_write_iterator_history_destroy(struct vy_write_iterator *stream, + struct region *region, size_t used) +{ + for (int i = 0; i < stream->rv_count; ++i) { + if (stream->read_views[i].history != NULL) + vy_write_history_destroy(stream->read_views[i].history); + stream->read_views[i].history = NULL; + } + region_truncate(region, used); +} + /** * Split the current key into a sequence of read view * statements. @sa struct vy_write_iterator comment for details @@ -960,9 +989,12 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count) struct region *region = &fiber()->gc; size_t used = region_used(region); stream->rv_used_count = 0; + int rc = 0; if (vy_write_iterator_build_history(stream, &raw_count, - &is_first_insert) != 0) - goto error; + &is_first_insert) != 0) { + rc = -1; + goto cleanup; + } if (raw_count == 0) { /* A key is fully optimized. */ region_truncate(region, used); @@ -983,8 +1015,10 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count) if (rv->history == NULL) continue; if (vy_read_view_merge(stream, hint, rv, - is_first_insert) != 0) - goto error; + is_first_insert) != 0) { + rc = -1; + goto cleanup; + } assert(rv->history == NULL); if (rv->tuple == NULL) continue; @@ -992,11 +1026,10 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count) ++*count; hint = rv->tuple; } - region_truncate(region, used); - return 0; -error: - region_truncate(region, used); - return -1; + +cleanup: + vy_write_iterator_history_destroy(stream, region, used); + return rc; } /** diff --git a/src/errinj.h b/src/errinj.h index 383dafcb5..b7550bb5e 100644 --- a/src/errinj.h +++ b/src/errinj.h @@ -128,6 +128,7 @@ struct errinj { _(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT, {.iparam = 0}) \ _(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})\ ENUM0(errinj_id, ERRINJ_LIST); extern struct errinj errinjs[]; diff --git a/test/box/errinj.result b/test/box/errinj.result index efbb4e85e..e1b9fbe2a 100644 --- a/test/box/errinj.result +++ b/test/box/errinj.result @@ -68,6 +68,7 @@ evals - ERRINJ_VY_READ_PAGE: false - ERRINJ_VY_READ_PAGE_DELAY: false - ERRINJ_VY_READ_PAGE_TIMEOUT: 0 + - ERRINJ_VY_READ_VIEW_MERGE_FAIL: false - ERRINJ_VY_RUN_DISCARD: false - ERRINJ_VY_RUN_FILE_RENAME: false - ERRINJ_VY_RUN_WRITE: false diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result index 1afc02bef..af116a4b4 100644 --- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result @@ -121,3 +121,128 @@ errinj.set('ERRINJ_VY_STMT_ALLOC', -1) s:drop() | --- | ... + +-- All the same except for delayed vy_stmt_alloc() fail. +-- Re-create space for the sake of test purity. +-- +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() + | --- + | ... + +errinj = box.error.injection + | --- + | ... +errinj.set('ERRINJ_VY_STMT_ALLOC', 5) + | --- + | - ok + | ... +-- Compaction of first range fails, so it is re-scheduled and +-- then successfully finishes at the second attempt. +-- +compact() + | --- + | ... +assert(s.index.pk:stat().range_count == 2) + | --- + | - true + | ... +assert(s.index.pk:stat().run_count == 2) + | --- + | - true + | ... +assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1) + | --- + | - true + | ... +errinj.set('ERRINJ_VY_STMT_ALLOC', -1) + | --- + | - ok + | ... +-- Unthrottle scheduler to allow next dump. +-- +errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.0001) + | --- + | - ok + | ... + +s:drop() + | --- + | ... + +-- Once again but test that clean-up is made in case +-- vy_read_view_merge() fails. +-- +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() + | --- + | ... + +errinj = box.error.injection + | --- + | ... +errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', true) + | --- + | - ok + | ... +compact() + | --- + | ... +assert(s.index.pk:stat().range_count == 2) + | --- + | - true + | ... +assert(s.index.pk:stat().run_count == 2) + | --- + | - true + | ... +assert(errinj.get('ERRINJ_VY_READ_VIEW_MERGE_FAIL') == false) + | --- + | - true + | ... +errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', false) + | --- + | - ok + | ... +s:drop() + | --- + | ... + +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 bf70bdf75..a68c73d32 100644 --- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua @@ -53,3 +53,56 @@ assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1) errinj.set('ERRINJ_VY_STMT_ALLOC', -1) s:drop() + +-- All the same except for delayed vy_stmt_alloc() fail. +-- Re-create space for the sake of test purity. +-- +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() + +errinj = box.error.injection +errinj.set('ERRINJ_VY_STMT_ALLOC', 5) +-- Compaction of first range fails, so it is re-scheduled and +-- then successfully finishes at the second attempt. +-- +compact() +assert(s.index.pk:stat().range_count == 2) +assert(s.index.pk:stat().run_count == 2) +assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1) +errinj.set('ERRINJ_VY_STMT_ALLOC', -1) +-- Unthrottle scheduler to allow next dump. +-- +errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.0001) + +s:drop() + +-- Once again but test that clean-up is made in case +-- vy_read_view_merge() fails. +-- +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() + +errinj = box.error.injection +errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', true) +compact() +assert(s.index.pk:stat().range_count == 2) +assert(s.index.pk:stat().run_count == 2) +assert(errinj.get('ERRINJ_VY_READ_VIEW_MERGE_FAIL') == false) +errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', false) +s:drop() + +errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0) -- 2.17.1