<HTML><BODY><p>Please take a look at updated patch<br><br>ticket: https://github.com/tarantool/tarantool/issues/3397<br>branch: https://github.com/tarantool/tarantool/tree/kbelyavs/gh-3397-force-del-logs-on-no-disk-space</p><br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Понедельник, 18 июня 2018, 15:35 +03:00 от Vladimir Davydov <vdavydov.dev@gmail.com>:<br><br><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15293253270000000072_BODY">On Sat, Jun 09, 2018 at 05:05:10PM +0300, Konstantin Belyavskiy wrote:<br>
> Garbage collector do not delete xlog unless replica do not notify<br>
> master with newer vclock. This can lead to running out of disk<br>
> space error and this is not right behaviour since it will stop the<br>
> master.<br><br>
AFAIU this is the second iteration of this patch. If so, you should<br>
have appended the version number to the subject prefix (i.e. PATCH v2)<br>
according to our guideline:<br><br><a href="https://tarantool.io/en/doc/1.9/dev_guide/developer_guidelines.html#how-to-submit-a-patch-for-review" target="_blank">https://tarantool.io/en/doc/1.9/dev_guide/developer_guidelines.html#how-to-submit-a-patch-for-review</a><br><br>
> <br>
> List of changes:<br>
> - Promoting error from wal_thread to tx via cpipe.<br>
> - Introduce states for gc_consumers (OK and OFF). GC does not<br>
> take into account consumers with state OFF.<br>
> - Add an error injection and a test.<br><br>
The list of changes between different versions of the same patch should<br>
go after the link to the branch and the ticket. For example see<br><br><a href="https://www.freelists.org/post/tarantool-patches/PATCH-v2-box-make-gc-info-public" target="_blank">https://www.freelists.org/post/tarantool-patches/PATCH-v2-box-make-gc-info-public</a><br><br>
> <br>
> Closes #3397<br>
> ---<br>
> ticket: <a href="https://github.com/tarantool/tarantool/issues/3397" target="_blank">https://github.com/tarantool/tarantool/issues/3397</a><br>
> branch: <a href="https://github.com/tarantool/tarantool/tree/kbelyavs/gh-3397-force-del-logs-on-no-disk-space" target="_blank">https://github.com/tarantool/tarantool/tree/kbelyavs/gh-3397-force-del-logs-on-no-disk-space</a><br>
> <br>
> src/box/gc.c | 52 ++++++++++-<br>
> src/box/gc.h | 13 +++<br>
> src/box/wal.cc | 56 +++++++++--<br>
> src/errinj.h | 1 +<br>
> src/fio.c | 7 ++<br>
> test/box/errinj.result | 2 +<br>
> test/replication/force_gc_on_err_nospace.result | 107 ++++++++++++++++++++++<br>
> test/replication/force_gc_on_err_nospace.test.lua | 38 ++++++++<br>
> 8 files changed, 265 insertions(+), 11 deletions(-)<br>
> create mode 100644 test/replication/force_gc_on_err_nospace.result<br>
> create mode 100644 test/replication/force_gc_on_err_nospace.test.lua<br>
> <br>
> diff --git a/src/box/gc.c b/src/box/gc.c<br>
> index 12e68f3dc..efe033aaf 100644<br>
> --- a/src/box/gc.c<br>
> +++ b/src/box/gc.c<br>
> @@ -59,6 +59,8 @@ struct gc_consumer {<br>
> gc_node_t node;<br>
> /** Human-readable name. */<br>
> char *name;<br>
> + /** Dead or alive. GC ignores dead consumers. */<br>
> + enum gc_consumer_state state;<br><br>
Keeping dead consumers around is pointless. A consumer must be removed<br>
from the tree as soon as it is evicted.</div></div></div></div></blockquote>Fixed<blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15293253270000000072_BODY"><br>
> +void<br>
> +gc_xdir_clean_notify(bool force_clean)<br>
> +{<br>
> + if (force_clean == gc.force_clean_flag)<br>
> + return; // nothing to do<br>
> + else if (force_clean) { // xdir clean required<br>
> + gc.force_clean_flag = true;<br><br>
We don't use C++ style comments in the code.</div></div></div></div></blockquote>Fixed<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15293253270000000072_BODY"><br>
> + /**<br>
> + * Mark consumer with least recent vclock as "dead" and<br>
> + * invoke garbage collection. If nothing to delete find<br>
> + * next alive consumer etc. Originally created for<br>
> + * cases with running out of disk space because of<br>
> + * disconnected replica.<br>
> + */<br>
> + struct gc_consumer *leftmost =<br>
> + gc_tree_first_alive(&gc.consumers);<br>
> + if (leftmost == NULL)<br>
> + return; // do nothing<br>
> + int64_t signature = leftmost->signature;<br>
> + while (true) {<br>
> + leftmost->state = CONSUMER_OFF;<br>
> + leftmost = gc_tree_first_alive(&gc.consumers);<br>
> + if (leftmost == NULL ||<br>
> + leftmost->signature > signature) {<br>
> + gc_run();<br>
> + return;<br><br>
A consumer may correspond to a backup procedure, in which case it must<br>
not be evicted, even on ENOSPC.</div></div></div></div></blockquote>Fixed<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15293253270000000072_BODY"><br><br>
> + }<br>
> + }<br>
> + } else<br>
> + gc.force_clean_flag = false;<br>
> +}<br><br>
> diff --git a/src/box/wal.cc b/src/box/wal.cc<br>
> index 099c70caa..9dd8347d5 100644<br>
> --- a/src/box/wal.cc<br>
> +++ b/src/box/wal.cc<br>
> @@ -59,6 +62,11 @@ struct wal_thread {<br>
> struct cord cord;<br>
> /** A pipe from 'tx' thread to 'wal' */<br>
> struct cpipe wal_pipe;<br>
> + /**<br>
> + * Return pipe from 'wal' to tx'. This is a<br>
> + * priority pipe and DOES NOT support yield.<br>
> + */<br>
> + struct cpipe tx_prio_pipe;<br>
> /** Return pipe from 'wal' to tx' */<br>
> struct cpipe tx_pipe;<br>
> };<br>
> @@ -154,7 +166,7 @@ static void<br>
> tx_schedule_commit(struct cmsg *msg);<br>
> <br>
> static struct cmsg_hop wal_request_route[] = {<br>
> - {wal_write_to_disk, &wal_thread.tx_pipe},<br>
> + {wal_write_to_disk, &wal_thread.tx_prio_pipe},<br><br>
AFAICS you rename wal_thread.tx_pipe to tx_prio_pipe and create a new<br>
pipe called wal_thread.tx_pipe to be used for garbage collection, all in<br>
one patch. This is very difficult to review. Provided you really need<br>
it, renaming should be done in a separate patch with a good explanation<br>
why you're doing what you're doing. Currently, I fail to see a reason.<br><br>
> +static void<br>
> +gc_status_update(struct cmsg *msg)<br>
> +{<br>
> + msg->route = NULL;<br>
> + say_info("QQQ: gc_status_update");<br><br>
What is this?</div></div></div></div></blockquote>Sorry, my fault (use to find in logs).<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15293253270000000072_BODY"><br><br>
> + gc_xdir_clean_notify(static_cast<gc_msg*>(msg)->io_err);<br>
> +}<br>
> +<br>
> static void<br>
> wal_write_to_disk(struct cmsg *msg)<br>
> {<br>
> @@ -647,11 +667,29 @@ wal_write_to_disk(struct cmsg *msg)<br>
> last_committed = stailq_last(&wal_msg->commit);<br>
> <br>
> done:<br>
> + bool need_send = false;<br>
> struct error *error = diag_last_error(diag_get());<br>
> if (error) {<br>
> /* Until we can pass the error to tx, log it and clear. */<br>
> error_log(error);<br>
> diag_clear(diag_get());<br>
> + if (errno == ENOSPC) {<br>
> + err_nospace = true;<br>
> + need_send = true;<br>
> + }<br>
> + } else if (err_nospace) {<br>
> + /** Clear flag and send message to gc. */<br>
> + err_nospace = false;<br>
> + need_send = true;<br>
> + }<br>
> + if (need_send) {<br><br>
This joggling with flags looks confusing. What are you trying to achieve<br>
here? Avoid invoking garbage collector twice on ENOSPC error? At the<br>
very least, this code should be accompanied by a comment.<br><br>
Also, AFAICS you fail a transaction that hit ENOSPC and triggered<br>
garbage collection. I don't think it's good. I see two ways of<br>
rectifying this: either retry WAL write after invoking garbage<br>
collection or use fallocate to reserve some space beyond the end of<br>
the file before writing to WAL.</div></div></div></div></blockquote><br>Fixed by comparing current timestamp with the previous one.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15293253270000000072_BODY"><br><br>
> + struct gc_msg msg;<br>
> + static const struct cmsg_hop route[] = {<br>
> + {gc_status_update, NULL}<br>
> + };<br>
> + cmsg_init(&msg, route);<br>
> + msg.io_err = err_nospace;<br>
> + cpipe_push(&wal_thread.tx_pipe, &msg);<br>
> }<br><br>
Pushing a message defined on stack to a pipe and not waiting for it to<br>
be delivered doesn't look right.</div></div></div></div></blockquote>
Fixed<br>
<br>Best regards,<br>Konstantin Belyavskiy<br>k.belyavskiy@tarantool.org<br></BODY></HTML>