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

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

24.02.2022 23:18, Cyrill Gorcunov пишет:
> ---

Thanks for the fixes!
Only two minor comments left. Please find them below.

Also, please, consider this diff fixing code style:

==============================

diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index 7607e084b..8634b63b4 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -726,8 +726,7 @@ txn_limbo_wait_empty(struct txn_limbo *limbo, double 
timeout)
  }

  void
-txn_limbo_apply(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));

@@ -792,8 +791,7 @@ txn_limbo_apply(struct txn_limbo *limbo,
  }

  void
-txn_limbo_process(struct txn_limbo *limbo,
-                 const struct synchro_request *req)
+txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request 
*req)
  {
         txn_limbo_begin(limbo);
         txn_limbo_apply(limbo, req);
diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
index 1f5da7abb..8c9671ca5 100644
--- a/src/box/txn_limbo.h
+++ b/src/box/txn_limbo.h
@@ -235,8 +235,7 @@ txn_limbo_replica_term(const struct txn_limbo 
*limbo, uint32_t replica_id)
   * data from it. The check is only valid when elections are enabled.
   */
  static inline bool
-txn_limbo_is_replica_outdated(struct txn_limbo *limbo,
-                             uint32_t replica_id)
+txn_limbo_is_replica_outdated(struct txn_limbo *limbo, uint32_t replica_id)
  {
         latch_lock(&limbo->promote_latch);
         uint64_t v = vclock_get(&limbo->promote_term_map, replica_id);

==============================


...

> diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h
> index 53e52f676..1f5da7abb 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;

1. As I can see, you only use `promote_is_latched` to
     push box.info.synchro.queue.latched.

    I suggest you simply use latch_is_locked instead.

2. Also, let's rename `box.info.synchro.queue.latched`
    to something like `box.info.synchro.queue.busy`.
    Feel free to suggest other variants.

    "Latched" just sounds too technical IMO.

>   	/**
>   	 * Maximal LSN gathered quorum and either already confirmed in WAL, or
>   	 * whose confirmation is in progress right now. Any attempt to confirm
> @@ -226,11 +235,14 @@ txn_limbo_replica_term(const struct txn_limbo *limbo, uint32_t replica_id)
>    * data from it. The check is only valid when elections are enabled.
>    */
>   static inline bool
> -txn_limbo_is_replica_outdated(const struct txn_limbo *limbo,
> +txn_limbo_is_replica_outdated(struct txn_limbo *limbo,
>   			      uint32_t replica_id)
>   {
> -	return txn_limbo_replica_term(limbo, replica_id) <
> -	       limbo->promote_greatest_term;
> +	latch_lock(&limbo->promote_latch);
> +	uint64_t v = vclock_get(&limbo->promote_term_map, replica_id);
> +	bool res = v < limbo->promote_greatest_term;
> +	latch_unlock(&limbo->promote_latch);
> +	return res;
>   }
>   
>
...

-- 
Serge Petrenko