From 96d18d373436fda5c643abd8621d932002b2a007 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Thu, 14 Apr 2016 11:03:31 +0200 Subject: [PATCH 2/2] downloads: minimize syscalls to improve performance The previous approach was to open/fdopen/fclose the file for each chunk that gets passed from curl. This incurrs a huge performance hit when close() triggers a hashing of the file content on systems where integrity protection via IMA is enabled. Now the file is opened only once and kept open until the download is complete. In addition, the unnecessary usage of C file IO is avoided. The semantic is changed as little as possible: - file gets created only after the first chunk of data arrived - file descriptors do not leak to child processes (O_CLOEXEC) - data gets appended to existing files (via O_APPEND, used to keep the code simple and avoid an additional lseek) - data gets flushed explicitly for each chunk (via fdatasync(), which somewhat approximates the effect that an explicit close() may have had) As an additional improvement, failures during close() are checked when downloading single files. However, perform_curl_io_and_complete() still ignores the error. Upstream-Status: Submitted [https://github.com/clearlinux/swupd-client/issues/41] Signed-off-by: Patrick Ohly --- include/swupd.h | 3 +++ src/curl.c | 69 ++++++++++++++++++++++++++++++++++++--------------------- src/download.c | 7 ++++++ 3 files changed, 54 insertions(+), 25 deletions(-) diff --git a/include/swupd.h b/include/swupd.h index 554e795..fbaa5ae 100644 --- a/include/swupd.h +++ b/include/swupd.h @@ -82,6 +82,7 @@ struct file { int last_change; struct update_stat stat; + unsigned int fd_valid : 1; unsigned int is_dir : 1; unsigned int is_file : 1; unsigned int is_link : 1; @@ -101,6 +102,7 @@ struct file { char *staging; /* output name used during download & staging */ CURL *curl; /* curl handle if downloading */ + int fd; /* file written into during downloading, unset when fd_valid is false */ }; extern bool download_only; @@ -183,6 +185,7 @@ extern void swupd_curl_cleanup(void); extern void swupd_curl_set_current_version(int v); extern void swupd_curl_set_requested_version(int v); extern size_t swupd_download_file(void *ptr, size_t size, size_t nmemb, void *userdata); +extern CURLcode swupd_download_file_complete(CURLcode curl_ret, struct file *file); extern int swupd_curl_get_file(const char *url, char *filename, struct file *file, char *tmp_version, bool pack); #define SWUPD_CURL_LOW_SPEED_LIMIT 1 diff --git a/src/curl.c b/src/curl.c index c989426..c4e1398 100644 --- a/src/curl.c +++ b/src/curl.c @@ -122,37 +122,54 @@ size_t swupd_download_file(void *ptr, size_t size, size_t nmemb, void *userdata) const char *outfile; int fd; FILE *f; - size_t written; + size_t written, remaining; outfile = file->staging; + if (file->fd_valid) { + fd = file->fd; + } else { + fd = open(outfile, O_CREAT | O_RDWR | O_CLOEXEC | O_APPEND, 00600); + if (fd < 0) { + LOG_ERROR(file, "Cannot open file for write", class_file_io, + "\\*outfile=\"%s\",strerror=\"%s\"*\\", outfile, strerror(errno)); + return -1; + } + file->fd = fd; + file->fd_valid = 1; + } + + /* handle short writes with repeated write() calls */ + for (remaining = size*nmemb; remaining; remaining -= written) { + written = write(fd, ptr, size*nmemb); + if (written < 0) { + LOG_ERROR(file, "write error", class_file_io, + "\\*outfile=\"%s\",strerror=\"%s\"*\\", outfile, strerror(errno)); + return -1; + } + } - fd = open(outfile, O_CREAT | O_RDWR , 00600); - if (fd < 0) { - LOG_ERROR(file, "Cannot open file for write", class_file_io, - "\\*outfile=\"%s\",strerror=\"%s\"*\\", outfile, strerror(errno)); + if (fdatasync(fd)) { + LOG_ERROR(file, "fdatasync", class_file_io, + "\\*outfile=\"%s\",strerror=\"%s\"*\\", outfile, strerror(errno)); return -1; - } + } - f = fdopen(fd, "a"); - if (!f) { - LOG_ERROR(file, "Cannot fdopen file for write", class_file_io, - "\\*outfile=\"%s\",strerror=\"%s\"*\\", outfile, strerror(errno)); - close(fd); - return -1; - } - - written = fwrite(ptr, size*nmemb, 1, f); - - fflush(f); - fclose(f); + return size*nmemb; +} - if (written != 1) { - LOG_ERROR(file, "short write", class_file_io, - "\\*outfile=\"%s\",strerror=\"%s\"*\\", outfile, strerror(errno)); - return -1; +CURLcode swupd_download_file_complete(CURLcode curl_ret, struct file *file) +{ + if (file->fd_valid) { + if (close(file->fd)) { + LOG_ERROR(file, "Cannot close file after write", class_file_io, + "\\*outfile=\"%s\",strerror=\"%s\"*\\", file->staging, strerror(errno)); + if (curl_ret == CURLE_OK) { + curl_ret = CURLE_WRITE_ERROR; + } + } + file->fd_valid = 0; } - - return size*nmemb; + return curl_ret; } /* Download a single file SYNCHRONOUSLY @@ -193,7 +210,6 @@ int swupd_curl_get_file(const char *url, char *filename, struct file *file, } } local->staging = filename; - if (lstat(filename, &stat) == 0) { if (pack) { curl_ret = curl_easy_setopt(curl, CURLOPT_RESUME_FROM_LARGE, (curl_off_t) stat.st_size); @@ -263,6 +279,9 @@ int swupd_curl_get_file(const char *url, char *filename, struct file *file, } exit: + if (local) { + curl_ret = swupd_download_file_complete(curl_ret, local); + } if (curl_ret == CURLE_OK) { /* curl command succeeded, download might've failed, let our caller LOG_ */ switch (ret) { diff --git a/src/download.c b/src/download.c index 211ee24..2f88fb1 100644 --- a/src/download.c +++ b/src/download.c @@ -165,6 +165,7 @@ static void free_curl_list_data(void *data) struct file *file = (struct file*)data; CURL *curl = file->curl; + swupd_download_file_complete(CURLE_OK, file); curl_multi_remove_handle(mcurl, curl); curl_easy_cleanup(curl); } @@ -379,6 +380,12 @@ static int perform_curl_io_and_complete(int *left) continue; } + curl_ret = swupd_download_file_complete(curl_ret, file); + /* + * Any error as logged already. Let's continue and + * hope for the best... or abort via "return -1"? + */ + if (ret == 200) { untar_full_download(file); } else { -- 2.1.4