* [PATCH 1/8] Don't silently terminate td when no I/O performed due to error @ 2017-04-04 20:22 kusumi.tomohiro 2017-04-04 20:22 ` [PATCH 2/8] dump_td_info() doesn't really need to be a function kusumi.tomohiro ` (7 more replies) 0 siblings, 8 replies; 11+ messages in thread From: kusumi.tomohiro @ 2017-04-04 20:22 UTC (permalink / raw) To: axboe, fio; +Cc: Tomohiro Kusumi From: Tomohiro Kusumi <tkusumi@tuxera.com> Some runtime configurations can cause threads/processes to terminate without any I/O performed, yet with no explicit error message, which is quite confusing. In the example below, fio finishes with neither error nor regular statistics due to file offset + bs being larger than the file size while in get_io_u(). This commit calls log_err() when this happens. Since it's difficult to tell the exact reason after thread has left the main I/O loop[*], it gives advice to use a command line option --debug=io, similar to the way td_io_init() gives advice. [*] It can't just replace dprint(FD_IO, ...) calls with td_verror() for e.g. while in get_io_u(), since those are also used by non error paths too. -- # ./fio --name=xxxxx --ioengine=sync --rw=read --bs=2k --size=1m --nrfiles=1k --unlink=1 xxxxx: (g=0): rw=read, bs=(R) 2048B-2048B, (W) 2048B-2048B, (T) 2048B-2048B, ioengine=sync, iodepth=1 fio-2.18-59-g618e Starting 1 process xxxxx: Laying out IO files (1024 files / total 1MiB) Run status group 0 (all jobs): Disk stats (read/write): dm-0: ios=0/0, merge=0/0, ticks=0/0, in_queue=0, util=0.00%, aggrios=0/0, aggrmerge=0/0, aggrticks=0/0, aggrin_queue=0, aggrutil=0.00% sda: ios=0/0, merge=0/0, ticks=0/0, in_queue=0, util=0.00% Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com> --- backend.c | 17 +++++++++++++++-- io_u.c | 5 +++-- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/backend.c b/backend.c index 38ef348..e5be187 100644 --- a/backend.c +++ b/backend.c @@ -1456,6 +1456,7 @@ static void *thread_main(void *data) struct thread_data *td = fd->td; struct thread_options *o = &td->o; struct sk_out *sk_out = fd->sk_out; + uint64_t bytes_done[DDIR_RWDIR_CNT]; int deadlock_loop_cnt; int clear_state; int ret; @@ -1677,7 +1678,9 @@ static void *thread_main(void *data) sizeof(td->bw_sample_time)); } + memset(bytes_done, 0, sizeof(bytes_done)); clear_state = 0; + while (keep_running(td)) { uint64_t verify_bytes; @@ -1696,8 +1699,6 @@ static void *thread_main(void *data) if (td->o.verify_only && td_write(td)) verify_bytes = do_dry_run(td); else { - uint64_t bytes_done[DDIR_RWDIR_CNT]; - do_io(td, bytes_done); if (!ddir_rw_sum(bytes_done)) { @@ -1776,6 +1777,18 @@ static void *thread_main(void *data) break; } + /* + * If td ended up with no I/O when it should have had, + * then something went wrong unless FIO_NOIO or FIO_DISKLESSIO. + * (Are we not missing other flags that can be ignored ?) + */ + if ((td->o.size || td->o.io_size) && !ddir_rw_sum(bytes_done) && + !(td_ioengine_flagged(td, FIO_NOIO) || + td_ioengine_flagged(td, FIO_DISKLESSIO))) + log_err("%s: No I/O performed by %s, " + "perhaps try --debug=io option for details?\n", + td->o.name, td->io_ops->name); + td_set_runstate(td, TD_FINISHING); update_rusage_stat(td); diff --git a/io_u.c b/io_u.c index 363bfe1..88f35c9 100644 --- a/io_u.c +++ b/io_u.c @@ -899,8 +899,9 @@ static int fill_io_u(struct thread_data *td, struct io_u *io_u) } if (io_u->offset + io_u->buflen > io_u->file->real_file_size) { - dprint(FD_IO, "io_u %p, offset too large\n", io_u); - dprint(FD_IO, " off=%llu/%lu > %llu\n", + dprint(FD_IO, "io_u %p, offset + buflen exceeds file size\n", + io_u); + dprint(FD_IO, " offset=%llu/buflen=%lu > %llu\n", (unsigned long long) io_u->offset, io_u->buflen, (unsigned long long) io_u->file->real_file_size); return 1; -- 2.9.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/8] dump_td_info() doesn't really need to be a function 2017-04-04 20:22 [PATCH 1/8] Don't silently terminate td when no I/O performed due to error kusumi.tomohiro @ 2017-04-04 20:22 ` kusumi.tomohiro 2017-04-04 20:22 ` [PATCH 3/8] Add assert(0) to DDIR_DATASYNC sync path if fdatasync(2) is unsupported kusumi.tomohiro ` (6 subsequent siblings) 7 siblings, 0 replies; 11+ messages in thread From: kusumi.tomohiro @ 2017-04-04 20:22 UTC (permalink / raw) To: axboe, fio; +Cc: Tomohiro Kusumi From: Tomohiro Kusumi <tkusumi@tuxera.com> since it's short enough, and too specific to be called as dump_td_info(). Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com> --- backend.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/backend.c b/backend.c index e5be187..67b4c6b 100644 --- a/backend.c +++ b/backend.c @@ -1861,14 +1861,6 @@ err: return (void *) (uintptr_t) td->error; } -static void dump_td_info(struct thread_data *td) -{ - log_err("fio: job '%s' (state=%d) hasn't exited in %lu seconds, it " - "appears to be stuck. Doing forceful exit of this job.\n", - td->o.name, td->runstate, - (unsigned long) time_since_now(&td->terminate_time)); -} - /* * Run over the job map and reap the threads that have exited, if any. */ @@ -1953,7 +1945,11 @@ static void reap_threads(unsigned int *nr_running, uint64_t *t_rate, if (td->terminate && td->runstate < TD_FSYNCING && time_since_now(&td->terminate_time) >= FIO_REAP_TIMEOUT) { - dump_td_info(td); + log_err("fio: job '%s' (state=%d) hasn't exited in " + "%lu seconds, it appears to be stuck. Doing " + "forceful exit of this job.\n", + td->o.name, td->runstate, + (unsigned long) time_since_now(&td->terminate_time)); td_set_runstate(td, TD_REAPED); goto reaped; } -- 2.9.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/8] Add assert(0) to DDIR_DATASYNC sync path if fdatasync(2) is unsupported 2017-04-04 20:22 [PATCH 1/8] Don't silently terminate td when no I/O performed due to error kusumi.tomohiro 2017-04-04 20:22 ` [PATCH 2/8] dump_td_info() doesn't really need to be a function kusumi.tomohiro @ 2017-04-04 20:22 ` kusumi.tomohiro 2017-04-08 17:02 ` Jens Axboe 2017-04-04 20:22 ` [PATCH 4/8] Make lib/prio_tree.c a stand-alone library kusumi.tomohiro ` (5 subsequent siblings) 7 siblings, 1 reply; 11+ messages in thread From: kusumi.tomohiro @ 2017-04-04 20:22 UTC (permalink / raw) To: axboe, fio; +Cc: Tomohiro Kusumi From: Tomohiro Kusumi <tkusumi@tuxera.com> If ddir is DDIR_DATASYNC, it means fio supports fdatasync(2), or it at least compiled on ./configure. If a platform without fdatasync(2) happens to take DDIR_DATASYNC path in do_io_u_sync(), it's simply wrong (because ddir should never be DDIR_DATASYNC due to td->o.fdatasync_blocks == 0) thus should be aborted rather than continue with EINVAL. This commit also leaves the existing code from #else path to avoid compilers complain for uninitialized ret variable. Tested on FreeBSD and DragonFlyBSD. Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com> --- HOWTO | 3 ++- init.c | 4 ++++ io_u.c | 1 + 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/HOWTO b/HOWTO index 80b9e75..b9114b4 100644 --- a/HOWTO +++ b/HOWTO @@ -1109,7 +1109,8 @@ I/O type Like :option:`fsync` but uses :manpage:`fdatasync(2)` to only sync data and not metadata blocks. In Windows, FreeBSD, and DragonFlyBSD there is no - :manpage:`fdatasync(2)`, this falls back to using :manpage:`fsync(2)`. + :manpage:`fdatasync(2)`, this falls back to using :manpage:`fsync(2)` + by overwriting :manpage:`fsync(2)` value with this option. Defaults to 0, which means no sync data every certain number of writes. .. option:: write_barrier=int diff --git a/init.c b/init.c index 2f9433b..90aaea3 100644 --- a/init.c +++ b/init.c @@ -782,6 +782,10 @@ static int fixup_options(struct thread_data *td) } #ifndef CONFIG_FDATASYNC + /* + * If the platform doesn't support fdatasync(2) (e.g. FreeBSD), + * force fsync= using fdatasync= value specified. + */ if (o->fdatasync_blocks) { log_info("fio: this platform does not support fdatasync()" " falling back to using fsync(). Use the 'fsync'" diff --git a/io_u.c b/io_u.c index 88f35c9..86c5fc1 100644 --- a/io_u.c +++ b/io_u.c @@ -2115,6 +2115,7 @@ int do_io_u_sync(const struct thread_data *td, struct io_u *io_u) #else ret = io_u->xfer_buflen; io_u->error = EINVAL; + assert(0); /* should be falling back to fsync(2) */ #endif } else if (io_u->ddir == DDIR_SYNC_FILE_RANGE) ret = do_sync_file_range(td, io_u->file); -- 2.9.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/8] Add assert(0) to DDIR_DATASYNC sync path if fdatasync(2) is unsupported 2017-04-04 20:22 ` [PATCH 3/8] Add assert(0) to DDIR_DATASYNC sync path if fdatasync(2) is unsupported kusumi.tomohiro @ 2017-04-08 17:02 ` Jens Axboe 0 siblings, 0 replies; 11+ messages in thread From: Jens Axboe @ 2017-04-08 17:02 UTC (permalink / raw) To: kusumi.tomohiro; +Cc: fio, Tomohiro Kusumi On Tue, Apr 04 2017, kusumi.tomohiro@gmail.com wrote: > From: Tomohiro Kusumi <tkusumi@tuxera.com> > > If ddir is DDIR_DATASYNC, it means fio supports fdatasync(2), or it > at least compiled on ./configure. If a platform without fdatasync(2) > happens to take DDIR_DATASYNC path in do_io_u_sync(), it's simply wrong > (because ddir should never be DDIR_DATASYNC due to td->o.fdatasync_blocks == 0) > thus should be aborted rather than continue with EINVAL. > > This commit also leaves the existing code from #else path to avoid > compilers complain for uninitialized ret variable. EINVAL should cause termination, I don't think there's any reason to use assert here. In general, if we can handle even cases that we think should not occur, then we should do so instead of dumping core. -- Jens Axboe ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/8] Make lib/prio_tree.c a stand-alone library 2017-04-04 20:22 [PATCH 1/8] Don't silently terminate td when no I/O performed due to error kusumi.tomohiro 2017-04-04 20:22 ` [PATCH 2/8] dump_td_info() doesn't really need to be a function kusumi.tomohiro 2017-04-04 20:22 ` [PATCH 3/8] Add assert(0) to DDIR_DATASYNC sync path if fdatasync(2) is unsupported kusumi.tomohiro @ 2017-04-04 20:22 ` kusumi.tomohiro 2017-04-04 20:22 ` [PATCH 5/8] Make lib/memalign.c " kusumi.tomohiro ` (4 subsequent siblings) 7 siblings, 0 replies; 11+ messages in thread From: kusumi.tomohiro @ 2017-04-04 20:22 UTC (permalink / raw) To: axboe, fio; +Cc: Tomohiro Kusumi From: Tomohiro Kusumi <tkusumi@tuxera.com> lib/prio_tree.c not having dependency on fio.h enables it to be a stand-alone library, which is useful for debugging purpose. In fact, most of the files under lib/ directory do things this way. -- # cat ./test2.c #include <stdio.h> #include "lib/prio_tree.h" int main(void) { struct prio_tree_root tree; struct prio_tree_node node; INIT_PRIO_TREE_ROOT(&tree); INIT_PRIO_TREE_NODE(&node); prio_tree_insert(&tree, &node); prio_tree_remove(&tree, &node); printf("%d\n", prio_tree_empty(&tree)); return 0; } # gcc -Wall -g -DBITS_PER_LONG=64 ./test2.c ./lib/prio_tree.c # ./a.out 1 Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com> --- lib/prio_tree.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/prio_tree.c b/lib/prio_tree.c index e18ae32..de3fe1c 100644 --- a/lib/prio_tree.c +++ b/lib/prio_tree.c @@ -13,9 +13,12 @@ #include <stdlib.h> #include <limits.h> -#include "../fio.h" + +#include "../compiler/compiler.h" #include "prio_tree.h" +#define ARRAY_SIZE(x) (sizeof((x)) / (sizeof((x)[0]))) + /* * A clever mix of heap and radix trees forms a radix priority search tree (PST) * which is useful for storing intervals, e.g, we can consider a vma as a closed -- 2.9.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 5/8] Make lib/memalign.c a stand-alone library 2017-04-04 20:22 [PATCH 1/8] Don't silently terminate td when no I/O performed due to error kusumi.tomohiro ` (2 preceding siblings ...) 2017-04-04 20:22 ` [PATCH 4/8] Make lib/prio_tree.c a stand-alone library kusumi.tomohiro @ 2017-04-04 20:22 ` kusumi.tomohiro 2017-04-04 20:22 ` [PATCH 6/8] Make lib/num2str.c a stand-alone library by adding lib/num2str.h kusumi.tomohiro ` (3 subsequent siblings) 7 siblings, 0 replies; 11+ messages in thread From: kusumi.tomohiro @ 2017-04-04 20:22 UTC (permalink / raw) To: axboe, fio; +Cc: Tomohiro Kusumi From: Tomohiro Kusumi <tkusumi@tuxera.com> lib/memalign.c not having dependency on fio.h enables it to be a stand-alone library, which is useful for debugging purpose. In fact, most of the files under lib/ directory do things this way. (For lib/memalign.c, it was actually myself who introduced this dependency as a part of changes made by 248c9436, so this commit is partially reverting that) -- # cat ./test3.c #include <stdio.h> #include "lib/memalign.h" int main(void) { char *p = fio_memalign(4096, 1000000); printf("%p\n", p); fio_memfree(p, 1000000); return 0; } # gcc -Wall -g ./test3.c ./lib/memalign.c # ./a.out 0x7fbb7bbbc000 Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com> --- lib/memalign.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/memalign.c b/lib/memalign.c index 1d1ba9b..137cc8e 100644 --- a/lib/memalign.c +++ b/lib/memalign.c @@ -3,7 +3,9 @@ #include <inttypes.h> #include "memalign.h" -#include "../fio.h" + +#define PTR_ALIGN(ptr, mask) \ + (char *)((uintptr_t)((ptr) + (mask)) & ~(mask)) struct align_footer { unsigned int offset; -- 2.9.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 6/8] Make lib/num2str.c a stand-alone library by adding lib/num2str.h 2017-04-04 20:22 [PATCH 1/8] Don't silently terminate td when no I/O performed due to error kusumi.tomohiro ` (3 preceding siblings ...) 2017-04-04 20:22 ` [PATCH 5/8] Make lib/memalign.c " kusumi.tomohiro @ 2017-04-04 20:22 ` kusumi.tomohiro 2017-04-04 20:22 ` [PATCH 7/8] Fix num2str() output when maxlen <= strlen(tmp) kusumi.tomohiro ` (2 subsequent siblings) 7 siblings, 0 replies; 11+ messages in thread From: kusumi.tomohiro @ 2017-04-04 20:22 UTC (permalink / raw) To: axboe, fio; +Cc: Tomohiro Kusumi From: Tomohiro Kusumi <tkusumi@tuxera.com> lib/num2str.c not having dependency on fio.h by adding a new header lib/num2str.h enables it to be a stand-alone library function, which is useful for debugging purpose. In fact, most of the files under lib/ directory do things this way. -- # cat ./test1.c #include <stdio.h> #include "lib/num2str.h" int main(void) { printf("%s\n", num2str(1024, 4, 1, 1, N2S_BYTE)); return 0; } # gcc -Wall -g -DCONFIG_STATIC_ASSERT ./test1.c ./lib/num2str.c # ./a.out 1024B Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com> --- fio.h | 9 +-------- lib/num2str.c | 7 +++++-- lib/num2str.h | 15 +++++++++++++++ 3 files changed, 21 insertions(+), 10 deletions(-) create mode 100644 lib/num2str.h diff --git a/fio.h b/fio.h index 3955a81..2975076 100644 --- a/fio.h +++ b/fio.h @@ -35,6 +35,7 @@ #include "oslib/getopt.h" #include "lib/rand.h" #include "lib/rbtree.h" +#include "lib/num2str.h" #include "client.h" #include "server.h" #include "stat.h" @@ -520,7 +521,6 @@ extern void fio_options_mem_dupe(struct thread_data *); extern void td_fill_rand_seeds(struct thread_data *); extern void td_fill_verify_state_seed(struct thread_data *); extern void add_job_opts(const char **, int); -extern char *num2str(uint64_t, int, int, int, int); extern int ioengine_load(struct thread_data *); extern bool parse_dryrun(void); extern int fio_running_or_pending_io_threads(void); @@ -533,13 +533,6 @@ extern uintptr_t page_size; extern int initialize_fio(char *envp[]); extern void deinitialize_fio(void); -#define N2S_NONE 0 -#define N2S_BITPERSEC 1 /* match unit_base for bit rates */ -#define N2S_PERSEC 2 -#define N2S_BIT 3 -#define N2S_BYTE 4 -#define N2S_BYTEPERSEC 8 /* match unit_base for byte rates */ - #define FIO_GETOPT_JOB 0x89000000 #define FIO_GETOPT_IOENGINE 0x98000000 #define FIO_NR_OPTIONS (FIO_MAX_OPTS + 128) diff --git a/lib/num2str.c b/lib/num2str.c index ed3545d..2f714cc 100644 --- a/lib/num2str.c +++ b/lib/num2str.c @@ -2,7 +2,10 @@ #include <stdio.h> #include <string.h> -#include "../fio.h" +#include "../compiler/compiler.h" +#include "num2str.h" + +#define ARRAY_SIZE(x) (sizeof((x)) / (sizeof((x)[0]))) /** * num2str() - Cheesy number->string conversion, complete with carry rounding error. @@ -10,7 +13,7 @@ * @maxlen: max number of digits in the output string (not counting prefix and units) * @base: multiplier for num (e.g., if num represents Ki, use 1024) * @pow2: select unit prefix - 0=power-of-10 decimal SI, nonzero=power-of-2 binary IEC - * @units: select units - N2S_* macros defined in fio.h + * @units: select units - N2S_* macros defined in num2str.h * @returns a malloc'd buffer containing "number[<unit prefix>][<units>]" */ char *num2str(uint64_t num, int maxlen, int base, int pow2, int units) diff --git a/lib/num2str.h b/lib/num2str.h new file mode 100644 index 0000000..81358a1 --- /dev/null +++ b/lib/num2str.h @@ -0,0 +1,15 @@ +#ifndef FIO_NUM2STR_H +#define FIO_NUM2STR_H + +#include <inttypes.h> + +#define N2S_NONE 0 +#define N2S_BITPERSEC 1 /* match unit_base for bit rates */ +#define N2S_PERSEC 2 +#define N2S_BIT 3 +#define N2S_BYTE 4 +#define N2S_BYTEPERSEC 8 /* match unit_base for byte rates */ + +extern char *num2str(uint64_t, int, int, int, int); + +#endif -- 2.9.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 7/8] Fix num2str() output when maxlen <= strlen(tmp) 2017-04-04 20:22 [PATCH 1/8] Don't silently terminate td when no I/O performed due to error kusumi.tomohiro ` (4 preceding siblings ...) 2017-04-04 20:22 ` [PATCH 6/8] Make lib/num2str.c a stand-alone library by adding lib/num2str.h kusumi.tomohiro @ 2017-04-04 20:22 ` kusumi.tomohiro 2017-04-04 20:22 ` [PATCH 8/8] Fix num2str() output when modulo != -1U kusumi.tomohiro 2017-04-08 17:05 ` [PATCH 1/8] Don't silently terminate td when no I/O performed due to error Jens Axboe 7 siblings, 0 replies; 11+ messages in thread From: kusumi.tomohiro @ 2017-04-04 20:22 UTC (permalink / raw) To: axboe, fio; +Cc: Tomohiro Kusumi From: Tomohiro Kusumi <tkusumi@tuxera.com> Since a local variable decimals is unsigned int, this conditional if (decimals <= 1) needs cast as shown below. if ((int)decimals <= 1) Otherwise it results in showing some garbage in case of maxlen == 0, # cat ./test0.c #include <stdio.h> #include "lib/num2str.h" int main(void) { printf("%s\n", num2str(123, 3, 1, 1, N2S_BYTE)); printf("%s\n", num2str(123, 2, 1, 1, N2S_BYTE)); printf("%s\n", num2str(123, 1, 1, 1, N2S_BYTE)); printf("%s\n", num2str(123, 0, 1, 1, N2S_BYTE)); return 0; } # gcc -Wall -g -DCONFIG_STATIC_ASSERT ./test0.c ./lib/num2str.c # ./a.out 123B 0KiB 0KiB 0.0?$-JB # ./a.out 123B 0KiB 0KiB 0.0#;bB # ./a.out 123B 0KiB 0KiB 0.0|B whereas with this commit it prints "0B". # gcc -Wall -g -DCONFIG_STATIC_ASSERT ./test0.c ./lib/num2str.c # ./a.out 123B 0KiB 0KiB 0B Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com> --- lib/num2str.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/num2str.c b/lib/num2str.c index 2f714cc..448d3ff 100644 --- a/lib/num2str.c +++ b/lib/num2str.c @@ -86,7 +86,7 @@ done: sprintf(tmp, "%llu", (unsigned long long) num); decimals = maxlen - strlen(tmp); - if (decimals <= 1) { + if ((int)decimals <= 1) { if (carry) num++; goto done; -- 2.9.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 8/8] Fix num2str() output when modulo != -1U 2017-04-04 20:22 [PATCH 1/8] Don't silently terminate td when no I/O performed due to error kusumi.tomohiro ` (5 preceding siblings ...) 2017-04-04 20:22 ` [PATCH 7/8] Fix num2str() output when maxlen <= strlen(tmp) kusumi.tomohiro @ 2017-04-04 20:22 ` kusumi.tomohiro 2017-04-04 20:46 ` Elliott, Robert (Persistent Memory) 2017-04-08 17:05 ` [PATCH 1/8] Don't silently terminate td when no I/O performed due to error Jens Axboe 7 siblings, 1 reply; 11+ messages in thread From: kusumi.tomohiro @ 2017-04-04 20:22 UTC (permalink / raw) To: axboe, fio; +Cc: Tomohiro Kusumi From: Tomohiro Kusumi <tkusumi@tuxera.com> I'm not sure what 05463816 actually fixed (as it only says "fix"), but this isn't properly working at least for some input numbers unless ".%s" part of sprintf() is meant to be something other than decimal part of the input number. This commit might be breaking something that 05463816 had fixed, which then needs to be rejected, but it at least prints decimal number as expected. For example, when 12345 is 12.0556640625 KiB, and 23456 is 22.90625 KiB, # python -c "print(12345.0 / 1024)" 12.0556640625 # python -c "print(23456.0 / 1024)" 22.90625 running this code as well as fio result in # cat ./test1.c #include <stdio.h> #include "lib/num2str.h" int main(void) { printf("%s-%s\n", num2str(12345, 4, 1, 1, N2S_BYTE), num2str(23456, 4, 1, 1, N2S_BYTE)); return 0; } # gcc -Wall -g -DCONFIG_STATIC_ASSERT ./test1.c ./lib/num2str.c below with the current implementation # ./a.out 12.6KiB-22.1KiB # ./fio --name=xxxxx --ioengine=sync --rw=read --size=1m --unlink=1 --bsrange=12345:23456 | grep "rw=read" xxxxx: (g=0): rw=read, bs=(R) 12.6KiB-22.1KiB, (W) 12.6KiB-22.1KiB, (T) 12.6KiB-22.1KiB, ioengine=sync, iodepth=1 whereas below with this commit. # ./a.out 12.1KiB-22.9KiB # ./fio --name=xxxxx --ioengine=sync --rw=read --size=1m --unlink=1 --bsrange=12345:23456 | grep "rw=read" xxxxx: (g=0): rw=read, bs=(R) 12.1KiB-22.9KiB, (W) 12.1KiB-22.9KiB, (T) 12.1KiB-22.9KiB, ioengine=sync, iodepth=1 -- some more verification with various parameters # cat ./testx.c #include <stdio.h> #include "lib/num2str.h" static void f(uint64_t n, int maxlen, int base, int pow2, int units) { printf("%10jd %s\n", n, num2str(n, maxlen, base, pow2, units)); } int main(void) { /* 1 */ f(1, 1, 1, 1, N2S_BYTE); f(1, 1, 1, 0, N2S_BYTE); printf("====================\n"); /* 10 */ f(10, 2, 1, 1, N2S_BYTE); f(10, 2, 1, 0, N2S_BYTE); printf("====================\n"); /* 1000 */ f(1000, 5, 1, 1, N2S_BYTE); f(1000, 5, 1, 0, N2S_BYTE); f(1000, 4, 1, 1, N2S_BYTE); f(1000, 4, 1, 0, N2S_BYTE); f(1000, 3, 1, 1, N2S_BYTE); f(1000, 3, 1, 0, N2S_BYTE); f(1000, 2, 1, 1, N2S_BYTE); f(1000, 2, 1, 0, N2S_BYTE); f(1000, 1, 1, 1, N2S_BYTE); f(1000, 1, 1, 0, N2S_BYTE); printf("====================\n"); /* 1024 */ f(1024, 5, 1, 1, N2S_BYTE); f(1024, 5, 1, 0, N2S_BYTE); f(1024, 4, 1, 1, N2S_BYTE); f(1024, 4, 1, 0, N2S_BYTE); f(1024, 3, 1, 1, N2S_BYTE); f(1024, 3, 1, 0, N2S_BYTE); f(1024, 2, 1, 1, N2S_BYTE); f(1024, 2, 1, 0, N2S_BYTE); f(1024, 1, 1, 1, N2S_BYTE); f(1024, 1, 1, 0, N2S_BYTE); printf("====================\n"); /* 12345 */ f(12345, 6, 1, 1, N2S_BYTE); f(12345, 6, 1, 0, N2S_BYTE); f(12345, 5, 1, 1, N2S_BYTE); f(12345, 5, 1, 0, N2S_BYTE); f(12345, 4, 1, 1, N2S_BYTE); f(12345, 4, 1, 0, N2S_BYTE); f(12345, 3, 1, 1, N2S_BYTE); f(12345, 3, 1, 0, N2S_BYTE); f(12345, 2, 1, 1, N2S_BYTE); f(12345, 2, 1, 0, N2S_BYTE); f(12345, 1, 1, 1, N2S_BYTE); f(12345, 1, 1, 0, N2S_BYTE); printf("====================\n"); /* 1234567 */ f(1234567, 8, 1, 1, N2S_BYTE); f(1234567, 8, 1, 0, N2S_BYTE); f(1234567, 7, 1, 1, N2S_BYTE); f(1234567, 7, 1, 0, N2S_BYTE); f(1234567, 6, 1, 1, N2S_BYTE); f(1234567, 6, 1, 0, N2S_BYTE); f(1234567, 5, 1, 1, N2S_BYTE); f(1234567, 5, 1, 0, N2S_BYTE); f(1234567, 4, 1, 1, N2S_BYTE); f(1234567, 4, 1, 0, N2S_BYTE); f(1234567, 3, 1, 1, N2S_BYTE); f(1234567, 3, 1, 0, N2S_BYTE); f(1234567, 2, 1, 1, N2S_BYTE); f(1234567, 2, 1, 0, N2S_BYTE); f(1234567, 1, 1, 1, N2S_BYTE); f(1234567, 1, 1, 0, N2S_BYTE); printf("====================\n"); /* 1234567 with base 1024 */ f(1234567, 8, 1024, 1, N2S_BYTE); f(1234567, 8, 1024, 0, N2S_BYTE); f(1234567, 7, 1024, 1, N2S_BYTE); f(1234567, 7, 1024, 0, N2S_BYTE); f(1234567, 6, 1024, 1, N2S_BYTE); f(1234567, 6, 1024, 0, N2S_BYTE); f(1234567, 5, 1024, 1, N2S_BYTE); f(1234567, 5, 1024, 0, N2S_BYTE); f(1234567, 4, 1024, 1, N2S_BYTE); f(1234567, 4, 1024, 0, N2S_BYTE); f(1234567, 3, 1024, 1, N2S_BYTE); f(1234567, 3, 1024, 0, N2S_BYTE); f(1234567, 2, 1024, 1, N2S_BYTE); f(1234567, 2, 1024, 0, N2S_BYTE); f(1234567, 1, 1024, 1, N2S_BYTE); f(1234567, 1, 1024, 0, N2S_BYTE); printf("====================\n"); /* 1234567 with base 1024 with N2S_BIT */ f(1234567, 9, 1024, 1, N2S_BIT); f(1234567, 9, 1024, 0, N2S_BIT); f(1234567, 8, 1024, 1, N2S_BIT); f(1234567, 8, 1024, 0, N2S_BIT); f(1234567, 7, 1024, 1, N2S_BIT); f(1234567, 7, 1024, 0, N2S_BIT); f(1234567, 6, 1024, 1, N2S_BIT); f(1234567, 6, 1024, 0, N2S_BIT); f(1234567, 5, 1024, 1, N2S_BIT); f(1234567, 5, 1024, 0, N2S_BIT); f(1234567, 4, 1024, 1, N2S_BIT); f(1234567, 4, 1024, 0, N2S_BIT); f(1234567, 3, 1024, 1, N2S_BIT); f(1234567, 3, 1024, 0, N2S_BIT); f(1234567, 2, 1024, 1, N2S_BIT); f(1234567, 2, 1024, 0, N2S_BIT); f(1234567, 1, 1024, 1, N2S_BIT); f(1234567, 1, 1024, 0, N2S_BIT); printf("====================\n"); return 0; } # gcc -Wall -g -DCONFIG_STATIC_ASSERT ./testx.c ./lib/num2str.c # ./a.out 1 1B 1 1B ==================== 10 10B 10 10B ==================== 1000 1000B 1000 1000B 1000 1000B 1000 1000B 1000 0.0KiB 1000 1.0kB 1000 1KiB 1000 1kB 1000 1KiB 1000 1kB ==================== 1024 1024B 1024 1024B 1024 1024B 1024 1024B 1024 1.0KiB 1024 1.0kB 1024 1KiB 1024 1kB 1024 1KiB 1024 1kB ==================== 12345 12345B 12345 12345B 12345 12345B 12345 12345B 12345 12.1KiB 12345 12.3kB 12345 12KiB 12345 12kB 12345 12KiB 12345 12kB 12345 0MiB 12345 0MB ==================== 1234567 1234567B 1234567 1234567B 1234567 1234567B 1234567 1234567B 1234567 1205.6KiB 1234567 1234.6kB 1234567 1206KiB 1234567 1235kB 1234567 1206KiB 1234567 1235kB 1234567 1.2MiB 1234567 1.2MB 1234567 1MiB 1234567 1MB 1234567 1MiB 1234567 1MB ==================== 1234567 1234567KiB 1234567 1234567kB 1234567 1234567KiB 1234567 1234567kB 1234567 1205.6MiB 1234567 1234.6MB 1234567 1206MiB 1234567 1235MB 1234567 1206MiB 1234567 1235MB 1234567 1.2GiB 1234567 1.2GB 1234567 1GiB 1234567 1GB 1234567 1GiB 1234567 1GB ==================== 1234567 9876536Kibit 1234567 9876536kbit 1234567 9876536Kibit 1234567 9876536kbit 1234567 9876536Kibit 1234567 9876536kbit 1234567 9645.1Mibit 1234567 9876.5Mbit 1234567 9645Mibit 1234567 9877Mbit 1234567 9645Mibit 1234567 9877Mbit 1234567 9.4Gibit 1234567 9.9Gbit 1234567 9Gibit 1234567 10Gbit 1234567 9Gibit 1234567 10Gbit ==================== Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com> --- lib/num2str.c | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/lib/num2str.c b/lib/num2str.c index 448d3ff..8d08841 100644 --- a/lib/num2str.c +++ b/lib/num2str.c @@ -10,7 +10,7 @@ /** * num2str() - Cheesy number->string conversion, complete with carry rounding error. * @num: quantity (e.g., number of blocks, bytes or bits) - * @maxlen: max number of digits in the output string (not counting prefix and units) + * @maxlen: max number of digits in the output string (not counting prefix and units, but counting .) * @base: multiplier for num (e.g., if num represents Ki, use 1024) * @pow2: select unit prefix - 0=power-of-10 decimal SI, nonzero=power-of-2 binary IEC * @units: select units - N2S_* macros defined in num2str.h @@ -23,9 +23,9 @@ char *num2str(uint64_t num, int maxlen, int base, int pow2, int units) const char **unitprefix; const char *unitstr[] = { "", "/s", "B", "bit", "B/s", "bit/s" }; const unsigned int thousand[] = { 1000, 1024 }; - unsigned int modulo, decimals; + unsigned int modulo; int unit_index = 0, post_index, carry = 0; - char tmp[32]; + char tmp[32], fmt[32]; char *buf; compiletime_assert(sizeof(sistr) == sizeof(iecstr), "unit prefix arrays must be identical sizes"); @@ -62,6 +62,9 @@ char *num2str(uint64_t num, int maxlen, int base, int pow2, int units) break; } + /* + * Divide by K/Ki until string length of num <= maxlen. + */ modulo = -1U; while (post_index < sizeof(sistr)) { sprintf(tmp, "%llu", (unsigned long long) num); @@ -74,6 +77,9 @@ char *num2str(uint64_t num, int maxlen, int base, int pow2, int units) post_index++; } + /* + * If no modulo, then we're done. + */ if (modulo == -1U) { done: if (post_index >= ARRAY_SIZE(sistr)) @@ -84,23 +90,25 @@ done: return buf; } + /* + * If no room for decimals, then we're done. + */ sprintf(tmp, "%llu", (unsigned long long) num); - decimals = maxlen - strlen(tmp); - if ((int)decimals <= 1) { + if ((int)(maxlen - strlen(tmp)) <= 1) { if (carry) num++; goto done; } - do { - sprintf(tmp, "%u", modulo); - if (strlen(tmp) <= decimals - 1) - break; - - modulo = (modulo + 9) / 10; - } while (1); + /* + * Fill in everything and return the result. + */ + assert(maxlen - strlen(tmp) - 1 > 0); + assert(modulo < thousand[!!pow2]); + sprintf(fmt, "%%.%df", (int)(maxlen - strlen(tmp) - 1)); + sprintf(tmp, fmt, (double)modulo / (double)thousand[!!pow2]); - sprintf(buf, "%llu.%u%s%s", (unsigned long long) num, modulo, + sprintf(buf, "%llu.%s%s%s", (unsigned long long) num, &tmp[2], unitprefix[post_index], unitstr[unit_index]); return buf; } -- 2.9.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* RE: [PATCH 8/8] Fix num2str() output when modulo != -1U 2017-04-04 20:22 ` [PATCH 8/8] Fix num2str() output when modulo != -1U kusumi.tomohiro @ 2017-04-04 20:46 ` Elliott, Robert (Persistent Memory) 0 siblings, 0 replies; 11+ messages in thread From: Elliott, Robert (Persistent Memory) @ 2017-04-04 20:46 UTC (permalink / raw) To: kusumi.tomohiro, axboe, fio; +Cc: Tomohiro Kusumi > -----Original Message----- > From: fio-owner@vger.kernel.org [mailto:fio-owner@vger.kernel.org] On > Behalf Of kusumi.tomohiro@gmail.com > Sent: Tuesday, April 4, 2017 3:22 PM > Subject: [PATCH 8/8] Fix num2str() output when modulo != -1U ... > @@ -62,6 +62,9 @@ char *num2str(uint64_t num, int maxlen, int base, int > pow2, int units) > break; ... > + sprintf(fmt, "%%.%df", (int)(maxlen - strlen(tmp) - 1)); > + sprintf(tmp, fmt, (double)modulo / (double)thousand[!!pow2]); I suspect one of the goals of that function was to restrict itself to integer math and avoid invoking the floating point unit (which might not even exist on some CPUs). ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/8] Don't silently terminate td when no I/O performed due to error 2017-04-04 20:22 [PATCH 1/8] Don't silently terminate td when no I/O performed due to error kusumi.tomohiro ` (6 preceding siblings ...) 2017-04-04 20:22 ` [PATCH 8/8] Fix num2str() output when modulo != -1U kusumi.tomohiro @ 2017-04-08 17:05 ` Jens Axboe 7 siblings, 0 replies; 11+ messages in thread From: Jens Axboe @ 2017-04-08 17:05 UTC (permalink / raw) To: kusumi.tomohiro; +Cc: fio, Tomohiro Kusumi On Tue, Apr 04 2017, kusumi.tomohiro@gmail.com wrote: > From: Tomohiro Kusumi <tkusumi@tuxera.com> > > Some runtime configurations can cause threads/processes to terminate > without any I/O performed, yet with no explicit error message, which > is quite confusing. In the example below, fio finishes with neither > error nor regular statistics due to file offset + bs being larger > than the file size while in get_io_u(). > > This commit calls log_err() when this happens. Since it's difficult > to tell the exact reason after thread has left the main I/O loop[*], > it gives advice to use a command line option --debug=io, similar to > the way td_io_init() gives advice. > > [*] It can't just replace dprint(FD_IO, ...) calls with td_verror() > for e.g. while in get_io_u(), since those are also used by non error > paths too. Thanks, applied 1-2, and 4-7. -- Jens Axboe ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-04-08 17:05 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-04 20:22 [PATCH 1/8] Don't silently terminate td when no I/O performed due to error kusumi.tomohiro 2017-04-04 20:22 ` [PATCH 2/8] dump_td_info() doesn't really need to be a function kusumi.tomohiro 2017-04-04 20:22 ` [PATCH 3/8] Add assert(0) to DDIR_DATASYNC sync path if fdatasync(2) is unsupported kusumi.tomohiro 2017-04-08 17:02 ` Jens Axboe 2017-04-04 20:22 ` [PATCH 4/8] Make lib/prio_tree.c a stand-alone library kusumi.tomohiro 2017-04-04 20:22 ` [PATCH 5/8] Make lib/memalign.c " kusumi.tomohiro 2017-04-04 20:22 ` [PATCH 6/8] Make lib/num2str.c a stand-alone library by adding lib/num2str.h kusumi.tomohiro 2017-04-04 20:22 ` [PATCH 7/8] Fix num2str() output when maxlen <= strlen(tmp) kusumi.tomohiro 2017-04-04 20:22 ` [PATCH 8/8] Fix num2str() output when modulo != -1U kusumi.tomohiro 2017-04-04 20:46 ` Elliott, Robert (Persistent Memory) 2017-04-08 17:05 ` [PATCH 1/8] Don't silently terminate td when no I/O performed due to error 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.