* [Tarantool-patches] [PATCH v2 0/2] Safe truncation and deletion @ 2020-01-10 0:36 Ilya Kosarev 2020-01-10 0:36 ` [Tarantool-patches] [PATCH v2 1/2] b-tree: return NULL on matras_alloc fail Ilya Kosarev 2020-01-10 0:36 ` [Tarantool-patches] [PATCH v2 2/2] memtx: increase the memory quota if needed to truncate or delete Ilya Kosarev 0 siblings, 2 replies; 6+ messages in thread From: Ilya Kosarev @ 2020-01-10 0:36 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 memtx quota enlargement. Also possible bps_tree_create_leaf NULL dereference issue is fixed. Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-3807-safe-alloc-on-truncation Issue: https://github.com/tarantool/tarantool/issues/3807 Changes in v2: Approach changed completely: now we are not trying to allocate service tuples in some safe way, but increasing memtx quota so that space:truncate() and space:delete() won't fail on allocation. Ilya Kosarev (2): b-tree: return NULL on matras_alloc fail memtx: increase the memory quota if needed to truncate or delete src/box/box.cc | 34 +++++++++++++++++++++++++++++++++- src/box/memtx_engine.c | 18 ++++++++++++++++++ src/box/memtx_engine.h | 5 +++++ src/lib/salad/bps_tree.h | 7 +++++-- 4 files changed, 61 insertions(+), 3 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Tarantool-patches] [PATCH v2 1/2] b-tree: return NULL on matras_alloc fail 2020-01-10 0:36 [Tarantool-patches] [PATCH v2 0/2] Safe truncation and deletion Ilya Kosarev @ 2020-01-10 0:36 ` Ilya Kosarev 2020-01-10 0:36 ` [Tarantool-patches] [PATCH v2 2/2] memtx: increase the memory quota if needed to truncate or delete Ilya Kosarev 1 sibling, 0 replies; 6+ messages in thread From: Ilya Kosarev @ 2020-01-10 0:36 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. Part of: #3807 --- src/lib/salad/bps_tree.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/lib/salad/bps_tree.h b/src/lib/salad/bps_tree.h index d28b53f53..ede2a3b7f 100644 --- a/src/lib/salad/bps_tree.h +++ b/src/lib/salad/bps_tree.h @@ -2147,8 +2147,11 @@ bps_tree_create_leaf(struct bps_tree *tree, bps_tree_block_id_t *id) { struct bps_leaf *res = (struct bps_leaf *) bps_tree_garbage_pop(tree, id); - if (!res) - res = (struct bps_leaf *)matras_alloc(&tree->matras, 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] 6+ messages in thread
* [Tarantool-patches] [PATCH v2 2/2] memtx: increase the memory quota if needed to truncate or delete 2020-01-10 0:36 [Tarantool-patches] [PATCH v2 0/2] Safe truncation and deletion Ilya Kosarev 2020-01-10 0:36 ` [Tarantool-patches] [PATCH v2 1/2] b-tree: return NULL on matras_alloc fail Ilya Kosarev @ 2020-01-10 0:36 ` Ilya Kosarev 1 sibling, 0 replies; 6+ messages in thread From: Ilya Kosarev @ 2020-01-10 0:36 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy Trying to perform space:truncate() and space:delete() while reaching memtx_memory limit we could experience slab allocator failure. This behavior seems to be quite surprising for users. Now we are increasing memtx quota if needed for truncation or deletion. After performing it quota is being set back to the previous value if possible, while it should be so for almost any case, since we are meant to free some space during deletion or truncation. Closes #3807 --- src/box/box.cc | 34 +++++++++++++++++++++++++++++++++- src/box/memtx_engine.c | 18 ++++++++++++++++++ src/box/memtx_engine.h | 5 +++++ 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/src/box/box.cc b/src/box/box.cc index 1b2b27d61..03e2f3cc9 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -1250,7 +1250,26 @@ box_delete(uint32_t space_id, uint32_t index_id, const char *key, request.index_id = index_id; request.key = key; request.key_end = key_end; - return box_process1(&request, result); + + struct space *space = space_cache_find(space_id); + if (space == NULL) + return -1; + struct memtx_engine *memtx = + (struct memtx_engine *)space->engine; + size_t total; + bool extended; + memtx_engine_guarantee_memory(memtx, memtx->arena.slab_size, + &total, &extended); + + int rc = box_process1(&request, result); + + if (extended) { + size_t new_total = quota_set(&memtx->quota, total); + if (new_total > total) + quota_set(&memtx->quota, quota_used(&memtx->quota)); + } + + return rc; } int @@ -1321,9 +1340,22 @@ space_truncate(struct space *space) ops_buf_end = mp_encode_uint(ops_buf_end, 1); assert(ops_buf_end < buf + buf_size); + struct memtx_engine *memtx = + (struct memtx_engine *)space->engine; + size_t total; + bool extended; + memtx_engine_guarantee_memory(memtx, memtx->arena.slab_size, + &total, &extended); + if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end, ops_buf, ops_buf_end, 0, NULL) != 0) diag_raise(); + + if (extended) { + size_t new_total = quota_set(&memtx->quota, total); + if (new_total > total) + quota_set(&memtx->quota, quota_used(&memtx->quota)); + } } int diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c index 23ccc4703..cbac015e3 100644 --- a/src/box/memtx_engine.c +++ b/src/box/memtx_engine.c @@ -1090,6 +1090,24 @@ memtx_engine_set_memory(struct memtx_engine *memtx, size_t size) return 0; } +void +memtx_engine_guarantee_memory(struct memtx_engine *memtx, + size_t request, size_t *old_total, + bool *extended) +{ + struct quota *memtx_quota = &memtx->quota; + size_t total, used; + quota_get_total_and_used(memtx_quota, &total, &used); + *old_total = total; + if (total - used < request) { + quota_set(memtx_quota, + total + request - (total - used)); + *extended = true; + return; + } + *extended = false; +} + void memtx_engine_set_max_tuple_size(struct memtx_engine *memtx, size_t max_size) { diff --git a/src/box/memtx_engine.h b/src/box/memtx_engine.h index f562c66df..f74f65e96 100644 --- a/src/box/memtx_engine.h +++ b/src/box/memtx_engine.h @@ -213,6 +213,11 @@ memtx_engine_set_snap_io_rate_limit(struct memtx_engine *memtx, double limit); int memtx_engine_set_memory(struct memtx_engine *memtx, size_t size); +void +memtx_engine_guarantee_memory(struct memtx_engine *memtx, + size_t request, size_t *old_total, + bool *extended); + void memtx_engine_set_max_tuple_size(struct memtx_engine *memtx, size_t max_size); -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Tarantool-patches] [PATCH v2 0/2] Safe truncation and deletion @ 2020-01-13 21:31 Ilya Kosarev 2020-01-13 21:31 ` [Tarantool-patches] [PATCH v2 1/2] b-tree: return NULL on matras_alloc fail Ilya Kosarev 0 siblings, 1 reply; 6+ messages in thread From: Ilya Kosarev @ 2020-01-13 21:31 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 memtx quota enlargement. Also possible bps_tree_create_leaf NULL dereference issue is fixed. Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-3807-safe-alloc-on-truncation Issue: https://github.com/tarantool/tarantool/issues/3807 Changes in v2: Approach changed completely: now we are not trying to allocate service tuples in some safe way, but increasing memtx quota so that space:truncate() and space:delete() won't fail on allocation. Ilya Kosarev (2): b-tree: return NULL on matras_alloc fail memtx: increase the memory quota if needed to truncate or delete src/box/blackhole.c | 1 + src/box/box.cc | 36 +++++++++++++++++++++++++++++++++++- src/box/engine.c | 11 +++++++++++ src/box/engine.h | 9 +++++++++ src/box/memtx_engine.c | 20 ++++++++++++++++++++ src/box/memtx_engine.h | 4 ++++ src/box/service_engine.c | 1 + src/box/sysview.c | 1 + src/box/vinyl.c | 1 + src/lib/salad/bps_tree.h | 7 +++++-- 10 files changed, 88 insertions(+), 3 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Tarantool-patches] [PATCH v2 1/2] b-tree: return NULL on matras_alloc fail 2020-01-13 21:31 [Tarantool-patches] [PATCH v2 0/2] Safe truncation and deletion Ilya Kosarev @ 2020-01-13 21:31 ` Ilya Kosarev 2020-01-14 21:00 ` Vladislav Shpilevoy 0 siblings, 1 reply; 6+ messages in thread From: Ilya Kosarev @ 2020-01-13 21:31 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. Part of: #3807 --- src/lib/salad/bps_tree.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/lib/salad/bps_tree.h b/src/lib/salad/bps_tree.h index d28b53f53..ede2a3b7f 100644 --- a/src/lib/salad/bps_tree.h +++ b/src/lib/salad/bps_tree.h @@ -2147,8 +2147,11 @@ bps_tree_create_leaf(struct bps_tree *tree, bps_tree_block_id_t *id) { struct bps_leaf *res = (struct bps_leaf *) bps_tree_garbage_pop(tree, id); - if (!res) - res = (struct bps_leaf *)matras_alloc(&tree->matras, 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] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/2] b-tree: return NULL on matras_alloc fail 2020-01-13 21:31 ` [Tarantool-patches] [PATCH v2 1/2] b-tree: return NULL on matras_alloc fail Ilya Kosarev @ 2020-01-14 21:00 ` Vladislav Shpilevoy 2020-01-20 18:13 ` Ilya Kosarev 0 siblings, 1 reply; 6+ messages in thread From: Vladislav Shpilevoy @ 2020-01-14 21:00 UTC (permalink / raw) To: Ilya Kosarev, tarantool-patches Hi! Thanks for the patch! On 13/01/2020 22:31, 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. > > Part of: #3807 Please, drop ':', we don't use it when reference tickets. > --- > src/lib/salad/bps_tree.h | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/src/lib/salad/bps_tree.h b/src/lib/salad/bps_tree.h > index d28b53f53..ede2a3b7f 100644 > --- a/src/lib/salad/bps_tree.h > +++ b/src/lib/salad/bps_tree.h > @@ -2147,8 +2147,11 @@ bps_tree_create_leaf(struct bps_tree *tree, bps_tree_block_id_t *id) > { > struct bps_leaf *res = (struct bps_leaf *) > bps_tree_garbage_pop(tree, id); > - if (!res) > - res = (struct bps_leaf *)matras_alloc(&tree->matras, id); > + if (!res) { > + res = (struct bps_leaf *) matras_alloc(&tree->matras, id); > + if (!res) > + return NULL; > + } Since you've changed this code anyway, lets use explicit '== NULL' instead of '!'. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 1/2] b-tree: return NULL on matras_alloc fail 2020-01-14 21:00 ` Vladislav Shpilevoy @ 2020-01-20 18:13 ` Ilya Kosarev 0 siblings, 0 replies; 6+ messages in thread From: Ilya Kosarev @ 2020-01-20 18:13 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 1289 bytes --] Hi! Thanks for the review. Remarks fixed in v3. >Среда, 15 января 2020, 0:00 +03:00 от Vladislav Shpilevoy < v.shpilevoy@tarantool.org >: > >Hi! Thanks for the patch! > >On 13/01/2020 22:31, 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. >> >> Part of: #3807 > >Please, drop ':', we don't use it when reference tickets. > >> --- >> src/lib/salad/bps_tree.h | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/src/lib/salad/bps_tree.h b/src/lib/salad/bps_tree.h >> index d28b53f53..ede2a3b7f 100644 >> --- a/src/lib/salad/bps_tree.h >> +++ b/src/lib/salad/bps_tree.h >> @@ -2147,8 +2147,11 @@ bps_tree_create_leaf(struct bps_tree *tree, bps_tree_block_id_t *id) >> { >> struct bps_leaf *res = (struct bps_leaf *) >> bps_tree_garbage_pop(tree, id); >> - if (!res) >> - res = (struct bps_leaf *)matras_alloc(&tree->matras, id); >> + if (!res) { >> + res = (struct bps_leaf *) matras_alloc(&tree->matras, id); >> + if (!res) >> + return NULL; >> + } > >Since you've changed this code anyway, lets use explicit >'== NULL' instead of '!'. -- Ilya Kosarev [-- Attachment #2: Type: text/html, Size: 2584 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-20 18:13 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-10 0:36 [Tarantool-patches] [PATCH v2 0/2] Safe truncation and deletion Ilya Kosarev 2020-01-10 0:36 ` [Tarantool-patches] [PATCH v2 1/2] b-tree: return NULL on matras_alloc fail Ilya Kosarev 2020-01-10 0:36 ` [Tarantool-patches] [PATCH v2 2/2] memtx: increase the memory quota if needed to truncate or delete Ilya Kosarev 2020-01-13 21:31 [Tarantool-patches] [PATCH v2 0/2] Safe truncation and deletion Ilya Kosarev 2020-01-13 21:31 ` [Tarantool-patches] [PATCH v2 1/2] b-tree: return NULL on matras_alloc fail Ilya Kosarev 2020-01-14 21:00 ` Vladislav Shpilevoy 2020-01-20 18:13 ` Ilya Kosarev
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox