[Tarantool-patches] [PATCH v2 0/2] Unthrottle vinyl scheduler on manual checkpoint

[Tarantool-patches] [PATCH v2 0/2] Unthrottle vinyl scheduler on manual checkpoint

[Nikita Pettik]

Branch: https://github.com/tarantool/tarantool/tree/np/gh-3519-unthrottle-sched-master-v2
Issue: https://github.com/tarantool/tarantool/issues/3519

Changes in v2:
 - now scheduler is unthrottled only on manual checkpoints, i.e.
fired via explicit box.snapshot() invocation. Scheduled checkpoints
still can be subjected to throttling;
 - to unthrottle scheduler only on manual checkpoints, auxiliary
boolean argument to engine->vtab->begin_checkpoint() is introduced
to tell checkpoint fired via daemon from checkpoint fired via box.snapshot();
 - patch-set has been re-based on master branch instead of 1.10.

@ChangeLog (2.5):
* box.snapshot() now ignores

Nikita Pettik (2):
  engine: add is_scheduled arg to engine->begin_checkpoint
  vinyl: unthrottle scheduler on manual checkpoint

 src/box/engine.c                 |  7 ++++---
 src/box/engine.h                 |  6 +++---
 src/box/gc.c                     |  8 ++++----
 src/box/memtx_engine.c           |  3 ++-
 src/box/vinyl.c                  |  5 +++--
 src/box/vy_scheduler.c           | 21 ++++++++++++++-------
 src/box/vy_scheduler.h           |  2 +-
 test/box/errinj.result           |  8 --------
 test/box/errinj.test.lua         |  2 --
 test/vinyl/errinj.result         |  8 --------
 test/vinyl/errinj.test.lua       |  3 ---
 test/vinyl/errinj_stat.result    |  8 --------
 test/vinyl/errinj_stat.test.lua  |  2 --
 test/vinyl/errinj_vylog.result   | 28 ----------------------------
 test/vinyl/errinj_vylog.test.lua |  8 --------
 15 files changed, 31 insertions(+), 88 deletions(-)

-- 
2.17.1

[Tarantool-patches] [PATCH v2 1/2] engine: add is_scheduled arg to engine->begin_checkpoint

[Nikita Pettik]

In some cases it may turn out to be useful to know whether checkpoint
process was launched manually (explicitly calling box.snapshot()) or
automatically via checkpoint daemon. In particular, to unthrottle vinyl
scheduler when it comes for manual checkpoints. So let's extend engine's
vtab method begin_checkpoint() with corresponding argument.

Needed for #3519
---
 src/box/engine.c       | 7 ++++---
 src/box/engine.h       | 6 +++---
 src/box/gc.c           | 8 ++++----
 src/box/memtx_engine.c | 3 ++-
 src/box/vinyl.c        | 3 ++-
 5 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/src/box/engine.c b/src/box/engine.c
index 8dc0df1d0..88ed92861 100644
--- a/src/box/engine.c
+++ b/src/box/engine.c
@@ -129,11 +129,11 @@ engine_end_recovery(void)
 }
 
 int
-engine_begin_checkpoint(void)
+engine_begin_checkpoint(bool is_scheduled)
 {
 	struct engine *engine;
 	engine_foreach(engine) {
-		if (engine->vtab->begin_checkpoint(engine) < 0)
+		if (engine->vtab->begin_checkpoint(engine, is_scheduled) < 0)
 			return -1;
 	}
 	return 0;
@@ -356,9 +356,10 @@ generic_engine_end_recovery(struct engine *engine)
 }
 
 int
-generic_engine_begin_checkpoint(struct engine *engine)
+generic_engine_begin_checkpoint(struct engine *engine, bool is_scheduled)
 {
 	(void)engine;
+	(void)is_scheduled;
 	return 0;
 }
 
diff --git a/src/box/engine.h b/src/box/engine.h
index 07d7fac9b..c4da01e13 100644
--- a/src/box/engine.h
+++ b/src/box/engine.h
@@ -157,7 +157,7 @@ struct engine_vtab {
 	 * engine (snapshot is a memtx idea of a checkpoint).
 	 * Must not yield.
 	 */
-	int (*begin_checkpoint)(struct engine *);
+	int (*begin_checkpoint)(struct engine *, bool is_scheduled);
 	/**
 	 * Wait for a checkpoint to complete.
 	 */
@@ -354,7 +354,7 @@ void
 engine_complete_join(struct engine_join_ctx *ctx);
 
 int
-engine_begin_checkpoint(void);
+engine_begin_checkpoint(bool is_scheduled);
 
 /**
  * Create a checkpoint.
@@ -396,7 +396,7 @@ int generic_engine_begin_initial_recovery(struct engine *,
 					  const struct vclock *);
 int generic_engine_begin_final_recovery(struct engine *);
 int generic_engine_end_recovery(struct engine *);
-int generic_engine_begin_checkpoint(struct engine *);
+int generic_engine_begin_checkpoint(struct engine *, bool);
 int generic_engine_wait_checkpoint(struct engine *, const struct vclock *);
 void generic_engine_commit_checkpoint(struct engine *, const struct vclock *);
 void generic_engine_abort_checkpoint(struct engine *);
diff --git a/src/box/gc.c b/src/box/gc.c
index cbcdf7d12..8e8ffea75 100644
--- a/src/box/gc.c
+++ b/src/box/gc.c
@@ -377,7 +377,7 @@ gc_add_checkpoint(const struct vclock *vclock)
 }
 
 static int
-gc_do_checkpoint(void)
+gc_do_checkpoint(bool is_scheduled)
 {
 	int rc;
 	struct wal_checkpoint checkpoint;
@@ -389,7 +389,7 @@ gc_do_checkpoint(void)
 	 * Rotate WAL and call engine callbacks to create a checkpoint
 	 * on disk for each registered engine.
 	 */
-	rc = engine_begin_checkpoint();
+	rc = engine_begin_checkpoint(is_scheduled);
 	if (rc != 0)
 		goto out;
 	rc = wal_begin_checkpoint(&checkpoint);
@@ -436,7 +436,7 @@ gc_checkpoint(void)
 				gc.checkpoint_schedule.interval);
 	fiber_wakeup(gc.checkpoint_fiber);
 
-	if (gc_do_checkpoint() != 0)
+	if (gc_do_checkpoint(false) != 0)
 		return -1;
 
 	/*
@@ -506,7 +506,7 @@ gc_checkpoint_fiber_f(va_list ap)
 			 */
 			continue;
 		}
-		if (gc_do_checkpoint() != 0)
+		if (gc_do_checkpoint(true) != 0)
 			diag_log();
 	}
 	return 0;
diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index 4da80824a..394c5d974 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -619,8 +619,9 @@ fail:
 }
 
 static int
-memtx_engine_begin_checkpoint(struct engine *engine)
+memtx_engine_begin_checkpoint(struct engine *engine, bool is_scheduled)
 {
+	(void) is_scheduled;
 	struct memtx_engine *memtx = (struct memtx_engine *)engine;
 
 	assert(memtx->checkpoint == NULL);
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index ea4839dea..2a01322f7 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -2726,8 +2726,9 @@ vinyl_engine_set_snap_io_rate_limit(struct engine *engine, double limit)
 /* {{{ Checkpoint */
 
 static int
-vinyl_engine_begin_checkpoint(struct engine *engine)
+vinyl_engine_begin_checkpoint(struct engine *engine, bool is_scheduled)
 {
+	(void) is_scheduled;
 	struct vy_env *env = vy_env(engine);
 	assert(env->status == VINYL_ONLINE);
 	/*
-- 
2.17.1

[Tarantool-patches] [PATCH v2 2/2] vinyl: unthrottle scheduler on manual checkpoint

[Nikita Pettik]

Before this patch box.snapshot() bails out immediately if it sees
that the scheduler is throttled due to errors. For instance:

box.error.injection.set('ERRINJ_VY_RUN_WRITE', true)
snapshot() -- fails due to ERRINJ_VY_RUN_WRITE
box.error.injection.set('ERRINJ_VY_RUN_WRITE', false)
snapshot() -- still fails despite the fact that injected error is unset

As a result, one has to wait up to a minute to make a snapshot. The
reason why throttling was introduced was to avoid flooding the log
in case of repeating disk errors.
What is more, to deal with schedule throttling in tests, we had to
introduce a new error injection (ERRINJ_VY_SCHED_TIMEOUT). It reduces
time duration during which the scheduler remains throttled, which is
ugly and race prone.  So, let's unthrottle scheduler when checkpoint
process is launched via manual box.snapshot() invocation.

Closes #3519
---
 src/box/vinyl.c                  |  2 +-
 src/box/vy_scheduler.c           | 21 ++++++++++++++-------
 src/box/vy_scheduler.h           |  2 +-
 test/box/errinj.result           |  8 --------
 test/box/errinj.test.lua         |  2 --
 test/vinyl/errinj.result         |  8 --------
 test/vinyl/errinj.test.lua       |  3 ---
 test/vinyl/errinj_stat.result    |  8 --------
 test/vinyl/errinj_stat.test.lua  |  2 --
 test/vinyl/errinj_vylog.result   | 28 ----------------------------
 test/vinyl/errinj_vylog.test.lua |  8 --------
 11 files changed, 16 insertions(+), 76 deletions(-)

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 2a01322f7..055e88f0f 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -2738,7 +2738,7 @@ vinyl_engine_begin_checkpoint(struct engine *engine, bool is_scheduled)
 	 */
 	if (lsregion_used(&env->mem_env.allocator) == 0)
 		return 0;
-	if (vy_scheduler_begin_checkpoint(&env->scheduler) != 0)
+	if (vy_scheduler_begin_checkpoint(&env->scheduler, is_scheduled) != 0)
 		return -1;
 	return 0;
 }
diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
index bf4c3fe58..cf58d5f60 100644
--- a/src/box/vy_scheduler.c
+++ b/src/box/vy_scheduler.c
@@ -681,22 +681,29 @@ vy_scheduler_complete_dump(struct vy_scheduler *scheduler)
 }
 
 int
-vy_scheduler_begin_checkpoint(struct vy_scheduler *scheduler)
+vy_scheduler_begin_checkpoint(struct vy_scheduler *scheduler, bool is_scheduled)
 {
 	assert(!scheduler->checkpoint_in_progress);
 
 	/*
-	 * If the scheduler is throttled due to errors, do not wait
+	 * If checkpoint is manually launched (via box.snapshot())
+	 * then ignore throttling and force dump process. Otherwise,
+	 * if the scheduler is throttled due to errors, do not wait
 	 * until it wakes up as it may take quite a while. Instead
 	 * fail checkpoint immediately with the last error seen by
 	 * the scheduler.
 	 */
 	if (scheduler->is_throttled) {
-		struct error *e = diag_last_error(&scheduler->diag);
-		diag_set_error(diag_get(), e);
-		say_error("cannot checkpoint vinyl, "
-			  "scheduler is throttled with: %s", e->errmsg);
-		return -1;
+		if (is_scheduled) {
+			struct error *e = diag_last_error(&scheduler->diag);
+			diag_set_error(diag_get(), e);
+			say_error("cannot checkpoint vinyl, "
+				  "scheduler is throttled with: %s", e->errmsg);
+			return -1;
+		}
+		say_info("scheduler is unthrottled due to manual checkpoint "
+			 "process");
+		scheduler->is_throttled = false;
 	}
 
 	if (!vy_scheduler_dump_in_progress(scheduler)) {
diff --git a/src/box/vy_scheduler.h b/src/box/vy_scheduler.h
index bc953975e..42e7b2f2f 100644
--- a/src/box/vy_scheduler.h
+++ b/src/box/vy_scheduler.h
@@ -225,7 +225,7 @@ vy_scheduler_force_compaction(struct vy_scheduler *scheduler,
  * after that.
  */
 int
-vy_scheduler_begin_checkpoint(struct vy_scheduler *);
+vy_scheduler_begin_checkpoint(struct vy_scheduler *, bool);
 
 /**
  * Wait for checkpoint. Please call vy_scheduler_end_checkpoint()
diff --git a/test/box/errinj.result b/test/box/errinj.result
index de877b708..e506e9d73 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -1585,10 +1585,6 @@ errinj.set('ERRINJ_VY_GC', true)
 ---
 - ok
 ...
-errinj.set('ERRINJ_VY_SCHED_TIMEOUT', 0.001)
----
-- ok
-...
 errinj.set('ERRINJ_VY_RUN_FILE_RENAME', true)
 ---
 - ok
@@ -1625,10 +1621,6 @@ errinj.set('ERRINJ_VY_INDEX_FILE_RENAME', false)
 ---
 - ok
 ...
-errinj.set('ERRINJ_VY_SCHED_TIMEOUT', 0)
----
-- ok
-...
 errinj.set('ERRINJ_VY_GC', false)
 ---
 - ok
diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
index 5d8f4c635..bb76e0791 100644
--- a/test/box/errinj.test.lua
+++ b/test/box/errinj.test.lua
@@ -575,7 +575,6 @@ box.snapshot()
 errinj.set('ERRINJ_VY_LOG_FILE_RENAME', false)
 
 errinj.set('ERRINJ_VY_GC', true)
-errinj.set('ERRINJ_VY_SCHED_TIMEOUT', 0.001)
 
 errinj.set('ERRINJ_VY_RUN_FILE_RENAME', true)
 box.space.test:insert{1}
@@ -590,7 +589,6 @@ box.space.test:insert{2}
 box.snapshot() -- error
 errinj.set('ERRINJ_VY_INDEX_FILE_RENAME', false)
 
-errinj.set('ERRINJ_VY_SCHED_TIMEOUT', 0)
 errinj.set('ERRINJ_VY_GC', false)
 
 test_run:cmd('restart server default')
diff --git a/test/vinyl/errinj.result b/test/vinyl/errinj.result
index 2bd701f70..bf49f4e46 100644
--- a/test/vinyl/errinj.result
+++ b/test/vinyl/errinj.result
@@ -10,10 +10,6 @@ fiber = require('fiber')
 errinj = box.error.injection
 ---
 ...
-errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.040)
----
-- ok
-...
 --
 -- Lost data in case of dump error
 --
@@ -361,10 +357,6 @@ box.snapshot();
 s:drop()
 ---
 ...
-errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0)
----
-- ok
-...
 --
 -- Check that upsert squash fiber does not crash if index or
 -- in-memory tree is gone.
diff --git a/test/vinyl/errinj.test.lua b/test/vinyl/errinj.test.lua
index 750d3bfe8..2c8121364 100644
--- a/test/vinyl/errinj.test.lua
+++ b/test/vinyl/errinj.test.lua
@@ -2,7 +2,6 @@ test_run = require('test_run').new()
 fio = require('fio')
 fiber = require('fiber')
 errinj = box.error.injection
-errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.040)
 --
 -- Lost data in case of dump error
 --
@@ -126,8 +125,6 @@ box.snapshot();
 #s:select({1})
 s:drop()
 
-errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0)
-
 --
 -- Check that upsert squash fiber does not crash if index or
 -- in-memory tree is gone.
diff --git a/test/vinyl/errinj_stat.result b/test/vinyl/errinj_stat.result
index a8b5e0312..38c27a924 100644
--- a/test/vinyl/errinj_stat.result
+++ b/test/vinyl/errinj_stat.result
@@ -214,10 +214,6 @@ errinj.set('ERRINJ_VY_RUN_WRITE', true)
 ---
 - ok
 ...
-errinj.set('ERRINJ_VY_SCHED_TIMEOUT', 0.01)
----
-- ok
-...
 s:replace{2}
 ---
 - [2]
@@ -241,10 +237,6 @@ errinj.set('ERRINJ_VY_RUN_WRITE', false)
 ---
 - ok
 ...
-errinj.set('ERRINJ_VY_SCHED_TIMEOUT', 0)
----
-- ok
-...
 test_run:wait_cond(function() return box.stat.vinyl().scheduler.tasks_completed > 1 end)
 ---
 - true
diff --git a/test/vinyl/errinj_stat.test.lua b/test/vinyl/errinj_stat.test.lua
index f331d9c2d..0d1e4af8e 100644
--- a/test/vinyl/errinj_stat.test.lua
+++ b/test/vinyl/errinj_stat.test.lua
@@ -66,14 +66,12 @@ stat.tasks_inprogress == 0
 stat.tasks_completed == 1
 stat.tasks_failed == 0
 errinj.set('ERRINJ_VY_RUN_WRITE', true)
-errinj.set('ERRINJ_VY_SCHED_TIMEOUT', 0.01)
 s:replace{2}
 box.snapshot()
 stat = box.stat.vinyl().scheduler
 stat.tasks_completed == 1
 stat.tasks_failed > 0
 errinj.set('ERRINJ_VY_RUN_WRITE', false)
-errinj.set('ERRINJ_VY_SCHED_TIMEOUT', 0)
 test_run:wait_cond(function() return box.stat.vinyl().scheduler.tasks_completed > 1 end)
 box.snapshot()
 i:compact()
diff --git a/test/vinyl/errinj_vylog.result b/test/vinyl/errinj_vylog.result
index 8fb5deda4..b9ae9332e 100644
--- a/test/vinyl/errinj_vylog.result
+++ b/test/vinyl/errinj_vylog.result
@@ -52,13 +52,6 @@ _ = s:create_index('pk')
 _ = s:insert{1, 'x'}
 ---
 ...
-SCHED_TIMEOUT = 0.05
----
-...
-box.error.injection.set('ERRINJ_VY_SCHED_TIMEOUT', SCHED_TIMEOUT)
----
-- ok
-...
 box.error.injection.set('ERRINJ_VY_LOG_FLUSH', true);
 ---
 - ok
@@ -71,13 +64,6 @@ box.error.injection.set('ERRINJ_VY_LOG_FLUSH', false);
 ---
 - ok
 ...
-fiber.sleep(2 * SCHED_TIMEOUT)
----
-...
-box.error.injection.set('ERRINJ_VY_SCHED_TIMEOUT', 0)
----
-- ok
-...
 _ = s:insert{2, 'y'}
 ---
 ...
@@ -161,24 +147,10 @@ s2.index.pk:drop()
 ---
 ...
 -- pending records must not be rolled back on error
-SCHED_TIMEOUT = 0.05
----
-...
-box.error.injection.set('ERRINJ_VY_SCHED_TIMEOUT', SCHED_TIMEOUT)
----
-- ok
-...
 box.snapshot() -- error
 ---
 - error: Error injection 'vinyl log flush'
 ...
-fiber.sleep(2 * SCHED_TIMEOUT)
----
-...
-box.error.injection.set('ERRINJ_VY_SCHED_TIMEOUT', 0)
----
-- ok
-...
 box.error.injection.set('ERRINJ_VY_LOG_FLUSH', false);
 ---
 - ok
diff --git a/test/vinyl/errinj_vylog.test.lua b/test/vinyl/errinj_vylog.test.lua
index 5ec58682e..4401f3015 100644
--- a/test/vinyl/errinj_vylog.test.lua
+++ b/test/vinyl/errinj_vylog.test.lua
@@ -27,15 +27,11 @@ s = box.schema.space.create('test', {engine = 'vinyl'})
 _ = s:create_index('pk')
 _ = s:insert{1, 'x'}
 
-SCHED_TIMEOUT = 0.05
-box.error.injection.set('ERRINJ_VY_SCHED_TIMEOUT', SCHED_TIMEOUT)
 box.error.injection.set('ERRINJ_VY_LOG_FLUSH', true);
 
 box.snapshot()
 
 box.error.injection.set('ERRINJ_VY_LOG_FLUSH', false);
-fiber.sleep(2 * SCHED_TIMEOUT)
-box.error.injection.set('ERRINJ_VY_SCHED_TIMEOUT', 0)
 
 _ = s:insert{2, 'y'}
 
@@ -79,11 +75,7 @@ _ = s1:insert{3, 'c'}
 s2.index.pk:drop()
 
 -- pending records must not be rolled back on error
-SCHED_TIMEOUT = 0.05
-box.error.injection.set('ERRINJ_VY_SCHED_TIMEOUT', SCHED_TIMEOUT)
 box.snapshot() -- error
-fiber.sleep(2 * SCHED_TIMEOUT)
-box.error.injection.set('ERRINJ_VY_SCHED_TIMEOUT', 0)
 
 box.error.injection.set('ERRINJ_VY_LOG_FLUSH', false);
 
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v2 0/2] Unthrottle vinyl scheduler on manual checkpoint

[Vladislav Shpilevoy]

Hi! Thanks for the patchset!

LGTM.

Re: [Tarantool-patches] [PATCH v2 0/2] Unthrottle vinyl scheduler on manual checkpoint

[Nikita Pettik]

On 03 May 19:01, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patchset!
> 
> LGTM.

Pushed to master; updated changelog; dropped branch.