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 391B66EC5B; Wed, 17 Feb 2021 23:46:10 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 391B66EC5B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1613594770; bh=49IsIvCZcm47KGYi/qEzbcUHD42ikm+FD2Nclix+B5o=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=DwRSzUP1xefJbyhwaXmHIYuXd1s0mGYFoJ7g5XvL6t/QC9HKl0AZlkP5U145ihIBp C4hIqdYiCN/xP07N0MDxuJQFJUio9iIhNoOHpEwfLpLDgCaQ69kNnwSwlARemMzYyS qjlS777QggjM1+1y3svvSlld0CcW7bfbEtRQaGLU= Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (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 0D53A6EC5B for ; Wed, 17 Feb 2021 23:46:09 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0D53A6EC5B Received: by smtp33.i.mail.ru with esmtpa (envelope-from ) id 1lCTi2-0007vi-Gb; Wed, 17 Feb 2021 23:46:07 +0300 To: Serge Petrenko , gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org References: <20210211121750.46298-1-sergepetrenko@tarantool.org> Message-ID: Date: Wed, 17 Feb 2021 21:46:05 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <20210211121750.46298-1-sergepetrenko@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD975C3EC174F56692254B0AABE1FB071B2BA6557555153D6A0182A05F5380850408FF2F5311AD616A24D48C36DB2D983BEA1BC383C4B434887CC762C46E7D31779 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE75AD53DF1D86BACA3EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637B9ECF1867A781AFF8638F802B75D45FF5571747095F342E8C7A0BC55FA0FE5FCCE974CD9D9042F9197FB7259B55E2AE0F51EAE17D9ED6542389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C07E7E81EEA8A9722B8941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B64854413538E1713FCC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C224929196211C827FF9C76E601842F6C81A12EF20D2F80756B5F7E9C4E3C761E06A776E601842F6C81A127C277FBC8AE2E8B1CB35882130B44B03AA81AA40904B5D9DBF02ECDB25306B2B25CBF701D1BE8734AD6D5ED66289B5278DA827A17800CE7D151390FFDBF639967F23339F89546C5A8DF7F3B2552694A6FED454B719173D6725E5C173C3A84C3E2C51E5CF308F7B035872C767BF85DA2F004C906525384306FED454B719173D6462275124DF8B9C934F12F0C005D1A85E5BFE6E7EFDEDCD789D4C264860C145E X-B7AD71C0: 14C14B24D00AF5AC321EF223B8115265C69B993890792DF82CDD5689AFBDA7A24A6D60772A99906F8E1CD14B953EB46D29549BD9B5CBF6FC355D89D7DBCDD132 X-C1DE0DAB: 0D63561A33F958A5F092992E790751A3ABCA1EF615A6714A0A3E0BBFBAC80E0DD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D349FD10E01B3CDC9E2C3A8FDA177DFCC2DC0DE54A3D3CC0331772163B87F0E3A30936B55DD793FC6A51D7E09C32AA3244C2F711F37E833BF0C7D0D6E1F12D53E9D5A1673A01BA68E40FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojJkVkVdRHm//airilFKIdVg== X-Mailru-Sender: 504CC1E875BF3E7D9BC0E5172ADA3110D84640A56CE5E3593A5749A1E6AEDE39E076DF1A7DD911DD07784C02288277CA03E0582D3806FB6A5317862B1921BA260ED6CFD6382C13A6112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2] wal: introduce limits on simultaneous writes 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! Now looks cool indeed. Another raw idea on which I don't insist and not even sure it is good. But just came to my mind: how about making a separate object called 'journal_queue'? Or 'journal_ctl'? Which is global and is not inside of one journal. It can't be changed to another queue/ctl, and is used by journal API. So we wouldn't need to worry if we configured the correct journal because now current_journal can change at runtime, but this ctl thing - can't. Another option - call this thing 'journal', and rename the old 'journal' to 'journal_storage' or 'journal_api' or 'journal_vtab' or something like this. Another option - ignore this, since it does not matter much. But just in case you would want to try to fit the solution into one of these ideas. See 8 comments below. > diff --git a/src/box/journal.c b/src/box/journal.c > index cb320b557..49441e596 100644 > --- a/src/box/journal.c > +++ b/src/box/journal.c > @@ -55,3 +55,66 @@ journal_entry_new(size_t n_rows, struct region *region, > complete_data); > return entry; > } > + > +struct journal_queue_entry { > + /** The fiber waiting for queue space to free. */ > + struct fiber *fiber; > + /** Whether the fiber should be waken up regardless of queue size. */ > + bool is_ready; > + /** A link in all waiting fibers list. */ > + struct rlist in_queue; > +}; > + > +/** > + * Wake up the next waiter in journal queue. > + */ > +static inline void > +journal_queue_wakeup_next(struct rlist *link, bool force_ready) 1. The flag is known in all usage places at compilation time. Is it possible to split the function into force/normal versions? The same for journal_queue_wakeup() from which this runtime uncertainty arises. Also it is worth adding a comment why is force mode even needed. > +{ > + /* Empty queue or last entry in queue. */ > + if (link == rlist_last(¤t_journal->waiters)) { 2. I am not sure I understand what is happening here. Why is this function in one place called with the pointer at the list itself, and in another place with the pointer at one element? > + current_journal->queue_is_woken = false; > + return; > + } > + /* > + * When the queue isn't forcefully emptied, no need to wake everyone > + * else up until there's some free space. > + */ > + if (!force_ready && journal_queue_is_full()) { > + current_journal->queue_is_woken = false; 3. Maybe woken -> awake? 4. Why do you need the flag? Can you just remove the awake entries from the queue right away? Then it wouldn't even be possible to make a double wakeup. See comment 5. > + return; > + } > + struct journal_queue_entry *e = rlist_entry(rlist_next(link), typeof(*e), > + in_queue); > + e->is_ready = force_ready; > + fiber_wakeup(e->fiber); > +} > + > +void > +journal_queue_wakeup(bool force_ready) > +{ > + assert(!rlist_empty(¤t_journal->waiters)); > + if (current_journal->queue_is_woken) > + return; > + current_journal->queue_is_woken = true; > + journal_queue_wakeup_next(¤t_journal->waiters, force_ready); > +} > + > +void > +journal_wait_queue(void) > +{ > + struct journal_queue_entry entry = { > + .fiber = fiber(), > + .is_ready = false, > + }; > + rlist_add_tail_entry(¤t_journal->waiters, &entry, in_queue); > + /* > + * Will be waken up by either queue emptying or a synchronous write. > + */ > + while (journal_queue_is_full() && !entry.is_ready) > + fiber_yield(); > + > + journal_queue_wakeup_next(&entry.in_queue, entry.is_ready); > + assert(&entry.in_queue == rlist_first(¤t_journal->waiters)); > + rlist_del(&entry.in_queue); 5. Can rlist_del be done along with fiber_wakeup()? Then you wouldn't need is_woken maybe. > +} > diff --git a/src/box/journal.h b/src/box/journal.h > index 5d8d5a726..d295dfa4b 100644 > --- a/src/box/journal.h > +++ b/src/box/journal.h > @@ -124,6 +143,81 @@ struct journal { > struct journal_entry *entry); > }; > > +/** > + * Depending on the step of recovery and instance configuration > + * points at a concrete implementation of the journal. > + */ > +extern struct journal *current_journal; > + > +/** Wake the journal queue up. */ > +void > +journal_queue_wakeup(bool force_ready); > + > +/** > + * Check whether any of the queue size limits is reached. > + * If the queue is full, we must wait for some of the entries to be written > + * before proceeding with a new asynchronous write request. > + */ > +static inline bool > +journal_queue_is_full(void) > +{ > + struct journal *j = current_journal; > + return (j->queue_max_size != 0 && j->queue_size >= j->queue_max_size) || > + (j->queue_max_len != 0 && j->queue_len >= j->queue_max_len); 6. Seems like a lot of checks. Option 1: make queue_max_size = INT64_MAX when user passes 0. Then no need to check for != 0. The same for queue_max_len. Option 2 which may be stupid (but combined with option 1): store a flag 'is_full' and update it when update queue_size and queue_len and see they exceeded the limit. But I am not sure it reduces number of branches. Didn't check. > +} > + > +/** > + * Check whether anyone is waiting for the journal queue to empty. If there are > + * other waiters we must go after them to preserve write order. > + */ > +static inline bool > +journal_queue_has_waiters(void) > +{ > + return !rlist_empty(¤t_journal->waiters); > +} > + > +/** Yield until there's some space in the journal queue. */ > +void > +journal_wait_queue(void); > + > +/** Set maximal journal queue size in bytes. */ > +static inline void > +journal_queue_set_max_size(struct journal *j, int64_t size) 7. Why do we have journal parameter here, but don't have it in the other functions? The same journal_queue_set_max_len. > +{ > + assert(j == current_journal); > + j->queue_max_size = size; > + if (journal_queue_has_waiters() && !journal_queue_is_full()) > + journal_queue_wakeup(false); > +} > @@ -159,6 +264,12 @@ journal_write(struct journal_entry *entry) > static inline int > journal_write_async(struct journal_entry *entry) > { > + /* > + * It's the job of the caller to check whether the queue is full prior > + * to submitting the request. 8. Maybe add an assert though. > + */ > + journal_queue_on_append(entry); > + > return current_journal->write_async(current_journal, entry); > }