From: Nikita Pettik <korablev@tarantool.org> To: Aleksandr Lyapunov <alyapunov@tarantool.org> Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org Subject: Re: [Tarantool-patches] [PATCH v3 2/3] vinyl: clean-up unprocessed read views in *_build_read_views() Date: Thu, 7 May 2020 00:29:22 +0000 [thread overview] Message-ID: <20200507002922.GA9992@tarantool.org> (raw) In-Reply-To: <fc6693b3-bd4a-cc89-3cc2-5395a2969e6e@tarantool.org> On 06 May 12:56, Aleksandr Lyapunov wrote: > This will work, but I think we have to make the patch better. > > The cause of the issue is that the state of an iterator is not obvious. Is > the history > collected or not? Is it merged already? Must it be cleaned up? Even the > cleanup > is split to two stages - tuple unrefs and region truncate. > > Now you propose to call vy_read_view_stmt_destroy from several places that > also > unrefs tuples from read view (not only from its history). This makes the > state > of an iterator even more unpredictable: when vy_write_iterator_stop is > called > sometimes read views are already cleaned up, sometimes are not. On the other hand clean-up provided by vy_read_view_stmt_destroy() is safe: it nullifies rv->tuple and rv->history so that next call of vy_read_view_stmt_destroy() will be no-op in fact. Anyway, I've refactored this code according to your plan, so vy_read_view_stmt_destroy() now unrefs only resulting tuple. > Let's fix this mess. > 1. History is collected (rallocated and refed) only in > vy_write_iterator_build_history. Check. > 2. The history must be destructed after call to > vy_write_iterator_build_history in any case, > whether an error occurred or not (comment about it?). But histories are used in vy_read_view_merge(). Do you mean destruct all histories after they are processed (i.e. before region_truncate())? > 3. There must be a method for history destruction with two steps: unref all > and rtruncate. > 4. Only vy_write_iterator_build_read_views must call this method. > vy_write_iterator_stop > or anybody else shouldn't be concerned about history. > 5. vy_write_iterator_build_read_views must not unref resulting tuples > (entry.stmt) in case of error, > vy_write_iterator_stop must do it. Ok, I agree. I've taken these points into consideration while preparing next version. Please, check it out. Also here's diff between versions: diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c index efb88d1ae..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,11 +792,7 @@ next_lsn: * statement around if this is major compaction, because * there's no tuple it could overwrite. */ - if (rc != 0) { - for (int i = 0; i < stream->rv_count; ++i) - vy_read_view_stmt_destroy(&stream->read_views[i]); - } else if (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; } @@ -952,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 @@ -972,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); @@ -996,11 +1016,8 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count) continue; if (vy_read_view_merge(stream, hint, rv, is_first_insert) != 0) { - while (rv >= &stream->read_views[0]) { - vy_read_view_stmt_destroy(rv); - rv--; - } - goto error; + rc = -1; + goto cleanup; } assert(rv->history == NULL); if (rv->tuple == NULL) @@ -1009,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; }
next prev parent reply other threads:[~2020-05-07 0:29 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 [this message] 2020-05-07 8:44 ` Aleksandr Lyapunov 2020-05-07 12:28 ` Nikita Pettik 2020-04-27 0:52 ` [Tarantool-patches] [PATCH v3 3/3] vinyl: clean-up write iterator if vy_task_write_run() fails Nikita Pettik 2020-05-01 0:55 ` 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=20200507002922.GA9992@tarantool.org \ --to=korablev@tarantool.org \ --cc=alyapunov@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v3 2/3] vinyl: clean-up unprocessed read views in *_build_read_views()' \ /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