From: Olga Arkhangelskaia <arkholga@tarantool.org> To: Nikita Pettik <korablev@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v6] memtx: fix out of memory handling for rtree Date: Tue, 11 Feb 2020 13:33:13 +0300 [thread overview] Message-ID: <61cf2d0b-e896-0fb9-a98b-493533169f92@tarantool.org> (raw) In-Reply-To: <20200210111826.GD1110@tarantool.org> Hi Nikita! Thanks for the review! I have changed everything according to your points. 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") New diff: 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; return index_replace(index, NULL, tuple, DUP_INSERT, &unused); diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c index a5cae8f45..612fcb2a9 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; }); struct memtx_engine *memtx = (struct memtx_engine *)base->engine; diff --git a/test/box/errinj.result b/test/box/errinj.result index c73fe3456..c272d288d 100644 --- a/test/box/errinj.result +++ b/test/box/errinj.result @@ -1784,7 +1784,8 @@ box.error.injection.get('ERRINJ_RELAY_TIMEOUT') - 0 ... -- --- gh-4619-RTREE-doesn't-handle-OOM-properly +-- 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"') diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua index 598e7da33..63c8b19fe 100644 --- a/test/box/errinj.test.lua +++ b/test/box/errinj.test.lua @@ -622,7 +622,8 @@ box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0) box.error.injection.get('ERRINJ_RELAY_TIMEOUT') -- --- gh-4619-RTREE-doesn't-handle-OOM-properly +-- 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")
next prev parent reply other threads:[~2020-02-11 10:33 UTC|newest] Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-02-10 7:47 Olga Arkhangelskaia 2020-02-10 11:18 ` Nikita Pettik 2020-02-11 10:33 ` Olga Arkhangelskaia [this message] 2020-02-11 10:51 ` Olga Arkhangelskaia 2020-02-11 12:42 ` Nikita Pettik 2020-02-18 12:46 ` Sergey Ostanevich 2020-02-18 18:20 ` Olga Arkhangelskaia 2020-02-19 16:28 ` Sergey Ostanevich 2020-03-03 11:50 ` Olga Arkhangelskaia 2020-03-03 14:17 ` Sergey Ostanevich 2020-03-04 21:18 ` Nikita Pettik 2020-03-10 11:25 ` Olga Arkhangelskaia 2020-03-10 13:34 ` Nikita Pettik
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=61cf2d0b-e896-0fb9-a98b-493533169f92@tarantool.org \ --to=arkholga@tarantool.org \ --cc=korablev@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v6] memtx: fix out of memory handling for rtree' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox