From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0E23D469719 for ; Tue, 3 Mar 2020 14:50:18 +0300 (MSK) References: <20200210074712.86876-1-arkholga@tarantool.org> <20200210111826.GD1110@tarantool.org> <2cbb1677-039b-9f91-cde7-9c06821653eb@tarantool.org> <20200211124245.GB5168@tarantool.org> <20200218124612.GA67746@tarantool.org> <714780e3-1383-62da-94f9-0e90542d47aa@tarantool.org> <20200219162843.GA68447@tarantool.org> From: Olga Arkhangelskaia Message-ID: <78db8579-e246-385b-6541-5cc70395be97@tarantool.org> Date: Tue, 3 Mar 2020 14:50:17 +0300 MIME-Version: 1.0 In-Reply-To: <20200219162843.GA68447@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH v6] memtx: fix out of memory handling for rtree List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Ostanevich Cc: tarantool-patches@dev.tarantool.org Hi Sergey! I need time to check everything! I can not use error injection that exists in memtx_index_extent_reserve(), because it is used  in memtx_space_replace_all_keys ant it is called prior memtx_rtree_index_reserve() . If we use memtx_index_extent_reserve(), injection we will not reach the place where error with rtree occures. So to make sure that we fixed the problem we need memtx_rtree_index_reserve() to fail. 19.02.2020 19:28, Sergey Ostanevich пишет: > Hi! > > I am sorry, I took the OKriw/gh-4619-RTREE-doesnt-handle-OOM-properly > branch for review, instead of OKriw/gh-RTREE-doesnt-handle-OOM-properly > > I can see memtx_index_extent_reserve() has an error injection already. > Is it not sufficient, so you still need a new one in > memtx_rtree_index_reserve() ? > Otherwise looks good. > > Regards, > Sergos > > > On 18 фев 21:20, Olga Arkhangelskaia wrote: >> Hi! Thanks for the review! But I am a bit confused. >> >> 18.02.2020 15:46, Sergey Ostanevich пишет: >>> Hi! >>> >>> Thanks for the patch! >>> >>> I wonder why do you use additional by-ref argument to pass an integer >>> result instead of redefining the interface into: >>> >>> int >>> rtree_insert(struct rtree *tree, struct rtree_rect *rect, record_t obj) >>> >>> instead, so that all uses of a form: >>> >>> int err = 0; >>> ... >>> rtree_insert(&index->tree, &rect, new_tuple, &err); >> >> There is no way rtree returns an error. Library itself can not fail.  And >> the best way is to reserve pages before malloc itself in the library. >> >> We have discussed in the very beginning with Konstantin Osipov and later >> with Aleksandr Lyapunov. >> >> https://lists.tarantool.org/pipermail/tarantool-patches/2019-November/012392.html >> >> So we reserve memory instead of returning an error. >> >>> if (err == 1) { >>> diag_set(OutOfMemory, sizeof(struct rtree), >>> "rtree", "rtree_page"); >>> return -1; >>> } >>> >>> into a much simpler: >>> >>> if (rtree_insert(&index->tree, &rect, new_tuple, &err)) { >>> diag_set(OutOfMemory, sizeof(struct rtree), >>> "rtree", "rtree_page"); >>> return -1; >>> } >>> >>> Sergos >>> >>> On 11 Feb 15:42, Nikita Pettik wrote: >>>> On 11 Feb 13:51, Olga Arkhangelskaia wrote: >>>>> Hi Nikita! Thanks for the review. >>>>> >>>>> I have fixed everything you have pointed out. Diff can be found below. >>>> LGTM >>>>> 10.02.2020 14:18, Nikita Pettik пишет: >>>>>> On 10 Feb 10:47, Olga Arkhangelskaia wrote: >>>>>  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")