Re: [Tarantool-patches] [PATCH v17 2/5] qsync: order access to the limbo terms

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thanks for the patch!

I consider this series as an independent patchset which fixes
the ordering, not split-brain. Like you said.

But now it is not independent. The main problem is that you
just blended in a few changes from the split-brain patches. I
point them out in my comments.

See 4 comments below.

>  src/box/applier.cc      | 16 +++++++---
>  src/box/box.cc          | 71 +++++++++++++++++++----------------------
>  src/box/memtx_engine.cc |  3 +-
>  src/box/txn_limbo.c     | 34 ++++++++++++++++++--
>  src/box/txn_limbo.h     | 49 +++++++++++++++++++++++++---
>  5 files changed, 121 insertions(+), 52 deletions(-)
> 
> diff --git a/src/box/applier.cc b/src/box/applier.cc
> index b981bd436..f0751b68a 100644
> --- a/src/box/applier.cc
> +++ b/src/box/applier.cc
> @@ -458,7 +458,8 @@ applier_wait_snapshot(struct applier *applier)
>  				struct synchro_request req;
>  				if (xrow_decode_synchro(&row, &req) != 0)
>  					diag_raise();
> -				txn_limbo_process(&txn_limbo, &req);
> +				if (txn_limbo_process(&txn_limbo, &req) != 0)
> +					diag_raise();

1. How txn_limbo_process() can fail? You just fixed ordering. Essentially, added
a few yields in some places. You didn't add any validation, any new errors in
this patchset. Please, drop the empty 'return 0's from this patchset. They
can't be and are not tested here anyway.

Addition of a lock-unlock pair to txn_limbo_process didn't affect whether it
can fail. It just blocks the execution sometimes for a while.

>  			} else if (iproto_type_is_raft_request(row.type)) {
>  				struct raft_request req;
>  				if (xrow_decode_raft(&row, &req, NULL) != 0)
> @@ -857,7 +858,7 @@ apply_synchro_row_cb(struct journal_entry *entry)
>  		applier_rollback_by_wal_io(entry->res);
>  	} else {
>  		replica_txn_wal_write_cb(synchro_entry->rcb);
> -		txn_limbo_process(&txn_limbo, synchro_entry->req);
> +		txn_limbo_process_run(&txn_limbo, synchro_entry->req);

2. _run is usually used for infinite loops in fibers, or for handling a
sequence of something. Like trigger_run(). Here you handle a single
request. The only difference from process() is that the lock is taken.
I propose to rename it to _do or _ex or _in_tx or something.

> diff --git a/src/box/box.cc b/src/box/box.cc
> index 7b11d56d6..19e67b186 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1670,48 +1669,43 @@ box_wait_limbo_acked(double timeout)
>  	return wait_lsn;
>  }
>  
> -/** Write and process a PROMOTE request. */
> -static void
> -box_issue_promote(uint32_t prev_leader_id, int64_t promote_lsn)
> +/** Write and process PROMOTE or DEMOTE request. */
> +static int
> +box_issue_synchro(uint16_t type, uint32_t prev_leader_id, int64_t promote_lsn)
>  {
>  	struct raft *raft = box_raft();
> +
> +	assert(type == IPROTO_RAFT_PROMOTE ||
> +	       type == IPROTO_RAFT_DEMOTE);
>  	assert(raft->volatile_term == raft->term);
>  	assert(promote_lsn >= 0);
> -	txn_limbo_write_promote(&txn_limbo, promote_lsn,
> -				raft->term);
> +
>  	struct synchro_request req = {
> -		.type = IPROTO_RAFT_PROMOTE,
> -		.replica_id = prev_leader_id,
> -		.origin_id = instance_id,
> -		.lsn = promote_lsn,
> -		.term = raft->term,
> +		.type		= type,
> +		.replica_id	= prev_leader_id,
> +		.origin_id	= instance_id,
> +		.lsn		= promote_lsn,
> +		.term		= raft->term,
>  	};
> -	txn_limbo_process(&txn_limbo, &req);
> +
> +	if (txn_limbo_process_begin(&txn_limbo, &req) != 0)
> +		return -1;
> +
> +	if (type == IPROTO_RAFT_PROMOTE)
> +		txn_limbo_write_promote(&txn_limbo, req.lsn, req.term);
> +	else
> +		txn_limbo_write_demote(&txn_limbo, req.lsn, req.term);
> +
> +	txn_limbo_process_run(&txn_limbo, &req);
>  	assert(txn_limbo_is_empty(&txn_limbo));
> +
> +	txn_limbo_process_commit(&txn_limbo);
> +	return 0;
>  }
>  
>  /** A guard to block multiple simultaneous promote()/demote() invocations. */
>  static bool is_in_box_promote = false;
>  
> -/** Write and process a DEMOTE request. */
> -static void
> -box_issue_demote(uint32_t prev_leader_id, int64_t promote_lsn)
> -{
> -	assert(box_raft()->volatile_term == box_raft()->term);
> -	assert(promote_lsn >= 0);
> -	txn_limbo_write_demote(&txn_limbo, promote_lsn,
> -				box_raft()->term);
> -	struct synchro_request req = {
> -		.type = IPROTO_RAFT_DEMOTE,
> -		.replica_id = prev_leader_id,
> -		.origin_id = instance_id,
> -		.lsn = promote_lsn,
> -		.term = box_raft()->term,
> -	};
> -	txn_limbo_process(&txn_limbo, &req);
> -	assert(txn_limbo_is_empty(&txn_limbo));

3. Why did you merge these 2 functions? AFAIR, their split was
deliberate. To make each of them simpler to understand and maintain.

> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index 70447caaf..eb9aa7780 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -786,6 +790,32 @@ txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req)
>  	return;
>  }
>  
> +int
> +txn_limbo_process_begin(struct txn_limbo *limbo,
> +			const struct synchro_request *req)
> +{
> +	latch_lock(&limbo->promote_latch);
> +	/*
> +	 * FIXME: For now we take a lock only but idea
> +	 * is to verify incoming requests to detect split
> +	 * brain situation. Thus we need to change the code
> +	 * semantics in advance.
> +	 */
> +	(void)req;
> +	return 0;

4. Return value is a part of the split-brain patch, not of the
ordering patch. It is clearly seen from this patchset, because
this series never changes `return 0` to anything else.

I get that you want to merge something. Hence we are working on this
independent issue of reodering. But then lets make it truly
independent.