[PATCH 1.10 0/2] Vinyl memory leak fixes

[PATCH 1.10 0/2] Vinyl memory leak fixes

[Vladimir Davydov]

This patch set fixes a couple of memory leaks reported by Mail.Ru FRS.
Neither the master branch nor 2.1 is affected to those issues so this
commits are intended solely for 1.10.

Vladimir Davydov (2):
  vinyl: clean up region after allocating surrogate statement
  vinyl: free region on vylog commit instead of resetting it

 src/box/vy_log.c  |  2 +-
 src/box/vy_stmt.c | 15 +++++++++++----
 2 files changed, 12 insertions(+), 5 deletions(-)

-- 
2.11.0

[PATCH 1.10 1/2] vinyl: clean up region after allocating surrogate statement

[Vladimir Davydov]

vy_stmt_new_surrogate_from_key() and vy_stmt_new_surrogate_delete_raw()
allocate temporary objects on the region, but don't clean up after
themselves. Those functions may be called by a vinyl reader threads:

  vy_page_read_cb
    vy_page_find_key
      vy_page_stmt
        vy_stmt_decode

In this case the region will grow infinitely, because reader threads
never call fiber_gc().

The leak was introduced to 1.10 by commit b907231713a7 ("vinyl: lookup
key in reader thread"), which moved vy_page_find_key() invocation to
reader threads for the sake of performance. The fix is trivial - call
region_truncate() from those functions.

Note, neither the master branch nor 2.1 is affected to this issue,
because region_truncate() was added there in the scope of the multikey
index feature.
---
 src/box/vy_stmt.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c
index ca4c55f4..9b7c5551 100644
--- a/src/box/vy_stmt.c
+++ b/src/box/vy_stmt.c
@@ -383,6 +383,7 @@ vy_stmt_new_surrogate_from_key(const char *key, enum iproto_type type,
 	/* UPSERT can't be surrogate. */
 	assert(type != IPROTO_UPSERT);
 	struct region *region = &fiber()->gc;
+	size_t region_svp = region_used(region);
 
 	uint32_t field_count = format->index_field_count;
 	struct iovec *iov = region_alloc(region, sizeof(*iov) * field_count);
@@ -410,7 +411,7 @@ vy_stmt_new_surrogate_from_key(const char *key, enum iproto_type type,
 
 	struct tuple *stmt = vy_stmt_alloc(format, bsize);
 	if (stmt == NULL)
-		return NULL;
+		goto out;
 
 	char *raw = (char *) tuple_data(stmt);
 	uint32_t *field_map = (uint32_t *) raw;
@@ -428,6 +429,8 @@ vy_stmt_new_surrogate_from_key(const char *key, enum iproto_type type,
 	}
 	assert(wpos == raw + bsize);
 	vy_stmt_set_type(stmt, type);
+out:
+	region_truncate(region, region_svp);
 	return stmt;
 }
 
@@ -443,10 +446,13 @@ struct tuple *
 vy_stmt_new_surrogate_delete_raw(struct tuple_format *format,
 				 const char *src_data, const char *src_data_end)
 {
+	struct region *region = &fiber()->gc;
+	size_t region_svp = region_used(region);
+
 	uint32_t src_size = src_data_end - src_data;
 	uint32_t total_size = src_size + format->field_map_size;
 	/* Surrogate tuple uses less memory than the original tuple */
-	char *data = region_alloc(&fiber()->gc, total_size);
+	char *data = region_alloc(region, total_size);
 	if (data == NULL) {
 		diag_set(OutOfMemory, src_size, "region", "tuple");
 		return NULL;
@@ -490,13 +496,14 @@ vy_stmt_new_surrogate_delete_raw(struct tuple_format *format,
 	uint32_t bsize = pos - data;
 	struct tuple *stmt = vy_stmt_alloc(format, bsize);
 	if (stmt == NULL)
-		return NULL;
+		goto out;
 	char *stmt_data = (char *) tuple_data(stmt);
 	char *stmt_field_map_begin = stmt_data - format->field_map_size;
 	memcpy(stmt_data, data, bsize);
 	memcpy(stmt_field_map_begin, field_map_begin, format->field_map_size);
 	vy_stmt_set_type(stmt, IPROTO_DELETE);
-
+out:
+	region_truncate(region, region_svp);
 	return stmt;
 }
 
-- 
2.11.0

[PATCH 1.10 2/2] vinyl: free region on vylog commit instead of resetting it

[Vladimir Davydov]

region_reset() only frees memory from the last slab. As a result, if
a vylog transaction happens to use more than one slab, memory used by
vy_log.pool won't be freed, neither will it be reused, i.e. we'll get
a memory leak. Fix it by using region_free() instead of region_reset().
It's okay from performance point of view, because vylog transactions
are rare events.

Note, the master branch isn't affected to this issue, because the vylog
memory management was completely reworked there. 2.1 isn't affected
either, because there region_reset() was modified to free all slabs.
However, rather than backporting any of those commits, I think it's
more appropriate to simply use region_free().
---
 src/box/vy_log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/box/vy_log.c b/src/box/vy_log.c
index a0c6480e..275d5d72 100644
--- a/src/box/vy_log.c
+++ b/src/box/vy_log.c
@@ -802,7 +802,7 @@ vy_log_flush(void)
 		return -1;
 
 	/* Success. Free flushed records. */
-	region_reset(&vy_log.pool);
+	region_free(&vy_log.pool);
 	stailq_create(&vy_log.tx);
 	return 0;
 }
-- 
2.11.0

Re: [PATCH 1.10 0/2] Vinyl memory leak fixes

[Vladimir Davydov]

Pushed to 1.10.