Re: [Tarantool-patches] [RFC v29 2/3] qsync: order access to the limbo terms

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

01.02.2022 00:55, Cyrill Gorcunov пишет:

Hi! Thanks for working on this!
Please find a couple of comments below.

There are only a few minor places to fix. Otherwise I think
the patch is almost ready for merging (once the test for promote/demote
is introduced).

Don't bother too much with my comments here, let's first
implement the test for promote/demote locking.

Overall the patch is in a very good condition.
Check out my suggestion for the promote() test in the next letter.


> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index 70447caaf..3363e2f9a 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -47,6 +47,8 @@ txn_limbo_create(struct txn_limbo *limbo)
>   	vclock_create(&limbo->vclock);
>   	vclock_create(&limbo->promote_term_map);
>   	limbo->promote_greatest_term = 0;
> +	latch_create(&limbo->promote_latch);
> +	limbo->promote_is_latched = false;
>   	limbo->confirmed_lsn = 0;
>   	limbo->rollback_count = 0;
>   	limbo->is_in_rollback = false;
> @@ -724,11 +726,14 @@ txn_limbo_wait_empty(struct txn_limbo *limbo, double timeout)
>   }
>   
>   void
> -txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req)
> +txn_limbo_apply(struct txn_limbo *limbo,
> +		const struct synchro_request *req)
>   {
> +	assert(latch_is_locked(&limbo->promote_latch));
> +
>   	uint64_t term = req->term;
>   	uint32_t origin = req->origin_id;
> -	if (txn_limbo_replica_term(limbo, origin) < term) {
> +	if (vclock_get(&limbo->promote_term_map, origin) < (int64_t)term) {


Extraneous change: you don't lock txn_limbo_replica_term() anyways.


>   		vclock_follow(&limbo->promote_term_map, origin, term);
>   		if (term > limbo->promote_greatest_term)
>   			limbo->promote_greatest_term = term;
> @@ -786,6 +791,15 @@ txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req)
>   	return;
>   }
>   
> +void
> +txn_limbo_process(struct txn_limbo *limbo,
> +		  const struct synchro_request *req)
> +{
> +	txn_limbo_begin(limbo);
> +	txn_limbo_apply(limbo, req);
> +	txn_limbo_commit(limbo);
> +}
> +
>   void
>   txn_limbo_on_parameters_change(struct txn_limbo *limbo)
>   {
> diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
> index 53e52f676..b9dddda77 100644
> --- a/src/box/txn_limbo.h
> +++ b/src/box/txn_limbo.h
> @@ -31,6 +31,7 @@
>    */
>   #include "small/rlist.h"
>   #include "vclock/vclock.h"
> +#include "latch.h"
>   
>   #include <stdint.h>
>   
> @@ -147,6 +148,14 @@ struct txn_limbo {
>   	 * limbo and raft are in sync and the terms are the same.
>   	 */
>   	uint64_t promote_greatest_term;
> +	/**
> +	 * To order access to the promote data.
> +	 */
> +	struct latch promote_latch;
> +	/**
> +	 * A flag to inform if limbo is locked (for tests mostly).
> +	 */
> +	bool promote_is_latched;


TBH, I liked 'waiter_count' more. First of all,

`promote_is_latched` duplicates `latch_is_locked(&promote_latch)`,

secondly, `waiter_count` gives more useful info.

When `waiter_count > 0`, promote is latched, but additionally you

know the count of blocked fibers.


I wasn't against `waiter_count`.

My only suggestion was to count `waiter_count` like this:

txn_limbo_begin(...) {

     waiter_count++;

     latch_lock();

     waiter_count--;

}

>   	/**
>   	 * Maximal LSN gathered quorum and either already confirmed in WAL, or
>   	 * whose confirmation is in progress right now. Any attempt to confirm
> @@ -216,7 +225,7 @@ txn_limbo_last_entry(struct txn_limbo *limbo)
>    * @a replica_id.
>    */
>   static inline uint64_t
> -txn_limbo_replica_term(const struct txn_limbo *limbo, uint32_t replica_id)
> +txn_limbo_replica_term(struct txn_limbo *limbo, uint32_t replica_id)


Extraneous change.