<HTML><BODY><div class="js-helper js-readmsg-msg"><div id="style_16418143601194002098"><div id="style_16418143601194002098_BODY"><div class="cl_843701"><div>Hi! Thanks for the patch!</div><div> </div><div>box_issue_promote() and box_issue_demote() need fine-grained locking anyway.</div><div>Otherwise it’s possible that promote() is already issued, but not yet written to WAL, and some</div><div>outdated request is applied by applier at that exact moment.</div><div> </div><div>You should take the lock before the WAL write, and release it only after txn_limbo_apply.</div><div> </div><div>No need to guard every limbo function there is, but we have to guard everything that</div><div>writes PROMOTE/DEMOTE.</div><div> </div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;">Четверг, 30 декабря 2021, 23:24 +03:00 от Cyrill Gorcunov <<a href="/compose?To=gorcunov@gmail.com">gorcunov@gmail.com</a>>:<br> <div id=""><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div id="style_16408958551347820643_BODY_mr_css_attr">Limbo terms tracking is shared between appliers and when<br>one of appliers is waiting for write to complete inside<br>journal_write() routine, an other may need to access read<br>term value to figure out if promote request is valid to<br>apply. Due to cooperative multitasking access to the terms<br>is not consistent so we need to be sure that other fibers<br>read up to date terms (ie written to the WAL).<br><br>For this sake we use a latching mechanism, when one fiber<br>takes a lock for updating other readers are waiting until<br>the operation is complete.<br><br>For example here is a call graph of two appliers<br><br>applier 1<br>---------<br>applier_apply_tx<br> (promote term = 3<br> current max term = 2)<br> applier_synchro_filter_tx<br> apply_synchro_row<br> journal_write<br> (sleeping)<br><br>at this moment another applier comes in with obsolete<br>data and term 2<br><br> applier 2<br> ---------<br> applier_apply_tx<br> (term 2)<br> applier_synchro_filter_tx<br> txn_limbo_is_replica_outdated -> false<br> journal_write (sleep)<br><br>applier 1<br>---------<br>journal wakes up<br> apply_synchro_row_cb<br> set max term to 3<br><br>So the applier 2 didn't notice that term 3 is already seen<br>and wrote obsolete data. With locking the applier 2 will<br>wait until applier 1 has finished its write.<br><br>We introduce the following helpers:<br><br>1) txn_limbo_begin: which takes a lock<br>2) txn_limbo_commit and txn_limbo_rollback which simply release<br> the lock but have different names for better semantics<br>3) txn_limbo_process is a general function which uses x_begin<br> and x_commit helper internally<br>4) txn_limbo_apply to do a real job over processing the<br> request, it implies that txn_limbo_begin been called<br><br>Testing such in-flight condition won't be easy so we introduce<br>"box.info.synchro.queue.waiters" field which represent current<br>number of fibers waiting for limbo to finish request processing.<br><br>@TarantoolBot document<br>Title: synchronous replication changes<br><br>`box.info.synchro.queue` gets a new field: `waiters`. It represents<br>current number of fibers waiting the synchronous transaction processing<br>to complete.<br><br>Part-of #6036<br><br>Signed-off-by: Cyrill Gorcunov <<a>gorcunov@gmail.com</a>><br>---<br> src/box/applier.cc | 12 ++++++++---<br> src/box/lua/info.c | 4 +++-<br> src/box/txn_limbo.c | 18 ++++++++++++++--<br> src/box/txn_limbo.h | 52 ++++++++++++++++++++++++++++++++++++++++-----<br> 4 files changed, 75 insertions(+), 11 deletions(-)<br> </div></div></div></div></blockquote><div> </div><div>…</div><div> </div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div>diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h<br>index 53e52f676..42d572595 100644<br>--- a/src/box/txn_limbo.h<br>+++ b/src/box/txn_limbo.h</div></div></div></div></blockquote><div> </div><div>…</div><div> </div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div>@@ -216,7 +225,7 @@ txn_limbo_last_entry(struct txn_limbo *limbo)<br> * @a replica_id.<br> */<br> static inline uint64_t<br>-txn_limbo_replica_term(const struct txn_limbo *limbo, uint32_t replica_id)<br>+txn_limbo_replica_term(struct txn_limbo *limbo, uint32_t replica_id)<br> {</div></div></div></div></blockquote><div> </div><div>You’ve forgot to lock the latch here, I guess.</div><div> </div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div> return vclock_get(&limbo->promote_term_map, replica_id);<br> }<br>@@ -226,11 +235,14 @@ txn_limbo_replica_term(const struct txn_limbo *limbo, uint32_t replica_id)<br> * data from it. The check is only valid when elections are enabled.<br> */<br> static inline bool<br>-txn_limbo_is_replica_outdated(const struct txn_limbo *limbo,<br>+txn_limbo_is_replica_outdated(struct txn_limbo *limbo,<br> uint32_t replica_id)<br> {<br>- return txn_limbo_replica_term(limbo, replica_id) <<br>- limbo->promote_greatest_term;<br>+ latch_lock(&limbo->promote_latch);<br>+ uint64_t v = vclock_get(&limbo->promote_term_map, replica_id);<br>+ bool res = v < limbo->promote_greatest_term;<br>+ latch_unlock(&limbo->promote_latch);<br>+ return res;<br> }<br> <br> /**<br>@@ -300,7 +312,37 @@ txn_limbo_ack(struct txn_limbo *limbo, uint32_t replica_id, int64_t lsn);<br> int<br> txn_limbo_wait_complete(struct txn_limbo *limbo, struct txn_limbo_entry *entry);<br> <br>-/** Execute a synchronous replication request. */<br>+/**<br>+ * Initiate execution of a synchronous replication request.<br>+ */<br>+static inline void<br>+txn_limbo_begin(struct txn_limbo *limbo)<br>+{<br>+ limbo->promote_latch_cnt++;<br>+ latch_lock(&limbo->promote_latch);</div></div></div></div></blockquote><div> </div><div>I suppose you should decrease the latch_cnt right after acquiring the lock.</div><div> </div><div>Otherwise you count the sole «limbo user» together with «limbo waiters».</div><div> </div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div>+}<br>+<br>+/** Commit a synchronous replication request. */<br>+static inline void<br>+txn_limbo_commit(struct txn_limbo *limbo)<br>+{<br>+ latch_unlock(&limbo->promote_latch);<br>+ limbo->promote_latch_cnt--;<br>+}<br>+<br>+/** Rollback a synchronous replication request. */<br>+static inline void<br>+txn_limbo_rollback(struct txn_limbo *limbo)<br>+{<br>+ latch_unlock(&limbo->promote_latch);</div></div></div></div></blockquote><div> </div><div>If you don’t want to decrease the counter right after latch_lock(), you should decrease it</div><div>here, as well as in txn_limbo_commit().</div><div> </div><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div><div class="js-helper_mr_css_attr js-readmsg-msg_mr_css_attr"><div><div>+}<br>+<br>+/** Apply a synchronous replication request after processing stage. */<br>+void<br>+txn_limbo_apply(struct txn_limbo *limbo,<br>+ const struct synchro_request *req);<br>+<br>+/** Process a synchronous replication request. */<br> void<br> txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req);<br> <br>--<br>2.31.1</div></div></div></div></blockquote><div> </div></div></div></div></div></BODY></HTML>