Re: [Tarantool-patches] [PATCH v2] wal: introduce limits on simultaneous writes

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Thu, Feb 11, 2021 at 03:17:50PM +0300, Serge Petrenko wrote:
> diff --git a/src/box/applier.cc b/src/box/applier.cc
> index 553db76fc..06aaa0a79 100644
> --- a/src/box/applier.cc
> +++ b/src/box/applier.cc
> @@ -967,6 +967,15 @@ applier_apply_tx(struct applier *applier, struct stailq *rows)
>  		goto success;
>  	}
>  
> +	/*
> +	 * Do not spam WAL with excess write requests, let it process what's
> +	 * piled up first.
> +	 * This is done before opening the transaction to avoid problems with
> +	 * yielding inside it.
> +	 */
> +	if (journal_queue_is_full(current_journal))
> +		journal_wait_queue();
> +

Serge, just a few comments, feel free to ignore.

Maybe it would be better to pass current_journal to journal_wait_queue,
otherwise it looks somehow inconsistent, no?

	if (journal_queue_is_full(current_journal))
		journal_wait_queue(current_journal);

Actually I would name journal queue engine as plain journalq, this would
look like

	if (journalq_is_full(current_journal))
		journalq_wait(current_journal);

But it is very humble pov, lets stick with long `journal_queue`.
...
> +/** Wake the journal queue up. */
> +static inline void
> +journal_queue_wakeup(struct journal *j, bool force_ready)
> +{
> +	assert(j == current_journal);

Seems this assert is not needed. The overall idea of passing
journal as an argument is quite the reverse, ie to work with
any journal. This is not a blocker, could be cleaned up on top
or simply ignored.

> +	assert(!rlist_empty(&j->waiters));
> +	if (j->queue_is_woken)
> +		return;
> +	j->queue_is_woken = true;
> +	journal_queue_wakeup_next(&j->waiters, force_ready);
> +}
> +
> +/**
> + * Check whether any of the queue size limits is reached.
> + * If the queue is full, we must wait for some of the entries to be written
> + * before proceeding with a new asynchronous write request.
> + */
> +static inline bool
> +journal_queue_is_full(struct journal *j)
> +{
> +	assert(j == current_journal);

same, no need for assert()

> +	return (j->queue_max_size != 0 && j->queue_size >= j->queue_max_size) ||
> +	       (j->queue_max_len != 0 && j->queue_len >= j->queue_max_len);
> +}
> +
> +/**
> + * Check whether anyone is waiting for the journal queue to empty. If there are
> + * other waiters we must go after them to preserve write order.
> + */
> +static inline bool
> +journal_queue_has_waiters(struct journal *j)
> +{
> +	assert(j == current_journal);

same, no need for assert()

> +	return !rlist_empty(&j->waiters);
> +}
> +
> +/** Yield until there's some space in the journal queue. */
> +void
> +journal_wait_queue(void);
> +
>  /**
>   * Complete asynchronous write.
>   */
> @@ -131,15 +199,15 @@ static inline void
>  journal_async_complete(struct journal_entry *entry)
>  {
>  	assert(entry->write_async_cb != NULL);
> +	current_journal->queue_len--;
> +	current_journal->queue_size -= entry->approx_len;

Myabe worth to make queue ops closed into some helper? Because
length and size can't be updated without a tangle. IOW, something
like

static inline void
journal_queue_attr_dec(struct journal *j, struct journal_entry *entry)
{
	j->queue_len--;
	j->queue_size -= entry->approx_len;
}

static inline void
journal_queue_attr_inc(struct journal *j, struct journal_entry *entry)
{
	j->queue_len++;
	j->queue_size += entry->approx_len;
}

Again, this is my pov, **free to ignore**. attr here stands for
attributes because queue_len and queue_size are not the queue
itself but attributes which controls when we need to wait
data to be flushed.

> +	assert(current_journal->queue_len >= 0);
> +	assert(current_journal->queue_size >= 0);
> +	if (journal_queue_has_waiters(current_journal))
> +		journal_queue_wakeup(current_journal, false);
>  	entry->write_async_cb(entry);
>  }