From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (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 C482B469710 for ; Fri, 20 Nov 2020 11:33:27 +0300 (MSK) References: <268bcf29eb045de623c18c58942c07b2491658f0.1605829282.git.v.shpilevoy@tarantool.org> From: Serge Petrenko Message-ID: <1de0fec2-86e3-358c-59a8-24e83fcd1fef@tarantool.org> Date: Fri, 20 Nov 2020 11:33:26 +0300 MIME-Version: 1.0 In-Reply-To: <268bcf29eb045de623c18c58942c07b2491658f0.1605829282.git.v.shpilevoy@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH v2 10/16] raft: make worker non-cancellable during WAL write List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org 20.11.2020 02:45, Vladislav Shpilevoy пишет: > This is a general practice throughout the code. If a fiber is not > cancellable, it always means a system fiber which can't be woken > up or canceled until its work done. It is used in gc (about > xlogs), recovery, vinyl, WAL, at least. > > Before raft used flag raft.is_write_in_progress. But it won't > work soon, because the worker fiber will move to box/raft.c, > where it would be incorrect to rely on deeply internal parts of > struct raft, such as is_write_in_progress. > > Hence, this patch makes raft use a more traditional way of > spurious wakeup avoidance. > > Part of #5303 Hi! Thanks for the patch! > --- > src/box/raft.c | 9 ++++++++- > src/box/raftlib.c | 10 ++++++---- > 2 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/src/box/raft.c b/src/box/raft.c > index 2c9ed11b6..8a034687b 100644 > --- a/src/box/raft.c > +++ b/src/box/raft.c > @@ -213,7 +213,14 @@ box_raft_write(struct raft *raft, const struct raft_msg *msg) > journal_entry_create(entry, 1, xrow_approx_len(&row), box_raft_write_cb, > fiber()); > > - if (journal_write(entry) != 0 || entry->res < 0) { > + /* > + * A non-cancelable fiber is considered non-wake-able, generally. Raft > + * follows this pattern of 'protection'. > + */ > + bool cancellable = fiber_set_cancellable(false); > + bool ok = (journal_write(entry) == 0 && entry->res >= 0); > + fiber_set_cancellable(cancellable); > + if (!ok) { > diag_set(ClientError, ER_WAL_IO); > diag_log(); > goto fail; > diff --git a/src/box/raftlib.c b/src/box/raftlib.c > index d28e51871..a1fca34cd 100644 > --- a/src/box/raftlib.c > +++ b/src/box/raftlib.c > @@ -986,11 +986,13 @@ raft_worker_wakeup(struct raft *raft) > fiber_set_joinable(raft->worker, true); > } > /* > - * Don't wake the fiber if it writes something. Otherwise it would be a > - * spurious wakeup breaking the WAL write not adapted to this. Also > - * don't wakeup the current fiber - it leads to undefined behaviour. > + * Don't wake the fiber if it writes something (not cancellable). > + * Otherwise it would be a spurious wakeup breaking the WAL write not > + * adapted to this. Also don't wakeup the current fiber - it leads to > + * undefined behaviour. > */ > - if (!raft->is_write_in_progress && fiber() != raft->worker) > + if ((raft->worker->flags & FIBER_IS_CANCELLABLE) != 0 && > + fiber() != raft->worker) > fiber_wakeup(raft->worker); > } > LGTM. -- Serge Petrenko