[Tarantool-patches] [RFC v29 2/3] qsync: order access to the limbo terms
Serge Petrenko
sergepetrenko at tarantool.org
Wed Feb 9 12:10:03 MSK 2022
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.
More information about the Tarantool-patches
mailing list