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 607F36EC55; Wed, 16 Jun 2021 11:02:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 607F36EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1623830536; bh=JgpjXfNIB3DeXNRkmqQIOWxjnEt/QIQ99s8QLgHwpxc=; 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=Zaq/zZ1C3/eV2Ol4kBUdWh/T0MqTgfi6WijoM8fdTjwCx4TLE/bH0p1O40pc3UFci 0dav0oRrI8+bH2mwx00ShF7b94N3KOw9/HDFnC0GzmX84pXh5jFKQVWS7A5F5QyXY8 NV2LTcfV5PYXSQLcSla/jP/CtDLpJz/SMOtiwFm4= Received: from mail-lj1-f175.google.com (mail-lj1-f175.google.com [209.85.208.175]) (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 4B4D96EC55 for ; Wed, 16 Jun 2021 11:02:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4B4D96EC55 Received: by mail-lj1-f175.google.com with SMTP id n17so2576347ljg.2 for ; Wed, 16 Jun 2021 01:02:15 -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:in-reply-to:user-agent; bh=p1x1jPYojxg4TQPTo/5L4W7ywc6c/LqZz8sF8XAJEp8=; b=J9Cj4zGUhb5ULXFuBKHT4xSRTlTMcQaoUybMdb5RXb1UsNuOblB5nBFdkZRl67oeNA QljdOnk2PzCChBbF8JA62bv9yjIt66831OsjmAitwzZK6Xld2ZnZbS/cHk+naBGgBQjr oFvsYdih9dIHLI+klqSzaenCJbXxW8Dya1zYpU+ZXUDmY74xKSMWLJFgewY49Pd2aRaF ZGBKiOR/CJdUCaC6kzmKfJPFOGWIp3jBFwV+VPUeSSuC850lUBoWOchhIIFzDigsEulB GTLJd2hBu63H8h27JKDIgQ1ZyBns/zIXcB6K5D1kTJ2xUJZyKi7Lbs7t6imbobezu4h5 jkfA== X-Gm-Message-State: AOAM5333OBBmDSMniQWjPd54hjMcWHhtIVcnt0a8I6Rog4CCAdAu0yQN F5Nmtidh86nEREVvS1QxzN2Na5Pv94M= X-Google-Smtp-Source: ABdhPJxjxewOWn1zVf4ITjMLaaoqHJmzWuMPg10nChzNOkVcnwAMdQlE2Vny/Ip4/dyKbwH1DgAxhw== X-Received: by 2002:a2e:9dce:: with SMTP id x14mr3411096ljj.154.1623830534115; Wed, 16 Jun 2021 01:02:14 -0700 (PDT) Received: from grain.localdomain ([5.18.171.94]) by smtp.gmail.com with ESMTPSA id u11sm163722lfs.257.2021.06.16.01.02.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Jun 2021 01:02:12 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id C01A65A002A; Wed, 16 Jun 2021 11:02:11 +0300 (MSK) Date: Wed, 16 Jun 2021 11:02:11 +0300 To: Vladislav Shpilevoy Message-ID: References: <9940ffc4-9d92-58b7-e95a-1128048e21da@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9940ffc4-9d92-58b7-e95a-1128048e21da@tarantool.org> User-Agent: Mutt/2.0.7 (2021-05-04) Subject: Re: [Tarantool-patches] [PATCH 04/13] wal: refactor wal_write_to_disk() 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: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" On Wed, Jun 16, 2021 at 08:22:15AM +0200, Vladislav Shpilevoy wrote: > > > On 15.06.2021 22:46, Cyrill Gorcunov wrote: > > On Fri, Jun 11, 2021 at 11:56:12PM +0200, Vladislav Shpilevoy wrote: > >> It didn't have a single fail path. That led to some amount of code > >> duplication, and it complicated future patches where the journal > >> entries are going to get a proper error reason instead of default > >> -1 without any details. > >> > >> The patch is a preparation for #6027 where it is wanted to have > >> more detailed errors on journal entry/transaction fail instead > >> of ER_WAL_IO for everything. Sometimes it can override a real > >> error like a cascade rollback, or a transaction conflict. > >> > >> Part of #6027 > >> --- > >> @@ -1038,7 +1036,10 @@ wal_write_to_disk(struct cmsg *msg) > >> { > >> struct wal_writer *writer = &wal_writer_singleton; > >> struct wal_msg *wal_msg = (struct wal_msg *) msg; > >> struct error *error; > >> + assert(!stailq_empty(&wal_msg->commit)); > > > > Hi Vlad, you know I don't understand why we need this assert... > > Otherwise in case of, for instance, rotate fail, the rollback won't > start. But the current whole code logic is assuming that if commit chain is empty then there was no data to write or rollback, iow codeflow is using list emptiness as a sign of data to procceed. If we really want to prohibit calling wal_write_to_disk with no entries then maybe better put panic here? The wal_msg->commit entry is accessed later in code anyway via bpu, which means that if we add if (stailq_empty(&wal_msg->commit)) panic("wal: attempt to update vclock without data"); this won't cause perf degradation since after a few lines you gonna be testing @commit again where bpu entry is already filled and this will allow us to catch problems on release builds as well. Don't get me wrong please I simply don't like assert() with a passion because it hides problems which might happen on release builds. Anyway, just a proposal, if you still prefer calling assert here, ok let it be. > > > > Jumps to "done" label change the code logic. Before the patch if we > > reached the write and say wal_opt_rotate failed we set up is_in_rollback > > sign and exit early, after the patch we start notifying watchers that > > there "write" happened which means relay code will be woken up while there > > no new data on disk level at all, which means watchers wanna be notified > > for no reason, no? Or I miss something obvious? > > You didn't miss anything. But I see no harm in that. WAL write fail is > extremely rare, so a rare spurious wakeup won't do anything bad. I > decided the code reusability and simplicity is more important here. OK, thanks for explanation!