[tarantool-patches] Re: [PATCH v4 5/9] wal: a dedicated wal scheduling fiber

[Konstantin Osipov <kostja@tarantool.org>]

* Georgy Kirichenko <georgy@tarantool.org> [19/06/20 09:54]:

First of all I think this patch is small enough to be collapsed
with the subsequent patch which uses the machinery.

Vova will remove yield from vinyl pretty soon, so we might not
need this patch after all.

> +	/** A queue to schedule journal entry completions. */
> +	struct stailq schedule_queue;
> +	/** True if writer is in rollback state. */
> +	bool is_in_rollback;


Why do you need a separate boolean variable? There is plenty of
state which could be used to check that we're in rollback already.

> +	/** A condition to signal about new schedule queue entries. */
> +	struct fiber_cond schedule_cond;
>  	/* ----------------- wal ------------------- */
>  	/** A setting from instance configuration - rows_per_wal */
>  	int64_t wal_max_rows;
> @@ -245,23 +251,35 @@ xlog_write_entry(struct xlog *l, struct journal_entry *entry)
>  	return xlog_tx_commit(l);
>  }
>  
> +/*
> + * Tx schedule fiber function.
> + */
> +static int
> +tx_schedule_f(va_list ap)
> +{
> +	struct wal_writer *writer = va_arg(ap, struct wal_writer *);
> +	while (!fiber_is_cancelled()) {
> +		while (!stailq_empty(&writer->schedule_queue)) {
> +			struct journal_entry *req =
> +				stailq_shift_entry(&writer->schedule_queue,
> +						   struct journal_entry, fifo);
> +			fiber_wakeup(req->fiber);
> +		}
> +		writer->is_in_rollback = false;
> +		fiber_cond_wait(&writer->schedule_cond);
> +	}
> +	return 0;
> +}
> +
>  /**
> - * Invoke fibers waiting for their journal_entry's to be
> - * completed. The fibers are invoked in strict fifo order:
> - * this ensures that, in case of rollback, requests are
> - * rolled back in strict reverse order, producing
> - * a consistent database state.
> + * Attach requests to a scheduling queue.
>   */
>  static void
>  tx_schedule_queue(struct stailq *queue)
>  {
> -	/*
> -	 * fiber_wakeup() is faster than fiber_call() when there
> -	 * are many ready fibers.
> -	 */
> -	struct journal_entry *req;
> -	stailq_foreach_entry(req, queue, fifo)
> -		fiber_wakeup(req->fiber);
> +	struct wal_writer *writer = &wal_writer_singleton;
> +	stailq_concat(&writer->schedule_queue, queue);
> +	fiber_cond_signal(&writer->schedule_cond);
>  }

Sorry for a nit, but you use fiber_wakeup() for tx fibers but
cond_signal, which incurs an extra stailq manipulation, with this
fiber.

Why do you need another queue? Shouldn't you replace existing
queues with a single one instead? 
Having commit, rollback and schedule queues is confusing.

I like it that you effectively finalize all tx in one roll, and
then yield to the user fibers. Effectively this is a double sweep
over the list of tx, but it is less context switches overall,
since we don't switch in and out of the scheduler fiber in this
case, but switch from one ready fiber to another. Could you please
confirm? 

-- 
Konstantin Osipov, Moscow, Russia