[tarantool-patches] [xlog 1/1] xlog: Remove inprogress files on start

[tarantool-patches] [xlog 1/1] xlog: Remove inprogress files on start

[Ilya Markov]

When tarantool crashes during writing to vylog, index, run or snapshot,
inprogress files remain. But garbage collector doesn't take into account
these files. So they remain until they are removed manually.

Fix this with adding function to box_cfg which traverses memtx_dir and
vinyl_dir and removes inprogress files.

* Add 4 errinj which simulate the crash before renaming inprogress
  files.
* Change signature of engine_commit_checkpoint function in order to
support error injection in snapshot commit.

Closes #3406
---
branch: gh-3406-remove-inprogress-files
 src/box/box.cc           |  20 +++++-
 src/box/engine.c         |   3 +-
 src/box/engine.h         |   2 +-
 src/box/memtx_engine.c   |  21 +++++-
 src/box/sysview_engine.c |   3 +-
 src/box/vinyl.c          |   3 +-
 src/box/vy_log.c         |   4 ++
 src/box/vy_run.c         |  14 ++++
 src/box/xlog.c           |  73 +++++++++++++++++++-
 src/box/xlog.h           |  13 ++++
 src/errinj.h             |   4 ++
 test/box/errinj.result   | 171 ++++++++++++++++++++++++++++++++++++++++++++++-
 test/box/errinj.test.lua |  59 ++++++++++++++++
 13 files changed, 378 insertions(+), 12 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index e3eb273..1e75b15 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1683,6 +1683,24 @@ tx_prio_cb(struct ev_loop *loop, ev_watcher *watcher, int events)
 	cbus_process(endpoint);
 }
 
+/**
+ * Removes .inprogress files in memtx_dir, vinyl_dir and vinyl_dir subdirs.
+ */
+static void
+collect_vinyl_subdirectories_inprogress()
+{
+	/* Remove inprogress files in directories. */
+	if (xdir_collect_inprogress(cfg_gets("memtx_dir")) < 0 ||
+	    (strcmp(cfg_gets("memtx_dir"),
+		    cfg_gets("vinyl_dir")) != 0 &&
+	     xdir_collect_inprogress(
+		     cfg_gets("vinyl_dir")) < 0))
+		diag_raise();
+	if (space_foreach(collect_vinyl_inprogress,
+			  (void *) cfg_gets("vinyl_dir")) < 0)
+		diag_raise();
+}
+
 void
 box_init(void)
 {
@@ -1810,7 +1828,6 @@ box_cfg_xc(void)
 		 */
 		memtx_engine_recover_snapshot_xc(memtx,
 				&last_checkpoint_vclock);
-
 		engine_begin_final_recovery_xc();
 		recovery_follow_local(recovery, &wal_stream.base, "hot_standby",
 				      cfg_getd("wal_dir_rescan_delay"));
@@ -1867,6 +1884,7 @@ box_cfg_xc(void)
 
 		/* Wait for the cluster to start up */
 		box_sync_replication(replication_connect_timeout, false);
+		collect_vinyl_subdirectories_inprogress();
 	} else {
 		if (!tt_uuid_is_nil(&instance_uuid))
 			INSTANCE_UUID = instance_uuid;
diff --git a/src/box/engine.c b/src/box/engine.c
index e4ae156..8a0e885 100644
--- a/src/box/engine.c
+++ b/src/box/engine.c
@@ -134,7 +134,8 @@ engine_commit_checkpoint(struct vclock *vclock)
 			return -1;
 	}
 	engine_foreach(engine) {
-		engine->vtab->commit_checkpoint(engine, vclock);
+		if (engine->vtab->commit_checkpoint(engine, vclock) < 0)
+			return -1;
 	}
 	return 0;
 }
diff --git a/src/box/engine.h b/src/box/engine.h
index 5f8b589..80b3306 100644
--- a/src/box/engine.h
+++ b/src/box/engine.h
@@ -149,7 +149,7 @@ struct engine_vtab {
 	 * All engines prepared their checkpoints,
 	 * fix up the changes.
 	 */
-	void (*commit_checkpoint)(struct engine *, struct vclock *);
+	int (*commit_checkpoint)(struct engine *, struct vclock *);
 	/**
 	 * An error in one of the engines, abort checkpoint.
 	 */
diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index 9a9aff5..bb350a9 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -703,7 +703,7 @@ memtx_engine_wait_checkpoint(struct engine *engine, struct vclock *vclock)
 	return result;
 }
 
-static void
+static int
 memtx_engine_commit_checkpoint(struct engine *engine, struct vclock *vclock)
 {
 	(void) vclock;
@@ -732,9 +732,16 @@ memtx_engine_commit_checkpoint(struct engine *engine, struct vclock *vclock)
 				fiber_sleep(0.001);
 		}
 #endif
+		ERROR_INJECT(ERRINJ_SNAP_MEMTX, {
+			diag_set(ClientError, ER_INJECTION,
+				 "memtx snapshot commit");
+			return -1;
+		});
 		int rc = coio_rename(from, to);
-		if (rc != 0)
-			panic("can't rename .snap.inprogress");
+		if (rc != 0) {
+			diag_set(SystemError, "can't rename .snap.inprogress");
+			return -1;
+		}
 	}
 
 	struct vclock last;
@@ -748,6 +755,7 @@ memtx_engine_commit_checkpoint(struct engine *engine, struct vclock *vclock)
 
 	checkpoint_destroy(memtx->checkpoint);
 	memtx->checkpoint = NULL;
+	return 0;
 }
 
 static void
@@ -767,6 +775,13 @@ memtx_engine_abort_checkpoint(struct engine *engine)
 
 	memtx_tuple_end_snapshot();
 
+	ERROR_INJECT(ERRINJ_SNAP_MEMTX, {
+		/* Don't remove inprogress files. */
+		checkpoint_destroy(memtx->checkpoint);
+		memtx->checkpoint = NULL;
+		return;
+	});
+
 	/** Remove garbage .inprogress file. */
 	char *filename =
 		xdir_format_filename(&memtx->checkpoint->dir,
diff --git a/src/box/sysview_engine.c b/src/box/sysview_engine.c
index b478e78..b63bfc0 100644
--- a/src/box/sysview_engine.c
+++ b/src/box/sysview_engine.c
@@ -335,11 +335,12 @@ sysview_engine_wait_checkpoint(struct engine *engine, struct vclock *vclock)
 	return 0;
 }
 
-static void
+static int
 sysview_engine_commit_checkpoint(struct engine *engine, struct vclock *vclock)
 {
 	(void)engine;
 	(void)vclock;
+	return 0;
 }
 
 static void
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 552d42b..ee4dbd8 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -2861,13 +2861,14 @@ vinyl_engine_wait_checkpoint(struct engine *engine, struct vclock *vclock)
 	return 0;
 }
 
-static void
+static int
 vinyl_engine_commit_checkpoint(struct engine *engine, struct vclock *vclock)
 {
 	(void)vclock;
 	struct vy_env *env = vy_env(engine);
 	assert(env->status == VINYL_ONLINE);
 	vy_scheduler_end_checkpoint(&env->scheduler);
+	return 0;
 }
 
 static void
diff --git a/src/box/vy_log.c b/src/box/vy_log.c
index 51e3753..cea3080 100644
--- a/src/box/vy_log.c
+++ b/src/box/vy_log.c
@@ -848,6 +848,10 @@ vy_log_open(struct xlog *xlog)
 	struct vy_log_record record;
 	vy_log_record_init(&record);
 	record.type = VY_LOG_SNAPSHOT;
+	ERROR_INJECT(ERRINJ_VY_LOG_OPEN, {
+		diag_set(ClientError, ER_INJECTION, "vy_log open");
+		return -1;
+	});
 	if (vy_log_record_encode(&record, &row) < 0 ||
 	    xlog_write_row(xlog, &row) < 0)
 		goto fail_close_xlog;
diff --git a/src/box/vy_run.c b/src/box/vy_run.c
index 980bc4d..6b41620 100644
--- a/src/box/vy_run.c
+++ b/src/box/vy_run.c
@@ -2021,6 +2021,14 @@ vy_run_write_index(struct vy_run *run, const char *dirpath,
 			goto fail;
 	}
 
+	ERROR_INJECT(ERRINJ_VY_INDEX_CREATE, {
+		region_truncate(region, mem_used);
+		xlog_tx_rollback(&index_xlog);
+		xlog_close(&index_xlog, false);
+		diag_set(ClientError, ER_INJECTION,
+			 "vinyl index write");
+		return -1;
+	});
 	if (xlog_tx_commit(&index_xlog) < 0 ||
 	    xlog_flush(&index_xlog) < 0 ||
 	    xlog_rename(&index_xlog) < 0)
@@ -2263,6 +2271,12 @@ vy_run_writer_commit(struct vy_run_writer *writer)
 	run->info.max_key = vy_key_dup(key);
 	if (run->info.max_key == NULL)
 		goto out;
+	ERROR_INJECT(ERRINJ_VY_RUN_COMMIT, {
+		region_truncate(&fiber()->gc, region_svp);
+		diag_set(ClientError, ER_INJECTION,
+			 "vinyl writer commit");
+		return -1;
+	});
 
 	/* Sync data and link the file to the final name. */
 	if (xlog_sync(&writer->data_xlog) < 0 ||
diff --git a/src/box/xlog.c b/src/box/xlog.c
index 824ad11..1750d52 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -46,6 +46,7 @@
 #include "xrow.h"
 #include "iproto_constants.h"
 #include "errinj.h"
+#include "space.h"
 
 /*
  * marker is MsgPack fixext2
@@ -621,6 +622,77 @@ xdir_collect_garbage(struct xdir *dir, int64_t signature, bool use_coio)
 	return 0;
 }
 
+int
+xdir_collect_inprogress(const char *dirname)
+{
+	DIR *dh = opendir(dirname);
+	if (dh == NULL) {
+		if (errno == ENOENT)
+			return 0;
+		diag_set(SystemError, "error reading directory '%s'",
+			 dirname);
+		return -1;
+	}
+
+	struct dirent *dent;
+	char path[PATH_MAX];
+	int total = snprintf(path, sizeof(path), "%s/", dirname);
+	if (total < 0) {
+		say_syserror("error snprintf %s", dirname);
+		diag_set(SystemError, "error snprintf'",
+			 path);
+		return -1;
+	}
+	while ((dent = readdir(dh)) != NULL) {
+		char *ext = strrchr(dent->d_name, '.');
+		if (ext == NULL || strcmp(ext, inprogress_suffix) != 0)
+			continue;
+		strcpy(path + total, dent->d_name);
+		int rc = coio_unlink(path);
+		if (rc < 0) {
+			say_syserror("error while removing %s", path);
+			diag_set(SystemError, "failed to unlink file '%s'",
+				 path);
+			return -1;
+		}
+	}
+	return 0;
+}
+
+int
+collect_vinyl_inprogress(struct space *space, void *param)
+{
+	if (space->def->engine_name[0] != 'v')
+		return 0;
+	const char * vinyl_dir = (const char *) param;
+	static char path[PATH_MAX];
+	int total = snprintf(path, sizeof(path), "%s/%i/", vinyl_dir,
+			     space->def->id);
+	if (total < 0) {
+		say_syserror("error snprintf %s", vinyl_dir);
+		diag_set(SystemError, "error snprintf'",
+			 path);
+		return -1;
+	}
+
+	DIR *dh = opendir(path);
+	if (dh == NULL) {
+		if (errno == ENOENT)
+			return 0;
+		diag_set(SystemError, "error reading directory '%s'",
+			 path);
+		return -1;
+	}
+	struct dirent *dent;
+	while ((dent = readdir(dh)) != NULL) {
+		/* Add iid to path */
+		strcpy(path + total, dent->d_name);
+		if (xdir_collect_inprogress(path) < 0)
+			return -1;
+	}
+	return 0;
+}
+
 /* }}} */
 
 
@@ -636,7 +708,6 @@ xlog_rename(struct xlog *l)
 	assert(l->is_inprogress);
 	assert(suffix);
 	assert(strcmp(suffix, inprogress_suffix) == 0);
-
 	/* Create a new filename without '.inprogress' suffix. */
 	memcpy(new_filename, filename, suffix - filename);
 	new_filename[suffix - filename] = '\0';
diff --git a/src/box/xlog.h b/src/box/xlog.h
index 973910d..cb3c8b0 100644
--- a/src/box/xlog.h
+++ b/src/box/xlog.h
@@ -44,6 +44,7 @@
 
 struct iovec;
 struct xrow_header;
+struct space;
 
 #if defined(__cplusplus)
 extern "C" {
@@ -181,6 +182,18 @@ int
 xdir_collect_garbage(struct xdir *dir, int64_t signature, bool use_coio);
 
 /**
+ * Remove .inprogress files in specified directory.
+ */
+int
+xdir_collect_inprogress(const char *dirname);
+
+/**
+ * Remove .inprogress files in vinyl subdirectories.
+ * Used as parameter to space_foreach.
+ */
+int
+collect_vinyl_inprogress(struct space *space, void *param);
+/**
  * Return LSN and vclock (unless @vclock is NULL) of the newest
  * file in a directory or -1 if the directory is empty.
  */
diff --git a/src/errinj.h b/src/errinj.h
index 78514ac..8cfeff1 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -111,6 +111,10 @@ struct errinj {
 	_(ERRINJ_IPROTO_TX_DELAY, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_LOG_ROTATE, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_SNAP_COMMIT_DELAY, ERRINJ_BOOL, {.bparam = 0}) \
+	_(ERRINJ_SNAP_MEMTX, ERRINJ_BOOL, {.bparam = false}) \
+	_(ERRINJ_VY_LOG_OPEN, ERRINJ_BOOL, {.bparam = false}) \
+	_(ERRINJ_VY_INDEX_CREATE, ERRINJ_BOOL, {.bparam = false}) \
+	_(ERRINJ_VY_RUN_COMMIT, ERRINJ_BOOL, {.bparam = false}) \
 
 ENUM0(errinj_id, ERRINJ_LIST);
 extern struct errinj errinjs[];
diff --git a/test/box/errinj.result b/test/box/errinj.result
index 6ced172..aca495e 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -4,6 +4,12 @@ errinj = box.error.injection
 net_box = require('net.box')
 ---
 ...
+fio = require("fio")
+---
+...
+fiber = require('fiber')
+---
+...
 space = box.schema.space.create('tweedledum')
 ---
 ...
@@ -16,6 +22,8 @@ errinj.info()
     state: 0
   ERRINJ_WAL_WRITE:
     state: false
+  ERRINJ_VY_RUN_COMMIT:
+    state: false
   ERRINJ_VYRUN_DATA_READ:
     state: false
   ERRINJ_VY_SCHED_TIMEOUT:
@@ -52,10 +60,16 @@ errinj.info()
     state: false
   ERRINJ_WAL_WRITE_DISK:
     state: false
+  ERRINJ_TUPLE_FIELD:
+    state: false
   ERRINJ_VY_RUN_WRITE:
     state: false
+  ERRINJ_VY_LOG_OPEN:
+    state: false
   ERRINJ_VY_LOG_FLUSH_DELAY:
     state: false
+  ERRINJ_VY_INDEX_DUMP:
+    state: -1
   ERRINJ_SNAP_COMMIT_DELAY:
     state: false
   ERRINJ_RELAY_FINAL_SLEEP:
@@ -74,7 +88,7 @@ errinj.info()
     state: false
   ERRINJ_BUILD_SECONDARY:
     state: -1
-  ERRINJ_TUPLE_FIELD:
+  ERRINJ_VY_INDEX_CREATE:
     state: false
   ERRINJ_XLOG_GARBAGE:
     state: false
@@ -90,8 +104,8 @@ errinj.info()
     state: 0
   ERRINJ_VY_LOG_FLUSH:
     state: false
-  ERRINJ_VY_INDEX_DUMP:
-    state: -1
+  ERRINJ_SNAP_MEMTX:
+    state: false
 ...
 errinj.set("some-injection", true)
 ---
@@ -693,6 +707,157 @@ errinj.set("ERRINJ_WAL_WRITE", false)
 space:drop()
 ---
 ...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+errinj.set("ERRINJ_SNAP_MEMTX", true)
+---
+- ok
+...
+box.snapshot()
+---
+- error: Error injection 'memtx snapshot commit'
+...
+errinj.set("ERRINJ_SNAP_MEMTX", false)
+---
+- ok
+...
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+---
+...
+#fio.glob(memtx_snap) > 0 -- true
+---
+- true
+...
+errinj.set("ERRINJ_VY_LOG_OPEN", true)
+---
+- ok
+...
+s = box.schema.space.create("vinyl_test", {engine='vinyl'})
+---
+...
+_ = s:create_index("primary")
+---
+...
+errinj.set("ERRINJ_VY_LOG_OPEN", false)
+---
+- ok
+...
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+---
+...
+#fio.glob(vinyl_vylog) > 0 -- true
+---
+- true
+...
+#fio.glob(memtx_snap) > 0 -- true
+---
+- true
+...
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+errinj.set("ERRINJ_VY_INDEX_CREATE", true)
+---
+- ok
+...
+-- insert big tuples in order to cause compaction without box.snapshot.
+for i = 1, 10000 do s:insert{i, string.rep('a', 10000)} end
+---
+...
+run_dir = fio.pathjoin(box.cfg.vinyl_dir, s.id, 0)
+---
+...
+while #fio.glob(vinyl_index) == 0 do fiber.sleep(0.001) end
+---
+...
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+errinj.set("ERRINJ_VY_RUN_COMMIT", true)
+---
+- ok
+...
+while not #fio.glob(vinyl_run) == 0  do fiber.sleep(0.001) end
+---
+...
+#fio.glob(memtx_snap) > 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_vylog) > 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_index) > 0 -- true
+---
+- true
+...
+test_run:cmd("restart server default")
+errinj = box.error.injection
+---
+...
+net_box = require('net.box')
+---
+...
+fio = require("fio")
+---
+...
+fiber = require('fiber')
+---
+...
+s = box.space.vinyl_test
+---
+...
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+---
+...
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+---
+...
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+-- no inprogress files should be present in memtx(vinyl)_dir.
+#fio.glob(memtx_snap) == 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_vylog) == 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_index) == 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_run) == 0 -- true
+---
+- true
+...
+box.space.vinyl_test:drop()
+---
+...
+run_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/*/*")
+---
+...
+for _, file in ipairs(fio.glob(run_dir)) do fio.unlink(file) end
+---
+...
+vinyl_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/")
+---
+...
+for _, dir in ipairs(fio.glob(vinyl_dir)) do fio.rmtree(dir) end
+---
+...
 --test space:bsize() in case of memory error
 utils = dofile('utils.lua')
 ---
diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
index 3af1b74..1bd1f05 100644
--- a/test/box/errinj.test.lua
+++ b/test/box/errinj.test.lua
@@ -1,5 +1,7 @@
 errinj = box.error.injection
 net_box = require('net.box')
+fio = require("fio")
+fiber = require('fiber')
 
 space = box.schema.space.create('tweedledum')
 index = space:create_index('primary', { type = 'hash' })
@@ -203,6 +205,63 @@ box.snapshot()
 errinj.set("ERRINJ_WAL_WRITE", false)
 space:drop()
 
+test_run:cmd("setopt delimiter ';'")
+test_run:cmd("setopt delimiter ''");
+
+errinj.set("ERRINJ_SNAP_MEMTX", true)
+box.snapshot()
+errinj.set("ERRINJ_SNAP_MEMTX", false)
+
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+#fio.glob(memtx_snap) > 0 -- true
+
+errinj.set("ERRINJ_VY_LOG_OPEN", true)
+s = box.schema.space.create("vinyl_test", {engine='vinyl'})
+_ = s:create_index("primary")
+errinj.set("ERRINJ_VY_LOG_OPEN", false)
+
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+#fio.glob(vinyl_vylog) > 0 -- true
+#fio.glob(memtx_snap) > 0 -- true
+
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+errinj.set("ERRINJ_VY_INDEX_CREATE", true)
+-- insert big tuples in order to cause compaction without box.snapshot.
+for i = 1, 10000 do s:insert{i, string.rep('a', 10000)} end
+
+run_dir = fio.pathjoin(box.cfg.vinyl_dir, s.id, 0)
+while #fio.glob(vinyl_index) == 0 do fiber.sleep(0.001) end
+
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+errinj.set("ERRINJ_VY_RUN_COMMIT", true)
+while not #fio.glob(vinyl_run) == 0  do fiber.sleep(0.001) end
+#fio.glob(memtx_snap) > 0 -- true
+#fio.glob(vinyl_vylog) > 0 -- true
+#fio.glob(vinyl_index) > 0 -- true
+
+test_run:cmd("restart server default")
+errinj = box.error.injection
+net_box = require('net.box')
+fio = require("fio")
+fiber = require('fiber')
+
+s = box.space.vinyl_test
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+
+-- no inprogress files should be present in memtx(vinyl)_dir.
+#fio.glob(memtx_snap) == 0 -- true
+#fio.glob(vinyl_vylog) == 0 -- true
+#fio.glob(vinyl_index) == 0 -- true
+#fio.glob(vinyl_run) == 0 -- true
+
+box.space.vinyl_test:drop()
+run_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/*/*")
+for _, file in ipairs(fio.glob(run_dir)) do fio.unlink(file) end
+vinyl_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/")
+for _, dir in ipairs(fio.glob(vinyl_dir)) do fio.rmtree(dir) end
 --test space:bsize() in case of memory error
 utils = dofile('utils.lua')
 s = box.schema.space.create('space_bsize')
-- 
2.7.4

Re: [tarantool-patches] [xlog 1/1] xlog: Remove inprogress files on start

[Vladimir Davydov]

0. Please carefully read and follow our guideline on how to submit
   patches:

   https://tarantool.io/en/doc/1.9/dev_guide/developer_guidelines.html#how-to-submit-a-patch-for-review

   In particular, the subject prefix should be PATCH (default if you use
   git-format-patch), not subsystem name, and the branch name as well as
   the GitHub ticket should be given as hyperlinks.

   When you resubmit a new version of your patch, please don't forget to
   append the version to the subject prefix (e.g. PATCH v2), and write a
   brief change log, e.g. see

   https://www.freelists.org/post/tarantool-patches/PATCH-v2-box-make-gc-info-public

   I'm kinda tired of reiterating those simple rules over and over
   again. I think I'll simply ignore all patches that don't conform to
   the guideline...

   Also, before submitting a patch for review, please make sure there
   are no stray hunks, like adding/removing blank lines. Your patch has
   a few. Fixing a comment or removing a blank line is OK only if it
   goes to a hunk you need anyway.

1. engine_vtab::commit_checkpoint() must not fail, because if it does,
   it may leave the box in inconsistent state, when there's no vinyl
   checkpoint corresponding to a memtx snapshot. An attempt to recover
   from such a state will fail. That's why we panic in
   memtx_engine_commit_checkpoint().

   To test deletion of snap.inprogress files, you don't need to change
   that - it's enough to set the error injection that delays renaming of
   the snap file (it already exists), start checkpoint in a fiber, and
   then restart the server.

2. Removal of engine-specific files should be done in engine callbacks.
   For instance, snap.inprogress files should probably be removed from
   memtx_engine_end_recovery().

3. Scanning vinyl_dir is totally wrong, because information about all
   vinyl files, even .inprogress ones, is stored in vylog. Moreover,
   code that removes incomplete vinyl files already exists - see
   vinyl_engine_end_recovery() - all you need to do is implement removal
   of .inprogress files there.

On Sat, Jun 09, 2018 at 05:46:37PM +0300, Ilya Markov wrote:
> When tarantool crashes during writing to vylog, index, run or snapshot,
> inprogress files remain. But garbage collector doesn't take into account
> these files. So they remain until they are removed manually.
> 
> Fix this with adding function to box_cfg which traverses memtx_dir and
> vinyl_dir and removes inprogress files.
> 
> * Add 4 errinj which simulate the crash before renaming inprogress
>   files.
> * Change signature of engine_commit_checkpoint function in order to
> support error injection in snapshot commit.
> 
> Closes #3406
> ---
> branch: gh-3406-remove-inprogress-files
>  src/box/box.cc           |  20 +++++-
>  src/box/engine.c         |   3 +-
>  src/box/engine.h         |   2 +-
>  src/box/memtx_engine.c   |  21 +++++-
>  src/box/sysview_engine.c |   3 +-
>  src/box/vinyl.c          |   3 +-
>  src/box/vy_log.c         |   4 ++
>  src/box/vy_run.c         |  14 ++++
>  src/box/xlog.c           |  73 +++++++++++++++++++-
>  src/box/xlog.h           |  13 ++++
>  src/errinj.h             |   4 ++
>  test/box/errinj.result   | 171 ++++++++++++++++++++++++++++++++++++++++++++++-
>  test/box/errinj.test.lua |  59 ++++++++++++++++
>  13 files changed, 378 insertions(+), 12 deletions(-)

[PATCH v2] xlog: Remove inprogress files on start

[Ilya Markov]

When tarantool crashes during writing to vylog, index, run or snapshot,
inprogress files remain. But garbage collector doesn't take into account
these files. So they remain until they are removed manually.

Fix this with adding deletion of such files to functions memtx_engine_end_recovery,
vinyl_engine_end_recovery and vy_run_remove_files.

Add 3 errinj which simulate the crash before renaming inprogress files.

Closes #3406
---
branch: https://github.com/tarantool/tarantool/tree/gh-3406-remove-inprogress-files
issue: https://github.com/tarantool/tarantool/issues/3406

 src/box/memtx_engine.c   |   2 +-
 src/box/vinyl.c          |   1 +
 src/box/vy_log.c         |   4 ++
 src/box/vy_run.c         |  17 +++++
 src/box/xlog.c           |  38 +++++++++++
 src/box/xlog.h           |   6 ++
 src/errinj.h             |   3 +
 test/box/errinj.result   | 168 +++++++++++++++++++++++++++++++++++++++++++++++
 test/box/errinj.test.lua |  63 ++++++++++++++++++
 9 files changed, 301 insertions(+), 1 deletion(-)

diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index 9a9aff5..3ab60f0 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -303,7 +303,7 @@ memtx_engine_end_recovery(struct engine *engine)
 		if (space_foreach(memtx_build_secondary_keys, memtx) != 0)
 			return -1;
 	}
-	return 0;
+	return xdir_collect_inprogress(memtx->snap_dir.dirname);
 }

 static struct space *
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 552d42b..69646b6 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -2941,6 +2941,7 @@ vinyl_engine_end_recovery(struct engine *engine)
 	case VINYL_FINAL_RECOVERY_LOCAL:
 		if (vy_log_end_recovery() != 0)
 			return -1;
+		xdir_collect_inprogress(e->path);
 		/*
 		 * If the instance is shut down while a dump or
 		 * compaction task is in progress, we'll get an
diff --git a/src/box/vy_log.c b/src/box/vy_log.c
index 51e3753..3519bde 100644
--- a/src/box/vy_log.c
+++ b/src/box/vy_log.c
@@ -848,6 +848,10 @@ vy_log_open(struct xlog *xlog)
 	struct vy_log_record record;
 	vy_log_record_init(&record);
 	record.type = VY_LOG_SNAPSHOT;
+	ERROR_INJECT(ERRINJ_VY_LOG_RENAME, {
+		diag_set(ClientError, ER_INJECTION, "vy_log open");
+		return -1;
+	});
 	if (vy_log_record_encode(&record, &row) < 0 ||
 	    xlog_write_row(xlog, &row) < 0)
 		goto fail_close_xlog;
diff --git a/src/box/vy_run.c b/src/box/vy_run.c
index 980bc4d..1dda93d 100644
--- a/src/box/vy_run.c
+++ b/src/box/vy_run.c
@@ -2021,6 +2021,14 @@ vy_run_write_index(struct vy_run *run, const char *dirpath,
 			goto fail;
 	}

+	ERROR_INJECT(ERRINJ_VY_INDEX_FILE_RENAME, {
+		region_truncate(region, mem_used);
+		xlog_tx_rollback(&index_xlog);
+		xlog_close(&index_xlog, false);
+		diag_set(ClientError, ER_INJECTION,
+			 "vinyl index write");
+		return -1;
+	});
 	if (xlog_tx_commit(&index_xlog) < 0 ||
 	    xlog_flush(&index_xlog) < 0 ||
 	    xlog_rename(&index_xlog) < 0)
@@ -2263,6 +2271,12 @@ vy_run_writer_commit(struct vy_run_writer *writer)
 	run->info.max_key = vy_key_dup(key);
 	if (run->info.max_key == NULL)
 		goto out;
+	ERROR_INJECT(ERRINJ_VY_RUN_RENAME, {
+		region_truncate(&fiber()->gc, region_svp);
+		diag_set(ClientError, ER_INJECTION,
+			 "vinyl writer commit");
+		return -1;
+	});

 	/* Sync data and link the file to the final name. */
 	if (xlog_sync(&writer->data_xlog) < 0 ||
@@ -2439,6 +2453,9 @@ vy_run_remove_files(const char *dir, uint32_t space_id,
 				(long long)run_id); return -1;});
 	int ret = 0;
 	char path[PATH_MAX];
+	vy_index_snprint_path(path, PATH_MAX, dir, space_id, iid);
+	if (xdir_collect_inprogress(path) < 0)
+		return -1;
 	for (int type = 0; type < vy_file_MAX; type++) {
 		vy_run_snprint_path(path, sizeof(path), dir,
 				    space_id, iid, run_id, type);
diff --git a/src/box/xlog.c b/src/box/xlog.c
index 824ad11..b5cc494 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -621,6 +621,44 @@ xdir_collect_garbage(struct xdir *dir, int64_t signature, bool use_coio)
 	return 0;
 }

+int
+xdir_collect_inprogress(const char *dirname)
+{
+	DIR *dh = opendir(dirname);
+	if (dh == NULL) {
+		if (errno == ENOENT)
+			return 0;
+		diag_set(SystemError, "error reading directory '%s'",
+			 dirname);
+		return -1;
+	}
+
+	struct dirent *dent;
+	char path[PATH_MAX];
+	int total = snprintf(path, sizeof(path), "%s/", dirname);
+	if (total < 0) {
+		say_syserror("error snprintf %s", dirname);
+		diag_set(SystemError, "error snprintf'",
+			 path);
+		return -1;
+	}
+	while ((dent = readdir(dh)) != NULL) {
+		char *ext = strrchr(dent->d_name, '.');
+		if (ext == NULL || strcmp(ext, inprogress_suffix) != 0)
+			continue;
+		strcpy(path + total, dent->d_name);
+		say_info("removing %s", path);
+		int rc = coio_unlink(path);
+		if (rc < 0) {
+			say_syserror("error while removing %s", path);
+			diag_set(SystemError, "failed to unlink file '%s'",
+				 path);
+			return -1;
+		}
+	}
+	return 0;
+}
+
 /* }}} */


diff --git a/src/box/xlog.h b/src/box/xlog.h
index 973910d..4faa763 100644
--- a/src/box/xlog.h
+++ b/src/box/xlog.h
@@ -181,6 +181,12 @@ int
 xdir_collect_garbage(struct xdir *dir, int64_t signature, bool use_coio);

 /**
+ * Remove .inprogress files in specified directory.
+ */
+int
+xdir_collect_inprogress(const char *dirname);
+
+/**
  * Return LSN and vclock (unless @vclock is NULL) of the newest
  * file in a directory or -1 if the directory is empty.
  */
diff --git a/src/errinj.h b/src/errinj.h
index 78514ac..8444479 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -111,6 +111,9 @@ struct errinj {
 	_(ERRINJ_IPROTO_TX_DELAY, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_LOG_ROTATE, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_SNAP_COMMIT_DELAY, ERRINJ_BOOL, {.bparam = 0}) \
+	_(ERRINJ_VY_LOG_RENAME, ERRINJ_BOOL, {.bparam = false}) \
+	_(ERRINJ_VY_INDEX_FILE_RENAME, ERRINJ_BOOL, {.bparam = false}) \
+	_(ERRINJ_VY_RUN_RENAME, ERRINJ_BOOL, {.bparam = false}) \

 ENUM0(errinj_id, ERRINJ_LIST);
 extern struct errinj errinjs[];
diff --git a/test/box/errinj.result b/test/box/errinj.result
index 6ced172..831c8e3 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -4,6 +4,12 @@ errinj = box.error.injection
 net_box = require('net.box')
 ---
 ...
+fio = require("fio")
+---
+...
+fiber = require('fiber')
+---
+...
 space = box.schema.space.create('tweedledum')
 ---
 ...
@@ -40,6 +46,8 @@ errinj.info()
     state: false
   ERRINJ_WAL_IO:
     state: false
+  ERRINJ_VY_INDEX_FILE_RENAME:
+    state: false
   ERRINJ_TUPLE_ALLOC:
     state: false
   ERRINJ_VY_READ_PAGE:
@@ -54,8 +62,12 @@ errinj.info()
     state: false
   ERRINJ_VY_RUN_WRITE:
     state: false
+  ERRINJ_VY_RUN_RENAME:
+    state: false
   ERRINJ_VY_LOG_FLUSH_DELAY:
     state: false
+  ERRINJ_VY_LOG_RENAME:
+    state: false
   ERRINJ_SNAP_COMMIT_DELAY:
     state: false
   ERRINJ_RELAY_FINAL_SLEEP:
@@ -693,6 +705,162 @@ errinj.set("ERRINJ_WAL_WRITE", false)
 space:drop()
 ---
 ...
+errinj.set("ERRINJ_VY_LOG_RENAME", true)
+---
+- ok
+...
+s = box.schema.space.create("vinyl_test", {engine='vinyl'})
+---
+...
+_ = s:create_index("primary")
+---
+...
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+---
+...
+#fio.glob(vinyl_vylog) > 0 -- true
+---
+- true
+...
+errinj.set("ERRINJ_SNAP_COMMIT_DELAY", true)
+---
+- ok
+...
+_ = fiber.create(function() box.snapshot() end)
+---
+...
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+---
+...
+while #fio.glob(memtx_snap) == 0 do fiber.sleep(0.001) end-- true
+---
+...
+#fio.glob(memtx_snap) > 0 --true
+---
+- true
+...
+test_run:cmd("restart server default")
+errinj = box.error.injection
+---
+...
+net_box = require('net.box')
+---
+...
+fio = require("fio")
+---
+...
+fiber = require('fiber')
+---
+...
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+---
+...
+#fio.glob(memtx_snap) == 0 -- true
+---
+- true
+...
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+---
+...
+#fio.glob(vinyl_vylog) == 0 --true
+---
+- true
+...
+s = box.space.vinyl_test
+---
+...
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+errinj.set("ERRINJ_VY_INDEX_FILE_RENAME", true)
+---
+- ok
+...
+-- insert big tuples in order to cause compaction without box.snapshot.
+for i = 1, 10000 do s:insert{i, string.rep('a', 10000)} end
+---
+...
+while #fio.glob(vinyl_index) == 0 do fiber.sleep(0.001) end
+---
+...
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+errinj.set("ERRINJ_VY_RUN_RENAME", true)
+---
+- ok
+...
+while #fio.glob(vinyl_run) == 0  do fiber.sleep(0.001) end
+---
+...
+#fio.glob(vinyl_run) > 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_index) > 0 -- true
+---
+- true
+...
+test_run:cmd("restart server default")
+errinj = box.error.injection
+---
+...
+net_box = require('net.box')
+---
+...
+fio = require("fio")
+---
+...
+fiber = require('fiber')
+---
+...
+s = box.space.vinyl_test
+---
+...
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+---
+...
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+---
+...
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+-- no inprogress files should be present in memtx(vinyl)_dir.
+#fio.glob(memtx_snap) == 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_vylog) == 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_index) == 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_run) == 0 -- true
+---
+- true
+...
+box.space.vinyl_test:drop()
+---
+...
+run_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/*/*")
+---
+...
+for _, file in ipairs(fio.glob(run_dir)) do fio.unlink(file) end
+---
+...
+vinyl_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/")
+---
+...
+for _, dir in ipairs(fio.glob(vinyl_dir)) do fio.rmtree(dir) end
+---
+...
 --test space:bsize() in case of memory error
 utils = dofile('utils.lua')
 ---
diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
index 3af1b74..515049a 100644
--- a/test/box/errinj.test.lua
+++ b/test/box/errinj.test.lua
@@ -1,5 +1,7 @@
 errinj = box.error.injection
 net_box = require('net.box')
+fio = require("fio")
+fiber = require('fiber')

 space = box.schema.space.create('tweedledum')
 index = space:create_index('primary', { type = 'hash' })
@@ -203,6 +205,67 @@ box.snapshot()
 errinj.set("ERRINJ_WAL_WRITE", false)
 space:drop()

+errinj.set("ERRINJ_VY_LOG_RENAME", true)
+s = box.schema.space.create("vinyl_test", {engine='vinyl'})
+_ = s:create_index("primary")
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+#fio.glob(vinyl_vylog) > 0 -- true
+
+errinj.set("ERRINJ_SNAP_COMMIT_DELAY", true)
+_ = fiber.create(function() box.snapshot() end)
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+while #fio.glob(memtx_snap) == 0 do fiber.sleep(0.001) end-- true
+#fio.glob(memtx_snap) > 0 --true
+
+test_run:cmd("restart server default")
+errinj = box.error.injection
+net_box = require('net.box')
+fio = require("fio")
+fiber = require('fiber')
+
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+#fio.glob(memtx_snap) == 0 -- true
+
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+#fio.glob(vinyl_vylog) == 0 --true
+
+s = box.space.vinyl_test
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+errinj.set("ERRINJ_VY_INDEX_FILE_RENAME", true)
+-- insert big tuples in order to cause compaction without box.snapshot.
+for i = 1, 10000 do s:insert{i, string.rep('a', 10000)} end
+
+while #fio.glob(vinyl_index) == 0 do fiber.sleep(0.001) end
+
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+errinj.set("ERRINJ_VY_RUN_RENAME", true)
+while #fio.glob(vinyl_run) == 0  do fiber.sleep(0.001) end
+#fio.glob(vinyl_run) > 0 -- true
+#fio.glob(vinyl_index) > 0 -- true
+
+test_run:cmd("restart server default")
+errinj = box.error.injection
+net_box = require('net.box')
+fio = require("fio")
+fiber = require('fiber')
+
+s = box.space.vinyl_test
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+
+-- no inprogress files should be present in memtx(vinyl)_dir.
+#fio.glob(memtx_snap) == 0 -- true
+#fio.glob(vinyl_vylog) == 0 -- true
+#fio.glob(vinyl_index) == 0 -- true
+#fio.glob(vinyl_run) == 0 -- true
+
+box.space.vinyl_test:drop()
+run_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/*/*")
+for _, file in ipairs(fio.glob(run_dir)) do fio.unlink(file) end
+vinyl_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/")
+for _, dir in ipairs(fio.glob(vinyl_dir)) do fio.rmtree(dir) end
 --test space:bsize() in case of memory error
 utils = dofile('utils.lua')
 s = box.schema.space.create('space_bsize')
--
2.7.4

Re: [PATCH v2] xlog: Remove inprogress files on start

[Vladimir Davydov]

On Mon, Jun 18, 2018 at 07:23:34PM +0300, Ilya Markov wrote:
> When tarantool crashes during writing to vylog, index, run or snapshot,
> inprogress files remain. But garbage collector doesn't take into account
> these files. So they remain until they are removed manually.
> 
> Fix this with adding deletion of such files to functions memtx_engine_end_recovery,
> vinyl_engine_end_recovery and vy_run_remove_files.
> 
> Add 3 errinj which simulate the crash before renaming inprogress files.
> 
> Closes #3406
> ---
> branch: https://github.com/tarantool/tarantool/tree/gh-3406-remove-inprogress-files
> issue: https://github.com/tarantool/tarantool/issues/3406

List of changes between v1 and v2 is missing, although I asked you to
add one:

On Mon, Jun 18, 2018 at 12:44:07PM +0300, Vladimir Davydov wrote:
>    When you resubmit a new version of your patch, please don't forget to
>    append the version to the subject prefix (e.g. PATCH v2), and write a
>    brief change log, e.g. see
> 
>    https://www.freelists.org/post/tarantool-patches/PATCH-v2-box-make-gc-info-public

Don't forget it next time.

> 
>  src/box/memtx_engine.c   |   2 +-
>  src/box/vinyl.c          |   1 +
>  src/box/vy_log.c         |   4 ++
>  src/box/vy_run.c         |  17 +++++
>  src/box/xlog.c           |  38 +++++++++++
>  src/box/xlog.h           |   6 ++
>  src/errinj.h             |   3 +
>  test/box/errinj.result   | 168 +++++++++++++++++++++++++++++++++++++++++++++++
>  test/box/errinj.test.lua |  63 ++++++++++++++++++
>  9 files changed, 301 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
> index 9a9aff5..3ab60f0 100644
> --- a/src/box/memtx_engine.c
> +++ b/src/box/memtx_engine.c
> @@ -303,7 +303,7 @@ memtx_engine_end_recovery(struct engine *engine)
>  		if (space_foreach(memtx_build_secondary_keys, memtx) != 0)
>  			return -1;
>  	}
> -	return 0;
> +	return xdir_collect_inprogress(memtx->snap_dir.dirname);

No need to abort recovery if an inprogress file can't be deleted, a
warning in the log will do. Garbage collection should be best-effort.

>  }
> 
>  static struct space *
> diff --git a/src/box/vinyl.c b/src/box/vinyl.c
> index 552d42b..69646b6 100644
> --- a/src/box/vinyl.c
> +++ b/src/box/vinyl.c
> @@ -2941,6 +2941,7 @@ vinyl_engine_end_recovery(struct engine *engine)
>  	case VINYL_FINAL_RECOVERY_LOCAL:
>  		if (vy_log_end_recovery() != 0)
>  			return -1;
> +		xdir_collect_inprogress(e->path);

Removal of vylog files should be done from vy_log_end_recovery()
(for the sake of encapsulation).

>  		/*
>  		 * If the instance is shut down while a dump or
>  		 * compaction task is in progress, we'll get an
> diff --git a/src/box/vy_log.c b/src/box/vy_log.c
> index 51e3753..3519bde 100644
> --- a/src/box/vy_log.c
> +++ b/src/box/vy_log.c
> @@ -848,6 +848,10 @@ vy_log_open(struct xlog *xlog)
>  	struct vy_log_record record;
>  	vy_log_record_init(&record);
>  	record.type = VY_LOG_SNAPSHOT;
> +	ERROR_INJECT(ERRINJ_VY_LOG_RENAME, {
> +		diag_set(ClientError, ER_INJECTION, "vy_log open");
> +		return -1;
> +	});

The error injection should go right before xlog_rename(), and it should
close the xlog before returning from the function.

Also, be consistent with other error messages used on injection:

  ERRINJ_VY_READ_PAGE      "vinyl page read"
  ERRINJ_VY_INDEX_DUMP     "vinyl index dump"
  ERRINJ_VY_LOG_FLUSH      "vinyl log flush"

So for ERRINJ_VY_LOG_RENAME it should be "vinyl log rename".


>  	if (vy_log_record_encode(&record, &row) < 0 ||
>  	    xlog_write_row(xlog, &row) < 0)
>  		goto fail_close_xlog;
> diff --git a/src/box/vy_run.c b/src/box/vy_run.c
> index 980bc4d..1dda93d 100644
> --- a/src/box/vy_run.c
> +++ b/src/box/vy_run.c
> @@ -2021,6 +2021,14 @@ vy_run_write_index(struct vy_run *run, const char *dirpath,
>  			goto fail;
>  	}
> 
> +	ERROR_INJECT(ERRINJ_VY_INDEX_FILE_RENAME, {
> +		region_truncate(region, mem_used);
> +		xlog_tx_rollback(&index_xlog);
> +		xlog_close(&index_xlog, false);
> +		diag_set(ClientError, ER_INJECTION,
> +			 "vinyl index write");
> +		return -1;
> +	});

Fix the error message as per above and use labels to remove this
copy&paste.

>  	if (xlog_tx_commit(&index_xlog) < 0 ||
>  	    xlog_flush(&index_xlog) < 0 ||
>  	    xlog_rename(&index_xlog) < 0)
> @@ -2263,6 +2271,12 @@ vy_run_writer_commit(struct vy_run_writer *writer)
>  	run->info.max_key = vy_key_dup(key);
>  	if (run->info.max_key == NULL)
>  		goto out;
> +	ERROR_INJECT(ERRINJ_VY_RUN_RENAME, {
> +		region_truncate(&fiber()->gc, region_svp);
> +		diag_set(ClientError, ER_INJECTION,
> +			 "vinyl writer commit");
> +		return -1;
> +	});

Ditto.

> 
>  	/* Sync data and link the file to the final name. */
>  	if (xlog_sync(&writer->data_xlog) < 0 ||
> @@ -2439,6 +2453,9 @@ vy_run_remove_files(const char *dir, uint32_t space_id,
>  				(long long)run_id); return -1;});
>  	int ret = 0;
>  	char path[PATH_MAX];
> +	vy_index_snprint_path(path, PATH_MAX, dir, space_id, iid);
> +	if (xdir_collect_inprogress(path) < 0)
> +		return -1;

What's the point of using xdir_collect_inprogress here? It scans the
directory which is an overkill, as file names are already known. Besides
it blocks the tx thread. You have a loop removing files below. Simply
make it remove inprogress files as well.

>  	for (int type = 0; type < vy_file_MAX; type++) {
>  		vy_run_snprint_path(path, sizeof(path), dir,
>  				    space_id, iid, run_id, type);
> diff --git a/src/box/xlog.c b/src/box/xlog.c
> index 824ad11..b5cc494 100644
> --- a/src/box/xlog.c
> +++ b/src/box/xlog.c
> @@ -621,6 +621,44 @@ xdir_collect_garbage(struct xdir *dir, int64_t signature, bool use_coio)
>  	return 0;
>  }
> 
> +int
> +xdir_collect_inprogress(const char *dirname)

Judging by its signature, this function should take an xdir object.

> +{
> +	DIR *dh = opendir(dirname);
> +	if (dh == NULL) {
> +		if (errno == ENOENT)
> +			return 0;
> +		diag_set(SystemError, "error reading directory '%s'",
> +			 dirname);
> +		return -1;
> +	}
> +
> +	struct dirent *dent;
> +	char path[PATH_MAX];
> +	int total = snprintf(path, sizeof(path), "%s/", dirname);
> +	if (total < 0) {

snprintf never fails.

> +		say_syserror("error snprintf %s", dirname);
> +		diag_set(SystemError, "error snprintf'",
> +			 path);
> +		return -1;
> +	}
> +	while ((dent = readdir(dh)) != NULL) {
> +		char *ext = strrchr(dent->d_name, '.');
> +		if (ext == NULL || strcmp(ext, inprogress_suffix) != 0)
> +			continue;
> +		strcpy(path + total, dent->d_name);
> +		say_info("removing %s", path);
> +		int rc = coio_unlink(path);

readdir is blocking so why use coio_unlink?

> +		if (rc < 0) {
> +			say_syserror("error while removing %s", path);
> +			diag_set(SystemError, "failed to unlink file '%s'",
> +				 path);
> +			return -1;
> +		}
> +	}
> +	return 0;
> +}
> +
>  /* }}} */
> 
> 
> diff --git a/src/box/xlog.h b/src/box/xlog.h
> index 973910d..4faa763 100644
> --- a/src/box/xlog.h
> +++ b/src/box/xlog.h
> @@ -181,6 +181,12 @@ int
>  xdir_collect_garbage(struct xdir *dir, int64_t signature, bool use_coio);
> 
>  /**
> + * Remove .inprogress files in specified directory.
> + */
> +int
> +xdir_collect_inprogress(const char *dirname);
> +
> +/**
>   * Return LSN and vclock (unless @vclock is NULL) of the newest
>   * file in a directory or -1 if the directory is empty.
>   */
> diff --git a/src/errinj.h b/src/errinj.h
> index 78514ac..8444479 100644
> --- a/src/errinj.h
> +++ b/src/errinj.h
> @@ -111,6 +111,9 @@ struct errinj {
>  	_(ERRINJ_IPROTO_TX_DELAY, ERRINJ_BOOL, {.bparam = false}) \
>  	_(ERRINJ_LOG_ROTATE, ERRINJ_BOOL, {.bparam = false}) \
>  	_(ERRINJ_SNAP_COMMIT_DELAY, ERRINJ_BOOL, {.bparam = 0}) \
> +	_(ERRINJ_VY_LOG_RENAME, ERRINJ_BOOL, {.bparam = false}) \
> +	_(ERRINJ_VY_INDEX_FILE_RENAME, ERRINJ_BOOL, {.bparam = false}) \
> +	_(ERRINJ_VY_RUN_RENAME, ERRINJ_BOOL, {.bparam = false}) \

Error injection names are inconsistent (VY_INDEX_FILE_RENAME vs
VY_RUN_RENAME). Fix it.

[PATCH v3] xlog: Remove inprogress files on start

[Ilya Markov]

When tarantool crashes during writing to vylog, index, run or snapshot,
inprogress files remain. But garbage collector doesn't take into account
these files. So they remain until they are removed manually.

Fix this with adding deletion of such files to functions memtx_engine_end_recovery,
vinyl_engine_end_recovery and vy_run_remove_files.

Add 3 errinj which simulate the crash before renaming inprogress files.
Moved const string inprogress_suffix to xlog.h in order to reuse it in
vinyl run and index files removal.

Closes #3406
---
branch: https://github.com/tarantool/tarantool/tree/gh-3406-remove-inprogress-files
issue: https://github.com/tarantool/tarantool/issues/3406

Changes in v3:
- Delete abortion of end_recovery in case if inprgress file deletion fails.
  Replace it with diag_log.
- Move deletion of vylog.inprogress to vy_log_end_recovery().
- Move error new injections before relevant calls of xlog_rename().
- Change names and error messages of error injections.
- Removed copy&paste in error injection code and add comment where it's impossible.
- Changed signature of xdir_collect_inprogress function.
- Removed call xdir_collect_inprogress from vy_run_remove_files,
  replace it with implicit call of coio_unlink.
- Replace coio_unlink in xdir_collect_inprogress with unlink.
- Move const string inprogress_suffix to xlog.h.

 src/box/memtx_engine.c   |   2 +
 src/box/vy_log.c         |   7 +-
 src/box/vy_run.c         |  40 +++++++++--
 src/box/xlog.c           |  35 +++++++++-
 src/box/xlog.h           |  11 ++++
 src/errinj.h             |   3 +
 test/box/errinj.result   | 168 +++++++++++++++++++++++++++++++++++++++++++++++
 test/box/errinj.test.lua |  63 ++++++++++++++++++
 8 files changed, 321 insertions(+), 8 deletions(-)

diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index 9a9aff5..1566d26 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -303,6 +303,8 @@ memtx_engine_end_recovery(struct engine *engine)
 		if (space_foreach(memtx_build_secondary_keys, memtx) != 0)
 			return -1;
 	}
+	if (xdir_collect_inprogress(&memtx->snap_dir) < 0)
+		diag_log();
 	return 0;
 }

diff --git a/src/box/vy_log.c b/src/box/vy_log.c
index 51e3753..cd343b9 100644
--- a/src/box/vy_log.c
+++ b/src/box/vy_log.c
@@ -852,6 +852,10 @@ vy_log_open(struct xlog *xlog)
 	    xlog_write_row(xlog, &row) < 0)
 		goto fail_close_xlog;

+	ERROR_INJECT(ERRINJ_VY_LOG_FILE_RENAME, {
+		diag_set(ClientError, ER_INJECTION, "vinyl log file rename");
+		return -1;
+	});
 	if (xlog_rename(xlog) < 0)
 		goto fail_close_xlog;

@@ -976,7 +980,8 @@ vy_log_end_recovery(void)
 			return -1;
 		}
 	}
-
+	if (xdir_collect_inprogress(&vy_log.dir) < 0)
+		diag_log();
 	vy_log.recovery = NULL;
 	return 0;
 }
diff --git a/src/box/vy_run.c b/src/box/vy_run.c
index 980bc4d..96736ba 100644
--- a/src/box/vy_run.c
+++ b/src/box/vy_run.c
@@ -2021,6 +2021,19 @@ vy_run_write_index(struct vy_run *run, const char *dirpath,
 			goto fail;
 	}

+	ERROR_INJECT(ERRINJ_VY_INDEX_FILE_RENAME, {
+		/*
+		 * Can't use here goto label fail,
+		 * because we don't want to unlink file in errinj and can't move
+		 * unlink before xlog_close call.
+		 */
+		region_truncate(region, mem_used);
+		xlog_tx_rollback(&index_xlog);
+		xlog_close(&index_xlog, false);
+		diag_set(ClientError, ER_INJECTION,
+			 "vinyl index file rename");
+		return -1;
+	});
 	if (xlog_tx_commit(&index_xlog) < 0 ||
 	    xlog_flush(&index_xlog) < 0 ||
 	    xlog_rename(&index_xlog) < 0)
@@ -2263,6 +2276,12 @@ vy_run_writer_commit(struct vy_run_writer *writer)
 	run->info.max_key = vy_key_dup(key);
 	if (run->info.max_key == NULL)
 		goto out;
+	ERROR_INJECT(ERRINJ_VY_RUN_FILE_RENAME, {
+		diag_set(ClientError, ER_INJECTION,
+			 "vinyl run file rename");
+		rc = -1;
+		goto out;
+	});

 	/* Sync data and link the file to the final name. */
 	if (xlog_sync(&writer->data_xlog) < 0 ||
@@ -2430,6 +2449,15 @@ close_err:
 	return -1;
 }

+static inline void
+remove_file(const char *path, int *ret) {
+	say_info("removing %s", path);
+	if (coio_unlink(path) < 0 && errno != ENOENT) {
+		say_syserror("error while removing %s", path);
+		*ret = -1;
+	}
+}
+
 int
 vy_run_remove_files(const char *dir, uint32_t space_id,
 		    uint32_t iid, int64_t run_id)
@@ -2439,14 +2467,14 @@ vy_run_remove_files(const char *dir, uint32_t space_id,
 				(long long)run_id); return -1;});
 	int ret = 0;
 	char path[PATH_MAX];
+	int total = 0;
 	for (int type = 0; type < vy_file_MAX; type++) {
-		vy_run_snprint_path(path, sizeof(path), dir,
+		total = vy_run_snprint_path(path, sizeof(path), dir,
 				    space_id, iid, run_id, type);
-		say_info("removing %s", path);
-		if (coio_unlink(path) < 0 && errno != ENOENT) {
-			say_syserror("error while removing %s", path);
-			ret = -1;
-		}
+		remove_file(path, &ret);
+		snprintf(path + total, sizeof(path) - total, "%s",
+			 inprogress_suffix);
+		remove_file(path, &ret);
 	}
 	return ret;
 }
diff --git a/src/box/xlog.c b/src/box/xlog.c
index 824ad11..7bd618e 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -58,7 +58,6 @@ typedef uint32_t log_magic_t;
 static const log_magic_t row_marker = mp_bswap_u32(0xd5ba0bab); /* host byte order */
 static const log_magic_t zrow_marker = mp_bswap_u32(0xd5ba0bba); /* host byte order */
 static const log_magic_t eof_marker = mp_bswap_u32(0xd510aded); /* host byte order */
-static const char inprogress_suffix[] = ".inprogress";

 enum {
 	/**
@@ -621,6 +620,40 @@ xdir_collect_garbage(struct xdir *dir, int64_t signature, bool use_coio)
 	return 0;
 }

+int
+xdir_collect_inprogress(struct xdir *xdir)
+{
+	const char *dirname = xdir->dirname;
+	DIR *dh = opendir(dirname);
+	if (dh == NULL) {
+		if (errno == ENOENT)
+			return 0;
+		diag_set(SystemError, "error reading directory '%s'",
+			 dirname);
+		return -1;
+	}
+
+	struct dirent *dent;
+	char path[PATH_MAX];
+	int total = snprintf(path, sizeof(path), "%s/", dirname);
+
+	while ((dent = readdir(dh)) != NULL) {
+		char *ext = strrchr(dent->d_name, '.');
+		if (ext == NULL || strcmp(ext, inprogress_suffix) != 0)
+			continue;
+		strcpy(path + total, dent->d_name);
+		say_info("removing %s", path);
+		int rc = unlink(path);
+		if (rc < 0) {
+			say_syserror("error while removing %s", path);
+			diag_set(SystemError, "failed to unlink file '%s'",
+				 path);
+			return -1;
+		}
+	}
+	return 0;
+}
+
 /* }}} */


diff --git a/src/box/xlog.h b/src/box/xlog.h
index 973910d..eba687f 100644
--- a/src/box/xlog.h
+++ b/src/box/xlog.h
@@ -70,6 +70,11 @@ enum xdir_type {
 enum log_suffix { NONE, INPROGRESS };

 /**
+ * Suffix added to path of inprogress files.
+ */
+static const char inprogress_suffix[] = ".inprogress";
+
+/**
  * A handle for a data directory with write ahead logs, snapshots,
  * vylogs.
  * Can be used to find the last log in the directory, scan
@@ -181,6 +186,12 @@ int
 xdir_collect_garbage(struct xdir *dir, int64_t signature, bool use_coio);

 /**
+ * Remove .inprogress files in specified directory.
+ */
+int
+xdir_collect_inprogress(struct xdir *xdir);
+
+/**
  * Return LSN and vclock (unless @vclock is NULL) of the newest
  * file in a directory or -1 if the directory is empty.
  */
diff --git a/src/errinj.h b/src/errinj.h
index 78514ac..7141af9 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -111,6 +111,9 @@ struct errinj {
 	_(ERRINJ_IPROTO_TX_DELAY, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_LOG_ROTATE, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_SNAP_COMMIT_DELAY, ERRINJ_BOOL, {.bparam = 0}) \
+	_(ERRINJ_VY_LOG_FILE_RENAME, ERRINJ_BOOL, {.bparam = false}) \
+	_(ERRINJ_VY_INDEX_FILE_RENAME, ERRINJ_BOOL, {.bparam = false}) \
+	_(ERRINJ_VY_RUN_FILE_RENAME, ERRINJ_BOOL, {.bparam = false}) \

 ENUM0(errinj_id, ERRINJ_LIST);
 extern struct errinj errinjs[];
diff --git a/test/box/errinj.result b/test/box/errinj.result
index 6ced172..d995a0f 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -4,6 +4,12 @@ errinj = box.error.injection
 net_box = require('net.box')
 ---
 ...
+fio = require("fio")
+---
+...
+fiber = require('fiber')
+---
+...
 space = box.schema.space.create('tweedledum')
 ---
 ...
@@ -40,8 +46,12 @@ errinj.info()
     state: false
   ERRINJ_WAL_IO:
     state: false
+  ERRINJ_VY_INDEX_FILE_RENAME:
+    state: false
   ERRINJ_TUPLE_ALLOC:
     state: false
+  ERRINJ_VY_RUN_FILE_RENAME:
+    state: false
   ERRINJ_VY_READ_PAGE:
     state: false
   ERRINJ_RELAY_REPORT_INTERVAL:
@@ -56,6 +66,8 @@ errinj.info()
     state: false
   ERRINJ_VY_LOG_FLUSH_DELAY:
     state: false
+  ERRINJ_VY_LOG_FILE_RENAME:
+    state: false
   ERRINJ_SNAP_COMMIT_DELAY:
     state: false
   ERRINJ_RELAY_FINAL_SLEEP:
@@ -693,6 +705,162 @@ errinj.set("ERRINJ_WAL_WRITE", false)
 space:drop()
 ---
 ...
+errinj.set("ERRINJ_VY_LOG_FILE_RENAME", true)
+---
+- ok
+...
+s = box.schema.space.create("vinyl_test", {engine='vinyl'})
+---
+...
+_ = s:create_index("primary")
+---
+...
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+---
+...
+#fio.glob(vinyl_vylog) > 0 -- true
+---
+- true
+...
+errinj.set("ERRINJ_SNAP_COMMIT_DELAY", true)
+---
+- ok
+...
+_ = fiber.create(function() box.snapshot() end)
+---
+...
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+---
+...
+while #fio.glob(memtx_snap) == 0 do fiber.sleep(0.001) end-- true
+---
+...
+#fio.glob(memtx_snap) > 0 --true
+---
+- true
+...
+test_run:cmd("restart server default")
+errinj = box.error.injection
+---
+...
+net_box = require('net.box')
+---
+...
+fio = require("fio")
+---
+...
+fiber = require('fiber')
+---
+...
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+---
+...
+#fio.glob(memtx_snap) == 0 -- true
+---
+- true
+...
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+---
+...
+#fio.glob(vinyl_vylog) == 0 --true
+---
+- true
+...
+s = box.space.vinyl_test
+---
+...
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+errinj.set("ERRINJ_VY_INDEX_FILE_RENAME", true)
+---
+- ok
+...
+-- insert big tuples in order to cause compaction without box.snapshot.
+for i = 1, 10000 do s:insert{i, string.rep('a', 10000)} end
+---
+...
+while #fio.glob(vinyl_index) == 0 do fiber.sleep(0.001) end
+---
+...
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+errinj.set("ERRINJ_VY_RUN_FILE_RENAME", true)
+---
+- ok
+...
+while #fio.glob(vinyl_run) == 0  do fiber.sleep(0.001) end
+---
+...
+#fio.glob(vinyl_run) > 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_index) > 0 -- true
+---
+- true
+...
+test_run:cmd("restart server default")
+errinj = box.error.injection
+---
+...
+net_box = require('net.box')
+---
+...
+fio = require("fio")
+---
+...
+fiber = require('fiber')
+---
+...
+s = box.space.vinyl_test
+---
+...
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+---
+...
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+---
+...
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+-- no inprogress files should be present in memtx(vinyl)_dir.
+#fio.glob(memtx_snap) == 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_vylog) == 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_index) == 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_run) == 0 -- true
+---
+- true
+...
+box.space.vinyl_test:drop()
+---
+...
+run_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/*/*")
+---
+...
+for _, file in ipairs(fio.glob(run_dir)) do fio.unlink(file) end
+---
+...
+vinyl_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/")
+---
+...
+for _, dir in ipairs(fio.glob(vinyl_dir)) do fio.rmtree(dir) end
+---
+...
 --test space:bsize() in case of memory error
 utils = dofile('utils.lua')
 ---
diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
index 3af1b74..756a8e7 100644
--- a/test/box/errinj.test.lua
+++ b/test/box/errinj.test.lua
@@ -1,5 +1,7 @@
 errinj = box.error.injection
 net_box = require('net.box')
+fio = require("fio")
+fiber = require('fiber')

 space = box.schema.space.create('tweedledum')
 index = space:create_index('primary', { type = 'hash' })
@@ -203,6 +205,67 @@ box.snapshot()
 errinj.set("ERRINJ_WAL_WRITE", false)
 space:drop()

+errinj.set("ERRINJ_VY_LOG_FILE_RENAME", true)
+s = box.schema.space.create("vinyl_test", {engine='vinyl'})
+_ = s:create_index("primary")
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+#fio.glob(vinyl_vylog) > 0 -- true
+
+errinj.set("ERRINJ_SNAP_COMMIT_DELAY", true)
+_ = fiber.create(function() box.snapshot() end)
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+while #fio.glob(memtx_snap) == 0 do fiber.sleep(0.001) end-- true
+#fio.glob(memtx_snap) > 0 --true
+
+test_run:cmd("restart server default")
+errinj = box.error.injection
+net_box = require('net.box')
+fio = require("fio")
+fiber = require('fiber')
+
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+#fio.glob(memtx_snap) == 0 -- true
+
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+#fio.glob(vinyl_vylog) == 0 --true
+
+s = box.space.vinyl_test
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+errinj.set("ERRINJ_VY_INDEX_FILE_RENAME", true)
+-- insert big tuples in order to cause compaction without box.snapshot.
+for i = 1, 10000 do s:insert{i, string.rep('a', 10000)} end
+
+while #fio.glob(vinyl_index) == 0 do fiber.sleep(0.001) end
+
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+errinj.set("ERRINJ_VY_RUN_FILE_RENAME", true)
+while #fio.glob(vinyl_run) == 0  do fiber.sleep(0.001) end
+#fio.glob(vinyl_run) > 0 -- true
+#fio.glob(vinyl_index) > 0 -- true
+
+test_run:cmd("restart server default")
+errinj = box.error.injection
+net_box = require('net.box')
+fio = require("fio")
+fiber = require('fiber')
+
+s = box.space.vinyl_test
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+
+-- no inprogress files should be present in memtx(vinyl)_dir.
+#fio.glob(memtx_snap) == 0 -- true
+#fio.glob(vinyl_vylog) == 0 -- true
+#fio.glob(vinyl_index) == 0 -- true
+#fio.glob(vinyl_run) == 0 -- true
+
+box.space.vinyl_test:drop()
+run_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/*/*")
+for _, file in ipairs(fio.glob(run_dir)) do fio.unlink(file) end
+vinyl_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/")
+for _, dir in ipairs(fio.glob(vinyl_dir)) do fio.rmtree(dir) end
 --test space:bsize() in case of memory error
 utils = dofile('utils.lua')
 s = box.schema.space.create('space_bsize')
--
2.7.4

Re: [PATCH v3] xlog: Remove inprogress files on start

[Vladimir Davydov]

On Tue, Jun 19, 2018 at 04:14:11PM +0300, Ilya Markov wrote:
> When tarantool crashes during writing to vylog, index, run or snapshot,
> inprogress files remain. But garbage collector doesn't take into account
> these files. So they remain until they are removed manually.
> 
> Fix this with adding deletion of such files to functions memtx_engine_end_recovery,
> vinyl_engine_end_recovery and vy_run_remove_files.
> 
> Add 3 errinj which simulate the crash before renaming inprogress files.
> Moved const string inprogress_suffix to xlog.h in order to reuse it in
> vinyl run and index files removal.
> 
> Closes #3406
> ---
> branch: https://github.com/tarantool/tarantool/tree/gh-3406-remove-inprogress-files
> issue: https://github.com/tarantool/tarantool/issues/3406
> 
> Changes in v3:
> - Delete abortion of end_recovery in case if inprgress file deletion fails.
>   Replace it with diag_log.
> - Move deletion of vylog.inprogress to vy_log_end_recovery().
> - Move error new injections before relevant calls of xlog_rename().
> - Change names and error messages of error injections.
> - Removed copy&paste in error injection code and add comment where it's impossible.
> - Changed signature of xdir_collect_inprogress function.
> - Removed call xdir_collect_inprogress from vy_run_remove_files,
>   replace it with implicit call of coio_unlink.
> - Replace coio_unlink in xdir_collect_inprogress with unlink.
> - Move const string inprogress_suffix to xlog.h.
> 
>  src/box/memtx_engine.c   |   2 +
>  src/box/vy_log.c         |   7 +-
>  src/box/vy_run.c         |  40 +++++++++--
>  src/box/xlog.c           |  35 +++++++++-
>  src/box/xlog.h           |  11 ++++
>  src/errinj.h             |   3 +
>  test/box/errinj.result   | 168 +++++++++++++++++++++++++++++++++++++++++++++++
>  test/box/errinj.test.lua |  63 ++++++++++++++++++
>  8 files changed, 321 insertions(+), 8 deletions(-)
> 
> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
> index 9a9aff5..1566d26 100644
> --- a/src/box/memtx_engine.c
> +++ b/src/box/memtx_engine.c
> @@ -303,6 +303,8 @@ memtx_engine_end_recovery(struct engine *engine)
>  		if (space_foreach(memtx_build_secondary_keys, memtx) != 0)
>  			return -1;
>  	}
> +	if (xdir_collect_inprogress(&memtx->snap_dir) < 0)
> +		diag_log();

You already log error in xdir_collect_inprogress(). No need in diag_log.
Better make xdir_collect_inprogress() return void.

>  	return 0;
>  }
> 
> diff --git a/src/box/vy_log.c b/src/box/vy_log.c
> index 51e3753..cd343b9 100644
> --- a/src/box/vy_log.c
> +++ b/src/box/vy_log.c
> @@ -852,6 +852,10 @@ vy_log_open(struct xlog *xlog)
>  	    xlog_write_row(xlog, &row) < 0)
>  		goto fail_close_xlog;
> 
> +	ERROR_INJECT(ERRINJ_VY_LOG_FILE_RENAME, {
> +		diag_set(ClientError, ER_INJECTION, "vinyl log file rename");
> +		return -1;
> +	});

You need to close xlog on error injection.

>  	if (xlog_rename(xlog) < 0)
>  		goto fail_close_xlog;
> 
> @@ -976,7 +980,8 @@ vy_log_end_recovery(void)
>  			return -1;
>  		}
>  	}
> -
> +	if (xdir_collect_inprogress(&vy_log.dir) < 0)
> +		diag_log();
>  	vy_log.recovery = NULL;
>  	return 0;
>  }
> diff --git a/src/box/vy_run.c b/src/box/vy_run.c
> index 980bc4d..96736ba 100644
> --- a/src/box/vy_run.c
> +++ b/src/box/vy_run.c
> @@ -2021,6 +2021,19 @@ vy_run_write_index(struct vy_run *run, const char *dirpath,
>  			goto fail;
>  	}
> 
> +	ERROR_INJECT(ERRINJ_VY_INDEX_FILE_RENAME, {
> +		/*
> +		 * Can't use here goto label fail,
> +		 * because we don't want to unlink file in errinj and can't move
> +		 * unlink before xlog_close call.
> +		 */

OK. I see. Remove this comment, please. It'd be OK if you had told me
that in reply to my review email.

> +		region_truncate(region, mem_used);
> +		xlog_tx_rollback(&index_xlog);
> +		xlog_close(&index_xlog, false);
> +		diag_set(ClientError, ER_INJECTION,
> +			 "vinyl index file rename");
> +		return -1;
> +	});
>  	if (xlog_tx_commit(&index_xlog) < 0 ||
>  	    xlog_flush(&index_xlog) < 0 ||
>  	    xlog_rename(&index_xlog) < 0)
> @@ -2263,6 +2276,12 @@ vy_run_writer_commit(struct vy_run_writer *writer)
>  	run->info.max_key = vy_key_dup(key);
>  	if (run->info.max_key == NULL)
>  		goto out;
> +	ERROR_INJECT(ERRINJ_VY_RUN_FILE_RENAME, {
> +		diag_set(ClientError, ER_INJECTION,
> +			 "vinyl run file rename");

> +		rc = -1;

Nit: this line is not needed, because rc is set to -1 in the beginning
of this function AFAICS.

> +		goto out;
> +	});
> 
>  	/* Sync data and link the file to the final name. */
>  	if (xlog_sync(&writer->data_xlog) < 0 ||
> @@ -2430,6 +2449,15 @@ close_err:
>  	return -1;
>  }
> 
> +static inline void
> +remove_file(const char *path, int *ret) {

Coding style.

> +	say_info("removing %s", path);
> +	if (coio_unlink(path) < 0 && errno != ENOENT) {
> +		say_syserror("error while removing %s", path);
> +		*ret = -1;
> +	}

I don't want to see "removing ..." message in the log if inprogress file
didn't exist (i.e. removing a committed run file on garbage collection).

> +}
> +
>  int
>  vy_run_remove_files(const char *dir, uint32_t space_id,
>  		    uint32_t iid, int64_t run_id)
> @@ -2439,14 +2467,14 @@ vy_run_remove_files(const char *dir, uint32_t space_id,
>  				(long long)run_id); return -1;});
>  	int ret = 0;
>  	char path[PATH_MAX];
> +	int total = 0;
>  	for (int type = 0; type < vy_file_MAX; type++) {
> -		vy_run_snprint_path(path, sizeof(path), dir,
> +		total = vy_run_snprint_path(path, sizeof(path), dir,
>  				    space_id, iid, run_id, type);
> -		say_info("removing %s", path);
> -		if (coio_unlink(path) < 0 && errno != ENOENT) {
> -			say_syserror("error while removing %s", path);
> -			ret = -1;
> -		}
> +		remove_file(path, &ret);
> +		snprintf(path + total, sizeof(path) - total, "%s",
> +			 inprogress_suffix);
> +		remove_file(path, &ret);

No, I don't really like it. I think adding inprogress files to
vy_file_type enum would be more concise and clean. I think it's
OK, because this enum is only used for formatting file names.
Something like this:

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 8a177446..db5c018a 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -3579,6 +3579,9 @@ vinyl_engine_backup(struct engine *engine, const struct vclock *vclock,
 				continue;
 			char path[PATH_MAX];
 			for (int type = 0; type < vy_file_MAX; type++) {
+				if (type == VY_FILE_RUN_INPROGRESS ||
+				    type == VY_FILE_INDEX_INPROGRESS)
+					continue;
 				vy_run_snprint_path(path, sizeof(path),
 						    env->path,
 						    lsm_info->space_id,
diff --git a/src/box/vy_run.c b/src/box/vy_run.c
index e2edbcaa..72e06244 100644
--- a/src/box/vy_run.c
+++ b/src/box/vy_run.c
@@ -66,8 +66,10 @@ static const uint64_t vy_run_info_key_map = (1 << VY_RUN_INFO_MIN_KEY) |
 #define XLOG_META_TYPE_INDEX "INDEX"
 
 const char *vy_file_suffix[] = {
-	"index",	/* VY_FILE_INDEX */
-	"run",		/* VY_FILE_RUN */
+	"index",		/* VY_FILE_INDEX */
+	"index.inprogress",	/* VY_FILE_INDEX_INPROGRESS */
+	"run",			/* VY_FILE_RUN */
+	"run.inprogress",	/* VY_FILE_RUN_INPROGRESS */
 };
 
 /**
diff --git a/src/box/vy_run.h b/src/box/vy_run.h
index 7bafffec..5030886c 100644
--- a/src/box/vy_run.h
+++ b/src/box/vy_run.h
@@ -377,7 +377,9 @@ vy_run_rebuild_index(struct vy_run *run, const char *dir,
 
 enum vy_file_type {
 	VY_FILE_INDEX,
+	VY_FILE_INDEX_INPROGRESS,
 	VY_FILE_RUN,
+	VY_FILE_RUN_INPROGRESS,
 	vy_file_MAX,
 };
 

>  	}
>  	return ret;
>  }
> diff --git a/src/box/xlog.c b/src/box/xlog.c
> index 824ad11..7bd618e 100644
> --- a/src/box/xlog.c
> +++ b/src/box/xlog.c
> @@ -58,7 +58,6 @@ typedef uint32_t log_magic_t;
>  static const log_magic_t row_marker = mp_bswap_u32(0xd5ba0bab); /* host byte order */
>  static const log_magic_t zrow_marker = mp_bswap_u32(0xd5ba0bba); /* host byte order */
>  static const log_magic_t eof_marker = mp_bswap_u32(0xd510aded); /* host byte order */
> -static const char inprogress_suffix[] = ".inprogress";
> 
>  enum {
>  	/**
> @@ -621,6 +620,40 @@ xdir_collect_garbage(struct xdir *dir, int64_t signature, bool use_coio)
>  	return 0;
>  }
> 
> +int
> +xdir_collect_inprogress(struct xdir *xdir)
> +{
> +	const char *dirname = xdir->dirname;
> +	DIR *dh = opendir(dirname);
> +	if (dh == NULL) {
> +		if (errno == ENOENT)
> +			return 0;
> +		diag_set(SystemError, "error reading directory '%s'",
> +			 dirname);

Since you don't use the return code anyway, replace the diag with
say_syserror and make the function return void.

> +		return -1;
> +	}
> +
> +	struct dirent *dent;
> +	char path[PATH_MAX];
> +	int total = snprintf(path, sizeof(path), "%s/", dirname);
> +
> +	while ((dent = readdir(dh)) != NULL) {
> +		char *ext = strrchr(dent->d_name, '.');
> +		if (ext == NULL || strcmp(ext, inprogress_suffix) != 0)
> +			continue;
> +		strcpy(path + total, dent->d_name);

This may write beyond path if strlen(path) + strlen(d_name) >
sizeof(path).

> +		say_info("removing %s", path);
> +		int rc = unlink(path);
> +		if (rc < 0) {
> +			say_syserror("error while removing %s", path);
> +			diag_set(SystemError, "failed to unlink file '%s'",
> +				 path);
> +			return -1;
> +		}
> +	}
> +	return 0;
> +}

> diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
> index 3af1b74..756a8e7 100644
> --- a/test/box/errinj.test.lua
> +++ b/test/box/errinj.test.lua
> @@ -1,5 +1,7 @@
>  errinj = box.error.injection
>  net_box = require('net.box')
> +fio = require("fio")
> +fiber = require('fiber')
> 
>  space = box.schema.space.create('tweedledum')
>  index = space:create_index('primary', { type = 'hash' })
> @@ -203,6 +205,67 @@ box.snapshot()
>  errinj.set("ERRINJ_WAL_WRITE", false)
>  space:drop()
> 
> +errinj.set("ERRINJ_VY_LOG_FILE_RENAME", true)
> +s = box.schema.space.create("vinyl_test", {engine='vinyl'})
> +_ = s:create_index("primary")
> +vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
> +#fio.glob(vinyl_vylog) > 0 -- true
> +
> +errinj.set("ERRINJ_SNAP_COMMIT_DELAY", true)
> +_ = fiber.create(function() box.snapshot() end)
> +memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
> +while #fio.glob(memtx_snap) == 0 do fiber.sleep(0.001) end-- true
> +#fio.glob(memtx_snap) > 0 --true
> +
> +test_run:cmd("restart server default")
> +errinj = box.error.injection
> +net_box = require('net.box')
> +fio = require("fio")
> +fiber = require('fiber')
> +
> +memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
> +#fio.glob(memtx_snap) == 0 -- true
> +
> +vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
> +#fio.glob(vinyl_vylog) == 0 --true
> +
> +s = box.space.vinyl_test
> +vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
> +errinj.set("ERRINJ_VY_INDEX_FILE_RENAME", true)
> +-- insert big tuples in order to cause compaction without box.snapshot.
> +for i = 1, 10000 do s:insert{i, string.rep('a', 10000)} end

First, this test doesn't pass on travis.

Second, it's going to take too long, because you insert so many tuples.
Do you really need it. Why can't you use box.snapshot()?

[PATCH 0/2 v4] xdir: Remove inprogress files on start

[Ilya Markov]

branch: https://github.com/tarantool/tarantool/tree/gh-3406-remove-inprogress-files
issue: https://github.com/tarantool/tarantool/issues/3406

Changes in v4:
- Make xdir_collect_inprogress return void.
- Add 2 vy_file_types for inprogress files.
- Change error injections using labels.
- Change test with box.snapshot instead of big tuples inserting.
- Change log message in xdir_collect_garbage and vy_run_remove_files.
- Get rid of remove_file function in vy_run.c.
- Remove unnecessary lines.


Ilya Markov (2):
  xdir: Change log messages in gc functions
  xlog: Remove inprogress files on start

 src/box/memtx_engine.c   |   1 +
 src/box/vinyl.c          |   3 +
 src/box/vy_log.c         |   7 +-
 src/box/vy_run.c         |  31 ++++++---
 src/box/vy_run.h         |   2 +
 src/box/xlog.c           |  47 ++++++++++---
 src/box/xlog.h           |  11 +++
 src/errinj.h             |   3 +
 test/box/errinj.result   | 171 +++++++++++++++++++++++++++++++++++++++++++++++
 test/box/errinj.test.lua |  64 ++++++++++++++++++
 10 files changed, 323 insertions(+), 17 deletions(-)

--
2.7.4

[PATCH 1/2 v4] xdir: Change log messages in gc functions

[Ilya Markov]

In order to log only about files that are actually removed
change log messages from "removing <name of file>" to "removed <name of
file>" in vy_run_remove_files and xdir_collect_garbage functions.

Prerequisite #3406
---
 src/box/vy_run.c | 12 +++++++-----
 src/box/xlog.c   | 18 +++++++++++-------
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/src/box/vy_run.c b/src/box/vy_run.c
index 980bc4d..2c5c0c9 100644
--- a/src/box/vy_run.c
+++ b/src/box/vy_run.c
@@ -2442,11 +2442,13 @@ vy_run_remove_files(const char *dir, uint32_t space_id,
 	for (int type = 0; type < vy_file_MAX; type++) {
 		vy_run_snprint_path(path, sizeof(path), dir,
 				    space_id, iid, run_id, type);
-		say_info("removing %s", path);
-		if (coio_unlink(path) < 0 && errno != ENOENT) {
-			say_syserror("error while removing %s", path);
-			ret = -1;
-		}
+		if (coio_unlink(path) < 0) {
+			if (errno != ENOENT) {
+				say_syserror("error while removing %s", path);
+				ret = -1;
+			}
+		} else
+			say_info("removed %s", path);
 	}
 	return ret;
 }
diff --git a/src/box/xlog.c b/src/box/xlog.c
index 824ad11..04f8587 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -603,18 +603,22 @@ xdir_collect_garbage(struct xdir *dir, int64_t signature, bool use_coio)
 	       vclock_sum(vclock) < signature) {
 		char *filename = xdir_format_filename(dir, vclock_sum(vclock),
 						      NONE);
-		say_info("removing %s", filename);
 		int rc;
 		if (use_coio)
 			rc = coio_unlink(filename);
 		else
 			rc = unlink(filename);
-		if (rc < 0 && errno != ENOENT) {
-			say_syserror("error while removing %s", filename);
-			diag_set(SystemError, "failed to unlink file '%s'",
-				 filename);
-			return -1;
-		}
+		if (rc < 0) {
+			if (errno != ENOENT) {
+				say_syserror("error while removing %s",
+					     filename);
+				diag_set(SystemError,
+					 "failed to unlink file '%s'",
+					 filename);
+				return -1;
+			}
+		} else
+			say_info("removed %s", filename);
 		vclockset_remove(&dir->index, vclock);
 		free(vclock);
 	}
--
2.7.4

[PATCH 2/2 v4] xlog: Remove inprogress files on start

[Ilya Markov]

When tarantool crashes during writing to vylog, index, run or snapshot,
inprogress files remain. But garbage collector doesn't take into account
these files. So they remain until they are removed manually.

Fix this with adding deletion of such files to functions memtx_engine_end_recovery,
vinyl_engine_end_recovery and vy_run_remove_files.

- Add 3 errinj which simulate the crash before renaming inprogress files.
- Moved const string inprogress_suffix to xlog.h and replace it with
  #defiled string in order to reuse it in vinyl run and index files removal.
- Add 2 new vy_file_type in order to support formatting of inprogress
  files names.

Closes #3406
---
 src/box/memtx_engine.c   |   1 +
 src/box/vinyl.c          |   3 +
 src/box/vy_log.c         |   7 +-
 src/box/vy_run.c         |  19 +++++-
 src/box/vy_run.h         |   2 +
 src/box/xlog.c           |  29 +++++++-
 src/box/xlog.h           |  11 +++
 src/errinj.h             |   3 +
 test/box/errinj.result   | 171 +++++++++++++++++++++++++++++++++++++++++++++++
 test/box/errinj.test.lua |  64 ++++++++++++++++++
 10 files changed, 305 insertions(+), 5 deletions(-)

diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index 9a9aff5..d211bcd 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -303,6 +303,7 @@ memtx_engine_end_recovery(struct engine *engine)
 		if (space_foreach(memtx_build_secondary_keys, memtx) != 0)
 			return -1;
 	}
+	xdir_collect_inprogress(&memtx->snap_dir);
 	return 0;
 }

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 552d42b..c82b7cd 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -3481,6 +3481,9 @@ vy_backup_cb(const struct vy_log_record *record, void *cb_arg)

 	char path[PATH_MAX];
 	for (int type = 0; type < vy_file_MAX; type++) {
+		if (type == VY_FILE_RUN_INPROGRESS ||
+			type == VY_FILE_INDEX_INPROGRESS)
+			continue;
 		vy_run_snprint_path(path, sizeof(path), arg->env->path,
 				    arg->space_id, arg->index_id,
 				    record->run_id, type);
diff --git a/src/box/vy_log.c b/src/box/vy_log.c
index 51e3753..7a7993d 100644
--- a/src/box/vy_log.c
+++ b/src/box/vy_log.c
@@ -852,6 +852,10 @@ vy_log_open(struct xlog *xlog)
 	    xlog_write_row(xlog, &row) < 0)
 		goto fail_close_xlog;

+	ERROR_INJECT(ERRINJ_VY_LOG_FILE_RENAME, {
+		diag_set(ClientError, ER_INJECTION, "vinyl log file rename");
+		goto fail_errinj;
+	});
 	if (xlog_rename(xlog) < 0)
 		goto fail_close_xlog;

@@ -860,6 +864,7 @@ vy_log_open(struct xlog *xlog)
 fail_close_xlog:
 	if (unlink(xlog->filename) < 0)
 		say_syserror("failed to delete file '%s'", xlog->filename);
+fail_errinj:
 	xlog_close(xlog, false);
 fail:
 	return -1;
@@ -976,7 +981,7 @@ vy_log_end_recovery(void)
 			return -1;
 		}
 	}
-
+	xdir_collect_inprogress(&vy_log.dir);
 	vy_log.recovery = NULL;
 	return 0;
 }
diff --git a/src/box/vy_run.c b/src/box/vy_run.c
index 2c5c0c9..0626e28 100644
--- a/src/box/vy_run.c
+++ b/src/box/vy_run.c
@@ -66,8 +66,10 @@ enum { VY_BLOOM_VERSION = 0 };
 #define XLOG_META_TYPE_INDEX "INDEX"

 const char *vy_file_suffix[] = {
-	"index",	/* VY_FILE_INDEX */
-	"run",		/* VY_FILE_RUN */
+	"index",			/* VY_FILE_INDEX */
+	"index" inprogress_suffix, 	/* VY_FILE_INDEX_INPROGRESS */
+	"run",				/* VY_FILE_RUN */
+	"run" inprogress_suffix, 	/* VY_FILE_RUN_INPROGRESS */
 };

 /**
@@ -2021,6 +2023,11 @@ vy_run_write_index(struct vy_run *run, const char *dirpath,
 			goto fail;
 	}

+	ERROR_INJECT(ERRINJ_VY_INDEX_FILE_RENAME, {
+		diag_set(ClientError, ER_INJECTION,
+			 "vinyl index file rename");
+		goto fail_errinj;
+	});
 	if (xlog_tx_commit(&index_xlog) < 0 ||
 	    xlog_flush(&index_xlog) < 0 ||
 	    xlog_rename(&index_xlog) < 0)
@@ -2029,10 +2036,11 @@ vy_run_write_index(struct vy_run *run, const char *dirpath,
 	region_truncate(region, mem_used);
 	return 0;
 fail:
+	unlink(path);
+fail_errinj:
 	region_truncate(region, mem_used);
 	xlog_tx_rollback(&index_xlog);
 	xlog_close(&index_xlog, false);
-	unlink(path);
 	return -1;
 }

@@ -2263,6 +2271,11 @@ vy_run_writer_commit(struct vy_run_writer *writer)
 	run->info.max_key = vy_key_dup(key);
 	if (run->info.max_key == NULL)
 		goto out;
+	ERROR_INJECT(ERRINJ_VY_RUN_FILE_RENAME, {
+		diag_set(ClientError, ER_INJECTION,
+			 "vinyl run file rename");
+		goto out;
+	});

 	/* Sync data and link the file to the final name. */
 	if (xlog_sync(&writer->data_xlog) < 0 ||
diff --git a/src/box/vy_run.h b/src/box/vy_run.h
index b3e3d1c..6afef2a 100644
--- a/src/box/vy_run.h
+++ b/src/box/vy_run.h
@@ -378,7 +378,9 @@ vy_run_rebuild_index(struct vy_run *run, const char *dir,

 enum vy_file_type {
 	VY_FILE_INDEX,
+	VY_FILE_INDEX_INPROGRESS,
 	VY_FILE_RUN,
+	VY_FILE_RUN_INPROGRESS,
 	vy_file_MAX,
 };

diff --git a/src/box/xlog.c b/src/box/xlog.c
index 04f8587..142cdcf 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -58,7 +58,6 @@ typedef uint32_t log_magic_t;
 static const log_magic_t row_marker = mp_bswap_u32(0xd5ba0bab); /* host byte order */
 static const log_magic_t zrow_marker = mp_bswap_u32(0xd5ba0bba); /* host byte order */
 static const log_magic_t eof_marker = mp_bswap_u32(0xd510aded); /* host byte order */
-static const char inprogress_suffix[] = ".inprogress";

 enum {
 	/**
@@ -625,6 +624,34 @@ xdir_collect_garbage(struct xdir *dir, int64_t signature, bool use_coio)
 	return 0;
 }

+void
+xdir_collect_inprogress(struct xdir *xdir)
+{
+	const char *dirname = xdir->dirname;
+	DIR *dh = opendir(dirname);
+	if (dh == NULL) {
+		if (errno != ENOENT)
+			say_syserror("error reading directory '%s'", dirname);
+		return;
+	}
+
+	struct dirent *dent;
+	char path[PATH_MAX];
+	int total = snprintf(path, sizeof(path), "%s/", dirname);
+
+	while ((dent = readdir(dh)) != NULL) {
+		char *ext = strrchr(dent->d_name, '.');
+		if (ext == NULL || strcmp(ext, inprogress_suffix) != 0)
+			continue;
+		strncpy(path + total, dent->d_name, sizeof(path) - total);
+		if (unlink(path) < 0) {
+			say_syserror("error while removing %s", path);
+			return;
+		} else
+			say_info("removed %s", path);
+	}
+}
+
 /* }}} */


diff --git a/src/box/xlog.h b/src/box/xlog.h
index 973910d..3b03588 100644
--- a/src/box/xlog.h
+++ b/src/box/xlog.h
@@ -70,6 +70,11 @@ enum xdir_type {
 enum log_suffix { NONE, INPROGRESS };

 /**
+ * Suffix added to path of inprogress files.
+ */
+#define inprogress_suffix ".inprogress"
+
+/**
  * A handle for a data directory with write ahead logs, snapshots,
  * vylogs.
  * Can be used to find the last log in the directory, scan
@@ -181,6 +186,12 @@ int
 xdir_collect_garbage(struct xdir *dir, int64_t signature, bool use_coio);

 /**
+ * Remove .inprogress files in specified directory.
+ */
+void
+xdir_collect_inprogress(struct xdir *xdir);
+
+/**
  * Return LSN and vclock (unless @vclock is NULL) of the newest
  * file in a directory or -1 if the directory is empty.
  */
diff --git a/src/errinj.h b/src/errinj.h
index 78514ac..7141af9 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -111,6 +111,9 @@ struct errinj {
 	_(ERRINJ_IPROTO_TX_DELAY, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_LOG_ROTATE, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_SNAP_COMMIT_DELAY, ERRINJ_BOOL, {.bparam = 0}) \
+	_(ERRINJ_VY_LOG_FILE_RENAME, ERRINJ_BOOL, {.bparam = false}) \
+	_(ERRINJ_VY_INDEX_FILE_RENAME, ERRINJ_BOOL, {.bparam = false}) \
+	_(ERRINJ_VY_RUN_FILE_RENAME, ERRINJ_BOOL, {.bparam = false}) \

 ENUM0(errinj_id, ERRINJ_LIST);
 extern struct errinj errinjs[];
diff --git a/test/box/errinj.result b/test/box/errinj.result
index 6ced172..34a5c69 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -4,6 +4,12 @@ errinj = box.error.injection
 net_box = require('net.box')
 ---
 ...
+fio = require("fio")
+---
+...
+fiber = require('fiber')
+---
+...
 space = box.schema.space.create('tweedledum')
 ---
 ...
@@ -40,8 +46,12 @@ errinj.info()
     state: false
   ERRINJ_WAL_IO:
     state: false
+  ERRINJ_VY_INDEX_FILE_RENAME:
+    state: false
   ERRINJ_TUPLE_ALLOC:
     state: false
+  ERRINJ_VY_RUN_FILE_RENAME:
+    state: false
   ERRINJ_VY_READ_PAGE:
     state: false
   ERRINJ_RELAY_REPORT_INTERVAL:
@@ -56,6 +66,8 @@ errinj.info()
     state: false
   ERRINJ_VY_LOG_FLUSH_DELAY:
     state: false
+  ERRINJ_VY_LOG_FILE_RENAME:
+    state: false
   ERRINJ_SNAP_COMMIT_DELAY:
     state: false
   ERRINJ_RELAY_FINAL_SLEEP:
@@ -693,6 +705,165 @@ errinj.set("ERRINJ_WAL_WRITE", false)
 space:drop()
 ---
 ...
+errinj.set("ERRINJ_VY_LOG_FILE_RENAME", true)
+---
+- ok
+...
+s = box.schema.space.create("vinyl_test", {engine='vinyl'})
+---
+...
+_ = s:create_index("primary")
+---
+...
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+---
+...
+#fio.glob(vinyl_vylog) > 0 -- true
+---
+- true
+...
+errinj.set("ERRINJ_SNAP_COMMIT_DELAY", true)
+---
+- ok
+...
+_ = fiber.create(function() box.snapshot() end)
+---
+...
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+---
+...
+while #fio.glob(memtx_snap) == 0 do fiber.sleep(0.001) end-- true
+---
+...
+#fio.glob(memtx_snap) > 0 --true
+---
+- true
+...
+test_run:cmd("restart server default")
+errinj = box.error.injection
+---
+...
+net_box = require('net.box')
+---
+...
+fio = require("fio")
+---
+...
+fiber = require('fiber')
+---
+...
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+---
+...
+#fio.glob(memtx_snap) == 0 -- true
+---
+- true
+...
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+---
+...
+#fio.glob(vinyl_vylog) == 0 --true
+---
+- true
+...
+s = box.space.vinyl_test
+---
+...
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+errinj.set("ERRINJ_VY_INDEX_FILE_RENAME", true)
+---
+- ok
+...
+for i = 1, 10 do s:insert{i} end
+---
+...
+box.snapshot()
+---
+- error: Error injection 'vinyl index file rename'
+...
+while #fio.glob(vinyl_index) == 0 do fiber.sleep(0.001) end
+---
+...
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+errinj.set("ERRINJ_VY_RUN_FILE_RENAME", true)
+---
+- ok
+...
+while #fio.glob(vinyl_run) == 0  do fiber.sleep(0.001) end
+---
+...
+#fio.glob(vinyl_run) > 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_index) > 0 -- true
+---
+- true
+...
+test_run:cmd("restart server default")
+errinj = box.error.injection
+---
+...
+net_box = require('net.box')
+---
+...
+fio = require("fio")
+---
+...
+fiber = require('fiber')
+---
+...
+s = box.space.vinyl_test
+---
+...
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+---
+...
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+---
+...
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+-- no inprogress files should be present in memtx(vinyl)_dir.
+#fio.glob(memtx_snap) == 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_vylog) == 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_index) == 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_run) == 0 -- true
+---
+- true
+...
+box.space.vinyl_test:drop()
+---
+...
+run_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/*/*")
+---
+...
+for _, file in ipairs(fio.glob(run_dir)) do fio.unlink(file) end
+---
+...
+vinyl_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/")
+---
+...
+for _, dir in ipairs(fio.glob(vinyl_dir)) do fio.rmtree(dir) end
+---
+...
 --test space:bsize() in case of memory error
 utils = dofile('utils.lua')
 ---
diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
index 3af1b74..647fb0a 100644
--- a/test/box/errinj.test.lua
+++ b/test/box/errinj.test.lua
@@ -1,5 +1,7 @@
 errinj = box.error.injection
 net_box = require('net.box')
+fio = require("fio")
+fiber = require('fiber')

 space = box.schema.space.create('tweedledum')
 index = space:create_index('primary', { type = 'hash' })
@@ -203,6 +205,68 @@ box.snapshot()
 errinj.set("ERRINJ_WAL_WRITE", false)
 space:drop()

+errinj.set("ERRINJ_VY_LOG_FILE_RENAME", true)
+s = box.schema.space.create("vinyl_test", {engine='vinyl'})
+_ = s:create_index("primary")
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+#fio.glob(vinyl_vylog) > 0 -- true
+
+errinj.set("ERRINJ_SNAP_COMMIT_DELAY", true)
+_ = fiber.create(function() box.snapshot() end)
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+while #fio.glob(memtx_snap) == 0 do fiber.sleep(0.001) end-- true
+#fio.glob(memtx_snap) > 0 --true
+
+test_run:cmd("restart server default")
+errinj = box.error.injection
+net_box = require('net.box')
+fio = require("fio")
+fiber = require('fiber')
+
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+#fio.glob(memtx_snap) == 0 -- true
+
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+#fio.glob(vinyl_vylog) == 0 --true
+
+s = box.space.vinyl_test
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+errinj.set("ERRINJ_VY_INDEX_FILE_RENAME", true)
+
+for i = 1, 10 do s:insert{i} end
+box.snapshot()
+
+while #fio.glob(vinyl_index) == 0 do fiber.sleep(0.001) end
+
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+errinj.set("ERRINJ_VY_RUN_FILE_RENAME", true)
+while #fio.glob(vinyl_run) == 0  do fiber.sleep(0.001) end
+#fio.glob(vinyl_run) > 0 -- true
+#fio.glob(vinyl_index) > 0 -- true
+
+test_run:cmd("restart server default")
+errinj = box.error.injection
+net_box = require('net.box')
+fio = require("fio")
+fiber = require('fiber')
+
+s = box.space.vinyl_test
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+
+-- no inprogress files should be present in memtx(vinyl)_dir.
+#fio.glob(memtx_snap) == 0 -- true
+#fio.glob(vinyl_vylog) == 0 -- true
+#fio.glob(vinyl_index) == 0 -- true
+#fio.glob(vinyl_run) == 0 -- true
+
+box.space.vinyl_test:drop()
+run_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/*/*")
+for _, file in ipairs(fio.glob(run_dir)) do fio.unlink(file) end
+vinyl_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/")
+for _, dir in ipairs(fio.glob(vinyl_dir)) do fio.rmtree(dir) end
 --test space:bsize() in case of memory error
 utils = dofile('utils.lua')
 s = box.schema.space.create('space_bsize')
--
2.7.4

[PATCH v5 0/2] Remove inprogress files on start

[Ilya Markov]

branch: https://github.com/tarantool/tarantool/tree/gh-3406-remove-inprogress-files
issue: https://github.com/tarantool/tarantool/issues/3406

Changes in v5:
- Fix one thread test failing on travis.

Ilya Markov (2):
  xdir: Change log messages in gc functions
  xlog: Remove inprogress files on start

 src/box/memtx_engine.c   |   1 +
 src/box/vinyl.c          |   3 +
 src/box/vy_log.c         |   7 +-
 src/box/vy_run.c         |  31 ++++++--
 src/box/vy_run.h         |   2 +
 src/box/xlog.c           |  47 ++++++++++--
 src/box/xlog.h           |  11 +++
 src/errinj.h             |   3 +
 test/box/errinj.result   | 185 +++++++++++++++++++++++++++++++++++++++++++++++
 test/box/errinj.test.lua |  72 +++++++++++++++++-
 10 files changed, 344 insertions(+), 18 deletions(-)

--
2.7.4

[PATCH v5 1/2] xdir: Change log messages in gc functions

[Ilya Markov]

In order to log only about files that are actually removed
change log messages from "removing <name of file>" to "removed <name of
file>" in vy_run_remove_files and xdir_collect_garbage functions.

Prerequisite #3406
---
 src/box/vy_run.c | 12 +++++++-----
 src/box/xlog.c   | 18 +++++++++++-------
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/src/box/vy_run.c b/src/box/vy_run.c
index 980bc4d..2c5c0c9 100644
--- a/src/box/vy_run.c
+++ b/src/box/vy_run.c
@@ -2442,11 +2442,13 @@ vy_run_remove_files(const char *dir, uint32_t space_id,
 	for (int type = 0; type < vy_file_MAX; type++) {
 		vy_run_snprint_path(path, sizeof(path), dir,
 				    space_id, iid, run_id, type);
-		say_info("removing %s", path);
-		if (coio_unlink(path) < 0 && errno != ENOENT) {
-			say_syserror("error while removing %s", path);
-			ret = -1;
-		}
+		if (coio_unlink(path) < 0) {
+			if (errno != ENOENT) {
+				say_syserror("error while removing %s", path);
+				ret = -1;
+			}
+		} else
+			say_info("removed %s", path);
 	}
 	return ret;
 }
diff --git a/src/box/xlog.c b/src/box/xlog.c
index 824ad11..04f8587 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -603,18 +603,22 @@ xdir_collect_garbage(struct xdir *dir, int64_t signature, bool use_coio)
 	       vclock_sum(vclock) < signature) {
 		char *filename = xdir_format_filename(dir, vclock_sum(vclock),
 						      NONE);
-		say_info("removing %s", filename);
 		int rc;
 		if (use_coio)
 			rc = coio_unlink(filename);
 		else
 			rc = unlink(filename);
-		if (rc < 0 && errno != ENOENT) {
-			say_syserror("error while removing %s", filename);
-			diag_set(SystemError, "failed to unlink file '%s'",
-				 filename);
-			return -1;
-		}
+		if (rc < 0) {
+			if (errno != ENOENT) {
+				say_syserror("error while removing %s",
+					     filename);
+				diag_set(SystemError,
+					 "failed to unlink file '%s'",
+					 filename);
+				return -1;
+			}
+		} else
+			say_info("removed %s", filename);
 		vclockset_remove(&dir->index, vclock);
 		free(vclock);
 	}
--
2.7.4

Re: [PATCH v5 1/2] xdir: Change log messages in gc functions

[Vladimir Davydov]

On Wed, Jun 20, 2018 at 07:25:55PM +0300, Ilya Markov wrote:
> In order to log only about files that are actually removed
> change log messages from "removing <name of file>" to "removed <name of
> file>" in vy_run_remove_files and xdir_collect_garbage functions.
> 
> Prerequisite #3406
> ---
>  src/box/vy_run.c | 12 +++++++-----
>  src/box/xlog.c   | 18 +++++++++++-------
>  2 files changed, 18 insertions(+), 12 deletions(-)

Pushed.

[PATCH v5 2/2] xlog: Remove inprogress files on start

[Ilya Markov]

When tarantool crashes during writing to vylog, index, run or snapshot,
inprogress files remain. But garbage collector doesn't take into account
these files. So they remain until they are removed manually.

Fix this with adding deletion of such files to functions memtx_engine_end_recovery,
vinyl_engine_end_recovery and vy_run_remove_files.

- Add 3 errinj which simulate the crash before renaming inprogress files.
- Moved const string inprogress_suffix to xlog.h and replace it with
  #defiled string in order to reuse it in vinyl run and index files removal.
- Add 2 new vy_file_type in order to support formatting of inprogress
  files names.

Closes #3406
---
 src/box/memtx_engine.c   |   1 +
 src/box/vinyl.c          |   3 +
 src/box/vy_log.c         |   7 +-
 src/box/vy_run.c         |  19 ++++-
 src/box/vy_run.h         |   2 +
 src/box/xlog.c           |  29 +++++++-
 src/box/xlog.h           |  11 +++
 src/errinj.h             |   3 +
 test/box/errinj.result   | 185 +++++++++++++++++++++++++++++++++++++++++++++++
 test/box/errinj.test.lua |  72 +++++++++++++++++-
 10 files changed, 326 insertions(+), 6 deletions(-)

diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index 9a9aff5..d211bcd 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -303,6 +303,7 @@ memtx_engine_end_recovery(struct engine *engine)
 		if (space_foreach(memtx_build_secondary_keys, memtx) != 0)
 			return -1;
 	}
+	xdir_collect_inprogress(&memtx->snap_dir);
 	return 0;
 }
 
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 552d42b..c82b7cd 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -3481,6 +3481,9 @@ vy_backup_cb(const struct vy_log_record *record, void *cb_arg)
 
 	char path[PATH_MAX];
 	for (int type = 0; type < vy_file_MAX; type++) {
+		if (type == VY_FILE_RUN_INPROGRESS ||
+			type == VY_FILE_INDEX_INPROGRESS)
+			continue;
 		vy_run_snprint_path(path, sizeof(path), arg->env->path,
 				    arg->space_id, arg->index_id,
 				    record->run_id, type);
diff --git a/src/box/vy_log.c b/src/box/vy_log.c
index 51e3753..7a7993d 100644
--- a/src/box/vy_log.c
+++ b/src/box/vy_log.c
@@ -852,6 +852,10 @@ vy_log_open(struct xlog *xlog)
 	    xlog_write_row(xlog, &row) < 0)
 		goto fail_close_xlog;
 
+	ERROR_INJECT(ERRINJ_VY_LOG_FILE_RENAME, {
+		diag_set(ClientError, ER_INJECTION, "vinyl log file rename");
+		goto fail_errinj;
+	});
 	if (xlog_rename(xlog) < 0)
 		goto fail_close_xlog;
 
@@ -860,6 +864,7 @@ vy_log_open(struct xlog *xlog)
 fail_close_xlog:
 	if (unlink(xlog->filename) < 0)
 		say_syserror("failed to delete file '%s'", xlog->filename);
+fail_errinj:
 	xlog_close(xlog, false);
 fail:
 	return -1;
@@ -976,7 +981,7 @@ vy_log_end_recovery(void)
 			return -1;
 		}
 	}
-
+	xdir_collect_inprogress(&vy_log.dir);
 	vy_log.recovery = NULL;
 	return 0;
 }
diff --git a/src/box/vy_run.c b/src/box/vy_run.c
index 2c5c0c9..0626e28 100644
--- a/src/box/vy_run.c
+++ b/src/box/vy_run.c
@@ -66,8 +66,10 @@ enum { VY_BLOOM_VERSION = 0 };
 #define XLOG_META_TYPE_INDEX "INDEX"
 
 const char *vy_file_suffix[] = {
-	"index",	/* VY_FILE_INDEX */
-	"run",		/* VY_FILE_RUN */
+	"index",			/* VY_FILE_INDEX */
+	"index" inprogress_suffix, 	/* VY_FILE_INDEX_INPROGRESS */
+	"run",				/* VY_FILE_RUN */
+	"run" inprogress_suffix, 	/* VY_FILE_RUN_INPROGRESS */
 };
 
 /**
@@ -2021,6 +2023,11 @@ vy_run_write_index(struct vy_run *run, const char *dirpath,
 			goto fail;
 	}
 
+	ERROR_INJECT(ERRINJ_VY_INDEX_FILE_RENAME, {
+		diag_set(ClientError, ER_INJECTION,
+			 "vinyl index file rename");
+		goto fail_errinj;
+	});
 	if (xlog_tx_commit(&index_xlog) < 0 ||
 	    xlog_flush(&index_xlog) < 0 ||
 	    xlog_rename(&index_xlog) < 0)
@@ -2029,10 +2036,11 @@ vy_run_write_index(struct vy_run *run, const char *dirpath,
 	region_truncate(region, mem_used);
 	return 0;
 fail:
+	unlink(path);
+fail_errinj:
 	region_truncate(region, mem_used);
 	xlog_tx_rollback(&index_xlog);
 	xlog_close(&index_xlog, false);
-	unlink(path);
 	return -1;
 }
 
@@ -2263,6 +2271,11 @@ vy_run_writer_commit(struct vy_run_writer *writer)
 	run->info.max_key = vy_key_dup(key);
 	if (run->info.max_key == NULL)
 		goto out;
+	ERROR_INJECT(ERRINJ_VY_RUN_FILE_RENAME, {
+		diag_set(ClientError, ER_INJECTION,
+			 "vinyl run file rename");
+		goto out;
+	});
 
 	/* Sync data and link the file to the final name. */
 	if (xlog_sync(&writer->data_xlog) < 0 ||
diff --git a/src/box/vy_run.h b/src/box/vy_run.h
index b3e3d1c..6afef2a 100644
--- a/src/box/vy_run.h
+++ b/src/box/vy_run.h
@@ -378,7 +378,9 @@ vy_run_rebuild_index(struct vy_run *run, const char *dir,
 
 enum vy_file_type {
 	VY_FILE_INDEX,
+	VY_FILE_INDEX_INPROGRESS,
 	VY_FILE_RUN,
+	VY_FILE_RUN_INPROGRESS,
 	vy_file_MAX,
 };
 
diff --git a/src/box/xlog.c b/src/box/xlog.c
index 04f8587..142cdcf 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -58,7 +58,6 @@ typedef uint32_t log_magic_t;
 static const log_magic_t row_marker = mp_bswap_u32(0xd5ba0bab); /* host byte order */
 static const log_magic_t zrow_marker = mp_bswap_u32(0xd5ba0bba); /* host byte order */
 static const log_magic_t eof_marker = mp_bswap_u32(0xd510aded); /* host byte order */
-static const char inprogress_suffix[] = ".inprogress";
 
 enum {
 	/**
@@ -625,6 +624,34 @@ xdir_collect_garbage(struct xdir *dir, int64_t signature, bool use_coio)
 	return 0;
 }
 
+void
+xdir_collect_inprogress(struct xdir *xdir)
+{
+	const char *dirname = xdir->dirname;
+	DIR *dh = opendir(dirname);
+	if (dh == NULL) {
+		if (errno != ENOENT)
+			say_syserror("error reading directory '%s'", dirname);
+		return;
+	}
+
+	struct dirent *dent;
+	char path[PATH_MAX];
+	int total = snprintf(path, sizeof(path), "%s/", dirname);
+
+	while ((dent = readdir(dh)) != NULL) {
+		char *ext = strrchr(dent->d_name, '.');
+		if (ext == NULL || strcmp(ext, inprogress_suffix) != 0)
+			continue;
+		strncpy(path + total, dent->d_name, sizeof(path) - total);
+		if (unlink(path) < 0) {
+			say_syserror("error while removing %s", path);
+			return;
+		} else
+			say_info("removed %s", path);
+	}
+}
+
 /* }}} */
 
 
diff --git a/src/box/xlog.h b/src/box/xlog.h
index 973910d..3b03588 100644
--- a/src/box/xlog.h
+++ b/src/box/xlog.h
@@ -70,6 +70,11 @@ enum xdir_type {
 enum log_suffix { NONE, INPROGRESS };
 
 /**
+ * Suffix added to path of inprogress files.
+ */
+#define inprogress_suffix ".inprogress"
+
+/**
  * A handle for a data directory with write ahead logs, snapshots,
  * vylogs.
  * Can be used to find the last log in the directory, scan
@@ -181,6 +186,12 @@ int
 xdir_collect_garbage(struct xdir *dir, int64_t signature, bool use_coio);
 
 /**
+ * Remove .inprogress files in specified directory.
+ */
+void
+xdir_collect_inprogress(struct xdir *xdir);
+
+/**
  * Return LSN and vclock (unless @vclock is NULL) of the newest
  * file in a directory or -1 if the directory is empty.
  */
diff --git a/src/errinj.h b/src/errinj.h
index 78514ac..7141af9 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -111,6 +111,9 @@ struct errinj {
 	_(ERRINJ_IPROTO_TX_DELAY, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_LOG_ROTATE, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_SNAP_COMMIT_DELAY, ERRINJ_BOOL, {.bparam = 0}) \
+	_(ERRINJ_VY_LOG_FILE_RENAME, ERRINJ_BOOL, {.bparam = false}) \
+	_(ERRINJ_VY_INDEX_FILE_RENAME, ERRINJ_BOOL, {.bparam = false}) \
+	_(ERRINJ_VY_RUN_FILE_RENAME, ERRINJ_BOOL, {.bparam = false}) \
 
 ENUM0(errinj_id, ERRINJ_LIST);
 extern struct errinj errinjs[];
diff --git a/test/box/errinj.result b/test/box/errinj.result
index 6ced172..3147cfb 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -40,8 +40,12 @@ errinj.info()
     state: false
   ERRINJ_WAL_IO:
     state: false
+  ERRINJ_VY_INDEX_FILE_RENAME:
+    state: false
   ERRINJ_TUPLE_ALLOC:
     state: false
+  ERRINJ_VY_RUN_FILE_RENAME:
+    state: false
   ERRINJ_VY_READ_PAGE:
     state: false
   ERRINJ_RELAY_REPORT_INTERVAL:
@@ -56,6 +60,8 @@ errinj.info()
     state: false
   ERRINJ_VY_LOG_FLUSH_DELAY:
     state: false
+  ERRINJ_VY_LOG_FILE_RENAME:
+    state: false
   ERRINJ_SNAP_COMMIT_DELAY:
     state: false
   ERRINJ_RELAY_FINAL_SLEEP:
@@ -693,6 +699,185 @@ errinj.set("ERRINJ_WAL_WRITE", false)
 space:drop()
 ---
 ...
+test_run:cmd("restart server default with cleanup=1")
+errinj = box.error.injection
+---
+...
+fio = require("fio")
+---
+...
+fiber = require('fiber')
+---
+...
+errinj.set("ERRINJ_VY_LOG_FILE_RENAME", true)
+---
+- ok
+...
+s = box.schema.space.create("vinyl_test", {engine='vinyl'})
+---
+...
+_ = s:create_index("primary")
+---
+...
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+---
+...
+while #fio.glob(vinyl_vylog) == 0 do fiber.sleep(0.001) end
+---
+...
+#fio.glob(vinyl_vylog) > 0 -- true
+---
+- true
+...
+space = box.schema.space.create("memtx_test")
+---
+...
+_ = space:create_index("primary")
+---
+...
+space:insert{1}
+---
+- [1]
+...
+errinj.set("ERRINJ_SNAP_COMMIT_DELAY", true)
+---
+- ok
+...
+_ = fiber.create(function() box.snapshot() end)
+---
+...
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+---
+...
+while #fio.glob(memtx_snap) == 0 do fiber.sleep(0.001) end
+---
+...
+#fio.glob(memtx_snap) > 0 --true
+---
+- true
+...
+test_run:cmd("restart server default")
+errinj = box.error.injection
+---
+...
+fio = require("fio")
+---
+...
+fiber = require('fiber')
+---
+...
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+---
+...
+#fio.glob(vinyl_vylog) == 0 -- true
+---
+- true
+...
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+---
+...
+#fio.glob(memtx_snap) == 0 -- true
+---
+- true
+...
+s = box.space.vinyl_test
+---
+...
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+errinj.set("ERRINJ_VY_INDEX_FILE_RENAME", true)
+---
+- ok
+...
+for i = 1, 10 do s:insert{i} end
+---
+...
+box.snapshot()
+---
+- error: Error injection 'vinyl index file rename'
+...
+while #fio.glob(vinyl_index) == 0 do fiber.sleep(0.001) end
+---
+...
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+errinj.set("ERRINJ_VY_RUN_FILE_RENAME", true)
+---
+- ok
+...
+while #fio.glob(vinyl_run) == 0  do fiber.sleep(0.001) end
+---
+...
+#fio.glob(vinyl_run) > 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_index) > 0 -- true
+---
+- true
+...
+test_run:cmd("restart server default")
+errinj = box.error.injection
+---
+...
+net_box = require('net.box')
+---
+...
+fio = require("fio")
+---
+...
+fiber = require('fiber')
+---
+...
+s = box.space.vinyl_test
+---
+...
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+---
+...
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+---
+...
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+---
+...
+-- no inprogress files should be present in memtx(vinyl)_dir.
+#fio.glob(memtx_snap) == 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_vylog) == 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_index) == 0 -- true
+---
+- true
+...
+#fio.glob(vinyl_run) == 0 -- true
+---
+- true
+...
+box.space.vinyl_test:drop()
+---
+...
+run_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/*/*")
+---
+...
+for _, file in ipairs(fio.glob(run_dir)) do fio.unlink(file) end
+---
+...
+vinyl_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/")
+---
+...
+for _, dir in ipairs(fio.glob(vinyl_dir)) do fio.rmtree(dir) end
+---
+...
 --test space:bsize() in case of memory error
 utils = dofile('utils.lua')
 ---
diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
index 3af1b74..66da1cf 100644
--- a/test/box/errinj.test.lua
+++ b/test/box/errinj.test.lua
@@ -197,12 +197,82 @@ _ = space:insert{1, require'digest'.urandom(192 * 1024)}
 errinj.set("ERRINJ_WAL_WRITE_DISK", false)
 
 _ = space:insert{1}
-
 errinj.set("ERRINJ_WAL_WRITE", true)
 box.snapshot()
 errinj.set("ERRINJ_WAL_WRITE", false)
 space:drop()
 
+test_run:cmd("restart server default with cleanup=1")
+errinj = box.error.injection
+fio = require("fio")
+fiber = require('fiber')
+
+errinj.set("ERRINJ_VY_LOG_FILE_RENAME", true)
+s = box.schema.space.create("vinyl_test", {engine='vinyl'})
+_ = s:create_index("primary")
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+while #fio.glob(vinyl_vylog) == 0 do fiber.sleep(0.001) end
+#fio.glob(vinyl_vylog) > 0 -- true
+
+space = box.schema.space.create("memtx_test")
+_ = space:create_index("primary")
+space:insert{1}
+
+errinj.set("ERRINJ_SNAP_COMMIT_DELAY", true)
+_ = fiber.create(function() box.snapshot() end)
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+while #fio.glob(memtx_snap) == 0 do fiber.sleep(0.001) end
+#fio.glob(memtx_snap) > 0 --true
+
+test_run:cmd("restart server default")
+errinj = box.error.injection
+fio = require("fio")
+fiber = require('fiber')
+
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+#fio.glob(vinyl_vylog) == 0 -- true
+
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+#fio.glob(memtx_snap) == 0 -- true
+
+s = box.space.vinyl_test
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+errinj.set("ERRINJ_VY_INDEX_FILE_RENAME", true)
+for i = 1, 10 do s:insert{i} end
+box.snapshot()
+
+while #fio.glob(vinyl_index) == 0 do fiber.sleep(0.001) end
+
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+errinj.set("ERRINJ_VY_RUN_FILE_RENAME", true)
+while #fio.glob(vinyl_run) == 0  do fiber.sleep(0.001) end
+
+#fio.glob(vinyl_run) > 0 -- true
+#fio.glob(vinyl_index) > 0 -- true
+
+test_run:cmd("restart server default")
+errinj = box.error.injection
+net_box = require('net.box')
+fio = require("fio")
+fiber = require('fiber')
+
+s = box.space.vinyl_test
+memtx_snap = string.format("%s/*.snap.inprogress", box.cfg.memtx_dir)
+vinyl_vylog = string.format("%s/*.vylog.inprogress", box.cfg.vinyl_dir)
+vinyl_run = string.format("%s/%s/*/*.run.inprogress", box.cfg.vinyl_dir, s.id)
+vinyl_index = string.format("%s/%s/*/*.index.inprogress", box.cfg.vinyl_dir, s.id)
+
+-- no inprogress files should be present in memtx(vinyl)_dir.
+#fio.glob(memtx_snap) == 0 -- true
+#fio.glob(vinyl_vylog) == 0 -- true
+#fio.glob(vinyl_index) == 0 -- true
+#fio.glob(vinyl_run) == 0 -- true
+
+box.space.vinyl_test:drop()
+run_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/*/*")
+for _, file in ipairs(fio.glob(run_dir)) do fio.unlink(file) end
+vinyl_dir = fio.pathjoin(box.cfg.vinyl_dir,"*/")
+for _, dir in ipairs(fio.glob(vinyl_dir)) do fio.rmtree(dir) end
 --test space:bsize() in case of memory error
 utils = dofile('utils.lua')
 s = box.schema.space.create('space_bsize')
-- 
2.7.4

Re: [PATCH v5 2/2] xlog: Remove inprogress files on start

[Vladimir Davydov]

On Wed, Jun 20, 2018 at 07:25:56PM +0300, Ilya Markov wrote:
> When tarantool crashes during writing to vylog, index, run or snapshot,
> inprogress files remain. But garbage collector doesn't take into account
> these files. So they remain until they are removed manually.
> 
> Fix this with adding deletion of such files to functions memtx_engine_end_recovery,
> vinyl_engine_end_recovery and vy_run_remove_files.
> 
> - Add 3 errinj which simulate the crash before renaming inprogress files.
> - Moved const string inprogress_suffix to xlog.h and replace it with
>   #defiled string in order to reuse it in vinyl run and index files removal.
> - Add 2 new vy_file_type in order to support formatting of inprogress
>   files names.
> 
> Closes #3406
> ---
>  src/box/memtx_engine.c   |   1 +
>  src/box/vinyl.c          |   3 +
>  src/box/vy_log.c         |   7 +-
>  src/box/vy_run.c         |  19 ++++-
>  src/box/vy_run.h         |   2 +
>  src/box/xlog.c           |  29 +++++++-
>  src/box/xlog.h           |  11 +++
>  src/errinj.h             |   3 +
>  test/box/errinj.result   | 185 +++++++++++++++++++++++++++++++++++++++++++++++
>  test/box/errinj.test.lua |  72 +++++++++++++++++-
>  10 files changed, 326 insertions(+), 6 deletions(-)

Reworked the test, polished the code, and pushed.