From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id B09CC469710 for ; Mon, 25 May 2020 16:13:55 +0300 (MSK) Date: Mon, 25 May 2020 16:13:54 +0300 From: Kirill Yukhin Message-ID: <20200525131354.junz5joymznc6r6j@tarantool.org> References: <0f65635ef9ee95131ac0e83b0b70e8c204a322b8.1589968157.git.kyukhin@tarantool.org> <20200523183032.GB2714@atlas> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200523183032.GB2714@atlas> Subject: Re: [Tarantool-patches] [PATCH 1/2] Copy DSO module before load instead of symlink-ing List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Konstantin Osipov , tarantool-patches@dev.tarantool.org 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 [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 #include /** @@ -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)