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 E1B6E6EC58; Tue, 3 Aug 2021 14:23:27 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E1B6E6EC58 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627989808; bh=LCKnEf3/uO1g40BRFTXdbnX/tLtLJnIkQIXdwUy0lbk=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=GCBEuJN8yqS7mNzdLpffJhwBP7vJe9+va2yo/Mqv3IvlUqM1l47ICcUn3YWK4Fdc7 J0si2tGRRqYIumz40xSQskyEn3BjSveTjGmSX2VrtSFsWjOnz7GktH1GndRyFiK22L l2stmu8zjZdCPvahoZdcYu24eFg1wRyUGBb8kIKU= Received: from mail-lf1-f45.google.com (mail-lf1-f45.google.com [209.85.167.45]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id DB1276EC58 for ; Tue, 3 Aug 2021 14:23:26 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DB1276EC58 Received: by mail-lf1-f45.google.com with SMTP id c16so9620893lfc.2 for ; Tue, 03 Aug 2021 04:23:26 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=+5by5x3HokB3Ob+fx4l85Sv/oAozw7kSOBZRU47/H44=; b=HB+eGttbRozOEyLxKWfLXgUf19aB4IzLP4XqXhHXTRiqUvGP3gEbJeJrXmDdoj760l oj9MiKQ0rBfEUgGw1iRRdDEsZDRIto/k8oDyNiRKiiZnzsBb0ZAlUXMV0XWUncB/e1f+ IONucnHbDIgt3n38rx0Hqjf98sf6IY2DHSunqYlGdP/YoFNDpEV9bDbFqMppoS+iwOp1 CZcFs24cYr4gxGRmk4PHoGnoZM3pqqiQRjgMEBx2pvD8uX+OaJ+7QSJH60goQu3DfOZu qsHc319Tqc845v8gs8+4jgvAL/gIc/qDDR8r6tlaFBo5FD8tYhxbT/lLhidr1WmnprQx 7kiQ== X-Gm-Message-State: AOAM532gyrKCBDT1sR6MteAsck4MO3AfazWvXkA0g0V5vE01dtfEOQFa hQkt8T+7/cm+hlXKOxURQlzo+Xd3VKI= X-Google-Smtp-Source: ABdhPJzMQxX31pcQXdoNeemVwxjDDuKIGIhlH/4C4xQ9vUUVKlSqM60gDob5+hucurpogbfkoljcpA== X-Received: by 2002:a05:6512:3056:: with SMTP id b22mr15810250lfb.339.1627989805604; Tue, 03 Aug 2021 04:23:25 -0700 (PDT) Received: from grain.localdomain ([5.18.255.97]) by smtp.gmail.com with ESMTPSA id u30sm1222174lfn.188.2021.08.03.04.23.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 03 Aug 2021 04:23:24 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id 855B35A001E; Tue, 3 Aug 2021 14:23:23 +0300 (MSK) Date: Tue, 3 Aug 2021 14:23:23 +0300 To: Vladislav Shpilevoy Message-ID: References: <20210730113539.563318-1-gorcunov@gmail.com> <20210730113539.563318-4-gorcunov@gmail.com> <4afbed2a-36cd-9156-ccb6-d218a0db395a@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4afbed2a-36cd-9156-ccb6-d218a0db395a@tarantool.org> User-Agent: Mutt/2.0.7 (2021-05-04) 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: Cyrill Gorcunov via Tarantool-patches Reply-To: Cyrill Gorcunov Cc: tml Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" On Tue, Aug 03, 2021 at 01:48:18AM +0200, Vladislav Shpilevoy wrote: > 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); > ==================== > I thought about it, and initially used exactly this way. But I think the error code is bound to the execution context and we should setup fiber's error in a locked way simply because our fiber's schedule could be reworked in future where unlock would cause immediate resched with fiber switch and execution (say we introduce a fiber's priority), in result another fiber get running while this one woud sit without error assigned. Anyway, since sched reworks seems not happen in near future I'll update the code and use your snippet above. Kind of, I've to declare @rc earlier because otherwise I get /home/cyrill/projects/tarantool/tarantool.git/src/box/applier.cc:912:13: note: crosses initialization of ‘int rc’ 912 | int rc = journal_write(&entry.base); | ^~ > > 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'. We need to access unlocked value for read only purpose knowing that it might be changed after we've read it, and _raw suffix is commonly used for such things. Since you don't like the suffix I think better would be simply use direct access to the @promote_greatest_term variable since _volatile suffix would be too long. > > > 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. Because we pass the replica_id as an argument which implies that we fetch replica's term. But not a problem, will revert it back to the original naming. > > 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. OK > > > > +/** 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? Seems to loose it while been reordering the code, will add it back, thanks! > > + > > +/** 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? I'll drop it. --- Here is a diff on top - drop panic_on helper - access promote_greatest_term directly - bring txn_limbo_replica_term name back - use assert(latch_is_locked(&limbo->promote_latch)) test in _locked helpers - drop txn_limbo_term_is_locked helper I didn't pushed it out yet, because of the next patch which we have to discuss and rework, so mostl likely I'll send another series later, so this diff is rather to show what have been changed. --- src/box/applier.cc | 12 ++++++------ src/box/box.cc | 12 ++++++------ src/box/txn_limbo.c | 14 ++++++++------ src/box/txn_limbo.h | 32 +++++--------------------------- 4 files changed, 25 insertions(+), 45 deletions(-) diff --git a/src/box/applier.cc b/src/box/applier.cc index a7f472714..9db286ae2 100644 --- a/src/box/applier.cc +++ b/src/box/applier.cc @@ -867,6 +867,7 @@ static int apply_synchro_row(uint32_t replica_id, struct xrow_header *row) { assert(iproto_type_is_synchro_request(row->type)); + int rc = 0; struct synchro_request req; if (xrow_decode_synchro(row, &req) != 0) @@ -909,16 +910,15 @@ 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) - goto err_unlock; + 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); - goto err_unlock; + goto err; } - txn_limbo_term_unlock(&txn_limbo); return 0; -err_unlock: - txn_limbo_term_unlock(&txn_limbo); err: diag_log(); return -1; diff --git a/src/box/box.cc b/src/box/box.cc index a9f6da19d..31adb6c7d 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_term_max_raw(&txn_limbo) != promote_term) { + if (txn_limbo.promote_greatest_term != promote_term) { diag_set(ClientError, ER_INTERFERING_PROMOTE, txn_limbo.owner_id); return -1; @@ -1585,7 +1585,7 @@ box_check_promote_term_intact(uint64_t promote_term) static int box_trigger_elections(void) { - uint64_t promote_term = txn_limbo_term_max_raw(&txn_limbo); + uint64_t promote_term = txn_limbo.promote_greatest_term; raft_new_term(box_raft()); if (box_raft_wait_term_persisted() < 0) return -1; @@ -1596,7 +1596,7 @@ box_trigger_elections(void) static int box_try_wait_confirm(double timeout) { - uint64_t promote_term = txn_limbo_term_max_raw(&txn_limbo); + uint64_t promote_term = txn_limbo.promote_greatest_term; txn_limbo_wait_empty(&txn_limbo, timeout); return box_check_promote_term_intact(promote_term); } @@ -1612,7 +1612,7 @@ box_wait_limbo_acked(void) if (txn_limbo_is_empty(&txn_limbo)) return txn_limbo.confirmed_lsn; - uint64_t promote_term = txn_limbo_term_max_raw(&txn_limbo); + uint64_t promote_term = txn_limbo.promote_greatest_term; int quorum = replication_synchro_quorum; struct txn_limbo_entry *last_entry; last_entry = txn_limbo_last_synchro_entry(&txn_limbo); @@ -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_term(&txn_limbo, instance_id) == + bool is_leader = txn_limbo_replica_term(&txn_limbo, instance_id) == raft->term && txn_limbo.owner_id == instance_id; if (box_election_mode != ELECTION_MODE_OFF) is_leader = is_leader && raft->state == RAFT_STATE_LEADER; @@ -1784,7 +1784,7 @@ box_demote(void) return 0; /* Currently active leader is the only one who can issue a DEMOTE. */ - bool is_leader = txn_limbo_term(&txn_limbo, instance_id) == + bool is_leader = txn_limbo_replica_term(&txn_limbo, instance_id) == box_raft()->term && txn_limbo.owner_id == instance_id; if (box_election_mode != ELECTION_MODE_OFF) is_leader = is_leader && box_raft()->state == RAFT_STATE_LEADER; diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c index be5e0adf5..5a5565a70 100644 --- a/src/box/txn_limbo.c +++ b/src/box/txn_limbo.c @@ -46,7 +46,7 @@ txn_limbo_create(struct txn_limbo *limbo) fiber_cond_create(&limbo->wait_cond); vclock_create(&limbo->vclock); vclock_create(&limbo->promote_term_map); - limbo->promote_term_max = 0; + limbo->promote_greatest_term = 0; latch_create(&limbo->promote_latch); limbo->confirmed_lsn = 0; limbo->rollback_count = 0; @@ -309,7 +309,7 @@ txn_limbo_checkpoint(const struct txn_limbo *limbo, req->type = IPROTO_PROMOTE; req->replica_id = limbo->owner_id; req->lsn = limbo->confirmed_lsn; - req->term = limbo->promote_term_max; + req->term = limbo->promote_greatest_term; } static void @@ -728,20 +728,22 @@ void txn_limbo_process_locked(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_term_locked(limbo, origin) < term) { vclock_follow(&limbo->promote_term_map, origin, term); - if (term > limbo->promote_term_max) - limbo->promote_term_max = term; + if (term > limbo->promote_greatest_term) + limbo->promote_greatest_term = term; } else if (iproto_type_is_promote_request(req->type) && - limbo->promote_term_max > 1) { + limbo->promote_greatest_term > 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_term_max); + (long long)limbo->promote_greatest_term); return; } diff --git a/src/box/txn_limbo.h b/src/box/txn_limbo.h index 25faffd2b..abec9f72a 100644 --- a/src/box/txn_limbo.h +++ b/src/box/txn_limbo.h @@ -147,7 +147,7 @@ struct txn_limbo { * instance hasn't read its PROMOTE request yet. During other times the * limbo and raft are in sync and the terms are the same. */ - uint64_t promote_term_max; + uint64_t promote_greatest_term; /** * To order access to the promote data. */ @@ -224,26 +224,17 @@ txn_limbo_term_lock(struct txn_limbo *limbo) } /** Unlock promote data. */ -static void +static inline void 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); + assert(latch_is_locked(&limbo->promote_latch)); return vclock_get(&limbo->promote_term_map, replica_id); } @@ -252,7 +243,7 @@ txn_limbo_term_locked(struct txn_limbo *limbo, uint32_t replica_id) * @a replica_id. */ static inline uint64_t -txn_limbo_term(struct txn_limbo *limbo, uint32_t replica_id) +txn_limbo_replica_term(struct txn_limbo *limbo, uint32_t replica_id) { txn_limbo_term_lock(limbo); uint64_t v = txn_limbo_term_locked(limbo, replica_id); @@ -260,19 +251,6 @@ txn_limbo_term(struct txn_limbo *limbo, uint32_t replica_id) return v; } -/** - * Fiber's preempt not safe read of @a terms_max. - * - * Use it if you're interested in current value - * only and ready that the value is getting updated - * if after the read yield happens. - */ -static inline uint64_t -txn_limbo_term_max_raw(struct txn_limbo *limbo) -{ - return limbo->promote_term_max; -} - /** * Check whether replica with id @a source_id is too old to apply synchronous * data from it. The check is only valid when elections are enabled. @@ -283,7 +261,7 @@ txn_limbo_is_replica_outdated(struct txn_limbo *limbo, { txn_limbo_term_lock(limbo); bool res = txn_limbo_term_locked(limbo, replica_id) < - limbo->promote_term_max; + limbo->promote_greatest_term; txn_limbo_term_unlock(limbo); return res; } -- 2.31.1