* [Tarantool-patches] [PATCH 0/3] Safe allocation on truncation and deletion
@ 2019-12-13 7:05 Ilya Kosarev
2019-12-13 7:05 ` [Tarantool-patches] [PATCH 1/3] b-tree: return NULL on matras_alloc fail Ilya Kosarev
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Ilya Kosarev @ 2019-12-13 7:05 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
space:truncate() and space:delete() could fail on memory allocations
when reaching memtx_memory limit. As far as it is quite an ill
behaviour, it is fixed in this patchset through denial of extra
memory reservation and unconstrained allocation of service tuples
in BOX_TRUNCATE_ID space.
Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-3807-safe-alloc-on-truncation
Issue: https://github.com/tarantool/tarantool/issues/3807
Ilya Kosarev (3):
b-tree: return NULL on matras_alloc fail
memtx: don't reserve extra memory if not needed
tuple: use calloc for service truncation tuples
src/box/memtx_engine.c | 50 ++++++++++++++++++++++++++--------------
src/box/memtx_engine.h | 2 +-
src/box/memtx_space.c | 34 ++++++++++++++++++++-------
src/box/tuple.c | 16 +++++++++----
src/box/tuple.h | 2 +-
src/box/tuple_format.c | 1 +
src/box/tuple_format.h | 10 +++++++-
src/box/vy_stmt.c | 3 ++-
src/lib/salad/bps_tree.h | 2 ++
9 files changed, 87 insertions(+), 33 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [Tarantool-patches] [PATCH 1/3] b-tree: return NULL on matras_alloc fail 2019-12-13 7:05 [Tarantool-patches] [PATCH 0/3] Safe allocation on truncation and deletion Ilya Kosarev @ 2019-12-13 7:05 ` Ilya Kosarev 2019-12-19 0:32 ` Vladislav Shpilevoy 2019-12-13 7:05 ` [Tarantool-patches] [PATCH 2/3] memtx: don't reserve extra memory if not needed Ilya Kosarev 2019-12-13 7:05 ` [Tarantool-patches] [PATCH 3/3] tuple: use calloc for service truncation tuples Ilya Kosarev 2 siblings, 1 reply; 7+ messages in thread From: Ilya Kosarev @ 2019-12-13 7:05 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy In bps_tree_create_leaf we use matras_alloc in case bps_tree_garbage_pop didn't work out. However it also might not succeed. Then we need to return NULL instead of dereferencing NULL pointer. Prerequisites: #3807 --- src/lib/salad/bps_tree.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/lib/salad/bps_tree.h b/src/lib/salad/bps_tree.h index d28b53f53c8..51f5c8b5556 100644 --- a/src/lib/salad/bps_tree.h +++ b/src/lib/salad/bps_tree.h @@ -2149,6 +2149,8 @@ bps_tree_create_leaf(struct bps_tree *tree, bps_tree_block_id_t *id) bps_tree_garbage_pop(tree, id); if (!res) res = (struct bps_leaf *)matras_alloc(&tree->matras, id); + if (!res) + return NULL; res->header.type = BPS_TREE_BT_LEAF; tree->leaf_count++; return res; -- 2.17.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/3] b-tree: return NULL on matras_alloc fail 2019-12-13 7:05 ` [Tarantool-patches] [PATCH 1/3] b-tree: return NULL on matras_alloc fail Ilya Kosarev @ 2019-12-19 0:32 ` Vladislav Shpilevoy 0 siblings, 0 replies; 7+ messages in thread From: Vladislav Shpilevoy @ 2019-12-19 0:32 UTC (permalink / raw) To: Ilya Kosarev, tarantool-patches Hi! Thanks for the patch! On 13/12/2019 08:05, Ilya Kosarev wrote: > In bps_tree_create_leaf we use matras_alloc in case > bps_tree_garbage_pop didn't work out. However it also might not > succeed. Then we need to return NULL instead of dereferencing NULL > pointer. > > Prerequisites: #3807 > --- > src/lib/salad/bps_tree.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/src/lib/salad/bps_tree.h b/src/lib/salad/bps_tree.h > index d28b53f53c8..51f5c8b5556 100644 > --- a/src/lib/salad/bps_tree.h > +++ b/src/lib/salad/bps_tree.h > @@ -2149,6 +2149,8 @@ bps_tree_create_leaf(struct bps_tree *tree, bps_tree_block_id_t *id) > bps_tree_garbage_pop(tree, id); > if (!res) > res = (struct bps_leaf *)matras_alloc(&tree->matras, id); > + if (!res) > + return NULL; In case res was not NULL (what is 99.(9)% of all cases), you will double check it. Please, move the second check into first 'if's body after matras_alloc. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Tarantool-patches] [PATCH 2/3] memtx: don't reserve extra memory if not needed 2019-12-13 7:05 [Tarantool-patches] [PATCH 0/3] Safe allocation on truncation and deletion Ilya Kosarev 2019-12-13 7:05 ` [Tarantool-patches] [PATCH 1/3] b-tree: return NULL on matras_alloc fail Ilya Kosarev @ 2019-12-13 7:05 ` Ilya Kosarev 2019-12-19 0:31 ` Vladislav Shpilevoy 2019-12-13 7:05 ` [Tarantool-patches] [PATCH 3/3] tuple: use calloc for service truncation tuples Ilya Kosarev 2 siblings, 1 reply; 7+ messages in thread From: Ilya Kosarev @ 2019-12-13 7:05 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy memtx_index_extent_reserve was always being called from memtx_space_replace_all_keys. It could lead to slab allocator failure while reaching memtx_memory limit, which is especially distressing on space:truncate() and space:delete() invocations. we Now it is not being called if not needed, including space:truncate() and space:delete() cases. Part of #3807 --- src/box/memtx_space.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c index 6ef84e04564..1e8759778e7 100644 --- a/src/box/memtx_space.c +++ b/src/box/memtx_space.c @@ -260,10 +260,18 @@ memtx_space_replace_all_keys(struct space *space, struct tuple *old_tuple, * Ensure we have enough slack memory to guarantee * successful statement-level rollback. */ - if (memtx_index_extent_reserve(memtx, new_tuple != NULL ? - RESERVE_EXTENTS_BEFORE_REPLACE : - RESERVE_EXTENTS_BEFORE_DELETE) != 0) + if (space->index_count > 1 && new_tuple != NULL) { + if (memtx_index_extent_reserve(memtx, new_tuple != NULL ? + RESERVE_EXTENTS_BEFORE_REPLACE : + RESERVE_EXTENTS_BEFORE_DELETE) != 0) + return -1; + } + ERROR_INJECT(ERRINJ_INDEX_ALLOC, { + /* same error as in mempool_alloc */ + diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, + "mempool", "new slab"); return -1; + }); uint32_t i = 0; -- 2.17.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/3] memtx: don't reserve extra memory if not needed 2019-12-13 7:05 ` [Tarantool-patches] [PATCH 2/3] memtx: don't reserve extra memory if not needed Ilya Kosarev @ 2019-12-19 0:31 ` Vladislav Shpilevoy 0 siblings, 0 replies; 7+ messages in thread From: Vladislav Shpilevoy @ 2019-12-19 0:31 UTC (permalink / raw) To: Ilya Kosarev, tarantool-patches Thanks for the patch! On 13/12/2019 08:05, Ilya Kosarev wrote: > memtx_index_extent_reserve was always being called from > memtx_space_replace_all_keys. It could lead to slab allocator failure > while reaching memtx_memory limit, which is especially distressing on > space:truncate() and space:delete() invocations. we Now it is not > being called if not needed, including space:truncate() and > space:delete() cases. The thing is that delete also can cause tree rebalancing. Maybe that is why reservation was done for all cases. > > Part of #3807 > --- > src/box/memtx_space.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c > index 6ef84e04564..1e8759778e7 100644 > --- a/src/box/memtx_space.c > +++ b/src/box/memtx_space.c > @@ -260,10 +260,18 @@ memtx_space_replace_all_keys(struct space *space, struct tuple *old_tuple, > * Ensure we have enough slack memory to guarantee > * successful statement-level rollback. > */ > - if (memtx_index_extent_reserve(memtx, new_tuple != NULL ? > - RESERVE_EXTENTS_BEFORE_REPLACE : > - RESERVE_EXTENTS_BEFORE_DELETE) != 0) > + if (space->index_count > 1 && new_tuple != NULL) { > + if (memtx_index_extent_reserve(memtx, new_tuple != NULL ? > + RESERVE_EXTENTS_BEFORE_REPLACE : > + RESERVE_EXTENTS_BEFORE_DELETE) != 0) > + return -1; Why does it work only when index count > 1? Why delete don't need a reservation (I pointed out above that it needs)? And why do you check for new_tuple != NULL second time after having it already checked in the if's condition? > + } > + ERROR_INJECT(ERRINJ_INDEX_ALLOC, { > + /* same error as in mempool_alloc */ > + diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, > + "mempool", "new slab"); > return -1; > + }); > > uint32_t i = 0; > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Tarantool-patches] [PATCH 3/3] tuple: use calloc for service truncation tuples 2019-12-13 7:05 [Tarantool-patches] [PATCH 0/3] Safe allocation on truncation and deletion Ilya Kosarev 2019-12-13 7:05 ` [Tarantool-patches] [PATCH 1/3] b-tree: return NULL on matras_alloc fail Ilya Kosarev 2019-12-13 7:05 ` [Tarantool-patches] [PATCH 2/3] memtx: don't reserve extra memory if not needed Ilya Kosarev @ 2019-12-13 7:05 ` Ilya Kosarev 2019-12-19 0:31 ` Vladislav Shpilevoy 2 siblings, 1 reply; 7+ messages in thread From: Ilya Kosarev @ 2019-12-13 7:05 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy Trying to perform space:truncate() while reaching memtx_memory limit we could experience slab allocator failure on allocation for service tuple being upserted into service space to keep record of truncations. This behavior seems to be quite surprising for users. Now we are using calloc() to allocate tuples in BOX_TRUNCATE_ID space. Closes: #3807 --- src/box/memtx_engine.c | 50 ++++++++++++++++++++++++++++-------------- src/box/memtx_engine.h | 2 +- src/box/memtx_space.c | 20 ++++++++++++----- src/box/tuple.c | 16 ++++++++++---- src/box/tuple.h | 2 +- src/box/tuple_format.c | 1 + src/box/tuple_format.h | 10 ++++++++- src/box/vy_stmt.c | 3 ++- 8 files changed, 74 insertions(+), 30 deletions(-) diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c index 23ccc4703fe..59148059f2b 100644 --- a/src/box/memtx_engine.c +++ b/src/box/memtx_engine.c @@ -1113,7 +1113,7 @@ memtx_leave_delayed_free_mode(struct memtx_engine *memtx) } struct tuple * -memtx_tuple_new(struct tuple_format *format, const char *data, const char *end) +memtx_tuple_new(struct tuple_format *format, const char *data, const char *end, bool use_calloc) { struct memtx_engine *memtx = (struct memtx_engine *)format->engine; assert(mp_typeof(*data) == MP_ARRAY); @@ -1139,15 +1139,27 @@ memtx_tuple_new(struct tuple_format *format, const char *data, const char *end) } struct memtx_tuple *memtx_tuple; - while ((memtx_tuple = smalloc(&memtx->alloc, total)) == NULL) { - bool stop; - memtx_engine_run_gc(memtx, &stop); - if (stop) - break; - } - if (memtx_tuple == NULL) { - diag_set(OutOfMemory, total, "slab allocator", "memtx_tuple"); - goto end; + if (use_calloc) { + memtx_tuple = calloc(1, total); + if (memtx_tuple == NULL) { + diag_set(OutOfMemory, total, "malloc", + "memtx_tuple"); + goto end; + } + format->is_malloced = true; + } else { + while ((memtx_tuple = smalloc(&memtx->alloc, + total)) == NULL) { + bool stop; + memtx_engine_run_gc(memtx, &stop); + if (stop) + break; + } + if (memtx_tuple == NULL) { + diag_set(OutOfMemory, total, "slab allocator", + "memtx_tuple"); + goto end; + } } tuple = &memtx_tuple->base; tuple->refs = 0; @@ -1177,16 +1189,20 @@ memtx_tuple_delete(struct tuple_format *format, struct tuple *tuple) struct memtx_engine *memtx = (struct memtx_engine *)format->engine; say_debug("%s(%p)", __func__, tuple); assert(tuple->refs == 0); - tuple_format_unref(format); struct memtx_tuple *memtx_tuple = container_of(tuple, struct memtx_tuple, base); size_t total = tuple_size(tuple) + offsetof(struct memtx_tuple, base); - if (memtx->alloc.free_mode != SMALL_DELAYED_FREE || - memtx_tuple->version == memtx->snapshot_version || - format->is_temporary) - smfree(&memtx->alloc, memtx_tuple, total); - else - smfree_delayed(&memtx->alloc, memtx_tuple, total); + if (format->is_malloced) { + free(memtx_tuple); + } else { + if (memtx->alloc.free_mode != SMALL_DELAYED_FREE || + memtx_tuple->version == memtx->snapshot_version || + format->is_temporary) + smfree(&memtx->alloc, memtx_tuple, total); + else + smfree_delayed(&memtx->alloc, memtx_tuple, total); + } + tuple_format_unref(format); } void diff --git a/src/box/memtx_engine.h b/src/box/memtx_engine.h index f562c66df4f..a2ae8134d30 100644 --- a/src/box/memtx_engine.h +++ b/src/box/memtx_engine.h @@ -235,7 +235,7 @@ memtx_leave_delayed_free_mode(struct memtx_engine *memtx); /** Allocate a memtx tuple. @sa tuple_new(). */ struct tuple * -memtx_tuple_new(struct tuple_format *format, const char *data, const char *end); +memtx_tuple_new(struct tuple_format *format, const char *data, const char *end, bool use_calloc); /** Free a memtx tuple. @sa tuple_delete(). */ void diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c index 1e8759778e7..c413fd9b1e3 100644 --- a/src/box/memtx_space.c +++ b/src/box/memtx_space.c @@ -332,7 +332,9 @@ memtx_space_execute_replace(struct space *space, struct txn *txn, struct txn_stmt *stmt = txn_current_stmt(txn); enum dup_replace_mode mode = dup_replace_mode(request->type); stmt->new_tuple = memtx_tuple_new(space->format, request->tuple, - request->tuple_end); + request->tuple_end, + request->space_id + == BOX_TRUNCATE_ID); if (stmt->new_tuple == NULL) return -1; tuple_ref(stmt->new_tuple); @@ -406,7 +408,9 @@ memtx_space_execute_update(struct space *space, struct txn *txn, return -1; stmt->new_tuple = memtx_tuple_new(format, new_data, - new_data + new_size); + new_data + new_size, + request->space_id + == BOX_TRUNCATE_ID); if (stmt->new_tuple == NULL) return -1; tuple_ref(stmt->new_tuple); @@ -475,7 +479,9 @@ memtx_space_execute_upsert(struct space *space, struct txn *txn, return -1; } stmt->new_tuple = memtx_tuple_new(format, request->tuple, - request->tuple_end); + request->tuple_end, + request->space_id + == BOX_TRUNCATE_ID); if (stmt->new_tuple == NULL) return -1; tuple_ref(stmt->new_tuple); @@ -499,7 +505,9 @@ memtx_space_execute_upsert(struct space *space, struct txn *txn, return -1; stmt->new_tuple = memtx_tuple_new(format, new_data, - new_data + new_size); + new_data + new_size, + request->space_id + == BOX_TRUNCATE_ID); if (stmt->new_tuple == NULL) return -1; tuple_ref(stmt->new_tuple); @@ -548,7 +556,9 @@ memtx_space_ephemeral_replace(struct space *space, const char *tuple, { struct memtx_space *memtx_space = (struct memtx_space *)space; struct tuple *new_tuple = memtx_tuple_new(space->format, tuple, - tuple_end); + tuple_end, + space->def->id + == BOX_TRUNCATE_ID); if (new_tuple == NULL) return -1; struct tuple *old_tuple; diff --git a/src/box/tuple.c b/src/box/tuple.c index 4d676b090c2..bafa8e0cb5c 100644 --- a/src/box/tuple.c +++ b/src/box/tuple.c @@ -60,7 +60,7 @@ static void runtime_tuple_delete(struct tuple_format *format, struct tuple *tuple); static struct tuple * -runtime_tuple_new(struct tuple_format *format, const char *data, const char *end); +runtime_tuple_new(struct tuple_format *format, const char *data, const char *end, bool use_calloc); /** A virtual method table for tuple_format_runtime */ static struct tuple_format_vtab tuple_format_runtime_vtab = { @@ -71,7 +71,7 @@ static struct tuple_format_vtab tuple_format_runtime_vtab = { }; static struct tuple * -runtime_tuple_new(struct tuple_format *format, const char *data, const char *end) +runtime_tuple_new(struct tuple_format *format, const char *data, const char *end, bool use_calloc) { assert(format->vtab.tuple_delete == tuple_format_runtime_vtab.tuple_delete); @@ -86,8 +86,13 @@ runtime_tuple_new(struct tuple_format *format, const char *data, const char *end size_t data_len = end - data; size_t total = sizeof(struct tuple) + field_map_size + data_len; - tuple = (struct tuple *) smalloc(&runtime_alloc, total); + if (use_calloc) { + tuple = (struct tuple *) calloc(1, total); + format->is_malloced = true; + } else + tuple = (struct tuple *) smalloc(&runtime_alloc, total); if (tuple == NULL) { + format->is_malloced = false; diag_set(OutOfMemory, (unsigned) total, "malloc", "tuple"); goto end; @@ -114,8 +119,11 @@ runtime_tuple_delete(struct tuple_format *format, struct tuple *tuple) say_debug("%s(%p)", __func__, tuple); assert(tuple->refs == 0); size_t total = tuple_size(tuple); + if (format->is_malloced) + free(tuple); + else + smfree(&runtime_alloc, tuple, total); tuple_format_unref(format); - smfree(&runtime_alloc, tuple, total); } int diff --git a/src/box/tuple.h b/src/box/tuple.h index 9a887721986..7fa62659acc 100644 --- a/src/box/tuple.h +++ b/src/box/tuple.h @@ -431,7 +431,7 @@ tuple_format(struct tuple *tuple) static inline struct tuple * tuple_new(struct tuple_format *format, const char *data, const char *end) { - return format->vtab.tuple_new(format, data, end); + return format->vtab.tuple_new(format, data, end, false); } /** diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c index 312c96621fb..d0e60acd334 100644 --- a/src/box/tuple_format.c +++ b/src/box/tuple_format.c @@ -789,6 +789,7 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine, memset(&format->vtab, 0, sizeof(format->vtab)); format->engine = engine; format->is_temporary = is_temporary; + format->is_malloced = false; format->is_ephemeral = is_ephemeral; format->exact_field_count = exact_field_count; format->epoch = ++formats_epoch; diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h index 021072d3d4c..cb9f0a0407d 100644 --- a/src/box/tuple_format.h +++ b/src/box/tuple_format.h @@ -82,7 +82,7 @@ struct tuple_format_vtab { */ struct tuple* (*tuple_new)(struct tuple_format *format, const char *data, - const char *end); + const char *end, bool use_calloc); /** * Free a tuple_chunk allocated for given tuple and * data. @@ -188,6 +188,14 @@ struct tuple_format { * in progress. */ bool is_temporary; + /** + * Tuple is being malloced if we use calloc instead of + * traditional smalloc to allocate it. It is happening + * if the tuple is being upserted into BOX_TRUNCATE_ID + * space as far as we want to perform space:truncate() + * even if memtx_memory limit is reached. + */ + bool is_malloced; /** * This format belongs to ephemeral space and thus might * be shared with other ephemeral spaces. diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c index 518a24f43d7..b6af0b00e5d 100644 --- a/src/box/vy_stmt.c +++ b/src/box/vy_stmt.c @@ -82,8 +82,9 @@ vy_stmt_persistent_flags(struct tuple *stmt, bool is_primary) } static struct tuple * -vy_tuple_new(struct tuple_format *format, const char *data, const char *end) +vy_tuple_new(struct tuple_format *format, const char *data, const char *end, bool use_calloc) { + (void)use_calloc; if (tuple_validate_raw(format, data) != 0) return NULL; -- 2.17.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/3] tuple: use calloc for service truncation tuples 2019-12-13 7:05 ` [Tarantool-patches] [PATCH 3/3] tuple: use calloc for service truncation tuples Ilya Kosarev @ 2019-12-19 0:31 ` Vladislav Shpilevoy 0 siblings, 0 replies; 7+ messages in thread From: Vladislav Shpilevoy @ 2019-12-19 0:31 UTC (permalink / raw) To: Ilya Kosarev, tarantool-patches Thanks for the patch! See 3 comments below. On 13/12/2019 08:05, Ilya Kosarev wrote: > Trying to perform space:truncate() while reaching memtx_memory limit we > could experience slab allocator failure on allocation for service tuple > being upserted into service space to keep record of truncations. This > behavior seems to be quite surprising for users. Now we are using > calloc() to allocate tuples in BOX_TRUNCATE_ID space. > > Closes: #3807 > --- > src/box/memtx_engine.c | 50 ++++++++++++++++++++++++++++-------------- > src/box/memtx_engine.h | 2 +- > src/box/memtx_space.c | 20 ++++++++++++----- > src/box/tuple.c | 16 ++++++++++---- > src/box/tuple.h | 2 +- > src/box/tuple_format.c | 1 + > src/box/tuple_format.h | 10 ++++++++- > src/box/vy_stmt.c | 3 ++- > 8 files changed, 74 insertions(+), 30 deletions(-) > > diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c > index 23ccc4703fe..59148059f2b 100644 > --- a/src/box/memtx_engine.c > +++ b/src/box/memtx_engine.c > @@ -1113,7 +1113,7 @@ memtx_leave_delayed_free_mode(struct memtx_engine *memtx) > } > > struct tuple * > -memtx_tuple_new(struct tuple_format *format, const char *data, const char *end) > +memtx_tuple_new(struct tuple_format *format, const char *data, const char *end, bool use_calloc) 1. Oh. No. Please. The whole point of format vtable is to avoid that. > { > struct memtx_engine *memtx = (struct memtx_engine *)format->engine; > assert(mp_typeof(*data) == MP_ARRAY); > @@ -1139,15 +1139,27 @@ memtx_tuple_new(struct tuple_format *format, const char *data, const char *end) > } > > struct memtx_tuple *memtx_tuple; > - while ((memtx_tuple = smalloc(&memtx->alloc, total)) == NULL) { > - bool stop; > - memtx_engine_run_gc(memtx, &stop); > - if (stop) > - break; > - } > - if (memtx_tuple == NULL) { > - diag_set(OutOfMemory, total, "slab allocator", "memtx_tuple"); > - goto end; > + if (use_calloc) { > + memtx_tuple = calloc(1, total); > + if (memtx_tuple == NULL) { > + diag_set(OutOfMemory, total, "malloc", > + "memtx_tuple"); > + goto end; > + } > + format->is_malloced = true; 2. Sorry, we can't change format here. The only reason why format is not const in that function is because we need increment its reference counter. A tuple can't change its own format. Especially in such a hot path. > + } else { > + while ((memtx_tuple = smalloc(&memtx->alloc, > + total)) == NULL) { > + bool stop; > + memtx_engine_run_gc(memtx, &stop); > + if (stop) > + break; > + } > + if (memtx_tuple == NULL) { > + diag_set(OutOfMemory, total, "slab allocator", > + "memtx_tuple"); > + goto end; > + } > } > tuple = &memtx_tuple->base; > tuple->refs = 0; > @@ -1177,16 +1189,20 @@ memtx_tuple_delete(struct tuple_format *format, struct tuple *tuple) > struct memtx_engine *memtx = (struct memtx_engine *)format->engine; > say_debug("%s(%p)", __func__, tuple); > assert(tuple->refs == 0); > - tuple_format_unref(format); > struct memtx_tuple *memtx_tuple = > container_of(tuple, struct memtx_tuple, base); > size_t total = tuple_size(tuple) + offsetof(struct memtx_tuple, base); > - if (memtx->alloc.free_mode != SMALL_DELAYED_FREE || > - memtx_tuple->version == memtx->snapshot_version || > - format->is_temporary) > - smfree(&memtx->alloc, memtx_tuple, total); > - else > - smfree_delayed(&memtx->alloc, memtx_tuple, total); > + if (format->is_malloced) { > + free(memtx_tuple); 3. There is a certain purpose for these free_mode, version, blah blah. These attributes create a read view of the whole memtx engine to make a snapshot, but do not stop new requests. Tuples are not deleted during snapshot. Having ignored the free mode you get this: ==================================================================== box.cfg{} errinj = box.error.injection fiber = require('fiber') s = box.schema.create_space('test') pk = s:create_index('pk') s:truncate() s:replace{1} box.error.injection.set('ERRINJ_SNAP_WRITE_DELAY', true) f = fiber.create(box.snapshot) box.space._truncate:delete{s.id} -- The code below improves reproducibility. It creates and -- deletes several tuples to unref the blessed tuple -- (tuple_bless), rewrite some recently freed heap chunks. s:get{1} collectgarbage('collect') s:replace{1} s:get{1} collectgarbage('collect') s:replace{1} s:get{1} collectgarbage('collect') s:replace{1} s:get{1} collectgarbage('collect') -- box.error.injection.set('ERRINJ_SNAP_WRITE_DELAY', false) tarantool> Process 94390 stopped * thread #2, name = 'snapshot', stop reason = EXC_BAD_ACCESS (code=2, address=0x105481000) frame #0: 0x00007fff5a964ce9 libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell + 41 libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell: -> 0x7fff5a964ce9 <+41>: rep movsb (%rsi), %es:(%rdi) 0x7fff5a964ceb <+43>: popq %rbp 0x7fff5a964cec <+44>: retq 0x7fff5a964ced <+45>: cmpq %rdi, %rsi Target 0: (tarantool) stopped. (lldb) bt * thread #2, name = 'snapshot', stop reason = EXC_BAD_ACCESS (code=2, address=0x105481000) * frame #0: 0x00007fff5a964ce9 libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell + 41 frame #1: 0x00000001003f56ba tarantool`obuf_dup(buf=0x000000010f0ff8b0, data=0x0000000105157aa6, size=85071264) at obuf.c:161:2 frame #2: 0x00000001003634e8 tarantool`xlog_write_row(log=0x000000010f0ff208, packet=0x000000010f0ff0d8) at xlog.c:1320:7 frame #3: 0x0000000100038aef tarantool`checkpoint_write_row(l=0x000000010f0ff208, row=0x000000010f0ff0d8) at memtx_engine.c:431:20 frame #4: 0x0000000100038a2e tarantool`checkpoint_write_tuple(l=0x000000010f0ff208, space_id=330, group_id=0, data="", size=85071264) at memtx_engine.c:459:9 frame #5: 0x0000000100038811 tarantool`checkpoint_f(ap=0x000000010f000278) at memtx_engine.c:603:8 frame #6: 0x0000000100005e5a tarantool`fiber_cxx_invoke(f=(tarantool`checkpoint_f at memtx_engine.c:577), ap=0x000000010f000278)(__va_list_tag*), __va_list_tag*) at fiber.h:762:10 frame #7: 0x000000010018168b tarantool`fiber_loop(data=0x0000000000000000) at fiber.c:830:18 frame #8: 0x00000001004165e7 tarantool`coro_init at coro.c:110:3 ==================================================================== Snapshoting means that you can't solve the issue on the format level, or tuple level. You need to patch the engine level (IMO). Honestly, I thought about this issue for quite a long time already, and I don't have a plan how to fix it. All ways look bad. All of them assume special patching of _truncate space insertion and crutches for deletion. For me the most promising way was to introduce an analogue of vy_quota_force_use() somehow, for _truncate, and use it in a before_replace trigger of _truncate. Forced use of quota would allow to temporary overflow the memory limit. But it won't fix deletion. Maybe the key is in introduction of quota overuse for deletions. I don't know. Personally, I don't think the issue is worth fixing at all. If a user wants to delete/truncate, he can increase memtx memory, do cleanup, and decrease it back. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-12-19 0:32 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-13 7:05 [Tarantool-patches] [PATCH 0/3] Safe allocation on truncation and deletion Ilya Kosarev 2019-12-13 7:05 ` [Tarantool-patches] [PATCH 1/3] b-tree: return NULL on matras_alloc fail Ilya Kosarev 2019-12-19 0:32 ` Vladislav Shpilevoy 2019-12-13 7:05 ` [Tarantool-patches] [PATCH 2/3] memtx: don't reserve extra memory if not needed Ilya Kosarev 2019-12-19 0:31 ` Vladislav Shpilevoy 2019-12-13 7:05 ` [Tarantool-patches] [PATCH 3/3] tuple: use calloc for service truncation tuples Ilya Kosarev 2019-12-19 0:31 ` Vladislav Shpilevoy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox