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

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

On Sun, Sep 26, 2021 at 04:29:13PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!

Hi! Thanks for review!

> 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.

It can't be completely separate and self consisting because of the
locks we introduce to order operations. In split brain series we
agreed that locking should be hidden inside txn_process_begin()
which can fail. In result plain txn_process() call become a plain
wrapper over begin/commit|rollback and since we're changing architecture
i think altering semantics immediately will make next patches less
intrusive.

If you prefer this way I can make it so but I think this is not
that good idea, though I don't have a strong preference here.

> > 
> > 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.

It is not just order fixing but scaffolds for second series of patches on top.
Anyway, will do the way you're asking since you prefer.

> 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.

I changed semantics early on a purpose, will rework to fit your request.

> >  			} 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.

OK

> 
> > 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.

To eliminate code duplication. These two functions are _completely_
identical in terms of operations: they write promote/demote packet,
which in turn are the same except packet type. So I don't follow how
this is easier to understand and maintain: if functions are the same
better have one instance, no?

But since you're asking I'll move these two functions back, no problem.

> > 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.

OK. Thanks for comments!

	Cyrill