Re: [Tarantool-patches] [PATCH v2 10/16] raft: make worker non-cancellable during WAL write

[Serge Petrenko <sergepetrenko@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