[PATCH 1/2] xlog: simplify xdir_add_vclock protocol

[PATCH 1/2] xlog: simplify xdir_add_vclock protocol

[Vladimir Davydov]

We add the vclock of a new snapshot/xlog/vylog file to the corresponding
xdir, because we need it for garbage collection and backup. We use the
xdir_add_vclock() function for that. Currently, the function protocol is
rather abstruse: it expects that the caller allocates a vclock with
malloc() and passes the ownership on to the xdir by calling this
function. This is done that way, because we add a vclock to an xdir
after committing a new xlog file, where we can't fail anymore. Since
malloc() can theoretically fail, we allocate a vclock before writing an
xlog and add it to an xdir only after the xlog is committed. This makes
the code look rather complicated.

Actually, as explained in #3534, malloc() doesn't normally fail so this
complexity is in fact needless and only makes it more difficult to patch
the code. Let's simplify the code by making xdir_add_vclock() allocate
vclock by itself and, since it can't fail, panic on allocation error.
---
https://github.com/tarantool/tarantool/issues/3624
https://github.com/tarantool/tarantool/commits/dv/gh-3624-vy-fix-vylog-backup

 src/box/memtx_engine.c | 26 ++++++++------------------
 src/box/vy_log.c       | 23 ++++-------------------
 src/box/wal.c          | 12 +-----------
 src/box/xlog.c         | 10 ++++++++++
 src/box/xlog.h         |  8 ++------
 5 files changed, 25 insertions(+), 54 deletions(-)

diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index f5ace926..1f80ce54 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -580,7 +580,7 @@ struct checkpoint {
 	struct cord cord;
 	bool waiting_for_snap_thread;
 	/** The vclock of the snapshot file. */
-	struct vclock *vclock;
+	struct vclock vclock;
 	struct xdir dir;
 	/**
 	 * Do nothing, just touch the snapshot file - the
@@ -597,14 +597,7 @@ checkpoint_init(struct checkpoint *ckpt, const char *snap_dirname,
 	ckpt->waiting_for_snap_thread = false;
 	xdir_create(&ckpt->dir, snap_dirname, SNAP, &INSTANCE_UUID);
 	ckpt->snap_io_rate_limit = snap_io_rate_limit;
-	/* May be used in abortCheckpoint() */
-	ckpt->vclock = malloc(sizeof(*ckpt->vclock));
-	if (ckpt->vclock == NULL) {
-		diag_set(OutOfMemory, sizeof(*ckpt->vclock),
-			 "malloc", "vclock");
-		return -1;
-	}
-	vclock_create(ckpt->vclock);
+	vclock_create(&ckpt->vclock);
 	ckpt->touch = false;
 	return 0;
 }
@@ -618,7 +611,6 @@ checkpoint_destroy(struct checkpoint *ckpt)
 	}
 	rlist_create(&ckpt->entries);
 	xdir_destroy(&ckpt->dir);
-	free(ckpt->vclock);
 }
 
 
@@ -656,7 +648,7 @@ checkpoint_f(va_list ap)
 	struct checkpoint *ckpt = va_arg(ap, struct checkpoint *);
 
 	if (ckpt->touch) {
-		if (xdir_touch_xlog(&ckpt->dir, ckpt->vclock) == 0)
+		if (xdir_touch_xlog(&ckpt->dir, &ckpt->vclock) == 0)
 			return 0;
 		/*
 		 * Failed to touch an existing snapshot, create
@@ -666,7 +658,7 @@ checkpoint_f(va_list ap)
 	}
 
 	struct xlog snap;
-	if (xdir_create_xlog(&ckpt->dir, &snap, ckpt->vclock) != 0)
+	if (xdir_create_xlog(&ckpt->dir, &snap, &ckpt->vclock) != 0)
 		return -1;
 
 	snap.rate_limit = ckpt->snap_io_rate_limit;
@@ -739,7 +731,7 @@ memtx_engine_wait_checkpoint(struct engine *engine,
 	    vclock_compare(&last, vclock) == 0) {
 		memtx->checkpoint->touch = true;
 	}
-	vclock_copy(memtx->checkpoint->vclock, vclock);
+	vclock_copy(&memtx->checkpoint->vclock, vclock);
 
 	if (cord_costart(&memtx->checkpoint->cord, "snapshot",
 			 checkpoint_f, memtx->checkpoint)) {
@@ -771,7 +763,7 @@ memtx_engine_commit_checkpoint(struct engine *engine,
 	small_alloc_setopt(&memtx->alloc, SMALL_DELAYED_FREE_MODE, false);
 
 	if (!memtx->checkpoint->touch) {
-		int64_t lsn = vclock_sum(memtx->checkpoint->vclock);
+		int64_t lsn = vclock_sum(&memtx->checkpoint->vclock);
 		struct xdir *dir = &memtx->checkpoint->dir;
 		/* rename snapshot on completion */
 		char to[PATH_MAX];
@@ -795,9 +787,7 @@ memtx_engine_commit_checkpoint(struct engine *engine,
 	if (xdir_last_vclock(&memtx->snap_dir, &last) < 0 ||
 	    vclock_compare(&last, vclock) != 0) {
 		/* Add the new checkpoint to the set. */
-		xdir_add_vclock(&memtx->snap_dir, memtx->checkpoint->vclock);
-		/* Prevent checkpoint_destroy() from freeing vclock. */
-		memtx->checkpoint->vclock = NULL;
+		xdir_add_vclock(&memtx->snap_dir, &memtx->checkpoint->vclock);
 	}
 
 	checkpoint_destroy(memtx->checkpoint);
@@ -824,7 +814,7 @@ memtx_engine_abort_checkpoint(struct engine *engine)
 	/** Remove garbage .inprogress file. */
 	char *filename =
 		xdir_format_filename(&memtx->checkpoint->dir,
-				     vclock_sum(memtx->checkpoint->vclock),
+				     vclock_sum(&memtx->checkpoint->vclock),
 				     INPROGRESS);
 	(void) coio_unlink(filename);
 
diff --git a/src/box/vy_log.c b/src/box/vy_log.c
index 3843cad6..d8613170 100644
--- a/src/box/vy_log.c
+++ b/src/box/vy_log.c
@@ -893,14 +893,9 @@ vy_log_bootstrap(void)
 		return vy_log_rebootstrap();
 
 	/* Add initial vclock to the xdir. */
-	struct vclock *vclock = malloc(sizeof(*vclock));
-	if (vclock == NULL) {
-		diag_set(OutOfMemory, sizeof(*vclock),
-			 "malloc", "struct vclock");
-		return -1;
-	}
-	vclock_create(vclock);
-	xdir_add_vclock(&vy_log.dir, vclock);
+	struct vclock vclock;
+	vclock_create(&vclock);
+	xdir_add_vclock(&vy_log.dir, &vclock);
 	return 0;
 }
 
@@ -1020,15 +1015,6 @@ vy_log_rotate(const struct vclock *vclock)
 		return 0;
 
 	assert(signature > prev_signature);
-
-	struct vclock *new_vclock = malloc(sizeof(*new_vclock));
-	if (new_vclock == NULL) {
-		diag_set(OutOfMemory, sizeof(*new_vclock),
-			 "malloc", "struct vclock");
-		return -1;
-	}
-	vclock_copy(new_vclock, vclock);
-
 	say_verbose("rotating vylog %lld => %lld",
 		    (long long)prev_signature, (long long)signature);
 
@@ -1065,14 +1051,13 @@ vy_log_rotate(const struct vclock *vclock)
 	vclock_copy(&vy_log.last_checkpoint, vclock);
 
 	/* Add the new vclock to the xdir so that we can track it. */
-	xdir_add_vclock(&vy_log.dir, new_vclock);
+	xdir_add_vclock(&vy_log.dir, vclock);
 
 	latch_unlock(&vy_log.latch);
 	say_verbose("done rotating vylog");
 	return 0;
 fail:
 	latch_unlock(&vy_log.latch);
-	free(new_vclock);
 	return -1;
 }
 
diff --git a/src/box/wal.c b/src/box/wal.c
index 41762a59..cd068a4e 100644
--- a/src/box/wal.c
+++ b/src/box/wal.c
@@ -579,26 +579,16 @@ wal_opt_rotate(struct wal_writer *writer)
 	if (xlog_is_open(&writer->current_wal))
 		return 0;
 
-	struct vclock *vclock = (struct vclock *)malloc(sizeof(*vclock));
-	if (vclock == NULL) {
-		diag_set(OutOfMemory, sizeof(*vclock),
-			 "malloc", "struct vclock");
-		diag_log();
-		return -1;
-	}
-	vclock_copy(vclock, &writer->vclock);
-
 	if (xdir_create_xlog(&writer->wal_dir, &writer->current_wal,
 			     &writer->vclock) != 0) {
 		diag_log();
-		free(vclock);
 		return -1;
 	}
 	/*
 	 * Keep track of the new WAL vclock. Required for garbage
 	 * collection, see wal_collect_garbage().
 	 */
-	xdir_add_vclock(&writer->wal_dir, vclock);
+	xdir_add_vclock(&writer->wal_dir, &writer->vclock);
 
 	wal_notify_watchers(writer, WAL_EVENT_ROTATE);
 	return 0;
diff --git a/src/box/xlog.c b/src/box/xlog.c
index e35fb4d9..d399a726 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -707,6 +707,16 @@ xdir_collect_inprogress(struct xdir *xdir)
 	}
 }
 
+void
+xdir_add_vclock(struct xdir *xdir, const struct vclock *vclock)
+{
+	struct vclock *copy = malloc(sizeof(*vclock));
+	if (copy == NULL)
+		panic("failed to allocate vclock");
+	vclock_copy(copy, vclock);
+	vclockset_insert(&xdir->index, copy);
+}
+
 /* }}} */
 
 
diff --git a/src/box/xlog.h b/src/box/xlog.h
index 476105c2..c2ac4774 100644
--- a/src/box/xlog.h
+++ b/src/box/xlog.h
@@ -208,13 +208,9 @@ xdir_last_vclock(struct xdir *xdir, struct vclock *vclock)
 
 /**
  * Insert a vclock into the file index of a directory.
- * The vclock must be allocated with malloc().
  */
-static inline void
-xdir_add_vclock(struct xdir *xdir, struct vclock *vclock)
-{
-	vclockset_insert(&xdir->index, vclock);
-}
+void
+xdir_add_vclock(struct xdir *xdir, const struct vclock *vclock);
 
 /* }}} */
 
-- 
2.11.0

[PATCH 2/2] vinyl: fix backup skipping vylog created after recovery

[Vladimir Davydov]

Commit 8e710090fcbc ("vinyl: simplify vylog recovery from backup") broke
backup in case the vylog directory is empty at the time of recovery
(i.e. vinyl isn't in use): before the commit we added the last
checkpoint vclock to the vylog directory index in this case while now we
don't. As a result, if the user starts using vinyl (creates a vinyl
space) after recovery, backup will not return the newly created vylog,
because it hasn't been indexed.

Fix this issue by restoring the code that adds the last checkpoint
vclock to the vylog directory index in case no vylog exists on recovery.

Closes #3624
---
https://github.com/tarantool/tarantool/issues/3624
https://github.com/tarantool/tarantool/commits/dv/gh-3624-vy-fix-vylog-backup

 src/box/vy_log.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/box/vy_log.c b/src/box/vy_log.c
index d8613170..fc8ede59 100644
--- a/src/box/vy_log.c
+++ b/src/box/vy_log.c
@@ -912,8 +912,17 @@ vy_log_begin_recovery(const struct vclock *vclock)
 	if (xdir_scan(&vy_log.dir) < 0 && errno != ENOENT)
 		return NULL;
 
-	if (xdir_last_vclock(&vy_log.dir, &vy_log.last_checkpoint) < 0)
+	if (xdir_last_vclock(&vy_log.dir, &vy_log.last_checkpoint) < 0) {
+		/*
+		 * Even if there's no vylog (i.e. vinyl isn't in use),
+		 * we still have to add the vclock to the xdir index,
+		 * because we may need it for garbage collection or
+		 * backup in case the user starts using vinyl after
+		 * recovery.
+		 */
+		xdir_add_vclock(&vy_log.dir, vclock);
 		vclock_copy(&vy_log.last_checkpoint, vclock);
+	}
 
 	int cmp = vclock_compare(&vy_log.last_checkpoint, vclock);
 	if (cmp > 0) {
-- 
2.11.0

[tarantool-patches] Re: [PATCH 1/2] xlog: simplify xdir_add_vclock protocol

[Kirill Yukhin]

Hello,
On 19 авг 23:44, Vladimir Davydov wrote:
> We add the vclock of a new snapshot/xlog/vylog file to the corresponding
> xdir, because we need it for garbage collection and backup. We use the
> xdir_add_vclock() function for that. Currently, the function protocol is
> rather abstruse: it expects that the caller allocates a vclock with
> malloc() and passes the ownership on to the xdir by calling this
> function. This is done that way, because we add a vclock to an xdir
> after committing a new xlog file, where we can't fail anymore. Since
> malloc() can theoretically fail, we allocate a vclock before writing an
> xlog and add it to an xdir only after the xlog is committed. This makes
> the code look rather complicated.
> 
> Actually, as explained in #3534, malloc() doesn't normally fail so this
> complexity is in fact needless and only makes it more difficult to patch
> the code. Let's simplify the code by making xdir_add_vclock() allocate
> vclock by itself and, since it can't fail, panic on allocation error.
> ---
> https://github.com/tarantool/tarantool/issues/3624
> https://github.com/tarantool/tarantool/commits/dv/gh-3624-vy-fix-vylog-backup
I've checked the patchset into 1.10 branch.

--
Regards, Kirill Yukhin