* [PATCH 0/5] Five fio patches
@ 2019-08-13 2:01 Bart Van Assche
2019-08-13 2:01 ` [PATCH 1/5] Optimize the code that copies strings Bart Van Assche
` (4 more replies)
0 siblings, 5 replies; 16+ messages in thread
From: Bart Van Assche @ 2019-08-13 2:01 UTC (permalink / raw)
To: Jens Axboe; +Cc: fio, Damien Le Moal, Bart Van Assche
Hi Jens,
The following changes are included in this patch series:
- Use snprintf() instead of strncpy() + explicit null termination.
- Rework the approach for suppressing gcc 9 address of packed member warnings.
- Make Travis run the zbd tests.
Please consider these patches for the official fio git repository.
Thanks,
Bart.
Bart Van Assche (5):
Optimize the code that copies strings
Verify the size of struct jobs_eta at compile time
Refine packed annotations in stat.h
Revert "eta: Fix compiler warning"
make test: Also run zbd tests
.travis.yml | 3 +++
Makefile | 14 ++++++++++++++
cconv.c | 7 +++----
client.c | 5 +++--
diskutil.c | 9 ++++-----
engines/net.c | 6 ++----
engines/sg.c | 4 ++--
eta.c | 11 +++++------
filesetup.c | 6 ++----
fio.h | 2 ++
gclient.c | 4 ++--
init.c | 19 +++++--------------
ioengines.c | 3 +--
options.c | 3 +--
parse.c | 6 ++----
server.c | 26 +++++++++++---------------
stat.c | 15 ++++++++-------
stat.h | 11 ++++++-----
verify.c | 3 +--
19 files changed, 77 insertions(+), 80 deletions(-)
--
2.22.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/5] Optimize the code that copies strings
2019-08-13 2:01 [PATCH 0/5] Five fio patches Bart Van Assche
@ 2019-08-13 2:01 ` Bart Van Assche
2019-08-13 2:01 ` [PATCH 2/5] Verify the size of struct jobs_eta at compile time Bart Van Assche
` (3 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Bart Van Assche @ 2019-08-13 2:01 UTC (permalink / raw)
To: Jens Axboe; +Cc: fio, Damien Le Moal, Bart Van Assche
Using strncpy() to copy strings is suboptimal because strncpy writes a
bunch of additional unnecessary null bytes. Use snprintf() instead of
strncpy(). An additional advantage of snprintf() is that it guarantees
that the output string is '\0'-terminated.
This patch is an improvement for commit 32e31c8c5f7b ("Fix string copy
compilation warnings").
Cc: Damien Le Moal <damien.lemoal@wdc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
cconv.c | 7 +++----
client.c | 5 +++--
diskutil.c | 9 ++++-----
engines/net.c | 6 ++----
engines/sg.c | 4 ++--
filesetup.c | 6 ++----
gclient.c | 4 ++--
init.c | 19 +++++--------------
ioengines.c | 3 +--
options.c | 3 +--
parse.c | 6 ++----
server.c | 26 +++++++++++---------------
stat.c | 15 ++++++++-------
verify.c | 3 +--
14 files changed, 47 insertions(+), 69 deletions(-)
diff --git a/cconv.c b/cconv.c
index 50e45c63a636..0e6572462788 100644
--- a/cconv.c
+++ b/cconv.c
@@ -13,10 +13,9 @@ static void string_to_cpu(char **dst, const uint8_t *src)
static void __string_to_net(uint8_t *dst, const char *src, size_t dst_size)
{
- if (src) {
- dst[dst_size - 1] = '\0';
- strncpy((char *) dst, src, dst_size - 1);
- } else
+ if (src)
+ snprintf((char *) dst, dst_size, "%s", src);
+ else
dst[0] = '\0';
}
diff --git a/client.c b/client.c
index 43cfbd43b9d3..e0047af06a3e 100644
--- a/client.c
+++ b/client.c
@@ -520,7 +520,7 @@ static void probe_client(struct fio_client *client)
sname = server_name(client, buf, sizeof(buf));
memset(pdu.server, 0, sizeof(pdu.server));
- strncpy((char *) pdu.server, sname, sizeof(pdu.server) - 1);
+ snprintf((char *) pdu.server, sizeof(pdu.server), "%s", sname);
fio_net_send_cmd(client->fd, FIO_NET_CMD_PROBE, &pdu, sizeof(pdu), &tag, &client->cmd_list);
}
@@ -574,7 +574,8 @@ static int fio_client_connect_sock(struct fio_client *client)
memset(addr, 0, sizeof(*addr));
addr->sun_family = AF_UNIX;
- strncpy(addr->sun_path, client->hostname, sizeof(addr->sun_path) - 1);
+ snprintf(addr->sun_path, sizeof(addr->sun_path), "%s",
+ client->hostname);
fd = socket(AF_UNIX, SOCK_STREAM, 0);
if (fd < 0) {
diff --git a/diskutil.c b/diskutil.c
index 7be4c022431e..f074401501ba 100644
--- a/diskutil.c
+++ b/diskutil.c
@@ -181,8 +181,7 @@ static int get_device_numbers(char *file_name, int *maj, int *min)
/*
* must be a file, open "." in that path
*/
- tempname[PATH_MAX - 1] = '\0';
- strncpy(tempname, file_name, PATH_MAX - 1);
+ snprintf(tempname, ARRAY_SIZE(tempname), "%s", file_name);
p = dirname(tempname);
if (stat(p, &st)) {
perror("disk util stat");
@@ -314,7 +313,8 @@ static struct disk_util *disk_util_add(struct thread_data *td, int majdev,
sfree(du);
return NULL;
}
- strncpy((char *) du->dus.name, basename(path), FIO_DU_NAME_SZ - 1);
+ snprintf((char *) du->dus.name, ARRAY_SIZE(du->dus.name), "%s",
+ basename(path));
du->sysfs_root = strdup(path);
du->major = majdev;
du->minor = mindev;
@@ -435,8 +435,7 @@ static struct disk_util *__init_per_file_disk_util(struct thread_data *td,
log_err("unknown sysfs layout\n");
return NULL;
}
- tmp[PATH_MAX - 1] = '\0';
- strncpy(tmp, p, PATH_MAX - 1);
+ snprintf(tmp, ARRAY_SIZE(tmp), "%s", p);
sprintf(path, "%s", tmp);
}
diff --git a/engines/net.c b/engines/net.c
index ca6fb344b897..91f25774690a 100644
--- a/engines/net.c
+++ b/engines/net.c
@@ -1105,8 +1105,7 @@ static int fio_netio_setup_connect_unix(struct thread_data *td,
struct sockaddr_un *soun = &nd->addr_un;
soun->sun_family = AF_UNIX;
- memset(soun->sun_path, 0, sizeof(soun->sun_path));
- strncpy(soun->sun_path, path, sizeof(soun->sun_path) - 1);
+ snprintf(soun->sun_path, sizeof(soun->sun_path), "%s", path);
return 0;
}
@@ -1135,9 +1134,8 @@ static int fio_netio_setup_listen_unix(struct thread_data *td, const char *path)
mode = umask(000);
- memset(addr, 0, sizeof(*addr));
addr->sun_family = AF_UNIX;
- strncpy(addr->sun_path, path, sizeof(addr->sun_path) - 1);
+ snprintf(addr->sun_path, sizeof(addr->sun_path), "%s", path);
unlink(path);
len = sizeof(addr->sun_family) + strlen(path) + 1;
diff --git a/engines/sg.c b/engines/sg.c
index c46b9abaec87..a1a6de4ce248 100644
--- a/engines/sg.c
+++ b/engines/sg.c
@@ -1181,8 +1181,8 @@ static char *fio_sgio_errdetails(struct io_u *io_u)
}
if (!(hdr->info & SG_INFO_CHECK) && !strlen(msg))
- strncpy(msg, "SG Driver did not report a Host, Driver or Device check",
- MAXERRDETAIL - 1);
+ snprintf(msg, MAXERRDETAIL, "%s",
+ "SG Driver did not report a Host, Driver or Device check");
return msg;
}
diff --git a/filesetup.c b/filesetup.c
index 17fa31fb3cd4..57eca1bf5bf0 100644
--- a/filesetup.c
+++ b/filesetup.c
@@ -805,8 +805,7 @@ static unsigned long long get_fs_free_counts(struct thread_data *td)
} else if (f->filetype != FIO_TYPE_FILE)
continue;
- buf[255] = '\0';
- strncpy(buf, f->file_name, 255);
+ snprintf(buf, ARRAY_SIZE(buf), "%s", f->file_name);
if (stat(buf, &sb) < 0) {
if (errno != ENOENT)
@@ -829,8 +828,7 @@ static unsigned long long get_fs_free_counts(struct thread_data *td)
continue;
fm = calloc(1, sizeof(*fm));
- strncpy(fm->__base, buf, sizeof(fm->__base));
- fm->__base[255] = '\0';
+ snprintf(fm->__base, ARRAY_SIZE(fm->__base), "%s", buf);
fm->base = basename(fm->__base);
fm->key = sb.st_dev;
flist_add(&fm->list, &list);
diff --git a/gclient.c b/gclient.c
index 04275a1384c2..64324177ef1a 100644
--- a/gclient.c
+++ b/gclient.c
@@ -318,7 +318,7 @@ static void gfio_update_thread_status(struct gui_entry *ge,
static char message[100];
const char *m = message;
- strncpy(message, status_message, sizeof(message) - 1);
+ snprintf(message, sizeof(message), "%s", status_message);
gtk_progress_bar_set_text(GTK_PROGRESS_BAR(ge->thread_status_pb), m);
gtk_progress_bar_set_fraction(GTK_PROGRESS_BAR(ge->thread_status_pb), perc / 100.0);
gtk_widget_queue_draw(ge->ui->window);
@@ -330,7 +330,7 @@ static void gfio_update_thread_status_all(struct gui *ui, char *status_message,
static char message[100];
const char *m = message;
- strncpy(message, status_message, sizeof(message) - 1);
+ strncpy(message, sizeof(message), "%s", status_message);
gtk_progress_bar_set_text(GTK_PROGRESS_BAR(ui->thread_status_pb), m);
gtk_progress_bar_set_fraction(GTK_PROGRESS_BAR(ui->thread_status_pb), perc / 100.0);
gtk_widget_queue_draw(ui->window);
diff --git a/init.c b/init.c
index c9f6198ea63a..63f2168eabcb 100644
--- a/init.c
+++ b/init.c
@@ -1273,8 +1273,7 @@ static char *make_filename(char *buf, size_t buf_size,struct thread_options *o,
for (f = &fpre_keywords[0]; f->keyword; f++)
f->strlen = strlen(f->keyword);
- buf[buf_size - 1] = '\0';
- strncpy(buf, o->filename_format, buf_size - 1);
+ snprintf(buf, buf_size, "%s", o->filename_format);
memset(copy, 0, sizeof(copy));
for (f = &fpre_keywords[0]; f->keyword; f++) {
@@ -1353,7 +1352,7 @@ static char *make_filename(char *buf, size_t buf_size,struct thread_options *o,
if (post_start)
strncpy(dst, buf + post_start, dst_left);
- strncpy(buf, copy, buf_size - 1);
+ snprintf(buf, buf_size, "%s", copy);
} while (1);
}
@@ -2029,20 +2028,12 @@ static int __parse_jobs_ini(struct thread_data *td,
*/
if (access(filename, F_OK) &&
(ts = strrchr(file, '/'))) {
- int len = ts - file +
- strlen(filename) + 2;
-
- if (!(full_fn = calloc(1, len))) {
+ if (asprintf(&full_fn, "%.*s%s",
+ (int)(ts - file + 1), file,
+ filename) < 0) {
ret = ENOMEM;
break;
}
-
- strncpy(full_fn,
- file, (ts - file) + 1);
- strncpy(full_fn + (ts - file) + 1,
- filename,
- len - (ts - file) - 1);
- full_fn[len - 1] = 0;
filename = full_fn;
}
diff --git a/ioengines.c b/ioengines.c
index aa4ccd2755c9..40fa75c382b4 100644
--- a/ioengines.c
+++ b/ioengines.c
@@ -125,8 +125,7 @@ static struct ioengine_ops *__load_ioengine(const char *name)
{
char engine[64];
- engine[sizeof(engine) - 1] = '\0';
- strncpy(engine, name, sizeof(engine) - 1);
+ snprintf(engine, sizeof(engine), "%s", name);
/*
* linux libaio has alias names, so convert to what we want
diff --git a/options.c b/options.c
index f4c9bedf377f..447f231e7148 100644
--- a/options.c
+++ b/options.c
@@ -4902,8 +4902,7 @@ char *fio_option_dup_subs(const char *opt)
return NULL;
}
- in[OPT_LEN_MAX] = '\0';
- strncpy(in, opt, OPT_LEN_MAX);
+ snprintf(in, sizeof(in), "%s", opt);
while (*inptr && nchr > 0) {
if (inptr[0] == '$' && inptr[1] == '{') {
diff --git a/parse.c b/parse.c
index a7d4516e4702..c4fd4626df9f 100644
--- a/parse.c
+++ b/parse.c
@@ -602,8 +602,7 @@ static int __handle_option(const struct fio_option *o, const char *ptr,
if (!is_time && o->is_time)
is_time = o->is_time;
- tmp[sizeof(tmp) - 1] = '\0';
- strncpy(tmp, ptr, sizeof(tmp) - 1);
+ snprintf(tmp, sizeof(tmp), "%s", ptr);
p = strchr(tmp, ',');
if (p)
*p = '\0';
@@ -829,8 +828,7 @@ static int __handle_option(const struct fio_option *o, const char *ptr,
char tmp[128];
char *p1, *p2;
- tmp[sizeof(tmp) - 1] = '\0';
- strncpy(tmp, ptr, sizeof(tmp) - 1);
+ snprintf(tmp, sizeof(tmp), "%s", ptr);
/* Handle bsrange with separate read,write values: */
p1 = strchr(tmp, ',');
diff --git a/server.c b/server.c
index 23e549a590a7..e7846227f2b1 100644
--- a/server.c
+++ b/server.c
@@ -865,7 +865,8 @@ static int handle_probe_cmd(struct fio_net_cmd *cmd)
strcpy(me, (char *) pdu->server);
gethostname((char *) probe.hostname, sizeof(probe.hostname));
- strncpy((char *) probe.fio_version, fio_version_string, sizeof(probe.fio_version) - 1);
+ snprintf((char *) probe.fio_version, sizeof(probe.fio_version), "%s",
+ fio_version_string);
/*
* If the client supports compression and we do too, then enable it
@@ -1470,12 +1471,10 @@ void fio_server_send_ts(struct thread_stat *ts, struct group_run_stats *rs)
memset(&p, 0, sizeof(p));
- strncpy(p.ts.name, ts->name, FIO_JOBNAME_SIZE);
- p.ts.name[FIO_JOBNAME_SIZE - 1] = '\0';
- strncpy(p.ts.verror, ts->verror, FIO_VERROR_SIZE);
- p.ts.verror[FIO_VERROR_SIZE - 1] = '\0';
- strncpy(p.ts.description, ts->description, FIO_JOBDESC_SIZE);
- p.ts.description[FIO_JOBDESC_SIZE - 1] = '\0';
+ snprintf(p.ts.name, sizeof(p.ts.name), "%s", ts->name);
+ snprintf(p.ts.verror, sizeof(p.ts.verror), "%s", ts->verror);
+ snprintf(p.ts.description, sizeof(p.ts.description), "%s",
+ ts->description);
p.ts.error = cpu_to_le32(ts->error);
p.ts.thread_number = cpu_to_le32(ts->thread_number);
@@ -1666,8 +1665,7 @@ static void convert_dus(struct disk_util_stat *dst, struct disk_util_stat *src)
{
int i;
- dst->name[FIO_DU_NAME_SZ - 1] = '\0';
- strncpy((char *) dst->name, (char *) src->name, FIO_DU_NAME_SZ - 1);
+ snprintf((char *) dst->name, sizeof(dst->name), "%s", src->name);
for (i = 0; i < 2; i++) {
dst->s.ios[i] = cpu_to_le64(src->s.ios[i]);
@@ -1977,8 +1975,7 @@ int fio_send_iolog(struct thread_data *td, struct io_log *log, const char *name)
else
pdu.compressed = 0;
- strncpy((char *) pdu.name, name, FIO_NET_NAME_MAX);
- pdu.name[FIO_NET_NAME_MAX - 1] = '\0';
+ snprintf((char *) pdu.name, sizeof(pdu.name), "%s", name);
/*
* We can't do this for a pre-compressed log, but for that case,
@@ -2195,9 +2192,8 @@ static int fio_init_server_sock(void)
mode = umask(000);
- memset(&addr, 0, sizeof(addr));
addr.sun_family = AF_UNIX;
- strncpy(addr.sun_path, bind_sock, sizeof(addr.sun_path) - 1);
+ snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", bind_sock);
len = sizeof(addr.sun_family) + strlen(bind_sock) + 1;
@@ -2247,9 +2243,9 @@ static int fio_init_server_connection(void)
if (p)
strcat(p, port);
else
- strncpy(bind_str, port, sizeof(bind_str) - 1);
+ snprintf(bind_str, sizeof(bind_str), "%s", port);
} else
- strncpy(bind_str, bind_sock, sizeof(bind_str) - 1);
+ snprintf(bind_str, sizeof(bind_str), "%s", bind_sock);
log_info("fio: server listening on %s\n", bind_str);
diff --git a/stat.c b/stat.c
index bf87917c2956..33637900df62 100644
--- a/stat.c
+++ b/stat.c
@@ -1828,10 +1828,11 @@ void __show_run_stats(void)
/*
* These are per-group shared already
*/
- strncpy(ts->name, td->o.name, FIO_JOBNAME_SIZE - 1);
+ snprintf(ts->name, sizeof(ts->name), "%s", td->o.name);
if (td->o.description)
- strncpy(ts->description, td->o.description,
- FIO_JOBDESC_SIZE - 1);
+ snprintf(ts->description,
+ sizeof(ts->description), "%s",
+ td->o.description);
else
memset(ts->description, 0, FIO_JOBDESC_SIZE);
@@ -1868,12 +1869,12 @@ void __show_run_stats(void)
if (!td->error && td->o.continue_on_error &&
td->first_error) {
ts->error = td->first_error;
- ts->verror[sizeof(ts->verror) - 1] = '\0';
- strncpy(ts->verror, td->verror, sizeof(ts->verror) - 1);
+ snprintf(ts->verror, sizeof(ts->verror), "%s",
+ td->verror);
} else if (td->error) {
ts->error = td->error;
- ts->verror[sizeof(ts->verror) - 1] = '\0';
- strncpy(ts->verror, td->verror, sizeof(ts->verror) - 1);
+ snprintf(ts->verror, sizeof(ts->verror), "%s",
+ td->verror);
}
}
diff --git a/verify.c b/verify.c
index f79ab43aab6b..48ba051da3ad 100644
--- a/verify.c
+++ b/verify.c
@@ -1635,8 +1635,7 @@ struct all_io_list *get_all_io_list(int save_mask, size_t *sz)
s->rand.state32.s[3] = 0;
s->rand.use64 = 0;
}
- s->name[sizeof(s->name) - 1] = '\0';
- strncpy((char *) s->name, td->o.name, sizeof(s->name) - 1);
+ snprintf((char *) s->name, sizeof(s->name), "%s", td->o.name);
next = io_list_next(s);
}
--
2.22.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/5] Verify the size of struct jobs_eta at compile time
2019-08-13 2:01 [PATCH 0/5] Five fio patches Bart Van Assche
2019-08-13 2:01 ` [PATCH 1/5] Optimize the code that copies strings Bart Van Assche
@ 2019-08-13 2:01 ` Bart Van Assche
2019-08-13 2:01 ` [PATCH 3/5] Refine packed annotations in stat.h Bart Van Assche
` (2 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Bart Van Assche @ 2019-08-13 2:01 UTC (permalink / raw)
To: Jens Axboe; +Cc: fio, Damien Le Moal, Bart Van Assche
This helps to verify the correctness of a later patch that will modify
this structure.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
eta.c | 2 ++
fio.h | 2 ++
2 files changed, 4 insertions(+)
diff --git a/eta.c b/eta.c
index 647a1bdd8eed..99c537d189a3 100644
--- a/eta.c
+++ b/eta.c
@@ -736,6 +736,8 @@ void print_thread_status(void)
void print_status_init(int thr_number)
{
+ BUILD_BUG_ON(sizeof(struct jobs_eta) != 160);
+
DRD_IGNORE_VAR(__run_str);
__run_str[thr_number] = 'P';
update_condensed_str(__run_str, run_str);
diff --git a/fio.h b/fio.h
index 2094d30b863e..4bf96fc1cd62 100644
--- a/fio.h
+++ b/fio.h
@@ -66,6 +66,8 @@
#include <cuda.h>
#endif
+#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+
struct fio_sem;
/*
--
2.22.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/5] Refine packed annotations in stat.h
2019-08-13 2:01 [PATCH 0/5] Five fio patches Bart Van Assche
2019-08-13 2:01 ` [PATCH 1/5] Optimize the code that copies strings Bart Van Assche
2019-08-13 2:01 ` [PATCH 2/5] Verify the size of struct jobs_eta at compile time Bart Van Assche
@ 2019-08-13 2:01 ` Bart Van Assche
2019-08-13 2:53 ` Jens Axboe
2019-08-13 2:01 ` [PATCH 4/5] Revert "eta: Fix compiler warning" Bart Van Assche
2019-08-13 2:01 ` [PATCH 5/5] make test: Also run zbd tests Bart Van Assche
4 siblings, 1 reply; 16+ messages in thread
From: Bart Van Assche @ 2019-08-13 2:01 UTC (permalink / raw)
To: Jens Axboe; +Cc: fio, Damien Le Moal, Bart Van Assche
Instead of declaring the whole structure packed, only declare non-aligned
members packed. This patch is an alternative way to fix the following gcc 9
compiler warnings:
eta.c: In function 'calc_thread_status':
eta.c:510:7: error: taking address of packed member of 'struct jobs_eta' may result in an unaligned pointer value [-Werror=address-of-packed-member]
510 | je->rate);
| ~~^~~~~~
eta.c:522:66: error: taking address of packed member of 'struct jobs_eta' may result in an unaligned pointer value [-Werror=address-of-packed-member]
522 | calc_rate(unified_rw_rep, disp_time, io_bytes, disp_io_bytes, je->rate);
| ~~^~~~~~
eta.c:523:64: error: taking address of packed member of 'struct jobs_eta' may result in an unaligned pointer value [-Werror=address-of-packed-member]
523 | calc_iops(unified_rw_rep, disp_time, io_iops, disp_io_iops, je->iops);
|
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
stat.h | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/stat.h b/stat.h
index e9551381ce7b..c6353c70ae08 100644
--- a/stat.h
+++ b/stat.h
@@ -260,10 +260,11 @@ struct jobs_eta {
uint64_t m_rate[DDIR_RWDIR_CNT], t_rate[DDIR_RWDIR_CNT];
uint64_t rate[DDIR_RWDIR_CNT];
- uint32_t m_iops[DDIR_RWDIR_CNT], t_iops[DDIR_RWDIR_CNT];
- uint32_t iops[DDIR_RWDIR_CNT];
- uint64_t elapsed_sec;
- uint64_t eta_sec;
+ uint32_t m_iops[DDIR_RWDIR_CNT] __attribute__((packed));
+ uint32_t t_iops[DDIR_RWDIR_CNT] __attribute__((packed));
+ uint32_t iops[DDIR_RWDIR_CNT] __attribute__((packed));
+ uint64_t elapsed_sec __attribute__((packed));
+ uint64_t eta_sec __attribute__((packed));
uint32_t is_pow2;
uint32_t unit_base;
@@ -276,7 +277,7 @@ struct jobs_eta {
*/
uint32_t nr_threads;
uint8_t run_str[];
-} __attribute__((packed));
+};
struct io_u_plat_entry {
struct flist_head list;
--
2.22.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 4/5] Revert "eta: Fix compiler warning"
2019-08-13 2:01 [PATCH 0/5] Five fio patches Bart Van Assche
` (2 preceding siblings ...)
2019-08-13 2:01 ` [PATCH 3/5] Refine packed annotations in stat.h Bart Van Assche
@ 2019-08-13 2:01 ` Bart Van Assche
2019-08-13 2:01 ` [PATCH 5/5] make test: Also run zbd tests Bart Van Assche
4 siblings, 0 replies; 16+ messages in thread
From: Bart Van Assche @ 2019-08-13 2:01 UTC (permalink / raw)
To: Jens Axboe; +Cc: fio, Damien Le Moal, Bart Van Assche
This reverts commit df0ca15ce2ffac0df3e9dcf4bfe4121333c4aa27. Due to
the previous patch it is no longer necessary to confuse the compiler
about accesses of the 'rate' and 'iops' members in struct jobs_eta.
Cc: Damien Le Moal <damien.lemoal@wdc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
eta.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/eta.c b/eta.c
index 99c537d189a3..77c3dd5f0453 100644
--- a/eta.c
+++ b/eta.c
@@ -392,9 +392,6 @@ bool calc_thread_status(struct jobs_eta *je, int force)
static unsigned long long disp_io_iops[DDIR_RWDIR_CNT];
static struct timespec rate_prev_time, disp_prev_time;
- void *je_rate = (void *) je->rate;
- void *je_iops = (void *) je->iops;
-
if (!force) {
if (!(output_format & FIO_OUTPUT_NORMAL) &&
f_out == stdout)
@@ -510,7 +507,7 @@ bool calc_thread_status(struct jobs_eta *je, int force)
if (write_bw_log && rate_time > bw_avg_time && !in_ramp_time(td)) {
calc_rate(unified_rw_rep, rate_time, io_bytes, rate_io_bytes,
- je_rate);
+ je->rate);
memcpy(&rate_prev_time, &now, sizeof(now));
add_agg_sample(sample_val(je->rate[DDIR_READ]), DDIR_READ, 0);
add_agg_sample(sample_val(je->rate[DDIR_WRITE]), DDIR_WRITE, 0);
@@ -522,8 +519,8 @@ bool calc_thread_status(struct jobs_eta *je, int force)
if (!force && !eta_time_within_slack(disp_time))
return false;
- calc_rate(unified_rw_rep, disp_time, io_bytes, disp_io_bytes, je_rate);
- calc_iops(unified_rw_rep, disp_time, io_iops, disp_io_iops, je_iops);
+ calc_rate(unified_rw_rep, disp_time, io_bytes, disp_io_bytes, je->rate);
+ calc_iops(unified_rw_rep, disp_time, io_iops, disp_io_iops, je->iops);
memcpy(&disp_prev_time, &now, sizeof(now));
--
2.22.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 5/5] make test: Also run zbd tests
2019-08-13 2:01 [PATCH 0/5] Five fio patches Bart Van Assche
` (3 preceding siblings ...)
2019-08-13 2:01 ` [PATCH 4/5] Revert "eta: Fix compiler warning" Bart Van Assche
@ 2019-08-13 2:01 ` Bart Van Assche
2019-08-13 4:39 ` Sitsofe Wheeler
4 siblings, 1 reply; 16+ messages in thread
From: Bart Van Assche @ 2019-08-13 2:01 UTC (permalink / raw)
To: Jens Axboe; +Cc: fio, Damien Le Moal, Bart Van Assche
Include the ZBD tests in Travis runs such that the ZBD tests are run when
a fio pull request is submitted from the github website.
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
.travis.yml | 3 +++
Makefile | 14 ++++++++++++++
2 files changed, 17 insertions(+)
diff --git a/.travis.yml b/.travis.yml
index 4a87fe6c45f8..26ed74ab7416 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -1,4 +1,7 @@
language: c
+sudo: required
+# Choose trusty because the default dist (xenial) does not have null_blk
+dist: trusty
os:
- linux
compiler:
diff --git a/Makefile b/Makefile
index fe02bf1df86f..803074e7db7f 100644
--- a/Makefile
+++ b/Makefile
@@ -530,6 +530,20 @@ doc: tools/plot/fio2gnuplot.1
test: fio
./fio --minimal --thread --exitall_on_error --runtime=1s --name=nulltest --ioengine=null --rw=randrw --iodepth=2 --norandommap --random_generator=tausworthe64 --size=16T --name=verifyfstest --filename=fiotestfile.tmp --unlink=1 --rw=write --verify=crc32c --verify_state_save=0 --size=16K
+ if modinfo null_blk >/dev/null 2>&1 && \
+ sudo modprobe null_blk && \
+ [ -e /sys/kernel/config/nullb ]; then \
+ git clone https://github.com/hgst/libzbc && \
+ (cd libzbc && \
+ ./autogen.sh && \
+ ./configure --prefix=/usr && \
+ make -j && \
+ sudo make install) && \
+ sudo t/zbd/run-tests-against-regular-nullb && \
+ if modinfo null_blk | grep -q '^parm:[[:blank:]]*zoned:'; then \
+ sudo t/zbd/run-tests-against-zoned-nullb; \
+ fi; \
+ fi
install: $(PROGS) $(SCRIPTS) tools/plot/fio2gnuplot.1 FORCE
$(INSTALL) -m 755 -d $(DESTDIR)$(bindir)
--
2.22.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/5] Refine packed annotations in stat.h
2019-08-13 2:01 ` [PATCH 3/5] Refine packed annotations in stat.h Bart Van Assche
@ 2019-08-13 2:53 ` Jens Axboe
2019-08-13 15:12 ` Bart Van Assche
0 siblings, 1 reply; 16+ messages in thread
From: Jens Axboe @ 2019-08-13 2:53 UTC (permalink / raw)
To: Bart Van Assche; +Cc: fio, Damien Le Moal
On 8/12/19 8:01 PM, Bart Van Assche wrote:
> Instead of declaring the whole structure packed, only declare non-aligned
> members packed. This patch is an alternative way to fix the following gcc 9
> compiler warnings:
>
> eta.c: In function 'calc_thread_status':
> eta.c:510:7: error: taking address of packed member of 'struct jobs_eta' may result in an unaligned pointer value [-Werror=address-of-packed-member]
> 510 | je->rate);
> | ~~^~~~~~
> eta.c:522:66: error: taking address of packed member of 'struct jobs_eta' may result in an unaligned pointer value [-Werror=address-of-packed-member]
> 522 | calc_rate(unified_rw_rep, disp_time, io_bytes, disp_io_bytes, je->rate);
> | ~~^~~~~~
> eta.c:523:64: error: taking address of packed member of 'struct jobs_eta' may result in an unaligned pointer value [-Werror=address-of-packed-member]
> 523 | calc_iops(unified_rw_rep, disp_time, io_iops, disp_io_iops, je->iops);
> |
This seems fragile. Not that we change the struct all the time, or even often,
but it'd be easy to add members and end up with different layout on 32-bit
vs 64-bit.
How do we improve on that?
--
Jens Axboe
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 5/5] make test: Also run zbd tests
2019-08-13 2:01 ` [PATCH 5/5] make test: Also run zbd tests Bart Van Assche
@ 2019-08-13 4:39 ` Sitsofe Wheeler
2019-08-13 15:06 ` Bart Van Assche
0 siblings, 1 reply; 16+ messages in thread
From: Sitsofe Wheeler @ 2019-08-13 4:39 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, fio, Damien Le Moal
On Tue, 13 Aug 2019 at 03:02, Bart Van Assche <bvanassche@acm.org> wrote:
>
> Include the ZBD tests in Travis runs such that the ZBD tests are run when
> a fio pull request is submitted from the github website.
>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> .travis.yml | 3 +++
> Makefile | 14 ++++++++++++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/.travis.yml b/.travis.yml
> index 4a87fe6c45f8..26ed74ab7416 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -1,4 +1,7 @@
> language: c
> +sudo: required
> +# Choose trusty because the default dist (xenial) does not have null_blk
> +dist: trusty
> os:
> - linux
> compiler:
> diff --git a/Makefile b/Makefile
> index fe02bf1df86f..803074e7db7f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -530,6 +530,20 @@ doc: tools/plot/fio2gnuplot.1
>
> test: fio
> ./fio --minimal --thread --exitall_on_error --runtime=1s --name=nulltest --ioengine=null --rw=randrw --iodepth=2 --norandommap --random_generator=tausworthe64 --size=16T --name=verifyfstest --filename=fiotestfile.tmp --unlink=1 --rw=write --verify=crc32c --verify_state_save=0 --size=16K
> + if modinfo null_blk >/dev/null 2>&1 && \
> + sudo modprobe null_blk && \
> + [ -e /sys/kernel/config/nullb ]; then \
> + git clone https://github.com/hgst/libzbc && \
> + (cd libzbc && \
> + ./autogen.sh && \
> + ./configure --prefix=/usr && \
> + make -j && \
> + sudo make install) && \
> + sudo t/zbd/run-tests-against-regular-nullb && \
> + if modinfo null_blk | grep -q '^parm:[[:blank:]]*zoned:'; then \
> + sudo t/zbd/run-tests-against-zoned-nullb; \
> + fi; \
> + fi
>
> install: $(PROGS) $(SCRIPTS) tools/plot/fio2gnuplot.1 FORCE
> $(INSTALL) -m 755 -d $(DESTDIR)$(bindir)
> --
> 2.22.0
>
I don't feel comfortable trying to clone a repo, run sudo, installing
stuff as root and loading a kernel module just because someone ran
"make test". Maybe we should introduce another target for this?
--
Sitsofe | http://sucs.org/~sits/
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 5/5] make test: Also run zbd tests
2019-08-13 4:39 ` Sitsofe Wheeler
@ 2019-08-13 15:06 ` Bart Van Assche
2019-08-13 20:10 ` Sitsofe Wheeler
0 siblings, 1 reply; 16+ messages in thread
From: Bart Van Assche @ 2019-08-13 15:06 UTC (permalink / raw)
To: Sitsofe Wheeler; +Cc: Jens Axboe, fio, Damien Le Moal
On 8/12/19 9:39 PM, Sitsofe Wheeler wrote:
> On Tue, 13 Aug 2019 at 03:02, Bart Van Assche <bvanassche@acm.org> wrote:
>> test: fio
>> ./fio --minimal --thread --exitall_on_error --runtime=1s --name=nulltest --ioengine=null --rw=randrw --iodepth=2 --norandommap --random_generator=tausworthe64 --size=16T --name=verifyfstest --filename=fiotestfile.tmp --unlink=1 --rw=write --verify=crc32c --verify_state_save=0 --size=16K
>> + if modinfo null_blk >/dev/null 2>&1 && \
>> + sudo modprobe null_blk && \
>> + [ -e /sys/kernel/config/nullb ]; then \
>> + git clone https://github.com/hgst/libzbc && \
>> + (cd libzbc && \
>> + ./autogen.sh && \
>> + ./configure --prefix=/usr && \
>> + make -j && \
>> + sudo make install) && \
>> + sudo t/zbd/run-tests-against-regular-nullb && \
>> + if modinfo null_blk | grep -q '^parm:[[:blank:]]*zoned:'; then \
>> + sudo t/zbd/run-tests-against-zoned-nullb; \
>> + fi; \
>> + fi
>>
>> install: $(PROGS) $(SCRIPTS) tools/plot/fio2gnuplot.1 FORCE
>> $(INSTALL) -m 755 -d $(DESTDIR)$(bindir)
>>
>
> I don't feel comfortable trying to clone a repo, run sudo, installing
> stuff as root and loading a kernel module just because someone ran
> "make test". Maybe we should introduce another target for this?
Hi Sitsofe,
How about executing the new code only on Travis, e.g. by checking
whether the TRAVIS environment variable has been set?
Bart.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/5] Refine packed annotations in stat.h
2019-08-13 2:53 ` Jens Axboe
@ 2019-08-13 15:12 ` Bart Van Assche
2019-08-13 20:25 ` Jens Axboe
0 siblings, 1 reply; 16+ messages in thread
From: Bart Van Assche @ 2019-08-13 15:12 UTC (permalink / raw)
To: Jens Axboe; +Cc: fio, Damien Le Moal
On 8/12/19 7:53 PM, Jens Axboe wrote:
> On 8/12/19 8:01 PM, Bart Van Assche wrote:
>> Instead of declaring the whole structure packed, only declare non-aligned
>> members packed. This patch is an alternative way to fix the following gcc 9
>> compiler warnings:
>>
>> eta.c: In function 'calc_thread_status':
>> eta.c:510:7: error: taking address of packed member of 'struct jobs_eta' may result in an unaligned pointer value [-Werror=address-of-packed-member]
>> 510 | je->rate);
>> | ~~^~~~~~
>> eta.c:522:66: error: taking address of packed member of 'struct jobs_eta' may result in an unaligned pointer value [-Werror=address-of-packed-member]
>> 522 | calc_rate(unified_rw_rep, disp_time, io_bytes, disp_io_bytes, je->rate);
>> | ~~^~~~~~
>> eta.c:523:64: error: taking address of packed member of 'struct jobs_eta' may result in an unaligned pointer value [-Werror=address-of-packed-member]
>> 523 | calc_iops(unified_rw_rep, disp_time, io_iops, disp_io_iops, je->iops);
>> |
>
> This seems fragile. Not that we change the struct all the time, or even often,
> but it'd be easy to add members and end up with different layout on 32-bit
> vs 64-bit.
>
> How do we improve on that?
Hi Jens,
Do you agree that the "BUILD_BUG_ON(sizeof(struct jobs_eta) != 160)"
statement added by the previous patch should catch such differences? 160
bytes namely is the size of an entirely packed jobs_eta structure.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 5/5] make test: Also run zbd tests
2019-08-13 15:06 ` Bart Van Assche
@ 2019-08-13 20:10 ` Sitsofe Wheeler
2019-08-13 20:22 ` Jens Axboe
0 siblings, 1 reply; 16+ messages in thread
From: Sitsofe Wheeler @ 2019-08-13 20:10 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, fio, Damien Le Moal
On Tue, 13 Aug 2019 at 16:06, Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 8/12/19 9:39 PM, Sitsofe Wheeler wrote:
> > On Tue, 13 Aug 2019 at 03:02, Bart Van Assche <bvanassche@acm.org> wrote:
> >> test: fio
> >> ./fio --minimal --thread --exitall_on_error --runtime=1s --name=nulltest --ioengine=null --rw=randrw --iodepth=2 --norandommap --random_generator=tausworthe64 --size=16T --name=verifyfstest --filename=fiotestfile.tmp --unlink=1 --rw=write --verify=crc32c --verify_state_save=0 --size=16K
> >> + if modinfo null_blk >/dev/null 2>&1 && \
> >> + sudo modprobe null_blk && \
> >> + [ -e /sys/kernel/config/nullb ]; then \
> >> + git clone https://github.com/hgst/libzbc && \
> >> + (cd libzbc && \
> >> + ./autogen.sh && \
> >> + ./configure --prefix=/usr && \
> >> + make -j && \
> >> + sudo make install) && \
> >> + sudo t/zbd/run-tests-against-regular-nullb && \
> >> + if modinfo null_blk | grep -q '^parm:[[:blank:]]*zoned:'; then \
> >> + sudo t/zbd/run-tests-against-zoned-nullb; \
> >> + fi; \
> >> + fi
> >>
> >> install: $(PROGS) $(SCRIPTS) tools/plot/fio2gnuplot.1 FORCE
> >> $(INSTALL) -m 755 -d $(DESTDIR)$(bindir)
> >>
> >
> > I don't feel comfortable trying to clone a repo, run sudo, installing
> > stuff as root and loading a kernel module just because someone ran
> > "make test". Maybe we should introduce another target for this?
>
> Hi Sitsofe,
>
> How about executing the new code only on Travis, e.g. by checking
> whether the TRAVIS environment variable has been set?
That's a bit better but I still think we could introduce a test_travis
target, have travis call that and run any extra commands we want...
--
Sitsofe | http://sucs.org/~sits/
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 5/5] make test: Also run zbd tests
2019-08-13 20:10 ` Sitsofe Wheeler
@ 2019-08-13 20:22 ` Jens Axboe
0 siblings, 0 replies; 16+ messages in thread
From: Jens Axboe @ 2019-08-13 20:22 UTC (permalink / raw)
To: Sitsofe Wheeler, Bart Van Assche; +Cc: fio, Damien Le Moal
On 8/13/19 2:10 PM, Sitsofe Wheeler wrote:
> On Tue, 13 Aug 2019 at 16:06, Bart Van Assche <bvanassche@acm.org> wrote:
>>
>> On 8/12/19 9:39 PM, Sitsofe Wheeler wrote:
>>> On Tue, 13 Aug 2019 at 03:02, Bart Van Assche <bvanassche@acm.org> wrote:
>>>> test: fio
>>>> ./fio --minimal --thread --exitall_on_error --runtime=1s --name=nulltest --ioengine=null --rw=randrw --iodepth=2 --norandommap --random_generator=tausworthe64 --size=16T --name=verifyfstest --filename=fiotestfile.tmp --unlink=1 --rw=write --verify=crc32c --verify_state_save=0 --size=16K
>>>> + if modinfo null_blk >/dev/null 2>&1 && \
>>>> + sudo modprobe null_blk && \
>>>> + [ -e /sys/kernel/config/nullb ]; then \
>>>> + git clone https://github.com/hgst/libzbc && \
>>>> + (cd libzbc && \
>>>> + ./autogen.sh && \
>>>> + ./configure --prefix=/usr && \
>>>> + make -j && \
>>>> + sudo make install) && \
>>>> + sudo t/zbd/run-tests-against-regular-nullb && \
>>>> + if modinfo null_blk | grep -q '^parm:[[:blank:]]*zoned:'; then \
>>>> + sudo t/zbd/run-tests-against-zoned-nullb; \
>>>> + fi; \
>>>> + fi
>>>>
>>>> install: $(PROGS) $(SCRIPTS) tools/plot/fio2gnuplot.1 FORCE
>>>> $(INSTALL) -m 755 -d $(DESTDIR)$(bindir)
>>>>
>>>
>>> I don't feel comfortable trying to clone a repo, run sudo, installing
>>> stuff as root and loading a kernel module just because someone ran
>>> "make test". Maybe we should introduce another target for this?
>>
>> Hi Sitsofe,
>>
>> How about executing the new code only on Travis, e.g. by checking
>> whether the TRAVIS environment variable has been set?
>
> That's a bit better but I still think we could introduce a test_travis
> target, have travis call that and run any extra commands we want...
I agree with that, but let's call it something like fulltest instead.
That gives travis/CI the option to run test and fulltest, where a
"normal" user would probably just do make test. If they do want
fulltest, then it's available to run.
--
Jens Axboe
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/5] Refine packed annotations in stat.h
2019-08-13 15:12 ` Bart Van Assche
@ 2019-08-13 20:25 ` Jens Axboe
2019-08-13 20:51 ` Bart Van Assche
0 siblings, 1 reply; 16+ messages in thread
From: Jens Axboe @ 2019-08-13 20:25 UTC (permalink / raw)
To: Bart Van Assche; +Cc: fio, Damien Le Moal
On 8/13/19 9:12 AM, Bart Van Assche wrote:
> On 8/12/19 7:53 PM, Jens Axboe wrote:
>> On 8/12/19 8:01 PM, Bart Van Assche wrote:
>>> Instead of declaring the whole structure packed, only declare non-aligned
>>> members packed. This patch is an alternative way to fix the following gcc 9
>>> compiler warnings:
>>>
>>> eta.c: In function 'calc_thread_status':
>>> eta.c:510:7: error: taking address of packed member of 'struct jobs_eta' may result in an unaligned pointer value [-Werror=address-of-packed-member]
>>> 510 | je->rate);
>>> | ~~^~~~~~
>>> eta.c:522:66: error: taking address of packed member of 'struct jobs_eta' may result in an unaligned pointer value [-Werror=address-of-packed-member]
>>> 522 | calc_rate(unified_rw_rep, disp_time, io_bytes, disp_io_bytes, je->rate);
>>> | ~~^~~~~~
>>> eta.c:523:64: error: taking address of packed member of 'struct jobs_eta' may result in an unaligned pointer value [-Werror=address-of-packed-member]
>>> 523 | calc_iops(unified_rw_rep, disp_time, io_iops, disp_io_iops, je->iops);
>>> |
>>
>> This seems fragile. Not that we change the struct all the time, or even often,
>> but it'd be easy to add members and end up with different layout on 32-bit
>> vs 64-bit.
>>
>> How do we improve on that?
>
> Hi Jens,
>
> Do you agree that the "BUILD_BUG_ON(sizeof(struct jobs_eta) != 160)"
> statement added by the previous patch should catch such differences? 160
> bytes namely is the size of an entirely packed jobs_eta structure.
I guess that's good enough, though it'd be nice to check for holes
explicitly. I wonder if there's an easy way to do that, though...
--
Jens Axboe
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/5] Refine packed annotations in stat.h
2019-08-13 20:25 ` Jens Axboe
@ 2019-08-13 20:51 ` Bart Van Assche
2019-08-13 21:04 ` Jens Axboe
0 siblings, 1 reply; 16+ messages in thread
From: Bart Van Assche @ 2019-08-13 20:51 UTC (permalink / raw)
To: Jens Axboe; +Cc: fio, Damien Le Moal
On 8/13/19 1:25 PM, Jens Axboe wrote:
> On 8/13/19 9:12 AM, Bart Van Assche wrote:
>> On 8/12/19 7:53 PM, Jens Axboe wrote:
>>> On 8/12/19 8:01 PM, Bart Van Assche wrote:
>>>> Instead of declaring the whole structure packed, only declare non-aligned
>>>> members packed. This patch is an alternative way to fix the following gcc 9
>>>> compiler warnings:
>>>>
>>>> eta.c: In function 'calc_thread_status':
>>>> eta.c:510:7: error: taking address of packed member of 'struct jobs_eta' may result in an unaligned pointer value [-Werror=address-of-packed-member]
>>>> 510 | je->rate);
>>>> | ~~^~~~~~
>>>> eta.c:522:66: error: taking address of packed member of 'struct jobs_eta' may result in an unaligned pointer value [-Werror=address-of-packed-member]
>>>> 522 | calc_rate(unified_rw_rep, disp_time, io_bytes, disp_io_bytes, je->rate);
>>>> | ~~^~~~~~
>>>> eta.c:523:64: error: taking address of packed member of 'struct jobs_eta' may result in an unaligned pointer value [-Werror=address-of-packed-member]
>>>> 523 | calc_iops(unified_rw_rep, disp_time, io_iops, disp_io_iops, je->iops);
>>>> |
>>>
>>> This seems fragile. Not that we change the struct all the time, or even often,
>>> but it'd be easy to add members and end up with different layout on 32-bit
>>> vs 64-bit.
>>>
>>> How do we improve on that?
>>
>> Hi Jens,
>>
>> Do you agree that the "BUILD_BUG_ON(sizeof(struct jobs_eta) != 160)"
>> statement added by the previous patch should catch such differences? 160
>> bytes namely is the size of an entirely packed jobs_eta structure.
>
> I guess that's good enough, though it'd be nice to check for holes
> explicitly. I wonder if there's an easy way to do that, though...
Hi Jens,
The pahole tool should be able to check for holes. Unfortunately that
tool doesn't seem to work very well on Ubuntu:
$ pahole -H 1 *.o
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x12d> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x175> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x58> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x15e> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x44> not handled!
die__process_function: tag not supported (INVALID)!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0xb5> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0xf2> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x48> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x4cc> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x1ab> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x4b6> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x736> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x4ba> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x187> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x4e4> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x42> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0xd1> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0xfd> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0xaf> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x103> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x73> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0xb5> not handled!
struct _IO_FILE {
int _flags; /* 0 4 */
/* XXX 4 bytes hole, try to pack */
char * _IO_read_ptr; /* 8 8 */
char * _IO_read_end; /* 16 8 */
char * _IO_read_base; /* 24 8 */
char * _IO_write_base; /* 32 8 */
char * _IO_write_ptr; /* 40 8 */
char * _IO_write_end; /* 48 8 */
char * _IO_buf_base; /* 56 8 */
/* --- cacheline 1 boundary (64 bytes) --- */
char * _IO_buf_end; /* 64 8 */
char * _IO_save_base; /* 72 8 */
char * _IO_backup_base; /* 80 8 */
char * _IO_save_end; /* 88 8 */
struct _IO_marker * _markers; /* 96 8 */
struct _IO_FILE * _chain; /* 104 8 */
int _fileno; /* 112 4 */
int _flags2; /* 116 4 */
__off_t _old_offset; /* 120 8 */
/* --- cacheline 2 boundary (128 bytes) --- */
short unsigned int _cur_column; /* 128 2 */
signed char _vtable_offset; /* 130 1 */
char _shortbuf[1]; /* 131 1 */
/* XXX 4 bytes hole, try to pack */
_IO_lock_t * _lock; /* 136 8 */
__off64_t _offset; /* 144 8 */
struct _IO_codecvt * _codecvt; /* 152 8 */
struct _IO_wide_data * _wide_data; /* 160 8 */
struct _IO_FILE * _freeres_list; /* 168 8 */
void * _freeres_buf; /* 176 8 */
size_t __pad5; /* 184 8 */
/* --- cacheline 3 boundary (192 bytes) --- */
int _mode; /* 192 4 */
char _unused2[20]; /* 196 20 */
/* size: 216, cachelines: 4, members: 29 */
/* sum members: 208, holes: 2, sum holes: 8 */
/* last cacheline: 24 bytes */
};
struct fio_filelock {
uint32_t hash; /* 0 4 */
/* XXX 4 bytes hole, try to pack */
struct fio_sem lock; /* 8 104 */
/* XXX last struct has 4 bytes of padding */
/* --- cacheline 1 boundary (64 bytes) was 48 bytes ago --- */
struct flist_head list; /* 112 16 */
/* --- cacheline 2 boundary (128 bytes) --- */
unsigned int references; /* 128 4 */
/* size: 136, cachelines: 3, members: 4 */
/* sum members: 128, holes: 1, sum holes: 4 */
/* padding: 4 */
/* paddings: 1, sum paddings: 4 */
/* last cacheline: 8 bytes */
};
struct io_u_ring {
unsigned int head; /* 0 4 */
unsigned int tail; /* 4 4 */
unsigned int max; /* 8 4 */
/* XXX 4 bytes hole, try to pack */
struct io_u * * ring; /* 16 8 */
/* size: 24, cachelines: 1, members: 4 */
/* sum members: 20, holes: 1, sum holes: 4 */
/* last cacheline: 24 bytes */
};
struct json_object {
struct json_pair * * pairs; /* 0 8 */
int pair_cnt; /* 8 4 */
/* XXX 4 bytes hole, try to pack */
struct json_value * parent; /* 16 8 */
/* size: 24, cachelines: 1, members: 3 */
/* sum members: 20, holes: 1, sum holes: 4 */
/* last cacheline: 24 bytes */
};
struct json_array {
struct json_value * * values; /* 0 8 */
int value_cnt; /* 8 4 */
/* XXX 4 bytes hole, try to pack */
struct json_value * parent; die__process_unit:
DW_TAG_restrict_type (0x37) @ <0x283> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0xfc> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x14d> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x127> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0xae> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x4f> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x4ba> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0xda> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x552> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0xa5> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0xfb> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x59> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x72> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x4ca> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0xe5> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x4ba> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0xe5> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0xf6> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x5d> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x41> not handled!
die__process_unit: DW_TAG_restrict_type (0x37) @ <0x4c5> not handled!
/* 16 8 */
/* size: 24, cachelines: 1, members: 3 */
/* sum members: 20, holes: 1, sum holes: 4 */
/* last cacheline: 24 bytes */
};
struct json_value {
int type; /* 0 4 */
/* XXX 4 bytes hole, try to pack */
union {
long long int integer_number; /* 8 */
double float_number; /* 8 */
char * string; /* 8 */
struct json_object * object; /* 8 */
struct json_array * array; /* 8 */
}; /* 8 8 */
int parent_type; /* 16 4 */
/* XXX 4 bytes hole, try to pack */
union {
struct json_pair * parent_pair; /* 8 */
struct json_array * parent_array; /* 8 */
}; /* 24 8 */
/* size: 32, cachelines: 1, members: 4 */
/* sum members: 24, holes: 2, sum holes: 8 */
/* last cacheline: 32 bytes */
};
Bart.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/5] Refine packed annotations in stat.h
2019-08-13 20:51 ` Bart Van Assche
@ 2019-08-13 21:04 ` Jens Axboe
2019-08-13 21:46 ` Bart Van Assche
0 siblings, 1 reply; 16+ messages in thread
From: Jens Axboe @ 2019-08-13 21:04 UTC (permalink / raw)
To: Bart Van Assche; +Cc: fio, Damien Le Moal
On 8/13/19 2:51 PM, Bart Van Assche wrote:
> On 8/13/19 1:25 PM, Jens Axboe wrote:
>> On 8/13/19 9:12 AM, Bart Van Assche wrote:
>>> On 8/12/19 7:53 PM, Jens Axboe wrote:
>>>> On 8/12/19 8:01 PM, Bart Van Assche wrote:
>>>>> Instead of declaring the whole structure packed, only declare non-aligned
>>>>> members packed. This patch is an alternative way to fix the following gcc 9
>>>>> compiler warnings:
>>>>>
>>>>> eta.c: In function 'calc_thread_status':
>>>>> eta.c:510:7: error: taking address of packed member of 'struct jobs_eta' may result in an unaligned pointer value [-Werror=address-of-packed-member]
>>>>> 510 | je->rate);
>>>>> | ~~^~~~~~
>>>>> eta.c:522:66: error: taking address of packed member of 'struct jobs_eta' may result in an unaligned pointer value [-Werror=address-of-packed-member]
>>>>> 522 | calc_rate(unified_rw_rep, disp_time, io_bytes, disp_io_bytes, je->rate);
>>>>> | ~~^~~~~~
>>>>> eta.c:523:64: error: taking address of packed member of 'struct jobs_eta' may result in an unaligned pointer value [-Werror=address-of-packed-member]
>>>>> 523 | calc_iops(unified_rw_rep, disp_time, io_iops, disp_io_iops, je->iops);
>>>>> |
>>>>
>>>> This seems fragile. Not that we change the struct all the time, or even often,
>>>> but it'd be easy to add members and end up with different layout on 32-bit
>>>> vs 64-bit.
>>>>
>>>> How do we improve on that?
>>>
>>> Hi Jens,
>>>
>>> Do you agree that the "BUILD_BUG_ON(sizeof(struct jobs_eta) != 160)"
>>> statement added by the previous patch should catch such differences? 160
>>> bytes namely is the size of an entirely packed jobs_eta structure.
>>
>> I guess that's good enough, though it'd be nice to check for holes
>> explicitly. I wonder if there's an easy way to do that, though...
>
> Hi Jens,
>
> The pahole tool should be able to check for holes. Unfortunately that
> tool doesn't seem to work very well on Ubuntu:
I was more thinking at build time. It'd be nice if you could do
something ala:
struct bla {
int a;
long b;
};
struct bla b1;
struct bla b2 __attribute__((__packed__));
BUILD_BUG_ON(sizeof(b1) != sizeof(b2));
If we rely on external components to do this, we might as well not do
it.
--
Jens Axboe
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/5] Refine packed annotations in stat.h
2019-08-13 21:04 ` Jens Axboe
@ 2019-08-13 21:46 ` Bart Van Assche
0 siblings, 0 replies; 16+ messages in thread
From: Bart Van Assche @ 2019-08-13 21:46 UTC (permalink / raw)
To: Jens Axboe; +Cc: fio, Damien Le Moal
On 8/13/19 2:04 PM, Jens Axboe wrote:
> I was more thinking at build time. It'd be nice if you could do
> something ala:
>
> struct bla {
> int a;
> long b;
> };
>
> struct bla b1;
> struct bla b2 __attribute__((__packed__));
>
> BUILD_BUG_ON(sizeof(b1) != sizeof(b2));
>
> If we rely on external components to do this, we might as well not do
> it.
Perhaps something like this?
diff --git a/eta.c b/eta.c
index 647a1bdd8eed..4095a257d206 100644
--- a/eta.c
+++ b/eta.c
@@ -736,6 +736,8 @@ void print_thread_status(void)
void print_status_init(int thr_number)
{
+ BUILD_BUG_ON(sizeof(struct jobs_eta) != sizeof(struct jobs_eta_packed));
+
DRD_IGNORE_VAR(__run_str);
__run_str[thr_number] = 'P';
update_condensed_str(__run_str, run_str);
diff --git a/fio.h b/fio.h
index 2094d30b863e..4bf96fc1cd62 100644
--- a/fio.h
+++ b/fio.h
@@ -66,6 +66,8 @@
#include <cuda.h>
#endif
+#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+
struct fio_sem;
/*
diff --git a/stat.h b/stat.h
index e9551381ce7b..c209ab6c7a96 100644
--- a/stat.h
+++ b/stat.h
@@ -251,32 +251,36 @@ struct thread_stat {
uint64_t cachemiss;
} __attribute__((packed));
-struct jobs_eta {
- uint32_t nr_running;
- uint32_t nr_ramp;
-
- uint32_t nr_pending;
- uint32_t nr_setting_up;
-
- uint64_t m_rate[DDIR_RWDIR_CNT], t_rate[DDIR_RWDIR_CNT];
- uint64_t rate[DDIR_RWDIR_CNT];
- uint32_t m_iops[DDIR_RWDIR_CNT], t_iops[DDIR_RWDIR_CNT];
- uint32_t iops[DDIR_RWDIR_CNT];
- uint64_t elapsed_sec;
- uint64_t eta_sec;
- uint32_t is_pow2;
- uint32_t unit_base;
-
- uint32_t sig_figs;
-
- uint32_t files_open;
+#define JOBS_ETA { \
+ uint32_t nr_running; \
+ uint32_t nr_ramp; \
+ \
+ uint32_t nr_pending; \
+ uint32_t nr_setting_up; \
+ \
+ uint64_t m_rate[DDIR_RWDIR_CNT], t_rate[DDIR_RWDIR_CNT]; \
+ uint64_t rate[DDIR_RWDIR_CNT]; \
+ uint32_t m_iops[DDIR_RWDIR_CNT] __attribute__((packed)); \
+ uint32_t t_iops[DDIR_RWDIR_CNT] __attribute__((packed)); \
+ uint32_t iops[DDIR_RWDIR_CNT] __attribute__((packed)); \
+ uint64_t elapsed_sec __attribute__((packed)); \
+ uint64_t eta_sec __attribute__((packed)); \
+ uint32_t is_pow2; \
+ uint32_t unit_base; \
+ \
+ uint32_t sig_figs; \
+ \
+ uint32_t files_open; \
+ \
+ /* \
+ * Network 'copy' of run_str[] \
+ */ \
+ uint32_t nr_threads; \
+ uint8_t run_str[]; \
+}
- /*
- * Network 'copy' of run_str[]
- */
- uint32_t nr_threads;
- uint8_t run_str[];
-} __attribute__((packed));
+struct jobs_eta JOBS_ETA;
+struct jobs_eta_packed JOBS_ETA __attribute__((packed));
struct io_u_plat_entry {
struct flist_head list;
Thanks,
Bart.
^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-08-13 21:46 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-13 2:01 [PATCH 0/5] Five fio patches Bart Van Assche
2019-08-13 2:01 ` [PATCH 1/5] Optimize the code that copies strings Bart Van Assche
2019-08-13 2:01 ` [PATCH 2/5] Verify the size of struct jobs_eta at compile time Bart Van Assche
2019-08-13 2:01 ` [PATCH 3/5] Refine packed annotations in stat.h Bart Van Assche
2019-08-13 2:53 ` Jens Axboe
2019-08-13 15:12 ` Bart Van Assche
2019-08-13 20:25 ` Jens Axboe
2019-08-13 20:51 ` Bart Van Assche
2019-08-13 21:04 ` Jens Axboe
2019-08-13 21:46 ` Bart Van Assche
2019-08-13 2:01 ` [PATCH 4/5] Revert "eta: Fix compiler warning" Bart Van Assche
2019-08-13 2:01 ` [PATCH 5/5] make test: Also run zbd tests Bart Van Assche
2019-08-13 4:39 ` Sitsofe Wheeler
2019-08-13 15:06 ` Bart Van Assche
2019-08-13 20:10 ` Sitsofe Wheeler
2019-08-13 20:22 ` Jens Axboe
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.