From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 310B86EC55; Thu, 15 Jul 2021 14:48:51 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 310B86EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1626349731; bh=Yq+cVeQ9JBY9SWo5powRa2UQk5o3ItZLoZFRUrXTKH8=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=YTFfBOLHWP/NAqVZNu4lNKkeOfbquxLLInjHwNoFCahbJOZ3Eo8YNu0AMkwKHq4r1 ChhWGBJyOG1Mn+UiPAmQN2iziO7KuYfifncXtFJ09VZx6edIJRNSo9EAQLvDu7mbb/ x1S0fWH6IC/zzeAoQ4xyEh7svbGalxw6KRfABsTs= Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 2E1C86EC55 for ; Thu, 15 Jul 2021 14:48:50 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2E1C86EC55 Received: by smtp16.mail.ru with esmtpa (envelope-from ) id 1m3zrF-00035j-E2; Thu, 15 Jul 2021 14:48:49 +0300 To: Cyrill Gorcunov , tml Cc: Vladislav Shpilevoy References: <20210714212328.701280-1-gorcunov@gmail.com> <20210714212328.701280-5-gorcunov@gmail.com> Message-ID: <5e43c72a-b837-2437-4e8c-e7ef739f0476@tarantool.org> Date: Thu, 15 Jul 2021 14:48:48 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210714212328.701280-5-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD941C43E597735A9C351B198F4576AC7B255962EB053FB468C182A05F5380850401D079F4FD37BD45F2DCB820CE5619D4F517EA09000C8860A5B0FD41195330653 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7F28A07ACE42D84A9EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006374C960BD2D03BF0BDEA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BBCA57AF85F7723F23ACCC119708C20A0E1AAEF526AB2E700CC7F00164DA146DAFE8445B8C89999728AA50765F7900637F6B57BC7E64490618DEB871D839B7333395957E7521B51C2DFABB839C843B9C08941B15DA834481F8AA50765F790063783E00425F71A4181389733CBF5DBD5E9B5C8C57E37DE458B9E9CE733340B9D5F3BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E7350555CCFDA08FA3FAC4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A5A36A8341BF3C0503F2E64D04CA90AA4FF57A286DBBAAFC26D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7501A9DF589746230F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34D16EA493CC1FD9F89062DE83E03F06B18284EA5D6222D6C89A9659B6B214D54162A3C37F7EC751DF1D7E09C32AA3244CAE58E8138D974829044CF33ABE43BFB397FE24653F78E668FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojSyb42jm8PHI6s1qodkLW5g== X-Mailru-Sender: 3B9A0136629DC9125D61937A2360A4462E41D233DA486C259856B2301EF5372F932997F95CA482A3424AE0EB1F3D1D21E2978F233C3FAE6EE63DB1732555E4A8EE80603BA4A5B0BC112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [RFC v5 4/5] limbo: order access to the promote terms X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Serge Petrenko via Tarantool-patches Reply-To: Serge Petrenko Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 15.07.2021 00:23, Cyrill Gorcunov пишет: > Promote terms tracking is shared between appliers and when > one of appliers is waiting for write to complete inside > journal_write() routine, an other may need to access read > term value to figure out if promote request is valid to > apply. Due to cooperative multitasking access to the terms > is not consistent so we need to be sure that other fibers > either read up to date terms (ie written to the WAL). > > For this sake we use latching mechanism, when one fiber > took promote-lock for terms updating other readers are > waiting until update is complete. > > For example here is a call graph of two appliers > > applier 1 > --------- > applier_apply_tx > (promote term = 3 > current max term = 2) > applier_synchro_filter_tx > apply_synchro_row > journal_write > (sleeping) > > at this moment another applier comes in with obsolete > data and term 2 > > applier 2 > --------- > applier_apply_tx > (term 2) > applier_synchro_filter_tx > txn_limbo_is_replica_outdated -> false > journal_write (sleep) > > applier 1 > --------- > journal wakes up > apply_synchro_row_cb > set max term to 3 > > So the applier 2 didn't notice that term 3 is already seen > and wrote obsolete data. With locking the applier 2 will > wait until applier 1 has finished its write. > > Part-of #6036 > > Signed-off-by: Cyrill Gorcunov > --- > src/box/applier.cc | 10 +++++--- > src/box/box.cc | 3 +-- > src/box/txn_limbo.c | 18 ++++++++++++-- > src/box/txn_limbo.h | 59 +++++++++++++++++++++++++++++++++++++++++---- > 4 files changed, 78 insertions(+), 12 deletions(-) Thanks for the patch! I think all panic_on(!cond) invocations may be replaced with assert(cond) Even if we use some functions incorrectly, this may be catched by an assertion. Other than that looks good so far. > diff --git a/src/box/applier.cc b/src/box/applier.cc > index 978383e64..838aa372d 100644 > --- a/src/box/applier.cc > +++ b/src/box/applier.cc > @@ -854,7 +854,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_locked(&txn_limbo, synchro_entry->req); > trigger_run(&replicaset.applier.on_wal_write, NULL); > } > fiber_wakeup(synchro_entry->owner); > @@ -870,6 +870,7 @@ apply_synchro_row(uint32_t replica_id, struct xrow_header *row) > if (xrow_decode_synchro(row, &req) != 0) > goto err; > > + txn_limbo_promote_lock(&txn_limbo); > struct replica_cb_data rcb_data; > struct synchro_entry entry; > /* > @@ -907,12 +908,15 @@ apply_synchro_row(uint32_t replica_id, struct xrow_header *row) > * transactions side, including the async ones. > */ > if (journal_write(&entry.base) != 0) > - goto err; > + goto err_unlock; > if (entry.base.res < 0) { > diag_set_journal_res(entry.base.res); > - goto err; > + goto err_unlock; > } > + txn_limbo_promote_unlock(&txn_limbo); > return 0; > +err_unlock: > + txn_limbo_promote_unlock(&txn_limbo); > err: > diag_log(); > return -1; > diff --git a/src/box/box.cc b/src/box/box.cc > index d211589b5..8b0f9859e 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -1566,10 +1566,9 @@ box_clear_synchro_queue(bool demote) > * (synchronous replication and leader election are in sync, and > * both chose this node as a leader). > */ > - if (!demote && txn_limbo_replica_term(&txn_limbo, instance_id) == > + if (!demote && txn_limbo_term(&txn_limbo, instance_id) == > box_raft()->term) > return 0; > - > break; > default: > unreachable(); > diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c > index 957fe0d1e..d24df3606 100644 > --- a/src/box/txn_limbo.c > +++ b/src/box/txn_limbo.c > @@ -40,6 +40,7 @@ struct txn_limbo txn_limbo; > static void > txn_limbo_promote_create(struct txn_limbo_promote *pmt) > { > + latch_create(&pmt->latch); > vclock_create(&pmt->terms_map); > pmt->terms_max = 0; > } > @@ -731,12 +732,17 @@ 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_process_locked(struct txn_limbo *limbo, > + const struct synchro_request *req) > { > struct txn_limbo_promote *pmt = &limbo->promote; > uint64_t term = req->term; > uint32_t origin = req->origin_id; > - if (txn_limbo_replica_term(limbo, origin) < term) { > + > + panic_on(!txn_limbo_promote_is_locked(limbo), > + "limbo: unlocked processing of a request"); > + > + if (txn_limbo_term_locked(limbo, origin) < term) { > vclock_follow(&pmt->terms_map, origin, term); > if (term > pmt->terms_max) > pmt->terms_max = term; > @@ -794,6 +800,14 @@ 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_promote_lock(limbo); > + txn_limbo_process_locked(limbo, req); > + txn_limbo_promote_unlock(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 70a5fbfd5..a2595bcff 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 > > @@ -80,6 +81,10 @@ txn_limbo_entry_is_complete(const struct txn_limbo_entry *e) > * situation and other errors. > */ > struct txn_limbo_promote { > + /** > + * To order access to the promote data. > + */ > + struct latch latch; > /** > * Latest terms received with PROMOTE entries from remote instances. > * Limbo uses them to filter out the transactions coming not from the > @@ -222,15 +227,52 @@ txn_limbo_last_entry(struct txn_limbo *limbo) > in_queue); > } > > +/** Lock promote data. */ > +static inline void > +txn_limbo_promote_lock(struct txn_limbo *limbo) > +{ > + struct txn_limbo_promote *pmt = &limbo->promote; > + latch_lock(&pmt->latch); > +} > + > +/** Unlock promote data. */ > +static void > +txn_limbo_promote_unlock(struct txn_limbo *limbo) > +{ > + struct txn_limbo_promote *pmt = &limbo->promote; > + latch_unlock(&pmt->latch); > +} > + > +/** Test if promote data is locked. */ > +static inline bool > +txn_limbo_promote_is_locked(struct txn_limbo *limbo) > +{ > + const struct txn_limbo_promote *pmt = &limbo->promote; > + return latch_is_locked(&pmt->latch); > +} > + > +/** Fetch replica's term with lock taken. */ > +static inline uint64_t > +txn_limbo_term_locked(struct txn_limbo *limbo, uint32_t replica_id) > +{ > + struct txn_limbo_promote *pmt = &limbo->promote; > + panic_on(!txn_limbo_promote_is_locked(limbo), > + "limbo: unlocked term read for replica %u", > + replica_id); > + return vclock_get(&pmt->terms_map, replica_id); > +} > + > /** > * Return the latest term as seen in PROMOTE requests from instance with id > * @a replica_id. > */ > static inline uint64_t > -txn_limbo_replica_term(const struct txn_limbo *limbo, uint32_t replica_id) > +txn_limbo_term(struct txn_limbo *limbo, uint32_t replica_id) > { > - const struct txn_limbo_promote *pmt = &limbo->promote; > - return vclock_get(&pmt->terms_map, replica_id); > + txn_limbo_promote_lock(limbo); > + uint64_t v = txn_limbo_term_locked(limbo, replica_id); > + txn_limbo_promote_unlock(limbo); > + return v; > } > > /** > @@ -238,12 +280,15 @@ 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) > { > const struct txn_limbo_promote *pmt = &limbo->promote; > - return txn_limbo_replica_term(limbo, replica_id) < > + txn_limbo_promote_lock(limbo); > + bool res = txn_limbo_term_locked(limbo, replica_id) < > pmt->terms_max; > + txn_limbo_promote_unlock(limbo); > + return res; > } > > /** > @@ -317,6 +362,10 @@ txn_limbo_wait_complete(struct txn_limbo *limbo, struct txn_limbo_entry *entry); > void > txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req); > > +void > +txn_limbo_process_locked(struct txn_limbo *limbo, > + const struct synchro_request *req); > + > /** > * Waiting for confirmation of all "sync" transactions > * during confirm timeout or fail. -- Serge Petrenko