* [Tarantool-patches] [PATCH 0/2] Fix C module reloading @ 2020-05-20 9:51 Kirill Yukhin 2020-05-20 9:51 ` [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing Kirill Yukhin ` (3 more replies) 0 siblings, 4 replies; 19+ messages in thread From: Kirill Yukhin @ 2020-05-20 9:51 UTC (permalink / raw) To: tarantool-patches 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing 2020-05-20 9:51 [Tarantool-patches] [PATCH 0/2] Fix C module reloading Kirill Yukhin @ 2020-05-20 9:51 ` Kirill Yukhin 2020-05-23 18:30 ` Konstantin Osipov 2020-05-20 9:51 ` [Tarantool-patches] [PATCH 2/2] Allow to set directory for copying DSO before load Kirill Yukhin ` (2 subsequent siblings) 3 siblings, 1 reply; 19+ messages in thread From: Kirill Yukhin @ 2020-05-20 9:51 UTC (permalink / raw) To: tarantool-patches 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing 2020-05-20 9:51 ` [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing Kirill Yukhin @ 2020-05-23 18:30 ` Konstantin Osipov 2020-05-25 13:13 ` Kirill Yukhin 0 siblings, 1 reply; 19+ messages in thread From: Konstantin Osipov @ 2020-05-23 18:30 UTC (permalink / raw) To: Kirill Yukhin; +Cc: tarantool-patches * 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing 2020-05-23 18:30 ` Konstantin Osipov @ 2020-05-25 13:13 ` Kirill Yukhin 2020-05-25 14:34 ` Konstantin Osipov 2020-05-26 15:16 ` Cyrill Gorcunov 0 siblings, 2 replies; 19+ messages in thread From: Kirill Yukhin @ 2020-05-25 13:13 UTC (permalink / raw) To: Konstantin Osipov, tarantool-patches 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) ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing 2020-05-25 13:13 ` Kirill Yukhin @ 2020-05-25 14:34 ` Konstantin Osipov 2020-05-25 15:18 ` Cyrill Gorcunov 2020-05-26 11:11 ` Kirill Yukhin 2020-05-26 15:16 ` Cyrill Gorcunov 1 sibling, 2 replies; 19+ messages in thread From: Konstantin Osipov @ 2020-05-25 14:34 UTC (permalink / raw) To: Kirill Yukhin; +Cc: tarantool-patches * 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing 2020-05-25 14:34 ` Konstantin Osipov @ 2020-05-25 15:18 ` Cyrill Gorcunov 2020-05-25 15:26 ` Cyrill Gorcunov 2020-05-25 16:45 ` Konstantin Osipov 2020-05-26 11:11 ` Kirill Yukhin 1 sibling, 2 replies; 19+ messages in thread From: Cyrill Gorcunov @ 2020-05-25 15:18 UTC (permalink / raw) To: Konstantin Osipov; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing 2020-05-25 15:18 ` Cyrill Gorcunov @ 2020-05-25 15:26 ` Cyrill Gorcunov 2020-05-25 16:45 ` Konstantin Osipov 1 sibling, 0 replies; 19+ messages in thread From: Cyrill Gorcunov @ 2020-05-25 15:26 UTC (permalink / raw) To: Konstantin Osipov; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing 2020-05-25 15:18 ` Cyrill Gorcunov 2020-05-25 15:26 ` Cyrill Gorcunov @ 2020-05-25 16:45 ` Konstantin Osipov 2020-05-25 18:38 ` Cyrill Gorcunov 1 sibling, 1 reply; 19+ messages in thread From: Konstantin Osipov @ 2020-05-25 16:45 UTC (permalink / raw) To: Cyrill Gorcunov; +Cc: tarantool-patches * 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing 2020-05-25 16:45 ` Konstantin Osipov @ 2020-05-25 18:38 ` Cyrill Gorcunov 0 siblings, 0 replies; 19+ messages in thread From: Cyrill Gorcunov @ 2020-05-25 18:38 UTC (permalink / raw) To: Konstantin Osipov, Kirill Yukhin, tarantool-patches 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing 2020-05-25 14:34 ` Konstantin Osipov 2020-05-25 15:18 ` Cyrill Gorcunov @ 2020-05-26 11:11 ` Kirill Yukhin 1 sibling, 0 replies; 19+ messages in thread From: Kirill Yukhin @ 2020-05-26 11:11 UTC (permalink / raw) To: Konstantin Osipov, tarantool-patches 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing 2020-05-25 13:13 ` Kirill Yukhin 2020-05-25 14:34 ` Konstantin Osipov @ 2020-05-26 15:16 ` Cyrill Gorcunov 2020-05-27 9:17 ` Kirill Yukhin 1 sibling, 1 reply; 19+ messages in thread From: Cyrill Gorcunov @ 2020-05-26 15:16 UTC (permalink / raw) To: Kirill Yukhin; +Cc: tarantool-patches 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). ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing 2020-05-26 15:16 ` Cyrill Gorcunov @ 2020-05-27 9:17 ` Kirill Yukhin 2020-05-27 10:49 ` Cyrill Gorcunov 0 siblings, 1 reply; 19+ messages in thread From: Kirill Yukhin @ 2020-05-27 9:17 UTC (permalink / raw) To: Cyrill Gorcunov; +Cc: tarantool-patches 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) ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing 2020-05-27 9:17 ` Kirill Yukhin @ 2020-05-27 10:49 ` Cyrill Gorcunov 0 siblings, 0 replies; 19+ messages in thread From: Cyrill Gorcunov @ 2020-05-27 10:49 UTC (permalink / raw) To: Kirill Yukhin; +Cc: tarantool-patches 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> ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Tarantool-patches] [PATCH 2/2] Allow to set directory for copying DSO before load 2020-05-20 9:51 [Tarantool-patches] [PATCH 0/2] Fix C module reloading Kirill Yukhin 2020-05-20 9:51 ` [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing Kirill Yukhin @ 2020-05-20 9:51 ` Kirill Yukhin 2020-05-29 10:59 ` Cyrill Gorcunov 2020-05-20 20:41 ` [Tarantool-patches] [PATCH 0/2] Fix C module reloading Nikita Pettik 2020-06-01 10:52 ` Kirill Yukhin 3 siblings, 1 reply; 19+ messages in thread From: Kirill Yukhin @ 2020-05-20 9:51 UTC (permalink / raw) To: tarantool-patches 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] Allow to set directory for copying DSO before load 2020-05-20 9:51 ` [Tarantool-patches] [PATCH 2/2] Allow to set directory for copying DSO before load Kirill Yukhin @ 2020-05-29 10:59 ` Cyrill Gorcunov 2020-06-01 10:53 ` Kirill Yukhin 0 siblings, 1 reply; 19+ messages in thread From: Cyrill Gorcunov @ 2020-05-29 10:59 UTC (permalink / raw) To: Kirill Yukhin; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] Allow to set directory for copying DSO before load 2020-05-29 10:59 ` Cyrill Gorcunov @ 2020-06-01 10:53 ` Kirill Yukhin 0 siblings, 0 replies; 19+ messages in thread From: Kirill Yukhin @ 2020-06-01 10:53 UTC (permalink / raw) To: Cyrill Gorcunov; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/2] Fix C module reloading 2020-05-20 9:51 [Tarantool-patches] [PATCH 0/2] Fix C module reloading Kirill Yukhin 2020-05-20 9:51 ` [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing Kirill Yukhin 2020-05-20 9:51 ` [Tarantool-patches] [PATCH 2/2] Allow to set directory for copying DSO before load Kirill Yukhin @ 2020-05-20 20:41 ` Nikita Pettik 2020-05-21 8:41 ` Kirill Yukhin 2020-06-01 10:52 ` Kirill Yukhin 3 siblings, 1 reply; 19+ messages in thread From: Nikita Pettik @ 2020-05-20 20:41 UTC (permalink / raw) To: Kirill Yukhin; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/2] Fix C module reloading 2020-05-20 20:41 ` [Tarantool-patches] [PATCH 0/2] Fix C module reloading Nikita Pettik @ 2020-05-21 8:41 ` Kirill Yukhin 0 siblings, 0 replies; 19+ messages in thread From: Kirill Yukhin @ 2020-05-21 8:41 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/2] Fix C module reloading 2020-05-20 9:51 [Tarantool-patches] [PATCH 0/2] Fix C module reloading Kirill Yukhin ` (2 preceding siblings ...) 2020-05-20 20:41 ` [Tarantool-patches] [PATCH 0/2] Fix C module reloading Nikita Pettik @ 2020-06-01 10:52 ` Kirill Yukhin 3 siblings, 0 replies; 19+ messages in thread From: Kirill Yukhin @ 2020-06-01 10:52 UTC (permalink / raw) To: tarantool-patches 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2020-06-01 10:53 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-20 9:51 [Tarantool-patches] [PATCH 0/2] Fix C module reloading Kirill Yukhin 2020-05-20 9:51 ` [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing Kirill Yukhin 2020-05-23 18:30 ` Konstantin Osipov 2020-05-25 13:13 ` Kirill Yukhin 2020-05-25 14:34 ` Konstantin Osipov 2020-05-25 15:18 ` Cyrill Gorcunov 2020-05-25 15:26 ` Cyrill Gorcunov 2020-05-25 16:45 ` Konstantin Osipov 2020-05-25 18:38 ` Cyrill Gorcunov 2020-05-26 11:11 ` Kirill Yukhin 2020-05-26 15:16 ` Cyrill Gorcunov 2020-05-27 9:17 ` Kirill Yukhin 2020-05-27 10:49 ` Cyrill Gorcunov 2020-05-20 9:51 ` [Tarantool-patches] [PATCH 2/2] Allow to set directory for copying DSO before load Kirill Yukhin 2020-05-29 10:59 ` Cyrill Gorcunov 2020-06-01 10:53 ` Kirill Yukhin 2020-05-20 20:41 ` [Tarantool-patches] [PATCH 0/2] Fix C module reloading Nikita Pettik 2020-05-21 8:41 ` Kirill Yukhin 2020-06-01 10:52 ` Kirill Yukhin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox