[Tarantool-patches] [PATCH 0/2] Fix C module reloading

[Tarantool-patches] [PATCH 0/2] Fix C module reloading

[Kirill Yukhin]

The patchset fixes an issue when C module is not always
reloaded. It also introduces possibility to redirect
storage for temporary DSO copies to TMPDIR.

See extensive comment for 1st patch for details.

Branch: https://github.com/tarantool/tarantool/tree/kyukhin/gh-4945-fix-module-reload
Issue: https://github.com/tarantool/tarantool/issues/4945

Kirill Yukhin (2):
  Copy DSO module before load instead of symlink-ing
  Allow to set directory for copying DSO before load

 src/box/call.c                | 12 ++++++---
 src/box/func.c                | 42 ++++++++++++++++++++++++-----
 test/box/func_reload.result   | 62 +++++++++++++++++++++----------------------
 test/box/func_reload.test.lua | 33 ++++++++++++-----------
 4 files changed, 91 insertions(+), 58 deletions(-)

-- 
1.8.3.1

[Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing

[Kirill Yukhin]

Tarantool module reload mechanism allows to reload a module even
if there're functions running. It is implemented by refcounting
each invocation of module's routines.
If reload is called, then refcounter is checked:
 - If it is 0, then no routines are in flight and module is
   reloaded by simple pair of dlclose()/dlopen().
 - If it is non-zero, then there're routines in flight. To allow
   to load multiple versions of modules it is loaded not from the
   DSO specified. Symlink to tempdir is created and dlopen() is
   invoked against it (w/RTLD_LOCAL flag to avoid conflicts).

This trick was implemented in order to fool a dynamic linker: one
cannot invoke dlopen() against same file, so let's pretend
there're to independent DSOs.

The problem is that dynamic linker is smart enough. It tracks not
filenames, but i-nodes. Moreover it is smart enough to do stat -L
against DSO to follow symlinks! [1][2] So, any attempts to create
a symlinks to fool dynamic linker fail and instead of doing actual
load it just increments internal refcounter in map w/
corresponding inode, which in turn leads to not-reloading.

This wasn't caught by test since old module was always unlinked
before new one is copied in place.

The patch always copies DSO instead of creating a symlink. Also
it fixes the test so in SEGFAULTs without the change.

Closes #4945

[1] - https://sourceware.org/git/?p=glibc.git;a=blob;f=elf/dl-load\
.c;h=a5318f9c8d1d42745a254479cf6bb1cd2acd516f;hb=58557c229319a3b8d\
2eefdb62e7df95089eabe37#l898

[2] - https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/pos\
ix/dl-fileid.h;hb=58557c229319a3b8d2eefdb62e7df95089eabe37#l33
---
 src/box/func.c                | 19 +++++++++++++++++--
 test/box/func_reload.result   | 32 +++++++-------------------------
 test/box/func_reload.test.lua | 20 +++++++-------------
 3 files changed, 31 insertions(+), 40 deletions(-)

diff --git a/src/box/func.c b/src/box/func.c
index 04b04b6..a42a269 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -269,10 +269,25 @@ module_load(const char *package, const char *package_end)
 	char load_name[PATH_MAX + 1];
 	snprintf(load_name, sizeof(load_name), "%s/%.*s." TARANTOOL_LIBEXT,
 		 dir_name, package_len, package);
-	if (symlink(path, load_name) < 0) {
-		diag_set(SystemError, "failed to create dso link");
+
+	FILE *source, *target;
+	source = fopen(path, "r");
+	if (source == NULL) {
+		diag_set(SystemError, "failed to open module file for reading");
+		goto error;
+	}
+	target = fopen(load_name, "w");
+	if (target == NULL) {
+		fclose(source);
+		diag_set(SystemError, "failed to open temporary file for writing");
 		goto error;
 	}
+	int ch;
+	while( ( ch = fgetc(source) ) != EOF )
+		fputc(ch, target);
+	fclose(source);
+	fclose(target);
+
 	module->handle = dlopen(load_name, RTLD_NOW | RTLD_LOCAL);
 	if (unlink(load_name) != 0)
 		say_warn("failed to unlink dso link %s", load_name);
diff --git a/test/box/func_reload.result b/test/box/func_reload.result
index b024cd1..1313fdf 100644
--- a/test/box/func_reload.result
+++ b/test/box/func_reload.result
@@ -46,7 +46,7 @@ box.schema.user.grant('guest', 'read,write', 'space', 'test')
 _ = fio.unlink(reload_path)
 ---
 ...
-fio.symlink(reload1_path, reload_path)
+fio.copyfile(reload1_path, reload_path)
 ---
 - true
 ...
@@ -68,10 +68,7 @@ box.space.test:delete{0}
 ---
 - [0]
 ...
-_ = fio.unlink(reload_path)
----
-...
-fio.symlink(reload2_path, reload_path)
+fio.copyfile(reload2_path, reload_path)
 ---
 - true
 ...
@@ -92,10 +89,7 @@ box.space.test:truncate()
 ---
 ...
 -- test case with hanging calls
-_ = fio.unlink(reload_path)
----
-...
-fio.symlink(reload1_path, reload_path)
+fio.copyfile(reload1_path, reload_path)
 ---
 - true
 ...
@@ -115,10 +109,7 @@ while box.space.test:count() < fibers do fiber.sleep(0.001) end
 box.schema.func.reload("reload")
 ---
 ...
-_ = fio.unlink(reload_path)
----
-...
-fio.symlink(reload2_path, reload_path)
+fio.copyfile(reload2_path, reload_path)
 ---
 - true
 ...
@@ -162,10 +153,7 @@ box.schema.func.drop("reload.foo")
 box.space.test:drop()
 ---
 ...
-_ = fio.unlink(reload_path)
----
-...
-fio.symlink(reload1_path, reload_path)
+fio.copyfile(reload1_path, reload_path)
 ---
 - true
 ...
@@ -196,10 +184,7 @@ s:delete({1})
 ---
 - [1, 2]
 ...
-_ = fio.unlink(reload_path)
----
-...
-fio.symlink(reload2_path, reload_path)
+fio.copyfile(reload2_path, reload_path)
 ---
 - true
 ...
@@ -236,10 +221,7 @@ c:call("reload.test_reload_fail")
 ---
 - [[2]]
 ...
-_ = fio.unlink(reload_path)
----
-...
-fio.symlink(reload1_path, reload_path)
+fio.copyfile(reload1_path, reload_path)
 ---
 - true
 ...
diff --git a/test/box/func_reload.test.lua b/test/box/func_reload.test.lua
index 8906898..4c062fd 100644
--- a/test/box/func_reload.test.lua
+++ b/test/box/func_reload.test.lua
@@ -17,7 +17,7 @@ _ = box.schema.space.create('test')
 _ = box.space.test:create_index('primary', {parts = {1, "integer"}})
 box.schema.user.grant('guest', 'read,write', 'space', 'test')
 _ = fio.unlink(reload_path)
-fio.symlink(reload1_path, reload_path)
+fio.copyfile(reload1_path, reload_path)
 
 --check not fail on non-load func
 box.schema.func.reload("reload")
@@ -26,8 +26,7 @@ box.schema.func.reload("reload")
 box.space.test:insert{0}
 c:call("reload.foo", {1})
 box.space.test:delete{0}
-_ = fio.unlink(reload_path)
-fio.symlink(reload2_path, reload_path)
+fio.copyfile(reload2_path, reload_path)
 
 box.schema.func.reload("reload")
 c:call("reload.foo")
@@ -35,8 +34,7 @@ box.space.test:select{}
 box.space.test:truncate()
 
 -- test case with hanging calls
-_ = fio.unlink(reload_path)
-fio.symlink(reload1_path, reload_path)
+fio.copyfile(reload1_path, reload_path)
 box.schema.func.reload("reload")
 
 fibers = 10
@@ -46,8 +44,7 @@ while box.space.test:count() < fibers do fiber.sleep(0.001) end
 -- double reload doesn't fail waiting functions
 box.schema.func.reload("reload")
 
-_ = fio.unlink(reload_path)
-fio.symlink(reload2_path, reload_path)
+fio.copyfile(reload2_path, reload_path)
 box.schema.func.reload("reload")
 c:call("reload.foo")
 
@@ -55,9 +52,8 @@ while box.space.test:count() < 2 * fibers + 1 do fiber.sleep(0.001) end
 box.space.test:select{}
 box.schema.func.drop("reload.foo")
 box.space.test:drop()
-_ = fio.unlink(reload_path)
 
-fio.symlink(reload1_path, reload_path)
+fio.copyfile(reload1_path, reload_path)
 box.schema.func.create('reload.test_reload', {language = "C"})
 box.schema.user.grant('guest', 'execute', 'function', 'reload.test_reload')
 s = box.schema.space.create('test_reload')
@@ -67,8 +63,7 @@ ch = fiber.channel(2)
 -- call first time to load function
 c:call("reload.test_reload")
 s:delete({1})
-_ = fio.unlink(reload_path)
-fio.symlink(reload2_path, reload_path)
+fio.copyfile(reload2_path, reload_path)
 _ = fiber.create(function() ch:put(c:call("reload.test_reload")) end)
 while s:get({1}) == nil do fiber.yield(0.0001) end
 box.schema.func.reload("reload")
@@ -80,8 +75,7 @@ s:drop()
 box.schema.func.create('reload.test_reload_fail', {language = "C"})
 box.schema.user.grant('guest', 'execute', 'function', 'reload.test_reload_fail')
 c:call("reload.test_reload_fail")
-_ = fio.unlink(reload_path)
-fio.symlink(reload1_path, reload_path)
+fio.copyfile(reload1_path, reload_path)
 c:call("reload.test_reload")
 c:call("reload.test_reload_fail")
 
-- 
1.8.3.1

[Tarantool-patches] [PATCH 2/2] Allow to set directory for copying DSO before load

[Kirill Yukhin]

Make it possible to set temporary directory where
module will be copied before load.

@TarantoolBot document
Title: Justify module (re-)loading semantics

It is now possible to set directory where temporary
copies of modules to be loaded will be created.
It is done by setting $(TMPDIR) variable. It
will be "/tmp" if variable was not set.

Follow up #4945
---
 src/box/call.c                | 12 ++++++++----
 src/box/func.c                | 23 +++++++++++++++++++----
 test/box/func_reload.result   | 30 +++++++++++++++++++++++-------
 test/box/func_reload.test.lua | 13 ++++++++++---
 4 files changed, 60 insertions(+), 18 deletions(-)

diff --git a/src/box/call.c b/src/box/call.c
index 7c70d81..c16ac70 100644
--- a/src/box/call.c
+++ b/src/box/call.c
@@ -129,10 +129,14 @@ box_module_reload(const char *name)
 		return -1;
 	}
 	struct module *module = NULL;
-	if (module_reload(name, name + strlen(name), &module) == 0 &&
-	    module != NULL)
-		return 0;
-	diag_set(ClientError, ER_NO_SUCH_MODULE, name);
+	if (module_reload(name, name + strlen(name), &module) == 0) {
+		if (module != NULL)
+			return 0;
+		else {
+			diag_set(ClientError, ER_NO_SUCH_MODULE, name);
+			return -1;
+		}
+	}
 	return -1;
 }
 
diff --git a/src/box/func.c b/src/box/func.c
index a42a269..0405eb9 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -261,14 +261,29 @@ module_load(const char *package, const char *package_end)
 	module->package[package_len] = 0;
 	rlist_create(&module->funcs);
 	module->calls = 0;
-	char dir_name[] = "/tmp/tntXXXXXX";
+
+	const char *tmpdir = getenv("TMPDIR");
+	if (tmpdir == NULL)
+		tmpdir = "/tmp";
+	char dir_name[PATH_MAX + 1];
+	int rc = snprintf(dir_name, sizeof(dir_name), "%s/tntXXXXXX", tmpdir);
+	if (rc < 0 || (size_t) rc >= sizeof(dir_name)) {
+		diag_set(SystemError, "failed to generate path to tmp dir");
+		goto error;
+	}
+
 	if (mkdtemp(dir_name) == NULL) {
-		diag_set(SystemError, "failed to create unique dir name");
+		diag_set(SystemError, "failed to create unique dir name: %s",
+			 dir_name);
 		goto error;
 	}
 	char load_name[PATH_MAX + 1];
-	snprintf(load_name, sizeof(load_name), "%s/%.*s." TARANTOOL_LIBEXT,
-		 dir_name, package_len, package);
+	rc = snprintf(load_name, sizeof(load_name), "%s/%.*s." TARANTOOL_LIBEXT,
+		      dir_name, package_len, package);
+	if (rc < 0 || (size_t) rc >= sizeof(dir_name)) {
+		diag_set(SystemError, "failed to generate path to DSO");
+		goto error;
+	}
 
 	FILE *source, *target;
 	source = fopen(path, "r");
diff --git a/test/box/func_reload.result b/test/box/func_reload.result
index 1313fdf..fbc2ad7 100644
--- a/test/box/func_reload.result
+++ b/test/box/func_reload.result
@@ -233,20 +233,36 @@ c:call("reload.test_reload_fail")
 ---
 - [[2]]
 ...
-box.schema.func.drop("reload.test_reload")
+box.schema.func.reload()
 ---
+- error: 'bad argument #1 to ''?'' (string expected, got no value)'
 ...
-box.schema.func.drop("reload.test_reload_fail")
+box.schema.func.reload("non-existing")
 ---
+- error: Module 'non-existing' does not exist
 ...
-_ = fio.unlink(reload_path)
+-- Make sure that $TMPDIR env variable is used to generate temporary
+-- path for DSO copy
+os.setenv("TMPDIR", "/dev/null")
 ---
 ...
-box.schema.func.reload()
+_, err = pcall(box.schema.func.reload, "reload")
 ---
-- error: 'bad argument #1 to ''?'' (string expected, got no value)'
 ...
-box.schema.func.reload("non-existing")
+tostring(err):gsub(': [/%w]+$', '')
+---
+- failed to create unique dir name
+- 1
+...
+os.setenv("TMPDIR", nil)
+---
+...
+box.schema.func.drop("reload.test_reload")
+---
+...
+box.schema.func.drop("reload.test_reload_fail")
+---
+...
+_ = fio.unlink(reload_path)
 ---
-- error: Module 'non-existing' does not exist
 ...
diff --git a/test/box/func_reload.test.lua b/test/box/func_reload.test.lua
index 4c062fd..5b1b4e9 100644
--- a/test/box/func_reload.test.lua
+++ b/test/box/func_reload.test.lua
@@ -79,9 +79,16 @@ fio.copyfile(reload1_path, reload_path)
 c:call("reload.test_reload")
 c:call("reload.test_reload_fail")
 
+box.schema.func.reload()
+box.schema.func.reload("non-existing")
+
+-- Make sure that $TMPDIR env variable is used to generate temporary
+-- path for DSO copy
+os.setenv("TMPDIR", "/dev/null")
+_, err = pcall(box.schema.func.reload, "reload")
+tostring(err):gsub(': [/%w]+$', '')
+os.setenv("TMPDIR", nil)
+
 box.schema.func.drop("reload.test_reload")
 box.schema.func.drop("reload.test_reload_fail")
 _ = fio.unlink(reload_path)
-
-box.schema.func.reload()
-box.schema.func.reload("non-existing")
-- 
1.8.3.1

Re: [Tarantool-patches] [PATCH 0/2] Fix C module reloading

[Nikita Pettik]

On 20 May 12:51, Kirill Yukhin wrote:
> The patchset fixes an issue when C module is not always
> reloaded. It also introduces possibility to redirect
> storage for temporary DSO copies to TMPDIR.
> 
> See extensive comment for 1st patch for details.
> 
> Branch: https://github.com/tarantool/tarantool/tree/kyukhin/gh-4945-fix-module-reload
> Issue: https://github.com/tarantool/tarantool/issues/4945

Changelog is missing, I guess.
 
> Kirill Yukhin (2):
>   Copy DSO module before load instead of symlink-ing
>   Allow to set directory for copying DSO before load
> 
>  src/box/call.c                | 12 ++++++---
>  src/box/func.c                | 42 ++++++++++++++++++++++++-----
>  test/box/func_reload.result   | 62 +++++++++++++++++++++----------------------
>  test/box/func_reload.test.lua | 33 ++++++++++++-----------
>  4 files changed, 91 insertions(+), 58 deletions(-)
> 
> -- 
> 1.8.3.1

Re: [Tarantool-patches] [PATCH 0/2] Fix C module reloading

[Kirill Yukhin]

Hello,

On 20 май 20:41, Nikita Pettik wrote:
> On 20 May 12:51, Kirill Yukhin wrote:
> > The patchset fixes an issue when C module is not always
> > reloaded. It also introduces possibility to redirect
> > storage for temporary DSO copies to TMPDIR.
> > 
> > See extensive comment for 1st patch for details.
> > 
> > Branch: https://github.com/tarantool/tarantool/tree/kyukhin/gh-4945-fix-module-reload
> > Issue: https://github.com/tarantool/tarantool/issues/4945
> 
> Changelog is missing, I guess.

Sure

ChangeLog entry:
* Fixed a bug in C module reloading (gh-4945).

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing

[Konstantin Osipov]

* Kirill Yukhin <kyukhin@tarantool.org> [20/05/20 12:54]:
> +	int ch;
> +	while( ( ch = fgetc(source) ) != EOF )
> +		fputc(ch, target);


This code looks like it is copied from stackoverflow?
For one, it does not follow Tarantool coding style. 
and it's copying the file one byte at a time, good at least it's
in buffered mode?!

> +	fclose(source);
> +	fclose(target);


Please use libeio or, as last resort, sendfile().

Tarantool is a single threaded high-performance database and
application server. It's not OK to block the event loop for a few
hundred thousand instructions (and here we can easily get
millions).


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing

[Kirill Yukhin]

Hello,

Thanks a lot fr your inputs.

My answers are inlined. Iterative patch at the bottom.
I've force pushed updated branch.

On 23 май 21:30, Konstantin Osipov wrote:
> * Kirill Yukhin <kyukhin@tarantool.org> [20/05/20 12:54]:
> > +	fclose(source);
> > +	fclose(target);
> 
> 
> Please use libeio or, as last resort, sendfile().
> 
> Tarantool is a single threaded high-performance database and
> application server. It's not OK to block the event loop for a few
> hundred thousand instructions (and here we can easily get
> millions).

As a first try, I took a look @ coio_copyfile(), but it seems
that we cannot use anything which could yield as the routine
might be called from within a transaction.

So, I've decided to use eio_sendfile() and calculate chunk
size using stat() syscall, just like in coio_do_copyfile().

I still think that blocking TX thread for couple of
milliseconds is okay and updated patch is overkill, since
copying of libcurl DSO (0.5MB) takes about 2 milliseconds
according to my measurements.

> -- 
> Konstantin Osipov, Moscow, Russia

--
Regards, Kirill Yukhin

diff --git a/src/box/func.c b/src/box/func.c
index a42a269..c44da99 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -40,6 +40,8 @@
 #include "port.h"
 #include "schema.h"
 #include "session.h"
+#include "libeio/eio.h"
+#include <fcntl.h>
 #include <dlfcn.h>
 
 /**
@@ -270,23 +272,43 @@ module_load(const char *package, const char *package_end)
 	snprintf(load_name, sizeof(load_name), "%s/%.*s." TARANTOOL_LIBEXT,
 		 dir_name, package_len, package);
 
-	FILE *source, *target;
-	source = fopen(path, "r");
-	if (source == NULL) {
-		diag_set(SystemError, "failed to open module file for reading");
+	struct stat st;
+	if (stat(path, &st) < 0) {
+		diag_set(SystemError, "failed to stat() module %s", path);
 		goto error;
 	}
-	target = fopen(load_name, "w");
-	if (target == NULL) {
-		fclose(source);
-		diag_set(SystemError, "failed to open temporary file for writing");
+
+	int source_fd = open(path, O_RDONLY);
+	if (source_fd < 0) {
+		diag_set(SystemError, "failed to open module %s file for" \
+			 " reading", path);
+		goto error;
+	}
+	int dest_fd = open(load_name, O_WRONLY|O_CREAT|O_TRUNC,
+			   st.st_mode & 0777);
+	if (dest_fd < 0) {
+		diag_set(SystemError, "failed to open file %s for writing ",
+			 load_name);
+		close(source_fd);
 		goto error;
 	}
-	int ch;
-	while( ( ch = fgetc(source) ) != EOF )
-		fputc(ch, target);
-	fclose(source);
-	fclose(target);
+
+	off_t pos, left;
+	for (left = st.st_size, pos = 0; left > 0;) {
+		off_t ret = eio_sendfile_sync(dest_fd, source_fd, pos,
+					      st.st_size);
+		if (ret < 0) {
+			diag_set(SystemError, "failed to copy DSO %s to %s",
+				 path, load_name);
+			close(source_fd);
+			close(dest_fd);
+			goto error;
+		}
+		pos += ret;
+		left -= ret;
+	}
+	close(source_fd);
+	close(dest_fd);
 
 	module->handle = dlopen(load_name, RTLD_NOW | RTLD_LOCAL);
 	if (unlink(load_name) != 0)

Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing

[Konstantin Osipov]

* Kirill Yukhin <kyukhin@tarantool.org> [20/05/25 16:17]:
> Hello,
> 
> Thanks a lot fr your inputs.
> 
> My answers are inlined. Iterative patch at the bottom.
> I've force pushed updated branch.
> 
> On 23 май 21:30, Konstantin Osipov wrote:
> > * Kirill Yukhin <kyukhin@tarantool.org> [20/05/20 12:54]:
> > > +	fclose(source);
> > > +	fclose(target);
> > 
> > 
> > Please use libeio or, as last resort, sendfile().
> > 
> > Tarantool is a single threaded high-performance database and
> > application server. It's not OK to block the event loop for a few
> > hundred thousand instructions (and here we can easily get
> > millions).
> 
> As a first try, I took a look @ coio_copyfile(), but it seems
> that we cannot use anything which could yield as the routine
> might be called from within a transaction.

There is no commitment to allow plugin reload within a
transaction, and I doubt anyone would use it this way. It's a
coincidence - since Vova changed to allow everything which doesn't
yield to run within a transaction, it became allowed.

> +	off_t pos, left;
> +	for (left = st.st_size, pos = 0; left > 0;) {
> +		off_t ret = eio_sendfile_sync(dest_fd, source_fd, pos,
> +					      st.st_size);

I wonder why do you use eio_sendfile_sync()?

I don't know why coio_copyfile uses it BTW. Why is eio_sendfile()
not enough?

> +		if (ret < 0) {
> +			diag_set(SystemError, "failed to copy DSO %s to %s",
> +				 path, load_name);
> +			close(source_fd);
> +			close(dest_fd);
> +			goto error;
> +		}
> +		pos += ret;
> +		left -= ret;
> +	}
> +	close(source_fd);
> +	close(dest_fd);
>  
>  	module->handle = dlopen(load_name, RTLD_NOW | RTLD_LOCAL);
>  	if (unlink(load_name) != 0)

-- 
Konstantin Osipov, Moscow, Russia
https://scylladb.com

Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing

[Cyrill Gorcunov]

On Mon, May 25, 2020 at 05:34:19PM +0300, Konstantin Osipov wrote:
> 
> > +	off_t pos, left;
> > +	for (left = st.st_size, pos = 0; left > 0;) {
> > +		off_t ret = eio_sendfile_sync(dest_fd, source_fd, pos,
> > +					      st.st_size);
> 
> I wonder why do you use eio_sendfile_sync()?
> 
> I don't know why coio_copyfile uses it BTW. Why is eio_sendfile()
> not enough?

```
eio_ssize_t
eio_sendfile_sync (int ofd, int ifd, off_t offset, size_t count)
{
  return eio__sendfile (ofd, ifd, offset, count);
}
```

They are simply alias to each other.

Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing

[Cyrill Gorcunov]

On Mon, May 25, 2020 at 06:18:48PM +0300, Cyrill Gorcunov wrote:
> On Mon, May 25, 2020 at 05:34:19PM +0300, Konstantin Osipov wrote:
> > 
> > > +	off_t pos, left;
> > > +	for (left = st.st_size, pos = 0; left > 0;) {
> > > +		off_t ret = eio_sendfile_sync(dest_fd, source_fd, pos,
> > > +					      st.st_size);
> > 
> > I wonder why do you use eio_sendfile_sync()?
> > 
> > I don't know why coio_copyfile uses it BTW. Why is eio_sendfile()
> > not enough?
> 
> ```
> eio_ssize_t
> eio_sendfile_sync (int ofd, int ifd, off_t offset, size_t count)
> {
>   return eio__sendfile (ofd, ifd, offset, count);
> }
> ```
> 
> They are simply alias to each other.

If you meant that there is no need for a loop, then indeed,
looks like eio__sendfile can handle partial writes by self.
Still it comes from commit

commit 04bf646f359cdf9ef0e07121b4d316c5d5d6213e
Author: Cyrill Gorcunov <gorcunov@gmail.com>
Date:   Thu Apr 18 23:49:58 2019 +0300

    core/coio_file: Use eio_sendfile_sync instead of a chunk mode
    
    eio library provides a portable version of sendfile syscall
    which works a way more efficient than explicit copying file
    by 4K chunks.

we've left this cycle here to be able to test partial transfers.
Probably worth to simply drop it off by now, the commit is more
than year old and I think we can assume that everything is tested
enoough and we don't need error injection in this code anymore.

Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [20/05/25 18:23]:

> On Mon, May 25, 2020 at 05:34:19PM +0300, Konstantin Osipov wrote:
> > 
> > > +	off_t pos, left;
> > > +	for (left = st.st_size, pos = 0; left > 0;) {
> > > +		off_t ret = eio_sendfile_sync(dest_fd, source_fd, pos,
> > > +					      st.st_size);
> > 
> > I wonder why do you use eio_sendfile_sync()?
> > 
> > I don't know why coio_copyfile uses it BTW. Why is eio_sendfile()
> > not enough?
> 
> ```
> eio_ssize_t
> eio_sendfile_sync (int ofd, int ifd, off_t offset, size_t count)
> {
>   return eio__sendfile (ofd, ifd, offset, count);
> }
> ```
> 
> They are simply alias to each other.

Right, but one is public api and the other is not?

Check eio.pod please.

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing

[Cyrill Gorcunov]

On Mon, May 25, 2020 at 07:45:01PM +0300, Konstantin Osipov wrote:
> > > I don't know why coio_copyfile uses it BTW. Why is eio_sendfile()
> > > not enough?
> > 
> > ```
> > eio_ssize_t
> > eio_sendfile_sync (int ofd, int ifd, off_t offset, size_t count)
> > {
> >   return eio__sendfile (ofd, ifd, offset, count);
> > }
> > ```
> > 
> > They are simply alias to each other.
> 
> Right, but one is public api and the other is not?
> 
> Check eio.pod please.

Indeed :( Thanks! That said I think we simply need to use
eio_sendfile_sync directly, without yields and without
using any for(;;) cycles, the eio will do all inside.

Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing

[Kirill Yukhin]

Hello,

Thanks a lot for your comments. My answers are inlined.

On 25 май 17:34, Konstantin Osipov wrote:
> * Kirill Yukhin <kyukhin@tarantool.org> [20/05/25 16:17]:
> > Hello,
> > 
> > Thanks a lot fr your inputs.
> > 
> > My answers are inlined. Iterative patch at the bottom.
> > I've force pushed updated branch.
> > 
> > On 23 май 21:30, Konstantin Osipov wrote:
> > > * Kirill Yukhin <kyukhin@tarantool.org> [20/05/20 12:54]:
> > > > +	fclose(source);
> > > > +	fclose(target);
> > > 
> > > 
> > > Please use libeio or, as last resort, sendfile().
> > > 
> > > Tarantool is a single threaded high-performance database and
> > > application server. It's not OK to block the event loop for a few
> > > hundred thousand instructions (and here we can easily get
> > > millions).
> > 
> > As a first try, I took a look @ coio_copyfile(), but it seems
> > that we cannot use anything which could yield as the routine
> > might be called from within a transaction.
> 
> There is no commitment to allow plugin reload within a
> transaction, and I doubt anyone would use it this way. It's a
> coincidence - since Vova changed to allow everything which doesn't
> yield to run within a transaction, it became allowed.

According to code module load occurs on first routine invocation.
That means, that it possibly occur inside a transaction. See
func_c_call() for details. Hence we are prohibited to yield.

> > +	off_t pos, left;
> > +	for (left = st.st_size, pos = 0; left > 0;) {
> > +		off_t ret = eio_sendfile_sync(dest_fd, source_fd, pos,
> > +					      st.st_size);
> 
> I wonder why do you use eio_sendfile_sync()?

Since we cannot yield from within a transaction.

> -- 
> Konstantin Osipov, Moscow, Russia
> https://scylladb.com

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing

[Cyrill Gorcunov]

On Mon, May 25, 2020 at 04:13:54PM +0300, Kirill Yukhin wrote:
> +
> +	off_t pos, left;
> +	for (left = st.st_size, pos = 0; left > 0;) {
> +		off_t ret = eio_sendfile_sync(dest_fd, source_fd, pos,
> +					      st.st_size);
> +		if (ret < 0) {
> +			diag_set(SystemError, "failed to copy DSO %s to %s",
> +				 path, load_name);
> +			close(source_fd);
> +			close(dest_fd);
> +			goto error;
> +		}
> +		pos += ret;
> +		left -= ret;
> +	}
> +	close(source_fd);
> +	close(dest_fd);

Kirill, we don't need the for() cycle here, the eio_sendfile_sync
will handle the cycle by self (inside implementation).

Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing

[Kirill Yukhin]

Hello,

On 26 май 18:16, Cyrill Gorcunov wrote:
> On Mon, May 25, 2020 at 04:13:54PM +0300, Kirill Yukhin wrote:
> > +
> > +	off_t pos, left;
> > +	for (left = st.st_size, pos = 0; left > 0;) {
> > +		off_t ret = eio_sendfile_sync(dest_fd, source_fd, pos,
> > +					      st.st_size);
> > +		if (ret < 0) {
> > +			diag_set(SystemError, "failed to copy DSO %s to %s",
> > +				 path, load_name);
> > +			close(source_fd);
> > +			close(dest_fd);
> > +			goto error;
> > +		}
> > +		pos += ret;
> > +		left -= ret;
> > +	}
> > +	close(source_fd);
> > +	close(dest_fd);
> 
> Kirill, we don't need the for() cycle here, the eio_sendfile_sync
> will handle the cycle by self (inside implementation).

Ah, sure, thanks! Iterative patch in the bottom.
Branch force-pushed.

We should probably update coio_do_copyfile() as well.
This is where I've copy-and-pasted from.

--
Regards, Kirill Yukhin

diff --git a/src/box/func.c b/src/box/func.c
index e0c45a4..a8697d3 100644
--- a/src/box/func.c
+++ b/src/box/func.c
@@ -308,22 +308,14 @@ module_load(const char *package, const char *package_end)
 		goto error;
 	}
 
-	off_t pos, left;
-	for (left = st.st_size, pos = 0; left > 0;) {
-		off_t ret = eio_sendfile_sync(dest_fd, source_fd, pos,
-					      st.st_size);
-		if (ret < 0) {
-			diag_set(SystemError, "failed to copy DSO %s to %s",
-				 path, load_name);
-			close(source_fd);
-			close(dest_fd);
-			goto error;
-		}
-		pos += ret;
-		left -= ret;
-	}
+	off_t ret = eio_sendfile_sync(dest_fd, source_fd, 0, st.st_size);
 	close(source_fd);
 	close(dest_fd);
+	if (ret != st.st_size) {
+		diag_set(SystemError, "failed to copy DSO %s to %s",
+			 path, load_name);
+		goto error;
+	}
 
 	module->handle = dlopen(load_name, RTLD_NOW | RTLD_LOCAL);
 	if (unlink(load_name) != 0)

Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing

[Cyrill Gorcunov]

On Wed, May 27, 2020 at 12:17:13PM +0300, Kirill Yukhin wrote:
> 
> We should probably update coio_do_copyfile() as well.
> This is where I've copy-and-pasted from.

True.

Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>

Re: [Tarantool-patches] [PATCH 2/2] Allow to set directory for copying DSO before load

[Cyrill Gorcunov]

On Wed, May 20, 2020 at 12:51:18PM +0300, Kirill Yukhin wrote:
...
> --- a/src/box/call.c
> +++ b/src/box/call.c
> @@ -129,10 +129,14 @@ box_module_reload(const char *name)
>  		return -1;
>  	}
>  	struct module *module = NULL;
> -	if (module_reload(name, name + strlen(name), &module) == 0 &&
> -	    module != NULL)
> -		return 0;
> -	diag_set(ClientError, ER_NO_SUCH_MODULE, name);
> +	if (module_reload(name, name + strlen(name), &module) == 0) {
> +		if (module != NULL)
> +			return 0;
> +		else {
> +			diag_set(ClientError, ER_NO_SUCH_MODULE, name);
> +			return -1;

Redundant return -1? Since we're gonna return -1 a few lines lower anyway.
I might be missing something obvious but I don't get why have you moved
module != NULL into a separate block. If you prefer this form I don't
mind though.

> +		}
> +	}
>  	return -1;
>  }
>  
> diff --git a/src/box/func.c b/src/box/func.c
> index a42a269..0405eb9 100644
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -261,14 +261,29 @@ module_load(const char *package, const char *package_end)
>  	module->package[package_len] = 0;
>  	rlist_create(&module->funcs);
>  	module->calls = 0;
> -	char dir_name[] = "/tmp/tntXXXXXX";
> +
> +	const char *tmpdir = getenv("TMPDIR");
> +	if (tmpdir == NULL)
> +		tmpdir = "/tmp";
> +	char dir_name[PATH_MAX + 1];

No need for PATH_MAX + 1, PATH_MAX should be enough. Other than this -- Ack.

Re: [Tarantool-patches] [PATCH 0/2] Fix C module reloading

[Kirill Yukhin]

Hello,

On 20 май 12:51, Kirill Yukhin wrote:
> The patchset fixes an issue when C module is not always
> reloaded. It also introduces possibility to redirect
> storage for temporary DSO copies to TMPDIR.
> 
> See extensive comment for 1st patch for details.
> 
> Branch: https://github.com/tarantool/tarantool/tree/kyukhin/gh-4945-fix-module-reload
> Issue: https://github.com/tarantool/tarantool/issues/4945
> 
> Kirill Yukhin (2):
>   Copy DSO module before load instead of symlink-ing
>   Allow to set directory for copying DSO before load
> 
>  src/box/call.c                | 12 ++++++---
>  src/box/func.c                | 42 ++++++++++++++++++++++++-----
>  test/box/func_reload.result   | 62 +++++++++++++++++++++----------------------
>  test/box/func_reload.test.lua | 33 ++++++++++++-----------
>  4 files changed, 91 insertions(+), 58 deletions(-)

I've checked the patchset into 2.3, 2.4 and master.
I've also cherry-pick 1st patch into 1.10.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH 2/2] Allow to set directory for copying DSO before load

[Kirill Yukhin]

Hello,

Thanks a lot! Both objections fixed.

--
Regards, Kirill Yukhin

On 29 май 13:59, Cyrill Gorcunov wrote:
> On Wed, May 20, 2020 at 12:51:18PM +0300, Kirill Yukhin wrote:
> ...
> > --- a/src/box/call.c
> > +++ b/src/box/call.c
> > @@ -129,10 +129,14 @@ box_module_reload(const char *name)
> >  		return -1;
> >  	}
> >  	struct module *module = NULL;
> > -	if (module_reload(name, name + strlen(name), &module) == 0 &&
> > -	    module != NULL)
> > -		return 0;
> > -	diag_set(ClientError, ER_NO_SUCH_MODULE, name);
> > +	if (module_reload(name, name + strlen(name), &module) == 0) {
> > +		if (module != NULL)
> > +			return 0;
> > +		else {
> > +			diag_set(ClientError, ER_NO_SUCH_MODULE, name);
> > +			return -1;
> 
> Redundant return -1? Since we're gonna return -1 a few lines lower anyway.
> I might be missing something obvious but I don't get why have you moved
> module != NULL into a separate block. If you prefer this form I don't
> mind though.
> 
> > +		}
> > +	}
> >  	return -1;
> >  }
> >  
> > diff --git a/src/box/func.c b/src/box/func.c
> > index a42a269..0405eb9 100644
> > --- a/src/box/func.c
> > +++ b/src/box/func.c
> > @@ -261,14 +261,29 @@ module_load(const char *package, const char *package_end)
> >  	module->package[package_len] = 0;
> >  	rlist_create(&module->funcs);
> >  	module->calls = 0;
> > -	char dir_name[] = "/tmp/tntXXXXXX";
> > +
> > +	const char *tmpdir = getenv("TMPDIR");
> > +	if (tmpdir == NULL)
> > +		tmpdir = "/tmp";
> > +	char dir_name[PATH_MAX + 1];
> 
> No need for PATH_MAX + 1, PATH_MAX should be enough. Other than this -- Ack.