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 BBF176EC58; Tue, 3 Aug 2021 02:48:22 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BBF176EC58 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627948102; bh=rX6PZiENPyMTkQh7fyQ1PdkwY/bS+nMZl0RCnuQSlPI=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=ngWHjnt+H5UlRSwgKM/Z0pPty9lNZotb8At55S/+/ON3zK9T+jbi4HDOsRpaNrFnp d5z8zThsLnHgQUXDOvC+ngERxOQpVi69LUgMVEaaQdrI0RoM1Z2wRJ52SbOAmbboX6 5r1Gj6UeiwREVGwwPxP/jSSAFZp1upDd+LPLDB7M= Received: from smtpng3.i.mail.ru (smtpng3.i.mail.ru [94.100.177.149]) (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 998CC6EC58 for ; Tue, 3 Aug 2021 02:48:20 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 998CC6EC58 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1mAhfP-0007vk-J0; Tue, 03 Aug 2021 02:48:20 +0300 To: Cyrill Gorcunov , tml References: <20210730113539.563318-1-gorcunov@gmail.com> <20210730113539.563318-4-gorcunov@gmail.com> Message-ID: <4afbed2a-36cd-9156-ccb6-d218a0db395a@tarantool.org> Date: Tue, 3 Aug 2021 01:48:18 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: <20210730113539.563318-4-gorcunov@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD941C43E597735A9C351B198F4576AC7B20CA14D9DFB46B94A182A05F538085040EBDB47A1AEDC31DC67F08DA04BE9856093F14DA92E29FD235D262F8842E82E10 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7880943D09B4F39CCC2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE7E99F7FC786761FD88F08D7030A58E5AD1A62830130A00468AEEEE3FBA3A834EE7353EFBB5533756685811CA530B5049186918F0CF0A788D0C4A647A1B3002D0EA471835C12D1D9774AD6D5ED66289B5278DA827A17800CE767883B903EA3BAEA9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3A7AC412AE061D850117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637BBEA499411984DA1EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B505B82F503E3171A40E21B04F5F6C3B546C X-C1DE0DAB: 0D63561A33F958A5D45C5D29750AAA2A53371A819381647FD7FF34C11F6B7AC3D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA751B940EDA0DFB0535410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34D2AC226BFD455724436A9CA9E319B58C0862D42B81E50F5A6F3C6B2A4971C4294819A252E5ECDA0B1D7E09C32AA3244CE8833A3E428442B04ACD88E3F5284BE3B038C9161EF167A1FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj9N286KAyvN4E+rGCmX35hA== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D9EB648B27B5280295F27574C8F35D0973841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v9 3/5] limbo: order access to the limbo 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: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Thanks for the patch! See 6 comments below. > diff --git a/src/box/applier.cc b/src/box/applier.cc > index f621fa657..a7f472714 100644 > --- a/src/box/applier.cc > +++ b/src/box/applier.cc > @@ -909,12 +910,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_term_unlock(&txn_limbo); > return 0; > +err_unlock: > + txn_limbo_term_unlock(&txn_limbo); 1. Could be done simpler: ==================== @@ -908,7 +909,9 @@ apply_synchro_row(uint32_t replica_id, struct xrow_header *row) * before trying to commit. But that requires extra steps from the * transactions side, including the async ones. */ - if (journal_write(&entry.base) != 0) + int rc = journal_write(&entry.base); + txn_limbo_term_unlock(&txn_limbo); + if (rc != 0) goto err; if (entry.base.res < 0) { diag_set_journal_res(entry.base.res); ==================== > diff --git a/src/box/box.cc b/src/box/box.cc > index 535f30292..5ca617e32 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -1573,7 +1573,7 @@ box_run_elections(void) > static int > box_check_promote_term_intact(uint64_t promote_term) > { > - if (txn_limbo.promote_greatest_term != promote_term) { > + if (txn_limbo_term_max_raw(&txn_limbo) != promote_term) { 2. In raft terminology we call such data 'volatile' instead of 'raw'. > diag_set(ClientError, ER_INTERFERING_PROMOTE, > txn_limbo.owner_id); > return -1; > @@ -1728,7 +1728,7 @@ box_promote(void) > * Currently active leader (the instance that is seen as leader by both > * raft and txn_limbo) can't issue another PROMOTE. > */ > - bool is_leader = txn_limbo_replica_term(&txn_limbo, instance_id) == > + bool is_leader = txn_limbo_term(&txn_limbo, instance_id) == 3. Why did you change the name? The old one was closer to reality. When you say 'limbo term', it is not clear whether you mean the max term or term of one of the nodes. Please, extract all such renames into a separate commit. Otherwise it is harder to find the functional changes in this one. > diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c > index 570f77c46..be5e0adf5 100644 > --- a/src/box/txn_limbo.c > +++ b/src/box/txn_limbo.c > @@ -724,22 +725,23 @@ 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) > { 4. Please, add an assertion that the latch is occupied. The same for the other _locked functions. > uint64_t term = req->term; > uint32_t origin = req->origin_id; > - if (txn_limbo_replica_term(limbo, origin) < term) { > + if (txn_limbo_term_locked(limbo, origin) < term) { > vclock_follow(&limbo->promote_term_map, origin, term); > - if (term > limbo->promote_greatest_term) > - limbo->promote_greatest_term = term; > + if (term > limbo->promote_term_max) > + limbo->promote_term_max = term; > } else if (iproto_type_is_promote_request(req->type) && > - limbo->promote_greatest_term > 1) { > + limbo->promote_term_max > 1) { > /* PROMOTE for outdated term. Ignore. */ > say_info("RAFT: ignoring %s request from instance " > "id %u for term %llu. Greatest term seen " > "before (%llu) is bigger.", > iproto_type_name(req->type), origin, (long long)term, > - (long long)limbo->promote_greatest_term); > + (long long)limbo->promote_term_max); > return; > } > diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h > index 53e52f676..25faffd2b 100644 > --- a/src/box/txn_limbo.h > +++ b/src/box/txn_limbo.h > @@ -211,14 +216,61 @@ txn_limbo_last_entry(struct txn_limbo *limbo) > in_queue); > } > > +/** Lock promote data. */ > +static inline void > +txn_limbo_term_lock(struct txn_limbo *limbo) > +{ > + latch_lock(&limbo->promote_latch); > +} > + > +/** Unlock promote data. */ > +static void 5. Why isn't it inline while the others are? > +txn_limbo_term_unlock(struct txn_limbo *limbo) > +{ > + latch_unlock(&limbo->promote_latch); > +} > + > +/** Test if promote data is locked. */ > +static inline bool > +txn_limbo_term_is_locked(const struct txn_limbo *limbo) > +{ > + return latch_is_locked(&limbo->promote_latch); > +} > + > +/** Fetch replica's term with lock taken. */ > +static inline uint64_t > +txn_limbo_term_locked(struct txn_limbo *limbo, uint32_t replica_id) > +{ > + panic_on(!txn_limbo_term_is_locked(limbo), > + "limbo: unlocked term read for replica_id %u", > + replica_id); 6. This new macro seems counter-intuitive because works vice versa compared to assert(). Can you try to rename it somehow and make it accept a condition which must be true instead of false? > + return vclock_get(&limbo->promote_term_map, replica_id); > +}