From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 8A22646970E for ; Tue, 21 Jan 2020 23:59:30 +0300 (MSK) References: <45d9f491f91d93c8835ca725c234f7113c0d2aa8.1579541242.git.i.kosarev@tarantool.org> <20200121114253.GH82780@tarantool.org> From: Vladislav Shpilevoy Message-ID: <0b514379-806d-c66c-8c68-8780572f16ff@tarantool.org> Date: Tue, 21 Jan 2020 21:59:28 +0100 MIME-Version: 1.0 In-Reply-To: <20200121114253.GH82780@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik , Ilya Kosarev Cc: tarantool-patches@dev.tarantool.org Thanks for the patch! I answer some of Nikita's questions below and provide my own review further. See 7 comments below. On 21/01/2020 12:42, Nikita Pettik wrote: > On 20 Jan 21:13, Ilya Kosarev wrote: >> diff --git a/src/box/box.cc b/src/box/box.cc >> index 1b2b27d61..678c9ffe3 100644 >> --- a/src/box/box.cc >> +++ b/src/box/box.cc >> @@ -1321,9 +1321,14 @@ space_truncate(struct space *space) >> ops_buf_end = mp_encode_uint(ops_buf_end, 1); >> assert(ops_buf_end < buf + buf_size); >> >> + struct space *truncate_space = space_cache_find_xc(BOX_TRUNCATE_ID); >> + memtx_engine_set_quota_strictness(truncate_space->engine, false); > > I wouldn't call it strictness. In fact you simply turn quota on/off. > So I'd better call it quota_enable/disable. > > Nit: please accompany your code with (at least brief) comments. 1. On/off sounds good. >> + >> if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end, >> ops_buf, ops_buf_end, 0, NULL) != 0) >> diag_raise(); > > If box_upsert() fails, quota is not re-enabled, isn't it? 2. This is a bug, yes. But not the only one. The upsert will write to WAL and yield. During that yield the quota is turned off. And any other fiber can interfere and overuse the quota even more with something not related to delete/truncate. This can be fixed, for example, by making txn_begin() + quota_off() + upsert() + quota_on() + commit(). Also we could just not touch truncate and fix delete() only. Delete() can never leave the quota overused, because is not stored anywhere in memtx. While truncate is stored in _truncate. And delete would be sufficient, because can be used to free the quota enough so as to call truncate()/whatever else the user wanted. >> + >> + memtx_engine_set_quota_strictness(truncate_space->engine, true); >> } >> >> int >> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c >> index 4da80824a..c19205d34 100644 >> --- a/src/box/memtx_engine.c >> +++ b/src/box/memtx_engine.c >> @@ -1090,6 +1090,15 @@ memtx_engine_set_memory(struct memtx_engine *memtx, size_t size) >> return 0; >> } >> >> +void >> +memtx_engine_set_quota_strictness(struct engine *engine, bool strict) >> +{ >> + if (strncmp(engine->name, "memtx", 5) != 0) >> + return; > > memtx_ prefix in the name of this function assumes that it is called > only for memtx engine. Note that there's no such checks in memtx_engine > interface functions. > >> + struct memtx_engine *memtx = (struct memtx_engine *)engine; >> + quota_set_strictness(&memtx->quota, strict); > > What happens if quota is turned on back, but it still exceeds limit? > I guess it would turn out to be in inconsistent state, since in > normal operation mode there's no chance of being beyond the limit. 3. That problem is not solvable. There is always a chance, that truncate won't free any memory (for example, the target space could be empty already). If, after the quota is back on, it appears to be overused, nothing can be done with it. It will stay overused until more delete/truncate will be executed and actually delete something, or the user will increase memtx memory. > commit 45d9f491f91d93c8835ca725c234f7113c0d2aa8 > Author: Ilya Kosarev > Date: Mon Jan 20 20:21:37 2020 +0300 > > memtx: allow quota overuse for truncation and deletion > > > 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 allowing > to overuse memtx quota if needed for truncation or deletion, using flag > in struct quota. After performing truncation or deletion we reset the > flag so that quota can be overused any more. > > Closes #3807 > > diff --git a/src/box/box.cc b/src/box/box.cc > index 1b2b27d61..678c9ffe3 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -1321,9 +1321,14 @@ space_truncate(struct space *space) > ops_buf_end = mp_encode_uint(ops_buf_end, 1); > assert(ops_buf_end < buf + buf_size); > > + struct space *truncate_space = space_cache_find_xc(BOX_TRUNCATE_ID); > + memtx_engine_set_quota_strictness(truncate_space->engine, false); > + > if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end, > ops_buf, ops_buf_end, 0, NULL) != 0) > diag_raise(); > + > + memtx_engine_set_quota_strictness(truncate_space->engine, true); > } 4. IMO, we should not touch quota at all until we see a not empty diag with OOM error type in it. Both here and in delete. Truncate is not a frequent operation (although it may be in some rare installations), but delete surely should not touch quota each time. > int > diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c > index 4da80824a..c19205d34 100644 > --- a/src/box/memtx_engine.c > +++ b/src/box/memtx_engine.c > @@ -1090,6 +1090,15 @@ memtx_engine_set_memory(struct memtx_engine *memtx, size_t size) > return 0; > } > > +void > +memtx_engine_set_quota_strictness(struct engine *engine, bool strict) > +{ > + if (strncmp(engine->name, "memtx", 5) != 0) > + return; 5. That check should not exist. It slows down the caller, and in upper code you always know whether you work with memtx or not. So it makes no sense to check it here. Truncate is always about insertion into a memtx space. Delete touches qouta inside memtx_space implementation. No chances that a vinyl space would call this function. > + struct memtx_engine *memtx = (struct memtx_engine *)engine; > + quota_set_strictness(&memtx->quota, strict); > +} > + > void > memtx_engine_set_max_tuple_size(struct memtx_engine *memtx, size_t max_size) > { > diff --git a/src/lib/small b/src/lib/small > index 50cb78743..bdcd569f9 160000 > --- a/src/lib/small > +++ b/src/lib/small > @@ -1 +1 @@ > -Subproject commit 50cb7874380b286f3b34c82299cf5eb88b0d0059 > +Subproject commit bdcd569f9ed753efb805f708089ca86e2520a44f 6. Please, write tests. They should include the bug test itself, and complex things such as quota overuse kept in case truncate() didn't free anything. You can use memtx_memory parameter to make it small enough so as the test would not take much time. 7. Seems like you changed something in small/. You need to send it as a separate patch with its own tests, and all.