From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 00BC62F7FF for ; Wed, 29 May 2019 14:35:10 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id zKQhxEp5V1lB for ; Wed, 29 May 2019 14:35:09 -0400 (EDT) Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id A299C2F7F9 for ; Wed, 29 May 2019 14:35:09 -0400 (EDT) Received: by smtp56.i.mail.ru with esmtpa (envelope-from ) id 1hW3Pl-0006Ww-6H for tarantool-patches@freelists.org; Wed, 29 May 2019 21:35:05 +0300 Date: Wed, 29 May 2019 21:35:03 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH 4/5] vinyl: do not allow to cancel a fiber reading a page Message-ID: <20190529183503.GD6799@atlas> References: <8086c5b179d85d01a0c7bb01b1c12182204d8a8b.1559142561.git.vdavydov.dev@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8086c5b179d85d01a0c7bb01b1c12182204d8a8b.1559142561.git.vdavydov.dev@gmail.com> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org * Vladimir Davydov [19/05/29 21:03]: > To handle fiber cancellation during page read we need to pin all objects > referenced by vy_page_read_task. Currently, there's the only such > object, vy_run. It has reference counting so pinning it is trivial. > However, to move page lookup to a reader thread, we need to also > reference key def, tuple format, and key. Format and key have reference > counting, but key def doesn't - we typically copy it. Copying it in this > case is too heavy. Let's simply drop this functionality - nothing bad > happens if a cancelled fiber won't exit until disk read is complete. This is perhaps a relevant statement with PCIE attached flash drives. It used to be not true with rotating disks, since a rotating disk controller could retry reading a block indefinitely on read failure. It is still dangerously false with Network Attached Storage. On the other hand this has never been tested. What is not tested, can and should be removed. So I agree let's kill this branch for now, we will need to re-think query timeout handling for entire call chain as soon as Tarantool is more often used for complex SQL queries, so we'll be forced to rethink this. Please at least call testcancel() after the message has returned. This agreement also withdraws my comment about the previous patch. Please also document carefully the reasoning why we decided to remove timeout handling in the code not just in the changeset comment. Otherwise LGTM. > --- > src/box/vy_run.c | 51 +++++++++++++-------------------------------------- > 1 file changed, 13 insertions(+), 38 deletions(-) > > diff --git a/src/box/vy_run.c b/src/box/vy_run.c > index a8ca3afe..1dc7271f 100644 > --- a/src/box/vy_run.c > +++ b/src/box/vy_run.c > @@ -198,18 +198,14 @@ vy_run_env_enable_coio(struct vy_run_env *env) > > /** > * Execute a task on behalf of a reader thread. > - * Calls free_cb on failure. > */ > static int > vy_run_env_coio_call(struct vy_run_env *env, struct cbus_call_msg *msg, > - cbus_call_f func, cbus_call_f free_cb, double timeout) > + cbus_call_f func) > { > /* Optimization: use blocking I/O during WAL recovery. */ > - if (env->reader_pool == NULL) { > - if (func(msg) != 0) > - goto fail; > - return 0; > - } > + if (env->reader_pool == NULL) > + return func(msg); > > /* Pick a reader thread. */ > struct vy_run_reader *reader; > @@ -217,21 +213,14 @@ vy_run_env_coio_call(struct vy_run_env *env, struct cbus_call_msg *msg, > env->next_reader %= env->reader_pool_size; > > /* Post the task to the reader thread. */ > + bool cancellable = fiber_set_cancellable(false); > int rc = cbus_call(&reader->reader_pipe, &reader->tx_pipe, > - msg, func, free_cb, timeout); > - if (!msg->complete) { > - /* > - * Operation timed out or the fiber was cancelled. > - * free_cb will be called on task completion. > - */ > - return -1; > - } > + msg, func, NULL, TIMEOUT_INFINITY); > + fiber_set_cancellable(cancellable); > if (rc != 0) > - goto fail; > + return -1; > + > return 0; > -fail: > - free_cb(msg); > - return -1; > } > > /** > @@ -986,20 +975,6 @@ vy_page_read_cb(struct cbus_call_msg *base) > } > > /** > - * vinyl read task cleanup callback > - */ > -static int > -vy_page_read_cb_free(struct cbus_call_msg *base) > -{ > - struct vy_page_read_task *task = (struct vy_page_read_task *)base; > - struct vy_run_env *env = task->run->env; > - vy_page_delete(task->page); > - vy_run_unref(task->run); > - mempool_free(&env->read_task_pool, task); > - return 0; > -} > - > -/** > * Read a page from disk given its number. > * The function caches two most recently read pages. > * > @@ -1044,14 +1019,14 @@ vy_run_iterator_load_page(struct vy_run_iterator *itr, uint32_t page_no, > task->run = slice->run; > task->page_info = page_info; > task->page = page; > - vy_run_ref(task->run); > > - if (vy_run_env_coio_call(env, &task->base, vy_page_read_cb, > - vy_page_read_cb_free, TIMEOUT_INFINITY) != 0) > - return -1; > + int rc = vy_run_env_coio_call(env, &task->base, vy_page_read_cb); > > - vy_run_unref(task->run); > mempool_free(&env->read_task_pool, task); > + if (rc != 0) { > + vy_page_delete(page); > + return -1; > + } > > /* Update cache */ > if (itr->prev_page != NULL) > -- > 2.11.0 > -- Konstantin Osipov, Moscow, Russia