1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
|
From 96d18d373436fda5c643abd8621d932002b2a007 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 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/37]
Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
---
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
|
