[Tarantool-patches] [PATCH v2] wal: introduce limits on simultaneous writes
Cyrill Gorcunov
gorcunov at gmail.com
Mon Feb 15 14:17:59 MSK 2021
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);
> }
More information about the Tarantool-patches
mailing list