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 0B29A70349; Fri, 22 Oct 2021 01:06:09 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0B29A70349 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1634853969; bh=ugRY383rd3+bvPHCdsSSDSImqL3XHq51Dh3RqjZkUBM=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=kvrO/rvR4jTSS6p1zRtl9JK5iyw5cLio+i6a2TlOKs1HPrRUpi1CAnSZBT5WZJOc+ GbGcF9D1zOyps8C3o+YYskQdErglxZukbtBsJP/QDRu3BJEO0P0MjmwmEj0MLIeQMo XX6U4eygTIJfycd3BcnchJITEXf4+cPRwqQ8Vk5Q= Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 03BA770349 for ; Fri, 22 Oct 2021 01:06:06 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 03BA770349 Received: by smtp43.i.mail.ru with esmtpa (envelope-from ) id 1mdgCM-0007Gg-9K; Fri, 22 Oct 2021 01:06:06 +0300 Message-ID: <0c3afef7-dbc9-b429-9c7f-ae9fdcf0c634@tarantool.org> Date: Fri, 22 Oct 2021 00:06:05 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Content-Language: en-US To: Cyrill Gorcunov , tml References: <20211014215622.49732-1-gorcunov@gmail.com> <20211014215622.49732-3-gorcunov@gmail.com> In-Reply-To: <20211014215622.49732-3-gorcunov@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9C7814344C8C501C8A4649CC2A06649B6A97F49A0BDC630D4182A05F538085040B143CD9E77D9341FB56B66A98A64DBE1C73CB81B88AD3777EB41E8CB57574BC7 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE73166BA5FA54FC0D3EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063791F3A0CD9A153DC48638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D87D11356BCA5283DD0F274C65735A2CC8117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC566404C906FA8ADEA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352026055571C92BF10FBDFBBEFFF4125B51D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EED58E677C372A3F47C8623B8F170C382FD8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE3E6BDB36F057AC83C302FCEF25BFAB345C4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F79006377AA2284B41911753EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B5052EAD22D8EDE88196C449971BDD4A2DA1 X-C1DE0DAB: 0D63561A33F958A526C98BDA246C9F60F06C32C6BF7F40719F0E029CA9691757D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75FA7FF33AA1A4D21C410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3429538671E6527D32A839E8348CDDA253071C8BF094E38C9452B9D1CB22C9D5458F33EAFEB2A8D5C91D7E09C32AA3244C40389F3AEC4E658F147948ADB8CCC88D55E75C8D0ED9F6EE729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojXelGRflEPnAUAHTK+wyMYg== X-Mailru-Sender: 1F3202E75A95DDEF0AE1703729B258988462B1BDEC572CD9FA761E45E01C7C9735EFD1F3DE9B108307784C02288277CA03E0582D3806FB6A5317862B1921BA260ED6CFD6382C13A6112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v23 2/3] qsync: 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 4 comments below. > diff --git a/src/box/box.cc b/src/box/box.cc > index e082e1a3d..6a9be745a 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -1686,8 +1684,11 @@ box_issue_promote(uint32_t prev_leader_id, int64_t promote_lsn) > .lsn = promote_lsn, > .term = raft->term, > }; > - txn_limbo_process(&txn_limbo, &req); > + txn_limbo_process_begin(&txn_limbo); > + txn_limbo_write_promote(&txn_limbo, req.lsn, req.term); > + txn_limbo_process_core(&txn_limbo, &req); > assert(txn_limbo_is_empty(&txn_limbo)); > + txn_limbo_process_commit(&txn_limbo); 1. What was wrong with txn_limbo_begin/commit/rollback? I mean `txn_limbo` prefix, without `_process_` suffix. From this hunk we can see that you call `process_begin` but then you call txn_limbo_write_promote - it is not 'process'. Using 'process' because of that looks inconsistent. Also if you would drop it from begin/commit/rollback names, then you could leave txn_limbo_process as is, without this _core suffix. > @@ -1708,8 +1707,12 @@ box_issue_demote(uint32_t prev_leader_id, int64_t promote_lsn) > .lsn = promote_lsn, > .term = box_raft()->term, > }; > - txn_limbo_process(&txn_limbo, &req); > + txn_limbo_process_begin(&txn_limbo); > + txn_limbo_write_demote(&txn_limbo, promote_lsn, > + box_raft()->term); 2. This expression fits into one line (< 80 symbols) just fine. Maybe lets write it as one line? > + txn_limbo_process_core(&txn_limbo, &req); > assert(txn_limbo_is_empty(&txn_limbo)); > + txn_limbo_process_commit(&txn_limbo); > } > diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c > index 70447caaf..9b643072a 100644 > --- a/src/box/txn_limbo.c > +++ b/src/box/txn_limbo.c > @@ -542,10 +543,30 @@ txn_limbo_read_demote(struct txn_limbo *limbo, int64_t lsn) > } > > void > -txn_limbo_ack(struct txn_limbo *limbo, uint32_t replica_id, int64_t lsn) > +txn_limbo_ack(struct txn_limbo *limbo, uint32_t owner_id, > + uint32_t replica_id, int64_t lsn) > { > + /* > + * ACKs are collected only by the transactions originator > + * (which is the single master in 100% so far). Other instances > + * wait for master's CONFIRM message instead. > + * > + * Due to cooperative multitasking there might be limbo owner > + * migration in-fly (while writing data to journal), so for > + * simplicity sake the test for owner is done here instead > + * of putting this check to the callers. > + */ 3. This does not improve simplicity anyhow TBH, rather vice versa. This code looks very suspicious, counter-intuitive. But lets wait for more tests. See comments to the last commit. 4. Why doesn't txn_limbo_ack() keeps the lock for the time of txn_limbo_write_confirm() and txn_limbo_read_confirm()? I think that probably all the limbo functions, literally all of them, must get the latch. From the beginning to the end. And only after careful testing we could try to drop some of the locks, or at least reduce their critical sections. box.ctl functions related to the limbo/raft too. But do not insist on that. This is just how I would do it maybe.