aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorPatrick Ohly <patrick.ohly@intel.com>2016-11-15 15:16:08 +0100
committerPatrick Ohly <patrick.ohly@intel.com>2016-12-08 14:12:56 +0100
commitafe7643a8f43c84273f88d86c42bbce767e97ec4 (patch)
treef68a4c54821cc6984ee95fca8d99aa88dce876ef
parentddde21be154d12f37ac22613851a86625d270907 (diff)
downloadmeta-swupd-afe7643a8f43c84273f88d86c42bbce767e97ec4.tar.gz
meta-swupd-afe7643a8f43c84273f88d86c42bbce767e97ec4.tar.bz2
meta-swupd-afe7643a8f43c84273f88d86c42bbce767e97ec4.zip
swupd-client: speed up download of large files when using IMA
When IMA is active, the kernel ended up updating the file hash each time swupd wrote a chunk, because files were getting opened and closed for each chunk. Now they get opened before downloading and closed when done. Fixes: clearlinux/swupd-client/#41 Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
-rw-r--r--recipes-core/swupd-client/swupd-client/0001-downloads-minimize-syscalls-to-improve-performance.patch186
-rw-r--r--recipes-core/swupd-client/swupd-client/0002-downloads-open-FILE-in-advance-and-use-default-write.patch184
-rw-r--r--recipes-core/swupd-client/swupd-client_git.bb2
3 files changed, 372 insertions, 0 deletions
diff --git a/recipes-core/swupd-client/swupd-client/0001-downloads-minimize-syscalls-to-improve-performance.patch b/recipes-core/swupd-client/swupd-client/0001-downloads-minimize-syscalls-to-improve-performance.patch
new file mode 100644
index 0000000..59875b7
--- /dev/null
+++ b/recipes-core/swupd-client/swupd-client/0001-downloads-minimize-syscalls-to-improve-performance.patch
@@ -0,0 +1,186 @@
+From 9bc713b7ed0dba91304c5d7ed4905f5924ad8e42 Mon Sep 17 00:00:00 2001
+From: Patrick Ohly <patrick.ohly@intel.com>
+Date: Thu, 14 Apr 2016 11:03:31 +0200
+Subject: [PATCH 1/3] 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. To
+keep error handling as much as before, the completion function which has
+the close() takes the current curl error code and replaces it if it
+encounters a write error.
+
+[v2 of the patch with fixes by Dmitry Rozhkov, see https://github.com/pohly/swupd-client/pull/1]
+
+Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
+---
+ include/swupd.h | 3 +++
+ src/curl.c | 63 ++++++++++++++++++++++++++++++++++++++++-----------------
+ src/download.c | 8 +++++++-
+ 3 files changed, 54 insertions(+), 20 deletions(-)
+
+diff --git a/include/swupd.h b/include/swupd.h
+index 14e65ab..3bac8b2 100644
+--- a/include/swupd.h
++++ b/include/swupd.h
+@@ -89,6 +89,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;
+@@ -109,6 +110,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;
+@@ -199,6 +201,7 @@ extern void swupd_curl_set_current_version(int v);
+ extern void swupd_curl_set_requested_version(int v);
+ extern double swupd_query_url_content_size(char *url);
+ 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,
+ struct version_container *tmp_version, bool pack);
+ #define SWUPD_CURL_LOW_SPEED_LIMIT 1
+diff --git a/src/curl.c b/src/curl.c
+index b14193b..cab1ef2 100644
+--- a/src/curl.c
++++ b/src/curl.c
+@@ -165,35 +165,57 @@ 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) {
++ fprintf(stderr, "Cannot open file for write \\*outfile=\"%s\",strerror=\"%s\"*\\\n",
++ outfile, strerror(errno));
++ return -1;
++ }
++ file->fd = fd;
++ file->fd_valid = 1;
++ }
+
+- fd = open(outfile, O_CREAT | O_RDWR, 00600);
+- if (fd < 0) {
+- printf("Error: Cannot open %s for write: %s\n",
+- outfile, strerror(errno));
+- return -1;
++ /* handle short writes with repeated write() calls */
++ for (remaining = size * nmemb; remaining; remaining -= written) {
++ written = write(fd, ptr, size*nmemb);
++ if (written < 0) {
++ if (errno == EINTR) {
++ written = 0;
++ continue;
++ }
++ fprintf(stderr, "write error \\*outfile=\"%s\",strerror=\"%s\"*\\\n",
++ outfile, strerror(errno));
++ return -1;
++ }
+ }
+
+- f = fdopen(fd, "a");
+- if (!f) {
+- printf("Error: Cannot fdopen %s for write: %s\n",
+- outfile, strerror(errno));
+- close(fd);
++ if (fdatasync(fd)) {
++ fprintf(stderr, "fdatasync \\*outfile=\"%s\",strerror=\"%s\"*\\\n", outfile, strerror(errno));
+ return -1;
+ }
+
+- written = fwrite(ptr, size * nmemb, 1, f);
+-
+- fflush(f);
+- fclose(f);
++ return size*nmemb;
++}
+
+- if (written != 1) {
+- return -1;
++CURLcode swupd_download_file_complete(CURLcode curl_ret, struct file *file)
++{
++ if (file->fd_valid) {
++ if (close(file->fd)) {
++ fprintf(stderr, "Cannot close file after write \\*outfile=\"%s\",strerror=\"%s\"*\\\n",
++ 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
+@@ -281,6 +303,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 handle */
+ switch (ret) {
+diff --git a/src/download.c b/src/download.c
+index 6d81d81..c4a7a07 100644
+--- a/src/download.c
++++ b/src/download.c
+@@ -164,6 +164,7 @@ static void free_curl_list_data(void *data)
+ {
+ struct file *file = (struct file *)data;
+ CURL *curl = file->curl;
++ (void) swupd_download_file_complete(CURLE_OK, file);
+ if (curl != NULL) {
+ curl_multi_remove_handle(mcurl, curl);
+ curl_easy_cleanup(curl);
+@@ -368,9 +369,14 @@ static int perform_curl_io_and_complete(int *left)
+ continue;
+ }
+
++ /* Get error code from easy handle and augment it if
++ * completing the download encounters further problems. */
++ curl_ret = msg->data.result;
++ curl_ret = swupd_download_file_complete(curl_ret, file);
++
+ /* The easy handle may have an error set, even if the server returns
+ * HTTP 200, so retry the download for this case. */
+- if (ret == 200 && msg->data.result != CURLE_OK) {
++ if (ret == 200 && curl_ret != CURLE_OK) {
+ printf("Error for %s download: %s\n", file->hash,
+ curl_easy_strerror(msg->data.result));
+ failed = list_prepend_data(failed, file);
+--
+2.1.4
+
diff --git a/recipes-core/swupd-client/swupd-client/0002-downloads-open-FILE-in-advance-and-use-default-write.patch b/recipes-core/swupd-client/swupd-client/0002-downloads-open-FILE-in-advance-and-use-default-write.patch
new file mode 100644
index 0000000..4d8339a
--- /dev/null
+++ b/recipes-core/swupd-client/swupd-client/0002-downloads-open-FILE-in-advance-and-use-default-write.patch
@@ -0,0 +1,184 @@
+From 26c603ad25469d3e37fc00b78ad161b34093f5fa Mon Sep 17 00:00:00 2001
+From: Patrick Ohly <patrick.ohly@intel.com>
+Date: Tue, 15 Nov 2016 15:01:38 +0100
+Subject: [PATCH 2/3] downloads: open FILE in advance and use default write
+ handler
+
+Now that the number of pending downloads is kept below a certain limit
+(see poll_fewer_than()) it is possible to open files before starting
+the transfer. Using the default curl write handler and explicit
+open/close of the file makes the code simpler.
+
+Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
+---
+ include/swupd.h | 5 ++---
+ src/curl.c | 59 +++++++++++++--------------------------------------------
+ src/download.c | 6 ++++--
+ 3 files changed, 19 insertions(+), 51 deletions(-)
+
+diff --git a/include/swupd.h b/include/swupd.h
+index 3bac8b2..ed4b82c 100644
+--- a/include/swupd.h
++++ b/include/swupd.h
+@@ -89,7 +89,6 @@ 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;
+@@ -110,7 +109,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 */
++ FILE *fh; /* file written into during downloading */
+ };
+
+ extern bool download_only;
+@@ -200,7 +199,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 double swupd_query_url_content_size(char *url);
+-extern size_t swupd_download_file(void *ptr, size_t size, size_t nmemb, void *userdata);
++extern CURLcode swupd_download_file_start(struct file *file);
+ 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,
+ struct version_container *tmp_version, bool pack);
+diff --git a/src/curl.c b/src/curl.c
+index cab1ef2..009fdd5 100644
+--- a/src/curl.c
++++ b/src/curl.c
+@@ -158,62 +158,28 @@ static size_t swupd_download_version_to_memory(void *ptr, size_t size, size_t nm
+ return data_len;
+ }
+
+-/* curl easy CURLOPT_WRITEFUNCTION callback */
+-size_t swupd_download_file(void *ptr, size_t size, size_t nmemb, void *userdata)
++CURLcode swupd_download_file_start(struct file *file)
+ {
+- struct file *file = (struct file *)userdata;
+- const char *outfile;
+- int fd;
+- FILE *f;
+- 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) {
+- fprintf(stderr, "Cannot open file for write \\*outfile=\"%s\",strerror=\"%s\"*\\\n",
+- 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) {
+- if (errno == EINTR) {
+- written = 0;
+- continue;
+- }
+- fprintf(stderr, "write error \\*outfile=\"%s\",strerror=\"%s\"*\\\n",
+- outfile, strerror(errno));
+- return -1;
+- }
++ file->fh = fopen(file->staging, "w");
++ if (!file->fh) {
++ fprintf(stderr, "Cannot open file for write \\*outfile=\"%s\",strerror=\"%s\"*\\\n",
++ file->staging, strerror(errno));
++ return CURLE_WRITE_ERROR;
+ }
+-
+- if (fdatasync(fd)) {
+- fprintf(stderr, "fdatasync \\*outfile=\"%s\",strerror=\"%s\"*\\\n", outfile, strerror(errno));
+- return -1;
+- }
+-
+- return size*nmemb;
++ return CURLE_OK;
+ }
+
+ CURLcode swupd_download_file_complete(CURLcode curl_ret, struct file *file)
+ {
+- if (file->fd_valid) {
+- if (close(file->fd)) {
++ if (file->fh) {
++ if (fclose(file->fh)) {
+ fprintf(stderr, "Cannot close file after write \\*outfile=\"%s\",strerror=\"%s\"*\\\n",
+ file->staging, strerror(errno));
+ if (curl_ret == CURLE_OK) {
+ curl_ret = CURLE_WRITE_ERROR;
+ }
+ }
+- file->fd_valid = 0;
++ file->fh = NULL;
+ }
+ return curl_ret;
+ }
+@@ -246,6 +212,7 @@ int swupd_curl_get_file(const char *url, char *filename, struct file *file,
+
+ if (file) {
+ local = file;
++ local->fh = NULL;
+ } else {
+ local = calloc(1, sizeof(struct file));
+ if (!local) {
+@@ -266,11 +233,11 @@ int swupd_curl_get_file(const char *url, char *filename, struct file *file,
+ if (curl_ret != CURLE_OK) {
+ goto exit;
+ }
+- curl_ret = curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, swupd_download_file);
++ curl_ret = swupd_download_file_start(local);
+ if (curl_ret != CURLE_OK) {
+ goto exit;
+ }
+- curl_ret = curl_easy_setopt(curl, CURLOPT_WRITEDATA, (void *)local);
++ curl_ret = curl_easy_setopt(curl, CURLOPT_WRITEDATA, (void *)local->fh);
+ if (curl_ret != CURLE_OK) {
+ goto exit;
+ }
+diff --git a/src/download.c b/src/download.c
+index c4a7a07..397d56c 100644
+--- a/src/download.c
++++ b/src/download.c
+@@ -475,6 +475,7 @@ void full_download(struct file *file)
+ CURLMcode curlm_ret = CURLM_OK;
+ CURLcode curl_ret = CURLE_OK;
+
++ file->fh = NULL;
+ ret = swupd_curl_hashmap_insert(file);
+ if (ret > 0) { /* no download needed */
+ /* File already exists - report success */
+@@ -510,11 +511,11 @@ void full_download(struct file *file)
+ if (curl_ret != CURLE_OK) {
+ goto out_bad;
+ }
+- curl_ret = curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, swupd_download_file);
++ curl_ret = swupd_download_file_start(file);
+ if (curl_ret != CURLE_OK) {
+ goto out_bad;
+ }
+- curl_ret = curl_easy_setopt(curl, CURLOPT_WRITEDATA, (void *)file);
++ curl_ret = curl_easy_setopt(curl, CURLOPT_WRITEDATA, (void *)file->fh);
+ if (curl_ret != CURLE_OK) {
+ goto out_bad;
+ }
+@@ -536,6 +537,7 @@ void full_download(struct file *file)
+ goto out_good;
+
+ out_bad:
++ (void) swupd_download_file_complete(CURLE_OK, file);
+ failed = list_prepend_data(failed, file);
+ if (curl != NULL) {
+ /* Must remove handle out of multi queue first!*/
+--
+2.1.4
+
diff --git a/recipes-core/swupd-client/swupd-client_git.bb b/recipes-core/swupd-client/swupd-client_git.bb
index a039b0f..fd73223 100644
--- a/recipes-core/swupd-client/swupd-client_git.bb
+++ b/recipes-core/swupd-client/swupd-client_git.bb
@@ -12,6 +12,8 @@ SRC_URI = "\
file://0001-Add-configure-option-to-re-enable-updating-of-config.patch \
file://Make-pinned-pubkey-configurable.patch \
file://ignore-xattrs-when-verifying-Manifest-files.patch \
+ file://0001-downloads-minimize-syscalls-to-improve-performance.patch \
+ file://0002-downloads-open-FILE-in-advance-and-use-default-write.patch \
"
SRCREV = "f4000c5b22be47ec1af2f8748fd71a36148b5dc4"