From f5204b8af34dfda32fd6fec768c97d6c17ed3da4 Mon Sep 17 00:00:00 2001 From: aggieNick02 Date: Sun, 13 Feb 2022 21:42:27 -0600 Subject: [PATCH] Cleanup __check_min_rate This is a cleanup of __check_min_rate. In looking at stuff for previous fixes, it seems like there are a lot of boolean checks of things that are always true or always false. I'll explain my reasoning for each change; it is possible I'm missing something somehow but I've run through it a few times. Here's my logic: 1) td->rate_bytes and td->rate_blocks are 0 on first call to __check_min_rate, and then are the previous iteration's value of td->this_io_bytes and td->this_io_blocks on subsequent calls 2) bytes and iops are the current iteration's values of td->this_io_bytes and td->this_io_blocks 3) The values of td->this_io_bytes and td->this_io_blocks are monotonic with respect to each call of __check_min_rate Therefore, bytes and iops are always greater than or equal to td->rate_bytes and td->rate_blocks. This means the "if (bytes < td->rate_bytes[ddir]) {" on line 176 can never happen. Now, I want to say the same thing about line 197, but that line is weird/wrong in another way. rate_iops is td->o.rate_iops, the specified desired iops rate from the job. So I believe that is a bug - the specified desired iops rate should not even be examined in this function, just like the same is true for the desired bytes rate. I'm pretty sure what is meant is to compare iops to td->rate_blocks just like bytes is compared to td->rate_bytes in line 176, which would similarly always be false. Now we can focus on the else caluses (lines 180-192 and lines 202-213). If spent is 0, we should just be returning false early like in 169-170, so let's move that case up with it. The "if (rate < ratemin || bytes < td->rate_bytes[ddir]) {" and "if (rate < rate_iops_min || iops < td->rate_blocks[ddir]) {" both have impossibilities as the second part of the or clause. All we really want is to compare computed bytes rate to ratemin, and computed iops rate to rate_iops_min. With all of that, this function becomes a lot simpler. The rest of the cleanup is renaming of variables to make what they are clearer, and some other simple things (like initializing the variables directly instead of initializing to zero and then doing +=). The renames are as follows: - td->lastrate to td->last_rate_check_time, the last time a min rate check was performed - bytes to current_rate_check_bytes, the number of bytes transferred so far at the time this call to __check_min_rate was made - iops to current_rate_check_blocks, the number of blocks transferred so far at the time this call to __check_min_rate was made - rate to current_rate_bytes or current_rate_iops, depending on if it is used as the current cycle's byte rate or block rate - ratemin to option_rate_bytes_min, the user supplied desired minimum bytes rate - rate_iops eliminated - should not be used in this function - rate_iops_min to option_rate_iops_min, the user supplied desired minimum block rate - td->rate_bytes to td->last_rate_check_bytes - the number of bytes transferred the *last* time a minimum rate check was called *and* passed (not shortcircuited because not enough time had elapsed for the cycle or settling) - td->rate_blocks to td->last_rate_check_blocks - the number of blocks transferred the *last* time a minimum rate check was called *and* passed (not shortcircuited because not enough time had elapsed for the cycle or settling) Signed-off-by: Nick Neumann nick@pcpartpicker.com --- backend.c | 81 +++++++++++++++++++------------------------------------ fio.h | 6 ++--- libfio.c | 4 +-- 3 files changed, 32 insertions(+), 59 deletions(-) diff --git a/backend.c b/backend.c index c035baed..a21dfef6 100644 --- a/backend.c +++ b/backend.c @@ -136,13 +136,10 @@ static void set_sig_handlers(void) static bool __check_min_rate(struct thread_data *td, struct timespec *now, enum fio_ddir ddir) { - unsigned long long bytes = 0; - unsigned long iops = 0; - unsigned long spent; - unsigned long long rate; - unsigned long long ratemin = 0; - unsigned int rate_iops = 0; - unsigned int rate_iops_min = 0; + unsigned long long current_rate_check_bytes = td->this_io_bytes[ddir]; + unsigned long current_rate_check_blocks = td->this_io_blocks[ddir]; + unsigned long long option_rate_bytes_min = td->o.ratemin[ddir]; + unsigned int option_rate_iops_min = td->o.rate_iops_min[ddir]; assert(ddir_rw(ddir)); @@ -155,68 +152,44 @@ static bool __check_min_rate(struct thread_data *td, struct timespec *now, if (mtime_since(&td->start, now) < 2000) return false; - iops += td->this_io_blocks[ddir]; - bytes += td->this_io_bytes[ddir]; - ratemin += td->o.ratemin[ddir]; - rate_iops += td->o.rate_iops[ddir]; - rate_iops_min += td->o.rate_iops_min[ddir]; - /* - * if rate blocks is set, sample is running + * if last_rate_check_blocks or last_rate_check_bytes is set, + * we can compute a rate per ratecycle */ - if (td->rate_bytes[ddir] || td->rate_blocks[ddir]) { - spent = mtime_since(&td->lastrate[ddir], now); - if (spent < td->o.ratecycle) + if (td->last_rate_check_bytes[ddir] || td->last_rate_check_blocks[ddir]) { + unsigned long spent = mtime_since(&td->last_rate_check_time[ddir], now); + if (spent < td->o.ratecycle || spent==0) return false; - if (td->o.rate[ddir] || td->o.ratemin[ddir]) { + if (td->o.ratemin[ddir]) { /* * check bandwidth specified rate */ - if (bytes < td->rate_bytes[ddir]) { - log_err("%s: rate_min=%lluB/s not met, only transferred %lluB\n", - td->o.name, ratemin, bytes); + unsigned long long current_rate_bytes = + ((current_rate_check_bytes - td->last_rate_check_bytes[ddir]) * 1000) / spent; + if (current_rate_bytes < option_rate_bytes_min) { + log_err("%s: rate_min=%lluB/s not met, got %lluB/s\n", + td->o.name, option_rate_bytes_min, current_rate_bytes); return true; - } else { - if (spent) - rate = ((bytes - td->rate_bytes[ddir]) * 1000) / spent; - else - rate = 0; - - if (rate < ratemin || - bytes < td->rate_bytes[ddir]) { - log_err("%s: rate_min=%lluB/s not met, got %lluB/s\n", - td->o.name, ratemin, rate); - return true; - } } } else { /* * checks iops specified rate */ - if (iops < rate_iops) { - log_err("%s: rate_iops_min=%u not met, only performed %lu IOs\n", - td->o.name, rate_iops, iops); + unsigned long long current_rate_iops = + ((current_rate_check_blocks - td->last_rate_check_blocks[ddir]) * 1000) / spent; + + if (current_rate_iops < option_rate_iops_min) { + log_err("%s: rate_iops_min=%u not met, got %llu IOPS\n", + td->o.name, option_rate_iops_min, current_rate_iops); return true; - } else { - if (spent) - rate = ((iops - td->rate_blocks[ddir]) * 1000) / spent; - else - rate = 0; - - if (rate < rate_iops_min || - iops < td->rate_blocks[ddir]) { - log_err("%s: rate_iops_min=%u not met, got %llu IOPS\n", - td->o.name, rate_iops_min, rate); - return true; - } } } } - td->rate_bytes[ddir] = bytes; - td->rate_blocks[ddir] = iops; - memcpy(&td->lastrate[ddir], now, sizeof(*now)); + td->last_rate_check_bytes[ddir] = current_rate_check_bytes; + td->last_rate_check_blocks[ddir] = current_rate_check_blocks; + memcpy(&td->last_rate_check_time[ddir], now, sizeof(*now)); return false; } @@ -1845,11 +1818,11 @@ static void *thread_main(void *data) if (o->ratemin[DDIR_READ] || o->ratemin[DDIR_WRITE] || o->ratemin[DDIR_TRIM]) { - memcpy(&td->lastrate[DDIR_READ], &td->bw_sample_time, + memcpy(&td->last_rate_check_time[DDIR_READ], &td->bw_sample_time, sizeof(td->bw_sample_time)); - memcpy(&td->lastrate[DDIR_WRITE], &td->bw_sample_time, + memcpy(&td->last_rate_check_time[DDIR_WRITE], &td->bw_sample_time, sizeof(td->bw_sample_time)); - memcpy(&td->lastrate[DDIR_TRIM], &td->bw_sample_time, + memcpy(&td->last_rate_check_time[DDIR_TRIM], &td->bw_sample_time, sizeof(td->bw_sample_time)); } diff --git a/fio.h b/fio.h index 7b0ca843..88df117d 100644 --- a/fio.h +++ b/fio.h @@ -335,10 +335,10 @@ struct thread_data { */ uint64_t rate_bps[DDIR_RWDIR_CNT]; uint64_t rate_next_io_time[DDIR_RWDIR_CNT]; - unsigned long long rate_bytes[DDIR_RWDIR_CNT]; - unsigned long rate_blocks[DDIR_RWDIR_CNT]; + unsigned long long last_rate_check_bytes[DDIR_RWDIR_CNT]; + unsigned long last_rate_check_blocks[DDIR_RWDIR_CNT]; unsigned long long rate_io_issue_bytes[DDIR_RWDIR_CNT]; - struct timespec lastrate[DDIR_RWDIR_CNT]; + struct timespec last_rate_check_time[DDIR_RWDIR_CNT]; int64_t last_usec[DDIR_RWDIR_CNT]; struct frand_state poisson_state[DDIR_RWDIR_CNT]; diff --git a/libfio.c b/libfio.c index 01fa7452..1a891776 100644 --- a/libfio.c +++ b/libfio.c @@ -87,8 +87,8 @@ static void reset_io_counters(struct thread_data *td, int all) td->this_io_bytes[ddir] = 0; td->stat_io_blocks[ddir] = 0; td->this_io_blocks[ddir] = 0; - td->rate_bytes[ddir] = 0; - td->rate_blocks[ddir] = 0; + td->last_rate_check_bytes[ddir] = 0; + td->last_rate_check_blocks[ddir] = 0; td->bytes_done[ddir] = 0; td->rate_io_issue_bytes[ddir] = 0; td->rate_next_io_time[ddir] = 0; -- 2.25.1