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 4A1D96EC40; Mon, 27 Sep 2021 10:42:35 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4A1D96EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1632728555; bh=9vQT6FYDKl82AmdPK2MsQLYjAC2u7eeT79bNElhwC9c=; 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=W8iUg/N7/r74fMHok2Gv6znQDfICs8nsOg4ydY4a1tz3A/0oL8cYpl3+kl0S+2u2P KzLpdQDgKq5rjUPUHBvuAiiWpJ73WTyYMNa6BZN07fxO7pJlv6fOk5uKvW7HkKUBuk BMw5qBZDHT5gq2kC+gAq/+FTAijkTvUR+pbM7SJs= Received: from mail-lf1-f50.google.com (mail-lf1-f50.google.com [209.85.167.50]) (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 2D6C66EC40 for ; Mon, 27 Sep 2021 10:42:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2D6C66EC40 Received: by mail-lf1-f50.google.com with SMTP id y28so72892013lfb.0 for ; Mon, 27 Sep 2021 00:42:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=mJSIORwv9cz5KC+dX+DrAUkXdaTE5ijxvQSlzcU0nMU=; b=yNDC6Jvk7CCRTY4NiXD+EN3penI8LWzFKvTYG69Iewl91sBFESMVzmPj7Ns7chDy2E ckUcRO9RHbPUBr8HNhM8nlR4Kj7VUHYWXueyT+tHTakn8jOtAE4WdOOpB1nFIcg8Sz9g jLasoYRB+n/NHsDF4oNYpQEndW/BWkUnN1lPEQtCXShFzxehjQzpHcVhvQ2yMyxvfY+g GMxXHKwJURhEddBrFl5IPnWxxIg810V9D/Cd9BareEYQyjzYgU86iCMKED1tqXRVJspN zyAXZ9wyn9EhBAXdNJksh8Xbi3kv/uPDBnv7EVV9hqGBMjehcUlL5BkLzCca7iMFUFnO LF9w== X-Gm-Message-State: AOAM530s8qvSPd2gKuOPaQk0TsnYfllIoLKXUbfmhNc0WorIw9OOrxxs REfFm39QncGd1lLZeFm+1z/dLKxnmSGDxw== X-Google-Smtp-Source: ABdhPJzNp/G7xWrb6Kh1gAcGHu5cdFBPxqU41tnvEpY3JVBmlf1zPKTfgJx3mkkx8+ZoPqCzcq9u2w== X-Received: by 2002:a05:6512:202f:: with SMTP id s15mr23002615lfs.290.1632728552946; Mon, 27 Sep 2021 00:42:32 -0700 (PDT) Received: from grain.localdomain ([5.18.253.97]) by smtp.gmail.com with ESMTPSA id x20sm1512601lfr.307.2021.09.27.00.42.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 27 Sep 2021 00:42:31 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id 6FB675A001E; Mon, 27 Sep 2021 10:42:31 +0300 (MSK) Date: Mon, 27 Sep 2021 10:42:31 +0300 To: Vladislav Shpilevoy Message-ID: References: <20210922130535.79479-1-gorcunov@gmail.com> <20210922130535.79479-3-gorcunov@gmail.com> <8b6bec31-0c60-3486-ba72-b1a12d2a1144@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8b6bec31-0c60-3486-ba72-b1a12d2a1144@tarantool.org> User-Agent: Mutt/2.0.7 (2021-05-04) Subject: Re: [Tarantool-patches] [PATCH v17 2/5] 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: Cyrill Gorcunov via Tarantool-patches Reply-To: Cyrill Gorcunov Cc: tml Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" On Sun, Sep 26, 2021 at 04:29:13PM +0200, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! Hi! Thanks for review! > I consider this series as an independent patchset which fixes > the ordering, not split-brain. Like you said. > > But now it is not independent. The main problem is that you > just blended in a few changes from the split-brain patches. I > point them out in my comments. It can't be completely separate and self consisting because of the locks we introduce to order operations. In split brain series we agreed that locking should be hidden inside txn_process_begin() which can fail. In result plain txn_process() call become a plain wrapper over begin/commit|rollback and since we're changing architecture i think altering semantics immediately will make next patches less intrusive. If you prefer this way I can make it so but I think this is not that good idea, though I don't have a strong preference here. > > > > diff --git a/src/box/applier.cc b/src/box/applier.cc > > index b981bd436..f0751b68a 100644 > > --- a/src/box/applier.cc > > +++ b/src/box/applier.cc > > @@ -458,7 +458,8 @@ applier_wait_snapshot(struct applier *applier) > > struct synchro_request req; > > if (xrow_decode_synchro(&row, &req) != 0) > > diag_raise(); > > - txn_limbo_process(&txn_limbo, &req); > > + if (txn_limbo_process(&txn_limbo, &req) != 0) > > + diag_raise(); > > 1. How txn_limbo_process() can fail? You just fixed ordering. Essentially, added > a few yields in some places. You didn't add any validation, any new errors in > this patchset. Please, drop the empty 'return 0's from this patchset. They > can't be and are not tested here anyway. It is not just order fixing but scaffolds for second series of patches on top. Anyway, will do the way you're asking since you prefer. > Addition of a lock-unlock pair to txn_limbo_process didn't affect whether it > can fail. It just blocks the execution sometimes for a while. I changed semantics early on a purpose, will rework to fit your request. > > } else if (iproto_type_is_raft_request(row.type)) { > > struct raft_request req; > > if (xrow_decode_raft(&row, &req, NULL) != 0) > > @@ -857,7 +858,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_run(&txn_limbo, synchro_entry->req); > > 2. _run is usually used for infinite loops in fibers, or for handling a > sequence of something. Like trigger_run(). Here you handle a single > request. The only difference from process() is that the lock is taken. > I propose to rename it to _do or _ex or _in_tx or something. OK > > > diff --git a/src/box/box.cc b/src/box/box.cc > > index 7b11d56d6..19e67b186 100644 > > --- a/src/box/box.cc > > +++ b/src/box/box.cc > > @@ -1670,48 +1669,43 @@ box_wait_limbo_acked(double timeout) > > return wait_lsn; > > } > > > > -/** Write and process a PROMOTE request. */ > > -static void > > -box_issue_promote(uint32_t prev_leader_id, int64_t promote_lsn) > > +/** Write and process PROMOTE or DEMOTE request. */ > > +static int > > +box_issue_synchro(uint16_t type, uint32_t prev_leader_id, int64_t promote_lsn) > > { > > struct raft *raft = box_raft(); > > + > > + assert(type == IPROTO_RAFT_PROMOTE || > > + type == IPROTO_RAFT_DEMOTE); > > assert(raft->volatile_term == raft->term); > > assert(promote_lsn >= 0); > > - txn_limbo_write_promote(&txn_limbo, promote_lsn, > > - raft->term); > > + > > struct synchro_request req = { > > - .type = IPROTO_RAFT_PROMOTE, > > - .replica_id = prev_leader_id, > > - .origin_id = instance_id, > > - .lsn = promote_lsn, > > - .term = raft->term, > > + .type = type, > > + .replica_id = prev_leader_id, > > + .origin_id = instance_id, > > + .lsn = promote_lsn, > > + .term = raft->term, > > }; > > - txn_limbo_process(&txn_limbo, &req); > > + > > + if (txn_limbo_process_begin(&txn_limbo, &req) != 0) > > + return -1; > > + > > + if (type == IPROTO_RAFT_PROMOTE) > > + txn_limbo_write_promote(&txn_limbo, req.lsn, req.term); > > + else > > + txn_limbo_write_demote(&txn_limbo, req.lsn, req.term); > > + > > + txn_limbo_process_run(&txn_limbo, &req); > > assert(txn_limbo_is_empty(&txn_limbo)); > > + > > + txn_limbo_process_commit(&txn_limbo); > > + return 0; > > } > > > > /** A guard to block multiple simultaneous promote()/demote() invocations. */ > > static bool is_in_box_promote = false; > > > > -/** Write and process a DEMOTE request. */ > > -static void > > -box_issue_demote(uint32_t prev_leader_id, int64_t promote_lsn) > > -{ > > - assert(box_raft()->volatile_term == box_raft()->term); > > - assert(promote_lsn >= 0); > > - txn_limbo_write_demote(&txn_limbo, promote_lsn, > > - box_raft()->term); > > - struct synchro_request req = { > > - .type = IPROTO_RAFT_DEMOTE, > > - .replica_id = prev_leader_id, > > - .origin_id = instance_id, > > - .lsn = promote_lsn, > > - .term = box_raft()->term, > > - }; > > - txn_limbo_process(&txn_limbo, &req); > > - assert(txn_limbo_is_empty(&txn_limbo)); > > 3. Why did you merge these 2 functions? AFAIR, their split was > deliberate. To make each of them simpler to understand and maintain. To eliminate code duplication. These two functions are _completely_ identical in terms of operations: they write promote/demote packet, which in turn are the same except packet type. So I don't follow how this is easier to understand and maintain: if functions are the same better have one instance, no? But since you're asking I'll move these two functions back, no problem. > > diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c > > index 70447caaf..eb9aa7780 100644 > > --- a/src/box/txn_limbo.c > > +++ b/src/box/txn_limbo.c > > @@ -786,6 +790,32 @@ txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req) > > return; > > } > > > > +int > > +txn_limbo_process_begin(struct txn_limbo *limbo, > > + const struct synchro_request *req) > > +{ > > + latch_lock(&limbo->promote_latch); > > + /* > > + * FIXME: For now we take a lock only but idea > > + * is to verify incoming requests to detect split > > + * brain situation. Thus we need to change the code > > + * semantics in advance. > > + */ > > + (void)req; > > + return 0; > > 4. Return value is a part of the split-brain patch, not of the > ordering patch. It is clearly seen from this patchset, because > this series never changes `return 0` to anything else. > > I get that you want to merge something. Hence we are working on this > independent issue of reodering. But then lets make it truly > independent. OK. Thanks for comments! Cyrill