[Tarantool-patches] [PATCH v2 0/2] Safe truncation and deletion

[Tarantool-patches] [PATCH v2 0/2] Safe truncation and deletion

[Ilya Kosarev]

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

[Tarantool-patches] [PATCH v2 1/2] b-tree: return NULL on matras_alloc fail

[Ilya Kosarev]

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

Re: [Tarantool-patches] [PATCH v2 1/2] b-tree: return NULL on matras_alloc fail

[Vladislav Shpilevoy]

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 '!'.

Re: [Tarantool-patches] [PATCH v2 1/2] b-tree: return NULL on matras_alloc fail

[Ilya Kosarev]

[-- 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 --]

[Tarantool-patches] [PATCH v2 2/2] memtx: increase the memory quota if needed to truncate or delete

[Ilya Kosarev]

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/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 +
 9 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/src/box/blackhole.c b/src/box/blackhole.c
index 69f1deba1..af587f434 100644
--- a/src/box/blackhole.c
+++ b/src/box/blackhole.c
@@ -194,6 +194,7 @@ static const struct engine_vtab blackhole_engine_vtab = {
 	/* .commit_checkpoint = */ generic_engine_commit_checkpoint,
 	/* .abort_checkpoint = */ generic_engine_abort_checkpoint,
 	/* .collect_garbage = */ generic_engine_collect_garbage,
+	/* .guarantee_memory = */ generic_engine_guarantee_memory,
 	/* .backup = */ generic_engine_backup,
 	/* .memory_stat = */ generic_engine_memory_stat,
 	/* .reset_stat = */ generic_engine_reset_stat,
diff --git a/src/box/box.cc b/src/box/box.cc
index 1b2b27d61..18c09ce1b 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1250,7 +1250,27 @@ 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;
+	size_t total;
+	bool extended = false;
+	space->engine->vtab->guarantee_memory(space->engine,
+					      MEMTX_SLAB_SIZE,
+					      &total, &extended);
+
+	int rc = box_process1(&request, result);
+
+	if (extended) {
+		struct memtx_engine *memtx =
+			(struct memtx_engine *)space->engine;
+		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 +1341,23 @@ space_truncate(struct space *space)
 	ops_buf_end = mp_encode_uint(ops_buf_end, 1);
 	assert(ops_buf_end < buf + buf_size);
 
+	size_t total;
+	bool extended = false;
+	space->engine->vtab->guarantee_memory(space->engine,
+					      MEMTX_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) {
+		struct memtx_engine *memtx =
+			(struct memtx_engine *)space->engine;
+		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/engine.c b/src/box/engine.c
index 8dc0df1d0..f393a2629 100644
--- a/src/box/engine.c
+++ b/src/box/engine.c
@@ -412,6 +412,17 @@ generic_engine_memory_stat(struct engine *engine,
 	(void)stat;
 }
 
+void
+generic_engine_guarantee_memory(struct engine *engine,
+				size_t request, size_t *old_total,
+				bool *extended)
+{
+	(void)engine;
+	(void)request;
+	*old_total = 0;
+	*extended = false;
+}
+
 void
 generic_engine_reset_stat(struct engine *engine)
 {
diff --git a/src/box/engine.h b/src/box/engine.h
index 07d7fac9b..d1e3e998f 100644
--- a/src/box/engine.h
+++ b/src/box/engine.h
@@ -185,6 +185,14 @@ struct engine_vtab {
 	 */
 	void (*collect_garbage)(struct engine *engine,
 				const struct vclock *vclock);
+	/**
+	 * Performing space:truncate() or space:delete() while reaching
+	 * memory limit might lead to slab allocator failure. To avoid
+	 * it, we temporally increase memory quota using this function.
+	 */
+        void (*guarantee_memory)(struct engine *engine,
+                                  size_t request, size_t *old_total,
+                                  bool *extended);
 	/**
 	 * Backup callback. It is supposed to call @cb for each file
 	 * that needs to be backed up in order to restore from the
@@ -404,6 +412,7 @@ void generic_engine_collect_garbage(struct engine *, const struct vclock *);
 int generic_engine_backup(struct engine *, const struct vclock *,
 			  engine_backup_cb, void *);
 void generic_engine_memory_stat(struct engine *, struct engine_memory_stat *);
+void generic_engine_guarantee_memory(struct engine *engine, size_t request, size_t *old_total, bool *extended);
 void generic_engine_reset_stat(struct engine *);
 int generic_engine_check_space_def(struct space_def *);
 
diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index 23ccc4703..6c80b5919 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -927,6 +927,7 @@ static const struct engine_vtab memtx_engine_vtab = {
 	/* .commit_checkpoint = */ memtx_engine_commit_checkpoint,
 	/* .abort_checkpoint = */ memtx_engine_abort_checkpoint,
 	/* .collect_garbage = */ memtx_engine_collect_garbage,
+	/* .guarantee_memory = */ memtx_engine_guarantee_memory,
 	/* .backup = */ memtx_engine_backup,
 	/* .memory_stat = */ memtx_engine_memory_stat,
 	/* .reset_stat = */ generic_engine_reset_stat,
@@ -1090,6 +1091,25 @@ memtx_engine_set_memory(struct memtx_engine *memtx, size_t size)
 	return 0;
 }
 
+void
+memtx_engine_guarantee_memory(struct engine *engine,
+			      size_t request, size_t *old_total,
+			      bool *extended)
+{
+	struct quota *memtx_quota =
+		&((struct memtx_engine *)engine)->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..b8489fffe 100644
--- a/src/box/memtx_engine.h
+++ b/src/box/memtx_engine.h
@@ -213,6 +213,10 @@ 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 engine *engine, size_t request,
+			      size_t *old_total, bool *extended);
+
 void
 memtx_engine_set_max_tuple_size(struct memtx_engine *memtx, size_t max_size);
 
diff --git a/src/box/service_engine.c b/src/box/service_engine.c
index 5a33a735a..cfbd4ee60 100644
--- a/src/box/service_engine.c
+++ b/src/box/service_engine.c
@@ -112,6 +112,7 @@ static const struct engine_vtab service_engine_vtab = {
 	/* .commit_checkpoint = */ generic_engine_commit_checkpoint,
 	/* .abort_checkpoint = */ generic_engine_abort_checkpoint,
 	/* .collect_garbage = */ generic_engine_collect_garbage,
+	/* .guarantee_memory = */ generic_engine_guarantee_memory,
 	/* .backup = */ generic_engine_backup,
 	/* .memory_stat = */ generic_engine_memory_stat,
 	/* .reset_stat = */ generic_engine_reset_stat,
diff --git a/src/box/sysview.c b/src/box/sysview.c
index 00c320b6f..bfc7bd1dd 100644
--- a/src/box/sysview.c
+++ b/src/box/sysview.c
@@ -584,6 +584,7 @@ static const struct engine_vtab sysview_engine_vtab = {
 	/* .commit_checkpoint = */ generic_engine_commit_checkpoint,
 	/* .abort_checkpoint = */ generic_engine_abort_checkpoint,
 	/* .collect_garbage = */ generic_engine_collect_garbage,
+	/* .guarantee_memory = */ generic_engine_guarantee_memory,
 	/* .backup = */ generic_engine_backup,
 	/* .memory_stat = */ generic_engine_memory_stat,
 	/* .reset_stat = */ generic_engine_reset_stat,
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 5f169f09b..9343423c5 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -4489,6 +4489,7 @@ static const struct engine_vtab vinyl_engine_vtab = {
 	/* .commit_checkpoint = */ vinyl_engine_commit_checkpoint,
 	/* .abort_checkpoint = */ vinyl_engine_abort_checkpoint,
 	/* .collect_garbage = */ vinyl_engine_collect_garbage,
+	/* .guarantee_memory = */ generic_engine_guarantee_memory,
 	/* .backup = */ vinyl_engine_backup,
 	/* .memory_stat = */ vinyl_engine_memory_stat,
 	/* .reset_stat = */ vinyl_engine_reset_stat,
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v2 2/2] memtx: increase the memory quota if needed to truncate or delete

[Vladislav Shpilevoy]

Thanks for the patch!

JFI, I am still against this patch. It adds huge and
unnecessary complexity to the code, which we will need
to support forever. It is just not worth the pros the
patch gives.

On 13/01/2020 22:31, Ilya Kosarev wrote:
> 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/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 +
>  9 files changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/blackhole.c b/src/box/blackhole.c
> index 69f1deba1..af587f434 100644
> --- a/src/box/blackhole.c
> +++ b/src/box/blackhole.c
> @@ -194,6 +194,7 @@ static const struct engine_vtab blackhole_engine_vtab = {
>  	/* .commit_checkpoint = */ generic_engine_commit_checkpoint,
>  	/* .abort_checkpoint = */ generic_engine_abort_checkpoint,
>  	/* .collect_garbage = */ generic_engine_collect_garbage,
> +	/* .guarantee_memory = */ generic_engine_guarantee_memory,

The only problem is with memtx engine, and I propose to solve it
on memtx engine level. Vinyl will never need this method.

(But even better I propose to drop the patch and close the issue as
won't fix.)

>  	/* .backup = */ generic_engine_backup,
>  	/* .memory_stat = */ generic_engine_memory_stat,
>  	/* .reset_stat = */ generic_engine_reset_stat,
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 1b2b27d61..18c09ce1b 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1321,9 +1341,23 @@ space_truncate(struct space *space)
>  	ops_buf_end = mp_encode_uint(ops_buf_end, 1);
>  	assert(ops_buf_end < buf + buf_size);
>  
> +	size_t total;
> +	bool extended = false;
> +	space->engine->vtab->guarantee_memory(space->engine,
> +					      MEMTX_SLAB_SIZE,
> +					      &total, &extended);
> +

Truncate is always about insertion into the memtx space _truncate.
Here you are calling 'guarantee_memory' for the user space's engine.
And it just won't work in case I try to truncate a vinyl space.

Moreover, the encapsulation of 'memory guarantee' is broken anyway,
because 1) you pass 'MEMTX_SLAB_SIZE' parameter to the engine's
virtual method, 2) below you touch memtx engine explicitly.

>  	if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end,
>  		       ops_buf, ops_buf_end, 0, NULL) != 0)
>  		diag_raise();
> +
> +	if (extended) {
> +		struct memtx_engine *memtx =
> +			(struct memtx_engine *)space->engine;

Why is space->engine assumed to be memtx? This is a user's space.
It can be vinyl.

> +		size_t new_total = quota_set(&memtx->quota, total);
> +		if (new_total > total)
> +			quota_set(&memtx->quota, quota_used(&memtx->quota));
> +	}
>  }

Since this is a bug fix, there should be a regression test.

But I once again say, that it is even better to drop the patch and
close the issue. This is just sugar, which besides may lead to a side
effect when quota is increased, but can't be decreased back.

I can't find a way how to fix it gracefully and simple.

Especially for truncate which is a total disaster. The space, you have
truncated, could be empty, but you will insert a new tuple into _truncate,
and it will be kept, and will occupy memory. No memory is freed. And you
won't be able to decrease the quota.

AFAIR, at this moment there is no necessity in having _truncate space.
There was something about vylog, why we added that space, and from what
I remember, that problem has already gone. We could just drop _truncate.
Although its existence also may be related to replication. It should be
checked.

Below is an idea I had recently when was thinking about the issue. I don't
think it is good and finished, but maybe it will help you to evolve it to
something better. We could allow to overuse quota. For example, add a flag
to struct quota like 'is_soft'. It is false by default. When quota is not
soft, it works just like now. When we want to allocate something above the
quota, we set the flag to true. Now the quota is soft, and any alloc
succeeds (unless there is no memory in the system, of course).

We set the flag in memtx_space_execute_delete() before
memtx_space->replace(space, old_tuple, NULL), and unset right after it.

For truncate we set this flag in space_truncate() before box_upsert(), and
unset right afterwards (just like you did with guarantee_memory()).

That allows us to overuse quota, but don't touch quota limit. In case the
quota is overused, and is not soft, it behaves just like when it has
reached the limit - does not allow new allocations.

Re: [Tarantool-patches] [PATCH v2 2/2] memtx: increase the memory quota if needed to truncate or delete

[Ilya Kosarev]

[-- Attachment #1: Type: text/plain, Size: 6009 bytes --]


Thanks for the review!
 
Ok, I see, this one is still too clumsy. Also i messed up user space
and service engines. There are 5 answers below.
 
Let’s try one more. Sent v3 with a new approach based on your idea.
 
>Среда, 15 января 2020, 0:00 +03:00 от Vladislav Shpilevoy < v.shpilevoy@tarantool.org >:
>  
>Thanks for the patch!
>
>JFI, I am still against this patch. It adds huge and
>unnecessary complexity to the code, which we will need
>to support forever. It is just not worth the pros the
>patch gives.
>
>On 13/01/2020 22:31, Ilya Kosarev wrote:
>> 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/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 +
>> 9 files changed, 83 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/box/blackhole.c b/src/box/blackhole.c
>> index 69f1deba1..af587f434 100644
>> --- a/src/box/blackhole.c
>> +++ b/src/box/blackhole.c
>> @@ -194,6 +194,7 @@ static const struct engine_vtab blackhole_engine_vtab = {
>> /* .commit_checkpoint = */ generic_engine_commit_checkpoint,
>> /* .abort_checkpoint = */ generic_engine_abort_checkpoint,
>> /* .collect_garbage = */ generic_engine_collect_garbage,
>> + /* .guarantee_memory = */ generic_engine_guarantee_memory,
>
>The only problem is with memtx engine, and I propose to solve it
>on memtx engine level. Vinyl will never need this method.
>
>(But even better I propose to drop the patch and close the issue as
>won't fix.)
>
>> /* .backup = */ generic_engine_backup,
>> /* .memory_stat = */ generic_engine_memory_stat,
>> /* .reset_stat = */ generic_engine_reset_stat,
>> diff --git a/src/box/box.cc b/src/box/box.cc
>> index 1b2b27d61..18c09ce1b 100644
>> --- a/src/box/box.cc
>> +++ b/src/box/box.cc
>> @@ -1321,9 +1341,23 @@ space_truncate(struct space *space)
>> ops_buf_end = mp_encode_uint(ops_buf_end, 1);
>> assert(ops_buf_end < buf + buf_size);
>>
>> + size_t total;
>> + bool extended = false;
>> + space->engine->vtab->guarantee_memory(space->engine,
>> + MEMTX_SLAB_SIZE,
>> + &total, &extended);
>> +
>
>Truncate is always about insertion into the memtx space _truncate.
>Here you are calling 'guarantee_memory' for the user space's engine.
>And it just won't work in case I try to truncate a vinyl space.
>
>Moreover, the encapsulation of 'memory guarantee' is broken anyway,
>because 1) you pass 'MEMTX_SLAB_SIZE' parameter to the engine's
>virtual method, 2) below you touch memtx engine explicitly.
*  
Right, this won't work in case user engine is vinyl.
>
>> if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end,
>> ops_buf, ops_buf_end, 0, NULL) != 0)
>> diag_raise();
>> +
>> + if (extended) {
>> + struct memtx_engine *memtx =
>> + (struct memtx_engine *)space->engine;
>
>Why is space->engine assumed to be memtx? This is a user's space.
>It can be vinyl.
*  
Well, extended may only be true if the engine is memtx.
>
>> + size_t new_total = quota_set(&memtx->quota, total);
>> + if (new_total > total)
>> + quota_set(&memtx->quota, quota_used(&memtx->quota));
>> + }
>> }
>
>Since this is a bug fix, there should be a regression test.
*  
I can’t really see suitable regression test.
>
>But I once again say, that it is even better to drop the patch and
>close the issue. This is just sugar, which besides may lead to a side
>effect when quota is increased, but can't be decreased back.
>
>I can't find a way how to fix it gracefully and simple.
>
>Especially for truncate which is a total disaster. The space, you have
>truncated, could be empty, but you will insert a new tuple into _truncate,
>and it will be kept, and will occupy memory. No memory is freed. And you
>won't be able to decrease the quota.
>
>AFAIR, at this moment there is no necessity in having _truncate space.
>There was something about vylog, why we added that space, and from what
>I remember, that problem has already gone. We could just drop _truncate.
>Although its existence also may be related to replication. It should be
>checked.
*  
As far as i see, _truncate space was introduced in
https://github.com/tarantool/tarantool/commit/353bcdc5d0102e20c88ad910f106156d3dd2d9da
and is needed not only for vylog handling, but also for atomic
internal truncation using trigger.
>
>Below is an idea I had recently when was thinking about the issue. I don't
>think it is good and finished, but maybe it will help you to evolve it to
>something better. We could allow to overuse quota. For example, add a flag
>to struct quota like 'is_soft'. It is false by default. When quota is not
>soft, it works just like now. When we want to allocate something above the
>quota, we set the flag to true. Now the quota is soft, and any alloc
>succeeds (unless there is no memory in the system, of course).
>
>We set the flag in memtx_space_execute_delete() before
>memtx_space->replace(space, old_tuple, NULL), and unset right after it.
>
>For truncate we set this flag in space_truncate() before box_upsert(), and
>unset right afterwards (just like you did with guarantee_memory()).
>
>That allows us to overuse quota, but don't touch quota limit. In case the
>quota is overused, and is not soft, it behaves just like when it has
>reached the limit - does not allow new allocations.
*  
Thanks for the idea! I tried to implement it in v3.
 
 
--
Ilya Kosarev
 
 

[-- Attachment #2: Type: text/html, Size: 10688 bytes --]

Re: [Tarantool-patches] [PATCH v2 2/2] memtx: increase the memory quota if needed to truncate or delete

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [20/01/15 00:03]:
> Thanks for the patch!
> 
> JFI, I am still against this patch. It adds huge and
> unnecessary complexity to the code, which we will need
> to support forever. It is just not worth the pros the
> patch gives.

I don't get why insertion of truncation tuple can not use the
same reserved memory segments which are used for RTREE recovery.
We can just one +1 blog to the reserved list to accommodate for
truncate.


-- 
Konstantin Osipov, Moscow, Russia

[Tarantool-patches] [PATCH v2 0/2] Safe truncation and deletion

[Ilya Kosarev]

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

[Tarantool-patches] [PATCH v2 2/2] memtx: increase the memory quota if needed to truncate or delete

[Ilya Kosarev]

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