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

[Kirill Yukhin <kyukhin@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 <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)