Re: [Tarantool-patches] [PATCH v6] memtx: fix out of memory handling for rtree

[Olga Arkhangelskaia <arkholga@tarantool.org>]

Hi Nikita! Thanks for the review.

I have fixed everything you have pointed out. Diff can be found below.

10.02.2020 14:18, Nikita Pettik пишет:
> On 10 Feb 10:47, Olga Arkhangelskaia wrote:
>> Changes in v2:
>> - changed way of error handling, now we reserve pages before go to rtree lib
>> - changed test
>> - changed commit msg
>>
>> Changes in v3:
>> - added memtx_rtree_build_next function
>> - memory reservation is moved to memtx_rtree_build_next
>>
>> Changes in v4:
>> - added index reservation in build_next
>> - added memtx_rtree_reserve implementation
>>
>> Changes in v5:
>> - added error injection
>> - test is placed in errorinj.test.lua
>> - test is much faster now
>> - fixed indentation errors
>>
>> Changes in v6:
>> - fixed indentation
>> - fixed test output
> Bump patch version only in case functional changes are significant
> enough.
>
> Please, do not send new patch in response to the current review,
> simply inline your answers like "applied/skipped" etc.
>
>  From technical point of view patch LGTM. A few suggestions concerning
> code documentation below (skip them or apply - it is up to you).
> I guess smb else should look at the patch before it is pushed to
> master (as a rule 2 LGTMs are enough).
>
>> diff --git a/src/box/index.cc b/src/box/index.cc
>> index 4e4867118..750838bef 100644
>> --- a/src/box/index.cc
>> +++ b/src/box/index.cc
>> @@ -733,6 +733,8 @@ int
>>   generic_index_build_next(struct index *index, struct tuple *tuple)
>>   {
>>   	struct tuple *unused;
> I'd add a brief comment here:
>
> diff --git a/src/box/index.cc b/src/box/index.cc
> index 750838bef..c2fc00867 100644
> --- a/src/box/index.cc
> +++ b/src/box/index.cc
> @@ -733,6 +733,11 @@ int
>   generic_index_build_next(struct index *index, struct tuple *tuple)
>   {
>          struct tuple *unused;
> +       /*
> +        * Note this is not no-op call in case of rtee index:
> +        * reserving 0 bytes is required during rtree recovery.
> +        * For details see memtx_rtree_index_reserve().
> +        */
>          if (index_reserve(index, 0) != 0)
>                  return -1;
>
>> +	if (index_reserve(index, 0) != 0)
>> +		return -1;
>>   	return index_replace(index, NULL, tuple, DUP_INSERT, &unused);
>>   }
>>   
>> --- a/src/box/memtx_rtree.c
>> +++ b/src/box/memtx_rtree.c
>> @@ -242,6 +242,23 @@ memtx_rtree_index_replace(struct index *base, struct tuple *old_tuple,
>>   	return 0;
>>   }
>>   
>> +static int
>> +memtx_rtree_index_reserve(struct index *base, uint32_t size_hint)
>> +{
>> +	/*
>> +	 * In case of rtree we use reserve to make sure that memory allocation
>> +	 * will not fail durig any operationin rtree, because there is no
>> +	 * error handling in rtree lib.
>> +	 */
>> +	(void)size_hint;
>> +	ERROR_INJECT(ERRINJ_INDEX_RESERVE, {
>> +		diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, "slab allocator", "memtx extent");
>> +		return -1;
>> +	});
> Consider this small fix:
>
> diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
> index a5cae8f45..590321a7e 100644
> --- a/src/box/memtx_rtree.c
> +++ b/src/box/memtx_rtree.c
> @@ -246,13 +246,14 @@ static int
>   memtx_rtree_index_reserve(struct index *base, uint32_t size_hint)
>   {
>          /*
> -        * In case of rtree we use reserve to make sure that memory allocation
> -        * will not fail durig any operationin rtree, because there is no
> -        * error handling in rtree lib.
> +        * In case of rtree we use reserve to make sure that
> +        * memory allocation will not fail during any operation
> +        * on rtree, because there is no error handling in the
> +        * rtree lib.
>           */
>          (void)size_hint;
>          ERROR_INJECT(ERRINJ_INDEX_RESERVE, {
> -               diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, "slab allocator", "memtx extent");
> +               diag_set(OutOfMemory, MEMTX_EXTENT_SIZE,"mempool", "new slab");
>                  return -1;
>          });
>
>> diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
>> index 03c088677..598e7da33 100644
>> --- a/test/box/errinj.test.lua
>> +++ b/test/box/errinj.test.lua
>> @@ -620,3 +620,30 @@ box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0.5)
>>   box.error.injection.get('ERRINJ_RELAY_TIMEOUT')
>>   box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0)
>>   box.error.injection.get('ERRINJ_RELAY_TIMEOUT')
>> +
>> +--
>> +-- gh-4619-RTREE-doesn't-handle-OOM-properly
>> +--
> I'd a bit extend this comment like:
>
> -- gh-4619: make sure that if OOM takes place during rtree recovery,
> -- Tarantool instance will fail gracefully.
>
>> +test_run:cmd('create server rtree with script = "box/lua/cfg_rtree.lua"')
>> +test_run:cmd("start server rtree")
>> +test_run:cmd('switch rtree')
>> +math = require("math")
  diff --git a/src/box/index.cc b/src/box/index.cc
-index 4e4867118..c2fc00867 100644
+index 4e4867118..750838bef 100644
  --- a/src/box/index.cc
  +++ b/src/box/index.cc
-@@ -733,6 +733,13 @@ int
+@@ -733,6 +733,8 @@ int
   generic_index_build_next(struct index *index, struct tuple *tuple)
{
         struct tuple *unused;
-+ /*
-+       * Note this is not no-op call in case of rtee index:
-+       * reserving 0 bytes is required during rtree recovery.
-+       * For details see memtx_rtree_index_reserve().
-+ */
  +      if (index_reserve(index, 0) != 0)
  +              return -1;

diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
-index 8badad797..612fcb2a9 100644
+index 8badad797..a5cae8f45 100644
  --- a/src/box/memtx_rtree.c
  +++ b/src/box/memtx_rtree.c
-@@ -242,6 +242,24 @@ memtx_rtree_index_replace(struct index *base, 
struct tuple *old_tuple,
+@@ -242,6 +242,23 @@ memtx_rtree_index_replace(struct index *base, 
struct tuple *old_tuple,
         return 0;
}

@@ -82,14 +109,13 @@
  +memtx_rtree_index_reserve(struct index *base, uint32_t size_hint)
  +{
  + /*
-+         * In case of rtree we use reserve to make sure that
-+         * memory allocation will not fail during any operation
-+         * on rtree, because there is no error handling in the
-+         * rtree lib.
-+ */
++       * In case of rtree we use reserve to make sure that memory 
allocation
++       * will not fail durig any operationin rtree, because there is no
++       * error handling in rtree lib.
++ */
  + (void)size_hint;
  +      ERROR_INJECT(ERRINJ_INDEX_RESERVE, {
-+              diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, "mempool", "new 
slab");
++              diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, "slab 
allocator", "memtx extent");
  +              return -1;
  + });
  +      struct memtx_engine *memtx = (struct memtx_engine *)base->engine;

  diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
-index 03c088677..63c8b19fe 100644
+index 03c088677..598e7da33 100644
  --- a/test/box/errinj.test.lua
  +++ b/test/box/errinj.test.lua
-@@ -620,3 +620,31 @@ box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0.5)
+@@ -620,3 +620,30 @@ box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0.5)
box.error.injection.get('ERRINJ_RELAY_TIMEOUT')
   box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0)
box.error.injection.get('ERRINJ_RELAY_TIMEOUT')
  +
  +--
-+-- gh-4619: make sure that if OOM takes place during rtree recovery,
-+-- Tarantool instance will fail gracefully.
++-- gh-4619-RTREE-doesn't-handle-OOM-properly
  +--
  +test_run:cmd('create server rtree with script = 
"box/lua/cfg_rtree.lua"')
  +test_run:cmd("start server rtree")